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)`,