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) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user