chore: auth conformance closeout — SEP-990 fixture, migration.md + client docs#2359
chore: auth conformance closeout — SEP-990 fixture, migration.md + client docs#2359felixweinberger wants to merge 1 commit into
Conversation
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
78df4a3 to
c427613
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
47a57bd to
45d5724
Compare
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
45d5724 to
fa62df4
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
c427613 to
82f711a
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
2 times, most recently
from
99eca5e to
d66af0c
Compare
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
82f711a to
6a87d23
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
d66af0c to
89df6b0
Compare
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
docs/migration.md:1581— The intro of this same "Authorization (2026-07-28 spec)" section (line 1513, just above this hunk) still says the SDK "will implement the parts that land in SDK code ... as the SEP-2468/2352/2350/837/2207 behavior changes land", which contradicts the present-tense checklist intro added here ("The SDK enforces every 2026-07-28 authorization MUST that lands in SDK code"). Since all of those behavior changes have already landed on this branch and this PR consolidates the 2026 auth migration guidance, consider flipping that sentence to present tense in the same pass.Extended reasoning...
The issue. The new "Conformance obligations for
OAuthClientProviderimplementers" intro added by this PR (docs/migration.md:1581) states in the present tense: "The SDK enforces every 2026-07-28 authorization MUST that lands in SDK code." Two screens above, the unmodified intro of the same## Authorization (2026-07-28 spec)section (docs/migration.md:1513) still reads: "The SDK adds the public surface for these now and will implement the parts that land in SDK code (defaulting them on) as the SEP-2468/2352/2350/837/2207 behavior changes land..." — future tense, describing the enforcement as not yet present.Why the future tense is now wrong. All of the referenced behavior changes have already landed on this integration branch: SEP-837/2207 (a81ff07,
application_typeheuristic /https:token-endpoint guard), SEP-2350 (d62ce60, scope step-up), SEP-2468 (36a0046, RFC 9207issvalidation +finishAuth(URLSearchParams)), and SEP-2352 (c427613, issuer-stamped credential isolation).packages/client/src/client/auth.tscontainsforceReauthorization,IssuerMismatchError, and the issuer stamp onsaveTokens({ ...tokens, issuer }, infoCtx), so the enforcement is live, not pending.Why this PR is the right place to fix it. The contradiction is created within the section this PR rewrites: the same section now says (a) at line 1513 that the SDK will enforce these things later, and (b) at line 1581 that the SDK enforces them today, with checklist bullets that describe the enforcement as live (e.g. "the SDK discards a stored value whose stamp names a different authorization server"). Line 1513 even cross-links to the very "Conformance obligations" anchor whose content the PR replaces. The PR's stated purpose is consolidating the consumer-facing 2026 auth migration guidance, and it already updated the parallel stale wording in the
AuthOptionsand issuer-context subsections (the old "currently inert" / "does not yet pass" sentences are gone in the latest commit), so the section intro is now the lone remaining future-tense sentence.Concrete reader walkthrough. (1) A consumer lands on the Authorization (2026-07-28) section and reads at line 1513 that the SDK will only enforce RFC 9207 / issuer isolation / step-up "as the behavior changes land", concluding the defaults are not yet on. (2) They scroll to the conformance checklist at line 1581 and read that the SDK enforces every in-SDK MUST today and that their
saveTokens()must round-trip an SDK-stampedissuerfield or stored credentials get discarded. (3) The two statements cannot both be true; the reader either distrusts the checklist or assumes the issuer stamping is still inert and skips the round-trip obligation — the failure mode SEP-2352 exists to prevent.Why this is distinct from the existing comments. The earlier inline comment on this PR targeted the "does not yet pass" / "currently inert" sentences at lines 1525/1527, which the latest commit already fixed; and the other comment targets the SEP-2350 cross-reference at line ~1593. Neither touches line 1513, so fixing those would not resolve this sentence.
Fix. One-sentence wording change at line 1513, e.g.: "The SDK adds the public surface for these and implements the parts that land in SDK code (defaulting them on); the parts that live in your
OAuthClientProviderimplementation, yourclientMetadata, or your host UI are listed under [Conformance obligations...]". Prose-only with no behavioral impact, hence a nit. -
🟡
docs/migration.md:1527— The cross-reference added at the end of this paragraph links to#per-authorization-server-credential-isolation-sep-2352, but no heading in migration.md produces that anchor — the only heading that resembled it (#### SEP-2352 — per-authorization-server credential isolation) is removed by this same PR, so the link is dead. Point it at the section that actually describes the stamp:#credentials-are-bound-to-the-issuing-authorization-server-sep-2352(or rename the link text to match).Extended reasoning...
The bug. The rewritten paragraph in the
### \OAuthClientProvider` credential methods receive an `issuer` contextsection (docs/migration.md:1527) now ends with: *"See [Per-authorization-server credential isolation](#per-authorization-server-credential-isolation-sep-2352) for how the stamp is used."* That anchor does not exist anywhere inmigration.md`, so the rendered doc on GitHub gets a dead intra-document link.Why the anchor is dead. GitHub generates anchors by slugifying headings. Grepping every heading in the post-PR file, the only SEP-2352-related headings are
### \OAuthClientProvider` credential methods receive an `issuer` context(line 1523) and### Credentials are bound to the issuing authorization server (SEP-2352)(line 1567) — the latter slugifies to#credentials-are-bound-to-the-issuing-authorization-server-sep-2352, not the linked anchor. There is no explicitHTML anchor in the file either. The only heading that ever resembled the link text — the old#### SEP-2352 — per-authorization-server credential isolationh4 under the conformance-obligations section — is deleted by this very PR (replaced by the new bullet list), and even that heading would have slugified to#sep-2352--per-authorization-server-credential-isolation`, so the link never matched a real anchor.Step-by-step proof. (1) A reader on the rendered migration guide reaches the
issuer-context section and reads "See Per-authorization-server credential isolation for how the stamp is used." (2) They click the link. (3) GitHub looks for an element with idper-authorization-server-credential-isolation-sep-2352; none exists, so the page does not scroll — the click silently does nothing. (4) The reader never lands on### Credentials are bound to the issuing authorization server (SEP-2352), which is exactly the section that explains how the stamp is used and validated.Why nothing else catches it. The other anchors added in the same diff (e.g.
#scope-step-up-on-403-insufficient_scope-sep-2350,#authorization-server-mix-up-defense-rfc-9207--rfc-8414-33,#conformance-obligations-for-oauthclientprovider-implementers) all correctly match GitHub slugs of existing headings, so this one is simply a stale link target — likely written against the old h4 heading this PR removes — and there is no link checker in CI to flag it.Impact and fix. No behavioral impact, but the PR's stated purpose is consolidating accurate migration guidance, and this is a broken cross-reference inside that very guidance. One-token fix: change the link target to
#credentials-are-bound-to-the-issuing-authorization-server-sep-2352, or alternatively rename the link text to "Credentials are bound to the issuing authorization server (SEP-2352)" so the text matches the section it points at.
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
6a87d23 to
0868921
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
89df6b0 to
606e819
Compare
| ### Conformance obligations for `OAuthClientProvider` implementers | ||
|
|
||
| <!-- Filled in as the SEP-2352/2350/837/2207 behavior PRs land. --> | ||
| The SDK enforces every 2026-07-28 authorization MUST that lands in SDK code. The obligations below live in **your** `OAuthClientProvider` implementation, your `clientMetadata`, your host UI, or your resource-server configuration — the SDK structurally cannot enforce them. Each links to the example that demonstrates the conformant pattern. |
There was a problem hiding this comment.
🟡 The intro paragraph of the "## Authorization (2026-07-28 spec)" section (around line 1583) still says the SDK "will implement the parts that land in SDK code (defaulting them on) as the SEP-2468/2352/2350/837/2207 behavior changes land" — future tense that contradicts the new checklist intro added here, which says the SDK "enforces every 2026-07-28 authorization MUST that lands in SDK code." Since those behavior changes have all landed on this branch, consider updating the section intro to present tense, the same way this PR fixed the stale "currently inert" / "does not yet pass" wording lower in the section.
Extended reasoning...
The inconsistency. The new consumer-checklist intro this PR adds at docs/migration.md:1651 states: "The SDK enforces every 2026-07-28 authorization MUST that lands in SDK code." But the intro paragraph of the same `## Authorization (2026-07-28 spec)` section, one paragraph above the PR's edit range (≈line 1583), still reads: "The SDK adds the public surface for these now and will implement the parts that land in SDK code (defaulting them on) as the SEP-2468/2352/2350/837/2207 behavior changes land..." — future tense, implying the validation behavior is still pending.
Why the future tense is now stale. All of those behavior changes have landed on this integration branch — the recent commits implement SEP-837/2207 defaults and the https token-endpoint guard (`401f456`), SEP-2350 scope step-up (`1ac3c91`), SEP-2468 `iss` emission/validation (`0bae633`), and SEP-2352 issuer-stamped credential isolation (`6a87d23`). The surrounding sections of the same chapter already speak in the present tense: line ~1603 says `finishAuth()` and `auth()` now validate the `iss` parameter, and line ~1639 says `auth()` now stamps an `issuer` field. The landed-state of these changes is the entire premise of this closeout PR.
Reader walkthrough (concrete proof). (1) A consumer opens the `## Authorization (2026-07-28 spec)` section to learn what they must do for the 2026 revision. (2) The first paragraph they read (line 1583) tells them the SDK only adds the public surface now and will implement/default-on the enforcement later — so they conclude the mix-up defenses, issuer binding, and step-up behavior are not yet active. (3) A few paragraphs later, the checklist intro added by this PR (line 1651) tells them the SDK enforces every authorization MUST that lands in SDK code, and the bullets impose obligations (round-trip the issuer stamp, pass `iss` to `finishAuth`) that only make sense if enforcement is live. (4) The reader cannot tell which statement is current; the safest-looking interpretation (the section intro) is the wrong one.
Why nothing else corrects it. This is exactly the class of stale prose the PR fixes elsewhere in the same section — it removed "both currently inert — the validation behavior they feed lands in the follow-up changes tracked by SEP-2468" at ~1587 and "The SDK does not yet pass this argument" / "The field is currently inert" at ~1595/1597 (the latter two via the earlier review round). The section intro at 1583 was simply not touched, and no later note corrects it. The existing inline review comments on this PR cover the lines ~1525/1527 wording and the dead anchor — different sentences; this one has not been flagged.
Fix. A one-sentence wording change: update the intro to present tense, e.g. "The SDK adds the public surface for these and implements the parts that land in SDK code, defaulting them on (SEP-2468/2352/2350/837/2207); the obligations the SDK structurally cannot enforce are listed in the consumer checklist below." Since the substance of this PR is consolidating the 2026 authorization migration guidance, it is worth fixing here, but it is a documentation-consistency nit and not blocking.
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
0868921 to
4610b54
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
606e819 to
015154e
Compare
…ient docs Wires the existing CrossAppAccessProvider into the auth/enterprise-managed-authorization conformance fixture (8/8 checks pass, zero SDK code change). Consolidates the per-PR migration.md sections into a single 2026-authorization conformance contract aligned with the issuer-stamp model, and refreshes the clientGuide auth snippets + docs/client.md. Reconciles expected-failures baselines with the published 0.2.0-alpha.5 referee. The MyOAuthProvider guide example now implements state() and discoveryState()/ saveDiscoveryState() so the documented state-check is reachable and the callback-leg AS-binding is demonstrated; the deprecated saveAuthorizationServerUrl pair is dropped. The auth_finishAuth snippet now wires the CSRF check to provider.lastState and reconnects on a fresh StreamableHTTPClientTransport (a started transport cannot be restarted). Unused Prompt/Resource/Tool type imports dropped from the synced imports region. auth/* is fully green on both legs at the tip. Two client baseline entries remain at the alpha.5 pin: json-schema-ref-no-deref (SEP-2106 territory, unrelated) and auth/2025-03-26-oauth-metadata-backcompat (referee mock still serves a §3.3-violating issuer at this pin; tracked for a referee-side fix or removal at the next conformance pin bump). Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-5-sep2352-isolation
branch
from
4610b54 to
5fe7ec1
Compare
felixweinberger
force-pushed
the
fweinberger/auth-6-closeout
branch
from
015154e to
9019a41
Compare
1 participant
4839347b9Two closeout pieces with no
packages/runtime change:auth/enterprise-managed-authorization(SEP-990) to the existingrunCrossAppAccessCompleteFlowconformance handler — the SDK already implements SEP-990 viaCrossAppAccessProvider(Implement SEP-990 Enterprise Managed OAuth #1593); only the fixture mapping was missing.docs/migration.md(one bullet per consumer-side obligation the SDK structurally cannot enforce, each cross-referencing the example that demonstrates the conformant pattern) and adds theauth_oauthClientProvider+auth_finishAuthsnippet regions toclientGuide.examples.ts/docs/client.md. Closes Cannot find module '@modelcontextprotocol/sdk/client' or its corresponding type declarations #67.Motivation and Context
SEP-990 (issue modelcontextprotocol/modelcontextprotocol#990) is already implemented in the SDK; this only wires the conformance fixture. The doc consolidation closes out M13.1 so
auth/*conformance is green andmigration.mdcarries the single consumer checklist (PRD user story 24).How Has This Been Tested?
npm run test:conformance:client:alland:2026—auth/*fully green on both legs (the only remaining client baseline entry is the unrelatedjson-schema-ref-no-deref).pnpm sync:snippets --checkconfirms the new guide regions are in sync.Breaking Changes
None.
Types of changes
Checklist
Additional context
The conformance referee pin is unchanged in this PR — the integration branch already adopted
@modelcontextprotocol/conformance@0.2.0-alpha.5via #2335, which carries the RFC 8414 §3.3-compliant backcompat mock from conformance#359. With that referee, PR 2's issuer-echo check is default-ON andauth/2025-03-26-oauth-metadata-backcompatpasses; this PR just drops the now-stale expected-failures entries.migration-SKILL.mdis unchanged: every surface delta is additive-optional (finishAuth(code)still compiles), so there is no mechanical mapping.Re-opened from #2349, which was auto-closed by a transient stack-branch mis-push. Same branch, same content. Paul's review is on #2349.