feat(client): SEP-2350 scope step-up — union, retry cap, superset-gated refresh bypass#2346
Conversation
🦋 Changeset detectedLatest commit: 52b1cfc The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
1416e4a to
e06add7
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
99fbb49 to
5156ed7
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
e06add7 to
2bffe4c
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
5156ed7 to
409363a
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
2bffe4c to
f9285f6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
409363a to
32d3af6
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
f9285f6 to
8b2f271
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
2 times, most recently
from
b629c06 to
b445a38
Compare
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
8b2f271 to
a2c6968
Compare
…default, https token-endpoint guard Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-3-sep2350-stepup
branch
from
a2c6968 to
52b1cfc
Compare
felixweinberger
force-pushed
the
fweinberger/auth-2-sep837-2207
branch
from
b445a38 to
318fca3
Compare
felixweinberger
merged commit 52b1cfc
into
fweinberger/auth-1-sep2468-client
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| '@modelcontextprotocol/core': minor | ||
| --- | ||
|
|
||
| Dynamic Client Registration hygiene for the 2026-07-28 authorization requirements (SEP-837, SEP-2207). New `resolveClientMetadata(provider)` reads `provider.clientMetadata` and applies the spec defaults — `application_type` derived from the redirect URIs (loopback or custom scheme → `'native'`, otherwise `'web'`), `grant_types: ['authorization_code', 'refresh_token']` when omitted — and `auth()` feeds the resolved document to DCR only (scope selection still reads the raw consumer-supplied `clientMetadata` so statically-registered/CIMD clients are not pushed into `offline_access` + `prompt=consent`); consumer-set values are never overwritten. DCR rejection now throws the new `RegistrationRejectedError` carrying the HTTP status, raw body, and submitted metadata — **breaking for direct `registerClient()` callers**: rejection no longer throws `OAuthError`, so update `instanceof` checks. `OAuthClientMetadata` gains a typed `application_type?: string` field (expected `'native'` / `'web'`; tolerant on parse). `OAuthErrorCode` adds `InvalidRedirectUri`. The token-exchange, refresh, and Cross-App Access (`requestJwtAuthorizationGrant` / `exchangeJwtAuthGrant`) paths now throw the new `InsecureTokenEndpointError` for a non-`https:` token endpoint (`localhost` / `127.0.0.1` / `::1` exempt), and `auth()` surfaces it on the refresh branch instead of silently re-authorizing. |
There was a problem hiding this comment.
🔴 The PR title and description describe a SEP-2350 scope step-up feature (onInsufficientScope on the HTTP transports, exported computeScopeUnion()/InsufficientScopeError, an examples/scoped-tools story, conformance withOAuthRetry wiring, and client-auth:step-up:* e2e scenarios) that does not exist anywhere in this diff or at HEAD (52b1cfc) — the actual change is the SEP-837/SEP-2207 DCR-hygiene work described by this changeset and the HEAD commit message. Please rewrite the title/description to match the DCR-hygiene change actually contained here (or push the intended step-up commits); as-is the Motivation, How-Has-This-Been-Tested, and Breaking-Changes sections describe a different change and will mislead reviewers and the merge-commit history.
Extended reasoning...
What the mismatch is. The PR title ("feat(client): SEP-2350 scope step-up — union, retry cap, superset-gated refresh bypass") and the entire PR description claim a scope step-up feature: an onInsufficientScope option on both HTTP transports, exported computeScopeUnion() and InsufficientScopeError, an examples/scoped-tools headless story with a hardened demo AS, conformance-fixture withOAuthRetry wiring, and client-auth:step-up:* e2e scenarios. None of that content is present in this PR. The diff (and this changeset, .changeset/auth-dcr-hygiene.md) implements SEP-837/SEP-2207 Dynamic Client Registration hygiene instead: resolveClientMetadata() with application_type/grant_types defaults, RegistrationRejectedError, InsecureTokenEndpointError + assertSecureTokenEndpoint(), OAuthErrorCode.InvalidRedirectUri, and the matching e2e/conformance updates.\n\nStep-by-step verification at the PR head. (1) The changeset bot reports the PR's latest commit as 52b1cfca1d5a625d82aec89f373d0db4be4ffca3, which is exactly HEAD of this changeset, so this is not a stale-checkout artifact. (2) Grepping packages/client/src at that commit for computeScopeUnion, InsufficientScopeError (the transport-level class), onInsufficientScope, or _stepUpAuthorize returns no matches. (3) examples/scoped-tools does not exist, and the 18 changed files include no streamableHttp.ts, sse.ts, or test/conformance/src changes. (4) The new e2e scenarios are client-auth:dcr:*, client-auth:token-endpoint:https-guard, client-auth:refresh:rotation-handling, and client-auth:scope:offline-access-gate — not the client-auth:step-up:* rows the description claims under How Has This Been Tested. (5) The expected-failures edits remove SEP-837 application_type baseline entries; auth/scope-step-up remains baselined on the 2026-07-28 leg, contradicting the description's claim that it "burns" with this change. (6) The HEAD commit message itself reads "feat(client): SEP-837/2207 — application_type heuristic, grant_types default, https token-endpoint guard", matching the diff.\n\nWhy nothing else catches it. The earlier claude[bot] review comments referencing _stepUpAuthorize at an older head (8b2f271) indicate the branch previously carried the SEP-2350 work and was later force-pushed to the DCR-hygiene stack without updating the PR title/description. CI passes because the code that is present is internally consistent — there is no automated check that the description matches the diff.\n\nImpact. Reviewers approving this PR based on the description would be approving claims about transport behavior, breaking-change posture ("onInsufficientScope defaults to 'reauthorize'"), and testing evidence (step-up e2e scenarios, the conformance auth/scope-step-up burn) that the diff does not back. On squash-merge the description typically becomes the merge-commit body, permanently misdescribing what landed; future readers tracing SEP-2350 vs SEP-837/2207 history will be pointed at the wrong commit.\n\nHow to fix. Either (a) rewrite the PR title and description to describe the SEP-837/SEP-2207 DCR-hygiene change actually contained — the changeset text and the HEAD commit message are accurate starting points — including correcting the testing section to the client-auth:dcr:* / token-endpoint:https-guard scenarios and the SEP-837 expected-failures burn-down, or (b) push the intended SEP-2350 step-up commits to this branch if this PR was meant to carry them. This is an editorial/process issue rather than a code defect, but the description-vs-diff mismatch is total and should be resolved before merge.
| /** HTTP status code returned by the registration endpoint. */ | ||
| public readonly status: number; | ||
| /** Raw response body text (typically an RFC 7591 error JSON document). */ | ||
| public readonly body: string; | ||
| /** The exact client metadata that was POSTed (after SDK defaults were applied). */ | ||
| public readonly submittedMetadata: OAuthClientMetadata; | ||
|
|
||
| constructor(args: { status: number; body: string; submittedMetadata: OAuthClientMetadata }) { | ||
| super(`Dynamic Client Registration rejected (HTTP ${args.status}): ${args.body}`); | ||
| this.status = args.status; | ||
| this.body = args.body; | ||
| this.submittedMetadata = args.submittedMetadata; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🟡 The base-class OAuthClientFlowError JSDoc a few lines above still says "In the release that introduces it no subclass exists yet — the guard … will not match anything until the first behavior change ships", but this PR adds two concrete subclasses (RegistrationRejectedError, InsecureTokenEndpointError) in this same file and migration-SKILL.md now documents three subclasses with instanceof guidance. Doc-only nit: drop or rewrite that second @remarks sentence.
Extended reasoning...
What is stale. The @remarks on the OAuthClientFlowError base class near the top of packages/client/src/client/authErrors.ts reads: "Nothing in the SDK throws this base class directly. In the release that introduces it no subclass exists yet — the guard is a forward-compat hook and will not match anything until the first behavior change ships." The first sentence is still true; the second is now plainly false.
Why this PR makes it false. This PR adds two concrete subclasses to the same module, a few lines below that remark: RegistrationRejectedError (thrown by registerClient() when the AS rejects DCR) and InsecureTokenEndpointError (thrown by assertSecureTokenEndpoint() on the exchange/refresh/cross-app paths). Together with the pre-existing IssuerMismatchError (added in 318fca3), the family now has three shipped, actively-thrown members — so the instanceof OAuthClientFlowError guard the remark says "will not match anything" matches real errors the SDK throws on common paths.
Why it contradicts the PR's own docs. The migration-SKILL.md hunk added in this same PR publishes a three-row table of OAuthClientFlowError subclasses and their throw sites and explicitly recommends that consumers dispatch on them ("all extend OAuthClientFlowError, not OAuthError"). A reader who hovers the base class in their IDE sees JSDoc claiming the family is empty, while the migration guide shipped by the same PR tells them to catch its members.
Step-by-step. (1) Commit 874ca25 introduced OAuthClientFlowError with this remark, which was accurate at that moment (no subclasses). (2) Commit 318fca3 added IssuerMismatchError, making the remark stale. (3) This PR adds RegistrationRejectedError and InsecureTokenEndpointError directly below the remark and adds the consumer-facing subclass table — extending exactly the pattern the remark denies, in the same file, without updating it.
Impact. Documentation-only; no behavioral effect. The cost is reader confusion: the JSDoc actively tells consumers the catch-all guard is inert, which could discourage them from using the very instanceof dispatch the migration guide recommends.
How to fix. One-sentence JSDoc edit: delete the second @remarks sentence, or rewrite it to reflect reality, e.g. "Concrete subclasses currently include IssuerMismatchError, RegistrationRejectedError, and InsecureTokenEndpointError; catch the base class to handle the whole family." Since this PR touches the same file and adds the subclasses, this is the natural diff in which to make the change (the staleness technically began one commit earlier, but the PR directly compounds it).
2 participants
a0266c630(squash of07fdc0d2d+ae5453f21+9be8a7e97)Adds
onInsufficientScopehandling on both HTTP transports: on403 insufficient_scopethe transport computespreviously-granted ∪ challengedvia the new exportedcomputeScopeUnion(), applies a bounded per-transport retry cap, and — when the union is a strict superset of the held token's scope — bypasses the refresh branch and forces a freshstartAuthorization(RFC 6749 §6: refresh cannot widen scope). New exportedInsufficientScopeError. Adds theexamples/scoped-toolsheadless story (server + demo AS + client) with the demo AS hardened to loopback-onlyredirect_urisat DCR. Wires the conformance fixture'swithOAuthRetryto usecomputeScopeUnionsoauth/scope-step-upburns on the 2026 leg.Motivation and Context
SEP-2350 (modelcontextprotocol/modelcontextprotocol#2350).
Normative sentences implemented:
The superset-gated refresh bypass is the security-review-mandated guard from the PRD: a refresh grant cannot widen scope per RFC 6749 §6, so step-up to a strict superset must go through
startAuthorization.How Has This Been Tested?
computeScopeUnionset semantics; retry-cap counter.client-auth:step-up:{union, retry-cap, superset-bypass-refresh, throw-mode, get-stream-403},hosting-entry-authstep-up rows.examples/scoped-toolsis a self-verifying headless OAuth leg inpnpm run:examples.auth/scope-step-upburns on the 2026-07-28 leg (the fixture's fetch-levelwithOAuthRetrynow calls the exportedcomputeScopeUnionon 403, exercising the same helper a real consumer would).Breaking Changes
None.
onInsufficientScopedefaults to'reauthorize', which is the existing behavior at the plan pin (PRD user stories 30–31: the named consumer'swrapFetchWithStepUpDetectionkeeps working without an opt-in).'throw'is the new opt-in.Types of changes
Checklist
Additional context
Demo AS hardening (folded in from
ae5453f21). Theexamples/scoped-toolsdemo authorization server now: rejects any DCRredirect_urithat is not a loopback host (127.0.0.1,localhost,[::1]) with http/https scheme; validates/authorize'sredirect_uriexactly against the per-client registered set; binds both the demo AS and the MCP resource server to127.0.0.1only. This prevents an unauthenticated DCR from registering an external redirect target and exfiltrating authorization codes from the demo.Fixture wiring (folded in from
9be8a7e97). The conformance fixture's fetch-levelwithOAuthRetryintercepts 403 before the transport's_stepUpAuthorizesees it, so itshandle401now calls the exportedcomputeScopeUniondirectly.