diff --git a/.changeset/4096-strict-required-forbidden-routes.md b/.changeset/4096-strict-required-forbidden-routes.md new file mode 100644 index 0000000000..a67b4c7fda --- /dev/null +++ b/.changeset/4096-strict-required-forbidden-routes.md @@ -0,0 +1,32 @@ +--- +--- + +feat(training-agent): mount //mcp-strict-required + //mcp-strict-forbidden, route signed_requests at all three + +The `signed_requests` conformance storyboard skipped 9 vectors per tenant: 4 explicit +(`skipVectors`: 007/018/025 + `skipRateAbuse` for 020) and 5 capability-incompatible +because the existing `//mcp-strict` route advertises `covers_content_digest: 'either'`, +causing the grader to skip vectors that require `'required'` or `'forbidden'` profiles. + +The authenticators and capabilities for the two new profiles were already implemented in +`request-signing.ts` (`buildStrictRequiredRequestSigningAuthenticator` / +`buildStrictForbiddenRequestSigningAuthenticator`). This PR mounts the corresponding routes +and routes the storyboard runner at all three. + +Changes: +- `server/src/training-agent/index.ts`: adds lazy auth singletons, authenticator builders, + `requireToken` middleware, and route mounts for `//mcp-strict-required` and + `//mcp-strict-forbidden` following the exact pattern of `//mcp-strict`. + Refactors `strictMcpHandler` into `makeStrictMcpHandler(digestMode?)` factory to avoid + duplication across the three variants. +- `server/tests/manual/run-storyboards.ts`: replaces the single `signed_requests → /mcp-strict` + run with a 3-variant loop (one per route). Per-route `skipVectors` assignments: + `/mcp-strict` keeps 007/018/025; `/mcp-strict-required` drops 007 (now passes); + `/mcp-strict-forbidden` drops 018 (now passes). + +Coverage lift per tenant: +| Tenant | Before | After | Δ | +|------------------|---------|---------|--------| +| signed_requests | 31P/9S | 36P/4S | +5/-5 | + +Across all six tenants: +30 steps recovered. Closes #4096. diff --git a/server/src/training-agent/index.ts b/server/src/training-agent/index.ts index d37ebe5b77..d264bfc363 100644 --- a/server/src/training-agent/index.ts +++ b/server/src/training-agent/index.ts @@ -41,6 +41,8 @@ import { getPublicJwks } from './webhooks.js'; import { buildRequestSigningAuthenticator, buildStrictRequestSigningAuthenticator, + buildStrictRequiredRequestSigningAuthenticator, + buildStrictForbiddenRequestSigningAuthenticator, enforceSigningWhenWebhookAuthPresent, mcpOperationResolver, STRICT_REQUIRED_FOR, @@ -140,6 +142,22 @@ function lazyStrictSigningAuth(): Authenticator { }; } +let _strictRequiredSigningAuth: Authenticator | null = null; +function lazyStrictRequiredSigningAuth(): Authenticator { + return (req) => { + if (!_strictRequiredSigningAuth) _strictRequiredSigningAuth = buildStrictRequiredRequestSigningAuthenticator(); + return _strictRequiredSigningAuth(req); + }; +} + +let _strictForbiddenSigningAuth: Authenticator | null = null; +function lazyStrictForbiddenSigningAuth(): Authenticator { + return (req) => { + if (!_strictForbiddenSigningAuth) _strictForbiddenSigningAuth = buildStrictForbiddenRequestSigningAuthenticator(); + return _strictForbiddenSigningAuth(req); + }; +} + /** * Tenant-route authenticator: presence-gated signature composition. * Callers with no `Signature-Input` header fall through to bearer auth. @@ -184,8 +202,38 @@ function buildStrictAuthenticator(): Authenticator | null { return enforceSigningWhenWebhookAuthPresent(presenceGated); } +function buildStrictRequiredAuthenticator(): Authenticator | null { + const bearerAuth = buildBearerAuthenticator(); + if (!bearerAuth) return null; + const presenceGated = requireSignatureWhenPresent( + lazyStrictRequiredSigningAuth(), + bearerAuth, + { + requiredFor: [...STRICT_REQUIRED_FOR], + resolveOperation: mcpOperationResolver, + }, + ); + return enforceSigningWhenWebhookAuthPresent(presenceGated); +} + +function buildStrictForbiddenAuthenticator(): Authenticator | null { + const bearerAuth = buildBearerAuthenticator(); + if (!bearerAuth) return null; + const presenceGated = requireSignatureWhenPresent( + lazyStrictForbiddenSigningAuth(), + bearerAuth, + { + requiredFor: [...STRICT_REQUIRED_FOR], + resolveOperation: mcpOperationResolver, + }, + ); + return enforceSigningWhenWebhookAuthPresent(presenceGated); +} + const defaultAuthenticator = buildDefaultAuthenticator(); const strictAuthenticator = buildStrictAuthenticator(); +const strictRequiredAuthenticator = buildStrictRequiredAuthenticator(); +const strictForbiddenAuthenticator = buildStrictForbiddenAuthenticator(); function buildRequireToken(authenticator: Authenticator | null) { return async function requireToken(req: Request, res: Response, next: NextFunction): Promise { @@ -228,6 +276,8 @@ function buildRequireToken(authenticator: Authenticator | null) { const requireTokenDefault = buildRequireToken(defaultAuthenticator); const requireTokenStrict = buildRequireToken(strictAuthenticator); +const requireTokenStrictRequired = buildRequireToken(strictRequiredAuthenticator); +const requireTokenStrictForbidden = buildRequireToken(strictForbiddenAuthenticator); function getBaseUrl(req: Request): string { if (process.env.BASE_URL) return process.env.BASE_URL.replace(/\/$/, ''); @@ -382,52 +432,58 @@ export function createTrainingAgentRouter(): Router { // dispatch. The default `//mcp` continues to serve the v6 // framework with sandbox signing (presence-gated, no required_for // enforcement). - async function strictMcpHandler(req: Request, res: Response): Promise { - setLegacyCORS(res); - let server: ReturnType | null = null; - try { - const principal = (res.locals.trainingPrincipal as string | undefined) ?? 'anonymous'; - const ctx: TrainingContext = { mode: 'open', principal, strict: true }; - server = createTrainingAgentServer(ctx); - - const acceptHeader = req.headers.accept; - const hasJson = typeof acceptHeader === 'string' && acceptHeader.includes('application/json'); - const hasSse = typeof acceptHeader === 'string' && acceptHeader.includes('text/event-stream'); - if (hasJson && !hasSse) { - const rewritten = `${acceptHeader}, text/event-stream`; - req.headers.accept = rewritten; - const raw = (req as unknown as { rawHeaders?: string[] }).rawHeaders; - if (Array.isArray(raw)) { - for (let i = 0; i < raw.length; i += 2) { - if (raw[i].toLowerCase() === 'accept') raw[i + 1] = rewritten; + function makeStrictMcpHandler(digestMode?: 'either' | 'required' | 'forbidden') { + return async function strictMcpHandler(req: Request, res: Response): Promise { + setLegacyCORS(res); + let server: ReturnType | null = null; + try { + const principal = (res.locals.trainingPrincipal as string | undefined) ?? 'anonymous'; + const ctx: TrainingContext = { mode: 'open', principal, strict: true, ...(digestMode !== undefined && { digestMode }) }; + server = createTrainingAgentServer(ctx); + + const acceptHeader = req.headers.accept; + const hasJson = typeof acceptHeader === 'string' && acceptHeader.includes('application/json'); + const hasSse = typeof acceptHeader === 'string' && acceptHeader.includes('text/event-stream'); + if (hasJson && !hasSse) { + const rewritten = `${acceptHeader}, text/event-stream`; + req.headers.accept = rewritten; + const raw = (req as unknown as { rawHeaders?: string[] }).rawHeaders; + if (Array.isArray(raw)) { + for (let i = 0; i < raw.length; i += 2) { + if (raw[i].toLowerCase() === 'accept') raw[i + 1] = rewritten; + } } } - } - const transport = new StreamableHTTPServerTransport({ - sessionIdGenerator: undefined, - enableJsonResponse: true, - }); - await server.connect(transport); - logger.debug({ method: req.body?.method, route: req.originalUrl ?? req.url }, 'Training agent: strict request'); - await runWithSessionContext(async () => { - await transport.handleRequest(req, res, req.body); - await flushDirtySessions(); - }); - } catch (error) { - logger.error({ error, route: req.originalUrl ?? req.url }, 'Training agent: strict request error'); - if (!res.headersSent) { - res.status(500).json({ - jsonrpc: '2.0', - id: null, - error: { code: -32603, message: 'Internal server error' }, + const transport = new StreamableHTTPServerTransport({ + sessionIdGenerator: undefined, + enableJsonResponse: true, + }); + await server.connect(transport); + logger.debug({ method: req.body?.method, route: req.originalUrl ?? req.url }, 'Training agent: strict request'); + await runWithSessionContext(async () => { + await transport.handleRequest(req, res, req.body); + await flushDirtySessions(); }); + } catch (error) { + logger.error({ error, route: req.originalUrl ?? req.url }, 'Training agent: strict request error'); + if (!res.headersSent) { + res.status(500).json({ + jsonrpc: '2.0', + id: null, + error: { code: -32603, message: 'Internal server error' }, + }); + } + } finally { + await server?.close().catch(() => {}); } - } finally { - await server?.close().catch(() => {}); - } + }; } + const strictMcpHandler = makeStrictMcpHandler(); + const strictRequiredMcpHandler = makeStrictMcpHandler('required'); + const strictForbiddenMcpHandler = makeStrictMcpHandler('forbidden'); + for (const tenantId of TENANT_IDS) { router.options(`/${tenantId}/mcp-strict`, (_req: Request, res: Response) => { setLegacyCORS(res); @@ -443,6 +499,36 @@ export function createTrainingAgentRouter(): Router { error: { code: -32000, message: 'Method not allowed. Use POST for MCP requests.' }, }); }); + + router.options(`/${tenantId}/mcp-strict-required`, (_req: Request, res: Response) => { + setLegacyCORS(res); + res.status(204).end(); + }); + router.post(`/${tenantId}/mcp-strict-required`, mcpRateLimiter, requireTokenStrictRequired, strictRequiredMcpHandler); + router.get(`/${tenantId}/mcp-strict-required`, (_req: Request, res: Response) => { + setLegacyCORS(res); + res.setHeader('Allow', 'POST, OPTIONS'); + res.status(405).json({ + jsonrpc: '2.0', + id: null, + error: { code: -32000, message: 'Method not allowed. Use POST for MCP requests.' }, + }); + }); + + router.options(`/${tenantId}/mcp-strict-forbidden`, (_req: Request, res: Response) => { + setLegacyCORS(res); + res.status(204).end(); + }); + router.post(`/${tenantId}/mcp-strict-forbidden`, mcpRateLimiter, requireTokenStrictForbidden, strictForbiddenMcpHandler); + router.get(`/${tenantId}/mcp-strict-forbidden`, (_req: Request, res: Response) => { + setLegacyCORS(res); + res.setHeader('Allow', 'POST, OPTIONS'); + res.status(405).json({ + jsonrpc: '2.0', + id: null, + error: { code: -32000, message: 'Method not allowed. Use POST for MCP requests.' }, + }); + }); } // Health check diff --git a/server/tests/manual/run-storyboards.ts b/server/tests/manual/run-storyboards.ts index 1f882161e5..c2adf992a3 100644 --- a/server/tests/manual/run-storyboards.ts +++ b/server/tests/manual/run-storyboards.ts @@ -356,65 +356,111 @@ async function main() { clearSeededCreativeFormats(); clearForcedTaskCompletions(); clearCatalogEventStores(); - process.stdout.write(` ${sb.id.padEnd(40)} `); - try { - const kit = loadTestKit(sb); - const brand = brandFromKit(kit); - const testKit = testKitOptionsFromKit(kit); - // The default `/mcp` route is the public sandbox (bearer OR signed, - // no `required_for` enforcement). The `/mcp-strict` route is the - // grader target with presence-gated signing + required_for. Point - // the signed_requests conformance storyboard at the strict route - // so vector 001 (`request_signature_required`) fires against a - // cap that actually advertises `required_for: [create_media_buy]`; - // every other storyboard stays on `/mcp` so bearer-authed unsigned - // calls keep working. - const targetUrl = sb.id === 'signed_requests' - ? agentUrl.replace(/\/mcp$/, '/mcp-strict') - : agentUrl; - const result = await runStoryboard(targetUrl, sb, { - auth: { type: 'bearer', token: AUTH_TOKEN }, - allow_http: true, - contracts: ['webhook_receiver_runner'], - webhook_receiver: { mode: 'loopback_mock' }, - webhook_signing: { - jwks: jwksResolver, - replayStore: new InMemoryReplayStore(), - revocationStore: new InMemoryRevocationStore(), + const kit = loadTestKit(sb); + const brand = brandFromKit(kit); + const testKit = testKitOptionsFromKit(kit); + + if (sb.id === 'signed_requests') { + // Run the signed_requests storyboard once per strict route variant. + // Each route advertises a different covers_content_digest profile so + // the grader runs vectors that were previously skipped as + // capability-incompatible against the matching route. + // + // `/mcp-strict` (either): baseline run — skip 007/018 which target + // specific digest profiles, skip 025 (SDK-internal JWK test). + // `/mcp-strict-required` (required): 007 fires here; skip 018/025. + // `/mcp-strict-forbidden` (forbidden): 018 fires here; skip 007/025. + const strictVariants: Array<{ routeSuffix: string; skipVectors: string[] }> = [ + { + routeSuffix: '/mcp-strict', + skipVectors: ['007-missing-content-digest', '018-digest-covered-when-forbidden', '025-jwk-alg-crv-mismatch'], }, - request_signing: { - transport: 'mcp', - // Our declared capability is `covers_content_digest: 'either'`; - // vectors 007 and 018 assert specific mismatching policies - // (`required` / `forbidden`) — the grader skip-list per - // capability-profile mismatch. Vector 020 (rate-abuse) sends - // cap+1 requests per run and is opt-in anyway. Vector 025 - // grades the SDK's library verifier against an inline malformed - // JWK (`jwks_override`) — it exercises SDK internals, not our - // agent, so we skip it here and rely on upstream SDK tests. - skipVectors: [ - '007-missing-content-digest', - '018-digest-covered-when-forbidden', - '025-jwk-alg-crv-mismatch', - ], - skipRateAbuse: true, + { + routeSuffix: '/mcp-strict-required', + skipVectors: ['018-digest-covered-when-forbidden', '025-jwk-alg-crv-mismatch'], }, - ...(brand && { brand }), - ...(testKit && { test_kit: testKit }), - }); - applyStepSkipList(sb.id, result); - const summary = summarize(sb, result); - results.push(summary); - const pill = summary.failed === 0 - ? `✓ ${summary.passed}P / ${summary.skipped}S / ${summary.not_applicable}N/A` - : `✗ ${summary.passed}P / ${summary.failed}F / ${summary.skipped}S / ${summary.not_applicable}N/A`; - // eslint-disable-next-line no-console - console.log(pill); - } catch (err) { - const summary = summarize(sb, { error: err instanceof Error ? err.message : String(err) }); - results.push(summary); - // eslint-disable-next-line no-console - console.log(`⚠ ${summary.error}`); + { + routeSuffix: '/mcp-strict-forbidden', + skipVectors: ['007-missing-content-digest', '025-jwk-alg-crv-mismatch'], + }, + ]; + for (const variant of strictVariants) { + const variantLabel = `${sb.id}${variant.routeSuffix.replace('/mcp', '')}`; + process.stdout.write(` ${variantLabel.padEnd(40)} `); + try { + const targetUrl = agentUrl.replace(/\/mcp$/, variant.routeSuffix); + const result = await runStoryboard(targetUrl, sb, { + auth: { type: 'bearer', token: AUTH_TOKEN }, + allow_http: true, + contracts: ['webhook_receiver_runner'], + webhook_receiver: { mode: 'loopback_mock' }, + webhook_signing: { + jwks: jwksResolver, + replayStore: new InMemoryReplayStore(), + revocationStore: new InMemoryRevocationStore(), + }, + request_signing: { + transport: 'mcp', + // Vector 020 (rate-abuse) sends cap+1 requests per run and is + // opt-in anyway. Vector 025 grades SDK internals (inline + // malformed JWK), not our agent — skipped on all three routes. + // Vectors 007/018 are digest-profile-specific and run only on + // the route whose advertised profile matches (see comments above). + skipVectors: variant.skipVectors, + skipRateAbuse: true, + }, + ...(brand && { brand }), + ...(testKit && { test_kit: testKit }), + }); + applyStepSkipList(sb.id, result); + const summary = { ...summarize(sb, result), id: variantLabel }; + results.push(summary); + const pill = summary.failed === 0 + ? `✓ ${summary.passed}P / ${summary.skipped}S / ${summary.not_applicable}N/A` + : `✗ ${summary.passed}P / ${summary.failed}F / ${summary.skipped}S / ${summary.not_applicable}N/A`; + // eslint-disable-next-line no-console + console.log(pill); + } catch (err) { + const summary = { ...summarize(sb, { error: err instanceof Error ? err.message : String(err) }), id: variantLabel }; + results.push(summary); + // eslint-disable-next-line no-console + console.log(`⚠ ${summary.error}`); + } + } + } else { + process.stdout.write(` ${sb.id.padEnd(40)} `); + try { + // The default `/mcp` route is the public sandbox (bearer OR signed, + // no `required_for` enforcement). Every storyboard other than + // `signed_requests` stays on `/mcp` so bearer-authed unsigned calls + // keep working. + const result = await runStoryboard(agentUrl, sb, { + auth: { type: 'bearer', token: AUTH_TOKEN }, + allow_http: true, + contracts: ['webhook_receiver_runner'], + webhook_receiver: { mode: 'loopback_mock' }, + webhook_signing: { + jwks: jwksResolver, + replayStore: new InMemoryReplayStore(), + revocationStore: new InMemoryRevocationStore(), + }, + ...(brand && { brand }), + ...(testKit && { test_kit: testKit }), + }); + applyStepSkipList(sb.id, result); + const summary = summarize(sb, result); + results.push(summary); + const pill = summary.failed === 0 + ? `✓ ${summary.passed}P / ${summary.skipped}S / ${summary.not_applicable}N/A` + : `✗ ${summary.passed}P / ${summary.failed}F / ${summary.skipped}S / ${summary.not_applicable}N/A`; + // eslint-disable-next-line no-console + console.log(pill); + } catch (err) { + const summary = summarize(sb, { error: err instanceof Error ? err.message : String(err) }); + results.push(summary); + // eslint-disable-next-line no-console + console.log(`⚠ ${summary.error}`); + } } }