fix: rename routePrefix to issuerPath and add issuer-mismatch scenario #203
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| import type { Scenario, ConformanceCheck } from '../../../types.js'; | ||
| import { ScenarioUrls, SpecVersion } from '../../../types.js'; | ||
| import { createAuthServer } from './helpers/createAuthServer.js'; | ||
| import { createServer } from './helpers/createServer.js'; | ||
| import { ServerLifecycle } from './helpers/serverLifecycle.js'; | ||
| import { SpecReferences } from './spec-references.js'; | ||
|
|
||
| /** | ||
| * Scenario: Authorization Server Issuer Mismatch Detection | ||
| * | ||
| * Tests that clients correctly detect and reject when the Authorization | ||
| * Server metadata response contains an `issuer` value that doesn't match | ||
| * the issuer identifier used to construct the metadata URL. | ||
| * | ||
| * Per RFC 8414 §3.3, clients MUST validate that the issuer in the metadata | ||
| * response matches the issuer used to construct the well-known metadata URL. | ||
| * Failing to do so enables mix-up attacks where a malicious AS impersonates | ||
| * another. | ||
| * | ||
| * Setup: | ||
| * - PRM advertises authorization server at http://localhost:<port> (root issuer) | ||
| * - Client constructs metadata URL /.well-known/oauth-authorization-server | ||
| * - AS responds with issuer: "https://evil.example.com" (mismatch) | ||
| * | ||
| * Expected behavior: | ||
| * - Client should NOT proceed with authorization | ||
| * - Client should abort due to issuer mismatch | ||
| * - Test passes if client does NOT make an authorization request | ||
| */ | ||
| export class IssuerMismatchScenario implements Scenario { | ||
| name = 'auth/issuer-mismatch'; | ||
| specVersions: SpecVersion[] = ['draft']; | ||
| description = | ||
| 'Tests that client rejects when AS metadata issuer does not match the issuer used to construct the metadata URL (RFC 8414 §3.3)'; | ||
| allowClientError = true; | ||
|
|
||
| private authServer = new ServerLifecycle(); | ||
| private server = new ServerLifecycle(); | ||
| private checks: ConformanceCheck[] = []; | ||
| private authorizationRequestMade = false; | ||
|
|
||
| async start(): Promise<ScenarioUrls> { | ||
| this.checks = []; | ||
| this.authorizationRequestMade = false; | ||
|
|
||
| const authApp = createAuthServer(this.checks, this.authServer.getUrl, { | ||
| // Root issuer: metadata at /.well-known/oauth-authorization-server, | ||
| // so the expected issuer is just the base URL. Override it to a | ||
| // different origin to trigger the mismatch. | ||
| issuerOverride: 'https://evil.example.com', | ||
| onAuthorizationRequest: () => { | ||
| // If we get here, the client incorrectly proceeded past issuer validation | ||
| this.authorizationRequestMade = true; | ||
| } | ||
| }); | ||
| await this.authServer.start(authApp); | ||
|
|
||
| const app = createServer( | ||
| this.checks, | ||
| this.server.getUrl, | ||
| this.authServer.getUrl, | ||
| { | ||
| prmPath: '/.well-known/oauth-protected-resource/mcp' | ||
| } | ||
| ); | ||
| await this.server.start(app); | ||
|
|
||
| return { serverUrl: `${this.server.getUrl()}/mcp` }; | ||
| } | ||
|
|
||
| async stop() { | ||
| await this.authServer.stop(); | ||
| await this.server.stop(); | ||
| } | ||
|
|
||
| getChecks(): ConformanceCheck[] { | ||
| const timestamp = new Date().toISOString(); | ||
|
|
||
| if (!this.checks.some((c) => c.id === 'issuer-mismatch-rejected')) { | ||
| const correctlyRejected = !this.authorizationRequestMade; | ||
| this.checks.push({ | ||
| id: 'issuer-mismatch-rejected', | ||
| name: 'Client rejects mismatched issuer', | ||
| description: 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)', | ||
| status: correctlyRejected ? 'SUCCESS' : 'FAILURE', | ||
| timestamp, | ||
| specReferences: [SpecReferences.RFC_AUTH_SERVER_METADATA_VALIDATION], | ||
| details: { | ||
| metadataIssuer: 'https://evil.example.com', | ||
| expectedIssuer: this.authServer.getUrl(), | ||
| expectedBehavior: 'Client should NOT proceed with authorization', | ||
| authorizationRequestMade: this.authorizationRequestMade | ||
| } | ||
| }); | ||
|
Check failure on line 96 in src/scenarios/client/auth/issuer-mismatch.ts
|
||
| } | ||
|
|
||
| return this.checks; | ||
| } | ||
| } | ||
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.
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.
🔴 This check can false-pass:
correctlyRejectedis 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 onthis.checks.some(c => c.id === 'authorization-server-metadata')so the client must have actually fetched the metadata containing the bad issuer. (Note:resource-mismatch.tshas 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/authorizeendpoint was hit: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
/mcp, receives 401 withWWW-Authenticatepointing at the PRM.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.tsandmarch-spec-backcompat.ts, this scenario has noexpectedSlugsprecondition requiringauthorization-server-metadatato 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".createAuthServeralready pushes anauthorization-server-metadatacheck intothis.checkswhen 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:
resource-mismatch.tshas 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.