diff --git a/.changeset/3965-class-c-strict-route.md b/.changeset/3965-class-c-strict-route.md new file mode 100644 index 0000000000..076ebd60a6 --- /dev/null +++ b/.changeset/3965-class-c-strict-route.md @@ -0,0 +1,30 @@ +--- +--- + +fix(training-agent): restore per-tenant //mcp-strict, close #3965 Class C + +The 6.0.0 multi-tenant migration (#3713) dropped the legacy `/mcp-strict` route along with `STRICT_REQUIRED_FOR` enforcement and `enforceSigningWhenWebhookAuthPresent`. The `signed_requests` conformance storyboard — gated on `request_signing.supported: true` AND `required_for: [...]` — failed at discovery on every tenant since then. The runner targets `//mcp-strict` (per `run-storyboards.ts:350-352`) but the multi-tenant migration only mounted `//mcp`. + +Restoring per-tenant strict routes as a sibling mount: same v5 monolith handler as the legacy `/mcp` route, but stamped with `ctx.strict = true` so `selectSigningCapability(ctx)` advertises `STRICT_REQUIRED_FOR` instead of the lax sandbox capability. Request-signing is a transport-layer property, not specialism-specific, so the strict route doesn't need v6 platform dispatch — one handler shared across all six tenants. The default `//mcp` continues to serve the v6 framework with sandbox signing (presence-gated, no required_for enforcement). + +Strict authenticator composition rebuilds two enforcement gates the migration dropped: + +- `requireSignatureWhenPresent` with `requiredFor: STRICT_REQUIRED_FOR` + `mcpToolNameResolver` so unsigned `create_media_buy` calls surface `request_signature_required` (vector 001) instead of admitting bearer. +- `enforceSigningWhenWebhookAuthPresent` wrapper so unsigned webhook-registration carrying `push_notification_config.authentication` fires the same error code (vector 027) — bearer-bypass is the exact downgrade this rule prevents. + +Each route owns its own `InMemoryReplayStore` (#3338) — sharing one store lets a nonce consumed on `/mcp` falsely fire `request_signature_replayed` on `/mcp-strict`. + +Floors ratchet to capture the conformance lift: + +| Tenant | Old (post-#4052) | New (with strict) | Delta | +|-------------------|------------------|-------------------|-------| +| /signals | 65 / 23 | 66 / 54 | +1 / +31 | +| /sales | 64 / 217 | 64 / 248 | flat / +31 | +| /governance | 65 / 70 | 65 / 101 | flat / +31 | +| /creative | 66 / 83 | 66 / 114 | flat / +31 | +| /creative-builder | 60 / 65 | 60 / 96 | flat / +31 | +| /brand | 66 / 14 | 66 / 45 | flat / +31 | + +Files: `server/src/training-agent/index.ts` (strict authenticator + per-tenant mount), `.github/workflows/training-agent-storyboards.yml`, `scripts/run-storyboards-matrix.sh`. + +Closes #3965 Class C. The full #3965 cluster is now closed: A (6.9.0 / adcp-client#1455), B (in-tree), C (this PR), D (in-tree), E (6.9.0 / adcp-client#1477), F (in-tree), G (phantom). diff --git a/.github/workflows/training-agent-storyboards.yml b/.github/workflows/training-agent-storyboards.yml index a00a19714d..b41cccf0bf 100644 --- a/.github/workflows/training-agent-storyboards.yml +++ b/.github/workflows/training-agent-storyboards.yml @@ -36,31 +36,36 @@ jobs: # SDK 6.0.0). Rising is fine; regressing fails CI. tenant: [signals, sales, governance, creative, creative-builder, brand] include: - # SDK 6.9.0 bump closed #3965 Class E: ComplyControllerConfig.force - # gained typed slots for create_media_buy_arm + task_completion - # (adcp-client#1472/#1477), so the directive arm now reaches the - # dispatcher instead of falling through to UNKNOWN_SCENARIO. 6.9.0 - # also added a strict account.mode gate on comply_test_controller — - # accounts.resolve in every tenant now stamps mode: 'sandbox'. - # Floors capture the resulting clean baseline. + # #3965 Class C closed by per-tenant //mcp-strict route + # restoration. The 6.0.0 multi-tenant migration (#3713) dropped the + # legacy /mcp-strict; the signed_requests conformance storyboard + # (gated on request_signing.supported and required_for) failed at + # discovery on every tenant. Per-tenant strict mounts share one v5 + # monolith handler stamped with ctx.strict — request-signing is a + # transport-layer property, not specialism-specific. The strict + # authenticator carries STRICT_REQUIRED_FOR + mcpToolNameResolver + # and is wrapped by enforceSigningWhenWebhookAuthPresent so vectors + # 001 (request_signature_required) and 027 (webhook auth unsigned) + # both fire correctly. Floors capture the +31 step lift per tenant + # from the storyboard's positive + negative conformance vectors. - tenant: signals - min_clean_storyboards: 65 - min_passing_steps: 23 + min_clean_storyboards: 66 + min_passing_steps: 54 - tenant: sales - min_clean_storyboards: 63 - min_passing_steps: 217 - - tenant: governance min_clean_storyboards: 64 - min_passing_steps: 70 - - tenant: creative + min_passing_steps: 248 + - tenant: governance min_clean_storyboards: 65 - min_passing_steps: 83 + min_passing_steps: 101 + - tenant: creative + min_clean_storyboards: 66 + min_passing_steps: 114 - tenant: creative-builder - min_clean_storyboards: 59 - min_passing_steps: 65 + min_clean_storyboards: 60 + min_passing_steps: 96 - tenant: brand - min_clean_storyboards: 65 - min_passing_steps: 14 + min_clean_storyboards: 66 + min_passing_steps: 45 steps: - uses: actions/checkout@v6 diff --git a/scripts/run-storyboards-matrix.sh b/scripts/run-storyboards-matrix.sh index d69efa6c97..4729f5f238 100755 --- a/scripts/run-storyboards-matrix.sh +++ b/scripts/run-storyboards-matrix.sh @@ -20,12 +20,12 @@ bash "${SCRIPT_DIR}/overlay-compliance-cache.sh" || true # tenant:min_clean:min_passed — kept in sync with the matrix.include block in # .github/workflows/training-agent-storyboards.yml. TENANTS=( - "signals:65:23" - "sales:63:217" - "governance:64:70" - "creative:65:83" - "creative-builder:59:65" - "brand:65:14" + "signals:66:54" + "sales:64:248" + "governance:65:101" + "creative:66:114" + "creative-builder:60:96" + "brand:66:45" ) REGRESSED=0 diff --git a/server/src/training-agent/index.ts b/server/src/training-agent/index.ts index d1c0db55ab..8e8065ee15 100644 --- a/server/src/training-agent/index.ts +++ b/server/src/training-agent/index.ts @@ -24,6 +24,7 @@ import { extractBearerToken, respondUnauthorized, requireSignatureWhenPresent, + mcpToolNameResolver, signatureErrorCodeFromCause, AuthError, type Authenticator, @@ -38,7 +39,12 @@ import type { TrainingContext } from './types.js'; import { PUBLISHERS } from './publishers.js'; import { SIGNAL_PROVIDERS } from './signal-providers.js'; import { getPublicJwks } from './webhooks.js'; -import { buildRequestSigningAuthenticator } from './request-signing.js'; +import { + buildRequestSigningAuthenticator, + buildStrictRequestSigningAuthenticator, + enforceSigningWhenWebhookAuthPresent, + STRICT_REQUIRED_FOR, +} from './request-signing.js'; import { isWorkOSApiKeyFormat } from '../middleware/api-key-format.js'; import { PUBLIC_TEST_AGENT } from '../config/test-agent.js'; @@ -113,7 +119,10 @@ function buildBearerAuthenticator(): Authenticator | null { // Lazy so the signing authenticator builds on first auth call — // avoids reading the compliance test JWKS at module import time, which -// would break test setups that mock the compliance cache. +// would break test setups that mock the compliance cache. Each route +// owns its own InMemoryReplayStore (#3338) — sharing one store lets a +// nonce consumed on /mcp falsely fire request_signature_replayed on +// /mcp-strict, so the strict-route authenticator is built separately. let _signingAuth: Authenticator | null = null; function lazySigningAuth(): Authenticator { return (req) => { @@ -122,6 +131,14 @@ function lazySigningAuth(): Authenticator { }; } +let _strictSigningAuth: Authenticator | null = null; +function lazyStrictSigningAuth(): Authenticator { + return (req) => { + if (!_strictSigningAuth) _strictSigningAuth = buildStrictRequestSigningAuthenticator(); + return _strictSigningAuth(req); + }; +} + /** * Tenant-route authenticator: presence-gated signature composition. * Callers with no `Signature-Input` header fall through to bearer auth. @@ -133,7 +150,39 @@ function buildDefaultAuthenticator(): Authenticator | null { return requireSignatureWhenPresent(lazySigningAuth(), bearerAuth); } +/** + * Strict-route authenticator: same presence-gated composition wrapping + * the strict signing verifier, plus two enforcement gates: + * + * - `requiredFor: STRICT_REQUIRED_FOR` + `mcpToolNameResolver` so + * unsigned calls to `create_media_buy` (and other required-for ops) + * surface `request_signature_required` (vector 001) instead of + * admitting bearer. + * - `enforceSigningWhenWebhookAuthPresent` wrapper so an unsigned + * webhook-registration carrying `push_notification_config.authentication` + * fires the same `request_signature_required` error (vector 027). + * Bearer-bypass is the exact downgrade this rule prevents. + * + * Non-required ops (list tools, get_products, get_adcp_capabilities) + * still admit bearer so the grader can do setup probes without signing + * infrastructure. + */ +function buildStrictAuthenticator(): Authenticator | null { + const bearerAuth = buildBearerAuthenticator(); + if (!bearerAuth) return null; + const presenceGated = requireSignatureWhenPresent( + lazyStrictSigningAuth(), + bearerAuth, + { + requiredFor: [...STRICT_REQUIRED_FOR], + resolveOperation: mcpToolNameResolver, + }, + ); + return enforceSigningWhenWebhookAuthPresent(presenceGated); +} + const defaultAuthenticator = buildDefaultAuthenticator(); +const strictAuthenticator = buildStrictAuthenticator(); function buildRequireToken(authenticator: Authenticator | null) { return async function requireToken(req: Request, res: Response, next: NextFunction): Promise { @@ -175,6 +224,7 @@ function buildRequireToken(authenticator: Authenticator | null) { } const requireTokenDefault = buildRequireToken(defaultAuthenticator); +const requireTokenStrict = buildRequireToken(strictAuthenticator); function getBaseUrl(req: Request): string { if (process.env.BASE_URL) return process.env.BASE_URL.replace(/\/$/, ''); @@ -314,6 +364,81 @@ export function createTrainingAgentRouter(): Router { }); }); + // Per-tenant strict MCP routes — `//mcp-strict` is the + // conformance grader target for the `signed_requests` storyboard. + // Same v5 monolith handler as the legacy `/mcp` mount, but stamped + // with `ctx.strict = true` so `get_adcp_capabilities` advertises + // `request_signing.required_for: STRICT_REQUIRED_FOR` and the + // verifier rejects unsigned mutating calls with + // `request_signature_required` (vector 001). One handler shared + // across all tenants — request-signing is a transport-layer property, + // not specialism-specific, so the strict route doesn't need v6 + // platform 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; + } + } + } + + 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(() => {}); + } + } + + for (const tenantId of TENANT_IDS) { + router.options(`/${tenantId}/mcp-strict`, (_req: Request, res: Response) => { + setLegacyCORS(res); + res.status(204).end(); + }); + router.post(`/${tenantId}/mcp-strict`, mcpRateLimiter, requireTokenStrict, strictMcpHandler); + router.get(`/${tenantId}/mcp-strict`, (_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 router.get('/health', (_req: Request, res: Response) => { res.json({ status: 'healthy', service: 'training-agent' });