diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index 98d7607c..b90223d2 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -321,10 +321,12 @@ Keyword arguments: - `private_key`, `signing_algorithm`: Required with `private_key_jwt` - the key (a PEM string or `OpenSSL::PKey::PKey`, never written to `storage`) signs the client assertion with `"ES256"` or `"RS256"`; `client_secret` must not be set, because the private key is the credential. -- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to. A different authorization server raises `Flow::AuthorizationError`. +- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to, spelled as its metadata states it: + an `https` URL (`http` only on a loopback address) without query or fragment. A different authorization server raises `Flow::AuthorizationError`. A single trailing slash difference is accepted, as in the TypeScript SDK. When the protected resource metadata lists several authorization servers, - the one naming `issuer:` is used. Omitting `issuer:` is deprecated and emits a warning at construction. Without it, credentials go to whichever authorization server - the MCP server names, as before. To use credentials for another authorization server, construct a provider with that server's `issuer:` and credentials. + the one naming `issuer:` is used. Omitting `issuer:` is deprecated and emits a warning at construction. + Without it, credentials go to whichever authorization server the MCP server names, as before. To use credentials for another authorization server, + construct a provider with that server's `issuer:` and credentials. - `scope`, `storage`, `authorization_request_validator`, `token_request_params`, `http_client_customizer`: Optional, same meaning as on `Provider`. Use `token_request_params` for a parameter the authorization server requires on the `client_credentials` grant, such as Auth0's `audience`. diff --git a/lib/mcp/client/oauth/client_credentials_provider.rb b/lib/mcp/client/oauth/client_credentials_provider.rb index cb4ab430..a0f1eb08 100644 --- a/lib/mcp/client/oauth/client_credentials_provider.rb +++ b/lib/mcp/client/oauth/client_credentials_provider.rb @@ -35,7 +35,8 @@ module OAuth # the server's `token_endpoint_auth_signing_alg_values_supported` fails # loudly at construction instead of as a 401 (the TypeScript and Python SDKs # also take the algorithm as an explicit option). - # - `issuer` - String issuer identifier of the authorization server the credentials belong to. + # - `issuer` - String issuer identifier of the authorization server the credentials belong to, + # as its metadata states it: an `https` URL, or `http` on a loopback address, without query or fragment. # A different authorization server raises `Flow::AuthorizationError`. Omitting this is deprecated # and emits a warning; without it, credentials go to whichever authorization server the MCP server names. # - `scope` - String of space-separated scopes to request when the server's @@ -93,8 +94,9 @@ def initialize( "the MCP server names. Pass the authorization server's issuer URL as `issuer:` so they are only sent there.", uplevel: 1, ) - elsif !issuer.is_a?(String) || blank?(issuer) - raise InvalidCredentialsError, "issuer must be a non-empty String: the issuer identifier of the authorization server the credentials belong to." + elsif !Discovery.issuer_identifier?(issuer) + raise InvalidCredentialsError, "issuer must be the authorization server's issuer URL as its metadata states it, over HTTPS " \ + "(HTTP only on a loopback address) and without query or fragment, got #{issuer.inspect}." end client_information = { "client_id" => client_id, "token_endpoint_auth_method" => token_endpoint_auth_method } diff --git a/lib/mcp/client/oauth/cross_app_access_provider.rb b/lib/mcp/client/oauth/cross_app_access_provider.rb index 3f88eef0..44cdf7c1 100644 --- a/lib/mcp/client/oauth/cross_app_access_provider.rb +++ b/lib/mcp/client/oauth/cross_app_access_provider.rb @@ -21,9 +21,10 @@ module OAuth # # Optional keyword arguments: # - # - `issuer` - String issuer identifier of the MCP authorization server the credentials belong to. - # A mismatch raises `Flow::AuthorizationError` before `assertion_provider` is called. Omitting this is - # deprecated and emits a warning; without it, credentials go to whichever authorization server the MCP server names. + # - `issuer` - String issuer identifier of the MCP authorization server the credentials belong to, + # as its metadata states it: an `https` URL, or `http` on a loopback address, without query or fragment. + # A mismatch raises `Flow::AuthorizationError` before `assertion_provider` is called. Omitting this is deprecated + # and emits a warning; without it, credentials go to whichever authorization server the MCP server names. # - `scope` - String of space-separated scopes to request when the server's `WWW-Authenticate` and # the Protected Resource Metadata do not specify one. # - `storage` - Object responding to `tokens`, `save_tokens(tokens)`, `client_information`, and `save_client_information(info)`. @@ -74,8 +75,9 @@ def initialize( "the MCP server names. Pass the authorization server's issuer URL as `issuer:` so they are only sent there.", uplevel: 1, ) - elsif !issuer.is_a?(String) || blank?(issuer) - raise InvalidConfigurationError, "issuer must be a non-empty String: the issuer identifier of the authorization server the credentials belong to." + elsif !Discovery.issuer_identifier?(issuer) + raise InvalidConfigurationError, "issuer must be the authorization server's issuer URL as its metadata states it, over HTTPS " \ + "(HTTP only on a loopback address) and without query or fragment, got #{issuer.inspect}." end http_client_customizer = validated_http_client_customizer(http_client_customizer) diff --git a/lib/mcp/client/oauth/discovery.rb b/lib/mcp/client/oauth/discovery.rb index 1abc6c5b..c103b69c 100644 --- a/lib/mcp/client/oauth/discovery.rb +++ b/lib/mcp/client/oauth/discovery.rb @@ -200,6 +200,20 @@ def secure_url?(url) false end + # Whether `value` can be an authorization server issuer identifier the flow could ever match: a String naming an `https` URL, + # or `http` on a loopback address as `secure_url?` allows, without query or fragment (RFC 8414 Section 2). + # The issuer of the metadata is compared as a String, so a value of another type would never equal it, and an issuer with + # a query or fragment is one no authorization server may publish. + def issuer_identifier?(value) + return false unless value.is_a?(String) && secure_url?(value) + + uri = URI.parse(value) + + uri.query.nil? && uri.fragment.nil? + rescue URI::InvalidURIError + false + end + # Returns true when `url` and `other` share an origin: same scheme, same host, # and same port. Scheme and host compare case-insensitively, and an implicit default port compares # equal to the same port written out, because `URI::HTTP#port` already resolves to the default. diff --git a/test/mcp/client/oauth/client_credentials_provider_test.rb b/test/mcp/client/oauth/client_credentials_provider_test.rb index ee83c365..708c1bad 100644 --- a/test/mcp/client/oauth/client_credentials_provider_test.rb +++ b/test/mcp/client/oauth/client_credentials_provider_test.rb @@ -58,11 +58,29 @@ def test_initialize_stores_the_issuer_with_the_credentials assert_equal("https://auth.example.com", provider.client_information["issuer"]) end - def test_initialize_rejects_an_issuer_that_is_not_a_non_empty_string - ["", " ", :issuer, false, 123].each do |issuer| - assert_raises(ClientCredentialsProvider::InvalidCredentialsError) do + def test_initialize_rejects_an_issuer_that_is_not_an_issuer_url + [ + "", " ", :issuer, false, 123, :"https://auth.example.com", URI("https://auth.example.com"), + "auth.example.com", "ftp://auth.example.com", "https://", "https://auth.example.com/ metadata", + "https://auth.example.com/?tenant=1", "https://auth.example.com/#section", + "http://auth.example.com", "http://10.0.0.5", "http://localhost.example.com", "http://127.attacker.example" + ].each do |issuer| + error = assert_raises(ClientCredentialsProvider::InvalidCredentialsError, issuer.inspect) do ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", issuer: issuer) end + + assert_match(/issuer must be the authorization server's issuer URL/, error.message) + end + end + + def test_initialize_accepts_an_https_issuer_and_a_loopback_http_issuer + [ + "https://auth.example.com", "https://auth.example.com/", "https://auth.example.com/tenant", "HTTPS://auth.example.com", + "http://127.0.0.1:9292", "http://localhost:8080/auth", "http://[::1]:9292" + ].each do |issuer| + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", issuer: issuer) + + assert_equal(issuer, provider.client_information["issuer"]) end end diff --git a/test/mcp/client/oauth/cross_app_access_provider_test.rb b/test/mcp/client/oauth/cross_app_access_provider_test.rb index 4354dfe6..3011472f 100644 --- a/test/mcp/client/oauth/cross_app_access_provider_test.rb +++ b/test/mcp/client/oauth/cross_app_access_provider_test.rb @@ -54,9 +54,13 @@ def test_initialize_stores_the_issuer_with_the_credentials assert_equal("https://auth.example.com", provider.client_information["issuer"]) end - def test_initialize_rejects_an_issuer_that_is_not_a_non_empty_string - ["", " ", :issuer, false, 123].each do |issuer| - assert_raises(CrossAppAccessProvider::InvalidConfigurationError) do + def test_initialize_rejects_an_issuer_that_is_not_an_issuer_url + [ + "", " ", :issuer, false, 123, URI("https://auth.example.com"), + "auth.example.com", "ftp://auth.example.com", "https://", "https://auth.example.com/?tenant=1", + "http://auth.example.com", "http://10.0.0.5" + ].each do |issuer| + error = assert_raises(CrossAppAccessProvider::InvalidConfigurationError, issuer.inspect) do CrossAppAccessProvider.new( client_id: "xaa-client", client_secret: "xaa-secret", @@ -64,6 +68,21 @@ def test_initialize_rejects_an_issuer_that_is_not_a_non_empty_string issuer: issuer, ) end + + assert_match(/issuer must be the authorization server's issuer URL/, error.message) + end + end + + def test_initialize_accepts_an_https_issuer_and_a_loopback_http_issuer + ["https://auth.example.com", "HTTPS://auth.example.com", "http://127.0.0.1:9292", "http://[::1]:9292"].each do |issuer| + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag" }, + issuer: issuer, + ) + + assert_equal(issuer, provider.client_information["issuer"]) end end