Refuse an OAuth parameter that arrives as a list where one value is meant

app/controllers/oauth/authorizations_controller.rb

@@ -5,6 +5,8 @@ class Oauth::AuthorizationsController < Oauth::BaseController
   # An S256 challenge is the unpadded base64url SHA-256 of the verifier: 43 characters (RFC 7636 §4.2).
   S256_CODE_CHALLENGE = /\A[A-Za-z0-9_-]{43}\z/
   RETURN_URL_MAXIMUM_LENGTH = 2000
+  # Read to find the client and the redirect target, so refused before either.
+  LOOKUP_PARAMETERS = %w[ client_id redirect_uri ]
 
   # Browsers apply form-action to every hop of a form submission, redirects
   # included, so the app-wide form-action 'self' would block the consent POST's
@@ -16,12 +18,14 @@ class Oauth::AuthorizationsController < Oauth::BaseController
   # Gated ahead of sign-in so a dark server never asks anyone to sign in for
   # it. A denial is never gated: it only tells the client no.
   before_action :require_issuance_enabled, unless: :denial?
+  before_action :require_single_valued_parameters
   before_action :save_oauth_return_url
   before_action :require_authentication
   disallow_oauth_grants
 
   before_action :set_client
   before_action :validate_redirect_uri
+  before_action :validate_single_valued_parameters
   before_action :validate_response_type
   before_action :validate_pkce
   before_action :validate_scope
@@ -128,12 +132,16 @@ def redirect_with_error(error, description)
       if params[:redirect_uri].present? && @client&.allows_redirect?(params[:redirect_uri]) && @client.vetted_redirect?(params[:redirect_uri])
         redirect_to error_redirect_uri(error, description), allow_other_host: true
       else
-        @error = error
-        @error_description = description
-        render :error, status: :bad_request
+        render_error error, description
       end
     end
 
+    def render_error(error, description)
+      @error = error
+      @error_description = description
+      render :error, status: :bad_request
+    end
+
     def success_redirect_uri(code)
       build_redirect_uri params[:redirect_uri],
         code: code,
@@ -169,6 +177,25 @@ def validated_redirect_form_action_source
       nil
     end
 
+    # client_id and redirect_uri are refused before the client is looked up,
+    # and shown here, never redirected: a malformed redirect_uri is exactly the
+    # one that can't be validated.
+    def require_single_valued_parameters
+      if (malformed = malformed_parameter_names & LOOKUP_PARAMETERS).any?
+        render_error "invalid_request", malformed_parameters_description(malformed)
+      end
+    end
+
+    # The rest are refused once the redirect target is known, so they go back
+    # to a vetted client like any other error.
+    def validate_single_valued_parameters
+      malformed = malformed_parameter_names
+
+      if !performed? && malformed.any?
+        redirect_with_error "invalid_request", malformed_parameters_description(malformed)
+      end
+    end
+
     def build_redirect_uri(base, **query_params)
       uri = URI.parse(base)
       query = URI.decode_www_form(uri.query || "")

app/controllers/oauth/base_controller.rb

@@ -9,7 +9,56 @@ class Oauth::BaseController < ApplicationController
   # a halted chain skips after_actions (RFC 6749 §5.1, §5.2).
   prepend_before_action :prevent_caching, :prevent_referrer
 
+  # OAuth parameters are top-level names. Rails' JSON parameter wrapping would
+  # also copy a JSON body under the controller's name, and for the token
+  # endpoint that's `token`, an OAuth parameter of its own.
+  wrap_parameters false
+
+  # OAuth parameters that name one value (RFC 6749 §3.1: "Request and
+  # response parameters MUST NOT be included more than once"). Rails turns a
+  # name[]= form field or a JSON array or object into an Array or a nested
+  # Parameters, and handed to a lookup such as find_by(client_id: ...), an
+  # Array becomes an SQL IN list. Each endpoint refuses that shape right after
+  # its availability gate, so a dark server still answers 404, and before it
+  # reads anything. The authorization endpoint refuses client_id and
+  # redirect_uri that early, and the rest once its redirect target is known. client_secret isn't here: Oauth::ClientAuthentication reads
+  # it as a credential, and one that isn't a string fails authentication
+  # (invalid_client) like any wrong secret.
+  SINGLE_VALUED_PARAMETERS = %w[
+    client_id redirect_uri scope state
+    code code_verifier code_challenge code_challenge_method response_type
+    grant_type refresh_token token token_type_hint
+    resource subject_token subject_token_type requested_token_type audience account_id
+  ].freeze
+
   private
+    def require_single_valued_parameters
+      if (malformed = malformed_parameter_names).any?
+        refuse_malformed_parameters malformed
+      end
+    end
+
+    # A JSON null is left to each endpoint, which treats it as absent or, for a
+    # refresh's scope, as naming no scope.
+    def malformed_parameter_names
+      (SINGLE_VALUED_PARAMETERS - list_valued_parameter_names).select do |name|
+        params.key?(name) && !params[name].nil? && !params[name].is_a?(String)
+      end
+    end
+
+    # Parameters an endpoint takes as a list, exempt from the check above.
+    def list_valued_parameter_names
+      []
+    end
+
+    def refuse_malformed_parameters(names)
+      oauth_error "invalid_request", malformed_parameters_description(names)
+    end
+
+    def malformed_parameters_description(names)
+      "#{names.to_sentence} must #{names.one? ? "be" : "each be"} a single string"
+    end
+
     # Discovery follows acceptance (see Oauth::Availability): a dark server
     # answers as if it had no authorization server at all.
     def require_acceptance_enabled

app/controllers/oauth/clients_controller.rb

@@ -5,6 +5,7 @@ class Oauth::ClientsController < Oauth::BaseController
   rate_limit to: 10, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
 
   before_action :require_issuance_enabled
+  before_action :require_single_valued_parameters
   before_action :validate_redirect_uris
   before_action :validate_redirect_uri_origins
   before_action :validate_auth_method
@@ -29,6 +30,16 @@ def piloting_client_id
       nil
     end
 
+    # Registration metadata may give scope as a list (validated_scopes), and
+    # its other lists, redirect_uris and grant_types, aren't single-valued names.
+    def list_valued_parameter_names
+      %w[ scope ]
+    end
+
+    def refuse_malformed_parameters(names)
+      oauth_error "invalid_client_metadata", malformed_parameters_description(names)
+    end
+
     # Bounded before any URI is parsed, so an oversized list costs nothing more
     # than reading the request.
     def validate_redirect_uris

app/controllers/oauth/revocations_controller.rb

@@ -21,6 +21,7 @@ class Oauth::RevocationsController < Oauth::BaseController
   # throttled like the token endpoint (RFC 6749 §2.3.1, RFC 7009 §5).
   rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
 
+  before_action :require_single_valued_parameters
   before_action :reject_ambiguous_client_credentials
   before_action :require_token
 

app/controllers/oauth/tokens_controller.rb

@@ -7,6 +7,7 @@ class Oauth::TokensController < Oauth::BaseController
   rate_limit to: 20, within: 1.minute, only: :create, with: :oauth_rate_limit_exceeded
 
   before_action :require_issuance_enabled
+  before_action :require_single_valued_parameters
   before_action :validate_grant_type
 
   # Authenticate the client the request names before looking anything up about

test/integration/oauth_single_valued_parameters_test.rb

@@ -0,0 +1,166 @@
+require "test_helper"
+
+# RFC 6749 §3.1: request parameters MUST NOT be included more than once. Rails
+# parses a name[]= form field or a JSON array or object into an Array or a
+# nested Parameters, so each endpoint refuses that shape before any lookup.
+class OauthSingleValuedParametersTest < ActionDispatch::IntegrationTest
+  CHALLENGE = "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM"
+
+  setup do
+    @client = oauth_clients(:mcp_client)
+  end
+
+  # Authorization endpoint
+
+  test "an array client_id is refused before it becomes an IN list" do
+    sign_in_as :david
+
+    assert_no_queries_match(/oauth_clients/) do
+      untenanted { get new_oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) }
+    end
+
+    assert_response :bad_request
+    assert_select "code", text: "invalid_request"
+    assert_match "client_id", response.body
+  end
+
+  test "an array redirect_uri is shown an error, never redirected to" do
+    sign_in_as :david
+
+    untenanted { get new_oauth_authorization_path, params: authorize_params(redirect_uri: [ "http://127.0.0.1:8888/callback" ]) }
+
+    assert_response :bad_request
+    assert_select "code", text: "invalid_request"
+    assert_nil response.location
+  end
+
+  test "an array or object in any other authorization parameter goes back to a vetted client as invalid_request" do
+    sign_in_as :david
+
+    { scope: [ "read" ], response_type: [ "code" ], code_challenge: [ CHALLENGE ], code_challenge_method: [ "S256" ], state: { a: "b" } }.each do |name, value|
+      untenanted { get new_oauth_authorization_path, params: authorize_params(name => value) }
+
+      assert_response :redirect, name.to_s
+      query = CGI.parse(URI.parse(response.location).query)
+      assert_equal "invalid_request", query["error"].first, name.to_s
+      assert_match name.to_s, query["error_description"].first
+      assert_not query.key?("code")
+    end
+  end
+
+  test "an array in another authorization parameter from an unvetted client is shown here" do
+    sign_in_as :david
+    @client = Oauth::Client.create!(name: "Hosted", redirect_uris: %w[ https://connector.example.com/callback ], dynamically_registered: true)
+
+    untenanted { get new_oauth_authorization_path, params: authorize_params(redirect_uri: "https://connector.example.com/callback", scope: [ "read" ]) }
+
+    assert_response :bad_request
+    assert_select "code", text: "invalid_request"
+  end
+
+  test "the consent POST refuses an array client_id" do
+    sign_in_as :david
+
+    assert_no_queries_match(/oauth_clients/) do
+      untenanted { post oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) }
+    end
+
+    assert_response :bad_request
+    assert_select "code", text: "invalid_request"
+  end
+
+  test "a dark server still answers 404 to a malformed request" do
+    with_oauth_acceptance(false) do
+      untenanted { get new_oauth_authorization_path, params: authorize_params(client_id: [ @client.client_id ]) }
+    end
+
+    assert_response :not_found
+  end
+
+  # Token endpoint
+
+  test "the token endpoint refuses an array client_id before any lookup" do
+    assert_no_queries_match(/oauth_clients|identity_access_tokens/) do
+      untenanted do
+        post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: "fizzy_rt_x", client_id: [ @client.client_id ] }, as: :json
+      end
+    end
+
+    assert_response :bad_request
+    assert_equal "invalid_request", response.parsed_body["error"]
+  end
+
+  test "the token endpoint refuses an array or object in any single-valued parameter" do
+    [
+      { grant_type: [ "refresh_token" ], refresh_token: "fizzy_rt_x", client_id: @client.client_id },
+      { grant_type: "refresh_token", refresh_token: [ "fizzy_rt_x" ], client_id: @client.client_id },
+      { grant_type: "refresh_token", refresh_token: "fizzy_rt_x", client_id: @client.client_id, scope: [ "read" ] },
+      { grant_type: "authorization_code", code: { a: "b" }, code_verifier: "v", redirect_uri: "http://127.0.0.1:8888/callback", client_id: @client.client_id }
+    ].each do |body|
+      untenanted { post oauth_token_path, params: body, as: :json }
+
+      assert_response :bad_request, body.inspect
+      assert_equal "invalid_request", response.parsed_body["error"], body.inspect
+    end
+  end
+
+  test "a refresh's JSON null scope still means no scope, not a malformed one" do
+    untenanted do
+      post oauth_token_path, params: { grant_type: "refresh_token", refresh_token: "fizzy_rt_unknown", client_id: @client.client_id, scope: nil }, as: :json
+    end
+
+    assert_equal "invalid_grant", response.parsed_body["error"], response.body
+  end
+
+  # Revocation endpoint
+
+  test "revocation refuses an array client_id or token_type_hint before any lookup" do
+    [ { token: "x", client_id: [ @client.client_id ] }, { token: "x", token_type_hint: [ "access_token" ] } ].each do |body|
+      assert_no_queries_match(/oauth_clients|identity_access_tokens/) do
+        untenanted { post oauth_revocation_path, params: body, as: :json }
+      end
+
+      assert_response :bad_request, body.inspect
+      assert_equal "invalid_request", response.parsed_body["error"], body.inspect
+    end
+  end
+
+  # Dynamic client registration
+
+  test "registration refuses an array in a single-valued parameter, and still takes scope as a list" do
+    untenanted do
+      post oauth_clients_path, params: { redirect_uris: [ "http://127.0.0.1:9999/cb" ], client_id: [ "chosen" ] }, as: :json
+    end
+
+    assert_response :bad_request
+    assert_equal "invalid_client_metadata", response.parsed_body["error"]
+
+    untenanted do
+      post oauth_clients_path, params: { redirect_uris: [ "http://127.0.0.1:9999/cb" ], scope: [ "read" ] }, as: :json
+    end
+
+    assert_response :created
+    assert_equal "read", response.parsed_body["scope"]
+  end
+
+  private
+    def authorize_params(**overrides)
+      {
+        client_id: @client.client_id,
+        redirect_uri: "http://127.0.0.1:8888/callback",
+        response_type: "code",
+        code_challenge: CHALLENGE,
+        code_challenge_method: "S256",
+        scope: "read",
+        state: "xyz"
+      }.merge(overrides)
+    end
+
+    def with_oauth_acceptance(enabled)
+      previous = Rails.configuration.x.oauth.acceptance_enabled
+      Rails.configuration.x.oauth.acceptance_enabled = enabled
+      yield
+    ensure
+      Rails.configuration.x.oauth.acceptance_enabled = previous
+    end
+end