From 0da02918877bbef8a243b134ce9caa8b87a4a226 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 6 Sep 2026 03:19:52 +0900 Subject: [PATCH 01/17] [Doc] Explain when the HTTP client needs event_stream_parser ## Motivation and Context The Installation page names faraday as the example of an optional dependency but leaves out event_stream_parser, which the HTTP client transport needs as soon as a server answers with SSE. The Transports page documents both gems, so a reader who starts from Installation learns about the second one only when a streaming response fails to parse. The Transports page also marks event_stream_parser as "optional, required only for SSE responses", which reads as a corner case. Whether a response is JSON or SSE is the server's choice, made for each response, a client must accept both, and this SDK's own server picks SSE by default and always on the modern lifecycle, so a client written for arbitrary servers needs both gems. Both pages now say so, and the Transports page spells out the one setup that can leave the gem out: a server known to answer with JSON alone, such as this SDK's server in JSON response mode serving handshake-lifecycle clients, and no listening stream. The Installation page shows the faraday-only Gemfile first and then the one that adds event_stream_parser, so both cases are visible at a glance. ## How Has This Been Tested? Documentation only; the Gemfile snippets on both pages match. ## Breaking Changes None. --- docs/_client/transports.md | 9 ++++++++- docs/installation.md | 12 +++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/docs/_client/transports.md b/docs/_client/transports.md index a8a4764..a30259c 100644 --- a/docs/_client/transports.md +++ b/docs/_client/transports.md @@ -64,7 +64,14 @@ The stdio transport automatically handles: Use the `MCP::Client::HTTP` transport to interact with MCP servers using simple HTTP requests. -You'll need to add `faraday` as a dependency in order to use the HTTP transport layer. Add `event_stream_parser` as well if the server uses SSE (`text/event-stream`) responses: +You'll need to add `faraday` as a dependency in order to use the HTTP transport layer, and `event_stream_parser` +to read SSE (`text/event-stream`) responses. +Whether a response is JSON or SSE is the server's choice, made for each response, and a client must accept both, +so a client written for arbitrary servers needs both gems. This SDK's own server picks SSE by default and always +does on the modern lifecycle, and the listening stream that `on_elicitation` and `on_sampling` open is SSE as well. +`event_stream_parser` is loaded on the first SSE response, so it can be left out only against a server known to +answer with JSON alone, such as this SDK's server in [JSON response mode](/server/transports/#json-response-mode) +serving handshake-lifecycle clients, and only while no listening stream is opened: ```ruby gem "mcp" diff --git a/docs/installation.md b/docs/installation.md index f93c6e5..91aaee1 100644 --- a/docs/installation.md +++ b/docs/installation.md @@ -38,6 +38,16 @@ $ gem install mcp You may need to add additional dependencies depending on which features you wish to access. For example, the HTTP client transport requires the `faraday` gem: ```ruby -gem "mcp" gem "faraday", ">= 2.0" ``` + +Reading SSE (`text/event-stream`) responses needs `event_stream_parser` as well. +Whether a response is JSON or SSE is the server's choice, made for each response, and this SDK's own server picks SSE by default, +so a client written for arbitrary servers needs both gems: + +```ruby +gem "faraday", ">= 2.0" +gem "event_stream_parser", ">= 1.0" +``` + +The [Transports](/client/transports/#http-transport-layer) page describes the one setup where `faraday` alone is enough. From 09c89aef14fbbb5b6f9a05a65c355daf17a6c719 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 8 Sep 2026 17:04:43 +0900 Subject: [PATCH 02/17] Parse HTTP client response bodies without Faraday's JSON middleware ## Motivation and Context json 3.0, released on 2026-09-07, accepts the options of `JSON.parse` as keywords only, and Faraday's json response middleware, through 2.14.3, hands them over as a positional Hash, which Ruby 3 no longer converts. With both installed, every JSON body the client receives fails inside that middleware, and `MCP::Client::HTTP` reports each request as an internal error. Faraday's main branch passes the options as keywords, but no release carries that yet. The client already parses streamed bodies and buffered SSE bodies itself; only a JSON body delivered whole by an adapter without streaming support still went through the middleware. The client now parses that body too and no longer registers the middleware, so every body takes `parse_json_buffer` and a malformed one answers `:parse_error` whichever way it arrived. A JSON middleware added through the Faraday customizer block still works, since a body it has parsed already is used as is. ## How Has This Been Tested? A new test drives a buffered JSON response through a stand-in `JSON.parse` with the json 3.0 signature, options as keywords only, so on Ruby 3 it failed with the middleware registered whatever json version is installed and passes now. The existing buffered-body test keeps covering the plain path. The full suite passes on Ruby 3.0.7 with json 3.0.1 and faraday 2.14.3, where five client tests failed before. ## Breaking Changes None. A malformed JSON body delivered by an adapter without streaming support now raises `RequestHandlerError` with `error_type: :parse_error`, as the streamed path already did, instead of `:internal_error` wrapping a `Faraday::ParsingError`. --- lib/mcp/client/http.rb | 7 ++++--- test/mcp/client/http_test.rb | 20 ++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/mcp/client/http.rb b/lib/mcp/client/http.rb index 1c3fd86..6969b07 100644 --- a/lib/mcp/client/http.rb +++ b/lib/mcp/client/http.rb @@ -723,7 +723,8 @@ module MCP require_faraday! @client ||= Faraday.new(url) do |faraday| faraday.request(:json) - faraday.response(:json) + # The client parses response bodies itself (`resolve_response_body`), so streamed and buffered responses + # take the same path; Faraday's json response middleware is deliberately left out. faraday.response(:raise_error) faraday.headers["Accept"] = ACCEPT_HEADER @@ -1074,8 +1075,8 @@ module MCP elsif content_type&.include?("application/json") return parse_json_buffer(stream.buffer, method, params) unless stream.buffer.empty? - # Adapters without `on_data` support deliver the body via `response.body`, - # already parsed by the json response middleware. + # Adapters without `on_data` support deliver the body via `response.body`; a JSON middleware + # added by the Faraday customizer may have parsed it already. response.body.is_a?(String) ? parse_json_buffer(response.body, method, params) : response.body else raise RequestHandlerError.new( diff --git a/test/mcp/client/http_test.rb b/test/mcp/client/http_test.rb index 077db31..0b9960a 100644 --- a/test/mcp/client/http_test.rb +++ b/test/mcp/client/http_test.rb @@ -940,6 +940,26 @@ module MCP assert_equal({ "result" => { "tools" => [] } }, response) end + def test_send_request_parses_a_json_body_with_a_parser_taking_keyword_options_only + # json 3.0 accepts parser options as keywords only, and Faraday's json response middleware + # passes them as a positional Hash, which Ruby 3 no longer converts. The stand-in parser has + # the json 3.0 signature, so the body must reach `JSON.parse` through the client's own call. + stubs = Faraday::Adapter::Test::Stubs.new do |stub| + stub.post("/") do + [200, { "Content-Type" => "application/json" }, { result: { tools: [] } }.to_json] + end + end + client = HTTP.new(url: url) { |faraday| faraday.adapter(:test, stubs) } + parse = JSON.method(:parse) + keyword_options_only_parse = ->(source, **options) { parse.call(source, **options) } + + response = JSON.stub(:parse, keyword_options_only_parse) do + client.send_request(request: { jsonrpc: "2.0", id: "test_id", method: "tools/list" }) + end + + assert_equal({ "result" => { "tools" => [] } }, response) + end + def test_send_request_mirrors_x_mcp_header_params_into_mcp_param_headers # SEP-2243: on a modern connection, `tools/list` teaches the transport the `x-mcp-header` # declarations, and the following `tools/call` mirrors the annotated arguments into From d40d9018d51c2137928d12e480aec3a9848fa0e1 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 8 Sep 2026 17:15:26 +0900 Subject: [PATCH 03/17] Keep json below 3.0 on Ruby 2.7.0 through 2.7.2 in the Gemfile ## Motivation and Context json 3.0 declares Ruby 2.7 as its floor, but its `load_file` forwards arguments after a leading parameter, `def load_file(filespec, ...)`, syntax Ruby 2.7.3 was the first to parse. On Ruby 2.7.0 through 2.7.2, requiring json 3.0.1 is a `SyntaxError`, so `require "mcp"` fails before any test runs, and the CI job pinned to 2.7.0, the minimum the gemspec supports, fails the whole suite at load time. The Gemfile keeps json below 3.0 on those three patch levels only. Later 2.7 releases and Ruby 3 parse the syntax and stay on whatever json the resolver picks. The line carries a FIXME to drop it once a json release includes ruby/json#1072, whichever version that turns out to be; excluding only the two broken releases would silently trust a 3.0.2 that may not carry the fix. ## How Has This Been Tested? `gem "json", "3.0.1"; require "json"` raises the `SyntaxError` on Ruby 2.7.0 and loads on 2.7.3 and 2.7.8. With the pin, a fresh `bundle install` on Ruby 2.7.2 resolves json 2.x and `bundle exec rake test` passes. ## Breaking Changes None. The Gemfile is not part of the gem. --- Gemfile | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Gemfile b/Gemfile index 0aa50ee..fd061bd 100644 --- a/Gemfile +++ b/Gemfile @@ -19,6 +19,9 @@ gem "activesupport" gem "debug" if RUBY_VERSION >= "3.1" # Avoid i18n 1.15.0, which breaks on Ruby 3.1 (ruby-i18n/i18n#735). gem "i18n", "!= 1.15.0" +# FIXME: Drop this once a json release includes https://github.com/ruby/json/pull/1072. json 3.0.0 and 3.0.1 +# forward arguments after a leading parameter, syntax Ruby 2.7.3 was the first to parse, while allowing Ruby 2.7.0. +gem "json", "< 3" if RUBY_VERSION < "2.7.3" gem "rake", "~> 13.0" gem "sorbet-static-and-runtime" if RUBY_VERSION >= "3.0" gem "yard", "~> 0.9" From 3b44ec4f488f3d38271de0631c773cf1c3d45c88 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 9 Sep 2026 10:43:15 +0900 Subject: [PATCH 04/17] Release 1.5.1 This release keeps the HTTP client working with json 3.0, which accepts the options of `JSON.parse` as keywords only. Faraday's JSON response middleware, through 2.14.3, passes them as a positional Hash that Ruby 3 no longer converts, so every JSON body the client received failed as an internal error. The client now parses response bodies itself, the way it already handled streamed ones, and no longer registers the middleware. The one visible difference: a malformed JSON body delivered whole by an adapter without streaming support now raises `RequestHandlerError` with `error_type: :parse_error`, as the streamed path already did, instead of `:internal_error` wrapping a `Faraday::ParsingError`. --- CHANGELOG.md | 13 +++++++++++++ lib/mcp/version.rb | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 051f239..88ec62e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,19 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +## [1.5.1] - 2026-09-09 + +This release keeps the HTTP client working with json 3.0. That release accepts the options of `JSON.parse` +as keywords only, and Faraday's JSON response middleware, through 2.14.3, passes them as a positional Hash, +which Ruby 3 no longer converts, so every JSON body the client received failed as an internal error. +The client now parses response bodies itself, the way it already handled streamed ones, and no longer +registers the middleware. A malformed JSON body delivered whole by an adapter without streaming support +now raises `RequestHandlerError` with `error_type: :parse_error`, as the streamed path already did. + +### Fixed + +- Parse HTTP client response bodies without Faraday's JSON middleware, which json 3.0 breaks (#546) + ## [1.5.0] - 2026-09-05 This release makes the client answer a server's `ping` with the empty result the specification requires. diff --git a/lib/mcp/version.rb b/lib/mcp/version.rb index 1bbb8dd..2d0dff8 100644 --- a/lib/mcp/version.rb +++ b/lib/mcp/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module MCP - VERSION = "1.5.0" + VERSION = "1.5.1" end From 4192ea58eae515fce6304e1731f2c2c4cdaa1b39 Mon Sep 17 00:00:00 2001 From: Junyuan Zheng Date: Wed, 9 Sep 2026 12:14:21 -0400 Subject: [PATCH 05/17] fix(client/oauth): omit body client_id when using HTTP Basic --- lib/mcp/client/oauth/flow.rb | 49 ++++++++++++++++-------------- test/mcp/client/oauth/flow_test.rb | 49 +++++++++++++++++++++++++++++- 2 files changed, 74 insertions(+), 24 deletions(-) diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 9a825c0..5b4c0c1 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -997,9 +997,9 @@ module MCP post_to_token_endpoint(as_metadata: as_metadata, client_info: client_info, form: form) end - # Submits a form-encoded request to the token endpoint, applying - # the client authentication method advertised in `client_information` and - # adding `client_id` (and `client_secret` when not using HTTP Basic). + # Submits a form-encoded token request using the authentication method + # stored in `client_information`. The method determines whether client + # credentials belong in the form body, a Basic header, or a JWT assertion. def post_to_token_endpoint(as_metadata:, client_info:, form:) client_id = client_info_required_value(client_info, "client_id") unless client_id @@ -1010,12 +1010,13 @@ module MCP client_secret = client_info_required_value(client_info, "client_secret") token_endpoint_auth_method = client_info_value(client_info, "token_endpoint_auth_method") - form = if token_endpoint_auth_method == "private_key_jwt" - # RFC 7523 Section 2.2 JWT client assertion for the `private_key_jwt` method of - # the `io.modelcontextprotocol/oauth-client-credentials` extension (SEP-1046). - # The client identity travels in the assertion's `iss`/`sub` claims, so `client_id` is - # omitted from the body per RFC 7521 Section 4.2 (the `client_assertion` conveys the client identity). - # The audience is the issuer identifier that `ensure_issuer_matches!` already byte-validated. + # Apply one client authentication method per request (RFC 6749 Section 2.3). + headers = {} + form = case token_endpoint_auth_method + when "private_key_jwt" + # The assertion identifies the client through its `iss` and `sub` + # claims, so the body needs no separate `client_id` (RFC 7521 Section 4.2). + # Use the issuer already checked by `ensure_issuer_matches!` as the audience. unless @provider.respond_to?(:client_assertion) raise AuthorizationError, "token_endpoint_auth_method is private_key_jwt but the provider does not " \ @@ -1026,22 +1027,24 @@ module MCP "client_assertion_type" => JWTClientAssertion::ASSERTION_TYPE, "client_assertion" => @provider.client_assertion(audience: as_metadata["issuer"]), ) + when "client_secret_post" + # Send the client ID and available secret in the form body. + body = form.merge("client_id" => client_id) + body["client_secret"] = client_secret if client_secret + body else - form.merge("client_id" => client_id) - end - - headers = {} - if client_secret - case token_endpoint_auth_method - when "client_secret_post" - form["client_secret"] = client_secret - when "none" - # Public client; no credential. - else - # RFC 6749 §2.3.1 recommends Basic for confidential clients and - # both Python and TypeScript SDKs default here when - # the authentication method is not explicitly stored. + if client_secret && token_endpoint_auth_method != "none" + # Basic is also the fallback when a secret is present but no method + # is stored. A body `client_id` is optional (RFC 6749 Section 3.2.1); + # omit it because some servers treat it alongside Basic as a second + # authentication method and reject the request with `invalid_request`. headers["Authorization"] = "Basic " + basic_auth_credentials(client_id, client_secret) + form + else + # With `none` or no secret, identify the client using `client_id`. + # This is required for unauthenticated authorization-code exchanges + # (RFC 6749 Section 3.2.1). + form.merge("client_id" => client_id) end end diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index dc78e03..326dbd8 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -158,6 +158,21 @@ module MCP end end + def test_run_client_credentials_with_client_secret_basic_omits_client_id_from_body + # Explicit Basic authentication must keep both credentials in the header + # to avoid servers treating body credentials as a second auth method. + provider = client_credentials_provider(token_endpoint_auth_method: "client_secret_basic") + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + req.headers["Authorization"] == "Basic " + Base64.strict_encode64("cc-client:cc-secret") && + !form.key?("client_id") && + !form.key?("client_secret") + end + end + def test_run_client_credentials_raises_clean_error_when_client_information_missing # The constructor always stores credentials, but if a custom storage loses them # the grant must fail with a domain error rather than a `NoMethodError` from @@ -1860,8 +1875,10 @@ module MCP expected = "Basic " + Base64.strict_encode64("pre-registered-client:pre-registered-secret") assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h req.headers["Authorization"] == expected && - !URI.decode_www_form(req.body).to_h.key?("client_secret") + !form.key?("client_id") && + !form.key?("client_secret") end end @@ -2365,6 +2382,36 @@ module MCP assert_equal("saved-rt", provider.tokens["refresh_token"]) end + def test_refresh_with_client_secret_basic_omits_client_id_from_body + # Stored credentials without an auth method default to Basic. Refresh + # must also omit the body `client_id` when using that fallback. + stub_request(:post, "#{@auth_base}/token") + .with(body: hash_including("grant_type" => "refresh_token")) + .to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "fresh-at", token_type: "Bearer", expires_in: 3600), + ) + + provider = Provider.new( + client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(_url) {}, + callback_handler: -> { [nil, nil] }, + ) + provider.save_client_information("client_id" => "conf-client", "client_secret" => "conf-secret") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + req.headers["Authorization"] == "Basic " + Base64.strict_encode64("conf-client:conf-secret") && + !form.key?("client_id") && + !form.key?("client_secret") + end + end + def test_run_records_the_issuer_that_minted_the_tokens state_holder = {} provider = Provider.new( From 2f65a6e502e104bddab43d13786b9f3cbd474be7 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Wed, 9 Sep 2026 18:36:20 +0900 Subject: [PATCH 06/17] Drop the json pin for Ruby 2.7.0 through 2.7.2 from the Gemfile ## Motivation and Context json 3.0.2 loads on Ruby 2.7.0 through 2.7.2 again: it no longer forwards arguments after a leading parameter, the syntax those patch levels could not parse, so the Gemfile no longer has to keep json below 3.0 there, and the CI job pinned to 2.7.0, the minimum the gemspec supports, resolves json 3.0.2 like every other job. The maintainers intend to raise json's own Ruby floor to 2.7.3 in the release after 3.0.2, so those patch levels stay on 3.0.2 by json's gemspec from then on, and no pin of this Gemfile's own is needed for that either. ## How Has This Been Tested? A fresh `bundle install` on Ruby 2.7.2 resolves json 3.0.2, `require "json"` loads it there, and `bundle exec rake test` passes. Ruby 3.0 and later were never affected by the pin. ## Breaking Changes None. The Gemfile is not part of the gem. --- Gemfile | 3 --- 1 file changed, 3 deletions(-) diff --git a/Gemfile b/Gemfile index fd061bd..0aa50ee 100644 --- a/Gemfile +++ b/Gemfile @@ -19,9 +19,6 @@ gem "activesupport" gem "debug" if RUBY_VERSION >= "3.1" # Avoid i18n 1.15.0, which breaks on Ruby 3.1 (ruby-i18n/i18n#735). gem "i18n", "!= 1.15.0" -# FIXME: Drop this once a json release includes https://github.com/ruby/json/pull/1072. json 3.0.0 and 3.0.1 -# forward arguments after a leading parameter, syntax Ruby 2.7.3 was the first to parse, while allowing Ruby 2.7.0. -gem "json", "< 3" if RUBY_VERSION < "2.7.3" gem "rake", "~> 13.0" gem "sorbet-static-and-runtime" if RUBY_VERSION >= "3.0" gem "yard", "~> 0.9" From 7e507f2963a24709c5c62800bf1d5ccd8cc10736 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 12 Sep 2026 00:02:30 +0900 Subject: [PATCH 07/17] [Doc] Document the client's ping answers and the stdio request limit ## Motivation and Context The client answers server pings automatically, but the docs leave out where that applies and what the stdio transport does with the other server-to-client requests. The client Transports page now lists the automatic ping answer among the things the stdio transport handles, and a note states that `ping` is the only server-to-client request answered there: a wire-level `elicitation/create` or `sampling/createMessage` is ignored over stdio, so a server that sends one waits for an answer that never comes, while the SEP-2322 `input_required` route works on every transport and keeps `on_elicitation` and `on_sampling` handlers working over stdio. Without this note, the only trace of the limitation is a hung tool call. The client Ping page now says that over HTTP a ping reaches the client only on a stream that is already open and that the client never opens one to receive pings, so a server can only ping a client that has a stream open. Registering `on_server_request("ping")` moves to the end of the section as the advanced option it is, together with its side effect of opening the GET listening stream on `connect`. The client overview mentions the automatic answer next to `MCP::Client#ping`, and the server Ping page's Client Side section points at it. ## How Has This Been Tested? Documentation only; the linked headings and pages exist. ## Breaking Changes None. --- docs/_client/index.md | 3 ++- docs/_client/ping.md | 8 ++++++-- docs/_client/transports.md | 8 ++++++++ docs/_server/ping.md | 2 ++ 4 files changed, 18 insertions(+), 3 deletions(-) diff --git a/docs/_client/index.md b/docs/_client/index.md index c919622..2e3077c 100644 --- a/docs/_client/index.md +++ b/docs/_client/index.md @@ -17,7 +17,8 @@ This class supports: - Lifecycle negotiation and connection via `MCP::Client#connect`, adopting the modern lifecycle when the server serves it; see [Lifecycle](/client/lifecycle/) - Server discovery via the `server/discover` method (`MCP::Client#discover`); see [Explicit Discovery](/client/lifecycle/#explicit-discovery) -- Liveness check via the `ping` method (`MCP::Client#ping`) +- Liveness check via the `ping` method (`MCP::Client#ping`), and automatic answers to server pings; + see [Answering Server Pings](/client/ping/#answering-server-pings) - Tool listing via the `tools/list` method (`MCP::Client#tools`) - Tool invocation via the `tools/call` method (`MCP::Client#call_tool`) - Resource listing via the `resources/list` method (`MCP::Client#resources`) diff --git a/docs/_client/ping.md b/docs/_client/ping.md index f6889e9..f546afe 100644 --- a/docs/_client/ping.md +++ b/docs/_client/ping.md @@ -32,13 +32,17 @@ propagate as exceptions raised by the transport layer. ## Answering Server Pings On handshake-lifecycle connections a server may ping the client the same way, and the client answers automatically with -the empty result - no handler is needed. Registering `transport.on_server_request("ping")` on `MCP::Client::HTTP` replaces -the automatic answer. +the empty result - no handler is needed. Over HTTP a ping reaches the client only on a stream that is already open +(the SSE response of an in-flight POST, or the GET listening stream); the client never opens a stream just to receive pings. Over stdio, a ping that arrives while a response is awaited is answered inline; between requests it is answered when the next request starts reading. The answer is best effort: a pong that cannot be written (for example, over a broken pipe) is dropped rather than failing the request whose response is being read. Independently of pings, consider setting `read_timeout:` on `MCP::Client::Stdio`, since a server that never answers otherwise holds the read until the process exits. +To answer pings yourself, register `transport.on_server_request("ping")` on `MCP::Client::HTTP`; it replaces +the automatic answer and, like any handler registered there, opens the standalone GET listening stream on `connect`, +as described on the [Transports](/client/transports/#server-to-client-requests-elicitation) page. + ## Server Side How servers answer `ping` requests and ping the client themselves is documented on diff --git a/docs/_client/transports.md b/docs/_client/transports.md index a30259c..4362666 100644 --- a/docs/_client/transports.md +++ b/docs/_client/transports.md @@ -59,6 +59,14 @@ The stdio transport automatically handles: - Spawning the server process with `Open3.popen3` - MCP protocol initialization handshake (`initialize` request + `notifications/initialized`) - JSON-RPC 2.0 message framing over newline-delimited JSON +- Answering server `ping` requests; see [Answering Server Pings](/client/ping/#answering-server-pings) + +{: .note } +> `ping` is the only server-to-client request answered over stdio. A wire-level `elicitation/create` +> or `sampling/createMessage` is ignored on this transport, so a server that sends one waits for an answer +> that never comes; use Streamable HTTP for those, as described below. The SEP-2322 `input_required` route, +> which is how the modern lifecycle asks for the same input, works on every transport, so `on_elicitation` +> and `on_sampling` handlers still fire over stdio; see [Multi Round-Trip Requests](/client/mrtr/). ## HTTP Transport Layer diff --git a/docs/_server/ping.md b/docs/_server/ping.md index 6272062..d8c6627 100644 --- a/docs/_server/ping.md +++ b/docs/_server/ping.md @@ -48,3 +48,5 @@ Server-to-client requests are bounded by a timeout on the Streamable HTTP transp ## Client Side Pinging the server with `MCP::Client#ping` is documented on the client [Ping](/client/ping/) page. +The Ruby client answers the pings sent with `ServerSession#ping` automatically; +see [Answering Server Pings](/client/ping/#answering-server-pings). From ef3eb7b0b2c41e20a6a956ec0c44eb61a9144544 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sun, 13 Sep 2026 17:10:12 +0900 Subject: [PATCH 08/17] [Doc] State the Listen Stream Delivery Semantics The Subscriptions page documented the acknowledgement-first contract but not what delivery means: the stream carries only changes that happen after the acknowledgement, nothing is queued while no stream is connected, and a dropped stream is gone together with anything it missed. The Python SDK's subscriptions page states the same contract ("It is not a replay log"). Spelling it out gives client implementers the fetch-after-subscribing guidance and makes the acknowledgement-window behavior explicit. --- docs/_server/subscriptions.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/docs/_server/subscriptions.md b/docs/_server/subscriptions.md index 9777d8c..22116aa 100644 --- a/docs/_server/subscriptions.md +++ b/docs/_server/subscriptions.md @@ -49,6 +49,13 @@ The first SSE event on the stream is the acknowledgement: } ``` +## Delivery Semantics + +A stream delivers only the changes that happen after its acknowledgement; it is not a replay log. +Nothing is queued while no stream is connected, and a dropped stream is gone together with anything it missed. +Clients fetch the current state after subscribing and treat each notification as a cue to refetch, +rather than as a payload. + ## Transport Support The stream is served on the Streamable HTTP modern path; stdio answers `-32601`. From 8d1d5be4bad5a6b85b1e6cda8d1c1f7c0d739b68 Mon Sep 17 00:00:00 2001 From: Junyuan Zheng Date: Mon, 14 Sep 2026 09:45:38 -0400 Subject: [PATCH 09/17] fix(client/oauth): preserve token endpoint error diagnostics --- docs/_client/authorization.md | 21 ++++ lib/mcp/client/oauth/flow.rb | 58 ++++++++--- test/mcp/client/oauth/flow_test.rb | 156 ++++++++++++++++++++++++++++- 3 files changed, 218 insertions(+), 17 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index a673a19..750e0c6 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -159,6 +159,27 @@ provider = MCP::Client::OAuth::Provider.new( ) ``` +### Token Endpoint Errors + +When a token exchange or refresh fails, `MCP::Client::OAuth::Flow::AuthorizationError` includes the HTTP status and +the authorization server's `error` and `error_description` from [RFC 6749 Section 5.2](https://www.rfc-editor.org/rfc/rfc6749#section-5.2). +For example: + +```text +Token endpoint returned status 400. invalid_request: Client must not use multiple authentication methods +``` + +The exception exposes `http_status`, `error`, and `error_description` readers for structured diagnostics. Missing or non-string +diagnostic fields are `nil`; non-JSON responses retain the status-only message. Other authorization failures have `nil` readers. +An `invalid_grant` response still raises `Flow::InvalidGrantError`, a subclass of `Flow::AuthorizationError`, so refresh-token +recovery behavior is unchanged. + +Diagnostic fields are limited to 128 characters for `error` and 512 for `error_description`, including a trailing `...` when +truncated. Characters outside the RFC's printable ASCII set are replaced with spaces, and surrounding whitespace is removed. +The SDK excludes all other response fields, including `error_uri`, and does not include the raw response body in these errors. +Descriptions are provider-controlled text, not guaranteed to be free of sensitive information; apply your application's logging +and redaction policy before persisting them or displaying them to users. + ### Client Credentials Grant For a confidential machine-to-machine client (no user, no browser redirect), use `MCP::Client::OAuth::ClientCredentialsProvider` instead of `Provider`. diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 5b4c0c1..acc0f1c 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -14,7 +14,19 @@ module MCP # `Provider`; this class consumes a Provider plus signal data extracted from # the failing response (resource_metadata URL, scope challenge). class Flow - class AuthorizationError < StandardError; end + TOKEN_ENDPOINT_ERROR_MAX_LENGTH = 128 + TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH = 512 + + class AuthorizationError < StandardError + attr_reader :http_status, :error, :error_description + + def initialize(message = nil, http_status: nil, error: nil, error_description: nil) + super(message) + @http_status = http_status + @error = error + @error_description = error_description + end + end # Raised specifically when the token endpoint rejects a grant with # `error: "invalid_grant"` (RFC 6749 §5.2). Callers use this to @@ -1062,11 +1074,7 @@ module MCP end if response.status < 200 || response.status >= 300 - if token_endpoint_error_code(response) == "invalid_grant" - raise InvalidGrantError, "Token endpoint rejected the grant: invalid_grant." - end - - raise AuthorizationError, "Token endpoint returned status #{response.status}." + raise token_endpoint_error(response) end parsed = begin @@ -1087,17 +1095,35 @@ module MCP parsed end - # Extracts the `error` code from an RFC 6749 §5.2 error response body - # when one is parseable. Returns nil on any parse failure or when - # the body is not JSON. - def token_endpoint_error_code(response) - body = response_body_string(response).to_s - return if body.empty? + # Surface only RFC 6749 §5.2 diagnostic fields, never the raw response, + # which may contain tokens or other credentials. Classify the original + # code so sanitization cannot turn malformed input into invalid_grant. + def token_endpoint_error(response) + message = "Token endpoint returned status #{response.status}." + error_class = AuthorizationError + parsed = JSON.parse(response_body_string(response)) + parsed = {} unless parsed.is_a?(Hash) - parsed = JSON.parse(body) - parsed["error"] if parsed.is_a?(Hash) - rescue JSON::ParserError - nil + error_class = parsed["error"] == "invalid_grant" ? InvalidGrantError : AuthorizationError + error = token_endpoint_diagnostic(parsed["error"], limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH) + description = token_endpoint_diagnostic(parsed["error_description"], limit: TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH) + message += " #{[error, description].compact.join(": ")}" if error || description + + error_class.new(message, http_status: response.status, error: error, error_description: description) + rescue StandardError + # Diagnostics must not mask the endpoint failure or change refresh recovery. + error_class.new("Token endpoint returned status #{response.status}.", http_status: response.status) + end + + def token_endpoint_diagnostic(value, limit:) + return unless value.is_a?(String) + + # RFC 6749 permits printable ASCII except double quotes and backslashes. + # Replace other characters to keep provider text on one log line. + value = value.scrub(" ").gsub(/[^\x20-\x21\x23-\x5B\x5D-\x7E]/, " ").strip + return if value.empty? + + value.length > limit ? "#{value[0, limit - 3]}..." : value end # Per RFC 6749 Section 2.3.1, the `client_id` and `client_secret` MUST be diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index 326dbd8..5daff12 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -2731,7 +2731,161 @@ module MCP provider.save_client_information("client_id" => "test-client") provider.save_tokens("access_token" => "stale-at", "refresh_token" => "revoked-rt") - assert_raises(Flow::InvalidGrantError) do + error = assert_raises(Flow::InvalidGrantError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + assert_equal(400, error.http_status) + assert_equal("invalid_grant", error.error) + assert_equal("refresh token expired", error.error_description) + assert_equal("Token endpoint returned status 400. invalid_grant: refresh token expired", error.message) + end + + def test_token_exchange_preserves_oauth_diagnostics + stub_request(:post, "#{@auth_base}/token").to_return( + status: 400, + body: JSON.generate( + error: "invalid_request", + error_description: "Client must not use multiple authentication methods", + access_token: "do-not-log-access-token", + refresh_token: "do-not-log-refresh-token", + client_secret: "do-not-log-client-secret", + error_uri: "https://auth.example.com/error?secret=do-not-log", + ), + ) + + error = assert_raises(Flow::AuthorizationError) do + capture_authorization_scope(grant_types: ["authorization_code"]) + end + + assert_equal(400, error.http_status) + assert_equal("invalid_request", error.error) + assert_equal("Client must not use multiple authentication methods", error.error_description) + assert_equal( + "Token endpoint returned status 400. invalid_request: Client must not use multiple authentication methods", + error.message, + ) + end + + def test_refresh_preserves_oauth_diagnostics + error = refresh_token_endpoint_error( + JSON.generate(error: "invalid_client", error_description: "Client authentication failed"), + status: 401, + ) + + assert_instance_of(Flow::AuthorizationError, error) + assert_equal(401, error.http_status) + assert_equal("invalid_client", error.error) + assert_equal("Client authentication failed", error.error_description) + assert_equal("Token endpoint returned status 401. invalid_client: Client authentication failed", error.message) + end + + def test_token_endpoint_errors_fall_back_for_malformed_bodies + ["", "secret", "{broken", "null", "[]", '"secret"', "42"].each do |body| + error = refresh_token_endpoint_error(body) + + assert_instance_of(Flow::AuthorizationError, error) + assert_equal(400, error.http_status) + assert_nil(error.error) + assert_nil(error.error_description) + assert_equal("Token endpoint returned status 400.", error.message) + end + end + + def test_token_endpoint_errors_ignore_non_string_and_empty_fields + [nil, 42, true, [], { secret: "hidden" }, "", " \r\n\t"].each do |value| + error = refresh_token_endpoint_error(JSON.generate(error: value, error_description: value)) + + assert_nil(error.error) + assert_nil(error.error_description) + assert_equal("Token endpoint returned status 400.", error.message) + end + end + + def test_token_endpoint_errors_preserve_optional_fields_independently + error = refresh_token_endpoint_error(JSON.generate(error: "provider_extension")) + assert_equal("provider_extension", error.error) + assert_nil(error.error_description) + assert_equal("Token endpoint returned status 400. provider_extension", error.message) + + error = refresh_token_endpoint_error(JSON.generate(error_description: "Details without a code")) + assert_nil(error.error) + assert_equal("Details without a code", error.error_description) + assert_equal("Token endpoint returned status 400. Details without a code", error.message) + end + + def test_token_endpoint_errors_sanitize_without_changing_grant_classification + error = refresh_token_endpoint_error( + JSON.generate(error: "invalid_grant\n", error_description: "expired\r\n\t\e\u0000\"\\\u2028token"), + ) + + assert_instance_of(Flow::AuthorizationError, error) + assert_equal("invalid_grant", error.error) + assert_equal("expired token", error.error_description) + assert_equal("Token endpoint returned status 400. invalid_grant: expired token", error.message) + end + + def test_invalid_grant_with_invalid_utf8_description_preserves_classification + error = refresh_token_endpoint_error( + "{\"error\":\"invalid_grant\",\"error_description\":\"bad \xFF byte\"}".b, + ) + + assert_instance_of(Flow::InvalidGrantError, error) + assert_equal(400, error.http_status) + assert_equal("invalid_grant", error.error) + assert_equal("bad byte", error.error_description) + assert_equal("Token endpoint returned status 400. invalid_grant: bad byte", error.message) + end + + def test_token_endpoint_errors_scrub_invalid_utf8_in_both_fields + error = refresh_token_endpoint_error( + "{\"error\":\"invalid_grant\xFF\",\"error_description\":\"bad \xFF byte\"}".b, + ) + + assert_instance_of(Flow::AuthorizationError, error) + assert_equal(400, error.http_status) + assert_equal("invalid_grant", error.error) + assert_equal("bad byte", error.error_description) + assert_equal("Token endpoint returned status 400. invalid_grant: bad byte", error.message) + end + + def test_token_endpoint_errors_fall_back_when_diagnostic_extraction_raises + Flow.any_instance.stubs(:token_endpoint_diagnostic).raises(ArgumentError, "sensitive provider text") + + { "invalid_grant" => Flow::InvalidGrantError, "invalid_client" => Flow::AuthorizationError }.each do |code, klass| + error = refresh_token_endpoint_error(JSON.generate(error: code, error_description: "details")) + + assert_instance_of(klass, error) + assert_equal(400, error.http_status) + assert_nil(error.error) + assert_nil(error.error_description) + assert_equal("Token endpoint returned status 400.", error.message) + end + end + + def test_token_endpoint_errors_bound_diagnostic_lengths + error = refresh_token_endpoint_error(JSON.generate(error: "e" * 200, error_description: "d" * 1000)) + + assert_equal("#{"e" * 125}...", error.error) + assert_equal("#{"d" * 509}...", error.error_description) + assert_equal("Token endpoint returned status 400. #{error.error}: #{error.error_description}", error.message) + end + + def test_other_authorization_errors_have_no_token_endpoint_diagnostics + error = Flow::AuthorizationError.new("Discovery failed") + + assert_equal("Discovery failed", error.message) + assert_nil(error.http_status) + assert_nil(error.error) + assert_nil(error.error_description) + end + + def refresh_token_endpoint_error(body, status: 400) + stub_request(:post, "#{@auth_base}/token").to_return(status: status, body: body) + provider = ssrf_test_provider + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + assert_raises(Flow::AuthorizationError) do Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) end end From 99ef8732feb46e79713e59ee63866bbba9687409 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Tue, 15 Sep 2026 12:33:24 +0900 Subject: [PATCH 10/17] Fix a redundant regexp escape in the tool name validation ## Motivation and Context This PR suppresses the following offense of RuboCop. ```console $ bundle exec rubocop Inspecting 163 files (snip) Offenses: lib/mcp/tool.rb:152:48: C: [Correctable] Style/RedundantRegexpEscape: Redundant escape inside regexp literal unless tool_name.match?(/\A[A-Za-z\d_\-\.]+\z/) ^^ 163 files inspected, 1 offense detected, 1 offense autocorrectable ``` ## How Has This Been Tested? `bundle exec rake` passes: RuboCop reports no offenses. ## Breaking Changes None. --- lib/mcp/tool.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mcp/tool.rb b/lib/mcp/tool.rb index 37c5d92..1b76b80 100644 --- a/lib/mcp/tool.rb +++ b/lib/mcp/tool.rb @@ -149,7 +149,7 @@ module MCP raise ArgumentError, "Tool names should be between 1 and 128 characters in length (inclusive)." end - unless tool_name.match?(/\A[A-Za-z\d_\-\.]+\z/) + unless tool_name.match?(/\A[A-Za-z\d_\-.]+\z/) raise ArgumentError, <<~MESSAGE Tool names only allowed characters: uppercase and lowercase ASCII letters (A-Z, a-z), digits (0-9), underscore (_), hyphen (-), and dot (.). MESSAGE From 8ad80dd0b48fc7ad49befaea5e84f5b08e5bf1de Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 17 Sep 2026 00:52:10 +0900 Subject: [PATCH 11/17] Let a provider add parameters to the token requests it makes ## Motivation and Context `ClientCredentialsProvider` builds a fixed `client_credentials` token request: `grant_type`, `scope`, `resource`, and the client authentication that `Flow#post_to_token_endpoint` adds. An authorization server that requires further parameters, such as Auth0's `audience`, cannot be used through `oauth:` at all, and giving up `oauth:` also gives up the bearer header and the 401-driven retry that the transport provides. RFC 6749 Section 8.2 allows extension parameters on the token request, and the TypeScript SDK lets a provider shape its token request through `prepareTokenRequest`. `ClientCredentialsProvider.new` now takes `token_request_params:`, a Hash of String keys and values added to every token request the provider makes, and `Flow#post_to_token_endpoint` reads the same name by duck typing, so a `Provider` or `CrossAppAccessProvider` subclass that defines the method gets the same treatment on its authorization code exchange, refresh, or `jwt-bearer` request. `Provider.new` and `CrossAppAccessProvider.new` take the same keyword, held by `StorageBackedProvider` like `authorization_request_validator`, so every bundled provider refuses a bad value before anything is stored. The parameters go underneath the flow's own, so the SDK's values always win, and a key the SDK sets itself (`Flow::RESERVED_TOKEN_REQUEST_PARAMS`) is refused rather than silently overridden: with `ArgumentError` from the constructor, before any `client_information` is written, and again with `ArgumentError` from the flow, before the token request is sent. Only Strings are accepted because `URI.encode_www_form` encodes other scalars and Arrays in ways the caller did not write, and the provider keeps a frozen copy so a later change to the caller's Hash cannot alter what is sent. A Hash comparing keys by identity is refused too, since two equal keys are two entries there, sent twice by a provider method or silently collapsed into one by the copy. The flow raises `ArgumentError` rather than `AuthorizationError` because `MCP::Client::HTTP` treats a failed refresh as a reason to run the interactive flow, which would then fail the same way after the user signed in. Both refusals raise `Flow::InvalidTokenRequestParamsError`, a subclass of `ArgumentError`, so a caller can tell them from an unknown keyword, and the copy duplicates keys as well as values, since `Hash` copies only keys whose class is exactly `String`. Fixes #554. ## How Has This Been Tested? New tests cover every token request the parameters can ride (the `client_credentials` grant under all three client authentication methods, the authorization code exchange, refresh, and `jwt-bearer`), both refusal boundaries (the constructors of all three providers, and the flow for a provider defining the method), and the transport surfacing a refused hook from a refresh attempt without starting the interactive flow. All of them fail against the previous library. ## Breaking Changes None. The keyword is optional and the hook is opt-in; a provider that does not define `token_request_params` sends the same requests as before. --- docs/_client/authorization.md | 14 +- .../oauth/client_credentials_provider.rb | 18 +- .../client/oauth/cross_app_access_provider.rb | 15 +- lib/mcp/client/oauth/flow.rb | 64 +++++ lib/mcp/client/oauth/provider.rb | 9 +- .../client/oauth/storage_backed_provider.rb | 32 ++- .../oauth/client_credentials_provider_test.rb | 101 ++++++++ .../oauth/cross_app_access_provider_test.rb | 36 +++ test/mcp/client/oauth/flow_test.rb | 243 +++++++++++++++++- test/mcp/client/oauth/http_oauth_test.rb | 51 ++++ test/mcp/client/oauth/provider_test.rb | 24 ++ 11 files changed, 583 insertions(+), 24 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index 750e0c6..b35b597 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -112,6 +112,14 @@ Optional keyword arguments: served at the URL is a separate JSON artifact from the `client_metadata` keyword above: the DCR `client_metadata` MUST NOT include `client_id`, while the CIMD document MUST include `client_id` set to the document URL, `client_name`, and `redirect_uris` covering `redirect_uri`. +- `token_request_params`: Hash of String keys and values added to every token request the provider makes, + for parameters the authorization server requires beyond the grant itself, such as Auth0's `audience`. + Defaults to `nil`, which adds nothing. The authorization request is not affected. + A key the SDK sets itself (listed in `Flow::RESERVED_TOKEN_REQUEST_PARAMS`), a Hash that compares keys by identity, + or a value that is not a Hash of Strings is refused with `Flow::InvalidTokenRequestParamsError`, a subclass of `ArgumentError`, + when the provider is built, and the accepted Hash is copied and frozen. + Any provider that defines a `token_request_params` method gets the same treatment on every token request it makes; + the flow checks the returned value by the same rules and raises the same error before the token request is sent. {: .warning } > The OAuth 2.0 Dynamic Client Registration Protocol (RFC 7591) is deprecated as a client registration mechanism as of MCP 2026-07-28 in favor of Client ID Metadata Documents, @@ -192,6 +200,7 @@ provider = MCP::Client::OAuth::ClientCredentialsProvider.new( client_secret: ENV.fetch("MCP_CLIENT_SECRET"), # token_endpoint_auth_method: "client_secret_basic" (default), "client_secret_post", or "private_key_jwt" # scope: "mcp:read mcp:write" (optional; used when the server does not advertise scopes) + # token_request_params: { "audience" => "https://api.example.com" } (optional; parameters the authorization server requires) ) transport = MCP::Client::HTTP.new(url: "https://api.example.com/mcp", oauth: provider) @@ -207,7 +216,8 @@ 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. -- `scope`, `storage`, `authorization_request_validator`: Optional, same meaning as on `Provider`. +- `scope`, `storage`, `authorization_request_validator`, `token_request_params`: 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`. ### Cross-App Access (JWT Bearer) Grant @@ -244,7 +254,7 @@ Keyword arguments: - `assertion_provider`: Required. Callable invoked as `call(audience:, resource:)` and returning the ID-JAG assertion. `audience` is the MCP authorization server's validated issuer identifier; `resource` is the canonical MCP server URL (RFC 8707). Passing both through to `IDJAGTokenExchange.request` covers the common case. -- `scope`, `storage`, `authorization_request_validator`: Optional, same meaning as on `Provider`. +- `scope`, `storage`, `authorization_request_validator`, `token_request_params`: Optional, same meaning as on `Provider`. ### Communication Security diff --git a/lib/mcp/client/oauth/client_credentials_provider.rb b/lib/mcp/client/oauth/client_credentials_provider.rb index 21276bb..22ef56a 100644 --- a/lib/mcp/client/oauth/client_credentials_provider.rb +++ b/lib/mcp/client/oauth/client_credentials_provider.rb @@ -37,11 +37,15 @@ module MCP # also take the algorithm as an explicit option). # - `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)`. Defaults to - # an `InMemoryStorage`. The `client_id` / `client_secret` are written - # into it so the token exchange reads them through the same path as - # a pre-registered authorization-code client. + # - `storage` - Object responding to `tokens`, `save_tokens(tokens)`, `client_information`, + # and `save_client_information(info)`. Defaults to an `InMemoryStorage`. + # The `client_id` / `client_secret` are written into it so the token exchange reads + # them through the same path as a pre-registered authorization-code client. + # - `token_request_params` - Hash of String keys and values added to every + # token request this provider makes, for parameters the authorization + # server requires beyond the grant itself (Auth0's `audience`, for example). + # A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` raises `Flow::InvalidTokenRequestParamsError`. + # The Hash is copied and frozen. See `StorageBackedProvider#token_request_params`. class ClientCredentialsProvider include StorageBackedProvider @@ -61,7 +65,8 @@ module MCP signing_algorithm: nil, scope: nil, storage: nil, - authorization_request_validator: nil + authorization_request_validator: nil, + token_request_params: nil ) if blank?(client_id) raise InvalidCredentialsError, "client_id is required for the client_credentials grant." @@ -105,6 +110,7 @@ module MCP @scope = scope @storage = storage || InMemoryStorage.new @authorization_request_validator = authorization_request_validator + @token_request_params = frozen_token_request_params(token_request_params) @storage.save_client_information(client_information) end diff --git a/lib/mcp/client/oauth/cross_app_access_provider.rb b/lib/mcp/client/oauth/cross_app_access_provider.rb index 67342c0..422cbb5 100644 --- a/lib/mcp/client/oauth/cross_app_access_provider.rb +++ b/lib/mcp/client/oauth/cross_app_access_provider.rb @@ -25,6 +25,10 @@ module MCP # the Protected Resource Metadata do not specify one. # - `storage` - Object responding to `tokens`, `save_tokens(tokens)`, `client_information`, and `save_client_information(info)`. # Defaults to an `InMemoryStorage`. + # - `token_request_params` - Hash of String keys and values added to the `jwt-bearer` token request, for parameters + # the authorization server requires beyond the grant itself. A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` raises + # `Flow::InvalidTokenRequestParamsError`. The Hash is copied and frozen. + # See `StorageBackedProvider#token_request_params`. # # https://github.com/modelcontextprotocol/modelcontextprotocol/issues/990 class CrossAppAccessProvider @@ -35,7 +39,15 @@ module MCP attr_reader :scope, :storage - def initialize(client_id:, client_secret:, assertion_provider:, scope: nil, storage: nil, authorization_request_validator: nil) + def initialize( + client_id:, + client_secret:, + assertion_provider:, + scope: nil, + storage: nil, + authorization_request_validator: nil, + token_request_params: nil + ) if blank?(client_id) raise InvalidConfigurationError, "client_id is required for the jwt-bearer grant." end @@ -52,6 +64,7 @@ module MCP @scope = scope @storage = storage || InMemoryStorage.new @authorization_request_validator = authorization_request_validator + @token_request_params = frozen_token_request_params(token_request_params) @storage.save_client_information( "client_id" => client_id, "client_secret" => client_secret, diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index acc0f1c..1093eba 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -17,6 +17,23 @@ module MCP TOKEN_ENDPOINT_ERROR_MAX_LENGTH = 128 TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH = 512 + # Token request parameters the flow sets itself. Its values win over a provider's `token_request_params`, + # so a provider naming one of these is refused rather than left believing its value was sent. + RESERVED_TOKEN_REQUEST_PARAMS = [ + "grant_type", + "client_id", + "client_secret", + "client_assertion", + "client_assertion_type", + "scope", + "resource", + "code", + "code_verifier", + "redirect_uri", + "refresh_token", + "assertion", + ].freeze + class AuthorizationError < StandardError attr_reader :http_status, :error, :error_description @@ -40,6 +57,33 @@ module MCP # or authorization server metadata failure by rescuing a class rather than by matching the message text. class AuthorizationRefusedError < AuthorizationError; end + # Raised for a `token_request_params` value the SDK refuses: a reserved key, a Hash comparing keys by identity, + # or anything but a Hash of Strings. An `ArgumentError` because the value is a configuration mistake, + # not a failed authorization, and deliberately outside `AuthorizationError`, which `MCP::Client::HTTP` treats on + # a failed refresh as a reason to run the interactive flow. + class InvalidTokenRequestParamsError < ArgumentError; end + + class << self + # Returns why `params` cannot ride a token request as `token_request_params`, or `nil` when it can. + # Shared by the provider constructors and the flow, which both refuse the value with `InvalidTokenRequestParamsError`, + # so the same problem reads the same wherever it surfaces. + def token_request_params_problem(params) + return "must be a Hash (got #{params.class})." unless params.is_a?(Hash) + + # Two equal keys are two entries here, which would be sent twice from a provider method + # or silently collapse into one when the constructors copy the Hash. + return "must not compare keys by identity." if params.compare_by_identity? + + params.each do |key, value| + return "keys must be Strings (got #{key.class})." unless key.is_a?(String) + return "values must be Strings (got #{value.class} for #{key.inspect})." unless value.is_a?(String) + return "must not set #{key.inspect}, which the SDK sets itself." if RESERVED_TOKEN_REQUEST_PARAMS.include?(key) + end + + nil + end + end + def initialize(provider:, http_client_factory: nil) @provider = provider @http_client_factory = http_client_factory || -> { default_http_client } @@ -960,6 +1004,22 @@ module MCP @provider.authorization_flow end + # Parameters the provider adds to every token request it makes (RFC 6749 Section 8.2 leaves room for them; + # Auth0's `audience` is the usual one). Duck-typed like `authorization_flow`, so a provider without the method, + # or one returning `nil`, adds nothing. + # A bad value raises `InvalidTokenRequestParamsError`, as the provider constructors do. + def provider_token_request_params + return {} unless @provider.respond_to?(:token_request_params) + + params = @provider.token_request_params + return {} if params.nil? + + problem = self.class.token_request_params_problem(params) + raise InvalidTokenRequestParamsError, "The provider's token_request_params #{problem}" if problem + + params + end + def build_authorization_url(as_metadata:, client_id:, scope:, state:, code_challenge:, resource:) authorization_endpoint = as_metadata["authorization_endpoint"] unless authorization_endpoint @@ -1012,6 +1072,9 @@ module MCP # Submits a form-encoded token request using the authentication method # stored in `client_information`. The method determines whether client # credentials belong in the form body, a Basic header, or a JWT assertion. + # A provider's `token_request_params` go underneath the flow's own parameters, + # which therefore win, and are refused before the request is sent when they name + # a reserved parameter or are not a Hash of Strings. def post_to_token_endpoint(as_metadata:, client_info:, form:) client_id = client_info_required_value(client_info, "client_id") unless client_id @@ -1021,6 +1084,7 @@ module MCP client_secret = client_info_required_value(client_info, "client_secret") token_endpoint_auth_method = client_info_value(client_info, "token_endpoint_auth_method") + form = provider_token_request_params.merge(form) # Apply one client authentication method per request (RFC 6749 Section 2.3). headers = {} diff --git a/lib/mcp/client/oauth/provider.rb b/lib/mcp/client/oauth/provider.rb index 7d1a0f6..16f8c4e 100644 --- a/lib/mcp/client/oauth/provider.rb +++ b/lib/mcp/client/oauth/provider.rb @@ -48,6 +48,11 @@ module MCP # The document served at the URL is a separate JSON artifact from the `client_metadata` keyword: # DCR `client_metadata` MUST NOT include `client_id`, while the CIMD document MUST include `client_id` set # to the URL, `client_name`, and `redirect_uris` covering `redirect_uri`. + # - `token_request_params` - Hash of String keys and values added to every token request this provider makes + # (the authorization code exchange and refresh), for parameters the authorization server requires beyond + # the grant itself. The authorization request is not affected. A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` + # raises `Flow::InvalidTokenRequestParamsError`. + # The Hash is copied and frozen. See `StorageBackedProvider#token_request_params`. class Provider include StorageBackedProvider @@ -87,7 +92,8 @@ module MCP scope: nil, storage: nil, client_id_metadata_document_url: nil, - authorization_request_validator: nil + authorization_request_validator: nil, + token_request_params: nil ) unless Discovery.secure_url?(redirect_uri) raise InsecureRedirectURIError, @@ -117,6 +123,7 @@ module MCP @storage = storage || InMemoryStorage.new @client_id_metadata_document_url = client_id_metadata_document_url @authorization_request_validator = authorization_request_validator + @token_request_params = frozen_token_request_params(token_request_params) end # Identifies the OAuth flow this provider drives. diff --git a/lib/mcp/client/oauth/storage_backed_provider.rb b/lib/mcp/client/oauth/storage_backed_provider.rb index 44d09a1..2be8acc 100644 --- a/lib/mcp/client/oauth/storage_backed_provider.rb +++ b/lib/mcp/client/oauth/storage_backed_provider.rb @@ -4,11 +4,11 @@ module MCP class Client module OAuth # Shared token/credential persistence for the OAuth provider classes - # (`Provider` for the authorization-code flow and `ClientCredentialsProvider` - # for the client_credentials flow). The two grants differ in how they authenticate, - # but both read and write the same two pieces of state through a `storage` object: - # the token response and the client information. This module supplies that delegation - # so the `Flow` orchestrator can treat any provider uniformly. + # (`Provider` for the authorization-code flow, `ClientCredentialsProvider` + # for the client_credentials flow, and `CrossAppAccessProvider` for the jwt-bearer flow). + # The grants differ in how they authenticate, but all read and write the same two pieces of state + # through a `storage` object: the token response and the client information. This module supplies + # that delegation so the `Flow` orchestrator can treat any provider uniformly. # # Including classes must set `@storage` to an object responding to `tokens`, # `save_tokens(tokens)`, `client_information`, and `save_client_information(info)` @@ -21,6 +21,13 @@ module MCP # MCP SDK has today. attr_reader :authorization_request_validator + # Optional Hash of String keys and values added to every token request the provider makes, for parameters + # the authorization server requires beyond the grant itself (RFC 6749 Section 8.2 leaves room for them; + # Auth0's `audience` is the usual one). `nil` (the default) adds nothing. + # Set through `frozen_token_request_params`, which refuses a key `Flow` sets itself or a wrongly shaped value + # with `Flow::InvalidTokenRequestParamsError`. + attr_reader :token_request_params + def access_token tokens&.dig("access_token") || tokens&.dig(:access_token) end @@ -44,6 +51,21 @@ module MCP def clear_tokens! @storage.save_tokens(nil) end + + private + + # Constructors call this before writing to `storage`, so a rejected provider leaves it untouched. + # The copy has its own frozen keys and values, so a later change to the caller's Hash, to a key, + # or to a value cannot alter what is sent to the token endpoint. Keys are copied explicitly + # because `Hash` copies and freezes only keys whose class is exactly `String`. + def frozen_token_request_params(params) + return if params.nil? + + problem = Flow.token_request_params_problem(params) + raise Flow::InvalidTokenRequestParamsError, "token_request_params #{problem}" if problem + + params.each_with_object({}) { |(key, value), copy| copy[key.dup.freeze] = value.dup.freeze }.freeze + end end end end diff --git a/test/mcp/client/oauth/client_credentials_provider_test.rb b/test/mcp/client/oauth/client_credentials_provider_test.rb index c112aba..5ff4d20 100644 --- a/test/mcp/client/oauth/client_credentials_provider_test.rb +++ b/test/mcp/client/oauth/client_credentials_provider_test.rb @@ -7,6 +7,23 @@ module MCP class Client module OAuth class ClientCredentialsProviderTest < Minitest::Test + # Every parameter the flow sets on a token request, spelled out rather than read from + # `Flow::RESERVED_TOKEN_REQUEST_PARAMS`, so a key dropped from that constant fails here. + RESERVED_TOKEN_REQUEST_PARAMS = [ + "grant_type", + "client_id", + "client_secret", + "client_assertion", + "client_assertion_type", + "scope", + "resource", + "code", + "code_verifier", + "redirect_uri", + "refresh_token", + "assertion", + ].freeze + def test_initialize_stores_credentials_as_client_information provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret") @@ -160,6 +177,90 @@ module MCP assert_equal("cc-client", claims["sub"]) assert_equal("https://auth.example.com", claims["aud"]) end + + def test_token_request_params_is_nil_by_default + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret") + + assert_nil(provider.token_request_params) + end + + def test_initialize_keeps_a_frozen_copy_of_token_request_params + params = { "audience" => +"https://api.example.com" } + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", token_request_params: params) + + params["audience"] << "/changed" + params["organization"] = "org_123" + + assert_equal({ "audience" => "https://api.example.com" }, provider.token_request_params) + assert_predicate(provider.token_request_params, :frozen?) + assert_predicate(provider.token_request_params["audience"], :frozen?) + end + + def test_initialize_rejects_token_request_params_that_the_sdk_sets_itself + RESERVED_TOKEN_REQUEST_PARAMS.each do |key| + error = assert_raises(Flow::InvalidTokenRequestParamsError, "should reject #{key.inspect}") do + ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", token_request_params: { key => "x" }) + end + + assert_includes(error.message, key.inspect) + end + end + + def test_initialize_rejects_token_request_params_that_are_not_a_hash_of_strings + ["audience=x", { audience: "x" }, { "audience" => 1 }, { "audience" => nil }].each do |params| + assert_raises(Flow::InvalidTokenRequestParamsError, "should reject #{params.inspect}") do + ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", token_request_params: params) + end + end + end + + def test_initialize_rejects_token_request_params_that_compare_keys_by_identity + # Two equal keys are two entries in such a Hash and would silently collapse into one when copied. + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + ClientCredentialsProvider.new( + client_id: "cc-client", + client_secret: "cc-secret", + token_request_params: { "audience" => "x" }.compare_by_identity, + ) + end + + assert_match(/identity/, error.message) + end + + def test_initialize_copies_keys_that_hash_would_share_with_the_caller + # `Hash` copies and freezes only keys whose class is exactly `String`; a subclass key would otherwise + # stay shared with the caller, and a later `replace` would turn it into a reserved name. + key = Class.new(String).new("audience") + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", token_request_params: { key => "x" }) + + key.replace("grant_type") + + assert_equal({ "audience" => "x" }, provider.token_request_params) + refute_same(key, provider.token_request_params.keys.first) + assert_predicate(provider.token_request_params.keys.first, :frozen?) + end + + def test_initialize_keeps_empty_token_request_params_as_a_frozen_empty_hash + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", token_request_params: {}) + + assert_equal({}, provider.token_request_params) + assert_predicate(provider.token_request_params, :frozen?) + end + + def test_initialize_writes_no_client_information_when_token_request_params_are_rejected + storage = InMemoryStorage.new + + assert_raises(Flow::InvalidTokenRequestParamsError) do + ClientCredentialsProvider.new( + client_id: "cc-client", + client_secret: "cc-secret", + storage: storage, + token_request_params: { "grant_type" => "password" }, + ) + end + + assert_nil(storage.client_information) + end end 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 deb7e4b..1283b62 100644 --- a/test/mcp/client/oauth/cross_app_access_provider_test.rb +++ b/test/mcp/client/oauth/cross_app_access_provider_test.rb @@ -88,6 +88,42 @@ module MCP provider.clear_tokens! assert_nil(provider.tokens) end + + def test_token_request_params_is_nil_by_default + assert_nil(build_provider.token_request_params) + end + + def test_initialize_keeps_a_frozen_copy_of_token_request_params + params = { "audience" => +"https://api.example.com" } + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag" }, + token_request_params: params, + ) + + params["audience"] << "/changed" + + assert_equal({ "audience" => "https://api.example.com" }, provider.token_request_params) + assert_predicate(provider.token_request_params, :frozen?) + end + + def test_initialize_writes_no_client_information_when_token_request_params_are_rejected + storage = InMemoryStorage.new + + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag" }, + storage: storage, + token_request_params: { "assertion" => "x" }, + ) + end + + assert_includes(error.message, '"assertion"') + assert_nil(storage.client_information) + end end end end diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index 5daff12..c463fdc 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -85,14 +85,62 @@ module MCP ) end - def client_credentials_provider(token_endpoint_auth_method: "client_secret_basic") + def client_credentials_provider(token_endpoint_auth_method: "client_secret_basic", token_request_params: nil) ClientCredentialsProvider.new( client_id: "cc-client", client_secret: "cc-secret", token_endpoint_auth_method: token_endpoint_auth_method, + token_request_params: token_request_params, ) end + # An authorization-code provider that drives the code exchange and refresh with the given parameters. + def provider_with_token_request_params(params, redirect_handler: ->(_url) {}, callback_handler: -> { [nil, nil] }) + Provider.new(**authorization_code_provider_arguments(redirect_handler, callback_handler), token_request_params: params) + end + + # Overriding the reader bypasses the constructor's validation, leaving the flow's own check to be exercised. + def provider_returning_token_request_params(params, redirect_handler: ->(_url) {}, callback_handler: -> { [nil, nil] }) + provider_class = Class.new(Provider) do + define_method(:token_request_params) { params } + end + + provider_class.new(**authorization_code_provider_arguments(redirect_handler, callback_handler)) + end + + def authorization_code_provider_arguments(redirect_handler, callback_handler) + { + 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: redirect_handler, + callback_handler: callback_handler, + } + end + + # See `provider_returning_token_request_params`. + def client_credentials_provider_returning(token_request_params:) + provider_class = Class.new(ClientCredentialsProvider) do + define_method(:token_request_params) { token_request_params } + end + + provider_class.new(client_id: "cc-client", client_secret: "cc-secret") + end + + # `OpenSSL::PKey::EC.generate` only exists from the openssl gem 2.2 (Ruby 3.0); + # fall back to the pre-3.0 API on older Rubies. + def generate_es256_key + if OpenSSL::PKey::EC.respond_to?(:generate) + OpenSSL::PKey::EC.generate("prime256v1") + else + OpenSSL::PKey::EC.new("prime256v1").tap(&:generate_key) + end + end + # Runs the full authorization flow and returns the `scope` query parameter # sent on the authorization request. The caller stubs the AS metadata; # this helper supplies a provider whose `grant_types` and optional pre-set @@ -250,17 +298,10 @@ module MCP ), ) - # `OpenSSL::PKey::EC.generate` only exists from the openssl gem 2.2 (Ruby 3.0); - # fall back to the pre-3.0 API on older Rubies. - key = if OpenSSL::PKey::EC.respond_to?(:generate) - OpenSSL::PKey::EC.generate("prime256v1") - else - OpenSSL::PKey::EC.new("prime256v1").tap(&:generate_key) - end provider = ClientCredentialsProvider.new( client_id: "cc-client", token_endpoint_auth_method: "private_key_jwt", - private_key: key, + private_key: generate_es256_key, signing_algorithm: "ES256", ) @@ -285,6 +326,190 @@ module MCP end end + def test_run_client_credentials_sends_token_request_params_with_client_secret_basic + provider = client_credentials_provider(token_request_params: { "audience" => "https://api.example.com" }) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "client_credentials" && + form["resource"] == "https://srv.example.com/mcp" && + form["audience"] == "https://api.example.com" && + !form.key?("client_id") && + req.headers["Authorization"] == "Basic " + Base64.strict_encode64("cc-client:cc-secret") + end + end + + def test_run_client_credentials_sends_token_request_params_returned_by_a_provider_method + provider = client_credentials_provider_returning(token_request_params: { "audience" => "https://api.example.com" }) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + URI.decode_www_form(req.body).to_h["audience"] == "https://api.example.com" + end + end + + def test_run_client_credentials_sends_the_same_request_for_empty_token_request_params + provider = client_credentials_provider(token_request_params: {}) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + URI.decode_www_form(req.body).map(&:first) == ["grant_type", "resource"] + end + end + + def test_run_client_credentials_sends_token_request_params_with_client_secret_post + provider = client_credentials_provider( + token_endpoint_auth_method: "client_secret_post", + token_request_params: { "audience" => "https://api.example.com" }, + ) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["audience"] == "https://api.example.com" && + form["client_id"] == "cc-client" && + form["client_secret"] == "cc-secret" + end + end + + def test_run_client_credentials_sends_token_request_params_with_private_key_jwt + provider = ClientCredentialsProvider.new( + client_id: "cc-client", + token_endpoint_auth_method: "private_key_jwt", + private_key: generate_es256_key, + signing_algorithm: "ES256", + token_request_params: { "audience" => "https://api.example.com" }, + ) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["audience"] == "https://api.example.com" && + form["client_assertion_type"] == "urn:ietf:params:oauth:client-assertion-type:jwt-bearer" && + !form["client_assertion"].to_s.empty? + end + end + + def test_run_sends_token_request_params_on_the_authorization_code_exchange + state_value = nil + provider = provider_with_token_request_params( + { "audience" => "https://api.example.com" }, + redirect_handler: ->(url) { state_value = URI.decode_www_form(url.query).to_h.fetch("state") }, + callback_handler: -> { ["test-auth-code", state_value] }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "authorization_code" && + form["code"] == "test-auth-code" && + form["audience"] == "https://api.example.com" + end + end + + def test_refresh_sends_token_request_params + provider = provider_with_token_request_params({ "audience" => "https://api.example.com" }) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + result = Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:refreshed, result) + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "refresh_token" && + form["refresh_token"] == "saved-rt" && + form["audience"] == "https://api.example.com" + end + end + + def test_run_sends_token_request_params_on_the_jwt_bearer_grant + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag-assertion" }, + token_request_params: { "audience" => "https://api.example.com" }, + ) + + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "urn:ietf:params:oauth:grant-type:jwt-bearer" && + form["assertion"] == "id-jag-assertion" && + form["audience"] == "https://api.example.com" + end + end + + def test_run_refuses_token_request_params_that_name_a_reserved_parameter + provider = client_credentials_provider_returning(token_request_params: { "grant_type" => "password" }) + + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/"grant_type"/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_refuses_token_request_params_that_are_not_a_hash_of_strings + shapes = ["audience=x", { audience: "x" }, { "audience" => 1 }, { "audience" => nil }, { "audience" => "x" }.compare_by_identity] + shapes.each do |params| + provider = client_credentials_provider_returning(token_request_params: params) + + assert_raises(Flow::InvalidTokenRequestParamsError, "should refuse #{params.inspect}") do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + end + + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_refresh_refuses_token_request_params_that_name_a_reserved_parameter + # Not an `AuthorizationError`: the transport treats a failed refresh as a reason + # to run the interactive flow, which would fail the same way afterwards. + provider = provider_returning_token_request_params({ "refresh_token" => "other" }) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/"refresh_token"/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_refuses_token_request_params_on_the_authorization_code_exchange + state_value = nil + provider = provider_returning_token_request_params( + { "code" => "other" }, + redirect_handler: ->(url) { state_value = URI.decode_www_form(url.query).to_h.fetch("state") }, + callback_handler: -> { ["test-auth-code", state_value] }, + ) + + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/"code"/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + def test_run_uses_jwt_bearer_grant_for_cross_app_access_provider # SEP-990: the ID-JAG assertion (obtained out of band, typically via IdP token exchange) is presented at # the token endpoint with client_secret_basic; no PKCE, redirect, or DCR is involved. diff --git a/test/mcp/client/oauth/http_oauth_test.rb b/test/mcp/client/oauth/http_oauth_test.rb index 9970971..5b1be13 100644 --- a/test/mcp/client/oauth/http_oauth_test.rb +++ b/test/mcp/client/oauth/http_oauth_test.rb @@ -627,6 +627,57 @@ module MCP assert_equal("refreshed-token", provider.access_token) end + def test_send_request_surfaces_a_bad_token_request_params_hook_instead_of_reauthorizing + # A provider whose `token_request_params` names a reserved key is misconfigured, not unauthorized: + # the refresh attempt must raise rather than fall through to the interactive flow, + # which would fail the same way after the user signed in. + stub_request(:post, @mcp_url).to_return( + status: 401, + headers: { "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}") }, + body: "", + ) + + stub_request(:get, @prm_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + + stub_request(:get, "#{@auth_base}/.well-known/oauth-authorization-server").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", + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + ), + ) + + redirected = false + provider_class = Class.new(Provider) do + define_method(:token_request_params) { { "grant_type" => "password" } } + end + provider = provider_class.new( + client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(_url) { redirected = true }, + callback_handler: -> { ["code", "state"] }, + ) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-token", "refresh_token" => "saved-rt") + + transport = HTTP.new(url: @mcp_url, oauth: provider) + + assert_raises(Flow::InvalidTokenRequestParamsError) do + transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + end + refute(redirected) + assert_not_requested(:post, "#{@auth_base}/token") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + def test_send_request_preserves_refresh_token_when_refresh_hits_a_transient_failure # A 5xx (or any non-`invalid_grant`) from the token endpoint indicates # a transient AS outage, NOT that the refresh token is dead. diff --git a/test/mcp/client/oauth/provider_test.rb b/test/mcp/client/oauth/provider_test.rb index 3db9427..d9fa64e 100644 --- a/test/mcp/client/oauth/provider_test.rb +++ b/test/mcp/client/oauth/provider_test.rb @@ -187,6 +187,30 @@ module MCP assert_equal(:authorization_code, provider.authorization_flow) end + + def test_token_request_params_is_nil_by_default + provider = Provider.new(**args_for("https://app.example.com/callback")) + + assert_nil(provider.token_request_params) + end + + def test_initialize_keeps_a_frozen_copy_of_token_request_params + params = { "audience" => +"https://api.example.com" } + provider = Provider.new(**args_for("https://app.example.com/callback"), token_request_params: params) + + params["audience"] << "/changed" + + assert_equal({ "audience" => "https://api.example.com" }, provider.token_request_params) + assert_predicate(provider.token_request_params, :frozen?) + end + + def test_initialize_rejects_token_request_params_that_the_sdk_sets_itself + error = assert_raises(Flow::InvalidTokenRequestParamsError) do + Provider.new(**args_for("https://app.example.com/callback"), token_request_params: { "code" => "x" }) + end + + assert_includes(error.message, '"code"') + end end end end From 47eb74aa7aea5db27704e815a08a9082cd4b727b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 17 Sep 2026 12:30:05 +0900 Subject: [PATCH 12/17] Validate the issuer of legacy authorization server metadata ## Motivation and Context On the legacy 2025-03-26 path, taken when a server publishes no Protected Resource Metadata, the authorization server metadata is fetched from the MCP server's origin, which that spec names as the authorization base URL, but its `issuer` was never compared with that origin. RFC 8414 Section 3.3 requires the comparison, the 2025-03-26 spec makes RFC 8414 a MUST for clients, and the TypeScript and Python SDKs both compare on this path, tolerating only a trailing slash. The flow nevertheless used the unverified `issuer` as the authorization server's identity everywhere: tokens were stamped with it, stored client information was matched against it, the `private_key_jwt` and ID-JAG assertions took it as their audience, and `authorization_request_validator` was shown it. A server on that path could therefore claim any issuer. Claiming the one that minted the stored tokens made `refresh!` send the refresh token and the stored client secret to the server's own token endpoint, claiming the one that issued stored client information made the flow reuse those credentials there, and a validator allow-listing the claimed issuer approved the request. The legacy path now applies the same check as the modern one: the document's `issuer` must be the origin, rendered with or without a trailing slash, and any other document is refused before registration, redirect, or token request. A server without a metadata document still gets the spec's default endpoints. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb` serve legacy metadata at the MCP server origin that claims `https://auth.example.com` and check that the flow, a refresh of tokens issued by that server, and a flow holding client information bound to it are all refused before any request, with the validator never asked and the credentials never presented. Metadata naming the origin, with or without a trailing slash and with endpoints under a path prefix, completes the flow, including the RFC 9207 `iss` check. The former test of a path-prefix issuer now expects the refusal. The refusal tests fail against the previous library. ## Breaking Changes A server without Protected Resource Metadata whose authorization server metadata names an issuer other than its origin is refused instead of used. The TypeScript and Python SDKs already refuse such a server, and the 2025-03-26 spec places the metadata at the origin. --- docs/_client/authorization.md | 2 +- lib/mcp/client/oauth/flow.rb | 28 +++++- test/mcp/client/oauth/flow_test.rb | 150 ++++++++++++++++++++++++++++- 3 files changed, 169 insertions(+), 11 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index 750e0c6..e3c0709 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -37,7 +37,7 @@ pass an `MCP::Client::OAuth::Provider` to the transport instead of a static `Aut - On a `401 Unauthorized`, parse the `WWW-Authenticate` header, discover the authorization server (Protected Resource Metadata + RFC 8414 Authorization Server Metadata), perform Dynamic Client Registration if needed, run the OAuth 2.1 Authorization Code flow with PKCE (S256), and retry the failed request with the acquired token. - Fall back to the legacy 2025-03-26 discovery when the server publishes no Protected Resource Metadata, matching the TypeScript and Python SDKs: the MCP server's origin acts - as the authorization base URL, its metadata is fetched from `/.well-known/oauth-authorization-server` without the RFC 8414 issuer byte-match (which the legacy spec predates), + as the authorization base URL, its metadata is fetched from `/.well-known/oauth-authorization-server` and must name that origin as its `issuer` (RFC 8414 Section 3.3), and when even that is absent the spec's default endpoints `/authorize`, `/token`, and `/register` at the origin are used with PKCE S256 assumed. - On subsequent 401s with a saved `refresh_token`, exchange it at the token endpoint before falling back to the full interactive flow (RFC 6749 Section 6). - On a `403 Forbidden` whose `WWW-Authenticate` header carries `error="insufficient_scope"` (OAuth 2.0 step-up, RFC 6750 Section 3.1 and the MCP scope-selection-strategy), diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index acc0f1c..fdbf009 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -361,16 +361,26 @@ module MCP # Fetches and validates the authorization server's RFC 8414 metadata. # - # On the modern path the metadata `issuer` must be byte-identical to the discovery URL (RFC 8414 Section 3.3). - # On the legacy 2025-03-26 path that validation is skipped: the legacy spec predates the requirement, - # and a pre-PRM server may host its OAuth endpoints under a path prefix whose `issuer` legitimately differs from - # the origin the metadata was discovered at (neither the TypeScript nor the Python SDK validates the issuer on this path). + # The metadata `issuer` must be byte-identical to the discovery URL (RFC 8414 Section 3.3) on both paths. + # On the legacy 2025-03-26 path the discovery URL is the MCP server's origin, which that spec names as + # the authorization base URL and which a document may render with a trailing slash; the TypeScript and Python SDKs + # accept the same slash-only difference. A document naming any other issuer is refused: an unverified `issuer` + # would otherwise become the identity tokens and client information are bound to, assertions are minted for, + # and the validator is shown, so a server could claim another authorization server and unlock the credentials + # bound to it. # When even the metadata document is absent, the legacy spec's default endpoints are used. def authorization_server_metadata(authorization_server:, legacy:, server_url:) metadata = if legacy - begin + fetched = begin fetch_authorization_server_metadata(issuer_url: authorization_server) rescue AuthorizationError + nil + end + + if fetched + ensure_legacy_issuer_matches!(expected: authorization_server, returned: fetched["issuer"]) + fetched + else default_legacy_metadata(authorization_server) end else @@ -415,6 +425,14 @@ module MCP fetch_metadata_json(urls, label: "authorization server metadata") end + # The legacy authorization base is an origin, which a document may render as `https://host/`; + # both name the same server, and nothing else does. + def ensure_legacy_issuer_matches!(expected:, returned:) + return if returned == "#{expected}/" + + ensure_issuer_matches!(expected: expected, returned: returned) + end + # Reads `authorization_servers` from a PRM document and returns # the first entry, raising `AuthorizationError` for any of the malformed # shapes a non-compliant server could emit (missing field, non-Array diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index 5daff12..61d7559 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -917,22 +917,22 @@ module MCP stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) end - def test_run_falls_back_to_server_origin_metadata_without_prm - # Legacy 2025-03-26 shape: no PRM, AS metadata served from the MCP server origin, - # OAuth endpoints under a path prefix whose `issuer` differs from the discovery origin. - # The legacy path must not apply the RFC 8414 issuer byte-match (the legacy spec predates it). + # Legacy 2025-03-26 shape: no PRM, AS metadata served from the MCP server origin with the given `issuer` + # and the OAuth endpoints under a path prefix. + def stub_legacy_metadata_with_prefixed_endpoints(issuer:, iss_supported: false) stub_prm_not_found stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return( status: 200, headers: { "Content-Type" => "application/json" }, body: JSON.generate( - issuer: "https://srv.example.com/oauth", + issuer: issuer, authorization_endpoint: "https://srv.example.com/oauth/authorize", token_endpoint: "https://srv.example.com/oauth/token", registration_endpoint: "https://srv.example.com/oauth/register", response_types_supported: ["code"], code_challenge_methods_supported: ["S256"], token_endpoint_auth_methods_supported: ["none"], + authorization_response_iss_parameter_supported: iss_supported, ), ) stub_request(:post, "https://srv.example.com/oauth/register").to_return( @@ -945,6 +945,11 @@ module MCP headers: { "Content-Type" => "application/json" }, body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600), ) + end + + def test_run_falls_back_to_server_origin_metadata_without_prm + # The document names the origin, so the RFC 8414 Section 3.3 check passes and its prefixed endpoints are used. + stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com") holder = {} provider = build_legacy_discovery_provider(holder) @@ -953,11 +958,54 @@ module MCP assert_equal(:authorized, result) assert_equal("legacy-token", provider.access_token) + assert_equal("https://srv.example.com", provider.tokens["issuer"]) assert_equal("/oauth/authorize", holder[:authorization_url].path) assert_requested(:post, "https://srv.example.com/oauth/register") assert_requested(:post, "https://srv.example.com/oauth/token") end + def test_run_accepts_legacy_metadata_naming_the_origin_with_a_trailing_slash + # A root issuer rendered as `https://host/` names the same server; the TypeScript and Python SDKs accept it too, + # and the document's spelling is what the RFC 9207 `iss` and the recorded issuer carry. + stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com/", iss_supported: true) + + holder = {} + provider = 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) { holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") }, + callback_handler: -> { ["test-auth-code", holder[:state], "https://srv.example.com/"] }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url) + + assert_equal(:authorized, result) + assert_equal("https://srv.example.com/", provider.tokens["issuer"]) + end + + def test_run_refuses_legacy_metadata_whose_issuer_is_not_the_origin + # RFC 8414 Section 3.3 applies on the legacy path as well: the 2025-03-26 spec places the metadata + # at the origin, so an issuer under a path prefix is a mismatch, as it is for the TypeScript and Python SDKs. + stub_legacy_metadata_with_prefixed_endpoints(issuer: "https://srv.example.com/oauth") + + holder = {} + provider = build_legacy_discovery_provider(holder) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url) + end + + assert_match(/`issuer` does not match/, error.message) + assert_nil(holder[:authorization_url]) + assert_not_requested(:post, "https://srv.example.com/oauth/register") + assert_not_requested(:post, "https://srv.example.com/oauth/token") + end + def test_run_falls_back_to_default_endpoints_without_any_metadata # Legacy 2025-03-26 "Fallbacks for Servers without Metadata Discovery": with no PRM and no AS metadata, # the client MUST use /authorize, /token, and /register at the authorization base URL, still sending PKCE S256. @@ -1032,6 +1080,98 @@ module MCP assert_match(/`issuer` does not match/, error.message) end + # Legacy metadata served at the MCP server origin that claims the identity of another authorization server, + # with every endpoint at the origin itself. The claim fails the RFC 8414 check, so none of these endpoints is reached. + def stub_legacy_metadata_claiming(issuer) + stub_prm_not_found + stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + issuer: issuer, + authorization_endpoint: "https://srv.example.com/authorize", + token_endpoint: "https://srv.example.com/token", + registration_endpoint: "https://srv.example.com/register", + code_challenge_methods_supported: ["S256"], + ), + ) + stub_request(:post, "https://srv.example.com/register").to_return( + status: 201, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(client_id: "legacy-client"), + ) + stub_request(:post, "https://srv.example.com/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600), + ) + end + + def test_run_refuses_legacy_metadata_claiming_another_authorization_server_before_the_validator + stub_legacy_metadata_claiming(@auth_base) + recorder = [] + provider = 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) { recorder << :redirected }, + callback_handler: -> { ["test-auth-code", "state"] }, + authorization_request_validator: ->(request) { + recorder << request + true + }, + ) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url) + end + + assert_match(/`issuer` does not match/, error.message) + assert_empty(recorder) + assert_not_requested(:post, "https://srv.example.com/register") + assert_not_requested(:post, "https://srv.example.com/token") + end + + def test_refresh_refuses_legacy_metadata_claiming_the_issuer_that_minted_the_tokens + # The tokens came from https://auth.example.com; a PRM-less server claiming that issuer at its own origin + # must not receive them, so the refresh token stays for the server that issued it. + stub_legacy_metadata_claiming(@auth_base) + provider = build_legacy_discovery_provider({}) + provider.save_client_information("client_id" => "conf-client", "client_secret" => "conf-secret") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => @auth_base) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).refresh!(server_url: @server_url) + end + + assert_match(/`issuer` does not match/, error.message) + assert_not_requested(:post, "https://srv.example.com/token") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + + def test_run_does_not_present_client_information_bound_elsewhere_when_legacy_metadata_claims_that_issuer + # Client information issued by https://auth.example.com is bound to it (SEP-2352); a PRM-less server claiming + # that issuer is refused before the credentials could be presented anywhere. + stub_legacy_metadata_claiming(@auth_base) + holder = {} + provider = build_legacy_discovery_provider(holder) + provider.save_client_information("client_id" => "conf-client", "client_secret" => "conf-secret", "issuer" => @auth_base) + + assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).run!(server_url: @server_url) + end + + assert_nil(holder[:authorization_url]) + assert_not_requested(:post, "https://srv.example.com/register") + assert_not_requested(:post, "https://srv.example.com/token") + assert_equal("conf-secret", provider.client_information["client_secret"]) + assert_equal(@auth_base, provider.client_information["issuer"]) + end + def test_run_raises_when_prm_authorization_servers_is_not_an_array # `authorization_servers` MUST be an Array per RFC 9728. A misbehaving # PRM that returns a String would otherwise reach `.first` and raise From 29e8d3f2b91259f526c66329dff08a697d2fcf6f Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 17 Sep 2026 19:05:51 +0900 Subject: [PATCH 13/17] Let the client_credentials and jwt-bearer grants run without Protected Resource Metadata ## Motivation and Context When a server publishes no Protected Resource Metadata, discovery takes the legacy 2025-03-26 path and `prm` is `nil`. The authorization-code branch of `run!` allowed for that by passing `prm || {}` to `resolve_scope`, but `run_client_credentials!` and `run_jwt_bearer!` passed `prm` as is, and `resolve_scope` then called `prm["scopes_supported"]` on `nil`. Without a non-empty challenge scope, a `ClientCredentialsProvider` or `CrossAppAccessProvider` pointed at such a server therefore failed with `NoMethodError` after discovery and before the token request or the ID-JAG callback, instead of running its grant against the legacy authorization base the way `Provider` does. `resolve_scope` now treats a missing PRM as advertising no scopes, so the challenge scope or the provider's own scope applies, and the authorization-code branch passes `prm` unchanged like the other two. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb` run the `client_credentials` grant against a PRM-less server with metadata at the origin, checking the provider's scope, the `resource`, and the recorded issuer, and run the `jwt-bearer` grant against a server without any metadata, checking the default token endpoint, the assertion audience, the `resource`, and the recorded issuer. Both raise `NoMethodError` against the previous library. ## Breaking Changes None. --- lib/mcp/client/oauth/flow.rb | 5 ++- test/mcp/client/oauth/flow_test.rb | 69 ++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 2 deletions(-) diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index acc0f1c..f99b6f1 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -87,7 +87,7 @@ module MCP ensure_pkce_supported!(as_metadata) - effective_scope = resolve_scope(scope: scope, prm: prm || {}) + effective_scope = resolve_scope(scope: scope, prm: prm) effective_scope = normalize_offline_access_scope(effective_scope, as_metadata: as_metadata) # Asked before registering, not after: a refusal must not leave this client registered at an authorization server @@ -898,7 +898,8 @@ module MCP def resolve_scope(scope:, prm:) return scope if scope && !scope.empty? - supported = prm["scopes_supported"] + # `prm` is nil on the legacy path, where nothing advertises scopes. + supported = prm && prm["scopes_supported"] return supported.join(" ") if supported.is_a?(Array) && !supported.empty? return @provider.scope if @provider.scope && !@provider.scope.empty? diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index 5daff12..a91006d 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -991,6 +991,75 @@ module MCP end end + def test_run_client_credentials_without_prm_uses_the_legacy_authorization_base + # A PRM-less server takes the legacy path for this grant too: metadata at the origin, no advertised scopes, + # so the provider's own scope is used and the tokens are bound to the origin. + stub_prm_not_found + stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + issuer: "https://srv.example.com", + token_endpoint: "https://srv.example.com/token", + token_endpoint_auth_methods_supported: ["client_secret_basic"], + ), + ) + stub_request(:post, "https://srv.example.com/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600), + ) + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", scope: "mcp:read") + + result = Flow.new(provider: provider).run!(server_url: @server_url) + + assert_equal(:authorized, result) + assert_equal("legacy-token", provider.access_token) + assert_equal("https://srv.example.com", provider.tokens["issuer"]) + assert_requested(:post, "https://srv.example.com/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "client_credentials" && + form["scope"] == "mcp:read" && + form["resource"] == "https://srv.example.com/mcp" + end + end + + def test_run_jwt_bearer_without_any_metadata_uses_the_legacy_default_endpoints + stub_prm_not_found + stub_request(:get, "https://srv.example.com/.well-known/oauth-authorization-server").to_return(status: 404) + stub_request(:get, "https://srv.example.com/.well-known/openid-configuration").to_return(status: 404) + stub_request(:post, "https://srv.example.com/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "legacy-token", token_type: "Bearer", expires_in: 3600), + ) + received = nil + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(audience:, resource:) { + received = { audience: audience, resource: resource } + "id-jag-assertion" + }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url) + + assert_equal(:authorized, result) + # The assertion is minted for the legacy authorization base, which is the only identity such a server has. + assert_equal({ audience: "https://srv.example.com", resource: "https://srv.example.com/mcp" }, received) + assert_equal("https://srv.example.com", provider.tokens["issuer"]) + assert_requested(:post, "https://srv.example.com/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "urn:ietf:params:oauth:grant-type:jwt-bearer" && + form["assertion"] == "id-jag-assertion" && + form["resource"] == "https://srv.example.com/mcp" && + !form.key?("scope") + end + end + def test_run_legacy_fallback_rejects_insecure_authorization_base # The Communication Security requirement still applies on the legacy path: a remote plain-http origin must not # become the authorization base URL. From f51ccfbe662f4f0320ceb5974d39eab317589e3d Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 17 Sep 2026 02:39:45 +0900 Subject: [PATCH 14/17] Let providers without a CIMD URL reader refresh their tokens ## Motivation and Context `Flow#refresh!` reads `client_id_metadata_document_url` from the provider unconditionally, once to decide whether a provider without stored `client_information` may still refresh and once to build the CIMD `client_id`. Only `Provider` defines that reader: `CrossAppAccessProvider` and `ClientCredentialsProvider` raise `NoMethodError` once a stored `refresh_token` is found. `MCP::Client::HTTP` calls `refresh!` on any `401` while a `refresh_token` is stored, so a `jwt-bearer` client whose authorization server issued one fails with `NoMethodError` on the first expired token instead of refreshing. The error is not an `AuthorizationError`, so the transport does not fall back to the grant either. RFC 7521 Section 4.1 makes a refresh token unusual for an assertion grant, not forbidden, and one in storage must not crash the client. The flow now reads the URL through a duck-typed helper, like `authorization_flow` and `client_assertion`, so a provider without the reader has no CIMD URL and refreshes with its stored `client_information`: the `refresh_token` grant is sent with the stored client authentication, and no new ID-JAG assertion is obtained. The authorization-code registration path reads the URL through the same helper. The docs now say that a refresh token issued anyway is used on the next `401`, and that the assertion provider is not called again on refresh. A refresh token stored without the `issuer` the SDK records is refused for these two providers before any request: unlike `Provider`, they have no tokens that predate the record, and a refresh asks no validator, so such a token would be presented to whatever authorization server discovery names. The refusal is an `AuthorizationError`, which sends the transport back through the grant, and the grant asks. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb` refresh through a `CrossAppAccessProvider` and a `ClientCredentialsProvider` holding a `refresh_token`, asserting the `refresh_token` grant, the Basic credentials, and that the assertion provider is not called. A transport test in `test/mcp/client/oauth/http_oauth_test.rb` sends a `401` to a `CrossAppAccessProvider` with a stored `refresh_token` and expects the refreshed bearer token on the retry. All three raise `NoMethodError` against the previous library. Further tests pin the issuer stamp on the refreshed tokens, the refusal of an authorization server that did not issue them for a `CrossAppAccessProvider`, a clean `AuthorizationError` for a provider with neither the reader nor stored `client_information`, and registration through DCR for a provider without the reader. A refresh token without a recorded issuer is refused for both providers before any request is sent. ## Breaking Changes None. --- docs/_client/authorization.md | 6 +- lib/mcp/client/oauth/flow.rb | 32 +++++- test/mcp/client/oauth/flow_test.rb | 132 +++++++++++++++++++++++ test/mcp/client/oauth/http_oauth_test.rb | 62 +++++++++++ 4 files changed, 227 insertions(+), 5 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index b35b597..bb86458 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -40,6 +40,8 @@ pass an `MCP::Client::OAuth::Provider` to the transport instead of a static `Aut as the authorization base URL, its metadata is fetched from `/.well-known/oauth-authorization-server` without the RFC 8414 issuer byte-match (which the legacy spec predates), and when even that is absent the spec's default endpoints `/authorize`, `/token`, and `/register` at the origin are used with PKCE S256 assumed. - On subsequent 401s with a saved `refresh_token`, exchange it at the token endpoint before falling back to the full interactive flow (RFC 6749 Section 6). + `ClientCredentialsProvider` and `CrossAppAccessProvider` refresh the same way and fall back to their own grant instead; + their refresh also requires the `issuer` the SDK records on the tokens, so tokens stored without it run the grant again. - On a `403 Forbidden` whose `WWW-Authenticate` header carries `error="insufficient_scope"` (OAuth 2.0 step-up, RFC 6750 Section 3.1 and the MCP scope-selection-strategy), run a fresh authorization request for the union of the currently granted scope and the scope named in the challenge, then retry the failed request once. The refresh path is bypassed because refreshing would re-issue the same scope set the server just rejected. A `403` without that challenge is surfaced unchanged. @@ -192,7 +194,8 @@ and redaction policy before persisting them or displaying them to users. For a confidential machine-to-machine client (no user, no browser redirect), use `MCP::Client::OAuth::ClientCredentialsProvider` instead of `Provider`. The transport discovers the authorization server the same way, then exchanges the OAuth 2.1 `client_credentials` grant (RFC 6749 Section 4.4) at -the token endpoint. There is no authorization request, PKCE, or `offline_access`, because the grant does not issue a refresh token. +the token endpoint. There is no authorization request, PKCE, or `offline_access`, because the grant is not expected to issue a refresh token (RFC 6749 Section 4.4.3); +a refresh token the authorization server issues anyway is used on the next `401`. ```ruby provider = MCP::Client::OAuth::ClientCredentialsProvider.new( @@ -224,6 +227,7 @@ Keyword arguments: For enterprise MCP deployments where an identity provider (IdP) governs authorization (SEP-990), use `MCP::Client::OAuth::CrossAppAccessProvider` instead of `Provider`. The client exchanges an IdP-issued ID token for an Identity Assertion Authorization Grant (ID-JAG) at the IdP via RFC 8693 token exchange, then presents the ID-JAG to the MCP authorization server with the RFC 7523 `jwt-bearer` grant, authenticating with `client_secret_basic`. There is no authorization request, PKCE, DCR, or `offline_access`. +A refresh token the authorization server issues is exchanged on the next `401` with the stored client secret, without calling `assertion_provider` again. Mirrors `CrossAppAccessProvider` and `requestJwtAuthorizationGrant` in the TypeScript SDK. `MCP::Client::OAuth::IDJAGTokenExchange.request` performs the RFC 8693 exchange at the IdP token endpoint. Wrap it in a callable so the same provider can plug into diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 1093eba..c34d617 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -182,7 +182,7 @@ module MCP # Runs the OAuth 2.1 `client_credentials` grant (machine-to-machine, no user interaction) and persists # the resulting token. Shares the same discovery and security checks as `run!`; the only difference is # the grant exchanged at the token endpoint. There is no PKCE, redirect, or authorization request, - # and no `offline_access` augmentation because the grant does not issue a refresh token (OAuth 2.1 Section 4.3.3). + # and no `offline_access` augmentation because the grant is not expected to issue a refresh token (OAuth 2.1 Section 4.3.3). # The pre-registered `client_id` / `client_secret` come from the provider's stored `client_information`. # https://modelcontextprotocol.io/specification/2025-11-25/basic/authorization def run_client_credentials!(as_metadata:, prm:, resource:, scope:, server_url:) @@ -275,19 +275,22 @@ module MCP # checks before talking to it. # # Returns `:refreshed` on success. Raises `AuthorizationError` when the provider has no refresh token, no client information, + # when a `client_credentials` or `jwt-bearer` provider's tokens record no issuer, # or when the token endpoint refuses the refresh request. # https://www.rfc-editor.org/rfc/rfc6749#section-6 def refresh!(server_url:, resource_metadata_url: nil) refresh_token = read_token("refresh_token") raise AuthorizationError, "Cannot refresh: no refresh_token in provider storage." unless refresh_token + ensure_refresh_token_issuer_recorded! + stored_client_info = @provider.client_information have_stored_client_info = stored_client_info.is_a?(Hash) && client_info_required_value(stored_client_info, "client_id") # A CIMD-configured provider stores no `client_information` on purpose # (the CIMD URL is re-resolved against the live AS metadata on every flow). # Allow refresh to proceed in that case so the `refresh_token` obtained via the CIMD flow remains usable. - have_cimd_url = !@provider.client_id_metadata_document_url.nil? + have_cimd_url = !provider_client_id_metadata_document_url.nil? unless have_stored_client_info || have_cimd_url raise AuthorizationError, "Cannot refresh: no client_information in provider storage." @@ -323,7 +326,7 @@ module MCP ensure_refreshable_client_information!(stored_client_info, as_metadata: as_metadata) stored_client_info elsif as_metadata["client_id_metadata_document_supported"] == true - { "client_id" => @provider.client_id_metadata_document_url } + { "client_id" => provider_client_id_metadata_document_url } else raise AuthorizationError, "Cannot refresh: provider has a CIMD URL but the authorization server no longer advertises " \ @@ -684,7 +687,7 @@ module MCP # (or the operator may rotate the CIMD URL), and a stale `client_information` entry would otherwise # keep sending the old CIMD URL forever. Re-evaluating on every flow re-reads the current AS metadata # and the current `provider.client_id_metadata_document_url`. - cimd_url = @provider.client_id_metadata_document_url + cimd_url = provider_client_id_metadata_document_url if cimd_url && as_metadata["client_id_metadata_document_supported"] == true return { "client_id" => cimd_url } end @@ -814,6 +817,18 @@ module MCP MESSAGE end + # `Provider` tolerates tokens stored before the issuer was recorded (see `ensure_token_issuer!`). + # The `client_credentials` and `jwt-bearer` providers have no such tokens, since their refresh is new, + # and a refresh asks no validator, so a token without an `issuer` would be presented to whatever + # authorization server discovery names now. Refusing it sends the transport back through the grant, + # which does ask. + def ensure_refresh_token_issuer_recorded! + return if provider_authorization_flow == :authorization_code + return unless read_token("issuer").nil? + + raise AuthorizationError, "Cannot refresh: the stored tokens record no issuer; re-authorization is required." + end + def ensure_refreshable_client_information!(client_info, as_metadata:) stored_issuer = client_info_required_value(client_info, "issuer") return if stored_issuer.nil? @@ -1020,6 +1035,15 @@ module MCP params end + # The Client ID Metadata Document URL, when the provider has one. Only `Provider` exposes the reader + # (CIMD replaces Dynamic Client Registration on the authorization-code flow), while `refresh!` serves + # every provider that holds a `refresh_token`, so the read is duck-typed like `authorization_flow`. + def provider_client_id_metadata_document_url + return unless @provider.respond_to?(:client_id_metadata_document_url) + + @provider.client_id_metadata_document_url + end + def build_authorization_url(as_metadata:, client_id:, scope:, state:, code_challenge:, resource:) authorization_endpoint = as_metadata["authorization_endpoint"] unless authorization_endpoint diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index c463fdc..799ecd0 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -557,6 +557,138 @@ module MCP assert_not_requested(:post, "#{@auth_base}/token") end + def test_refresh_uses_the_stored_credentials_of_a_cross_app_access_provider + # RFC 7521 Section 4.1 makes a refresh token unusual for an assertion grant, not forbidden; + # when the authorization server issued one, it is exchanged with the stored client secret + # and no new ID-JAG assertion is minted. + assertion_calls = 0 + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { + assertion_calls += 1 + "id-jag-assertion" + }, + ) + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => @auth_base) + + result = Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:refreshed, result) + assert_equal("test-token-from-flow", provider.access_token) + assert_equal(@auth_base, provider.tokens["issuer"]) + assert_equal(0, assertion_calls) + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "refresh_token" && + form["refresh_token"] == "saved-rt" && + !form.key?("assertion") && + req.headers["Authorization"] == "Basic " + Base64.strict_encode64("xaa-client:xaa-secret") + end + end + + def test_refresh_uses_the_stored_credentials_of_a_client_credentials_provider + # RFC 6749 Section 4.4.3 says a refresh token SHOULD NOT be issued for this grant, + # so one in storage is unusual, but it must be exchanged rather than crash the refresh. + provider = client_credentials_provider + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => @auth_base) + + result = Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:refreshed, result) + assert_equal(@auth_base, provider.tokens["issuer"]) + assert_requested(:post, "#{@auth_base}/token") do |req| + form = URI.decode_www_form(req.body).to_h + + form["grant_type"] == "refresh_token" && + form["refresh_token"] == "saved-rt" && + req.headers["Authorization"] == "Basic " + Base64.strict_encode64("cc-client:cc-secret") + end + end + + def test_refresh_refuses_an_authorization_server_that_did_not_issue_a_cross_app_access_token + # The token-level issuer binding is what protects these providers: their stored `client_information` carries no `issuer`, + # so `ensure_refreshable_client_information!` has nothing to compare. + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag-assertion" }, + ) + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => "https://old-as.example.com") + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/issued by a different authorization server/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + + def test_refresh_refuses_tokens_without_a_recorded_issuer_for_the_client_credentials_and_jwt_bearer_providers + # These providers had no refresh before their tokens recorded the issuer, so nothing older needs + # tolerating; without the record a refresh would go to whatever server discovery names, unasked. + cross_app = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag-assertion" }, + ) + [cross_app, client_credentials_provider].each do |provider| + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + error = assert_raises(Flow::AuthorizationError, "should refuse #{provider.class}") do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/record no issuer/, error.message) + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + + assert_not_requested(:get, @prm_url) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_refresh_raises_cleanly_for_a_provider_with_neither_client_information_nor_a_cimd_url + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag-assertion" }, + ) + provider.storage.save_client_information(nil) + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt", "issuer" => @auth_base) + + error = assert_raises(Flow::AuthorizationError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/no client_information/, error.message) + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_registers_through_dcr_for_a_provider_without_a_cimd_url_reader + # Registration reads the CIMD URL through the same duck-typed helper as refresh, + # so a provider that lacks the reader goes to Dynamic Client Registration. + provider_class = Class.new(Provider) { undef_method :client_id_metadata_document_url } + state_value = nil + provider = provider_class.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_value = URI.decode_www_form(url.query).to_h.fetch("state") }, + callback_handler: -> { ["test-auth-code", state_value] }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:post, "#{@auth_base}/register") + end + def test_run_uses_authorization_code_grant_for_default_provider # A standard `Provider` declares `authorization_flow == :authorization_code`, # so `Flow` runs the interactive grant regardless of what `client_metadata[:grant_types]` happens to list. diff --git a/test/mcp/client/oauth/http_oauth_test.rb b/test/mcp/client/oauth/http_oauth_test.rb index 5b1be13..ce026c9 100644 --- a/test/mcp/client/oauth/http_oauth_test.rb +++ b/test/mcp/client/oauth/http_oauth_test.rb @@ -627,6 +627,68 @@ module MCP assert_equal("refreshed-token", provider.access_token) end + def test_send_request_refreshes_for_a_cross_app_access_provider_holding_a_refresh_token + # Refresh serves every provider, not only the authorization-code one that has a CIMD URL reader. + stub_request(:post, @mcp_url).with { |req| + req.headers["Authorization"] != "Bearer refreshed-token" + }.to_return( + status: 401, + headers: { "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}") }, + body: "", + ) + + stub_request(:post, @mcp_url).with( + headers: { "Authorization" => "Bearer refreshed-token" } + ).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(jsonrpc: "2.0", id: "1", result: { ok: true }), + ) + + stub_request(:get, @prm_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + + stub_request(:get, "#{@auth_base}/.well-known/oauth-authorization-server").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate( + issuer: @auth_base, + token_endpoint: "#{@auth_base}/token", + grant_types_supported: ["urn:ietf:params:oauth:grant-type:jwt-bearer", "refresh_token"], + token_endpoint_auth_methods_supported: ["client_secret_basic"], + ), + ) + + stub_request(:post, "#{@auth_base}/token").with( + body: hash_including("grant_type" => "refresh_token", "refresh_token" => "saved-rt") + ).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "refreshed-token", token_type: "Bearer", expires_in: 3600), + ) + + assertion_calls = 0 + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { + assertion_calls += 1 + "id-jag-assertion" + }, + ) + provider.save_tokens("access_token" => "stale-token", "refresh_token" => "saved-rt", "issuer" => @auth_base) + + transport = HTTP.new(url: @mcp_url, oauth: provider) + response = transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + + assert_equal({ "ok" => true }, response["result"]) + assert_equal("refreshed-token", provider.access_token) + assert_equal(0, assertion_calls) + end + def test_send_request_surfaces_a_bad_token_request_params_hook_instead_of_reauthorizing # A provider whose `token_request_params` names a reserved key is misconfigured, not unauthorized: # the refresh attempt must raise rather than fall through to the interactive flow, From c9f381a3ee03768952099f1daf3fff9bc2dba5da Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 18 Sep 2026 01:15:37 +0900 Subject: [PATCH 15/17] Let a provider customize the HTTP client the OAuth flow uses ## Motivation and Context `MCP::Client::OAuth::Flow` takes an `http_client_factory:`, but `MCP::Client::HTTP` builds the flow itself and passes none, so an application using `oauth:` could not add middleware to the requests the flow makes: Protected Resource Metadata and authorization server metadata discovery, dynamic client registration, and every token request. The block that customizes the transport's Faraday connection does not help, because that connection is bound to the MCP server URL and carries the `headers:` meant for that server, and the authorization server is a different origin. The TypeScript SDK hands the transport's `fetch` to `auth()` and the Python SDK sends the provider's requests through the transport's httpx client, so both let an application instrument these requests; this SDK did not. (#558) Providers now take `http_client_customizer:`, a callable invoked with the Faraday connection the flow builds for its own requests, next to the other callables a provider already holds. It runs after the SDK's defaults (`Accept: application/json`, no redirect following) and between two middlewares of the SDK's own: `Flow::RequestedURLStamp`, registered before the customizer is invoked, records the URL the flow asked for on the request environment, and `Flow::RequestedOriginGuard`, registered last, sees what the customizer's middleware did to the request after that record. A request that would leave that origin, by following a redirect or rewriting the URL, or that arrives without the record, is refused with `Flow::DestinationMismatchError` before it reaches the adapter. The record lives on the environment rather than in `env.request.context`, which stays the application's to fill or replace. The error is an `ArgumentError` rather than an `AuthorizationError` so neither discovery nor the transport's refresh fallback can absorb a configuration mistake. The boundary resembles the one the Python SDK keeps for its own requests, which follows a redirect only within the origin; this flow follows none itself. `Flow.build_http_client` assembles that connection and `Flow.new(http_client_factory:)` keeps replacing it whole. Following done inside an adapter cannot be seen by any guard, so the documentation keeps that, `Accept-Encoding`, and `raise_error` off the list of things a customizer may add, and notes that a recording middleware sees client credentials and tokens. Fixes #558. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb`, `test/mcp/client/oauth/http_oauth_test.rb`, and the three provider test files. Against the library before this change, every one of them fails, on the unknown keyword or on the missing reader. ## Breaking Changes None. The customizer is opt-in, and the guard cannot fire on the SDK's own middleware-free connection. --- docs/_client/authorization.md | 45 +- docs/_client/transports.md | 3 + lib/mcp/client/oauth/bounded_body.rb | 2 +- .../oauth/client_credentials_provider.rb | 8 +- .../client/oauth/cross_app_access_provider.rb | 8 +- lib/mcp/client/oauth/flow.rb | 116 +++++- lib/mcp/client/oauth/id_jag_token_exchange.rb | 2 +- lib/mcp/client/oauth/provider.rb | 8 +- .../client/oauth/storage_backed_provider.rb | 15 + .../oauth/client_credentials_provider_test.rb | 19 + .../oauth/cross_app_access_provider_test.rb | 30 ++ test/mcp/client/oauth/flow_test.rb | 390 +++++++++++++++++- test/mcp/client/oauth/http_oauth_test.rb | 216 +++++++++- test/mcp/client/oauth/provider_test.rb | 20 + 14 files changed, 856 insertions(+), 26 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index b35b597..314d712 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -95,6 +95,8 @@ Optional keyword arguments: - `scope`: Space-separated scopes to request when the server's `WWW-Authenticate` does not specify one. - `authorization_request_validator`: Callable invoked with an `MCP::Client::OAuth::AuthorizationRequest` before any authorization request is built. Returning a falsy value abandons the flow with `Flow::AuthorizationRefusedError`. See [Reviewing the authorization request](#reviewing-the-authorization-request). +- `http_client_customizer`: Callable invoked with the Faraday connection the SDK builds for the OAuth flow's own requests, after its defaults and before its origin guard. + See [Customizing the OAuth HTTP Client](#customizing-the-oauth-http-client). - `storage`: Object responding to `tokens`, `save_tokens(t)`, `client_information`, `save_client_information(info)`. Defaults to `MCP::Client::OAuth::InMemoryStorage`, which keeps credentials in process memory only. Persisted `client_information` is stamped with an `"issuer"` member binding it to the authorization server that issued it (SEP-2352): when the server's authorization server changes, the SDK discards the stale registration and its tokens and re-registers automatically @@ -216,7 +218,7 @@ 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. -- `scope`, `storage`, `authorization_request_validator`, `token_request_params`: Optional, same meaning as on `Provider`. +- `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`. ### Cross-App Access (JWT Bearer) Grant @@ -254,7 +256,39 @@ Keyword arguments: - `assertion_provider`: Required. Callable invoked as `call(audience:, resource:)` and returning the ID-JAG assertion. `audience` is the MCP authorization server's validated issuer identifier; `resource` is the canonical MCP server URL (RFC 8707). Passing both through to `IDJAGTokenExchange.request` covers the common case. -- `scope`, `storage`, `authorization_request_validator`, `token_request_params`: Optional, same meaning as on `Provider`. +- `scope`, `storage`, `authorization_request_validator`, `token_request_params`, `http_client_customizer`: Optional, same meaning as on `Provider`. + +### Customizing the OAuth HTTP Client + +The requests the OAuth flow makes (Protected Resource Metadata discovery on the MCP server's origin, authorization server metadata discovery, +dynamic client registration, and every token request the flow sends, whether the first exchange, a refresh, or a step-up) go over a Faraday connection of their own, +not over the transport's connection: the transport's is bound to the MCP server URL and carries the `headers:` and the customizer block meant for that server. +To add middleware to the OAuth flow's connection, or to swap its adapter, pass `http_client_customizer:` to the provider: + +```ruby +provider = MCP::Client::OAuth::ClientCredentialsProvider.new( + client_id: "my-service", + client_secret: ENV.fetch("MCP_CLIENT_SECRET"), + http_client_customizer: ->(faraday) { faraday.use MyApp::Middleware::HttpRecorder }, +) +``` + +The callable receives the `Faraday::Connection` after the SDK has applied its defaults and registered the middleware that records the requested URL, +and before the SDK registers its origin guard, the same position the transport's customizer block has on the MCP server connection. +It may be invoked more than once, and from more than one thread at a time, so keep it free of side effects and safe to run concurrently; +today it runs once per authorization attempt, but that is not a promise. +A few constraints follow from the checks described below: + +- Do not add redirect-following middleware. Every destination check runs against the URL as written, so a request that middleware added by the customizer would send + to a different origin after the SDK has recorded the requested URL, whether by following a `3xx` or by rewriting the URL, is refused with `Flow::DestinationMismatchError` + before it reaches the adapter, as is a request that reaches the guard without that record. Middleware inserted ahead of the record with `builder.insert(0, ...)` + that rewrites the URL first or rebuilds the environment is outside the guard, as is following done inside an adapter, so leave both off. +- Leave `Accept-Encoding` unset. The response cap below is measured on decoded bytes, and claiming the header turns Net::HTTP's decoding off. +- Do not add Faraday's `raise_error` middleware. The flow reads statuses itself, both to tell an absent metadata document from a failed request + and to turn a token endpoint error into `Flow::InvalidGrantError`. +- With an adapter that does not stream through `on_data`, the response cap is applied once the body has been buffered rather than as it arrives. +- A middleware that records requests sees the client credentials on token requests (`Authorization: Basic`, `client_secret`, `client_assertion`), refresh tokens, + and the access tokens in token responses; redact them before they reach a log. ### Communication Security @@ -286,8 +320,11 @@ The range check compares IP literals and does not resolve hostnames, so it canno such as `https://vault.corp.internal/`. Resolving names here would not close that gap either, because the address the SDK looked up need not be the one the HTTP client connects to a moment later. The same-origin rule is what protects the `resource_metadata` URL, which is the only one of these a server supplies directly. -If you replace the OAuth HTTP client through `MCP::Client::OAuth::Flow.new(http_client_factory:)`, do not add redirect-following middleware. Every check above runs against -the URL as written, so a connection that follows a `3xx` on its own would reach hosts these rules just refused. +On the connection the SDK builds, a middleware that would send a request to a different origin, by following a `3xx` or by rewriting the URL, is refused before the request +goes out (see [Customizing the OAuth HTTP Client](#customizing-the-oauth-http-client)). A connection supplied through `MCP::Client::OAuth::Flow.new(http_client_factory:)` +replaces that one, the provider's `http_client_customizer` and the guard included, so do not add redirect-following middleware to it: every check above runs against +the URL as written, and a connection that follows a `3xx` on its own would reach hosts these rules just refused. +A factory that wants to keep them can return `MCP::Client::OAuth::Flow.build_http_client(customizer)`, the connection the SDK builds for itself. The SDK also bounds what those endpoints may return. A discovery, dynamic client registration, token, or token exchange response is refused once it passes 4 MiB, measured as the body arrives rather than after it has been buffered, so a compressed body that expands past the limit is refused partway through the expansion. diff --git a/docs/_client/transports.md b/docs/_client/transports.md index 4362666..d69e5e9 100644 --- a/docs/_client/transports.md +++ b/docs/_client/transports.md @@ -248,6 +248,9 @@ http_transport = MCP::Client::HTTP.new(url: "https://api.example.com/mcp") do |f end ``` +The block customizes only the connection to the MCP server. The connection the OAuth flow uses for its own requests is customized through +the provider's `http_client_customizer:` keyword instead; see [Customizing the OAuth HTTP Client](/client/authorization/#customizing-the-oauth-http-client). + {: .note } > Answers to server-to-client requests (a pong, an elicitation result) are POSTed from inside > the SSE streaming callback of another response, re-entering the connection on the same thread. diff --git a/lib/mcp/client/oauth/bounded_body.rb b/lib/mcp/client/oauth/bounded_body.rb index 4aab2ad..f3b5efa 100644 --- a/lib/mcp/client/oauth/bounded_body.rb +++ b/lib/mcp/client/oauth/bounded_body.rb @@ -26,7 +26,7 @@ module MCP # Faraday `on_data` streaming callback. The chunks arrive decompressed: the default `Net::HTTP` adapter negotiates # `Accept-Encoding` itself and reads the body through `Net::HTTPResponse#inflater`, so a small compressed body # that expands past the cap is refused partway through the expansion rather than after it. That holds only while - # the connection leaves `Accept-Encoding` to the adapter; see `Flow#default_http_client`. + # the connection leaves `Accept-Encoding` to the adapter; see `Flow.build_http_client`. def on_data proc do |chunk, _received_bytes, _env| @buffer << chunk diff --git a/lib/mcp/client/oauth/client_credentials_provider.rb b/lib/mcp/client/oauth/client_credentials_provider.rb index 22ef56a..4e1e1cf 100644 --- a/lib/mcp/client/oauth/client_credentials_provider.rb +++ b/lib/mcp/client/oauth/client_credentials_provider.rb @@ -46,6 +46,8 @@ module MCP # server requires beyond the grant itself (Auth0's `audience`, for example). # A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` raises `Flow::InvalidTokenRequestParamsError`. # The Hash is copied and frozen. See `StorageBackedProvider#token_request_params`. + # - `http_client_customizer` - Callable invoked with the `Faraday::Connection` the flow builds for + # its own requests; see `StorageBackedProvider#http_client_customizer`. class ClientCredentialsProvider include StorageBackedProvider @@ -66,7 +68,8 @@ module MCP scope: nil, storage: nil, authorization_request_validator: nil, - token_request_params: nil + token_request_params: nil, + http_client_customizer: nil ) if blank?(client_id) raise InvalidCredentialsError, "client_id is required for the client_credentials grant." @@ -104,6 +107,8 @@ module MCP client_information["client_secret"] = client_secret end + http_client_customizer = validated_http_client_customizer(http_client_customizer) + @client_id = client_id @private_key = private_key @signing_algorithm = signing_algorithm @@ -111,6 +116,7 @@ module MCP @storage = storage || InMemoryStorage.new @authorization_request_validator = authorization_request_validator @token_request_params = frozen_token_request_params(token_request_params) + @http_client_customizer = http_client_customizer @storage.save_client_information(client_information) end diff --git a/lib/mcp/client/oauth/cross_app_access_provider.rb b/lib/mcp/client/oauth/cross_app_access_provider.rb index 422cbb5..44b5ffd 100644 --- a/lib/mcp/client/oauth/cross_app_access_provider.rb +++ b/lib/mcp/client/oauth/cross_app_access_provider.rb @@ -29,6 +29,8 @@ module MCP # the authorization server requires beyond the grant itself. A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` raises # `Flow::InvalidTokenRequestParamsError`. The Hash is copied and frozen. # See `StorageBackedProvider#token_request_params`. + # - `http_client_customizer` - Callable invoked with the `Faraday::Connection` the flow builds for its own requests; + # see `StorageBackedProvider#http_client_customizer`. # # https://github.com/modelcontextprotocol/modelcontextprotocol/issues/990 class CrossAppAccessProvider @@ -46,7 +48,8 @@ module MCP scope: nil, storage: nil, authorization_request_validator: nil, - token_request_params: nil + token_request_params: nil, + http_client_customizer: nil ) if blank?(client_id) raise InvalidConfigurationError, "client_id is required for the jwt-bearer grant." @@ -60,11 +63,14 @@ module MCP raise InvalidConfigurationError, "assertion_provider must be callable as `call(audience:, resource:)` and return the ID-JAG assertion." end + http_client_customizer = validated_http_client_customizer(http_client_customizer) + @assertion_provider = assertion_provider @scope = scope @storage = storage || InMemoryStorage.new @authorization_request_validator = authorization_request_validator @token_request_params = frozen_token_request_params(token_request_params) + @http_client_customizer = http_client_customizer @storage.save_client_information( "client_id" => client_id, "client_secret" => client_secret, diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 1093eba..5183c6b 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -63,6 +63,71 @@ module MCP # a failed refresh as a reason to run the interactive flow. class InvalidTokenRequestParamsError < ArgumentError; end + # Raised by `RequestedOriginGuard` when middleware added through the provider's `http_client_customizer` + # would send a request to an origin other than the one the flow validated, or has dropped the record of + # the URL the flow asked for. An `ArgumentError` because the middleware is a configuration mistake, + # and deliberately outside `AuthorizationError`, which discovery treats as "nothing published" + # and `MCP::Client::HTTP` treats on a failed refresh as a reason to run the interactive flow. + class DestinationMismatchError < ArgumentError; end + + # Faraday middleware registered on the connection `build_http_client` assembles before the customizer + # is invoked, so with the usual `use` it sits ahead of the customizer's middleware and sees the URL exactly + # as the flow requested it, which it records on the request environment for `RequestedOriginGuard`. + # The guard covers what happens to a request after that record; middleware inserted ahead of it with + # `builder.insert(0, ...)` that rewrites the URL before it or rebuilds the environment is outside the guard. + # The record lives on the environment, not in `env.request.context`: that slot belongs to the application, + # which may fill it on the connection or replace it from a middleware of its own. Only the first URL seen + # on an environment is recorded: a middleware inserted ahead of this one that re-enters the stack after + # a `3xx` with the same environment, or with its `dup`, which shares the record, cannot replace it with + # the redirected URL. + class RequestedURLStamp + KEY = :mcp_oauth_requested_url + + def initialize(app) + @app = app + end + + def call(env) + env[KEY] ||= env.url.to_s + @app.call(env) + end + end + + # Faraday middleware registered last on that connection, so it sees `env.url` after any customizer-added + # middleware has rewritten it or followed a redirect. A request that would leave the origin the flow asked + # for is refused before it reaches the adapter, since every destination check ran against the URL as + # written; a same-origin change stays with the server those checks admitted. The record survives + # the `env.dup` that redirect-following middleware performs, and a request that arrives without it is + # refused as well, so a middleware that rebuilds the environment fails closed rather than open. + # The origin boundary resembles the one the Python SDK keeps for its own auth requests, which follows + # a redirect itself only within the origin; this flow follows none. + class RequestedOriginGuard + def initialize(app) + @app = app + end + + def call(env) + requested = env[RequestedURLStamp::KEY] + unless requested + raise DestinationMismatchError, <<~MESSAGE + Request to #{Discovery.canonicalize_origin_and_path(env.url.to_s).inspect} carries no record of \ + the URL the flow asked for; middleware that rebuilds the request environment is refused. + MESSAGE + end + + unless Discovery.same_origin?(env.url.to_s, requested) + raise DestinationMismatchError, <<~MESSAGE + Request to #{Discovery.canonicalize_origin_and_path(requested).inspect} would be sent to \ + #{Discovery.canonicalize_origin_and_path(env.url.to_s).inspect}, on a different origin; \ + middleware that follows redirects or rewrites URLs is refused. + MESSAGE + end + + @app.call(env) + end + end + private_constant :RequestedURLStamp, :RequestedOriginGuard + class << self # Returns why `params` cannot ride a token request as `token_request_params`, or `nil` when it can. # Shared by the provider constructors and the flow, which both refuse the value with `InvalidTokenRequestParamsError`, @@ -82,6 +147,33 @@ module MCP nil end + + # Builds the connection the flow uses for its own requests: the SDK's defaults, `RequestedURLStamp`, + # then `customizer` (a provider's `http_client_customizer`, called with the `Faraday::Connection`), + # then `RequestedOriginGuard` last so it sees what the customizer's middleware does to each request + # after the stamp recorded it. Every request on the connection passes through both, so a caller using + # it directly is held to the same origin rule. + # + # Deliberately built without redirect-following middleware. Every destination check in this class runs + # against the URL as written, before the request goes out, so a connection that transparently followed + # a `3xx` would let a server reach a host the checks just refused. The guard turns following at + # the middleware level into a refusal; following inside an adapter stays invisible, so a customizer must + # not enable it. + # + # `Accept-Encoding` is deliberately left unset. `Net::HTTP::GenericRequest` negotiates it and decodes + # the response only while the caller has not claimed that header; assigning it turns `decode_content` off, + # which would silently move `BoundedBody`'s cap onto compressed bytes and let a small body expand past it + # after the check. + def build_http_client(customizer = nil) + require "faraday" + + Faraday.new do |faraday| + faraday.headers["Accept"] = "application/json" + faraday.use(RequestedURLStamp) + customizer&.call(faraday) + faraday.use(RequestedOriginGuard) + end + end end def initialize(provider:, http_client_factory: nil) @@ -1255,22 +1347,18 @@ module MCP @http_client ||= @http_client_factory.call end - # Deliberately built without redirect-following middleware. Every destination check in - # this class runs against the URL as written, before the request goes out, so a connection - # that transparently followed a `3xx` would let a server reach a host the checks just refused. - # A caller passing `http_client_factory:` takes on that responsibility: add redirect following here - # and the guards above only cover the first hop. - # - # `Accept-Encoding` is deliberately left unset. `Net::HTTP::GenericRequest` negotiates it and decodes - # the response only while the caller has not claimed that header; assigning it turns `decode_content` off, - # which would silently move `BoundedBody`'s cap onto compressed bytes and let a small body expand past it - # after the check. + # A connection supplied through `http_client_factory:` replaces this one, the provider's customizer and + # `RequestedOriginGuard` included, so that caller takes on the redirect responsibility described on + # `build_http_client`; `bounded_request` caps its responses all the same. def default_http_client - require "faraday" + self.class.build_http_client(provider_http_client_customizer) + end - Faraday.new do |faraday| - faraday.headers["Accept"] = "application/json" - end + # `nil` for a provider that predates the hook or leaves it unset. + def provider_http_client_customizer + return unless @provider.respond_to?(:http_client_customizer) + + @provider.http_client_customizer end def response_body_string(response) diff --git a/lib/mcp/client/oauth/id_jag_token_exchange.rb b/lib/mcp/client/oauth/id_jag_token_exchange.rb index 57701ba..ea771a9 100644 --- a/lib/mcp/client/oauth/id_jag_token_exchange.rb +++ b/lib/mcp/client/oauth/id_jag_token_exchange.rb @@ -95,7 +95,7 @@ module MCP assertion end - # `Accept-Encoding` is deliberately left unset, for the same reason as `Flow#default_http_client`: + # `Accept-Encoding` is deliberately left unset, for the same reason as `Flow.build_http_client`: # claiming that header turns Net::HTTP's `decode_content` off and would move `BoundedBody`'s cap # onto compressed bytes. def default_http_client diff --git a/lib/mcp/client/oauth/provider.rb b/lib/mcp/client/oauth/provider.rb index 16f8c4e..803b5b9 100644 --- a/lib/mcp/client/oauth/provider.rb +++ b/lib/mcp/client/oauth/provider.rb @@ -53,6 +53,8 @@ module MCP # the grant itself. The authorization request is not affected. A key in `Flow::RESERVED_TOKEN_REQUEST_PARAMS` # raises `Flow::InvalidTokenRequestParamsError`. # The Hash is copied and frozen. See `StorageBackedProvider#token_request_params`. + # - `http_client_customizer` - Callable invoked with the `Faraday::Connection` the flow builds for + # its own requests; see `StorageBackedProvider#http_client_customizer`. class Provider include StorageBackedProvider @@ -93,7 +95,8 @@ module MCP storage: nil, client_id_metadata_document_url: nil, authorization_request_validator: nil, - token_request_params: nil + token_request_params: nil, + http_client_customizer: nil ) unless Discovery.secure_url?(redirect_uri) raise InsecureRedirectURIError, @@ -115,6 +118,8 @@ module MCP "per the MCP authorization specification and `draft-ietf-oauth-client-id-metadata-document`." end + http_client_customizer = validated_http_client_customizer(http_client_customizer) + @client_metadata = client_metadata @redirect_uri = redirect_uri @redirect_handler = redirect_handler @@ -124,6 +129,7 @@ module MCP @client_id_metadata_document_url = client_id_metadata_document_url @authorization_request_validator = authorization_request_validator @token_request_params = frozen_token_request_params(token_request_params) + @http_client_customizer = http_client_customizer end # Identifies the OAuth flow this provider drives. diff --git a/lib/mcp/client/oauth/storage_backed_provider.rb b/lib/mcp/client/oauth/storage_backed_provider.rb index 2be8acc..087f223 100644 --- a/lib/mcp/client/oauth/storage_backed_provider.rb +++ b/lib/mcp/client/oauth/storage_backed_provider.rb @@ -28,6 +28,13 @@ module MCP # with `Flow::InvalidTokenRequestParamsError`. attr_reader :token_request_params + # Optional callable invoked with the `Faraday::Connection` the flow builds for its own requests, + # after the SDK's defaults and before its origin guard (see `Flow.build_http_client`), so an application can + # add middleware to discovery, registration, and token requests or swap their adapter. `nil` (the default) + # keeps the default connection. The transport's own connection and customizer block never serve these requests: + # they are bound to the MCP server URL and carry headers meant for that server. + attr_reader :http_client_customizer + def access_token tokens&.dig("access_token") || tokens&.dig(:access_token) end @@ -66,6 +73,14 @@ module MCP params.each_with_object({}) { |(key, value), copy| copy[key.dup.freeze] = value.dup.freeze }.freeze end + + # Returns `customizer` when it is `nil` or callable and raises otherwise, so a connection object passed + # in place of a callable fails at construction rather than at the first `401`. + def validated_http_client_customizer(customizer) + return customizer if customizer.nil? || customizer.respond_to?(:call) + + raise ArgumentError, "http_client_customizer must respond to call (got #{customizer.class})." + end end end end diff --git a/test/mcp/client/oauth/client_credentials_provider_test.rb b/test/mcp/client/oauth/client_credentials_provider_test.rb index 5ff4d20..6c679e3 100644 --- a/test/mcp/client/oauth/client_credentials_provider_test.rb +++ b/test/mcp/client/oauth/client_credentials_provider_test.rb @@ -49,6 +49,25 @@ module MCP assert_equal("client_secret_post", provider.client_information["token_endpoint_auth_method"]) end + def test_initialize_keeps_the_http_client_customizer + customizer = ->(_faraday) {} + provider = ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", http_client_customizer: customizer) + + assert_same(customizer, provider.http_client_customizer) + assert_nil(ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret").http_client_customizer) + end + + def test_initialize_rejects_a_non_callable_http_client_customizer_before_writing_credentials + storage = InMemoryStorage.new + + error = assert_raises(ArgumentError) do + ClientCredentialsProvider.new(client_id: "cc-client", client_secret: "cc-secret", storage: storage, http_client_customizer: "recorder") + end + + assert_equal("http_client_customizer must respond to call (got String).", error.message) + assert_nil(storage.client_information) + end + def test_initialize_rejects_missing_client_id ["", " ", nil].each do |value| assert_raises(ClientCredentialsProvider::InvalidCredentialsError, "should reject #{value.inspect}") do 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 1283b62..ed49b47 100644 --- a/test/mcp/client/oauth/cross_app_access_provider_test.rb +++ b/test/mcp/client/oauth/cross_app_access_provider_test.rb @@ -49,6 +49,36 @@ module MCP ) end + def test_initialize_keeps_the_http_client_customizer + customizer = ->(_faraday) {} + provider = CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag" }, + http_client_customizer: customizer, + ) + + assert_same(customizer, provider.http_client_customizer) + assert_nil(build_provider.http_client_customizer) + end + + def test_initialize_rejects_a_non_callable_http_client_customizer_before_writing_credentials + storage = InMemoryStorage.new + + error = assert_raises(ArgumentError) do + CrossAppAccessProvider.new( + client_id: "xaa-client", + client_secret: "xaa-secret", + assertion_provider: ->(**) { "id-jag" }, + storage: storage, + http_client_customizer: "recorder", + ) + end + + assert_equal("http_client_customizer must respond to call (got String).", error.message) + assert_nil(storage.client_information) + end + def test_initialize_rejects_missing_client_id assert_raises(CrossAppAccessProvider::InvalidConfigurationError) do CrossAppAccessProvider.new( diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index c463fdc..1d2a670 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -85,12 +85,13 @@ module MCP ) end - def client_credentials_provider(token_endpoint_auth_method: "client_secret_basic", token_request_params: nil) + def client_credentials_provider(token_endpoint_auth_method: "client_secret_basic", token_request_params: nil, http_client_customizer: nil) ClientCredentialsProvider.new( client_id: "cc-client", client_secret: "cc-secret", token_endpoint_auth_method: token_endpoint_auth_method, token_request_params: token_request_params, + http_client_customizer: http_client_customizer, ) end @@ -141,6 +142,118 @@ module MCP end end + # Records `[method, url]` for every request that passes through, standing in for the tracing middleware + # an application adds through `http_client_customizer`. + class RecordingMiddleware + def initialize(app, log) + @app = app + @log = log + end + + def call(env) + @log << [env.method, env.url.to_s] + @app.call(env) + end + end + + # Sends the request for `from` to `to` instead, the shape redirect-following middleware leaves behind: + # the flow asked for one URL and the adapter is handed another. + class URLRewritingMiddleware + def initialize(app, from:, to:) + @app = app + @from = from + @to = to + end + + def call(env) + env.url = URI(@to) if env.url.to_s == @from + @app.call(env) + end + end + + # Replaces the per-request context the way instrumentation middleware may; the guard must not depend on it. + class ContextReplacingMiddleware + def initialize(app) + @app = app + end + + def call(env) + env.request.context = { tag: "instrumented" } + @app.call(env) + end + end + + # Hands the next middleware an environment rebuilt from Faraday's own members only, dropping whatever + # an earlier middleware recorded on the original. + class EnvRebuildingMiddleware + def initialize(app) + @app = app + end + + def call(env) + @app.call(Faraday::Env.from(env.to_h)) + end + end + + # Follows a `3xx` the way `faraday-follow_redirects` does: duplicates the environment, points it at the `Location`, + # and re-enters the stack from its own position. + class RedirectFollowingMiddleware + def initialize(app) + @app = app + end + + def call(env) + response = @app.call(env) + location = response.headers["Location"] + return response unless location && (300..399).cover?(response.status) + + redirected = env.dup + redirected.url = URI(location) + redirected.response = nil + @app.call(redirected) + end + end + + # Replaces the request options wholesale, the way middleware that resets timeouts or contexts might. + class RequestOptionsReplacingMiddleware + def initialize(app) + @app = app + end + + def call(env) + env.request = Faraday::RequestOptions.new + @app.call(env) + end + end + + # Re-enters the stack once with the same environment after a `5xx`, the way retry middleware does. + class RetryingMiddleware + def initialize(app) + @app = app + end + + def call(env) + response = @app.call(env) + return response unless response.status >= 500 + + env.response = nil + @app.call(env) + end + end + + # Records the application's `trace_id` from the per-request context, as a tracing middleware would. + class ContextRecordingMiddleware + def initialize(app, log) + @app = app + @log = log + end + + def call(env) + @log << env.request.context&.dig(:trace_id) + @app.call(env) + end + end + # Runs the full authorization flow and returns the `scope` query parameter # sent on the authorization request. The caller stubs the AS metadata; # this helper supplies a provider whose `grant_types` and optional pre-set @@ -190,6 +303,278 @@ module MCP end end + def test_run_sends_every_authorization_server_request_through_the_provider_customizer + log = [] + provider = client_credentials_provider(http_client_customizer: ->(faraday) { faraday.use(RecordingMiddleware, log) }) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_equal([[:get, @prm_url], [:get, @as_metadata_url], [:post, "#{@auth_base}/token"]], log) + + # The SDK's defaults are applied before the customizer runs. + assert_requested(:get, @prm_url, headers: { "Accept" => "application/json" }) + end + + def test_run_refuses_a_customized_connection_that_sends_a_request_off_the_origin + stub_request(:get, "https://other.example.com/prm.json").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { faraday.use(URLRewritingMiddleware, from: @prm_url, to: "https://other.example.com/prm.json") }, + ) + + error = assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_equal(<<~MESSAGE, error.message) + Request to \"#{@prm_url}\" would be sent to \"https://other.example.com/prm.json\", on a different origin; \ + middleware that follows redirects or rewrites URLs is refused. + MESSAGE + + # The guard sits before the adapter, so the request never leaves. + assert_not_requested(:get, "https://other.example.com/prm.json") + assert_not_requested(:post, "#{@auth_base}/token") + end + + def test_run_keeps_a_same_origin_rewrite + moved_url = "https://srv.example.com/prm-moved.json" + stub_request(:get, moved_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { faraday.use(URLRewritingMiddleware, from: @prm_url, to: moved_url) }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:get, moved_url) + end + + def test_run_refuses_a_rewritten_request_even_when_middleware_replaces_the_request_context + stub_request(:get, "https://other.example.com/prm.json").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { + faraday.use(ContextReplacingMiddleware) + faraday.use(URLRewritingMiddleware, from: @prm_url, to: "https://other.example.com/prm.json") + }, + ) + + assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_not_requested(:get, "https://other.example.com/prm.json") + end + + def test_run_refuses_a_redirect_off_the_origin_followed_by_middleware_placed_ahead_of_the_stamp + # `builder.insert(0, ...)` is where middleware that wants to be outermost puts itself, so a follower can + # land ahead of the SDK's stamp and re-enter the stack; the record of the first URL must survive that. + stub_request(:get, @prm_url).to_return(status: 302, headers: { "Location" => "https://other.example.com/prm.json" }) + stub_request(:get, "https://other.example.com/prm.json").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { faraday.builder.insert(0, RedirectFollowingMiddleware) }, + ) + + error = assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/would be sent to "https:\/\/other\.example\.com\/prm\.json", on a different origin/, error.message) + assert_not_requested(:get, "https://other.example.com/prm.json") + end + + def test_run_keeps_a_same_origin_redirect_followed_by_middleware_placed_ahead_of_the_stamp + moved_url = "https://srv.example.com/prm-moved.json" + stub_request(:get, @prm_url).to_return(status: 302, headers: { "Location" => moved_url }) + stub_request(:get, moved_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { faraday.builder.insert(0, RedirectFollowingMiddleware) }, + ) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:get, moved_url) + end + + def test_run_refuses_a_request_whose_environment_was_rebuilt + provider = client_credentials_provider(http_client_customizer: ->(faraday) { faraday.use(EnvRebuildingMiddleware) }) + + error = assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_equal(<<~MESSAGE, error.message) + Request to \"#{@prm_url}\" carries no record of the URL the flow asked for; \ + middleware that rebuilds the request environment is refused. + MESSAGE + assert_not_requested(:get, @prm_url) + end + + def test_run_refuses_a_rewritten_token_request + stub_request(:post, "https://other.example.com/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "stolen", token_type: "Bearer"), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { + faraday.use(URLRewritingMiddleware, from: "#{@auth_base}/token", to: "https://other.example.com/token") + }, + ) + + assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_not_requested(:post, "https://other.example.com/token") + assert_not_requested(:post, "#{@auth_base}/token") + assert_nil(provider.access_token) + end + + def test_run_keeps_the_application_request_context_for_every_request + trace_ids = [] + + result = run_authorization_flow( + http_client_customizer: ->(faraday) { + faraday.options.context = { trace_id: "incident-123" } + faraday.use(ContextRecordingMiddleware, trace_ids) + }, + ) + + assert_equal(:authorized, result) + # Protected Resource Metadata and authorization server metadata (GET), registration (JSON POST), + # and the token exchange (form POST) all carry it. + assert_equal(["incident-123"] * 4, trace_ids) + end + + def test_run_keeps_the_application_request_context_on_a_factory_connection + trace_ids = [] + factory = -> { + Faraday.new do |faraday| + faraday.options.context = { trace_id: "incident-123" } + faraday.use(ContextRecordingMiddleware, trace_ids) + end + } + + result = Flow.new(provider: client_credentials_provider, http_client_factory: factory).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_equal(["incident-123"] * 3, trace_ids) + end + + def test_run_refuses_a_rewritten_request_even_when_middleware_replaces_the_request_options + stub_request(:get, "https://other.example.com/prm.json").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider( + http_client_customizer: ->(faraday) { + faraday.use(RequestOptionsReplacingMiddleware) + faraday.use(URLRewritingMiddleware, from: @prm_url, to: "https://other.example.com/prm.json") + }, + ) + + error = assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + # The mismatch, not a lost record: replacing the options must leave the record in place. + assert_match(/would be sent to "https:\/\/other\.example\.com\/prm\.json", on a different origin/, error.message) + assert_not_requested(:get, "https://other.example.com/prm.json") + end + + def test_run_refuses_a_redirect_off_the_origin_followed_by_middleware_placed_after_the_stamp + stub_request(:get, @prm_url).to_return(status: 302, headers: { "Location" => "https://other.example.com/prm.json" }) + stub_request(:get, "https://other.example.com/prm.json").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider(http_client_customizer: ->(faraday) { faraday.use(RedirectFollowingMiddleware) }) + + error = assert_raises(Flow::DestinationMismatchError) do + Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + # The mismatch, not a lost record: the duplicate the follower re-enters with shares the record. + assert_match(/would be sent to "https:\/\/other\.example\.com\/prm\.json", on a different origin/, error.message) + assert_not_requested(:get, "https://other.example.com/prm.json") + end + + def test_run_keeps_a_same_origin_redirect_followed_by_middleware_placed_after_the_stamp + moved_url = "https://srv.example.com/prm-moved.json" + stub_request(:get, @prm_url).to_return(status: 302, headers: { "Location" => moved_url }) + stub_request(:get, moved_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + provider = client_credentials_provider(http_client_customizer: ->(faraday) { faraday.use(RedirectFollowingMiddleware) }) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:get, moved_url) + end + + def test_run_keeps_a_retry_by_middleware_placed_ahead_of_the_stamp + # The same environment re-enters the stack with the same URL, so the first record still matches. + stub_request(:get, @prm_url).to_return( + { status: 503 }, + { + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + }, + ) + provider = client_credentials_provider(http_client_customizer: ->(faraday) { faraday.builder.insert(0, RetryingMiddleware) }) + + result = Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + assert_requested(:get, @prm_url, times: 2) + end + + def test_build_http_client_records_each_request_on_its_own + # A caller using the SDK-built connection directly may address different origins from one request + # to the next; each request gets its own record. + stub_request(:get, "https://a.example.com/one").to_return(status: 200, body: "one") + stub_request(:get, "https://b.example.com/two").to_return(status: 200, body: "two") + connection = Flow.build_http_client + + assert_equal("one", connection.get("https://a.example.com/one").body) + assert_equal("two", connection.get("https://b.example.com/two").body) + end + + def test_run_prefers_an_explicit_http_client_factory_over_the_customizer + provider = client_credentials_provider(http_client_customizer: ->(_faraday) { raise "the customizer must not run" }) + + result = Flow.new(provider: provider, http_client_factory: -> { Faraday.new }).run!(server_url: @server_url, resource_metadata_url: @prm_url) + + assert_equal(:authorized, result) + end + def test_run_client_credentials_with_client_secret_post_sends_credentials_in_body # `client_secret_post` puts the credentials in the form body rather # than an HTTP Basic header (RFC 6749 Section 2.3.1). @@ -586,7 +971,7 @@ module MCP # the Dynamic Client Registration request body. The default loopback redirect URI # exercises SEP-837's `"native"` inference; passing an HTTPS `redirect_uri` exercises # the `"web"` inference. - def run_authorization_flow(redirect_uri: "http://localhost:0/callback", client_metadata_extra: {}) + def run_authorization_flow(redirect_uri: "http://localhost:0/callback", client_metadata_extra: {}, http_client_customizer: nil) state_holder = {} provider = Provider.new( client_metadata: { @@ -598,6 +983,7 @@ module MCP redirect_uri: redirect_uri, redirect_handler: ->(url) { state_holder[:state] = URI.decode_www_form(url.query).to_h.fetch("state") }, callback_handler: -> { ["test-auth-code", state_holder[:state]] }, + http_client_customizer: http_client_customizer, ) Flow.new(provider: provider).run!(server_url: @server_url, resource_metadata_url: @prm_url) diff --git a/test/mcp/client/oauth/http_oauth_test.rb b/test/mcp/client/oauth/http_oauth_test.rb index 5b1be13..1edab1e 100644 --- a/test/mcp/client/oauth/http_oauth_test.rb +++ b/test/mcp/client/oauth/http_oauth_test.rb @@ -11,6 +11,46 @@ module MCP class Client module OAuth class HTTPOAuthTest < Minitest::Test + # Records `[method, url]` for every request that passes through, standing in for the tracing + # middleware an application adds through `http_client_customizer`. + class RecordingMiddleware + def initialize(app, log) + @app = app + @log = log + end + + def call(env) + @log << [env.method, env.url.to_s] + @app.call(env) + end + end + + # Sends the request for `from` to `to` instead, the shape redirect-following middleware leaves behind. + class URLRewritingMiddleware + def initialize(app, from:, to:) + @app = app + @from = from + @to = to + end + + def call(env) + env.url = URI(@to) if env.url.to_s == @from + @app.call(env) + end + end + + # Hands the next middleware an environment rebuilt from Faraday's own members only, dropping whatever + # an earlier middleware recorded on the original. + class EnvRebuildingMiddleware + def initialize(app) + @app = app + end + + def call(env) + @app.call(Faraday::Env.from(env.to_h)) + end + end + def setup WebMock.enable! @mcp_url = "https://srv.example.com/mcp" @@ -101,6 +141,91 @@ module MCP assert_equal("test-token-after-flow", provider.access_token) end + def test_send_request_runs_the_oauth_flow_through_the_provider_customizer + stub_request(:post, @mcp_url).with { |req| + req.headers["Authorization"].nil? + }.to_return( + status: 401, + headers: { + "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}"), + }, + body: "", + ) + + stub_request(:post, @mcp_url).with( + headers: { "Authorization" => "Bearer test-token-after-flow" } + ).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(jsonrpc: "2.0", id: "1", result: { ok: true }), + ) + + stub_request(:get, @prm_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + + stub_request(:get, "#{@auth_base}/.well-known/oauth-authorization-server").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"], + ), + ) + + stub_request(:post, "#{@auth_base}/register").to_return( + status: 201, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(client_id: "test-client"), + ) + + stub_request(:post, "#{@auth_base}/token").to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(access_token: "test-token-after-flow", token_type: "Bearer", expires_in: 3600), + ) + + log = [] + state_holder = {} + provider = Provider.new( + client_metadata: { + client_name: "ruby-sdk-test", + 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: -> { ["test-auth-code", state_holder[:state]] }, + http_client_customizer: ->(faraday) { faraday.use(RecordingMiddleware, log) }, + ) + + transport = HTTP.new(url: @mcp_url, oauth: provider) + response = transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + + assert_equal({ "ok" => true }, response["result"]) + assert_equal( + [ + [:get, @prm_url], + [:get, "#{@auth_base}/.well-known/oauth-authorization-server"], + [:post, "#{@auth_base}/register"], + [:post, "#{@auth_base}/token"], + ], + log, + ) + end + def test_send_request_does_not_follow_a_resource_metadata_challenge_off_the_server_origin # End to end over the transport, which is where the header is actually parsed: # a server that answers 401 must not be able to name an unrelated host in @@ -678,6 +803,94 @@ module MCP assert_equal("saved-rt", provider.tokens["refresh_token"]) end + def test_send_request_surfaces_a_refused_refresh_without_falling_back + # A customizer middleware that would carry the refresh request off the origin is refused before + # the request leaves, and that refusal is not one of the failures that start the interactive flow + # or discard the stored tokens. + stub_request(:post, @mcp_url).to_return( + status: 401, + headers: { "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}") }, + body: "", + ) + + stub_request(:get, @prm_url).to_return( + status: 200, + headers: { "Content-Type" => "application/json" }, + body: JSON.generate(resource: "https://srv.example.com/mcp", authorization_servers: [@auth_base]), + ) + + stub_request(:get, "#{@auth_base}/.well-known/oauth-authorization-server").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", + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + ), + ) + + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + stub_request(:post, "https://other.example.com/token").to_raise(StandardError.new("the rewritten request must not leave.")) + + provider = build_provider( + http_client_customizer: ->(faraday) { + faraday.use(URLRewritingMiddleware, from: "#{@auth_base}/token", to: "https://other.example.com/token") + }, + ) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-token", "refresh_token" => "saved-rt") + + transport = HTTP.new(url: @mcp_url, oauth: provider) + + assert_raises(MCP::Client::OAuth::Flow::DestinationMismatchError) do + transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + end + + assert_not_requested(:post, "https://other.example.com/token") + assert_not_requested(:post, "#{@auth_base}/token") + assert_not_requested(:post, "#{@auth_base}/register") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + + def test_send_request_surfaces_a_refresh_whose_environment_was_rebuilt_without_falling_back + # The missing-record refusal takes the same route as the origin mismatch: out of `send_request`, + # with no interactive fallback and the stored tokens intact. + stub_request(:post, @mcp_url).to_return( + status: 401, + headers: { "WWW-Authenticate" => %(Bearer error="invalid_token", resource_metadata="#{@prm_url}") }, + body: "", + ) + stub_request(:get, @prm_url).to_raise(StandardError.new("the rebuilt request must not leave.")) + stub_request(:post, "#{@auth_base}/register").to_raise(StandardError.new("DCR should not be called.")) + + interactive_flow_started = false + provider = Provider.new( + client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, + redirect_uri: "http://localhost:0/callback", + redirect_handler: ->(_url) { interactive_flow_started = true }, + callback_handler: -> { ["code", "state"] }, + http_client_customizer: ->(faraday) { faraday.use(EnvRebuildingMiddleware) }, + ) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-token", "refresh_token" => "saved-rt") + + transport = HTTP.new(url: @mcp_url, oauth: provider) + + error = assert_raises(MCP::Client::OAuth::Flow::DestinationMismatchError) do + transport.send_request(request: { jsonrpc: "2.0", id: "1", method: "tools/list" }) + end + + assert_match(/carries no record of the URL the flow asked for/, error.message) + refute(interactive_flow_started, "the refusal must not fall back to the interactive flow") + assert_not_requested(:get, @prm_url) + assert_not_requested(:post, "#{@auth_base}/token") + assert_not_requested(:post, "#{@auth_base}/register") + assert_equal({ "access_token" => "stale-token", "refresh_token" => "saved-rt" }, provider.tokens) + end + def test_send_request_preserves_refresh_token_when_refresh_hits_a_transient_failure # A 5xx (or any non-`invalid_grant`) from the token endpoint indicates # a transient AS outage, NOT that the refresh token is dead. @@ -1656,12 +1869,13 @@ module MCP provider end - def build_provider + def build_provider(http_client_customizer: nil) Provider.new( client_metadata: { redirect_uris: ["http://localhost:0/callback"] }, redirect_uri: "http://localhost:0/callback", redirect_handler: ->(_url) {}, callback_handler: -> { ["code", "state"] }, + http_client_customizer: http_client_customizer, ) end end diff --git a/test/mcp/client/oauth/provider_test.rb b/test/mcp/client/oauth/provider_test.rb index d9fa64e..7425131 100644 --- a/test/mcp/client/oauth/provider_test.rb +++ b/test/mcp/client/oauth/provider_test.rb @@ -38,6 +38,26 @@ module MCP end end + def test_initialize_defaults_http_client_customizer_to_nil + assert_nil(Provider.new(**args_for("https://app.example.com/callback")).http_client_customizer) + end + + def test_initialize_keeps_the_http_client_customizer + customizer = ->(_faraday) {} + provider = Provider.new(**args_for("https://app.example.com/callback"), http_client_customizer: customizer) + + assert_same(customizer, provider.http_client_customizer) + end + + def test_initialize_rejects_a_non_callable_http_client_customizer + # A connection object in place of a callable is the likely mistake. + error = assert_raises(ArgumentError) do + Provider.new(**args_for("https://app.example.com/callback"), http_client_customizer: Object.new) + end + + assert_equal("http_client_customizer must respond to call (got Object).", error.message) + end + def test_initialize_rejects_non_loopback_http_redirect_uri # Communication Security: a non-loopback `http://` redirect URI would # let an attacker steal the authorization code from a network sniffer, From 5081cc562618c518ddf587f2dfa72447175c075b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Fri, 18 Sep 2026 03:49:09 +0900 Subject: [PATCH 16/17] Surface unreachable Protected Resource Metadata instead of falling back to legacy discovery ## Motivation and Context `Flow#locate_authorization_server` treated every failure to fetch Protected Resource Metadata as "the server publishes none" and took the legacy 2025-03-26 path, where the MCP server's origin becomes the authorization base URL. A request that never reached the server, or a `5xx` or `429`, says nothing about what the server publishes, so a transient failure moved the flow to a different authorization server, discarded client information and tokens bound to the real one on the way, and surfaced as an unrelated error, such as a failed registration at the origin, rather than as the fetch failure itself. The comment claimed both reference SDKs behave that way, which is not so: outside browsers the TypeScript SDK propagates network errors from discovery, and the Python SDK propagates them and refuses the legacy path after a `5xx` or `429` from any candidate. Discovery now keeps trying candidate URLs until one serves a JSON object and classifies the outcome once they are exhausted. Like the Python SDK, it remembers a `5xx` or `429` from any candidate before deciding on the legacy path; moving on to the next candidate after a network error is what the loop already did. `MetadataNotPublishedError` (a `4xx` other than `429`, or a body that is not a JSON object) is the only failure that selects the legacy path; `MetadataUnreachableError` (a network error, or a `5xx` or `429`, from any candidate) is raised instead. Both subclass `AuthorizationError`, so callers rescuing that class keep working, and `MCP::Client::HTTP` still falls back from a failed refresh to the full flow, which repeats discovery and raises the same way unless the server has recovered. A body over the response cap is refused outright with a plain `AuthorizationError`, as it already was on the modern path. The message lists every candidate's failure, with the URL stripped of userinfo, query and fragment and cut to a fixed length, but otherwise spelled as requested, and with transport and parser messages bounded, as token endpoint diagnostics already are. The default legacy endpoints for an origin whose own metadata document is absent are unchanged. ## How Has This Been Tested? New tests in `test/mcp/client/oauth/flow_test.rb` and `test/mcp/client/oauth/discovery_test.rb`. Against the library before this change, discovery in the scenarios these tests set up took the legacy path instead of raising. ## Breaking Changes When no Protected Resource Metadata candidate serves a JSON object, a request that failed to reach the server, or returned a `5xx` or `429`, now raises instead of attempting the legacy 2025-03-26 discovery at the MCP server's origin, and a Protected Resource Metadata body over the response cap is refused instead of triggering that fallback. --- docs/_client/authorization.md | 2 + lib/mcp/client/oauth/discovery.rb | 23 +++ lib/mcp/client/oauth/flow.rb | 111 ++++++++---- test/mcp/client/oauth/discovery_test.rb | 13 ++ test/mcp/client/oauth/flow_test.rb | 226 +++++++++++++++++++++++- 5 files changed, 332 insertions(+), 43 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index ebd3014..11e9215 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -39,6 +39,8 @@ pass an `MCP::Client::OAuth::Provider` to the transport instead of a static `Aut - Fall back to the legacy 2025-03-26 discovery when the server publishes no Protected Resource Metadata, matching the TypeScript and Python SDKs: the MCP server's origin acts as the authorization base URL, its metadata is fetched from `/.well-known/oauth-authorization-server` and must name that origin as its `issuer` (RFC 8414 Section 3.3), and when even that is absent the spec's default endpoints `/authorize`, `/token`, and `/register` at the origin are used with PKCE S256 assumed. + When no Protected Resource Metadata candidate serves a JSON object, a request that could not reach the server, or that returned a `5xx` or `429`, + raises `Flow::MetadataUnreachableError` instead of triggering that fallback, and a body over the response cap is refused outright. - On subsequent 401s with a saved `refresh_token`, exchange it at the token endpoint before falling back to the full interactive flow (RFC 6749 Section 6). - On a `403 Forbidden` whose `WWW-Authenticate` header carries `error="insufficient_scope"` (OAuth 2.0 step-up, RFC 6750 Section 3.1 and the MCP scope-selection-strategy), run a fresh authorization request for the union of the currently granted scope and the scope named in the challenge, then retry the failed request once. diff --git a/lib/mcp/client/oauth/discovery.rb b/lib/mcp/client/oauth/discovery.rb index 04030cc..a8ad3be 100644 --- a/lib/mcp/client/oauth/discovery.rb +++ b/lib/mcp/client/oauth/discovery.rb @@ -336,6 +336,29 @@ module MCP uri.to_s end + # Drops userinfo, query and fragment from `url` and leaves the host and path spelled as given, for reporting + # a URL next to the request that failed, where it must still match the server's access log; `URI#to_s` + # still lowercases the scheme and drops an explicit default port. Unlike `canonicalize_origin_and_path` + # it neither normalizes nor resolves dot segments, so its cost stays linear in the URL length, which matters + # for a URL the server chose. A URL that does not parse is not echoed, since the raw value could carry + # the very credentials being dropped. + def redact_url(url) + uri = URI.parse(url.to_s) + + uri.fragment = nil + uri.query = nil + # `URI::Generic#userinfo=` is a no-op on Ruby 2.7 (the project's minimum supported version), + # so clear the components individually. + if uri.respond_to?(:user) && (uri.user || uri.password) + uri.user = nil + uri.password = nil + end + + uri.to_s + rescue URI::Error + "[unparseable URL]" + end + # Returns true when `prm` (a PRM `resource` URL) covers `server` (the MCP endpoint URL): # same scheme/host/port, with PRM's path being a prefix of the server's path. When PRM # also advertises a query string, the server's query MUST be identical to it (otherwise diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 0d38f2d..37c0cce 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -16,6 +16,8 @@ module MCP class Flow TOKEN_ENDPOINT_ERROR_MAX_LENGTH = 128 TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH = 512 + METADATA_DIAGNOSTIC_MAX_LENGTH = 128 + METADATA_URL_MAX_LENGTH = 2048 # Token request parameters the flow sets itself. Its values win over a provider's `token_request_params`, # so a provider naming one of these is refused rather than left believing its value was sent. @@ -57,6 +59,17 @@ module MCP # or authorization server metadata failure by rescuing a class rather than by matching the message text. class AuthorizationRefusedError < AuthorizationError; end + # Raised by metadata discovery when every candidate URL answered that nothing usable is published there + # (a `4xx` other than `429`, a redirect that was not followed, or a body that is not a JSON object). + # The only discovery failure that may select the legacy 2025-03-26 path. + class MetadataNotPublishedError < AuthorizationError; end + + # Raised by metadata discovery when the answer says nothing about what is published: the request failed to + # reach the server, or a candidate answered `5xx` or `429`. Falling back on this would move + # the flow to a different authorization server because of a transient failure, so it is surfaced instead, + # as the TypeScript SDK does for network errors outside browsers and the Python SDK does for both. + class MetadataUnreachableError < AuthorizationError; end + # Raised for a `token_request_params` value the SDK refuses: a reserved key, a Hash comparing keys by identity, # or anything but a Hash of Strings. An `ArgumentError` because the value is a configuration mistake, # not a failed authorization, and deliberately outside `AuthorizationError`, which `MCP::Client::HTTP` treats on @@ -375,7 +388,12 @@ module MCP # # Legacy path (2025-03-26 backwards compatibility): when the server publishes no PRM, `prm` is nil # and the MCP server's own origin acts as the authorization base URL, matching the TypeScript and Python SDKs. - # Any PRM discovery failure (404s, network errors, malformed documents) selects the legacy path, mirroring both SDKs' behavior. + # Only a discovery answer saying that nothing usable is published (`MetadataNotPublishedError`: a `4xx` other than `429`, + # a redirect that was not followed, or a body that is not a JSON object) selects the legacy path. + # A request that failed to reach the server, or returned a `5xx` or `429`, says nothing about what the server publishes, + # so once no candidate has served a usable document it is surfaced instead (`MetadataUnreachableError`), + # as both SDKs do for network errors (the TypeScript SDK outside browsers) and the Python SDK does for server errors. + # A body over the response cap is refused outright and never reaches the fallback either. # https://modelcontextprotocol.io/specification/2025-03-26/basic/authorization#fallbacks-for-servers-without-metadata-discovery def locate_authorization_server(server_url:, resource_metadata_url:) prm = begin @@ -383,7 +401,7 @@ module MCP server_url: server_url, resource_metadata_url: resource_metadata_url, ) - rescue AuthorizationError + rescue MetadataNotPublishedError nil end @@ -504,44 +522,63 @@ module MCP first end - # Walks candidate metadata URLs and returns the parsed JSON body of - # the first 2xx response. Raises `AuthorizationError` for transport - # failures (`Faraday::Error`) and malformed bodies (`JSON::ParserError`) - # so callers do not have to handle raw Faraday/JSON exceptions. + # Walks candidate metadata URLs and returns the parsed body of the first 2xx response that is a JSON object; + # the caller checks its fields. Candidates are tried until one serves such a body, since a later one may still + # be usable when an earlier one is broken or down (the URL from `WWW-Authenticate` against the well-known path, + # or the OAuth document against the OpenID one). Once the candidates are exhausted, an answer that said nothing + # about what is published (a network error, a `5xx`, or a `429`) outranks the rest and raises + # `MetadataUnreachableError`; otherwise (any other status, such as a `4xx` other than `429` or a redirect that + # was not followed, a body that is not JSON, or not a JSON object) `MetadataNotPublishedError`. + # A body over the cap is refused outright by `bounded_request` with a plain `AuthorizationError`, + # before any classification. Each failure is listed with its URL stripped of userinfo, query and fragment + # and cut to `METADATA_URL_MAX_LENGTH`, but otherwise spelled as requested, so it can be matched against + # a server's access log, and with exception text bounded, since the message lands in every log destination + # the error passes through. def fetch_metadata_json(urls, label:) - last_error = nil + failures = [] + inconclusive = false urls.each do |url| response = begin http_get(url) rescue Faraday::Error => e - last_error = "GET #{url} raised #{e.class}: #{e.message}" + detail = bounded_diagnostic(e.message, limit: METADATA_DIAGNOSTIC_MAX_LENGTH) + failures << "GET #{reported_url(url)} raised #{[e.class, detail].compact.join(": ")}" + inconclusive = true next end - if response.status >= 200 && response.status < 300 - parsed = begin - JSON.parse(response_body_string(response)) - rescue JSON::ParserError => e - raise AuthorizationError, "Failed to parse #{label} from #{url}: #{e.message}." - end - - # Even valid JSON can be the wrong shape (a top-level array, - # a bare `null`, a string, ...). The discovery callers index by - # name (`prm["authorization_servers"]`, etc.), so anything that - # is not a Hash would raise `TypeError` / `NoMethodError` - # downstream. Surface that as `AuthorizationError` instead so - # callers see a single, documented error type. - unless parsed.is_a?(Hash) - raise AuthorizationError, - "#{label} from #{url} is not a JSON object (got #{parsed.class})." - end - - return parsed + unless response.status >= 200 && response.status < 300 + failures << "GET #{reported_url(url)} returned #{response.status}" + inconclusive = true if response.status >= 500 || response.status == 429 + next end - last_error = "GET #{url} returned #{response.status}" + parsed = begin + JSON.parse(response_body_string(response)) + rescue JSON::ParserError => e + detail = bounded_diagnostic(e.message, limit: METADATA_DIAGNOSTIC_MAX_LENGTH) || e.class.name + failures << "GET #{reported_url(url)} returned a body that is not JSON: #{detail}" + next + end + + # Even valid JSON can be the wrong shape (a top-level array, a bare `null`, a string, ...). + # The discovery callers index by name (`prm["authorization_servers"]`, etc.), so anything that + # is not a Hash would raise `TypeError` / `NoMethodError` downstream. + unless parsed.is_a?(Hash) + failures << "GET #{reported_url(url)} returned a body that is not a JSON object (got #{parsed.class})" + next + end + + return parsed + end + + message = "Failed to fetch #{label}: #{failures.join("; ")}." + + if inconclusive + raise MetadataUnreachableError, message + else + raise MetadataNotPublishedError, message end - raise AuthorizationError, "Failed to fetch #{label}: #{last_error}." end def ensure_pkce_supported!(as_metadata) @@ -1188,8 +1225,8 @@ module MCP parsed = {} unless parsed.is_a?(Hash) error_class = parsed["error"] == "invalid_grant" ? InvalidGrantError : AuthorizationError - error = token_endpoint_diagnostic(parsed["error"], limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH) - description = token_endpoint_diagnostic(parsed["error_description"], limit: TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH) + error = bounded_diagnostic(parsed["error"], limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH) + description = bounded_diagnostic(parsed["error_description"], limit: TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH) message += " #{[error, description].compact.join(": ")}" if error || description error_class.new(message, http_status: response.status, error: error, error_description: description) @@ -1198,17 +1235,23 @@ module MCP error_class.new("Token endpoint returned status #{response.status}.", http_status: response.status) end - def token_endpoint_diagnostic(value, limit:) + def bounded_diagnostic(value, limit:) return unless value.is_a?(String) - # RFC 6749 permits printable ASCII except double quotes and backslashes. - # Replace other characters to keep provider text on one log line. + # RFC 6749 permits printable ASCII except double quotes and backslashes in token endpoint error fields. + # Replace other characters to keep text received off the network on one log line. value = value.scrub(" ").gsub(/[^\x20-\x21\x23-\x5B\x5D-\x7E]/, " ").strip return if value.empty? value.length > limit ? "#{value[0, limit - 3]}..." : value end + # A candidate URL as it goes into a failure string: redacted, and cut so a URL the server chose cannot + # grow the message without limit. + def reported_url(url) + bounded_diagnostic(Discovery.redact_url(url), limit: METADATA_URL_MAX_LENGTH) + end + # Per RFC 6749 Section 2.3.1, the `client_id` and `client_secret` MUST be # `application/x-www-form-urlencoded` encoded before they are joined with # `:` and base64-encoded for the `Authorization: Basic` header. This is diff --git a/test/mcp/client/oauth/discovery_test.rb b/test/mcp/client/oauth/discovery_test.rb index c8d9fda..d58c7f2 100644 --- a/test/mcp/client/oauth/discovery_test.rb +++ b/test/mcp/client/oauth/discovery_test.rb @@ -259,6 +259,19 @@ module MCP ) end + def test_redact_url_drops_credentials_query_and_fragment_but_keeps_the_spelling + # Reported next to a failed request, the URL must still match the server's access log, so the host + # and path are left alone: no lowercasing of the host, no dot-segment resolution, no decoding. + assert_equal( + "https://Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson", + Discovery.redact_url("https://user:pass@Srv.Example.COM:8443/tenant/%2e%2e/prm%2Ejson?token=abc#frag"), + ) + end + + def test_redact_url_does_not_echo_a_url_it_cannot_parse + assert_equal("[unparseable URL]", Discovery.redact_url("https://user:pass@srv.example.com/pr m.json?token=abc")) + end + def test_canonicalize_url_normalizes_query_to_match_faraday # Faraday rewrites `env.url` before sending a request: it sorts # parameters by name, uppercases percent-encoded hex, and drops diff --git a/test/mcp/client/oauth/flow_test.rb b/test/mcp/client/oauth/flow_test.rb index 26adb60..5a41b3b 100644 --- a/test/mcp/client/oauth/flow_test.rb +++ b/test/mcp/client/oauth/flow_test.rb @@ -1084,9 +1084,9 @@ module MCP end def test_run_falls_back_to_legacy_discovery_when_prm_is_not_a_json_object - # Valid JSON but the wrong shape. Any PRM discovery failure selects the legacy 2025-03-26 path - # (matching the TypeScript and Python SDKs); here the legacy path also dead-ends, surfacing - # a domain error rather than a raw `TypeError` from indexing the array. + # Valid JSON but the wrong shape counts as nothing usable being published, so it selects the legacy 2025-03-26 path + # (matching the TypeScript and Python SDKs); here the legacy path also dead-ends, surfacing a domain error rather than + # a raw `TypeError` from indexing the array. stub_request(:get, @prm_url).to_return( status: 200, headers: { "Content-Type" => "application/json" }, @@ -1118,6 +1118,211 @@ module MCP assert_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") end + def test_run_surfaces_a_network_error_during_prm_discovery_instead_of_falling_back + # A request that never reached the server says nothing about whether it publishes PRM, so once + # the other candidates answer 404 the flow stops instead of moving to the legacy authorization base; + # the TypeScript and Python SDKs propagate the error the same way. + challenge_url = "https://srv.example.com/prm.json" + stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused")) + stub_prm_not_found + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url) + end + + assert_match(/Faraday::ConnectionFailed/, error.message) + assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") + assert_not_requested(:post, "https://srv.example.com/register") + end + + def test_run_reports_a_prm_candidate_url_without_its_query_in_the_discovery_error + # The candidate URL comes from the server's `WWW-Authenticate` challenge and lands in every log line + # the error reaches, so the query is dropped, while the path keeps the spelling that was requested + # (`%2E` is not decoded) so the line can be matched against the server's access log. + challenge_url = "https://srv.example.com/prm%2Ejson?token=abc" + stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused")) + stub_prm_not_found + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url) + end + + assert_includes(error.message, "GET https://srv.example.com/prm%2Ejson raised Faraday::ConnectionFailed: connection refused; GET") + refute_includes(error.message, "token=abc") + end + + def test_run_cuts_an_overlong_prm_candidate_url_in_the_discovery_error + # The challenge URL is the server's to choose, so its length is bounded before it lands in the message. + challenge_url = "https://srv.example.com/#{"a" * 5000}/prm.json" + stub_request(:get, challenge_url).to_raise(Faraday::ConnectionFailed.new("connection refused")) + stub_prm_not_found + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url) + end + + assert_includes(error.message, "GET #{challenge_url[0, Flow::METADATA_URL_MAX_LENGTH - 3]}... raised Faraday::ConnectionFailed") + refute_includes(error.message, "a" * (Flow::METADATA_URL_MAX_LENGTH - 3)) + end + + def test_run_bounds_the_transport_error_message_in_the_discovery_error + # A caller-supplied client can raise with server-chosen text in the message (Faraday's `raise_error` middleware repeats + # the status line and the request URL), so it is bounded like the parser's message. + stub_request(:get, @prm_url).to_raise(Faraday::ConnectionFailed.new("x" * 5000)) + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_includes(error.message, "raised Faraday::ConnectionFailed: #{"x" * 125}...; GET") + refute_includes(error.message, "x" * 126) + end + + def test_run_bounds_the_parser_message_in_the_discovery_error + # json before 2.10 repeats the remaining source in `JSON::ParserError#message`, so a malformed body + # could otherwise put up to the response cap into the message once per candidate. + stub_request(:get, @prm_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "{") + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 503) + JSON.stubs(:parse).raises(JSON::ParserError, "unexpected token at '#{"{" * 5000}'") + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + # `METADATA_DIAGNOSTIC_MAX_LENGTH` is 128: the first 125 characters survive, then three dots. + assert_includes(error.message, <<~MESSAGE.chomp) + returned a body that is not JSON: unexpected token at '#{"{" * 104}...; \ + GET https://srv.example.com/.well-known/oauth-protected-resource returned 503. + MESSAGE + + refute_includes(error.message, "{" * 105) + end + + def test_run_surfaces_a_server_error_during_prm_discovery_instead_of_falling_back + # A `5xx` or `429` says nothing about what the server publishes either, even when a later candidate answers 404; + # the Python SDK refuses the legacy path the same way. + [503, 429].each do |status| + stub_request(:get, @prm_url).to_return(status: status) + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) + + error = assert_raises(Flow::MetadataUnreachableError, "status #{status}") do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/returned #{status}/, error.message) + end + + assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") + assert_not_requested(:post, "https://srv.example.com/register") + end + + def test_run_keeps_a_server_error_in_mind_when_a_later_candidate_is_unusable + # An unusable document from a later candidate must not turn an earlier `503` into "nothing published". + challenge_url = "https://srv.example.com/prm.json" + stub_request(:get, challenge_url).to_return(status: 503) + stub_request(:get, @prm_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "[]") + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url) + end + + assert_match(/returned 503/, error.message) + assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") + end + + def test_run_uses_a_later_prm_candidate_when_the_challenge_url_serves_an_unusable_document + # The setup stubs a valid document at the well-known path; a broken one at the challenge URL is skipped. + challenge_url = "https://srv.example.com/prm.json" + stub_request(:get, challenge_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: "[]") + + result = Flow.new(provider: build_legacy_discovery_provider({})).run!(server_url: @server_url, resource_metadata_url: challenge_url) + + assert_equal(:authorized, result) + assert_requested(:get, @prm_url) + end + + def test_run_recovers_authorization_server_metadata_from_a_later_candidate_after_a_network_error + # The OAuth document being unreachable must not stop the OpenID one from being tried. + stub_request(:get, @as_metadata_url).to_raise(Faraday::TimeoutError.new("execution expired")) + stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").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"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + ), + ) + + result = run_authorization_flow + + assert_equal(:authorized, result) + assert_requested(:get, "#{@auth_base}/.well-known/openid-configuration") + end + + def test_run_recovers_authorization_server_metadata_from_a_later_candidate_after_an_unusable_document + # An OAuth document that is not JSON, or not a JSON object, must not stop the OpenID one from being tried. + stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").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"], + code_challenge_methods_supported: ["S256"], + token_endpoint_auth_methods_supported: ["none"], + ), + ) + + ["{", "[]"].each do |body| + stub_request(:get, @as_metadata_url).to_return(status: 200, headers: { "Content-Type" => "application/json" }, body: body) + + assert_equal(:authorized, run_authorization_flow, "body #{body.inspect}") + end + + assert_requested(:get, "#{@auth_base}/.well-known/openid-configuration", times: 2) + end + + def test_refresh_surfaces_a_network_error_during_prm_discovery_instead_of_falling_back + stub_request(:get, @prm_url).to_raise(Faraday::TimeoutError.new("execution expired")) + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) + provider = build_legacy_discovery_provider({}) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") + assert_not_requested(:post, "https://srv.example.com/token") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + + def test_refresh_surfaces_a_server_error_during_prm_discovery_instead_of_falling_back + stub_request(:get, @prm_url).to_return(status: 503) + stub_request(:get, "https://srv.example.com/.well-known/oauth-protected-resource").to_return(status: 404) + provider = build_legacy_discovery_provider({}) + provider.save_client_information("client_id" => "test-client") + provider.save_tokens("access_token" => "stale-at", "refresh_token" => "saved-rt") + + error = assert_raises(Flow::MetadataUnreachableError) do + Flow.new(provider: provider).refresh!(server_url: @server_url, resource_metadata_url: @prm_url) + end + + assert_match(/returned 503/, error.message) + assert_not_requested(:post, "https://srv.example.com/token") + assert_equal("saved-rt", provider.tokens["refresh_token"]) + end + # Builds a provider for the legacy-discovery tests, capturing the authorization URL so tests can assert # which endpoint was used. def build_legacy_discovery_provider(holder) @@ -1511,10 +1716,10 @@ module MCP end def test_run_refuses_a_protected_resource_metadata_body_over_the_cap - # PRM discovery failures select the legacy path by design, so the refusal shows up as - # the fallback rather than as a raise. The padded document is valid JSON naming - # an authorization server, so contacting that server is exactly what would happen - # if the body had been read: the assertion below fails if the cap stops working. + # An oversized document is refused outright rather than treated as unpublished, so the flow stops + # without the legacy fallback. The padded document is valid JSON naming an authorization server, + # so contacting that server is exactly what would happen if the body had been read: + # the assertion below fails if the cap stops working. stub_request(:any, %r{\Ahttps://srv\.example\.com/}).to_return(status: 404) stub_request(:get, @prm_url).to_return( status: 200, @@ -1526,9 +1731,11 @@ module MCP ), ) - assert_raises(Flow::AuthorizationError) { run_authorization_flow } + error = assert_raises(Flow::AuthorizationError) { run_authorization_flow } + assert_match(/exceeds \d+ bytes/, error.message) assert_not_requested(:get, @as_metadata_url) + assert_not_requested(:get, "https://srv.example.com/.well-known/oauth-authorization-server") end def test_run_refuses_a_dynamic_client_registration_body_over_the_cap @@ -1632,6 +1839,7 @@ module MCP headers: { "Content-Type" => "application/json" }, body: "null", ) + stub_request(:get, "#{@auth_base}/.well-known/openid-configuration").to_return(status: 404) provider = Provider.new( client_metadata: { @@ -3283,7 +3491,7 @@ module MCP end def test_token_endpoint_errors_fall_back_when_diagnostic_extraction_raises - Flow.any_instance.stubs(:token_endpoint_diagnostic).raises(ArgumentError, "sensitive provider text") + Flow.any_instance.stubs(:bounded_diagnostic).raises(ArgumentError, "sensitive provider text") { "invalid_grant" => Flow::InvalidGrantError, "invalid_client" => Flow::AuthorizationError }.each do |code, klass| error = refresh_token_endpoint_error(JSON.generate(error: code, error_description: "details")) From 8173a38280b0c7c31b2ef5bc3e7dfd1f4ce7fa8b Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Sat, 19 Sep 2026 17:08:22 +0900 Subject: [PATCH 17/17] Validate the src and sizes of an icon against the specification ## Motivation and Context `MCP::Icon` validated `theme` but accepted anything for `src` and `sizes`: `src:` defaulted to `nil`, so `MCP::Icon.new.to_h` was `{}`, an icon with no source, and a `sizes` given as a String reached `to_h` unchanged and serialized as a JSON string where the specification's `Icon` type requires an array of strings. Nothing on the server side reported either; the server answered every request with 200 and the failure surfaced in a client, with no reference to the field responsible. The TypeScript SDK types `sizes` as `string[]` and `src` as required, and the Python SDK's model refuses such an icon at construction, so this SDK was the one that let it through. `src` is now a required keyword and must be a non-empty String, `sizes` must be `nil` or an Array of Strings, and `mime_type` must be `nil` or a String, each refused with `ArgumentError` at construction the way `theme` already was. The checks stop at the schema's types, as the reference SDKs do: the scheme of a URL and the `WxH` form of a size stay the caller's to get right. A message names the class of the rejected value, never the value, since `src` may carry a `data:` URI of any length. Coercing a String into a one-element Array was considered and rejected: it would hide the caller's mistake that the class exists to surface. The `theme` check let `false` through its truthiness test and serialized it; it now refuses everything but `nil`, `"light"`, and `"dark"`. Fixes #562. ## How Has This Been Tested? New tests in `test/mcp/icon_test.rb`. Against the previous library, an icon without `src` and an icon whose `sizes` is a String are both constructed and serialized. ## Breaking Changes `MCP::Icon.new` now raises `ArgumentError` where earlier releases produced an icon the specification's schema rejects: `src:` is required and must be a non-empty String, so a call without it, or with `nil`, an empty String, or a non-String, fails; `sizes:` must be `nil` or an Array of Strings, so a String, or an Array holding `nil` or a non-String, fails; `mime_type:` must be `nil` or a String; `theme:` must be `nil`, `"light"`, or `"dark"`, so `false` fails where it used to be serialized. Leaving the optional keywords out, or passing `nil` for them, is unchanged. --- lib/mcp/icon.rb | 37 ++++++++++++++++- test/mcp/icon_test.rb | 94 +++++++++++++++++++++++++++++++++++++++---- 2 files changed, 122 insertions(+), 9 deletions(-) diff --git a/lib/mcp/icon.rb b/lib/mcp/icon.rb index 3397c33..56ac6ad 100644 --- a/lib/mcp/icon.rb +++ b/lib/mcp/icon.rb @@ -1,13 +1,35 @@ # frozen_string_literal: true module MCP + # An icon attached to a server, tool, prompt, or resource, per the specification's `Icon` type: + # https://modelcontextprotocol.io/specification/2026-07-28/basic#icons + # + # Each argument is checked against the schema's type, as the TypeScript and Python SDKs check theirs, + # so an icon that would serialize into something a client rejects fails here instead. What a value means + # is not judged: `src` may be any non-empty `String` (the schema types it as a URI, which an empty `String` + # is not, and the specification allows an HTTP/HTTPS URL or a `data:` URI), and a size may be any `String` + # (the specification expects `WxH` or `"any"`). class Icon SUPPORTED_THEMES = ["light", "dark"].freeze attr_reader :mime_type, :sizes, :src, :theme - def initialize(mime_type: nil, sizes: nil, src: nil, theme: nil) - raise ArgumentError, 'The value of theme must specify "light" or "dark".' if theme && !SUPPORTED_THEMES.include?(theme) + def initialize(mime_type: nil, sizes: nil, src:, theme: nil) + unless src.is_a?(String) && !src.empty? + raise ArgumentError, "The value of src must be a non-empty String (got #{src.class})." + end + + unless mime_type.nil? || mime_type.is_a?(String) + raise ArgumentError, "The value of mime_type must be a String (got #{mime_type.class})." + end + + if (problem = sizes_problem(sizes)) + raise ArgumentError, "The value of sizes must be an Array of Strings such as [\"48x48\"] or [\"any\"] (#{problem})." + end + + unless theme.nil? || SUPPORTED_THEMES.include?(theme) + raise ArgumentError, 'The value of theme must specify "light" or "dark".' + end @mime_type = mime_type @sizes = sizes @@ -18,5 +40,16 @@ module MCP def to_h { mimeType: mime_type, sizes: sizes, src: src, theme: theme }.compact end + + private + + def sizes_problem(sizes) + return if sizes.nil? + return "got #{sizes.class}" unless sizes.is_a?(Array) + + index = sizes.index { |size| !size.is_a?(String) } + + "got #{sizes[index].class} inside the Array" if index + end end end diff --git a/test/mcp/icon_test.rb b/test/mcp/icon_test.rb index e8c837c..f2fbc9a 100644 --- a/test/mcp/icon_test.rb +++ b/test/mcp/icon_test.rb @@ -15,32 +15,112 @@ module MCP assert_equal({ mimeType: "image/png", sizes: ["48x48", "96x96"], src: "https://example.com", theme: "light" }, icon.to_h) end - def test_initialization_by_default - icon = Icon.new + def test_initialization_with_only_src + icon = Icon.new(src: "https://example.com/icon.png") assert_nil(icon.mime_type) assert_nil(icon.sizes) - assert_nil(icon.src) + assert_equal("https://example.com/icon.png", icon.src) assert_nil(icon.theme) - assert_equal({}, icon.to_h) + assert_equal({ src: "https://example.com/icon.png" }, icon.to_h) + end + + def test_src_is_required + exception = assert_raises(ArgumentError) do + Icon.new + end + assert_equal("missing keyword: :src", exception.message) + end + + def test_src_rejects_nil_and_an_empty_string + [nil, ""].each do |src| + exception = assert_raises(ArgumentError) do + Icon.new(src: src) + end + assert_equal("The value of src must be a non-empty String (got #{src.class}).", exception.message) + end + end + + def test_src_rejects_a_non_string + exception = assert_raises(ArgumentError) do + Icon.new(src: :icon) + end + assert_equal("The value of src must be a non-empty String (got Symbol).", exception.message) + end + + def test_sizes_accepts_an_array_of_strings + [["48x48", "96x96"], ["any"], []].each do |sizes| + icon = Icon.new(sizes: sizes, src: "https://example.com/icon.png") + + assert_equal(sizes, icon.to_h[:sizes]) + end + end + + # https://github.com/modelcontextprotocol/ruby-sdk/issues/562 + def test_sizes_rejects_a_string + exception = assert_raises(ArgumentError) do + Icon.new(mime_type: "image/png", sizes: "51x51", src: "https://example.com/icon.png") + end + assert_equal( + 'The value of sizes must be an Array of Strings such as ["48x48"] or ["any"] (got String).', + exception.message, + ) + end + + def test_sizes_rejects_an_array_holding_a_non_string + { + ["48x48", 96] => "Integer", + [nil] => "NilClass", + ["48x48", nil] => "NilClass", + [nil, 96] => "NilClass", + }.each do |sizes, offender| + exception = assert_raises(ArgumentError) do + Icon.new(sizes: sizes, src: "https://example.com/icon.png") + end + assert_equal( + "The value of sizes must be an Array of Strings such as [\"48x48\"] or [\"any\"] (got #{offender} inside the Array).", + exception.message, + ) + end + end + + def test_src_accepts_any_scheme_and_sizes_accept_any_string + string_subclass = Class.new(String) + icon = Icon.new(sizes: ["unconventional", +"48x48", string_subclass.new("any")], src: "custom:icon") + + assert_equal({ sizes: ["unconventional", "48x48", "any"], src: "custom:icon" }, icon.to_h) + end + + def test_mime_type_rejects_a_non_string + exception = assert_raises(ArgumentError) do + Icon.new(mime_type: :png, src: "https://example.com/icon.png") + end + assert_equal("The value of mime_type must be a String (got Symbol).", exception.message) end def test_valid_theme_for_light assert_nothing_raised do - Icon.new(theme: "light") + Icon.new(src: "https://example.com/icon.png", theme: "light") end end def test_valid_theme_for_dark assert_nothing_raised do - Icon.new(theme: "dark") + Icon.new(src: "https://example.com/icon.png", theme: "dark") end end def test_invalid_theme exception = assert_raises(ArgumentError) do - Icon.new(theme: "unexpected") + Icon.new(src: "https://example.com/icon.png", theme: "unexpected") + end + assert_equal('The value of theme must specify "light" or "dark".', exception.message) + end + + def test_theme_rejects_false + exception = assert_raises(ArgumentError) do + Icon.new(src: "https://example.com/icon.png", theme: false) end assert_equal('The value of theme must specify "light" or "dark".', exception.message) end