-
Notifications
You must be signed in to change notification settings - Fork 1.9k
chore: auth conformance closeout — SEP-990 fixture, migration.md + client docs #2349
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
felixweinberger
merged 1 commit into
fweinberger/auth-1-sep2468-client
from
fweinberger/auth-6-closeout
Jun 24, 2026
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@modelcontextprotocol/client': minor | ||
| --- | ||
|
|
||
| Per-authorization-server credential isolation (SEP-2352). `auth()` now stamps an `issuer` field onto every value it passes to `saveTokens()` / `saveClientInformation()` and threads `{ issuer }` to `tokens()` / `clientInformation()`; on read, a stored credential whose stamp names a different authorization server is treated as `undefined`, so a `client_id` / `refresh_token` issued by one AS is never sent to another. Providers that round-trip stored values verbatim are protected with no code change; multi-AS providers may key storage on `ctx.issuer`. New `discardIfIssuerMismatch()` helper and `AuthorizationServerMismatchError` (callback-leg gate). `OAuthClientProvider.saveAuthorizationServerUrl()` / `authorizationServerUrl()` are deprecated (still written, never read). `ClientCredentialsProvider`, `PrivateKeyJwtProvider`, `StaticPrivateKeyJwtProvider`, and `CrossAppAccessProvider` gain `expectedIssuer` and no longer define `saveClientInformation()`. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔴 The PR is titled and described as a docs-only chore ("Two closeout pieces with no
packages/runtime change", only the 'Documentation update' box checked, "Breaking Changes: None"), but the diff as it stands ships the full SEP-2352 runtime behavior change inpackages/client(issuer stamping, the new fail-closed callback-leg gate throwingAuthorizationServerMismatchError,invalidateCredentials('all')→'client'+'tokens',expectedIssueron four bundled providers with theirsaveClientInformation()removed, two new public exports) plus this changeset declaring a minor bump for@modelcontextprotocol/client. Either the branch is carrying the SEP-2352 feature commits that were meant to land separately (the changeset-bot head flipping between78df4a3andadd874f2suggests the base/head may be off), or the title/description/'Types of changes' need rewriting to reflect a behavior-changing minor release — please reconcile so reviewers triaging by the description don't skip the security-relevant runtime changes.Extended reasoning...
What the inconsistency is. The PR description opens with "Two closeout pieces with no
packages/runtime change", the title is achore:for a fixture mapping plus docs, only the Documentation update type-of-change is checked, and Breaking Changes says None. The diff, however, includes the entire SEP-2352 per-authorization-server credential-isolation behavior change:packages/client/src/client/auth.ts(issuer stamping intosaveTokens/saveClientInformation, the new exporteddiscardIfIssuerMismatch(), the fail-closed callback-leg gate that throws the newAuthorizationServerMismatchError, andinvalidateCredentials('all')changed to'client'+'tokens'oninvalid_client/unauthorized_client),authErrors.ts(new public error class),authExtensions.ts(expectedIssueroptions onClientCredentialsProvider/PrivateKeyJwtProvider/StaticPrivateKeyJwtProvider/CrossAppAccessProviderand removal of theirsaveClientInformation()methods — an observable public-surface change),index.ts(two new public exports), the new SEP-2352 unit/e2e test suites, and.changeset/auth-sep-2352-credential-isolation.mddeclaring a minor bump for@modelcontextprotocol/client. Both statements cannot be true at once: a PR whose delta contains a minor-bump changeset and runtime/public-API changes is not a docs-only chore.\n\nConcrete walk-through of the reviewer impact. (1) A reviewer triages PR #2349 by its title/description and the checked 'Documentation update' box, and budgets a docs-level review. (2) They reviewmigration.md,docs/client.mdregions, and the conformance fixture wiring, and approve. (3) The merge then lands a security-relevant runtime change — including a fail-closed path: any existingOAuthClientProviderthat implementssaveDiscoveryState()but cannot return discovery state on the callback leg now throwsAuthorizationServerMismatchErrorwhere it previously proceeded — plus a public-API removal (saveClientInformationgone from four bundled providers) and a published minor version bump, none of which were reviewed. The repo's review conventions ('Decompose by default'; read added.changeset/*.mdtext against the implementation in the same diff) exist precisely to prevent this.\n\nWhy this isn't caught elsewhere. Nothing in CI compares the PR description or the 'Types of changes' checklist against the diff; the changeset tooling only checks that a changeset exists for changed packages, not that the PR prose acknowledges it.\n\nAddressing the stacked-PR reading. One plausible explanation is that this is the top of a stacked auth series (fweinberger/auth-6-closeout) whose parent feature commits (78df4a3 "feat(client): SEP-2352 …", 9bc98d3, 46b6b01, …) are reviewed in their own PRs, and the runtime delta only appears here because the diff is rendered againstmainrather than the integration base. The changeset-bot timeline is consistent with something being off either way: at 21:20:07 it reports the head as78df4a3with the changeset detected, and five seconds later (21:20:12) it reports the head asadd874f2with no changeset and only@modelcontextprotocol/examples+@modelcontextprotocol/test-conformanceaffected. If the intended head is the latter, the PR base/head should be made to actually reflect that (retarget the base or drop the feature commits) so the rendered diff and changed-files list stop showing thepackages/clientchanges; if the intended head is the former (the diff as currently rendered), the description, title, and 'Types of changes' are wrong and must be rewritten as a behavior-changing minor release. Either way the PR as currently presented is internally inconsistent and needs reconciling before merge — this comment is not asserting which resolution is correct, only that one of them must happen.\n\nStep-by-step proof of the contradiction. (1) Open the changed-files tab: 18 files, including fivepackages/client/src/testfiles and.changeset/auth-sep-2352-credential-isolation.md. (2) Open that changeset: it declares'@modelcontextprotocol/client': minorand describes the SEP-2352 behavior change, the deprecations, and the provider-surface changes. (3) Read the PR description: "nopackages/runtime change", 'Documentation update' only, 'Breaking Changes: None'. (4) There is no state of the world in which (1)–(2) and (3) are simultaneously accurate for the delta that will merge.\n\nHow to fix. Pick one: (a) rebase/retarget so this PR's delta really is the SEP-990 fixture + docs only (the SEP-2352 commits and this changeset land via their own reviewed PR), or (b) keep the diff and rewrite the title (feat(client): …), the summary, the 'Types of changes' (New feature, and arguably Breaking change given the providers'saveClientInformationremoval and the new fail-closed throw path), and the Breaking Changes section to describe SEP-2352 — so the runtime change gets the review it warrants.