feat(sep-2468): cover the remaining untested traceability rows (#304)
Add two client scenarios for the SEP-2468 requirements that had no conformance coverage: - auth/metadata-issuer-mismatch: the AS metadata document declares an issuer that differs from the issuer identifier used to construct the well-known URL; the client must not use the metadata (sep-2468-client-validate-metadata-issuer). - auth/iss-normalized: the authorization redirect carries an iss value that only matches the recorded issuer after URL normalization; the client must compare with simple string comparison and reject (sep-2468-client-no-normalization). The two authorization-server-side rows (AS SHOULD include iss, AS MUST advertise authorization_response_iss_parameter_supported) are excluded: observing iss in an authorization response requires driving an Authorization Code Grant against the AS under test, which the authorization-server suite does not implement yet (#208).
This commit is contained in:
Paul Carleton
GitHub
parent
909ea0b5fd
commit
f12baed382
5 changed files with 518 additions and 13 deletions
|
|
@ -45,8 +45,20 @@ export interface AuthServerOptions {
|
|||
codeChallengeMethodsSupported?: string[] | null;
|
||||
/** Advertise authorization_response_iss_parameter_supported in AS metadata. Default: true. Pass null to omit. */
|
||||
issParameterSupported?: boolean | null;
|
||||
/** What iss value to include in authorization redirect. Default: 'correct'. */
|
||||
issInRedirect?: 'correct' | 'wrong' | 'omit';
|
||||
/**
|
||||
* What iss value to include in authorization redirect. Default: 'correct'.
|
||||
* 'normalized' sends a normalization-equivalent variant of the correct
|
||||
* issuer (trailing slash appended, exactly what `new URL(x).href`
|
||||
* round-tripping produces) — equal under RFC 3986 §6.2.2–6.2.3
|
||||
* normalization but not under simple string comparison.
|
||||
*/
|
||||
issInRedirect?: 'correct' | 'wrong' | 'omit' | 'normalized';
|
||||
/**
|
||||
* Override the `issuer` value served in the AS metadata document. Used to
|
||||
* test that clients validate the metadata issuer against the issuer
|
||||
* identifier used to construct the well-known URL (RFC 8414 §3.3).
|
||||
*/
|
||||
metadataIssuer?: string;
|
||||
tokenVerifier?: MockTokenVerifier;
|
||||
onTokenRequest?: (requestData: {
|
||||
scope?: string;
|
||||
|
|
@ -92,6 +104,7 @@ export function createAuthServer(
|
|||
codeChallengeMethodsSupported = ['S256'],
|
||||
issParameterSupported = true,
|
||||
issInRedirect = 'correct',
|
||||
metadataIssuer,
|
||||
tokenVerifier,
|
||||
onTokenRequest,
|
||||
onAuthorizationRequest,
|
||||
|
|
@ -140,7 +153,7 @@ export function createAuthServer(
|
|||
});
|
||||
|
||||
const metadata: any = {
|
||||
issuer: `${getAuthBaseUrl()}${routePrefix}`,
|
||||
issuer: metadataIssuer ?? `${getAuthBaseUrl()}${routePrefix}`,
|
||||
authorization_endpoint: `${getAuthBaseUrl()}${authRoutes.authorization_endpoint}`,
|
||||
token_endpoint: `${getAuthBaseUrl()}${authRoutes.token_endpoint}`,
|
||||
...(!disableDynamicRegistration && {
|
||||
|
|
@ -258,6 +271,11 @@ export function createAuthServer(
|
|||
redirectUrl.searchParams.set('iss', `${getAuthBaseUrl()}${routePrefix}`);
|
||||
} else if (issInRedirect === 'wrong') {
|
||||
redirectUrl.searchParams.set('iss', 'https://evil.example.com');
|
||||
} else if (issInRedirect === 'normalized') {
|
||||
// Normalization-equivalent variant of the correct issuer: identical
|
||||
// after RFC 3986 scheme-based normalization (trailing slash on an
|
||||
// empty path) but different under simple string comparison.
|
||||
redirectUrl.searchParams.set('iss', `${getAuthBaseUrl()}${routePrefix}/`);
|
||||
}
|
||||
|
||||
res.redirect(redirectUrl.toString());
|
||||
|
|
|
|||
|
|
@ -17,6 +17,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 issNormalizeClient } from '../../../../examples/clients/typescript/auth-test-iss-normalize';
|
||||
import { runClient as echoScopeClient } from '../../../../examples/clients/typescript/auth-test-echo-scope';
|
||||
import { getHandler } from '../../../../examples/clients/typescript/everything-client';
|
||||
import { setLogLevel } from '../../../../examples/clients/typescript/helpers/logger';
|
||||
|
|
@ -40,7 +41,10 @@ const allowClientErrorScenarios = new Set<string>([
|
|||
// Client is expected to error when iss validation fails
|
||||
'auth/iss-supported-missing',
|
||||
'auth/iss-wrong-issuer',
|
||||
'auth/iss-unexpected'
|
||||
'auth/iss-unexpected',
|
||||
'auth/iss-normalized',
|
||||
// Client is expected to error when AS metadata issuer validation fails
|
||||
'auth/metadata-issuer-mismatch'
|
||||
]);
|
||||
|
||||
describe('Client Auth Scenarios', () => {
|
||||
|
|
@ -214,4 +218,20 @@ describe('Negative tests', () => {
|
|||
allowClientError: true
|
||||
});
|
||||
});
|
||||
|
||||
test('client normalizes iss before comparison and accepts a trailing-slash variant', async () => {
|
||||
const runner = new InlineClientRunner(issNormalizeClient);
|
||||
await runClientAgainstScenario(runner, 'auth/iss-normalized', {
|
||||
expectedFailureSlugs: ['sep-2468-client-no-normalization'],
|
||||
allowClientError: true
|
||||
});
|
||||
});
|
||||
|
||||
test('client uses AS metadata whose issuer does not match the well-known URL', async () => {
|
||||
const runner = new InlineClientRunner(noIssValidationClient);
|
||||
await runClientAgainstScenario(runner, 'auth/metadata-issuer-mismatch', {
|
||||
expectedFailureSlugs: ['sep-2468-client-validate-metadata-issuer'],
|
||||
allowClientError: true
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -34,7 +34,9 @@ import {
|
|||
IssParameterNotAdvertisedScenario,
|
||||
IssParameterSupportedMissingScenario,
|
||||
IssParameterWrongIssuerScenario,
|
||||
IssParameterUnexpectedScenario
|
||||
IssParameterUnexpectedScenario,
|
||||
IssParameterNormalizedVariantScenario,
|
||||
MetadataIssuerMismatchScenario
|
||||
} from './issuer-parameter';
|
||||
|
||||
// Auth scenarios (required for tier 1)
|
||||
|
|
@ -75,5 +77,7 @@ export const draftScenariosList: Scenario[] = [
|
|||
new IssParameterNotAdvertisedScenario(),
|
||||
new IssParameterSupportedMissingScenario(),
|
||||
new IssParameterWrongIssuerScenario(),
|
||||
new IssParameterUnexpectedScenario()
|
||||
new IssParameterUnexpectedScenario(),
|
||||
new IssParameterNormalizedVariantScenario(),
|
||||
new MetadataIssuerMismatchScenario()
|
||||
];
|
||||
|
|
|
|||
|
|
@ -7,6 +7,10 @@ import { SpecReferences } from './spec-references.js';
|
|||
import { MockTokenVerifier } from './helpers/mockTokenVerifier.js';
|
||||
|
||||
const specRefs = [SpecReferences.RFC_9207_ISS_PARAMETER];
|
||||
const metadataSpecRefs = [
|
||||
SpecReferences.RFC_AUTH_SERVER_METADATA_REQUEST,
|
||||
SpecReferences.MCP_AUTH_DISCOVERY
|
||||
];
|
||||
|
||||
/**
|
||||
* Scenario: ISS Parameter Supported (positive)
|
||||
|
|
@ -406,3 +410,199 @@ export class IssParameterUnexpectedScenario implements Scenario {
|
|||
return this.checks;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Scenario: ISS Parameter Is a Normalization-Equivalent Variant (client must reject)
|
||||
*
|
||||
* Server advertises authorization_response_iss_parameter_supported: true and
|
||||
* includes an iss value that differs from the recorded issuer only by RFC 3986
|
||||
* normalization (a trailing slash on an empty path — exactly what
|
||||
* `new URL(x).href` round-tripping produces). SEP-2468 requires simple string
|
||||
* comparison with no scheme/host case folding, default-port elision,
|
||||
* trailing-slash, or percent-encoding normalization, so a conformant client
|
||||
* MUST treat the variant as a mismatch and reject the response.
|
||||
*/
|
||||
export class IssParameterNormalizedVariantScenario implements Scenario {
|
||||
name = 'auth/iss-normalized';
|
||||
readonly source = { introducedIn: DRAFT_PROTOCOL_VERSION } as const;
|
||||
description =
|
||||
'Tests that client compares iss using simple string comparison without applying URL normalization';
|
||||
allowClientError = true;
|
||||
|
||||
private authServer = new ServerLifecycle();
|
||||
private server = new ServerLifecycle();
|
||||
private checks: ConformanceCheck[] = [];
|
||||
private authReached = false;
|
||||
private tokenRequestMade = false;
|
||||
|
||||
async start(): Promise<ScenarioUrls> {
|
||||
this.checks = [];
|
||||
this.authReached = false;
|
||||
this.tokenRequestMade = false;
|
||||
|
||||
const tokenVerifier = new MockTokenVerifier(this.checks, []);
|
||||
|
||||
const authApp = createAuthServer(this.checks, this.authServer.getUrl, {
|
||||
tokenVerifier,
|
||||
issParameterSupported: true,
|
||||
issInRedirect: 'normalized', // correct issuer + trailing slash
|
||||
onAuthorizationRequest: () => {
|
||||
this.authReached = true;
|
||||
},
|
||||
onTokenRequest: () => {
|
||||
this.tokenRequestMade = true;
|
||||
return { token: `test-token-${Date.now()}`, scopes: [] };
|
||||
}
|
||||
});
|
||||
await this.authServer.start(authApp);
|
||||
|
||||
const app = createServer(
|
||||
this.checks,
|
||||
this.server.getUrl,
|
||||
this.authServer.getUrl,
|
||||
{ requiredScopes: [], tokenVerifier }
|
||||
);
|
||||
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 === '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()}/`,
|
||||
authReached: this.authReached,
|
||||
tokenRequestMade: this.tokenRequestMade
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
return this.checks;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Scenario: AS Metadata Issuer Mismatch (client must not use the metadata)
|
||||
*
|
||||
* The authorization server metadata document declares an `issuer` that is not
|
||||
* identical to the issuer identifier used to construct the well-known URL.
|
||||
* Per RFC 8414 §3.3 / OpenID Connect Discovery §4.3 (incorporated by
|
||||
* SEP-2468), the client MUST validate the issuer and MUST NOT use the
|
||||
* metadata on mismatch. The endpoints in the poisoned document live under a
|
||||
* non-default route prefix, so any request that reaches them proves the
|
||||
* client used the metadata.
|
||||
*/
|
||||
export class MetadataIssuerMismatchScenario implements Scenario {
|
||||
name = 'auth/metadata-issuer-mismatch';
|
||||
readonly source = { introducedIn: DRAFT_PROTOCOL_VERSION } as const;
|
||||
description =
|
||||
'Tests that client rejects authorization server metadata whose issuer does not match the issuer used to construct the well-known URL';
|
||||
allowClientError = true;
|
||||
|
||||
private authServer = new ServerLifecycle();
|
||||
private server = new ServerLifecycle();
|
||||
private checks: ConformanceCheck[] = [];
|
||||
private metadataEndpointsUsed = false;
|
||||
|
||||
async start(): Promise<ScenarioUrls> {
|
||||
this.checks = [];
|
||||
this.metadataEndpointsUsed = false;
|
||||
|
||||
const tokenVerifier = new MockTokenVerifier(this.checks, []);
|
||||
|
||||
const authApp = createAuthServer(this.checks, this.authServer.getUrl, {
|
||||
tokenVerifier,
|
||||
// The well-known URL is constructed from the AS URL advertised in PRM
|
||||
// (the bare origin), so the document's issuer must equal that origin.
|
||||
// Serve something else entirely.
|
||||
metadataIssuer: 'https://attacker.example.com',
|
||||
// Park the endpoints under a prefix only discoverable via the poisoned
|
||||
// metadata, so "client used the metadata" is observable as a request.
|
||||
routePrefix: '/mismatched-as',
|
||||
onRegistrationRequest: () => {
|
||||
this.metadataEndpointsUsed = true;
|
||||
return {
|
||||
clientId: 'test-client-id',
|
||||
clientSecret: 'test-client-secret'
|
||||
};
|
||||
},
|
||||
onAuthorizationRequest: () => {
|
||||
this.metadataEndpointsUsed = true;
|
||||
},
|
||||
onTokenRequest: () => {
|
||||
this.metadataEndpointsUsed = true;
|
||||
return { token: `test-token-${Date.now()}`, scopes: [] };
|
||||
}
|
||||
});
|
||||
await this.authServer.start(authApp);
|
||||
|
||||
const app = createServer(
|
||||
this.checks,
|
||||
this.server.getUrl,
|
||||
this.authServer.getUrl,
|
||||
{ requiredScopes: [], tokenVerifier }
|
||||
);
|
||||
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 === 'sep-2468-client-validate-metadata-issuer'
|
||||
)
|
||||
) {
|
||||
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
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
return this.checks;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,10 +4,6 @@ requirements:
|
|||
- check: sep-2468-client-validate-metadata-issuer
|
||||
text: 'After retrieving a metadata document, MCP clients MUST validate it as required by RFC8414 Section 3.3 or OpenID Connect Discovery Section 4.3: the issuer value in the document MUST be identical to the issuer identifier used to construct the well-known URL. If they differ, the client MUST NOT use the metadata.'
|
||||
url: https://modelcontextprotocol.io/specification/draft/basic/authorization#authorization-server-metadata-discovery
|
||||
- check: sep-2468-as-include-iss
|
||||
text: 'MCP authorization servers SHOULD include the iss parameter in authorization responses, including error responses, as defined in RFC9207 Section 2.'
|
||||
- check: sep-2468-as-advertise-iss-supported
|
||||
text: 'Authorization servers that include the iss parameter MUST advertise this by setting authorization_response_iss_parameter_supported to true in their metadata (RFC9207 Section 2.3).'
|
||||
- check: sep-2468-client-compare-iss-supported
|
||||
text: 'On receiving the authorization response, MCP clients MUST apply the validation in RFC9207 Section 2.4 before transmitting the authorization code to any token endpoint: authorization_response_iss_parameter_supported true, iss present -> Compare to the recorded issuer using simple string comparison / Before redirecting the user-agent, the client MUST record the issuer value from the selected authorization server validated metadata document and associate it with the same per-request record used to store the PKCE code verifier.'
|
||||
- check: sep-2468-client-reject-missing-iss
|
||||
|
|
@ -19,5 +15,11 @@ requirements:
|
|||
- check: sep-2468-client-no-normalization
|
||||
text: 'After decoding the iss value from the application/x-www-form-urlencoded response per RFC 9207 Section 2.4, clients MUST NOT apply scheme or host case folding, default-port elision, trailing-slash, or percent-encoding normalization (RFC 3986 Sections 6.2.2-6.2.3) before comparison.'
|
||||
|
||||
- text: 'MCP authorization servers SHOULD include the iss parameter in authorization responses, including error responses, as defined in RFC9207 Section 2.'
|
||||
excluded: 'Targets the authorization server under test; observing iss in an authorization response requires driving an Authorization Code Grant against the AS, which the authorization-server suite does not implement yet (it only probes the metadata endpoint).'
|
||||
issue: 'https://github.com/modelcontextprotocol/conformance/issues/208'
|
||||
- text: 'Authorization servers that include the iss parameter MUST advertise this by setting authorization_response_iss_parameter_supported to true in their metadata (RFC9207 Section 2.3).'
|
||||
excluded: 'Conditional on the AS actually including iss in an authorization response, which requires driving an Authorization Code Grant against the AS under test; the authorization-server suite does not implement that yet.'
|
||||
issue: 'https://github.com/modelcontextprotocol/conformance/issues/208'
|
||||
- text: 'This validation applies equally to error responses - on mismatch the client MUST NOT act on or display error, error_description, or error_uri.'
|
||||
excluded: 'display is UI-facing; act-on has no protocol-observable signal beyond the existing reject-on-mismatch checks'
|
||||
|
|
|
|||
Loading…
Reference in a new issue