From d4fa90cbbb2560c51e57c76ed0671588dd5e86d1 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 27 Apr 2026 07:16:28 -0400 Subject: [PATCH] fix(training-agent): isolate replay store on /mcp-strict (#3338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 from #3340), 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 training-agent-strict.test.ts:124 (was skipped per #3080 with a stale assertion); regex 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. Per-route signing-auth instances eliminate any leftover bypass surface regardless. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/fix-replay-store-3338.md | 14 ++++++++++++++ server/src/training-agent/index.ts | 18 ++++++++++++++---- server/src/training-agent/request-signing.ts | 9 +++++++++ .../integration/training-agent-strict.test.ts | 11 ++++++----- 4 files changed, 43 insertions(+), 9 deletions(-) create mode 100644 .changeset/fix-replay-store-3338.md diff --git a/.changeset/fix-replay-store-3338.md b/.changeset/fix-replay-store-3338.md new file mode 100644 index 0000000000..8648fababa --- /dev/null +++ b/.changeset/fix-replay-store-3338.md @@ -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. diff --git a/server/src/training-agent/index.ts b/server/src/training-agent/index.ts index 56ec7ff972..886e316aa4 100644 --- a/server/src/training-agent/index.ts +++ b/server/src/training-agent/index.ts @@ -33,6 +33,7 @@ import { SIGNAL_PROVIDERS } from './signal-providers.js'; import { getPublicJwks } from './webhooks.js'; import { buildRequestSigningAuthenticator, + buildStrictRequestSigningAuthenticator, buildStrictRequiredRequestSigningAuthenticator, buildStrictForbiddenRequestSigningAuthenticator, enforceSigningWhenWebhookAuthPresent, @@ -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) => { @@ -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) => { @@ -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); diff --git a/server/src/training-agent/request-signing.ts b/server/src/training-agent/request-signing.ts index c20637aac6..5aad6f8344 100644 --- a/server/src/training-agent/request-signing.ts +++ b/server/src/training-agent/request-signing.ts @@ -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()); diff --git a/server/tests/integration/training-agent-strict.test.ts b/server/tests/integration/training-agent-strict.test.ts index 01f915a9a3..1b8559c304 100644 --- a/server/tests/integration/training-agent-strict.test.ts +++ b/server/tests/integration/training-agent-strict.test.ts @@ -120,9 +120,10 @@ 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 }, @@ -130,8 +131,8 @@ describe('Training Agent /mcp-strict route', () => { }, { 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 () => {