SEP-2207: Refresh token guidance#1523
SEP-2207: Refresh token guidance#1523wdawson wants to merge 5 commits intomodelcontextprotocol:mainfrom
Conversation
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
This is ready for review now that the SEP is accepted. Edit: the conformance test has been merged |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Scope selection used consistently for DCR and the authorization request. | ||
| const resolvedScope = determineScope({ | ||
| requestedScope: scope, | ||
| resourceMetadata, | ||
| authServerMetadata: metadata, | ||
| clientMetadata: provider.clientMetadata | ||
| }); | ||
|
|
||
| // Handle client registration if needed | ||
| let clientInformation = await Promise.resolve(provider.clientInformation()); |
There was a problem hiding this comment.
🟡 Nit: fetchToken() (line 650) receives the raw scope parameter instead of resolvedScope, so non-interactive flows using prepareTokenRequest() miss the PRM scopes_supported fallback and the offline_access augmentation from determineScope(). In practice the impact is minimal — authorization_code flows don't send scope in the token request at all, and client_credentials + refresh tokens is not a valid combination per RFC 6749 §4.4.3 — but passing resolvedScope here would be more consistent with the stated goal of centralizing scope selection.
Extended reasoning...
What the bug is
In authInternal(), determineScope() computes resolvedScope (line 590-596) with a three-level fallback (WWW-Authenticate scope → PRM scopes_supported → clientMetadata.scope) plus SEP-2207 offline_access augmentation. This resolvedScope is correctly passed to registerClient() (line 631) and startAuthorization() (line 688). However, at line 650, fetchToken() receives the raw scope parameter — the original value from auth() options — not resolvedScope.
How fetchToken handles scope differently
Inside fetchToken() (line 1503), scope resolution follows a completely different chain: const effectiveScope = scope ?? provider.clientMetadata.scope. This bypasses both the PRM scopes_supported fallback (priority 2 in the MCP spec) and the offline_access augmentation added by this PR. The effectiveScope is then passed to provider.prepareTokenRequest(effectiveScope) for non-interactive flows.
Step-by-step proof for non-interactive flow
Consider a client_credentials provider where: (1) no explicit scope is passed to auth(), (2) PRM metadata has scopes_supported: ["mcp:read", "mcp:write"], (3) AS metadata has scopes_supported including offline_access, and (4) clientMetadata.grant_types includes refresh_token but clientMetadata.scope is undefined.
determineScope()returns"mcp:read mcp:write offline_access"(PRM fallback + augmentation)- DCR registration uses this resolved scope ✓
fetchToken()receivesscope = undefined, computeseffectiveScope = undefined ?? undefined = undefinedprepareTokenRequest(undefined)gets no scope at all, vs the intended"mcp:read mcp:write offline_access"
Why the practical impact is minimal
For authorization_code flows (the primary target of this PR), prepareAuthorizationCodeRequest() only sends grant_type, code, code_verifier, and redirect_uri — scope is not included in the token exchange request because it was already baked into the authorization URL via startAuthorization(resolvedScope). So this bug has zero impact on auth code flows.
For client_credentials flows, RFC 6749 §4.4.3 states the AS "MUST NOT issue a refresh token," making offline_access semantically meaningless. The PRM fallback gap is more relevant here, but providers implementing prepareTokenRequest() have full control over what scope they send.
Suggested fix
Pass resolvedScope instead of scope at line 650 for consistency with the PR's goal of centralizing scope selection via determineScope(). This is a one-line change: scope → resolvedScope in the fetchToken() call.
There was a problem hiding this comment.
@felixweinberger happy to update this if you want. Was trying to be surgical, but the review is probably accurate. lmk
| if ( | ||
| effectiveScope && | ||
| authServerMetadata?.scopes_supported?.includes('offline_access') && | ||
| !effectiveScope.split(' ').includes('offline_access') && |
There was a problem hiding this comment.
🟣 Nit (pre-existing): startAuthorization() at line 1259 uses scope?.includes('offline_access') (substring match on the string), while the new determineScope() correctly uses .split(' ').includes('offline_access') (exact token match). A hypothetical scope like "no_offline_access" would incorrectly trigger prompt=consent. Consider updating line 1259 to scope?.split(' ').includes('offline_access') for consistency.
Extended reasoning...
Bug Analysis
The new determineScope() function (line 493) correctly checks whether offline_access is present in a space-delimited scope string using exact token matching:
\!effectiveScope.split(' ').includes('offline_access')However, the consumer of this scope — startAuthorization() at line 1259 — checks for the same scope token using String.prototype.includes(), which is a substring match:
if (scope?.includes('offline_access')) {
authorizationUrl.searchParams.append('prompt', 'consent');
}How it manifests
Consider a scope string like "mcp:read no_offline_access". The determineScope() function would correctly determine that offline_access is not present as a discrete scope token. But when this scope string reaches startAuthorization(), the substring check "mcp:read no_offline_access".includes("offline_access") returns true, causing prompt=consent to be incorrectly appended to the authorization URL.
Step-by-step proof
determineScope()returns"mcp:read no_offline_access"(no augmentation, sinceoffline_accessis not a discrete token).authInternal()passes this asscope: resolvedScopetostartAuthorization().- Inside
startAuthorization(), line 1259 evaluates:"mcp:read no_offline_access"?.includes("offline_access")→true. prompt=consentis appended to the authorization URL — incorrectly, sinceoffline_accesswas never actually requested.
Why existing code does not prevent it
The determineScope() function uses the correct exact-token approach, but it cannot control how downstream consumers check the scope string. The startAuthorization() function predates this PR and was not updated.
Impact
This is a pre-existing issue — the substring check in startAuthorization() was there before this PR. However, this PR makes offline_access appear in scope strings more frequently via the SEP-2207 augmentation logic, which means the code path at line 1259 gets exercised more often. In practice, a scope token containing "offline_access" as a substring is extremely unlikely in real OAuth deployments, so the practical impact is negligible.
Fix
Replace the substring check on line 1259 with an exact token match:
if (scope?.split(' ').includes('offline_access')) {This is a one-line change that aligns startAuthorization() with the correct pattern already used in determineScope().
There was a problem hiding this comment.
@felixweinberger I'm happy to update this as well if you like, but we don't need to increase the scope here unnecessarily.
2 participants
Implement OIDC-flavored refresh token guidance (modelcontextprotocol/modelcontextprotocol#2207) in the Python SDK client.
Motivation and Context
MCP clients interacting with OIDC-flavored Authorization Servers often don't receive refresh tokens because they aren't requesting
offline_access. This leads to poor UX (frequent re-authentication). SEP-2207 provides guidance for how MCP clients should handle this.The SDK currently expects the caller to supply the supported
grant_typesfor the client. The example already shows providingrefresh_token, and callers are encouraged to do so as well.This PR:
determineScope()helper that follows the MCP Scope Selection Strategyoffline_accessto the authorization request scope when the AS advertises it inscopes_supportedand the client'sgrant_typesincludesrefresh_tokenNo changes are needed on the server side — the SDK's example AS already includes
offline_accessin itsscopes_supported, and the example middleware already omits it fromWWW-Authenticate(both compliant with the SEP).How Has This Been Tested?
determineScope()covering:scopes_supported,clientMetadata.scopefallback, omit)offline_accesswhen conditions met)clientMetadata.scopeor non-compliant PRM, whengrant_typesomitsrefresh_token, whengrant_typesis undefined, when no scopes are present)Breaking Changes
None. The
offline_accessaugmentation only activates when:offline_accessinscopes_supportedgrant_typesincludesrefresh_tokenClients that don't set
grant_typesor don't includerefresh_tokenare unaffected. Authorization Servers that don't recognizeoffline_accesswill simply ignore it per OAuth 2.1. The newly exporteddetermineScope()function is additive.Types of changes
Checklist
Additional context
determineScope()now encodes the MCP Scope Selection Strategy as a testable, exported function. It acceptsrequestedScope,resourceMetadata,authServerMetadata, andclientMetadata, and returns the effective scope string (orundefinedto omit the parameter).startAuthorization()already handles addingprompt=consentwhenoffline_accessis in scope, per the OIDC Core spec. No changes were needed there.grant_typesdefaulting was intentionally not added at the SDK level. TheOAuthClientMetadatatype is shared between client creation (DCR) and server-side parsing (CIMD), so schema-level defaults would violate OAuth spec defaults for CIMD. The example clients already includerefresh_tokeningrant_types.insufficient_scopehandler instreamableHttp.tsalready passes scope through toauth(), which now delegates todetermineScope()— no additional changes needed for step-up authorization flows.