chore: auth conformance closeout — SEP-990 fixture, migration.md + client docs - #2359
Conversation
Sorry, something went wrong.
|
@modelcontextprotocol/client
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/client@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/codemod@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/server-legacy@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/express@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/fastify@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/hono@2359
npm i https://pkg.pr.new/modelcontextprotocol/typescript-sdk/@modelcontextprotocol/node@2359 commit: 66918a9 |
Sorry, something went wrong.
⚠️ No Changeset foundLatest commit: 66918a9 Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Sorry, something went wrong.
⚠️ No Changeset foundLatest commit: 45d5724 Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. This PR includes no changesetsWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Sorry, something went wrong.
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 OAuthClientProvider implementers" 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_type heuristic / https: token-endpoint guard), SEP-2350 (d62ce60, scope step-up), SEP-2468 (36a0046, RFC 9207 iss validation + finishAuth(URLSearchParams)), and SEP-2352 (c427613, issuer-stamped credential isolation). packages/client/src/client/auth.ts contains forceReauthorization, IssuerMismatchError, and the issuer stamp on saveTokens({ ...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 AuthOptions and 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-stamped issuer field 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 OAuthClientProvider implementation, your clientMetadata, 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 explicit HTML 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 id per-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.
Sorry, something went wrong.
…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
4839347b9
Two closeout pieces with no packages/ runtime change:
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 and migration.md carries the single consumer checklist (PRD user story 24).
How Has This Been Tested?
npm run test:conformance:client:all and :2026 — auth/* fully green on both legs (the only remaining client baseline entry is the unrelated json-schema-ref-no-deref). pnpm sync:snippets --check confirms the new guide regions are in sync.
Breaking Changes
None.
Types of changes
Checklist
Additional context
The conformance referee bumps the conformance pin to 0.2.0-alpha.6 (which includes conformance#361, the callback-iss fix) and drops the now-stale expected-failures entries — expected-failures.yaml is empty for both client and server at this commit.
migration-SKILL.md is 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.