Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions .changeset/fix-replay-store-3338.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
---

fix(training-agent): isolate replay store on /mcp-strict (closes #3338)

The post-5.21.1 grader run surfaced `neg/016-replayed-nonce` accepting both submissions of the same `(keyid, nonce)` pair on `/mcp-strict` — a MUST-level RFC 9421 §3.3.2 violation.

Root cause: `/mcp-strict` was using the same `lazySigningAuth()` singleton as `/mcp`, so they shared one `InMemoryReplayStore`. The shared singleton was also bound to the *default* capability (`required_for: []`) rather than the strict one (`required_for: ['create_media_buy']`) — a quieter conformance gap that compounded with the replay leak.

Adds `buildStrictRequestSigningAuthenticator()` in `request-signing.ts` (parallel to the existing strict-required and strict-forbidden builders), and a matching `lazyStrictSigningAuth()` in `index.ts`. `/mcp-strict` now binds to its own replay store and the strict capability.

Un-skips the regression test at `server/tests/integration/training-agent-strict.test.ts:124` (was skipped per #3080 with a stale assertion); the message regex is updated to match the SDK's current `Signature required for create_media_buy.` text.

The triage's bug #1 ("bearer evaluated before signing") didn't reproduce against `@adcp/client@5.21.1` — `requireSignatureWhenPresent` already implements presence-first ordering. The per-route signing-auth instances eliminate any leftover bypass surface regardless.
18 changes: 14 additions & 4 deletions server/src/training-agent/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import { SIGNAL_PROVIDERS } from './signal-providers.js';
import { getPublicJwks } from './webhooks.js';
import {
buildRequestSigningAuthenticator,
buildStrictRequestSigningAuthenticator,
buildStrictRequiredRequestSigningAuthenticator,
buildStrictForbiddenRequestSigningAuthenticator,
enforceSigningWhenWebhookAuthPresent,
Expand Down Expand Up @@ -118,9 +119,10 @@ function buildBearerAuthenticator(): Authenticator | null {
return authenticators.length === 1 ? authenticators[0] : anyOf(...authenticators);
}

// Wrapped so the signing authenticator is lazily built on first auth call —
// avoids reading the compliance test JWKS at module import time, which would
// break test setups that mock the compliance cache.
// Per-route lazy signing authenticators. Each route MUST own its own
// `InMemoryReplayStore` — sharing one store lets a nonce consumed on
// one route falsely fire `request_signature_replayed` on another (#3338).
// Lazy so the compliance test JWKS isn't read at module import time.
let _signingAuth: Authenticator | null = null;
function lazySigningAuth(): Authenticator {
return (req) => {
Expand All @@ -129,6 +131,14 @@ function lazySigningAuth(): Authenticator {
};
}

let _strictSigningAuth: Authenticator | null = null;
function lazyStrictSigningAuth(): Authenticator {
return (req) => {
if (!_strictSigningAuth) _strictSigningAuth = buildStrictRequestSigningAuthenticator();
return _strictSigningAuth(req);
};
}

let _strictRequiredSigningAuth: Authenticator | null = null;
function lazyStrictRequiredSigningAuth(): Authenticator {
return (req) => {
Expand Down Expand Up @@ -206,7 +216,7 @@ function buildStrictModeAuthenticator(lazyAuth: () => Authenticator): Authentica
}

const defaultAuthenticator = buildDefaultAuthenticator();
const strictAuthenticator = buildStrictModeAuthenticator(lazySigningAuth);
const strictAuthenticator = buildStrictModeAuthenticator(lazyStrictSigningAuth);
const strictRequiredAuthenticator = buildStrictModeAuthenticator(lazyStrictRequiredSigningAuth);
const strictForbiddenAuthenticator = buildStrictModeAuthenticator(lazyStrictForbiddenSigningAuth);

Expand Down
9 changes: 9 additions & 0 deletions server/src/training-agent/request-signing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,15 @@ export function buildRequestSigningAuthenticator(): Authenticator {
return buildAuthenticatorWithCapability(getRequestSigningCapability());
}

/** Authenticator for `/mcp-strict`: presence-gated signing with
* `required_for: ['create_media_buy']` and `'either'` content-digest mode.
* Distinct from the default authenticator so each route owns an isolated
* `InMemoryReplayStore` — sharing one store lets a nonce consumed on `/mcp`
* falsely fire `request_signature_replayed` on `/mcp-strict` (#3338). */
export function buildStrictRequestSigningAuthenticator(): Authenticator {
return buildAuthenticatorWithCapability(getStrictRequestSigningCapability());
}

/** Authenticator for `/mcp-strict-required`: enforces `covers_content_digest='required'`. */
export function buildStrictRequiredRequestSigningAuthenticator(): Authenticator {
return buildAuthenticatorWithCapability(getStrictRequiredRequestSigningCapability());
Expand Down
11 changes: 6 additions & 5 deletions server/tests/integration/training-agent-strict.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,18 +120,19 @@ describe('Training Agent /mcp-strict route', () => {
});

describe('presence-gated enforcement', () => {
// Skipped: see #3080 — error_description changed to "Signature required for create_media_buy.";
// assertion regex /create_media_buy.*signed/ no longer matches. Fix lands with #3080.
it.skip('unsigned create_media_buy on /mcp-strict returns 401 request_signature_required', async () => {
// Regression gate for #3338: presence-gated signing on /mcp-strict must
// fire `request_signature_required` for unsigned mutations when the
// strict capability advertises `required_for: ['create_media_buy']`.
it('unsigned create_media_buy on /mcp-strict returns 401 request_signature_required', async () => {
// auth: false — bearer bypass (SDK #2586) short-circuits required_for; graders send no bearer.
const res = await callTool(app, '/mcp-strict', 'create_media_buy', {
account: { brand: { domain: 'strict-test.example.com' }, sandbox: true },
idempotency_key: '550e8400-e29b-41d4-a716-446655440000',
}, { auth: false });
expect(res.status).toBe(401);
expect(res.body.error).toBe('request_signature_required');
expect(res.body.error_description).toMatch(/create_media_buy.*signed/);
expect(res.headers['www-authenticate']).toMatch(/Signature error="request_signature_required"/);
expect(res.body.error_description).toMatch(/Signature required for create_media_buy/);
expect(res.headers['www-authenticate']).toMatch(/error="request_signature_required"/);
});

it('unsigned create_media_buy on /mcp still accepted (bearer fallthrough)', async () => {
Expand Down
Loading