FIX: Preserve padding parameter through user API key authorization flow (#36640)

ZogStriP · web-flow · commit 3e577b6ba8a6 · 2025-12-12T13:56:21.000+01:00
Users/apps running OpenSSL in FIPS 140-3 mode cannot decrypt user API key responses because FIPS forbids the legacy PKCS1_PADDING scheme. Commit 300ece3 added support for a `padding=oaep` parameter to use PKCS1_OAEP_PADDING instead, but the parameter was only handled in the POST endpoints. When users go through the authorization UI flow (GET /user-api-key/new → login → POST /user-api-key), the padding parameter was lost because it wasn't captured in the controller or passed through the form's hidden fields. This fix: - Captures @padding in the #new and #otp controller actions - Adds hidden field for padding in new.html.erb and otp.html.erb - Removes unused ALLOWED_PADDING_MODES constant - Refactors specs to be more organized and concise Internal ref - t/170427
diff --git a/app/controllers/user_api_keys_controller.rb b/app/controllers/user_api_keys_controller.rb
@@ -10,7 +10,6 @@ class UserApiKeysController < ApplicationController
   skip_before_action :check_xhr, :preload_json
 
   AUTH_API_VERSION = 4
-
   ALLOWED_PADDING_MODES = %w[pkcs1 oaep].freeze
 
   def new
@@ -19,8 +18,9 @@ def new
       return
     end
 
-    find_client
     require_params
+    find_client
+    require_client_params
     validate_params
 
     unless current_user
@@ -47,23 +47,20 @@ def new
     @push_url = params[:push_url]
     @localized_scopes = params[:scopes].split(",").map { |s| I18n.t("user_api_key.scopes.#{s}") }
     @scopes = params[:scopes]
+    @padding = params[:padding]
   rescue Discourse::InvalidAccess
     @generic_error = true
   end
 
   def create
-    find_client
     require_params
-
-    if params.key?(:auth_redirect)
-      if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect], client: @client)
-        raise Discourse::InvalidAccess
-      end
-    end
+    find_client
+    require_client_params
+    validate_params
+    validate_auth_redirect
 
     raise Discourse::InvalidAccess unless meets_tl?
 
-    validate_params
     scopes = params[:scopes].split(",")
 
     @client = UserApiKeyClient.new(client_id: params[:client_id]) if @client.blank?
@@ -89,15 +86,12 @@ def create
       api: AUTH_API_VERSION,
     }.to_json
 
-    public_key_str = (@client.public_key.presence || params[:public_key])
-    public_key = OpenSSL::PKey::RSA.new(public_key_str)
-
-    validate_payload_size_for_oaep!(@payload, public_key)
-    @payload = Base64.encode64(rsa_encrypt(public_key, @payload))
+    validate_payload_size_for_oaep!(@payload, parsed_public_key)
+    @payload = Base64.encode64(rsa_encrypt(parsed_public_key, @payload))
 
     if scopes.include?("one_time_password")
       # encrypt one_time_password separately to bypass 128 chars encryption limit
-      otp_payload = one_time_password(public_key, current_user.username)
+      otp_payload = one_time_password(parsed_public_key, current_user.username)
     end
 
     if params[:auth_redirect]
@@ -123,6 +117,7 @@ def create
 
   def otp
     require_params_otp
+    validate_params_otp
 
     unless current_user
       cookies[:destination_url] = request.fullpath
@@ -138,30 +133,27 @@ def otp
     @application_name = params[:application_name]
     @public_key = params[:public_key]
     @auth_redirect = params[:auth_redirect]
+    @padding = params[:padding]
   end
 
   def create_otp
     require_params_otp
+    validate_params_otp
+    validate_auth_redirect
 
-    if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect])
-      raise Discourse::InvalidAccess
-    end
     raise Discourse::InvalidAccess unless meets_tl?
 
-    public_key = OpenSSL::PKey::RSA.new(params[:public_key])
-    otp_payload = one_time_password(public_key, current_user.username)
+    otp_payload = one_time_password(parsed_public_key, current_user.username)
 
     redirect_path = "#{params[:auth_redirect]}?oneTimePassword=#{CGI.escape(otp_payload)}"
     redirect_to(redirect_path, allow_other_host: true)
   end
 
   def revoke
-    revoke_key = find_key if params[:id]
+    current_key = request.env["HTTP_USER_API_KEY"]
 
-    if current_key = request.env["HTTP_USER_API_KEY"]
-      request_key = UserApiKey.with_key(current_key).first
-      revoke_key ||= request_key
-    end
+    revoke_key = find_key if params[:id]
+    revoke_key ||= UserApiKey.with_key(current_key).first if current_key.present?
 
     raise Discourse::NotFound unless revoke_key
 
@@ -187,6 +179,9 @@ def find_client
 
   def require_params
     %i[nonce scopes client_id].each { |p| params.require(p) }
+  end
+
+  def require_client_params
     params.require(:public_key) if @client&.public_key.blank?
     params.require(:application_name) if @client&.application_name.blank?
   end
@@ -198,14 +193,40 @@ def validate_params
       raise Discourse::InvalidAccess
     end
 
-    # our pk has got to parse
-    OpenSSL::PKey::RSA.new(params[:public_key]) if params[:public_key]
+    parsed_public_key if public_key_str.present?
+    validate_padding
   end
 
   def require_params_otp
     %i[public_key auth_redirect application_name].each { |p| params.require(p) }
   end
 
+  def validate_params_otp
+    parsed_public_key
+    validate_padding
+  end
+
+  def validate_padding
+    return if params[:padding].blank?
+    return if ALLOWED_PADDING_MODES.include?(params[:padding])
+    raise Discourse::InvalidParameters.new(:padding)
+  end
+
+  def validate_auth_redirect
+    return unless params.key?(:auth_redirect)
+    if UserApiKeyClient.invalid_auth_redirect?(params[:auth_redirect], client: @client)
+      raise Discourse::InvalidAccess
+    end
+  end
+
+  def public_key_str
+    @client&.public_key.presence || params[:public_key]
+  end
+
+  def parsed_public_key
+    @parsed_public_key ||= OpenSSL::PKey::RSA.new(public_key_str)
+  end
+
   def meets_tl?
     current_user.staff? || current_user.in_any_groups?(SiteSetting.user_api_key_allowed_groups_map)
   end
diff --git a/app/models/api_key.rb b/app/models/api_key.rb
@@ -10,12 +10,7 @@ class KeyAccessError < StandardError
 
   scope :active, -> { where("revoked_at IS NULL") }
   scope :revoked, -> { where.not(revoked_at: nil) }
-
-  scope :with_key,
-        ->(key) do
-          hashed = self.hash_key(key)
-          where(key_hash: hashed)
-        end
+  scope :with_key, ->(key) { where(key_hash: ApiKey.hash_key(key)) }
 
   validates :description, length: { maximum: 255 }
   validate :at_least_one_granular_scope
diff --git a/app/views/user_api_keys/new.html.erb b/app/views/user_api_keys/new.html.erb
@@ -28,6 +28,7 @@
   <%= hidden_field_tag 'push_url', @push_url %>
   <%= hidden_field_tag 'public_key', @public_key%>
   <%= hidden_field_tag 'scopes', @scopes%>
+  <%= hidden_field_tag('padding', @padding) if @padding %>
   <%= submit_tag t('user_api_key.authorize'), class: 'btn btn-primary' %>
 <% end %>
 </div>
diff --git a/app/views/user_api_keys/otp.html.erb b/app/views/user_api_keys/otp.html.erb
@@ -4,6 +4,7 @@
   <%= hidden_field_tag 'application_name', @application_name %>
   <%= hidden_field_tag 'public_key', @public_key%>
   <%= hidden_field_tag('auth_redirect', @auth_redirect) %>
+  <%= hidden_field_tag('padding', @padding) if @padding %>
   <%= submit_tag t('user_api_key.authorize'), class: 'btn btn-primary' %>
 <% end %>
 </div>
diff --git a/spec/requests/user_api_keys_controller_spec.rb b/spec/requests/user_api_keys_controller_spec.rb
