feat(client): SEP-2468 RFC 9207 iss + RFC 8414 §3.3 issuer-echo validation#2344
feat(client): SEP-2468 RFC 9207 iss + RFC 8414 §3.3 issuer-echo validation#2344felixweinberger wants to merge 1 commit into
Conversation
🦋 Changeset detectedLatest commit: 7c38bad 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: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| */ | ||
| /** | ||
| * Reads RFC 9207's `authorization_response_iss_parameter_supported` from | ||
| * authorization-server metadata. The metadata schemas are `looseObject`, so the | ||
| * field passes through untyped; only a literal `true` counts as advertised — | ||
| * absent, `false`, or any non-boolean value means not advertised. | ||
| */ | ||
| function isIssParameterSupported(metadata: AuthorizationServerMetadata | undefined): boolean { | ||
| return ( | ||
| (metadata as { authorization_response_iss_parameter_supported?: unknown } | undefined) | ||
| ?.authorization_response_iss_parameter_supported === true | ||
| ); | ||
| } | ||
|
|
||
| export function validateAuthorizationResponseIssuer({ |
There was a problem hiding this comment.
🟡 The JSDoc block carrying the four-row decision table, the no-normalization rule, and the @throws tag (lines ~443-463) is separated from validateAuthorizationResponseIssuer by a second doc block and the private helper isIssParameterSupported(), so the new public export ends up with no JSDoc in editor hovers and generated API docs. Move the helper above the doc block (or below the exported function) so the doc directly precedes export function validateAuthorizationResponseIssuer.
Extended reasoning...
What the bug is. In packages/client/src/client/auth.ts, the long JSDoc comment that documents validateAuthorizationResponseIssuer — the RFC 9207 §2.4 four-row decision table, the "simple string equality, no normalization" rule, and the @throws {IssuerMismatchError} tag — is immediately followed by a second JSDoc block and the private helper function isIssParameterSupported(...). JSDoc/TSDoc tooling (typedoc, tsserver editor hovers, IDE quick-info) attaches a doc comment to the next following declaration, and only the closest preceding block counts. The result: isIssParameterSupported keeps its own short doc, the decision-table doc is orphaned (attached to nothing), and the actually exported validateAuthorizationResponseIssuer (declared right after the helper, around line 477) has no documentation at all.
Code path / how it manifests. Hover over validateAuthorizationResponseIssuer in an editor, or generate API docs for @modelcontextprotocol/client — the function shows up undocumented. This matters more than a typical doc nit because the function is a brand-new public export (re-exported from packages/client/src/index.ts), and the orphaned doc block is exactly the security-relevant contract the PR description highlights: which of the four issParameterSupported × iss combinations reject, and that no URL normalization is applied before comparison. Host authors deciding how to wire finishAuth(code, iss) are the audience for that table, and they won't see it.
Why nothing else prevents it. Nothing in the build or lint pipeline checks doc-comment attachment; TypeScript compiles identically either way, and the unit tests only exercise runtime behavior. The misplacement looks like an editing slip — the helper was inserted between the doc comment and its intended target during development.
Step-by-step proof.
- Look at the current ordering in
auth.ts: (a) JSDoc with the decision table +@throws, (b) JSDoc forisIssParameterSupported, (c)function isIssParameterSupported(...), (d)export function validateAuthorizationResponseIssuer(...). - TSDoc attachment rule: a declaration's doc is the comment block immediately preceding it. So (c) gets (b); (d) gets nothing, because (c)'s body sits between (a)/(b) and (d).
- Block (a) precedes block (b), not a declaration, so it is dropped by typedoc and ignored by editor hovers.
- Therefore hovering
validateAuthorizationResponseIssuer(or its re-export from@modelcontextprotocol/client) shows only the inline property comments on the options object — not the decision table, not the no-normalization rule, not the@throwstag.
Impact. Documentation-only; no runtime effect. But it strands the contract documentation for a new, security-relevant public API.
Fix. Move isIssParameterSupported (with its own doc block) above the decision-table JSDoc, or below validateAuthorizationResponseIssuer, so the decision-table doc directly precedes export function validateAuthorizationResponseIssuer. A one-block move, no behavioral change.
| if (!skipIssuerValidation) { | ||
| // RFC 8414 §3.3 / OIDC Discovery §4.3: the `issuer` value in the document MUST be | ||
| // identical to the issuer identifier used to construct the well-known URL. Compare | ||
| // against the raw input string — callers pass the exact issuer string the AS published. | ||
| const expectedIssuer = typeof authorizationServerUrl === 'string' ? authorizationServerUrl : authorizationServerUrl.href; | ||
| // One narrow tolerance: the SDK's own legacy-fallback path synthesizes the AS URL via | ||
| // `String(new URL('/', serverUrl))`, which always carries a trailing `/`. That value is | ||
| // SDK-generated (not attacker-controlled), so accept the slash-only difference here. | ||
| // The tolerance is one-directional and end-anchored — a different host or path is still | ||
| // a mismatch. | ||
| const matches = | ||
| parsed.issuer === expectedIssuer || (expectedIssuer.endsWith('/') && parsed.issuer === expectedIssuer.slice(0, -1)); | ||
| if (!matches) { | ||
| throw new IssuerMismatchError('metadata', expectedIssuer, parsed.issuer); |
There was a problem hiding this comment.
🟡 The RFC 8414 §3.3 trailing-slash tolerance fires for any expectedIssuer ending in / — including PRM-advertised authorization_servers[] entries like https://as.example.com/tenant/ and caller-supplied URLs — not only the SDK-synthesized legacy-fallback URL, contradicting the inline comment, the PR description, and the unit-test name. Either gate the tolerance to the discoverOAuthServerInfo fallback call site (e.g. an internal flag, or strip the synthesized slash before calling discovery) or correct the prose to match the actual behavior.
Extended reasoning...
What the code does vs. what the prose claims. The issuer-echo check in discoverAuthorizationServerMetadata() is:
const matches =
parsed.issuer === expectedIssuer || (expectedIssuer.endsWith('/') && parsed.issuer === expectedIssuer.slice(0, -1));The relaxation is keyed purely on the string shape of the input — expectedIssuer.endsWith('/') — with no provenance signal. The inline comment says the tolerance exists because "the SDK's own legacy-fallback path synthesizes the AS URL via String(new URL('/', serverUrl))... That value is SDK-generated (not attacker-controlled)", and the PR description states the tolerance is "applied only to the SDK-synthesized legacy-fallback URL". The code implements no such restriction.
Code paths that get the relaxation despite not being the legacy fallback. discoverAuthorizationServerMetadata() is a public export and is also invoked from authInternal's cached-state path and from discoverOAuthServerInfo with whatever the PRM advertised. Any of those inputs ending in / gets the slash-only tolerance: (1) a server-controlled authorization_servers[] entry like https://as.example.com/tenant/, (2) a URL passed directly by a caller, (3) a cached discoveryState.authorizationServerUrl string, and (4) any URL object whose pathname is / — .href always carries the trailing slash, so new URL('https://as.example.com') becomes https://as.example.com/ and triggers the branch.
Step-by-step example. The PRM at the MCP server advertises authorization_servers: ['https://as.example.com/tenant/']. discoverOAuthServerInfo passes that string to discoverAuthorizationServerMetadata. The metadata document fetched from /.well-known/oauth-authorization-server/tenant contains issuer: 'https://as.example.com/tenant' (no slash). expectedIssuer.endsWith('/') is true and parsed.issuer === expectedIssuer.slice(0, -1), so matches is true and the metadata is accepted — even though RFC 8414 §3.3 says the values "MUST be identical" and the spec text quoted in this PR explicitly forbids trailing-slash normalization before comparison. Note also that the new unit test labelled "tolerates a trailing slash on the SDK-synthesized discovery input only" actually exercises a generic caller-supplied string (discoverAuthorizationServerMetadata('https://auth.example.com/')), not the legacy-fallback path — the test demonstrates the tolerance is generic.
Why nothing else prevents it. There is no flag, parameter, or call-site distinction that tells discoverAuthorizationServerMetadata where the URL came from, so it cannot scope the tolerance to SDK-generated inputs. buildDiscoveryUrls strips the trailing slash before constructing the well-known URL, so both forms fetch the same document — which is exactly why the relaxation silently applies to non-fallback inputs too.
Impact. Practical security impact is small: the tolerated difference is a single trailing slash on the same scheme/host/path, and a mix-up attacker cannot exploit a slash-only delta to point at a different issuer. The real issue is that the prose (inline comment, PR description, test name) promises a narrower, security-relevant restriction than the code ships, and the implementation is a broader-than-claimed relaxation of a spec MUST for inputs that are server-controlled. All four verifiers confirmed the factual claim; none refuted it.
How to fix. Either (a) restrict the tolerance to the legacy-fallback call site — e.g. add an internal option set only by discoverOAuthServerInfo when it synthesizes String(new URL('/', serverUrl)), or simpler, strip the trailing slash from the synthesized URL before calling discovery and drop the tolerance branch entirely — or (b) keep the behavior and correct the inline comment, PR description, and test name to say the tolerance applies to any discovery input ending in /.
pcarleton
left a comment
pcarleton
left a comment
There was a problem hiding this comment.
overall looks good, small tweak to text on when iss fails
| const code = parsedUrl.searchParams.get('code'); | ||
| // RFC 9207: extract `iss` alongside `code` so the SDK can validate it | ||
| // against the recorded issuer before redeeming the code. | ||
| const iss = parsedUrl.searchParams.get('iss') ?? undefined; |
There was a problem hiding this comment.
low priority / non blocking: wonder if a helper for "handling the callback URL" would be helpful
|
|
||
| **Action required for hosts handling OAuth callbacks.** | ||
|
|
||
| `transport.finishAuth()` and `auth()` now validate the `iss` parameter from the authorization callback against the issuer recorded from the authorization server's validated metadata (RFC 9207). When the AS advertises `authorization_response_iss_parameter_supported: true`, a missing or mismatched `iss` is **rejected** with `IssuerMismatchError` before the authorization code is exchanged. |
There was a problem hiding this comment.
slight tweak: our profile of mixup defense rejects on a mismatched iss even if authorization_response_iss_parameter_supported is not present. it only rejects on missing if authorization_response_iss_parameter_supported is present.
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
0fdbe45 to
f6fb29c
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
afb76ea to
c843d7e
Compare
| import { IssuerMismatchError } from './authErrors.js'; | ||
|
|
||
| // Re-exported for back-compat — the canonical home is ./authErrors.js. | ||
| export { IssuerMismatchError } from './authErrors.js'; |
There was a problem hiding this comment.
🟡 Two doc-prose nits in the new auth.ts additions: (1) the comment "Re-exported for back-compat" on the IssuerMismatchError re-export is inaccurate — the class is brand-new in this PR, so there is no prior consumer to stay compatible with; either drop the redundant re-export (the package barrel already exports it from ./client/authErrors.js) or reword the comment to state the real reason (convenience). (2) The new RFC 8414 §3.3 issuer-validation paragraph in the discoverAuthorizationServerMetadata JSDoc (~lines 1359-1363) was inserted between the @param authorizationServerUrl description and the @param options tag, so JSDoc/typedoc render it as part of the parameter description instead of the function summary — move it above the @param list.
Extended reasoning...
Issue 1 — false "back-compat" rationale on the IssuerMismatchError re-export (lines 28-30).
The new lines in packages/client/src/client/auth.ts read:
// Re-exported for back-compat — the canonical home is ./authErrors.js.
export { IssuerMismatchError } from './authErrors.js';IssuerMismatchError is introduced for the first time in this PR — it does not exist anywhere at the parent commit (the predecessor commit that created authErrors.ts only contains OAuthClientFlowError), and it has never been published from auth.js or any other module. There is therefore no prior consumer for the re-export to be "back-compat" with, so the comment's stated justification is factually unsupported. The package barrel (packages/client/src/index.ts) already exports IssuerMismatchError directly from ./client/authErrors.js, and the only in-repo consumers of the auth.ts re-export are the new test files added in this same PR — so the re-export is at most a convenience for internal/test imports.
Step-by-step: (1) git grep IssuerMismatchError at the parent commit returns nothing — the class is new. (2) Therefore no published version ever exported it from auth.js. (3) Therefore "back-compat" cannot be the reason the re-export exists. (4) A reader (or a future refactor) trusting the comment would conclude the re-export must be preserved for external consumers, when in fact it could be dropped or relocated freely. Fix: either delete the re-export (the barrel export covers the public API) or reword the comment to state the real reason, e.g. "Convenience re-export so auth.ts consumers and {@linkcode} references can use it directly."
Issue 2 — RFC 8414 §3.3 prose attached to the wrong @param in the discoverAuthorizationServerMetadata JSDoc (lines ~1359-1363).
The new paragraph
The returned metadata's
issueris validated againstauthorizationServerUrlper RFC 8414 §3.3 … rejected with {@linkcode IssuerMismatchError} … SetskipIssuerValidation: trueto suppress this check…
was inserted after the tail of the @param authorizationServerUrl description and before the @param options tag. JSDoc/TSDoc parsers attribute all text following a block tag, up to the next tag, to that tag — so this function-level, security-relevant prose renders as part of the authorizationServerUrl parameter description in typedoc output and editor quick-info, and the function summary stays silent on the new default-on rejection behavior.
Step-by-step: (1) Hover discoverAuthorizationServerMetadata in an editor or generate typedoc. (2) The summary shows only the pre-existing description (fallback strategy, etc.). (3) Expand the authorizationServerUrl parameter and the issuer-validation paragraph appears there, describing behavior of the whole function rather than the parameter. Mitigating factors: the @throws {IssuerMismatchError} tag and the @param options.skipIssuerValidation line in the same block are correctly placed, so the information is misattributed rather than lost — which is why this stays a nit. Fix: move the paragraph above the @param list so it joins the main description.
This is distinct from the already-posted review comment about the orphaned JSDoc on validateAuthorizationResponseIssuer (~line 440) — that is a different doc block at a different site. Both items here are documentation-only with no runtime effect; combined they are a small, easy cleanup before merge.
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
f6fb29c to
d1ffd2f
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
c843d7e to
5dfc2be
Compare
| /** | ||
| * Thrown when an authorization-server issuer identifier fails validation. | ||
| * | ||
| * Two checks raise this error, distinguished by {@linkcode IssuerMismatchError.kind | kind}: | ||
| * - `'metadata'` — the `issuer` in fetched authorization-server metadata does | ||
| * not match the issuer identifier the well-known URL was constructed from | ||
| * (RFC 8414 §3.3 / OpenID Connect Discovery §4.3). | ||
| * - `'authorization_response'` — the `iss` parameter on the authorization | ||
| * callback failed RFC 9207 §2.4 validation against the recorded issuer. | ||
| * | ||
| * Intentionally does **not** extend `OAuthError`: the `auth()` | ||
| * orchestrator's `OAuthError` retry block must not swallow this — a mix-up | ||
| * indication is fatal for the flow, not a retryable credential problem. | ||
| * | ||
| * On the `'authorization_response'` path the {@linkcode IssuerMismatchError.received | received} | ||
| * value is attacker-controllable in a mix-up attack; callers **MUST NOT** display | ||
| * it (or any `error`/`error_description`/`error_uri` from the same callback) to | ||
| * end users. The values are JSON-encoded in the message to neutralize log-injection. | ||
| */ | ||
| export class IssuerMismatchError extends OAuthClientFlowError { |
There was a problem hiding this comment.
🟡 The @remarks on the OAuthClientFlowError base class (authErrors.ts lines ~15-17) still says "no subclass exists yet" and that an instanceof OAuthClientFlowError guard "will not match anything until the first behavior change ships" — but this same PR adds IssuerMismatchError extends OAuthClientFlowError in this file and throws it from default-on rejection paths. Please update or drop that sentence (e.g. note that IssuerMismatchError / SEP-2468 is the first concrete subclass), the same way the analogous "currently inert" AuthOptions prose was refreshed in this PR.
Extended reasoning...
What's stale. The class-level JSDoc on OAuthClientFlowError (lines ~15-17 of packages/client/src/client/authErrors.ts, just above this PR's additions) reads:
@remarks 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.
This PR adds IssuerMismatchError extends OAuthClientFlowError twenty lines below in the same file, and the SDK now throws it from several default-on paths: discoverAuthorizationServerMetadata() (RFC 8414 §3.3 issuer-echo check), authInternal(), exchangeAuthorization(), and fetchToken() (RFC 9207 iss validation). The second and third sentences of the @remarks are therefore false the moment this PR merges: a subclass does exist, and an instanceof OAuthClientFlowError guard will match a real, breaking, security-relevant error. Since both the base-class changeset and this PR's changeset will publish in the same client minor, even the "release that introduces it" framing no longer holds.
Why it matters. OAuthClientFlowError is publicly exported from packages/client/src/index.ts, so this @remarks text shows up in editor hovers and generated API docs for exactly the audience deciding how to catch the new error family. Telling a host author that catching OAuthClientFlowError "will not match anything" steers them away from a guard that, after this PR, catches the new mix-up-defense rejections (IssuerMismatchError with kind: 'metadata' | 'authorization_response').
Why nothing else fixes it. Nothing in the build or lint pipeline checks doc prose against behavior, and the diff never touches these lines. Notably, the analogous stale prose was refreshed elsewhere in this same PR — the AuthOptions.iss / skipIssuerMetadataValidation "currently inert" sentences in auth.ts were rewritten to describe the now-live validation — so leaving this @remarks unchanged looks like an oversight rather than intent.
Step-by-step proof.
- Read
authErrors.tsafter this PR: lines ~15-17 say "no subclass exists yet … will not match anything"; lines ~45-61 declareexport class IssuerMismatchError extends OAuthClientFlowError. discoverAuthorizationServerMetadata()throwsnew IssuerMismatchError('metadata', …)whenever the metadata'sissuerdoesn't echo the discovery URL (default-on; opt-out isskipIssuerValidation).authInternal()/exchangeAuthorization()/fetchToken()callvalidateAuthorizationResponseIssuer(), which throwsnew IssuerMismatchError('authorization_response', …)on a mismatched or advertised-but-missingiss(default-on, no opt-out).- Therefore
try { await transport.finishAuth(params) } catch (e) { if (e instanceof OAuthClientFlowError) … }matches a real error path in the released package — directly contradicting the hover text.
Impact and fix. Doc-only, no runtime effect — hence nit severity. Fix is a one-sentence edit: drop or replace the second sentence of the @remarks, e.g. "The first concrete subclass is {@linkcode IssuerMismatchError} (SEP-2468); catching OAuthClientFlowError matches all current and future flow-fatal rejections in this family." The first sentence ("Nothing in the SDK throws this base class directly") remains accurate and can stay.
| if (!skipIssuerValidation) { | ||
| // RFC 8414 §3.3 / OIDC Discovery §4.3: the `issuer` value in the document MUST be | ||
| // identical to the issuer identifier used to construct the well-known URL. Compare | ||
| // against the raw input string — callers pass the exact issuer string the AS published. | ||
| const expectedIssuer = typeof authorizationServerUrl === 'string' ? authorizationServerUrl : authorizationServerUrl.href; | ||
| // One narrow tolerance: the SDK's own legacy-fallback path synthesizes the AS URL via | ||
| // `String(new URL('/', serverUrl))`, which always carries a trailing `/`. That value is | ||
| // SDK-generated (not attacker-controlled), so accept the slash-only difference here. | ||
| // The tolerance is one-directional and end-anchored — a different host or path is still | ||
| // a mismatch. | ||
| const matches = | ||
| parsed.issuer === expectedIssuer || (expectedIssuer.endsWith('/') && parsed.issuer === expectedIssuer.slice(0, -1)); | ||
| if (!matches) { | ||
| throw new IssuerMismatchError('metadata', expectedIssuer, parsed.issuer); | ||
| } |
There was a problem hiding this comment.
🟡 The new default-on RFC 8414 §3.3 issuer-echo rejection is inherited inconsistently by other discovery callers: discoverAndRequestJwtAuthGrant() (crossAppAccess.ts:207, SEP-990) now throws IssuerMismatchError{kind:'metadata'} for an IdP whose published issuer doesn't echo the supplied idpUrl, but DiscoverAndRequestJwtAuthGrantOptions exposes no opt-out passthrough and the changeset/migration docs never mention this caller; meanwhile the deprecated-but-still-exported discoverOAuthMetadata() performs no issuer-echo check at all. Consider forwarding an opt-out (or adding a migration-doc sentence) for the cross-app-access path, and noting in discoverOAuthMetadata's @deprecated JSDoc that it does no issuer validation.
Extended reasoning...
Gap 1 — discoverAndRequestJwtAuthGrant() silently inherits the new rejection with no opt-out and no migration note. packages/client/src/client/crossAppAccess.ts:207 calls discoverAuthorizationServerMetadata(String(idpUrl), { fetchFn }) to find the IdP token endpoint for the SEP-990 enterprise-managed-authorization flow. With this PR's new validation block (auth.ts ~1417-1431), that call now throws IssuerMismatchError{kind:'metadata'} whenever the IdP's metadata issuer does not exactly echo the caller-supplied idpUrl (modulo the one-directional trailing-slash tolerance). DiscoverAndRequestJwtAuthGrantOptions (crossAppAccess.ts:71-77) only exposes idpUrl plus the RequestJwtAuthGrantOptions fields — no skipIssuerValidation/skipIssuerMetadataValidation passthrough — so callers of this public API hitting a known-misconfigured IdP have no per-call escape hatch, unlike auth(), which got AuthOptions.skipIssuerMetadataValidation in the same PR.
Step-by-step example. (1) A host calls discoverAndRequestJwtAuthGrant({ idpUrl: 'https://idp.example.com/tenant', ... }). (2) The IdP serves /.well-known/openid-configuration/tenant with issuer: 'https://idp.example.com' (a path-elided, §3.3-violating but previously tolerated configuration). (3) discoverAuthorizationServerMetadata parses the document, compares parsed.issuer to the raw idpUrl string, finds no match, and throws IssuerMismatchError('metadata', ...). (4) Before this PR the same call returned the metadata and the flow proceeded; now it fails with no flag the caller can set and no mention of this path in .changeset/auth-iss-validation.md, docs/migration.md, or docs/migration-SKILL.md (all three describe only discoverAuthorizationServerMetadata/auth/exchangeAuthorization/fetchToken/finishAuth).
Mitigating factors (why this is a nit, not a blocker): the rejection only fires for IdPs that actually violate RFC 8414 §3.3 — the option's JSDoc documents idpUrl as the IdP's issuer URL, so exact echo is the spec-correct expectation, and the validation is arguably desirable on this path too. Affected callers also have a workaround: call requestJwtAuthorizationGrant() directly with an explicit tokenEndpoint, bypassing discovery. And the changeset's statement that discoverAuthorizationServerMetadata() now rejects mismatched metadata transitively covers this caller. Still, an existing public API gaining a new default-on throw with neither opt-out parity nor a doc mention is the kind of completeness gap worth closing — either forward an opt-out on DiscoverAndRequestJwtAuthGrantOptions for parity with AuthOptions, or add one sentence about the cross-app-access path to the migration notes.
Gap 2 — the deprecated discoverOAuthMetadata() performs no issuer-echo check. The legacy helper (auth.ts ~1242, still re-exported from packages/client/src/index.ts) parses the fetched document with OAuthMetadataSchema and returns it without ever comparing parsed.issuer to the URL it was fetched for, and offers no skip flag either. Consumers still calling the legacy export therefore keep accepting §3.3-violating / spoofed-issuer metadata even though the changeset frames the metadata-echo defense as default-on.
Why this half is doc-level only (addressing the refutation that this is intentional / not actionable): a grep confirms discoverOAuthMetadata() has zero runtime callers inside the SDK — auth()/authInternal/discoverOAuthServerInfo/cross-app access all route through discoverAuthorizationServerMetadata(), so every SDK-driven flow gets the new defense and the PR's own claims are not undermined. Retrofitting a new default-on throw onto a deprecated legacy API would itself be an additional breaking change and is reasonably out of scope. The remaining ask is small: a sentence in the @deprecated JSDoc (and/or the migration notes) stating that the legacy helper performs no RFC 8414 §3.3 issuer validation and that callers must migrate to discoverAuthorizationServerMetadata() to get it — so external consumers don't assume the changeset's 'default-on' wording covers their direct calls to the old helper.
Suggested fix. (a) Add a skipIssuerValidation-style passthrough to DiscoverAndRequestJwtAuthGrantOptions (forwarded into the discovery call), or at minimum mention the cross-app-access path in docs/migration.md / docs/migration-SKILL.md alongside the other behavior changes. (b) Add one sentence to discoverOAuthMetadata's deprecation JSDoc noting it performs no issuer validation. Both are small, doc-or-options-level changes; no behavior change is required for compliant IdPs.
| * would, and the demo AS's auto-sign-in + `autoConsent` collapse every | ||
| * interactive step into a 302. | ||
| */ | ||
| async function followAuthorizationRedirects(authorizationUrl: URL): Promise<string> { | ||
| async function followAuthorizationRedirects(authorizationUrl: URL): Promise<URLSearchParams> { | ||
| let next = authorizationUrl.href; | ||
| // Crude cookie jar — enough for a single-origin demo AS. | ||
| const jar = new Map<string, string>(); |
There was a problem hiding this comment.
🟡 Two doc comments in this file were left on the old code-only flow after the body was migrated: the file-header flow description (steps 2–3) still says "the code is read straight off the Location header" and "transport.finishAuth(code) → SDK exchanges the code", and the JSDoc above followAuthorizationRedirects still says it returns "the code" — but the function now returns URLSearchParams and the call site passes the whole callback query to finishAuth(callbackParams). Update both blocks to describe returning/passing the callback query params so the SDK reads code + iss (RFC 9207).
Extended reasoning...
What's stale. This PR migrates the body of examples/oauth/client.ts to the iss-aware flow: followAuthorizationRedirects() now has the signature Promise<URLSearchParams> and returns resolved.searchParams, and step 3 of the run calls firstTransport.finishAuth(callbackParams) with the whole callback query. Two prose blocks in the same file were not updated to match:
- File-header flow description (steps 2–3, ~lines 22–27). It still reads "No callback server is bound — the code is read straight off the
Locationheader" and "3.transport.finishAuth(code)→ SDK exchanges the code (+ PKCE verifier)". After this PR the example never extracts a barecodeto hand tofinishAuth— it hands over the fullURLSearchParamsso the SDK can readcode+iss. - JSDoc above
followAuthorizationRedirects(~lines 44–48). It still says the function follows the redirect chain "and return thecode", while the signature changed in this very diff hunk toPromise<URLSearchParams>and the body returnsresolved.searchParams.
Why it matters. This file is the CI-runnable headless OAuth example and is explicitly positioned as the twin of simpleOAuthClient.ts; the README points readers here. The PR's own docs/migration.md marks the callback-handling change "Action required", and the README, docs/client.md, and the inline comments at the finishAuth call site (lines ~124–133) were all refreshed in this same PR — these two blocks are the only remaining prose in the example that still teaches the code-only pattern the migration guide tells hosts to move away from. A reader skimming the header to understand the flow gets a description that contradicts the code 20 lines below it.
Step-by-step proof.
- Read the post-PR file: line ~51 is
async function followAuthorizationRedirects(authorizationUrl: URL): Promise<URLSearchParams>and line ~79 isreturn resolved.searchParams;. - The JSDoc directly above (~lines 44–48) still says the function will "return the
code" — contradiction Better validation of data coming over the wire #1. - Line ~134 calls
await firstTransport.finishAuth(callbackParams);with theURLSearchParamsreturned in step 1. - The file header (~lines 22–27) still says "the code is read straight off the
Locationheader" and "transport.finishAuth(code)→ SDK exchanges the code" — contradiction Race condition between transport start and server connection #2.
Why nothing else catches it. Comments aren't checked by the compiler or lint, and the example still runs (the demo AS doesn't advertise authorization_response_iss_parameter_supported), so CI is green either way. This is also distinct from the earlier (now-resolved) review comment, which asked for the finishAuth(code) call itself and the README/docs prose to be migrated — that was done; these two comment blocks are the residual.
How to fix. Doc-only edit: in the header, change step 2 to say the callback query is read off the final Location header and step 3 to say transport.finishAuth(callbackParams) → the SDK reads code + iss (RFC 9207), validates iss against the recorded issuer, then exchanges the code; in the followAuthorizationRedirects JSDoc, say it returns the callback URLSearchParams (containing code, and iss when sent) instead of "the code". No runtime change.
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
d1ffd2f to
ec501fa
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
5dfc2be to
e2cecde
Compare
…ation
Implements the OAuth mix-up defenses, default-ON.
- IssuerMismatchError {kind:'metadata'|'authorization_response'} in
authErrors.ts (extends OAuthClientFlowError, not OAuthError, so the
auth() retry block does not swallow it). Re-exported from auth.ts for
back-compat.
- New validateAuthorizationResponseIssuer() pure helper implementing the
spec's 4-row decision table with exact === comparison (no normalization).
- discoverAuthorizationServerMetadata() validates issuer against the raw
discovery input per RFC 8414 §3.3; throws IssuerMismatchError{kind:metadata}
on mismatch unless skipIssuerValidation. One-directional trailing-slash
tolerance for the SDK-synthesized legacy-fallback URL only.
- authInternal/exchangeAuthorization/fetchToken validate iss before token
exchange (gated on authorizationCode \!== undefined).
- finishAuth(URLSearchParams) overload on StreamableHTTPClientTransport and
SSEClientTransport (reads code + iss); finishAuth(code, iss?) kept for
back-compat.
- AuthorizationServerMetadata.authorization_response_iss_parameter_supported
schema field (z.boolean().optional().catch(undefined) — tolerant of
non-boolean wire values).
- skipIssuerMetadataValidation suppresses AU-02 (metadata echo) only, not the
AU-01 runtime iss check.
- IssuerMismatchError.message JSON-encodes received/expected (log-injection
guard for attacker-controllable iss).
Conformance: auth/metadata-issuer-mismatch burns. The 6 auth/iss-* cells'
SEP-2468 check passes — they stay listed because the referee bundles a
SEP-837 application_type DCR check (burns with PR-B).
auth/2025-03-26-oauth-metadata-backcompat is added to the baseline: the §3.3
issuer-echo check correctly rejects the 0.2.0-alpha.5 referee's backcompat
mock (issuer carries a path component); tracked for a referee-side fix at the
next pin bump.
E2E: client-auth:iss:{match,mismatch-reject,supported-missing-reject,
unadvertised-proceed,no-normalize,opt-out}; as-metadata-discovery:issuer-
validation flips from knownFailure to pass.
Existing-test fixtures aligned to RFC 8414 §3.3 (issuer values only; the tests
assert other behavior).
Claude-Session: https://claude.ai/code/session_01XBib5gRe8AMPPJhySCz3EJ
felixweinberger
force-pushed
the
fweinberger/auth-0-surface
branch
from
ec501fa to
874ca25
Compare
felixweinberger
force-pushed
the
fweinberger/auth-1-sep2468-client
branch
from
e2cecde to
7c38bad
Compare
| /** | ||
| * Opt-out for the RFC 8414 §3.3 issuer-echo check during authorization | ||
| * server discovery. Disabling it is **security-weakening** and intended only | ||
| * for authorization servers known to publish a mismatched `issuer`. The | ||
| * check itself is wired up in a follow-up change; this flag is currently | ||
| * inert. | ||
| * for authorization servers known to publish a mismatched `issuer`. | ||
| * | ||
| * @default false | ||
| */ |
There was a problem hiding this comment.
🟡 The publicly exported withOAuth() middleware (packages/client/src/client/middleware.ts:38-97) is a third auth() entry point that inherits this PR's new default-on RFC 8414 §3.3 metadata-echo rejection but got no parity treatment: it takes no options bag, so the skipIssuerMetadataValidation escape hatch added to both transports and AuthOptions cannot reach it, and its catch block (middleware.ts:81-86) rewraps every non-UnauthorizedError as a generic UnauthorizedError('Failed to re-authenticate: ...'), masking the typed IssuerMismatchError mix-up signal. Consider accepting an options bag forwarded into auth() and rethrowing OAuthClientFlowError/IssuerMismatchError untouched, or at minimum documenting that the escape hatch and typed error do not apply through the middleware path.
Extended reasoning...
What the gap is. withOAuth(provider, baseUrl?) (packages/client/src/client/middleware.ts:38-97) is a publicly exported fetch middleware (re-exported from the package barrel) that handles 401s by calling auth(provider, { serverUrl, resourceMetadataUrl, scope, fetchFn: next }) at lines 64-69. This PR makes that auth() call newly throw IssuerMismatchError{kind:'metadata'} whenever the discovered AS metadata's issuer does not echo the discovery URL (RFC 8414 §3.3, default-on). The PR gave both transports parity treatment for this new rejection — skipIssuerMetadataValidation was added to StreamableHTTPClientTransportOptions / SSEClientTransportOptions and threaded through adaptOAuthProvider/handleOAuthUnauthorized, finishAuth(), and the 403 upscoping path, and docs/migration.md presents that flag as the escape hatch for a known-misconfigured AS — but withOAuth was not touched: it takes no options bag, so there is no way to forward the flag into its auth() call.\n\nThe mix-up signal is also masked. The catch block at middleware.ts:81-86 rethrows only UnauthorizedError instances and rewraps everything else as new UnauthorizedError('Failed to re-authenticate: ...'). IssuerMismatchError extends OAuthClientFlowError extends Error — deliberately not OAuthError (per the authErrors.ts JSDoc and the PR description) precisely so the auth() retry block does not swallow it and callers can instanceof-dispatch on the fatal mix-up indication. Through the middleware path, that design intent is defeated: the typed error (with its kind/expected/received fields) never reaches the caller; only the message text survives inside a generic credential-failure wrapper, indistinguishable from an ordinary 401.\n\nConcrete walk-through. (1) A host wires OAuth via withOAuth(provider) on a plain fetch pipeline (the documented use case for non-MCP requests). (2) Their AS publishes a §3.3-violating issuer (e.g. PRM advertises https://login.example.com/tenant while metadata says issuer: https://login.example.com) — a configuration that worked before this PR. (3) A request 401s, the middleware calls auth(), discovery throws IssuerMismatchError('metadata', ...). (4) The catch at line 81 sees a non-UnauthorizedError and throws UnauthorizedError('Failed to re-authenticate: Issuer mismatch in authorization server metadata...'). (5) The host's instanceof IssuerMismatchError / instanceof OAuthClientFlowError handling never fires, and the migration guide's prescribed fix (skipIssuerMetadataValidation: true) has nowhere to go — withOAuth exposes no option for it, so the only workaround is abandoning the middleware and re-implementing the flow around bare auth().\n\nWhy nothing else covers it. The existing review comments cover the transport-options gap (now addressed in this revision) and the crossAppAccess.discoverAndRequestJwtAuthGrant / discoverOAuthMetadata callers — neither mentions withOAuth, which is a third, distinct auth() entry point. There is no test exercising the new rejection through the middleware, and nothing at compile time flags the missing passthrough.\n\nMitigations (why this is a nit, not a blocker). The middleware's own JSDoc says MCP transports have built-in OAuth handling and "should not need this wrapper", so the affected audience is hosts using it for general-purpose fetch. The rewrap-everything catch behavior pre-dates this PR (only the new throw flowing through it is new), the issuer-mismatch message text is preserved inside the wrapped error, and the rejection only fires against ASes that actually violate RFC 8414 §3.3.\n\nHow to fix. Either (a) accept an options object on withOAuth — e.g. withOAuth(provider, { baseUrl, skipIssuerMetadataValidation }) — forwarded into the auth() call, and rethrow OAuthClientFlowError (and thus IssuerMismatchError) untouched in the catch block so the typed mix-up signal survives; or (b) at minimum add a sentence to docs/migration.md noting that the skipIssuerMetadataValidation escape hatch does not apply to withOAuth middleware users and that IssuerMismatchError surfaces there as a generic UnauthorizedError.
| async finishAuth(codeOrParams: string | URLSearchParams, iss?: string): Promise<void> { | ||
| if (!this._oauthProvider) { | ||
| throw new UnauthorizedError('finishAuth requires an OAuthClientProvider'); | ||
| } | ||
|
|
||
| let authorizationCode: string; | ||
| if (codeOrParams instanceof URLSearchParams) { | ||
| const code = codeOrParams.get('code'); | ||
| if (!code) { | ||
| throw new UnauthorizedError('Authorization callback is missing the "code" parameter'); | ||
| } | ||
| authorizationCode = code; | ||
| iss = codeOrParams.get('iss') ?? undefined; |
There was a problem hiding this comment.
🟡 When the authorization server returns an RFC 6749 §4.1.2.1 error callback (e.g. ?error=access_denied&error_description=..., no code), the new finishAuth(URLSearchParams) overload throws UnauthorizedError('Authorization callback is missing the "code" parameter') and silently discards the OAuth error code — the same applies to the identical overload in SSEClientTransport.finishAuth (sse.ts). Since docs/client.md and docs/migration.md now tell hosts to pass the whole callback query to finishAuth(url.searchParams) without checking error first, consider including the fixed-enum error code in the thrown message (not error_description, per the mix-up guidance), or documenting that hosts should check error before calling finishAuth.
Extended reasoning...
What the bug is. The new URLSearchParams overload of finishAuth() (StreamableHTTPClientTransport.finishAuth, packages/client/src/client/streamableHttp.ts ~721-733, and the byte-identical block in SSEClientTransport.finishAuth, sse.ts ~259-271) reads only code and iss from the callback params. When the params carry an RFC 6749 §4.1.2.1 error response — ?error=access_denied&error_description=... with no code — execution falls into the !code branch and throws UnauthorizedError('Authorization callback is missing the "code" parameter'). The actual OAuth error code that explains what happened (user denied, invalid_scope, server_error, ...) is never surfaced.\n\nThe code path that triggers it. A host follows the pattern this PR documents: docs/client.md (~196) says "pass the redirect URL's query to transport.finishAuth(url.searchParams)", and docs/migration.md says "You must pass the callback URL's query parameters to the SDK". Neither tells the host to check error first. The user clicks "Deny" on the consent screen → the AS redirects to the callback with ?error=access_denied and no code → the host hands url.searchParams to finishAuth() → the SDK reports "missing code parameter", a misleading diagnosis for an ordinary, expected outcome of the flow.\n\nStep-by-step proof. (1) AS callback: https://app.example/callback?error=access_denied&error_description=User+denied+the+request&state=xyz. (2) Host does await transport.finishAuth(new URL(callbackUrl).searchParams) exactly as docs/client.md instructs. (3) In finishAuth, codeOrParams instanceof URLSearchParams is true; codeOrParams.get('code') returns null; the !code branch throws UnauthorizedError('Authorization callback is missing the "code" parameter'). (4) The host's error handling/logging shows a "missing code" failure; access_denied never appears anywhere unless the host independently re-parses the params it already handed to the SDK.\n\nWhy nothing else prevents it. The in-repo examples avoid the trap only because they happen to branch on error themselves before calling finishAuth (examples/oauth/simpleOAuthClient.ts and examples/oauth/client.ts both do), but that pre-check is not part of the documented contract — docs/client.md, docs/migration.md, and the README all describe handing the whole query to the SDK with no mention of error. Since this PR makes the SDK the owner of callback-param parsing, the SDK is now the natural place to distinguish "callback carried an OAuth error response" from "callback genuinely missing code".\n\nOn the refutation (that this is intentional userland behavior and that surfacing the error would conflict with the SEP-2468 mix-up guidance): the migration guide's "do not display error/error_description/error_uri" caveat is scoped to callbacks rejected by the new IssuerMismatchError validation, not to ordinary AS error responses, and the suggestion here is limited to the fixed-enum error code (e.g. access_denied), not the free-text error_description/error_uri — so a more informative message does not run against that guidance. The "missing code" message is technically accurate and the host still holds the URLSearchParams, which is why this is a non-blocking DX nit rather than a correctness or security defect; but it is a gap introduced by this PR's new overload + documented pattern, and either a one-line message improvement or a one-sentence doc note ("check error before calling finishAuth") closes it.\n\nHow to fix. In both finishAuth overload bodies, before the !code throw, read codeOrParams.get('error') and, if present, throw an UnauthorizedError whose message includes the error code (e.g. "Authorization callback returned an OAuth error: access_denied") — still withholding error_description/error_uri. Alternatively (or additionally), add a sentence to docs/client.md / docs/migration.md telling hosts to check error on the callback before calling finishAuth.
| `transport.finishAuth()` and `auth()` now validate the `iss` parameter from the authorization callback against the issuer recorded from the authorization server's validated metadata (RFC 9207). A **mismatched** `iss` is rejected with `IssuerMismatchError` before the code is exchanged regardless of what the AS advertised; a **missing** `iss` is rejected only when the AS advertised `authorization_response_iss_parameter_supported: true`. | ||
|
|
||
| **You must** pass the callback URL's query parameters to the SDK so it can read `iss` alongside `code`: | ||
|
|
||
| ```typescript | ||
| const url = new URL(callbackUrl); | ||
| await transport.finishAuth(url.searchParams); // SDK reads `code` + `iss` | ||
| ``` | ||
|
|
||
| `transport.finishAuth(code, iss)` remains supported for back-compat. If you bypass `auth()` and call `exchangeAuthorization()` / `fetchToken()` directly, pass `iss` in the options bag — the same validation runs there. | ||
|
|
||
| **You must not** display or act on `error`, `error_description`, or `error_uri` from the callback URL when `IssuerMismatchError` is thrown — those values are attacker-controlled in a mix-up attack. | ||
|
|
||
| `discoverAuthorizationServerMetadata()` now rejects metadata whose `issuer` does not exactly match the URL it was fetched for (RFC 8414 §3.3). If you connect to a known-misconfigured AS, set `skipIssuerMetadataValidation: true` on `StreamableHTTPClientTransportOptions` / `SSEClientTransportOptions` (or on `AuthOptions` if you call `auth()` directly, or `skipIssuerValidation: true` on the low-level helper) — **this weakens the mix-up defense and should be treated as a temporary workaround.** It suppresses only the metadata-echo check; the callback-`iss` validation always runs (and degrades to a no-op only when `iss` is absent and the AS does not advertise support). |
There was a problem hiding this comment.
🟡 The migration prose overstates the RFC 9207 defense for the metadata-less legacy path: validateAuthorizationResponseIssuer() returns immediately when expectedIssuer (metadata?.issuer) is undefined, and discoverAuthorizationServerMetadata() returns undefined on the supported no-metadata fallback — so on that path a present, mismatched iss is silently accepted and the code is POSTed to the synthesized /token endpoint, contradicting "rejected … regardless of what the AS advertised" and "degrades to a no-op only when iss is absent and the AS does not advertise support" (and the equivalent sentence in docs/migration-SKILL.md). Either qualify both sentences ("provided AS metadata was discovered; with no metadata there is no recorded issuer and the comparison is skipped") or fall back to comparing iss against the authorization-server URL used for the flow when no metadata issuer exists.
Extended reasoning...
What the docs claim vs. what the code does. The new migration section says a mismatched iss is rejected with IssuerMismatchError before the code is exchanged "regardless of what the AS advertised" (docs/migration.md:1498) and that the callback-iss validation "degrades to a no-op only when iss is absent and the AS does not advertise support" (docs/migration.md:1511); docs/migration-SKILL.md (~line 579) repeats "a mismatched iss throws IssuerMismatchError regardless of advertised support". But validateAuthorizationResponseIssuer() (packages/client/src/client/auth.ts) begins with if (expectedIssuer === undefined) { return; }, and expectedIssuer is metadata?.issuer at every call site (authInternal, exchangeAuthorization, fetchToken). So there is a third no-op condition the "only" wording excludes: no metadata at all, with iss present and mismatched.
The code path that reaches it. discoverAuthorizationServerMetadata() returns undefined (it does not throw) when every well-known URL responds 4xx/502 — the supported legacy fallback for an AS that publishes no RFC 8414/OIDC metadata. authInternal then proceeds with metadata === undefined: startAuthorization() falls back to new URL('/authorize', authorizationServerUrl), the user is redirected, and on the callback leg finishAuth(url.searchParams) / auth({authorizationCode, iss}) calls validateAuthorizationResponseIssuer({ iss, expectedIssuer: undefined, … }), which returns immediately. fetchToken then POSTs the code to new URL('/token', authorizationServerUrl).
Step-by-step example. (1) The MCP server's AS at https://legacy-as.example.com serves no metadata documents — every well-known probe 404s, so discovery returns undefined. (2) The host redirects the user to the synthesized /authorize URL. (3) The callback arrives with ?code=abc&iss=https://attacker.example.com (e.g. injected in a mix-up scenario, or simply forwarded from a different issuer). (4) The host follows the migration guide and calls transport.finishAuth(url.searchParams). (5) validateAuthorizationResponseIssuer({ iss: 'https://attacker.example.com', expectedIssuer: undefined, issParameterSupported: false }) hits the expectedIssuer === undefined early return — no rejection. (6) The authorization code is transmitted to https://legacy-as.example.com/token. The migration prose says step (5) cannot end this way "regardless of what the AS advertised".
Why nothing else prevents it. The PR's own unit test ('no-ops when there is no recorded issuer (no validated metadata)') asserts exactly this behavior, and the function's JSDoc documents the no-baseline no-op as intentional — so the implementation choice appears deliberate and defensible (RFC 9207 frames the comparison against a validated metadata issuer, and comparing against the raw AS URL could produce false rejections since issuer identifiers need not equal the discovery URL). The problem is purely that the host-facing migration prose and the SKILL entry promise the opposite, with no caveat.
Impact. Hosts reading the migration guide may conclude the mix-up defense covers the metadata-less legacy flow and skip their own checks; in practice that path has neither the RFC 8414 echo nor the RFC 9207 comparison. The trigger is narrow (an AS with no published metadata that nonetheless sees an iss appended to the callback), which is why this is a nit rather than a blocker — but the prose on a security-relevant guarantee should match the shipped behavior.
How to fix. Either (a) doc-only: qualify the two sentences in docs/migration.md and the matching sentence in docs/migration-SKILL.md, e.g. "…provided authorization-server metadata was discovered; when no metadata is available the SDK has no recorded issuer and skips the comparison"; or (b) strengthen the code to fall back to comparing iss against the authorization-server URL used for the flow when no metadata issuer exists, which would make the current prose true.
| /** | ||
| * Opt-out for the RFC 8414 §3.3 issuer-echo check during authorization-server | ||
| * metadata discovery. **Security-weakening** — see | ||
| * {@linkcode index.AuthOptions.skipIssuerMetadataValidation | AuthOptions.skipIssuerMetadataValidation}. | ||
| * Only honoured when {@linkcode StreamableHTTPClientTransportOptions.authProvider | authProvider} | ||
| * is an `OAuthClientProvider`. | ||
| */ | ||
| skipIssuerMetadataValidation?: boolean; | ||
|
|
There was a problem hiding this comment.
🟡 The new transport-level skipIssuerMetadataValidation option (on StreamableHTTPClientTransportOptions / SSEClientTransportOptions) and its forwarding chain through adaptOAuthProvider()/handleOAuthUnauthorized(), finishAuth(), and the 403 insufficient_scope upscoping path have no test coverage — the only tests of the opt-out exercise the low-level skipIssuerValidation flag or call auth() directly. Since docs/migration.md documents this transport option as the escape hatch for a misconfigured AS, consider adding one test that constructs a transport with skipIssuerMetadataValidation: true against a mock AS whose metadata issuer mismatches and asserts the 401-driven auth() / finishAuth() path succeeds (and throws IssuerMismatchError without the flag), so a future refactor can't silently drop the forwarding.
Extended reasoning...
What's untested. This revision adds skipIssuerMetadataValidation to both StreamableHTTPClientTransportOptions and SSEClientTransportOptions and threads it through several forwarding sites: the constructor passes it into adaptOAuthProvider(provider, { skipIssuerMetadataValidation }), which forwards it to handleOAuthUnauthorized() and from there into auth() on the automatic 401 path; both finishAuth() implementations and the 403 insufficient_scope upscoping branch in StreamableHTTPClientTransport._send() also forward this._skipIssuerMetadataValidation into their auth() calls (streamableHttp.ts:293-295, 745, 911; sse.ts:108-110, 283). None of this plumbing is exercised by any test.
What the existing tests cover instead. A repo-wide grep for skipIssuerMetadataValidation in test files finds it only in test/e2e/scenarios/client-auth.test.ts (the client-auth:iss:opt-out scenario, lines ~626-640), which calls auth(provider, { serverUrl, skipIssuerMetadataValidation: true, fetchFn }) directly — bypassing the transport entirely. packages/client/test/client/auth.test.ts tests the low-level skipIssuerValidation flag on discoverAuthorizationServerMetadata() only, and the sse.test.ts changes in this PR merely fix the mock AS issuer so existing tests satisfy the new validation. So the chain transport option → adaptOAuthProvider extraAuthOptions → handleOAuthUnauthorized → auth() (and the finishAuth() / upscoping forwarding) has zero coverage.
Why it matters. docs/migration.md (and migration-SKILL.md) explicitly tell hosts hitting IssuerMismatchError{kind:'metadata'} to set skipIssuerMetadataValidation: true "on StreamableHTTPClientTransportOptions / SSEClientTransportOptions" — this transport option is the documented escape hatch for transport-managed hosts (it was added in this revision specifically in response to an earlier review comment about the flag being unreachable from the transport flow). The repo checklist requires vitest coverage for new behavior including error paths. Because the forwarding is a thin pass-through across four call sites, a future refactor of adaptOAuthProvider, handleOAuthUnauthorized, or finishAuth could drop one of the forwards and no test would fail — the documented workaround would silently stop working for exactly the hosts the migration guide targets.
Concrete regression scenario. (1) A later cleanup rewrites adaptOAuthProvider to drop the extraAuthOptions parameter (it looks unused — no test references it). (2) A host with a known-misconfigured AS sets skipIssuerMetadataValidation: true on the transport options per the migration guide. (3) Their first request 401s, handleOAuthUnauthorized calls auth() without the flag, discoverAuthorizationServerMetadata() throws IssuerMismatchError{kind:'metadata'}, and connect fails despite the documented opt-out being set. (4) CI is green the whole time, because the only opt-out tests call auth() directly with the flag.
Why this is a nit rather than a blocker. The forwarding code itself appears correct, and the underlying behavior (auth()-level opt-out and discoverAuthorizationServerMetadata()-level skip) is tested — the gap is purely the thin transport-to-auth() forwarding layer. There is no demonstrated runtime defect in this PR.
How to fix. Add one unit test (e.g. in packages/client/test/client/streamableHttp.test.ts or sse.test.ts) that constructs a transport with skipIssuerMetadataValidation: true and an OAuthClientProvider against a mock AS whose metadata issuer mismatches the discovery URL, drives the 401 → onUnauthorized → auth() path (or finishAuth()), and asserts the flow proceeds (e.g. reaches redirect/registration); a sibling assertion without the flag should reject with IssuerMismatchError. That single test pins all the new public-API forwarding this revision adds.
2 participants
9beb2b1d2Implements the OAuth mix-up defenses, default-ON: RFC 8414 §3.3 metadata
issuerecho validation indiscoverAuthorizationServerMetadata(), RFC 9207issvalidation before token exchange inauthInternal/exchangeAuthorization/fetchToken, andfinishAuth(code, iss?)on both HTTP transports. New exportedIssuerMismatchError {kind: 'metadata' | 'authorization_response'}and pure helpervalidateAuthorizationResponseIssuer().Motivation and Context
SEP-2468 (modelcontextprotocol/modelcontextprotocol#2468, spec wording in #2646).
Normative sentences implemented:
The validator implements the spec's 4-row decision table with exact
===comparison (noURLconstruction).IssuerMismatchErrorextendsError(notOAuthError) so the existingauth()retry/invalidate catch does not swallow it; its message JSON-encodes received/expected to guard against log injection from attacker-controllediss.How Has This Been Tested?
validateAuthorizationResponseIssuertable-driven over all 4 rows + each forbidden normalization (packages/client/test/client/auth.test.ts).client-auth:iss:{match, mismatch-reject, supported-missing-reject, unadvertised-proceed, no-normalize, opt-out}requirement rows;as-metadata-discovery:issuer-validationflips from knownFailure to pass.auth/metadata-issuer-mismatchburns; the 6auth/iss-*cells pass the SEP-2468 check (they stay listed until PR 3 because the referee bundles a SEP-837application_typecheck into the same scenarios).Breaking Changes
Yes — new default-on rejection paths.
discoverAuthorizationServerMetadata()now throwsIssuerMismatchError{kind:'metadata'}when the metadataissuerdoes not echo the well-known URL it was fetched from. Hosts whose AS serves a non-echoing issuer (a latent RFC 8414 §3.3 violation) will fail at discovery; the documented escape hatch isskipIssuerMetadataValidation: true(suppresses AU-02 only, not the runtimeisscheck).auth()/finishAuth()now reject the authorization response when the AS advertisedauthorization_response_iss_parameter_supported: truebut the host did not passiss. Hosts must extractissfrom the callback URL alongsidecodeand pass it tofinishAuth(code, iss). ASes that do not advertise support are unaffected.Both are spec MUSTs; per the gating frame in DECISIONS.md Q4 they ship default-on with a named opt-out, not opt-in.
Types of changes
Checklist
Additional context
skipIssuerMetadataValidationsuppresses only the metadata echo check (AU-02), never the runtimeisscheck (AU-01) — AU-01 already degrades safely via decision-table rows 3/4 when the AS doesn't advertise support.auth/2025-03-26-oauth-metadata-backcompatis newly listed in the 2025 baseline because the deprecated referee mock serves a §3.3-violating issuer; that scenario isremovedIn: 2025-06-18and the mock is fixed by conformance#359 (adopted in PR 7).