From 095bd819910e492df8ec9688e21642f6aceb4171 Mon Sep 17 00:00:00 2001 From: Koichi ITO Date: Thu, 1 Oct 2026 00:20:33 +0900 Subject: [PATCH] [Doc] Describe the pending authorization checks as the code establishes them ## Motivation and Context The documentation of `Flow#finish!` and the comments beside it claimed more than the checks establish. They said that validating the RFC 9207 `iss` before consuming the pending authorization means a forged callback carrying a valid `state` cannot discard the verifier the legitimate callback needs. The check compares a present `iss` with the recorded issuer, a public value, and passes a callback without one unless the authorization server advertises `authorization_response_iss_parameter_supported`, so what it refuses is a callback from another authorization server, as in a mix-up attack; whoever holds the `state` and passes that check consumes the entry, with an `error` or an unusable code as well as with the code itself, and the legitimate callback then finds nothing. The comment on the error response likewise called the values the authorization server's own, which equality with a public issuer string does not establish. Three more statements were imprecise: the age of a pending authorization is counted from the moment `run!` saves it, not from the redirect; a registration is refused when its `client_id` or issuer changed, not on any change; and nothing said that `finish!` needs the redirect's query passed whole, since an `iss` the caller drops reads as absent. The documentation and the comments now say what each check establishes and what remains with the sender, the storage, and the caller. No behavior changes. ## How Has This Been Tested? Documentation and comments only; the tests are unchanged. ## Breaking Changes None. --- docs/_client/authorization.md | 20 ++++++++++++-------- lib/mcp/client/oauth/flow.rb | 18 ++++++++++++------ lib/mcp/client/oauth/provider.rb | 4 ++-- 3 files changed, 26 insertions(+), 16 deletions(-) diff --git a/docs/_client/authorization.md b/docs/_client/authorization.md index 13d28192..436332e4 100644 --- a/docs/_client/authorization.md +++ b/docs/_client/authorization.md @@ -97,7 +97,8 @@ Optional keyword arguments: (with `iss` set to the RFC 9207 `iss` parameter from the redirect, or `nil` when absent) opts into SEP-2468 issuer validation: a present `iss` must match the authorization server's issuer, and a missing one is rejected when the server advertises `authorization_response_iss_parameter_supported`. Omit it when the redirect arrives in a later request, as it does in a web application; see [Authorization in Web Applications](#authorization-in-web-applications). -- `pending_authorization_max_age`: Integer seconds a pending authorization stays redeemable after the redirect when `callback_handler` is omitted. Defaults to 600. +- `pending_authorization_max_age`: Integer seconds a pending authorization stays redeemable, counted from the moment `run!` saves it, when `callback_handler` + is omitted. Defaults to 600. - `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). @@ -185,7 +186,8 @@ by a different process, and relaying the code to a request held open for that lo 1. When the transport meets a `401`, or a `403` step-up challenge, the flow runs discovery and registration as usual, saves a pending authorization in `storage` keyed by the `state` it generated, hands the authorization URL to `redirect_handler`, and raises `MCP::Client::OAuth::Flow::AuthorizationPendingError` instead of retrying. The error's `authorization_url` reader returns the same URL, so the application can send the user there from wherever is convenient. -2. The request that receives the redirect calls `MCP::Client::OAuth::Flow#finish!` with the redirect's whole query. Requests made afterwards use the stored tokens. +2. The request that receives the redirect calls `MCP::Client::OAuth::Flow#finish!` with the redirect's whole query. Pass the query as it arrived: an `iss` + the caller drops reads as absent, and the flow cannot tell the difference. Requests made afterwards use the stored tokens. ```ruby def mcp_oauth_provider(user) @@ -232,16 +234,18 @@ so the storage should expire entries older than `pending_authorization_max_age`, `finish!` redeems the code the way the authorization began, and refuses anything else with `Flow::AuthorizationError`: - The pending authorization is looked up by `state` before any request is made. An unknown, already used, or malformed one is refused, - and one older than `pending_authorization_max_age` is discarded and refused. + and one older than `pending_authorization_max_age`, counted from when `run!` saved it, is discarded and refused. - `server_url` must name the MCP server the authorization began with. -- The RFC 9207 `iss` parameter is validated against the recorded issuer before the pending authorization is consumed, so a forged callback carrying a valid `state` - cannot discard the verifier the legitimate callback needs. Because `finish!` sees the whole query, a missing `iss` is refused whenever the authorization server - advertises `authorization_response_iss_parameter_supported`. +- The RFC 9207 `iss` parameter is validated against the recorded issuer before the pending authorization is consumed, so a callback from another authorization + server, as in a mix-up attack, is refused without consuming the entry the legitimate callback needs. The check establishes only that a present `iss` + matches the recorded issuer, not who sent the callback; a callback without `iss` passes it unless the authorization server advertises + `authorization_response_iss_parameter_supported`, in which case a missing `iss` is refused. Whoever holds the `state`, passes that check, and reaches + the consume first takes the entry, with an `error` or an unusable code as well as with the code itself, and the legitimate callback then finds nothing. - The pending authorization is then consumed through `delete_pending_authorization`, and only a callback that gets the entry back proceeds, so of two callbacks racing on the same `state`, such as a retried redirect, at most one redeems the code. An `error` response is raised with its `error` and `error_description`, - bounded as [token endpoint errors](#token-endpoint-errors) are; it is read only after the `iss` check, since in a mix-up those parameters are the attacker's. + bounded as [token endpoint errors](#token-endpoint-errors) are; it is read only after the `iss` check, since in a mix-up those parameters are another server's. - The code is redeemed at the recorded token endpoint, with the client registration, `resource`, and `redirect_uri` used when the authorization began, - without running discovery again (SEP-2352). A registration replaced in the meantime is refused. + without running discovery again (SEP-2352). A registration whose `client_id` or issuer changed in the meantime is refused; its other members may change. {: .important } > `state` proves that this SDK started the authorization, not which user did. Binding the callback to the user who started it is the application's responsibility: diff --git a/lib/mcp/client/oauth/flow.rb b/lib/mcp/client/oauth/flow.rb index 13298e5d..c79a271e 100644 --- a/lib/mcp/client/oauth/flow.rb +++ b/lib/mcp/client/oauth/flow.rb @@ -493,15 +493,20 @@ def refresh!(server_url:, resource_metadata_url: nil) # Finishes an authorization that `run!` left pending, in the request that receives the redirect to `redirect_uri`, # which may run in another process. `callback_params` is that redirect's whole query as a Hash (`code`, `state`, and, - # when present, `iss`, `error`, and `error_description`); passing all of it, rather than picking values out, is what - # lets the flow tell an absent `iss` from one the caller did not look for. + # when present, `iss`, `error`, and `error_description`); passing all of it, rather than picking values out, is + # the caller's part of the `iss` check: an `iss` the caller drops reads as absent, and the flow cannot tell the difference. # # The pending authorization is looked up by `state` before any request is made, and it binds the rest of the exchange: # the code is redeemed at the token endpoint recorded when the authorization began, with the client registration, # `resource`, and `redirect_uri` used then, and without discovery running again, so the code reaches the authorization # server the user was sent to (SEP-2352). The RFC 9207 `iss` is validated against the recorded issuer before the pending - # authorization is consumed, so a forged callback carrying a valid `state` cannot discard the verifier the legitimate - # callback needs, and before the callback's `error` is read, since in a mix-up those parameters are the attacker's. + # authorization is consumed, so a callback from another authorization server, as in a mix-up, is refused without + # consuming the entry the legitimate callback needs, and before the callback's `error` is read, since in a mix-up + # those parameters are the other server's. The check establishes only that a present `iss` matches the recorded issuer, + # not who sent the callback, and a callback without `iss` passes it unless the metadata advertises + # `authorization_response_iss_parameter_supported`: whoever holds the `state`, passes that check, and reaches + # the consume first takes the entry, and the legitimate callback then finds nothing. + # # Past that check the pending authorization is consumed with `delete_pending_authorization`, which returns the entry # it removed; only a callback that gets the entry back redeems the code, so of callbacks racing on the same `state` # (a retried redirect, say), at most one does. @@ -1285,8 +1290,9 @@ def non_empty_string?(value) end # An RFC 6749 Section 4.1.2.1 error response, reported with the bounds a token endpoint error gets. - # Reached only after the `iss` check, so the values are the authorization server's own; they are still text it chose, - # so they are cut to a bounded length and confined to the printable ASCII the RFC permits. + # Reached only after the `iss` check, so any `iss` beside the values matched the recorded issuer; that does not + # prove who sent them, and they are text the sender chose, so they are cut to a bounded length and confined to + # the printable ASCII the RFC permits. def authorization_response_error(error, description) error = bounded_diagnostic(error, limit: TOKEN_ENDPOINT_ERROR_MAX_LENGTH) description = bounded_diagnostic(description, limit: TOKEN_ENDPOINT_ERROR_DESCRIPTION_MAX_LENGTH) diff --git a/lib/mcp/client/oauth/provider.rb b/lib/mcp/client/oauth/provider.rb index aab14ced..ddeff730 100644 --- a/lib/mcp/client/oauth/provider.rb +++ b/lib/mcp/client/oauth/provider.rb @@ -33,8 +33,8 @@ module OAuth # Omit it when the redirect arrives in a later request, as it does in a web application: # the flow then stops after `redirect_handler` with a pending authorization saved in `storage`, # and the request that receives the redirect finishes it with `Flow#finish!`. - # - `pending_authorization_max_age` - Seconds a pending authorization stays redeemable after the redirect, - # when `callback_handler` is omitted. Defaults to `DEFAULT_PENDING_AUTHORIZATION_MAX_AGE`. + # - `pending_authorization_max_age` - Seconds a pending authorization stays redeemable, counted from the moment + # `run!` saves it, when `callback_handler` is omitted. Defaults to `DEFAULT_PENDING_AUTHORIZATION_MAX_AGE`. # - `scope` - String of space-separated scopes to request when the server's # `WWW-Authenticate` does not specify one. # - `storage` - Object responding to `tokens`, `save_tokens(tokens)`,