Merge pull request #431 from koic/iss_parameter_validation

Validate the RFC 9207 `iss` Authorization Response Parameter per SEP-2468

README.md

@@ -2348,7 +2348,9 @@ Required keyword arguments to `Provider.new`:
   an explicit value always wins.
 - `redirect_uri`: String. Must use HTTPS or be a loopback URL (`localhost`, `127.0.0.0/8`, `::1`); other values raise `Provider::InsecureRedirectURIError`.
 - `redirect_handler`: Callable invoked with the fully-built authorization `URI`. Typically opens the user's browser.
-- `callback_handler`: Callable that returns `[code, state]` after the user is redirected back to `redirect_uri`.
+- `callback_handler`: Callable that returns `[code, state]` or `[code, state, iss]` after the user is redirected back to `redirect_uri`. Returning the 3-element form
+  (with `iss` set to the RFC 9207 `iss` parameter from the redirect, or `nil` when absent) opts into SEP-2468 issuer validation: a present `iss` must match
+  the authorization server's issuer, and a missing one is rejected when the server advertises `authorization_response_iss_parameter_supported`.
 
 Optional keyword arguments:
 

conformance/client.rb

@@ -95,7 +95,9 @@ def build_oauth_provider(context, scenario:)
 
   callback_handler = -> do
     query = URI.decode_www_form(callback_holder.fetch(:url).query).to_h
-    [query["code"], query["state"]]
+    # The 3-element form opts into SEP-2468 / RFC 9207 `iss` validation;
+    # `query["iss"]` is nil when the authorization response carried no `iss`.
+    [query["code"], query["state"], query["iss"]]
   end
 
   MCP::Client::OAuth::Provider.new(

lib/mcp/client/oauth/flow.rb

@@ -74,13 +74,20 @@ module MCP
           )
 
           @provider.redirect_handler.call(authorization_url)
-          code, returned_state = Array(@provider.callback_handler.call)
+          callback_result = Array(@provider.callback_handler.call)
+          code, returned_state, returned_iss = callback_result
           raise AuthorizationError, "Authorization callback did not return an authorization code." unless code
 
           unless states_match?(returned_state, state)
             raise AuthorizationError, "OAuth state mismatch (CSRF protection)."
           end
 
+          validate_authorization_response_issuer!(
+            as_metadata: as_metadata,
+            iss: returned_iss,
+            iss_provided: callback_result.length >= 3,
+          )
+
           tokens = exchange_authorization_code(
             as_metadata: as_metadata,
             client_info: client_info,
@@ -534,6 +541,43 @@ module MCP
           raise AuthorizationError, "#{label} #{url.inspect} is not a valid URI: #{e.message}."
         end
 
+        # Validates the RFC 9207 `iss` authorization response parameter against the issuer of the authorization server
+        # the flow is talking to, per SEP-2468 (mix-up attack mitigation in multi-IdP setups):
+        #
+        # - When the callback supplied a non-empty `iss`, it MUST equal the AS metadata `issuer` exactly (simple string comparison,
+        #   no normalization, per RFC 9207 Section 2.4); on mismatch the flow aborts before the authorization code is ever sent to
+        #   a token endpoint.
+        # - When the callback returned a 3-element `[code, state, iss]` whose `iss` is nil (asserting it inspected the authorization response
+        #   and found no `iss`) and the AS metadata advertises `authorization_response_iss_parameter_supported: true`, the missing parameter
+        #   is treated as an attack per RFC 9207 Section 2.4 and the flow aborts.
+        # - A legacy 2-element `[code, state]` callback skips the check: the caller never looked for `iss`, so its absence carries no signal.
+        #
+        # `as_metadata["issuer"]` has already been byte-compared against the discovery URL by `ensure_issuer_matches!`,
+        #  so it is the RFC 9207 anchor value. `iss` is not a secret; a plain `==` suffices.
+        #
+        # - https://github.com/modelcontextprotocol/modelcontextprotocol/pull/2468
+        # - https://www.rfc-editor.org/rfc/rfc9207
+        def validate_authorization_response_issuer!(as_metadata:, iss:, iss_provided:)
+          expected = as_metadata["issuer"]
+
+          if iss && !iss.to_s.empty?
+            return if iss.to_s == expected.to_s
+
+            raise AuthorizationError,
+              "Authorization response `iss` does not match the authorization server issuer " \
+                "(expected #{expected.inspect}, got #{iss.inspect}) (RFC 9207); " \
+                "refusing to exchange the authorization code."
+          end
+
+          return unless iss_provided
+          return unless as_metadata["authorization_response_iss_parameter_supported"] == true
+
+          raise AuthorizationError,
+            "Authorization server advertises `authorization_response_iss_parameter_supported` but " \
+              "the authorization response carried no `iss` parameter (RFC 9207 Section 2.4); " \
+              "refusing to exchange the authorization code."
+        end
+
         # Constant-time comparison for the OAuth `state` parameter to prevent timing-based discovery
         # of the expected value.
         # `OpenSSL.fixed_length_secure_compare` would be ideal, but it is not available on Ruby 2.7

lib/mcp/client/oauth/provider.rb

@@ -21,8 +21,13 @@ module MCP
       # - `redirect_handler` - Callable invoked with the fully-built authorization
       #   URL (a `URI`). Implementations typically open the user's browser.
       # - `callback_handler` - Callable invoked after `redirect_handler`. Returns
-      #   `[code, state]` where `code` is the authorization code and `state` is
-      #   the `state` parameter received on the redirect URI.
+      #   `[code, state]` or `[code, state, iss]`, where `code` is the authorization code,
+      #   `state` is the `state` parameter received on the redirect URI, and `iss` is
+      #   the RFC 9207 `iss` parameter (or nil when the authorization response carried none).
+      #   Returning the 3-element form opts into SEP-2468 issuer validation: a present `iss`
+      #   must match the authorization server's issuer, and a nil `iss` is
+      #   rejected when the AS advertises `authorization_response_iss_parameter_supported`.
+      #   The 2-element form skips the check for backward compatibility.
       #
       # Optional keyword arguments:
       # - `scope`   - String of space-separated scopes to request when the server's

test/mcp/client/oauth/flow_test.rb

@@ -985,6 +985,111 @@ module MCP
           assert_match(/client_id/i, error.message)
         end
 
+        # Builds a provider whose callback returns the given `iss` shape:
+        # `:omitted` keeps the legacy 2-element `[code, state]` form, any other value (including nil) returns
+        # the 3-element `[code, state, iss]` form that opts into SEP-2468 / RFC 9207 issuer validation.
+        def build_iss_validation_provider(callback_iss: :omitted)
+          state_holder = {}
+          callback_handler = if callback_iss == :omitted
+            -> { ["test-auth-code", state_holder[:state]] }
+          else
+            -> { ["test-auth-code", state_holder[:state], callback_iss] }
+          end
+
+          Provider.new(
+            client_metadata: {
+              redirect_uris: ["http://localhost:0/callback"],
+              grant_types: ["authorization_code"],
+              response_types: ["code"],
+              token_endpoint_auth_method: "none",
+            },
+            redirect_uri: "http://localhost:0/callback",
+            redirect_handler: ->(url) {
+              state_holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state")
+            },
+            callback_handler: callback_handler,
+          )
+        end
+
+        # Re-stubs the AS metadata with `authorization_response_iss_parameter_supported: true`.
+        def stub_as_metadata_with_iss_support
+          stub_request(:get, @as_metadata_url).to_return(
+            status: 200,
+            headers: { "Content-Type" => "application/json" },
+            body: JSON.generate(
+              issuer: @auth_base,
+              authorization_endpoint: "#{@auth_base}/authorize",
+              token_endpoint: "#{@auth_base}/token",
+              registration_endpoint: "#{@auth_base}/register",
+              response_types_supported: ["code"],
+              grant_types_supported: ["authorization_code"],
+              code_challenge_methods_supported: ["S256"],
+              token_endpoint_auth_methods_supported: ["none"],
+              authorization_response_iss_parameter_supported: true,
+            ),
+          )
+        end
+
+        def test_run_completes_when_authorization_response_iss_matches_issuer
+          provider = build_iss_validation_provider(callback_iss: @auth_base)
+
+          result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+
+          assert_equal(:authorized, result)
+          assert_equal("test-token-from-flow", provider.access_token)
+        end
+
+        def test_run_aborts_before_code_exchange_when_authorization_response_iss_mismatches
+          provider = build_iss_validation_provider(callback_iss: "https://evil.example.com")
+
+          error = assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+          end
+
+          # Per RFC 9207, the client MUST NOT send the authorization code to a token endpoint after an `iss` mismatch.
+          assert_match(/`iss` does not match/, error.message)
+          assert_not_requested(:post, "#{@auth_base}/token")
+        end
+
+        def test_run_aborts_when_iss_is_missing_but_as_advertises_iss_support
+          stub_as_metadata_with_iss_support
+          provider = build_iss_validation_provider(callback_iss: nil)
+
+          error = assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+          end
+
+          assert_match(/carried no `iss`/, error.message)
+          assert_not_requested(:post, "#{@auth_base}/token")
+        end
+
+        def test_run_completes_with_legacy_two_element_callback_even_when_as_advertises_iss_support
+          stub_as_metadata_with_iss_support
+          provider = build_iss_validation_provider
+
+          result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+
+          assert_equal(:authorized, result)
+        end
+
+        def test_run_completes_when_iss_is_missing_and_as_does_not_advertise_iss_support
+          provider = build_iss_validation_provider(callback_iss: nil)
+
+          result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+
+          assert_equal(:authorized, result)
+        end
+
+        def test_run_validates_present_iss_even_when_as_does_not_advertise_iss_support
+          provider = build_iss_validation_provider(callback_iss: "https://evil.example.com")
+
+          assert_raises(Flow::AuthorizationError) do
+            Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
+          end
+
+          assert_not_requested(:post, "#{@auth_base}/token")
+        end
+
         def test_run_skips_dcr_when_client_information_already_stored
           stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called."))