Skip to content

Commit dc8efd9

Browse files
committed
Replace authorization request parameters the endpoint URL already carries
## Motivation and Context `Flow#build_authorization_url` appended the flow's parameters after whatever query the authorization server metadata's `authorization_endpoint` already carried. An endpoint URL carrying `client_id`, `redirect_uri`, `state`, `code_challenge`, or `resource` of its own therefore produced a request with each of them twice, the URL's value first. RFC 6749 Section 3.1 forbids sending a parameter more than once, and which value a server honors is its own choice; on the legacy 2025-03-26 path the endpoint URL is served by the MCP server itself, so its query could speak for the client's identity, redirect URI, PKCE challenge, and resource at the authorization server. A parameter the flow sets now replaces one of the same name in the endpoint URL, and the rest of the URL's query is kept, which is what the TypeScript SDK's `searchParams.set` does. `request` and `request_uri` are dropped from the endpoint URL's query as well, although the flow sets neither. RFC 9101 has an authorization server take the whole authorization request from the object they carry, over every parameter in the query, and both are the client's to send, so an endpoint URL holding one of them could speak for the client through that door too. A `scope` in the endpoint URL stays when the flow has none of its own, as it does in the TypeScript SDK: an authorization server may place a default scope on its own endpoint URL. ## How Has This Been Tested? A new test in `test/mcp/client/oauth/flow_test.rb` serves an `authorization_endpoint` whose query carries those parameters and one more, and checks that the authorization URL holds each parameter once with the flow's value while keeping the extra one. It fails against the previous library. Two more tests in the same file check that `request` and `request_uri` in the endpoint URL's query are dropped, and that a `scope` there stays when the flow has none; the first of them fails against the previous library. ## Breaking Changes None.
1 parent 339f0a6 commit dc8efd9

2 files changed

Lines changed: 102 additions & 10 deletions

File tree

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

Lines changed: 24 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1206,16 +1206,30 @@ def build_authorization_url(as_metadata:, client_id:, scope:, state:, code_chall
12061206
"Authorization server metadata `authorization_endpoint` is not a valid URI: #{e.message}."
12071207
end
12081208

1209-
params = URI.decode_www_form(uri.query.to_s)
1210-
params << ["response_type", "code"]
1211-
params << ["client_id", client_id]
1212-
params << ["redirect_uri", @provider.redirect_uri]
1213-
params << ["code_challenge", code_challenge]
1214-
params << ["code_challenge_method", "S256"]
1215-
params << ["state", state]
1216-
params << ["scope", scope] if scope
1217-
params << ["resource", resource] if resource
1218-
uri.query = URI.encode_www_form(params)
1209+
# A parameter the flow sets replaces any of the same name the endpoint URL already carries.
1210+
# RFC 6749 Section 3.1 forbids sending a parameter twice, and which of two values a server would honor is
1211+
# its own choice; on the legacy path the endpoint URL is served by the MCP server, whose query must not speak
1212+
# for the client's `client_id`, `redirect_uri`, `code_challenge`, or `resource`.
1213+
# Other parameters in the URL are kept, as the TypeScript SDK's `searchParams.set` keeps them; that includes
1214+
# a `scope` when the flow has none, since an authorization server may set a default scope there.
1215+
# RFC 9101 `request` and `request_uri` are dropped as well, though the flow sets neither: a server takes
1216+
# the whole authorization request from the object they carry, over every parameter in the query, and both are
1217+
# the client's to send, never an endpoint URL's to supply.
1218+
own_params = [
1219+
["response_type", "code"],
1220+
["client_id", client_id],
1221+
["redirect_uri", @provider.redirect_uri],
1222+
["code_challenge", code_challenge],
1223+
["code_challenge_method", "S256"],
1224+
["state", state],
1225+
]
1226+
own_params << ["scope", scope] if scope
1227+
own_params << ["resource", resource] if resource
1228+
dropped_names = own_params.map(&:first) + ["request", "request_uri"]
1229+
1230+
params = URI.decode_www_form(uri.query.to_s).reject { |name, _value| dropped_names.include?(name) }
1231+
uri.query = URI.encode_www_form(params + own_params)
1232+
12191233
uri
12201234
end
12211235

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

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1572,6 +1572,52 @@ def test_run_raises_when_authorization_endpoint_is_malformed_uri
15721572
assert_match(/authorization_endpoint/i, error.message)
15731573
end
15741574

1575+
def test_run_replaces_authorization_request_parameters_the_endpoint_url_already_carries
1576+
# An `authorization_endpoint` may carry a query of its own. A parameter of the same name as one the flow sets is
1577+
# replaced rather than sent twice, so the URL cannot speak for the client's identity, redirect URI, or PKCE challenge;
1578+
# the rest of the query is kept.
1579+
query = authorization_url_query_for_endpoint_query(
1580+
"client_id=other&redirect_uri=https%3A%2F%2Fother.example.com%2Fcb&state=fixed&code_challenge=theirs&audience=api",
1581+
)
1582+
1583+
assert_equal(
1584+
["audience", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"],
1585+
query.map(&:first),
1586+
)
1587+
assert_equal("api", query.to_h["audience"])
1588+
assert_equal("test-client", query.to_h["client_id"])
1589+
assert_equal("http://localhost:0/callback", query.to_h["redirect_uri"])
1590+
refute_equal("theirs", query.to_h["code_challenge"])
1591+
refute_equal("fixed", query.to_h["state"])
1592+
end
1593+
1594+
def test_run_drops_request_object_parameters_the_endpoint_url_carries
1595+
# RFC 9101 has an authorization server take the whole authorization request from `request` or `request_uri`,
1596+
# over every parameter in the query, so neither may come from the endpoint URL even though the flow sets no
1597+
# parameter of either name.
1598+
query = authorization_url_query_for_endpoint_query(
1599+
"request_uri=https%3A%2F%2Fother.example.com%2Frequest&request=opaque&audience=api",
1600+
)
1601+
1602+
assert_equal(
1603+
["audience", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"],
1604+
query.map(&:first),
1605+
)
1606+
end
1607+
1608+
def test_run_keeps_an_endpoint_scope_when_the_flow_has_none
1609+
# An authorization server may place a default `scope` on its own endpoint URL. With no scope of its own
1610+
# (none requested, none in the resource metadata, none on the provider) the flow leaves it there,
1611+
# as the TypeScript SDK does.
1612+
query = authorization_url_query_for_endpoint_query("scope=openid")
1613+
1614+
assert_equal(
1615+
["scope", "response_type", "client_id", "redirect_uri", "code_challenge", "code_challenge_method", "state", "resource"],
1616+
query.map(&:first),
1617+
)
1618+
assert_equal("openid", query.to_h["scope"])
1619+
end
1620+
15751621
def test_run_raises_when_prm_resource_is_malformed_uri
15761622
stub_request(:get, @prm_url).to_return(
15771623
status: 200,
@@ -4436,6 +4482,38 @@ def test_run_sends_server_url_as_resource_when_prm_omits_it
44364482

44374483
private
44384484

4485+
# Serves authorization server metadata whose `authorization_endpoint` carries `endpoint_query`,
4486+
# runs the authorization-code flow to completion, and returns the query of the URL the browser was
4487+
# sent to as name/value pairs in order.
4488+
def authorization_url_query_for_endpoint_query(endpoint_query)
4489+
stub_request(:get, @as_metadata_url).to_return(
4490+
status: 200,
4491+
headers: { "Content-Type" => "application/json" },
4492+
body: JSON.generate(
4493+
issuer: @auth_base,
4494+
authorization_endpoint: "#{@auth_base}/authorize?#{endpoint_query}",
4495+
token_endpoint: "#{@auth_base}/token",
4496+
registration_endpoint: "#{@auth_base}/register",
4497+
response_types_supported: ["code"],
4498+
code_challenge_methods_supported: ["S256"],
4499+
token_endpoint_auth_methods_supported: ["none"],
4500+
),
4501+
)
4502+
holder = {}
4503+
provider = Provider.new(
4504+
**authorization_code_provider_arguments(
4505+
->(url) { holder[:authorization_url] = url },
4506+
-> { ["test-auth-code", URI.decode_www_form(holder[:authorization_url].query).to_h.fetch("state")] },
4507+
),
4508+
)
4509+
4510+
result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url)
4511+
4512+
assert_equal(:authorized, result)
4513+
4514+
URI.decode_www_form(holder[:authorization_url].query)
4515+
end
4516+
44394517
def refresh_only_provider
44404518
provider = Provider.new(
44414519
client_metadata: { redirect_uris: ["http://localhost:0/callback"] },

0 commit comments

Comments
 (0)