From 29cb8b8add5b21348f283ba80314c1ba89b0bf56 Mon Sep 17 00:00:00 2001 From: OfficialAbhinavSingh Date: Thu, 3 Sep 2026 01:15:44 +0530 Subject: [PATCH] fix(client/auth): bind negative-check verdicts to an observed prerequisite A negative check that reads only the final verdict scores SUCCESS whenever the client fails to reach the requirement at all: "declined" and "never got far enough to decide" are the same observation. auth/resource-mismatch decided on !authorizationRequestMade alone, so a client that threw before making a single HTTP request passed it. Gate each verdict on the prerequisite the harness already records: prm-pathbased-requested for the PRM resource comparison, and authorization-server-metadata for the RFC 9207 iss comparisons, which SEP-2468 conditions on the issuer recorded from the validated metadata document. When the prerequisite is not observed the requirement was never exercised, so report it via notTestable() (#248) rather than SUCCESS. auth/metadata-issuer-mismatch already gated on the metadata fetch but reported the unreached case as a plain violation; it now reports untestable too. All six checks record propertyReached and stopReason in details. Adds an inert client and a vitest case, so the checks are shown to catch something and not merely to avoid false-positiving. --- .../clients/typescript/auth-test-inert.ts | 22 ++ src/scenarios/client/auth/index.test.ts | 34 ++ src/scenarios/client/auth/issuer-parameter.ts | 297 +++++++++++++----- .../client/auth/resource-mismatch.ts | 69 ++-- 4 files changed, 320 insertions(+), 102 deletions(-) create mode 100644 examples/clients/typescript/auth-test-inert.ts diff --git a/examples/clients/typescript/auth-test-inert.ts b/examples/clients/typescript/auth-test-inert.ts new file mode 100644 index 00000000..74e9be3f --- /dev/null +++ b/examples/clients/typescript/auth-test-inert.ts @@ -0,0 +1,22 @@ +#!/usr/bin/env node + +import { runAsCli } from './helpers/cliRunner'; + +/** + * Broken client that gives up before performing any discovery request. + * + * BUG: it never fetches Protected Resource Metadata, so it never reads — let + * alone validates — the `resource` value the scenario mismatches on purpose. + * + * It exists to pin issue #467. `auth/resource-mismatch` decides its verdict + * from `!authorizationRequestMade` alone, and a client that does nothing at + * all satisfies that verdict, so the check scores SUCCESS for a client that + * cannot possibly have performed the validation under test. + */ +export async function runClient(_serverUrl: string): Promise { + throw new Error( + 'inert client: aborted before any discovery request (no PRM fetch, no authorization request)' + ); +} + +runAsCli(runClient, import.meta.url, 'auth-test-inert '); diff --git a/src/scenarios/client/auth/index.test.ts b/src/scenarios/client/auth/index.test.ts index 57ff27ac..6b2e0872 100644 --- a/src/scenarios/client/auth/index.test.ts +++ b/src/scenarios/client/auth/index.test.ts @@ -26,6 +26,7 @@ import { runClient as noPkceClient } from '../../../../examples/clients/typescri import { runClient as reuseCredsClient } from '../../../../examples/clients/typescript/auth-test-reuse-credentials'; import { runClient as noAppTypeClient } from '../../../../examples/clients/typescript/auth-test-no-application-type'; import { runClient as noIssValidationClient } from '../../../../examples/clients/typescript/auth-test'; +import { runClient as inertClient } from '../../../../examples/clients/typescript/auth-test-inert'; import { runClient as issNormalizeClient } from '../../../../examples/clients/typescript/auth-test-iss-normalize'; import { runClient as echoScopeClient } from '../../../../examples/clients/typescript/auth-test-echo-scope'; import { runClient as dpopBearerClient } from '../../../../examples/clients/typescript/auth-test-dpop-bearer'; @@ -443,3 +444,36 @@ describe('DPoP client nonce-less baseline (SEP-1932)', () => { expect(count('pkce-verifier-matches-challenge')).toBe(1); }); }); + +// Reason-bound negative checks (issue #467). +// +// A negative check that reads only the final verdict scores SUCCESS whenever +// the client fails to reach the requirement at all: "did not proceed" and +// "never got far enough to decide" are the same observation. These tests pin +// that the harness distinguishes them, so a client that cannot have performed +// the validation cannot bank a pass for it. +describe('Reason-bound negative checks (#467)', () => { + test('auth/resource-mismatch: an inert client does not pass by doing nothing', async () => { + // The inert client throws before any discovery request, so it never reads + // the mismatched `resource` it is required to validate. Bound only to the + // verdict (`!authorizationRequestMade`) this scored SUCCESS. + const runner = new InlineClientRunner(inertClient); + const checks = await runClientAgainstScenario( + runner, + 'auth/resource-mismatch', + { + allowClientError: true, + expectedFailureSlugs: ['resource-mismatch-rejected'] + } + ); + + const check = checks.find((c) => c.id === 'resource-mismatch-rejected'); + expect(check).toBeDefined(); + // Reported as untestable (#248), not as a plain violation: the client did + // not break the requirement, it never exercised it. + expect(check?.details?.untestable).toBe(true); + expect(check?.details?.propertyReached).toBe(false); + expect(check?.details?.stopReason).toBe('prm-not-requested'); + expect(check?.errorMessage).toMatch(/^Not testable: /); + }); +}); diff --git a/src/scenarios/client/auth/issuer-parameter.ts b/src/scenarios/client/auth/issuer-parameter.ts index b55e9746..85773e23 100644 --- a/src/scenarios/client/auth/issuer-parameter.ts +++ b/src/scenarios/client/auth/issuer-parameter.ts @@ -6,6 +6,7 @@ import { createServer } from './helpers/createServer.js'; import { ServerLifecycle } from './helpers/serverLifecycle.js'; import { SpecReferences } from './spec-references.js'; import { MockTokenVerifier } from './helpers/mockTokenVerifier.js'; +import { untestableCheck } from '../../untestable.js'; const specRefs = [SpecReferences.RFC_9207_ISS_PARAMETER]; const metadataSpecRefs = [ @@ -13,6 +14,93 @@ const metadataSpecRefs = [ SpecReferences.MCP_AUTH_DISCOVERY ]; +/** + * Reason-bound verdict for the RFC 9207 `iss` rejection checks (issue #467). + * + * `authReached && !tokenRequestMade` is a verdict, not a reason. SEP-2468 + * conditions every one of these requirements on the issuer the client recorded + * "from the selected authorization server validated metadata document", so a + * client that never retrieved that document cannot have performed the + * comparison under test — yet it satisfies the verdict, because not reaching + * the token endpoint is exactly what a client that fell over earlier also + * does. Absent the retrieval the requirement was never exercised, which is the + * untestable case (#248) rather than a pass or a violation. + * + * `auth/metadata-issuer-mismatch` in this same file already gates on the + * metadata fetch; the other checks did not. Keeping the policy in one function + * is deliberate: the duplication is what let five of six sites drift apart. + * + * Residual gap, deliberately not papered over: a client that receives the + * redirect and then aborts before the token request for an unrelated reason is + * still indistinguishable from one that rejected on `iss`. Closing that needs + * a signal from inside the client, which a black-box harness does not have. + * What this closes is the "never reached the requirement at all" class. + */ +function issRejectionCheck(opts: { + id: string; + name: string; + passDescription: string; + failDescription: string; + metadataRequested: boolean; + authReached: boolean; + tokenRequestMade: boolean; + observations: Record; + timestamp: string; +}): ConformanceCheck { + const { + id, + name, + passDescription, + failDescription, + metadataRequested, + authReached, + tokenRequestMade, + observations, + timestamp + } = opts; + + const reached = metadataRequested && authReached; + const observed = { + ...observations, + metadataRequested, + authReached, + tokenRequestMade + }; + + if (!reached) { + const reason = !metadataRequested + ? 'client never retrieved the authorization server metadata document, so it never recorded the issuer this comparison is made against' + : 'client never reached the authorization endpoint, so it never received an authorization response to validate'; + const check = untestableCheck(id, name, failDescription, reason, specRefs); + check.details = { + ...check.details, + ...observed, + propertyReached: false, + stopReason: !metadataRequested + ? 'as-metadata-not-requested' + : 'authorization-endpoint-not-reached' + }; + return check; + } + + const correctlyRejected = !tokenRequestMade; + return { + id, + name, + description: correctlyRejected ? passDescription : failDescription, + status: correctlyRejected ? 'SUCCESS' : 'FAILURE', + timestamp, + specReferences: specRefs, + details: { + ...observed, + propertyReached: true, + stopReason: correctlyRejected + ? 'declined-before-token-request' + : 'transmitted-code-to-token-endpoint' + } + }; +} + /** * Scenario: ISS Parameter Supported (positive) * @@ -217,23 +305,26 @@ export class IssParameterSupportedMissingScenario implements Scenario { if ( !this.checks.some((c) => c.id === 'sep-2468-client-reject-missing-iss') ) { - const correctlyRejected = this.authReached && !this.tokenRequestMade; - this.checks.push({ - id: 'sep-2468-client-reject-missing-iss', - name: 'Client rejects missing iss when required', - description: correctlyRejected - ? 'Client correctly rejected authorization response missing required iss parameter' - : 'Client MUST reject authorization response when server advertised iss support but iss is absent from redirect', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: specRefs, - details: { - serverAdvertisedSupport: true, - issSentInRedirect: false, + this.checks.push( + issRejectionCheck({ + id: 'sep-2468-client-reject-missing-iss', + name: 'Client rejects missing iss when required', + passDescription: + 'Client correctly rejected authorization response missing required iss parameter', + failDescription: + 'Client MUST reject authorization response when server advertised iss support but iss is absent from redirect', + metadataRequested: this.checks.some( + (c) => c.id === 'authorization-server-metadata' + ), authReached: this.authReached, - tokenRequestMade: this.tokenRequestMade - } - }); + tokenRequestMade: this.tokenRequestMade, + observations: { + serverAdvertisedSupport: true, + issSentInRedirect: false + }, + timestamp + }) + ); } return this.checks; @@ -304,23 +395,26 @@ export class IssParameterWrongIssuerScenario implements Scenario { if ( !this.checks.some((c) => c.id === 'sep-2468-client-compare-iss-supported') ) { - const correctlyRejected = this.authReached && !this.tokenRequestMade; - this.checks.push({ - id: 'sep-2468-client-compare-iss-supported', - name: 'Client rejects mismatched iss', - description: correctlyRejected - ? 'Client correctly rejected authorization response with mismatched iss parameter' - : 'Client MUST reject authorization response when iss does not match the authorization server issuer', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: specRefs, - details: { - serverAdvertisedSupport: true, - issSentInRedirect: 'https://evil.example.com', + this.checks.push( + issRejectionCheck({ + id: 'sep-2468-client-compare-iss-supported', + name: 'Client rejects mismatched iss', + passDescription: + 'Client correctly rejected authorization response with mismatched iss parameter', + failDescription: + 'Client MUST reject authorization response when iss does not match the authorization server issuer', + metadataRequested: this.checks.some( + (c) => c.id === 'authorization-server-metadata' + ), authReached: this.authReached, - tokenRequestMade: this.tokenRequestMade - } - }); + tokenRequestMade: this.tokenRequestMade, + observations: { + serverAdvertisedSupport: true, + issSentInRedirect: 'https://evil.example.com' + }, + timestamp + }) + ); } return this.checks; @@ -394,23 +488,26 @@ export class IssParameterUnexpectedScenario implements Scenario { (c) => c.id === 'sep-2468-client-compare-iss-unadvertised' ) ) { - const correctlyRejected = this.authReached && !this.tokenRequestMade; - this.checks.push({ - id: 'sep-2468-client-compare-iss-unadvertised', - name: 'Client compares unadvertised iss and rejects mismatch', - description: correctlyRejected - ? 'Client correctly compared unadvertised iss against recorded issuer and rejected the mismatch' - : 'Client MUST compare a present iss against the recorded issuer regardless of metadata advertisement, and reject on mismatch', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: specRefs, - details: { - serverAdvertisedSupport: false, - issSentInRedirect: 'https://evil.example.com', + this.checks.push( + issRejectionCheck({ + id: 'sep-2468-client-compare-iss-unadvertised', + name: 'Client compares unadvertised iss and rejects mismatch', + passDescription: + 'Client correctly compared unadvertised iss against recorded issuer and rejected the mismatch', + failDescription: + 'Client MUST compare a present iss against the recorded issuer regardless of metadata advertisement, and reject on mismatch', + metadataRequested: this.checks.some( + (c) => c.id === 'authorization-server-metadata' + ), authReached: this.authReached, - tokenRequestMade: this.tokenRequestMade - } - }); + tokenRequestMade: this.tokenRequestMade, + observations: { + serverAdvertisedSupport: false, + issSentInRedirect: 'https://evil.example.com' + }, + timestamp + }) + ); } return this.checks; @@ -483,23 +580,26 @@ export class IssParameterNormalizedVariantScenario implements Scenario { const timestamp = new Date().toISOString(); if (!this.checks.some((c) => c.id === 'sep-2468-client-no-normalization')) { - const correctlyRejected = this.authReached && !this.tokenRequestMade; - this.checks.push({ - id: 'sep-2468-client-no-normalization', - name: 'Client compares iss without URL normalization', - description: correctlyRejected - ? 'Client rejected an iss value that only matches the recorded issuer after URL normalization' - : 'Client MUST NOT apply scheme/host case folding, default-port elision, trailing-slash, or percent-encoding normalization to iss before comparison; a trailing-slash variant of the issuer must be treated as a mismatch', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: specRefs, - details: { - recordedIssuer: this.authServer.getUrl(), - issSentInRedirect: `${this.authServer.getUrl()}/`, + this.checks.push( + issRejectionCheck({ + id: 'sep-2468-client-no-normalization', + name: 'Client compares iss without URL normalization', + passDescription: + 'Client rejected an iss value that only matches the recorded issuer after URL normalization', + failDescription: + 'Client MUST NOT apply scheme/host case folding, default-port elision, trailing-slash, or percent-encoding normalization to iss before comparison; a trailing-slash variant of the issuer must be treated as a mismatch', + metadataRequested: this.checks.some( + (c) => c.id === 'authorization-server-metadata' + ), authReached: this.authReached, - tokenRequestMade: this.tokenRequestMade - } - }); + tokenRequestMade: this.tokenRequestMade, + observations: { + recordedIssuer: this.authServer.getUrl(), + issSentInRedirect: `${this.authServer.getUrl()}/` + }, + timestamp + }) + ); } return this.checks; @@ -589,26 +689,55 @@ export class MetadataIssuerMismatchScenario implements Scenario { const metadataRequested = this.checks.some( (c) => c.id === 'authorization-server-metadata' ); - const correctlyRejected = - metadataRequested && !this.metadataEndpointsUsed; - this.checks.push({ - id: 'sep-2468-client-validate-metadata-issuer', - name: 'Client validates metadata issuer against well-known URL', - description: correctlyRejected - ? 'Client rejected authorization server metadata whose issuer does not match the issuer identifier used to construct the well-known URL' - : metadataRequested - ? 'Client MUST NOT use authorization server metadata whose issuer differs from the issuer identifier used to construct the well-known URL; client used endpoints from the mismatched metadata' - : 'Client never retrieved the authorization server metadata document, so issuer validation could not be observed', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: metadataSpecRefs, - details: { - expectedIssuer: this.authServer.getUrl(), - metadataIssuer: 'https://attacker.example.com', - metadataRequested, - metadataEndpointsUsed: this.metadataEndpointsUsed - } - }); + const observations = { + expectedIssuer: this.authServer.getUrl(), + metadataIssuer: 'https://attacker.example.com', + metadataRequested, + metadataEndpointsUsed: this.metadataEndpointsUsed + }; + const failDescription = + 'Client MUST NOT use authorization server metadata whose issuer differs from the issuer identifier used to construct the well-known URL; client used endpoints from the mismatched metadata'; + + // This check already bound its verdict to the metadata fetch, but + // reported the unreached case as a plain violation. Per #248 a + // requirement that could not be exercised is untestable, so the report + // distinguishes "client used the poisoned metadata" from "client never + // fetched it" instead of collapsing both into FAILURE (#467). + if (!metadataRequested) { + const check = untestableCheck( + 'sep-2468-client-validate-metadata-issuer', + 'Client validates metadata issuer against well-known URL', + failDescription, + 'client never retrieved the authorization server metadata document, so issuer validation could not be observed', + metadataSpecRefs + ); + check.details = { + ...check.details, + ...observations, + propertyReached: false, + stopReason: 'as-metadata-not-requested' + }; + this.checks.push(check); + } else { + const correctlyRejected = !this.metadataEndpointsUsed; + this.checks.push({ + id: 'sep-2468-client-validate-metadata-issuer', + name: 'Client validates metadata issuer against well-known URL', + description: correctlyRejected + ? 'Client rejected authorization server metadata whose issuer does not match the issuer identifier used to construct the well-known URL' + : failDescription, + status: correctlyRejected ? 'SUCCESS' : 'FAILURE', + timestamp, + specReferences: metadataSpecRefs, + details: { + ...observations, + propertyReached: true, + stopReason: correctlyRejected + ? 'declined-poisoned-metadata' + : 'used-poisoned-metadata-endpoints' + } + }); + } } return this.checks; diff --git a/src/scenarios/client/auth/resource-mismatch.ts b/src/scenarios/client/auth/resource-mismatch.ts index 2e8b02f9..a5cc157a 100644 --- a/src/scenarios/client/auth/resource-mismatch.ts +++ b/src/scenarios/client/auth/resource-mismatch.ts @@ -6,6 +6,7 @@ import { createServer } from './helpers/createServer.js'; import { ServerLifecycle } from './helpers/serverLifecycle.js'; import { SpecReferences } from './spec-references.js'; import { MockTokenVerifier } from './helpers/mockTokenVerifier.js'; +import { untestableCheck } from '../../untestable.js'; /** * Scenario: Resource Mismatch Detection @@ -90,25 +91,57 @@ export class ResourceMismatchScenario implements Scenario { SpecReferences.MCP_RESOURCE_PARAMETER ]; - // The test passes if the client did NOT make an authorization request - // (meaning it correctly rejected the mismatched resource) + // Reason-bound verdict (#467). "Did not proceed with authorization" is not + // by itself evidence of validation: a client that never fetched the PRM + // document never read the mismatched `resource`, so it cannot have + // compared it. Absent that fetch the requirement was never exercised, + // which is the untestable case (#248) rather than a pass or a violation. if (!this.checks.some((c) => c.id === 'resource-mismatch-rejected')) { - const correctlyRejected = !this.authorizationRequestMade; - this.checks.push({ - id: 'resource-mismatch-rejected', - name: 'Client rejects mismatched resource', - description: correctlyRejected - ? 'Client correctly rejected authorization when PRM resource does not match server URL' - : 'Client MUST validate that PRM resource matches the server URL before proceeding with authorization', - status: correctlyRejected ? 'SUCCESS' : 'FAILURE', - timestamp, - specReferences: specRefs, - details: { - prmResource: 'https://evil.example.com/mcp', - expectedBehavior: 'Client should NOT proceed with authorization', - authorizationRequestMade: this.authorizationRequestMade - } - }); + const prmRequested = this.checks.some( + (c) => c.id === 'prm-pathbased-requested' + ); + const correctlyRejected = prmRequested && !this.authorizationRequestMade; + const observations = { + prmResource: 'https://evil.example.com/mcp', + expectedBehavior: 'Client should NOT proceed with authorization', + prmRequested, + authorizationRequestMade: this.authorizationRequestMade + }; + + if (!prmRequested) { + const check = untestableCheck( + 'resource-mismatch-rejected', + 'Client rejects mismatched resource', + 'Client MUST validate that PRM resource matches the server URL before proceeding with authorization', + 'client never requested the Protected Resource Metadata document, so it never read the resource value it was required to validate', + specRefs + ); + check.details = { + ...check.details, + ...observations, + propertyReached: false, + stopReason: 'prm-not-requested' + }; + this.checks.push(check); + } else { + this.checks.push({ + id: 'resource-mismatch-rejected', + name: 'Client rejects mismatched resource', + description: correctlyRejected + ? 'Client correctly rejected authorization when PRM resource does not match server URL' + : 'Client MUST validate that PRM resource matches the server URL before proceeding with authorization', + status: correctlyRejected ? 'SUCCESS' : 'FAILURE', + timestamp, + specReferences: specRefs, + details: { + ...observations, + propertyReached: true, + stopReason: correctlyRejected + ? 'declined-after-reading-prm' + : 'proceeded-to-authorization' + } + }); + } } return this.checks;