Skip to content

Commit b44a73c

Browse files
committed
Validate the issuer of machine-to-machine providers as an issuer URL
## Motivation and Context `issuer:` on `ClientCredentialsProvider` and `CrossAppAccessProvider` accepted any non-empty String. A value that is not an issuer URL can never match the metadata of an authorization server, so such credentials were refused at the first request with a message about a different authorization server, far from the configuration that caused it. The flow accepts an authorization server over HTTPS, or over HTTP on a loopback address, and the option is now held to the same rule at construction, where a mistake is reported with the value that was given. Since the issuer of the metadata is compared as a String, a URI object or a Symbol could never equal it either, nor could an issuer with a query or fragment, which RFC 8414 rules out; both are refused the same way. The Python SDK requires an http(s) scheme on its `issuer`; the rule here is the stricter one the flow already applies to every authorization server URL. ## How Has This Been Tested? test/mcp/client/oauth/client_credentials_provider_test.rb and test/mcp/client/oauth/cross_app_access_provider_test.rb: a bare host, a non-HTTP scheme, a URL without a host, a plain HTTP URL off the loopback, a query, a fragment, a URI object and a Symbol are refused with the new message, and HTTPS and loopback HTTP issuers (IPv6 and an uppercase scheme included) are kept; before the change the String values among them were accepted. ## Breaking Changes None for an `issuer:` that names an authorization server. A value that could never have matched a compliant one (RFC 8414 forbids a query or fragment in an issuer) is refused at construction instead of at the first request.
1 parent d9a19c1 commit b44a73c

6 files changed

Lines changed: 73 additions & 15 deletions

File tree

‎docs/_client/authorization.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -321,10 +321,12 @@ Keyword arguments:
321321
- `private_key`, `signing_algorithm`: Required with `private_key_jwt` - the key (a PEM string
322322
or `OpenSSL::PKey::PKey`, never written to `storage`) signs the client assertion with `"ES256"`
323323
or `"RS256"`; `client_secret` must not be set, because the private key is the credential.
324-
- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to. A different authorization server raises `Flow::AuthorizationError`.
324+
- `issuer`: Optional. The issuer identifier of the authorization server the credentials belong to, spelled as its metadata states it:
325+
an `https` URL (`http` only on a loopback address) without query or fragment. A different authorization server raises `Flow::AuthorizationError`.
325326
A single trailing slash difference is accepted, as in the TypeScript SDK. When the protected resource metadata lists several authorization servers,
326-
the one naming `issuer:` is used. Omitting `issuer:` is deprecated and emits a warning at construction. Without it, credentials go to whichever authorization server
327-
the MCP server names, as before. To use credentials for another authorization server, construct a provider with that server's `issuer:` and credentials.
327+
the one naming `issuer:` is used. Omitting `issuer:` is deprecated and emits a warning at construction.
328+
Without it, credentials go to whichever authorization server the MCP server names, as before. To use credentials for another authorization server,
329+
construct a provider with that server's `issuer:` and credentials.
328330
- `scope`, `storage`, `authorization_request_validator`, `token_request_params`, `http_client_customizer`: Optional, same meaning as on `Provider`.
329331
Use `token_request_params` for a parameter the authorization server requires on the `client_credentials` grant, such as Auth0's `audience`.
330332

‎lib/mcp/client/oauth/client_credentials_provider.rb‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,8 @@ module OAuth
3535
# the server's `token_endpoint_auth_signing_alg_values_supported` fails
3636
# loudly at construction instead of as a 401 (the TypeScript and Python SDKs
3737
# also take the algorithm as an explicit option).
38-
# - `issuer` - String issuer identifier of the authorization server the credentials belong to.
38+
# - `issuer` - String issuer identifier of the authorization server the credentials belong to,
39+
# as its metadata states it: an `https` URL, or `http` on a loopback address, without query or fragment.
3940
# A different authorization server raises `Flow::AuthorizationError`. Omitting this is deprecated
4041
# and emits a warning; without it, credentials go to whichever authorization server the MCP server names.
4142
# - `scope` - String of space-separated scopes to request when the server's
@@ -93,8 +94,9 @@ def initialize(
9394
"the MCP server names. Pass the authorization server's issuer URL as `issuer:` so they are only sent there.",
9495
uplevel: 1,
9596
)
96-
elsif !issuer.is_a?(String) || blank?(issuer)
97-
raise InvalidCredentialsError, "issuer must be a non-empty String: the issuer identifier of the authorization server the credentials belong to."
97+
elsif !Discovery.issuer_identifier?(issuer)
98+
raise InvalidCredentialsError, "issuer must be the authorization server's issuer URL as its metadata states it, over HTTPS " \
99+
"(HTTP only on a loopback address) and without query or fragment, got #{issuer.inspect}."
98100
end
99101

100102
client_information = { "client_id" => client_id, "token_endpoint_auth_method" => token_endpoint_auth_method }

‎lib/mcp/client/oauth/cross_app_access_provider.rb‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,8 @@ module OAuth
2121
#
2222
# Optional keyword arguments:
2323
#
24-
# - `issuer` - String issuer identifier of the MCP authorization server the credentials belong to.
24+
# - `issuer` - String issuer identifier of the MCP authorization server the credentials belong to, as its
25+
# metadata states it: an `https` URL, or `http` on a loopback address, without query or fragment.
2526
# A mismatch raises `Flow::AuthorizationError` before `assertion_provider` is called. Omitting this is
2627
# deprecated and emits a warning; without it, credentials go to whichever authorization server the MCP server names.
2728
# - `scope` - String of space-separated scopes to request when the server's `WWW-Authenticate` and
@@ -74,8 +75,10 @@ def initialize(
7475
"the MCP server names. Pass the authorization server's issuer URL as `issuer:` so they are only sent there.",
7576
uplevel: 1,
7677
)
77-
elsif !issuer.is_a?(String) || blank?(issuer)
78-
raise InvalidConfigurationError, "issuer must be a non-empty String: the issuer identifier of the authorization server the credentials belong to."
78+
elsif !Discovery.issuer_identifier?(issuer)
79+
raise InvalidConfigurationError,
80+
"issuer must be the authorization server's issuer URL as its metadata states it, over HTTPS " \
81+
"(HTTP only on a loopback address) and without query or fragment, got #{issuer.inspect}."
7982
end
8083

8184
http_client_customizer = validated_http_client_customizer(http_client_customizer)

‎lib/mcp/client/oauth/discovery.rb‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,20 @@ def secure_url?(url)
200200
false
201201
end
202202

203+
# Whether `value` can be an authorization server issuer identifier the flow could ever match: a String naming an `https` URL,
204+
# or `http` on a loopback address as `secure_url?` allows, without query or fragment (RFC 8414 Section 2).
205+
# The issuer of the metadata is compared as a String, so a value of another type would never equal it, and an issuer with
206+
# a query or fragment is one no authorization server may publish.
207+
def issuer_identifier?(value)
208+
return false unless value.is_a?(String) && secure_url?(value)
209+
210+
uri = URI.parse(value)
211+
212+
uri.query.nil? && uri.fragment.nil?
213+
rescue URI::InvalidURIError
214+
false
215+
end
216+
203217
# Returns true when `url` and `other` share an origin: same scheme, same host,
204218
# and same port. Scheme and host compare case-insensitively, and an implicit default port compares
205219
# equal to the same port written out, because `URI::HTTP#port` already resolves to the default.

‎test/mcp/client/oauth/client_credentials_provider_test.rb‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,29 @@ def test_initialize_stores_the_issuer_with_the_credentials
5858
assert_equal("https://auth.example.com", provider.client_information["issuer"])
5959
end
6060

61-
def test_initialize_rejects_an_issuer_that_is_not_a_non_empty_string
62-
["", " ", :issuer, false, 123].each do |issuer|
63-
assert_raises(ClientCredentialsProvider::InvalidCredentialsError) do
61+
def test_initialize_rejects_an_issuer_that_is_not_an_issuer_url
62+
[
63+
"", " ", :issuer, false, 123, :"https://auth.example.com", URI("https://auth.example.com"),
64+
"auth.example.com", "ftp://auth.example.com", "https://", "https://auth.example.com/ metadata",
65+
"https://auth.example.com/?tenant=1", "https://auth.example.com/#section",
66+
"http://auth.example.com", "http://10.0.0.5", "http://localhost.example.com", "http://127.attacker.example"
67+
].each do |issuer|
68+
error = assert_raises(ClientCredentialsProvider::InvalidCredentialsError, issuer.inspect) do
6469
ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", issuer: issuer)
6570
end
71+
72+
assert_match(/issuer must be the authorization server's issuer URL/, error.message)
73+
end
74+
end
75+
76+
def test_initialize_accepts_an_https_issuer_and_a_loopback_http_issuer
77+
[
78+
"https://auth.example.com", "https://auth.example.com/", "https://auth.example.com/tenant", "HTTPS://auth.example.com",
79+
"http://127.0.0.1:9292", "http://localhost:8080/auth", "http://[::1]:9292"
80+
].each do |issuer|
81+
provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", issuer: issuer)
82+
83+
assert_equal(issuer, provider.client_information["issuer"])
6684
end
6785
end
6886

‎test/mcp/client/oauth/cross_app_access_provider_test.rb‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -54,16 +54,35 @@ def test_initialize_stores_the_issuer_with_the_credentials
5454
assert_equal("https://auth.example.com", provider.client_information["issuer"])
5555
end
5656

57-
def test_initialize_rejects_an_issuer_that_is_not_a_non_empty_string
58-
["", " ", :issuer, false, 123].each do |issuer|
59-
assert_raises(CrossAppAccessProvider::InvalidConfigurationError) do
57+
def test_initialize_rejects_an_issuer_that_is_not_an_issuer_url
58+
[
59+
"", " ", :issuer, false, 123, URI("https://auth.example.com"),
60+
"auth.example.com", "ftp://auth.example.com", "https://", "https://auth.example.com/?tenant=1",
61+
"http://auth.example.com", "http://10.0.0.5"
62+
].each do |issuer|
63+
error = assert_raises(CrossAppAccessProvider::InvalidConfigurationError, issuer.inspect) do
6064
CrossAppAccessProvider.new(
6165
client_id: "xaa-client",
6266
client_secret: "xaa-secret",
6367
assertion_provider: ->(**) { "id-jag" },
6468
issuer: issuer,
6569
)
6670
end
71+
72+
assert_match(/issuer must be the authorization server's issuer URL/, error.message)
73+
end
74+
end
75+
76+
def test_initialize_accepts_an_https_issuer_and_a_loopback_http_issuer
77+
["https://auth.example.com", "HTTPS://auth.example.com", "http://127.0.0.1:9292", "http://[::1]:9292"].each do |issuer|
78+
provider = CrossAppAccessProvider.new(
79+
client_id: "xaa-client",
80+
client_secret: "xaa-secret",
81+
assertion_provider: ->(**) { "id-jag" },
82+
issuer: issuer,
83+
)
84+
85+
assert_equal(issuer, provider.client_information["issuer"])
6786
end
6887
end
6988

0 commit comments

Comments
 (0)