fix: rename routePrefix to issuerPath and add issuer-mismatch scenario#203
fix: rename routePrefix to issuerPath and add issuer-mismatch scenario#203
Conversation
The previous fix (#152) used routePrefix for the issuer path, conflating two concepts: (1) where OAuth endpoints are mounted and (2) the issuer path component per RFC 8414. This worked for metadata-var2/var3 where both were /tenant1, but broke auth/2025-03-26-oauth-metadata-backcompat where routePrefix was /oauth (endpoint namespacing) but the issuer should be root. Changes: - Rename routePrefix -> issuerPath in createAuthServer. Endpoints are mounted under the issuer path (the real-world multi-tenant model). - Drop the /oauth prefix from march-spec-backcompat entirely. No route collision exists with createServer, and test integrity against fallback-bypass is already guaranteed by expectedSlugs requiring authorization-server-metadata. - Rename authRoutePrefix -> authIssuerPath in discovery-metadata config. - Add issuerOverride option to createAuthServer for negative testing. - Add auth/issuer-mismatch scenario: AS returns a mismatched issuer (https://evil.example.com) and the client is expected to reject per RFC 8414 section 3.3. Skipped in CI for now as the TS SDK reference client does not yet validate issuer. Fixes #140
|
@claude review |
1 similar comment
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔴 This check can false-pass: correctlyRejected is derived solely from \!authorizationRequestMade, so a client that errors out before ever fetching AS metadata (e.g. fails PRM parsing or doesn't support OAuth at all) will be reported as having 'correctly rejected the mismatched issuer' — even though it never saw the issuer. Consider gating SUCCESS on this.checks.some(c => c.id === 'authorization-server-metadata') so the client must have actually fetched the metadata containing the bad issuer. (Note: resource-mismatch.ts has the analogous pre-existing weakness for PRM, but this PR adds a fresh instance.)
Extended reasoning...
What the bug is
IssuerMismatchScenario.getChecks() decides pass/fail purely on whether the /authorize endpoint was hit:
const correctlyRejected = \!this.authorizationRequestMade;
// ...
status: correctlyRejected ? 'SUCCESS' : 'FAILURE',It never checks whether the client actually fetched the AS metadata document containing the mismatched issuer. Combined with allowClientError = true (line 35), any client that errors out early — for reasons completely unrelated to issuer validation — will be credited with a SUCCESS.
Code path that triggers it
- Client hits
/mcp, receives 401 withWWW-Authenticatepointing at the PRM. - Client fails at this stage — e.g. it doesn't implement OAuth, can't parse the PRM response, or hits any unrelated exception.
- Client process exits non-zero. Because
allowClientError = true, the runner tolerates this. /authorizewas never called →authorizationRequestMadeis stillfalse./.well-known/oauth-authorization-serverwas also never called → noauthorization-server-metadatacheck was pushed intothis.checks.getChecks()computescorrectlyRejected = \!false = trueand pushes a SUCCESS forissuer-mismatch-rejectedwith the description "Client correctly rejected authorization when AS metadata issuer does not match the metadata URL".
The client never saw issuer: "https://evil.example.com", yet the conformance report says it correctly validated it.
Why existing code doesn't prevent it
Unlike discovery-metadata.ts and march-spec-backcompat.ts, this scenario has no expectedSlugs precondition requiring authorization-server-metadata to be present. The only signal used is the absence of an authorize call, which is necessary but not sufficient — it conflates "rejected after seeing the mismatch" with "never got that far".
createAuthServer already pushes an authorization-server-metadata check into this.checks when the metadata endpoint is hit (createAuthServer.ts:132-146), so the signal needed to disambiguate is already available — it's just not consulted.
Impact
For a conformance suite, false positives are arguably worse than false negatives: a client that doesn't implement RFC 8414 §3.3 issuer validation (or doesn't implement OAuth discovery at all) could be reported as conformant on this check. This is a draft/informational scenario and currently skipped in CI for the TS reference client, so the immediate blast radius is small — but third-party clients running the suite would get misleading results.
Fix
Gate the SUCCESS on the metadata having been fetched:
const sawMetadata = this.checks.some(
(c) => c.id === 'authorization-server-metadata'
);
const correctlyRejected = sawMetadata && \!this.authorizationRequestMade;
// ...
description: \!sawMetadata
? 'Client never fetched AS metadata, so issuer validation could not be tested'
: correctlyRejected
? 'Client correctly rejected authorization when AS metadata issuer does not match the metadata URL'
: 'Client MUST validate that the issuer in AS metadata matches the issuer used to construct the metadata URL (RFC 8414 §3.3)',resource-mismatch.ts has the same structural weakness (it should gate on the PRM endpoint having been hit), but that's pre-existing and out of scope for this PR.
1 participant
The previous fix (#152) used routePrefix for the issuer path, conflating two concepts: (1) where OAuth endpoints are mounted and (2) the issuer path component per RFC 8414. This worked for metadata-var2/var3 where both were /tenant1, but broke auth/2025-03-26-oauth-metadata-backcompat where routePrefix was /oauth (endpoint namespacing) but the issuer should be root.
Changes:
Fixes #140