## 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.
237 lines
10 KiB
Ruby
237 lines
10 KiB
Ruby
# frozen_string_literal: true
|
|
|
|
require "test_helper"
|
|
require "mcp/client/oauth"
|
|
|
|
module MCP
|
|
class Client
|
|
module OAuth
|
|
class ProviderTest < Minitest::Test
|
|
# Builds a complete Provider argument hash whose `client_metadata`
|
|
# registers the same `redirect_uri` that is being asserted on.
|
|
# The registered-URI check in `Provider#initialize` requires the two to
|
|
# match, so accept-path tests need them paired.
|
|
def args_for(redirect_uri)
|
|
{
|
|
client_metadata: {
|
|
redirect_uris: [redirect_uri],
|
|
grant_types: ["authorization_code"],
|
|
response_types: ["code"],
|
|
token_endpoint_auth_method: "none",
|
|
},
|
|
redirect_uri: redirect_uri,
|
|
redirect_handler: ->(_url) {},
|
|
callback_handler: -> { ["code", "state"] },
|
|
}
|
|
end
|
|
|
|
def test_initialize_accepts_https_redirect_uri
|
|
provider = Provider.new(**args_for("https://app.example.com/callback"))
|
|
|
|
assert_equal("https://app.example.com/callback", provider.redirect_uri)
|
|
end
|
|
|
|
def test_initialize_accepts_loopback_http_redirect_uri
|
|
["http://localhost/callback", "http://127.0.0.1:3000/cb", "http://[::1]/cb"].each do |uri|
|
|
provider = Provider.new(**args_for(uri))
|
|
assert_equal(uri, provider.redirect_uri)
|
|
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,
|
|
# so the provider must refuse to construct.
|
|
assert_raises(Provider::InsecureRedirectURIError) do
|
|
Provider.new(**args_for("http://app.example.com/callback"))
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_non_http_scheme_redirect_uri
|
|
assert_raises(Provider::InsecureRedirectURIError) do
|
|
Provider.new(**args_for("ftp://app.example.com/callback"))
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_hostname_tricks_that_resemble_loopback
|
|
["http://127.attacker.com/cb", "http://127.0.0.1.evil.com/cb", "http://foo.localhost/cb"].each do |uri|
|
|
assert_raises(Provider::InsecureRedirectURIError, "should reject #{uri}") do
|
|
Provider.new(**args_for(uri))
|
|
end
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_redirect_uri_not_listed_in_client_metadata
|
|
# Per the OAuth Dynamic Client Registration response, the AS will
|
|
# bind the client_id to a specific set of redirect_uris and reject any
|
|
# authorization request that uses something else. Detect that mismatch
|
|
# at Provider construction so the failure surfaces immediately.
|
|
args = args_for("http://localhost:0/callback")
|
|
args[:redirect_uri] = "http://localhost:0/different"
|
|
|
|
assert_raises(Provider::UnregisteredRedirectURIError) do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
|
|
def test_initialize_accepts_redirect_uri_with_string_keys_in_client_metadata
|
|
# `client_metadata` is hand-built by callers, so accept either symbol
|
|
# or string keys for `redirect_uris`.
|
|
args = args_for("http://localhost:0/callback")
|
|
args[:client_metadata] = { "redirect_uris" => ["http://localhost:0/callback"] }
|
|
|
|
provider = Provider.new(**args)
|
|
assert_equal("http://localhost:0/callback", provider.redirect_uri)
|
|
end
|
|
|
|
def test_initialize_defaults_client_id_metadata_document_url_to_nil
|
|
provider = Provider.new(**args_for("http://localhost:0/callback"))
|
|
|
|
assert_nil(provider.client_id_metadata_document_url)
|
|
end
|
|
|
|
def test_initialize_accepts_https_client_id_metadata_document_url
|
|
args = args_for("http://localhost:0/callback").merge(
|
|
client_id_metadata_document_url: "https://app.example.com/client-metadata.json",
|
|
)
|
|
|
|
provider = Provider.new(**args)
|
|
assert_equal(
|
|
"https://app.example.com/client-metadata.json",
|
|
provider.client_id_metadata_document_url,
|
|
)
|
|
end
|
|
|
|
def test_initialize_rejects_http_client_id_metadata_document_url
|
|
# The loopback `http` carve-out used for discovery does not extend to the CIMD URL:
|
|
# the URL is sent to the AS as the OAuth `client_id` and travels off-loopback,
|
|
# so MCP requires `https`.
|
|
["http://localhost:8080/client-metadata.json", "http://app.example.com/cm"].each do |uri|
|
|
args = args_for("http://localhost:0/callback").merge(client_id_metadata_document_url: uri)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError, "should reject #{uri}") do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_non_https_scheme_client_id_metadata_document_url
|
|
["ftp://app.example.com/cm", "file:///cm", "foo://bar/cm"].each do |uri|
|
|
args = args_for("http://localhost:0/callback").merge(client_id_metadata_document_url: uri)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError, "should reject #{uri}") do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_client_id_metadata_document_url_without_path
|
|
# The CIMD document is a discrete resource, not the origin.
|
|
# A bare origin or root-path URL would let two different deployments at
|
|
# the same origin shadow each other under one `client_id`.
|
|
["https://app.example.com", "https://app.example.com/"].each do |uri|
|
|
args = args_for("http://localhost:0/callback").merge(client_id_metadata_document_url: uri)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError, "should reject #{uri}") do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_client_id_metadata_document_url_with_fragment
|
|
# A fragment is never sent to the server; embedding one in the `client_id` value
|
|
# would make the OAuth `client_id` and the URL the AS dereferences point at different artifacts.
|
|
args = args_for("http://localhost:0/callback").merge(
|
|
client_id_metadata_document_url: "https://app.example.com/cm#frag",
|
|
)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError) do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_client_id_metadata_document_url_with_query
|
|
args = args_for("http://localhost:0/callback").merge(
|
|
client_id_metadata_document_url: "https://app.example.com/cm?x=1",
|
|
)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError) do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_client_id_metadata_document_url_with_userinfo
|
|
# Embedding credentials in the `client_id` leaks them to every AS
|
|
# log line and to the conformance audit trail.
|
|
args = args_for("http://localhost:0/callback").merge(
|
|
client_id_metadata_document_url: "https://user:pass@app.example.com/cm",
|
|
)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError) do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
|
|
def test_initialize_rejects_client_id_metadata_document_url_with_dot_segments
|
|
# `https://x/a/./b` and `https://x/a/b` are RFC 3986 equivalent,
|
|
# so accepting both would let one client publish under two `client_id` values.
|
|
# Reject `.`/`..` (including percent-encoded `.`).
|
|
[
|
|
"https://app.example.com/./cm",
|
|
"https://app.example.com/a/../cm",
|
|
"https://app.example.com/%2E/cm",
|
|
].each do |uri|
|
|
args = args_for("http://localhost:0/callback").merge(client_id_metadata_document_url: uri)
|
|
assert_raises(Provider::InvalidClientIDMetadataDocumentURLError, "should reject #{uri}") do
|
|
Provider.new(**args)
|
|
end
|
|
end
|
|
end
|
|
|
|
def test_authorization_flow_is_authorization_code
|
|
provider = Provider.new(**args_for("https://app.example.com/callback"))
|
|
|
|
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
|
|
end
|