From d42ca8dc575b30ce261d39dadb7aeef9be885f90 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 04:19:52 +0300 Subject: [PATCH] test(auth): cover collab-token api-key arm-seam; docs + dead-code cleanup (#501) Hardening follow-ups from PR #526 review (no defect fixes): - DO1: add auth.controller.spec tests for the collab-token handler, the arm-seam of the anti-laundering defense. Assert getCollabToken is invoked with { apiKeyId } only when the SIGNED req.raw marks an api_key principal, and undefined otherwise (session, missing apiKeyId, or a spoofed body). Verified non-vacuous: nulling the ternary reddens the ARMED test. - DO2: document the API_KEYS_ENABLED kill-switch in .env.example next to the other feature flags (default ON; strict true/false; OFF denies api-key auth and 404s the management endpoints). - DO3: remove dead bindAccessJwtVerifier (+ its now-orphaned AccessJwtVerifier interface and its dedicated spec block); prod switched to bindMcpBearerVerifier. verifyBearerAccess is retained (still used). Co-Authored-By: Claude Opus 4.8 (1M context) --- .env.example | 10 +++ .../src/core/auth/auth.controller.spec.ts | 68 +++++++++++++++++++ .../src/integrations/mcp/mcp-auth.helpers.ts | 39 +---------- .../src/integrations/mcp/mcp.service.spec.ts | 46 ------------- 4 files changed, 81 insertions(+), 82 deletions(-) diff --git a/.env.example b/.env.example index 6d7dec43..6b06ffe4 100644 --- a/.env.example +++ b/.env.example @@ -251,6 +251,16 @@ MCP_DOCMOST_PASSWORD= # Default 120000 (2 min). # AI_MCP_CALL_TIMEOUT_MS=120000 +# Kill-switch for the agent API-key feature (#501). Default ON when unset — a +# deploy that never sets it must NOT silently kill every agent. STRICT parse: +# only the literals `true` / `false` are accepted; a typo like `=0`/`=off`/`=False` +# FAILS AT BOOT by design (never silently read as "enabled"), so the switch is +# guaranteed to actually flip when an operator flips it during an incident. When +# set to `false`: all api-key auth is DENIED (every api-key token is rejected) and +# the api-key management endpoints return 404. The resolved state is logged at boot +# (`API keys: ENABLED/DISABLED (API_KEYS_ENABLED=...)`) so it is verifiable per deploy. +# API_KEYS_ENABLED=true + # Max JSON/urlencoded request body size (bytes). Fastify's 1 MiB default is too # small for a long AI-chat research turn: the client resends the FULL message # history (every tool call + search result) on each turn, so a deep conversation's diff --git a/apps/server/src/core/auth/auth.controller.spec.ts b/apps/server/src/core/auth/auth.controller.spec.ts index 4746f249..428e538e 100644 --- a/apps/server/src/core/auth/auth.controller.spec.ts +++ b/apps/server/src/core/auth/auth.controller.spec.ts @@ -20,3 +20,71 @@ describe('AuthController', () => { expect(controller).toBeDefined(); }); }); + +// The collab-token handler is the ARM-SEAM of the #501 anti-laundering defense: +// it derives the api-key origin args ({ apiKeyId }) from the SIGNED-derived +// `req.raw` fields (stamped by jwt.strategy) and threads them into the collab +// token mint, forcing principal='api_key' so a later key revoke rejects NEW +// collab connections. A regression here (reading `body`, dropping `apiKeyId`, +// or inverting the ternary) would silently mint api-key requests as +// principal='session' and reopen the laundering hole with a green suite. These +// tests pin the EXACT args the controller passes to authService.getCollabToken. +describe('AuthController.collabToken arms the api-key origin (#501)', () => { + let controller: AuthController; + let getCollabToken: jest.Mock; + + const user = { id: 'u-1', workspaceId: 'ws-1' } as any; + const workspace = { id: 'ws-1' } as any; + + beforeEach(() => { + getCollabToken = jest.fn().mockResolvedValue({ token: 'ct' }); + controller = new AuthController( + { getCollabToken } as any, // authService + {} as any, // sessionService + {} as any, // environmentService + {} as any, // moduleRef + {} as any, // auditService + ); + }); + + it('threads { apiKeyId } when req.raw marks an api_key principal (ARMED)', async () => { + const req = { raw: { authType: 'api_key', apiKeyId: 'k-1' } } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledTimes(1); + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', { + apiKeyId: 'k-1', + }); + }); + + it('passes undefined for a normal session request (NOT armed)', async () => { + const req = { raw: {} } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledTimes(1); + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); + + it('does NOT arm when api_key authType lacks an apiKeyId', async () => { + const req = { raw: { authType: 'api_key' } } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); + + it('reads the SIGNED req.raw, never a spoofable client body', async () => { + // A client-supplied body claiming api_key must be ignored: only the + // jwt.strategy-stamped req.raw can arm the api-key origin. + const req = { + raw: {}, + body: { authType: 'api_key', apiKeyId: 'attacker' }, + } as any; + + await controller.collabToken(user, workspace, req); + + expect(getCollabToken).toHaveBeenCalledWith(user, 'ws-1', undefined); + }); +}); diff --git a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts index 305c909c..e8b20880 100644 --- a/apps/server/src/integrations/mcp/mcp-auth.helpers.ts +++ b/apps/server/src/integrations/mcp/mcp-auth.helpers.ts @@ -304,39 +304,6 @@ export function clientIp(req: ClientIpRequest): string { return 'unknown'; } -// Minimal structural shape of the TokenService.verifyJwt method we depend on, -// so this module never imports the concrete TokenService (heavy graph). -export interface AccessJwtVerifier { - verifyJwt: ( - token: string, - type: JwtType, - ) => Promise<{ - sub?: string; - email?: string; - workspaceId?: string; - sessionId?: string; - }>; -} - -/** - * Bind a TokenService-like verifier into a one-arg `verifyJwt(token)` that - * ALWAYS enforces `JwtType.ACCESS`. This is the single place where the /mcp - * Bearer path pins the token type: a Bearer access token must be verified AS an - * access token (not refresh/exchange/collab/etc.), so the type literal is fixed - * here rather than at the call site. McpService.verifyMcpBearer delegates to - * this, keeping the `JwtType.ACCESS` choice testable without the heavy graph. - */ -export function bindAccessJwtVerifier( - tokenService: AccessJwtVerifier, -): (token: string) => Promise<{ - sub?: string; - email?: string; - workspaceId?: string; - sessionId?: string; -}> { - return (token: string) => tokenService.verifyJwt(token, JwtType.ACCESS); -} - // The decoded payload shared by the /mcp Bearer allowlist. Carries the `type` // discriminator and the API-key `apiKeyId`, on top of the access-token fields. export interface McpBearerPayload { @@ -358,9 +325,9 @@ export interface OneOfJwtVerifier { /** * Bind a TokenService-like verifier into a one-arg `verifyJwtOneOf(token)` that - * pins the /mcp Bearer ALLOWLIST to exactly {ACCESS, API_KEY}. This REPLACES - * `bindAccessJwtVerifier` as the single place the /mcp Bearer path pins the token - * type: the /mcp Bearer slot now legitimately accepts either an ACCESS token (a + * pins the /mcp Bearer ALLOWLIST to exactly {ACCESS, API_KEY}. This is the single + * place the /mcp Bearer path pins the token type: the /mcp Bearer slot + * legitimately accepts either an ACCESS token (a * human's session token) OR an API_KEY token (an agent's key), but NOTHING else * (collab/exchange/attachment/etc. are rejected with the generic type error). * The allowlist is fixed here rather than at the call site, and the signature is diff --git a/apps/server/src/integrations/mcp/mcp.service.spec.ts b/apps/server/src/integrations/mcp/mcp.service.spec.ts index 294cf32b..b2ee0236 100644 --- a/apps/server/src/integrations/mcp/mcp.service.spec.ts +++ b/apps/server/src/integrations/mcp/mcp.service.spec.ts @@ -10,7 +10,6 @@ import { bindMcpBearerVerifier, sharedTokenMatches, clientIp, - bindAccessJwtVerifier, extractBearer, decideBasicGate, mapAuthResultToResponse, @@ -1018,51 +1017,6 @@ describe('clientIp (XFF-fallback precedence, item 5)', () => { }); }); -describe('bindAccessJwtVerifier enforces JwtType.ACCESS (item 3)', () => { - it('calls TokenService.verifyJwt with JwtType.ACCESS as the second argument', async () => { - // Mock TokenService: assert the type literal is pinned to ACCESS so swapping - // to REFRESH (or omitting the type) breaks this test. - const verifyJwt = jest - .fn() - .mockResolvedValue({ sub: 'user-1', workspaceId: 'ws-1' }); - const verify = bindAccessJwtVerifier({ verifyJwt }); - - await verify('the.access.jwt'); - - expect(verifyJwt).toHaveBeenCalledTimes(1); - expect(verifyJwt).toHaveBeenCalledWith('the.access.jwt', JwtType.ACCESS); - // Pin the real enum value too, so renaming/repointing the enum member is caught. - expect(verifyJwt.mock.calls[0][1]).toBe('access'); - }); - - it('passes through the verified payload', async () => { - const payload = { sub: 'user-9', email: 'u@e.com', workspaceId: 'ws-1' }; - const verifyJwt = jest.fn().mockResolvedValue(payload); - await expect( - bindAccessJwtVerifier({ verifyJwt })('t'), - ).resolves.toBe(payload); - }); - - // The Bearer revocation/disabled checks (verifyBearerAccess) are covered above; - // this binds the ACCESS-type enforcement that verifyMcpBearer wires in. - it('feeds verifyBearerAccess so the whole Bearer chain enforces ACCESS', async () => { - const verifyJwt = jest.fn().mockResolvedValue({ - sub: 'user-1', - workspaceId: 'ws-1', - sessionId: 'sess-1', - }); - const res = await verifyBearerAccess('t', { - verifyJwt: bindAccessJwtVerifier({ verifyJwt }), - findUser: jest.fn().mockResolvedValue({ deactivatedAt: null }), - findActiveSession: jest - .fn() - .mockResolvedValue({ userId: 'user-1', workspaceId: 'ws-1' }), - }); - expect(verifyJwt).toHaveBeenCalledWith('t', JwtType.ACCESS); - expect(res).toEqual({ sub: 'user-1', email: undefined }); - }); -}); - describe('bindMcpBearerVerifier pins the {ACCESS, API_KEY} allowlist (#501)', () => { it('calls verifyJwtOneOf with exactly [ACCESS, API_KEY]', async () => { const verifyJwtOneOf = jest