diff --git a/docs/security-architecture.md b/docs/security-architecture.md index a2a3300b..c52453e8 100644 --- a/docs/security-architecture.md +++ b/docs/security-architecture.md @@ -124,7 +124,11 @@ loopback bind matters. The auth pipeline (`src/web/middleware/auth.ts`, `onRequest` hook) runs in this order: 1. **Localhost‑only exemptions** (always first): `POST /api/hook-event` and the QR - `/q/` short‑code path are exempt when `req.ip` is loopback (see §3). + `/q/` short‑code path are exempt when `req.ip` is loopback (see §3). While the + **managed tunnel is running**, the hook‑event exemption additionally requires + the per‑instance `X-Codeman-Hook-Secret` header (COD‑54); failed presentations + are rate‑limited in a **dedicated bucket** (separate from Basic‑Auth failures) + so misfiring hooks can never lock out the login path. 2. **Session cookie** check — a valid `codeman_session` cookie short‑circuits to allow. 3. **HTTP Basic** check — correct credentials short‑circuit to allow and clear @@ -165,17 +169,29 @@ protection is unchanged. with `req.ip = 127.0.0.1`**. The localhost‑only exemptions then treat those requests as local: -- `POST /api/hook-event` — auth‑exempt for loopback. Bounded impact: it is +- `POST /api/hook-event` — auth‑exempt for loopback **only while no managed tunnel + is running**. When Codeman's own tunnel is up, the exemption requires the + per‑instance shared secret (`X-Codeman-Hook-Secret`, 256‑bit hex in + `~/.codeman/hook-secret`, mode 0600, COD‑54). Local hook commands read the + secret file at execution time (`$CODEMAN_HOOK_SECRET_FILE`, exported into every + managed session), so they keep working — tunneled internet traffic can't know + it. Even without the secret the impact is bounded: the route is `HookEventSchema`‑validated and requires a valid in‑memory `sessionId`; it can drive respawn signals, SSE broadcasts, push notifications, and transcript - watching — **not** arbitrary terminal input or file reads. It is a - session‑disruption / notification‑spoofing surface, not RCE. + watching — **not** arbitrary terminal input or file reads. ⚠️ The gate keys off + the **managed** tunnel — an externally run loopback proxy (your own + `cloudflared`, `tailscale serve`) is invisible to it, so the plain loopback + exemption still applies there (prefer `tailscale serve`, which authenticates at + the tailnet layer). Hook configs regenerated since COD‑54 always present the + header, so a future release can require the secret unconditionally. - QR `/q/` — still protected by its own short‑code brute‑force limiter (10 failures / 60s against a 62⁶ space). **Mitigation:** set `CODEMAN_PASSWORD` whenever a loopback‑connecting tunnel is -up (it does not gate the hook‑event exemption, but it gates everything else and -is the documented practice). Prefer `tailscale serve` (below), which authenticates +up — it gates everything except the (secret‑gated) hook exemption and is the +documented practice; since COD‑55 enabling the managed tunnel **refuses** to start +without it unless `CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK=1` explicitly +acknowledges the exposure. Prefer `tailscale serve` (below), which authenticates at the tailnet layer so untrusted clients never reach the loopback port at all. ### Host‑header & Origin allowlist (DNS‑rebinding & CSRF defense) diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 29f6df11..f1d8bb48 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -3,8 +3,9 @@ * * Generates `.claude/settings.local.json` with hook definitions that POST * to Codeman's `/api/hook-event` endpoint when Claude Code fires hooks. - * Uses `$CODEMAN_API_URL` and `$CODEMAN_SESSION_ID` env vars (set on every - * managed session) so the config is static per case directory. + * Uses `$CODEMAN_API_URL`, `$CODEMAN_SESSION_ID`, and `$CODEMAN_HOOK_SECRET_FILE` + * env vars (set on every managed session) so the config is static per case + * directory and free of secret values. * * Key exports: * - `generateHooksConfig()` — returns hooks object for settings.local.json @@ -41,11 +42,18 @@ import { HOOK_TIMEOUT_MS } from './config/auth-config.js'; export function generateHooksConfig(): { hooks: Record } { // Read Claude Code's stdin JSON and forward it as the data field. // Falls back to empty object if stdin is unavailable or malformed. + // COD-54: present the per-instance hook secret so the bypass keeps working while + // a tunnel is running. The value is read from the secret file AT EXECUTION TIME + // (path via $CODEMAN_HOOK_SECRET_FILE, set in every managed session's env), so it + // never lands in this config and rotation needs no respawn. If the var/file is + // missing the header is empty — the middleware then allows the request only on + // the plain loopback bypass (tunnel down), same as pre-secret behavior. const curlCmd = (event: HookEventType) => `HOOK_DATA=$(cat 2>/dev/null || echo '{}'); ` + `printf '{"event":"${event}","sessionId":"%s","data":%s}' "$CODEMAN_SESSION_ID" "$HOOK_DATA" | ` + `curl -s -X POST "$CODEMAN_API_URL/api/hook-event" ` + `-H 'Content-Type: application/json' ` + + `-H "X-Codeman-Hook-Secret: $(cat "$CODEMAN_HOOK_SECRET_FILE" 2>/dev/null)" ` + `--data @- ` + `2>/dev/null || true`; diff --git a/src/session-cli-builder.ts b/src/session-cli-builder.ts index e39c9e95..ad6ed85f 100644 --- a/src/session-cli-builder.ts +++ b/src/session-cli-builder.ts @@ -11,6 +11,7 @@ import type { ClaudeMode, EffortLevel } from './types.js'; import { isEffortLevel } from './types.js'; import { getAugmentedPath } from './utils/index.js'; +import { dataPath } from './config/instance.js'; /** * Build Claude CLI permission flags based on the configured mode. @@ -113,6 +114,8 @@ export function buildClaudeEnv(sessionId: string): Record | null; authFailures: StaleExpirationMap | null; qrAuthFailures: StaleExpirationMap | null; + hookSecretFailures: StaleExpirationMap | null; } /** @@ -53,6 +54,7 @@ export function registerAuthMiddleware( authSessions: null, authFailures: null, qrAuthFailures: null, + hookSecretFailures: null, }; const authPassword = process.env.CODEMAN_PASSWORD; @@ -79,11 +81,26 @@ export function registerAuthMiddleware( refreshOnGet: false, }); + // Separate hook-secret failure counter (COD-54). MUST NOT share authFailures: + // legacy (pre-secret) hook configs fire constantly from 127.0.0.1, and counting + // their 401s against the shared bucket would 429 every cookie-less request from + // loopback — locking out the Basic-Auth login path (and, through a tunnel, every + // client, since tunneled traffic also arrives as 127.0.0.1). + state.hookSecretFailures = new StaleExpirationMap({ + ttlMs: AUTH_FAILURE_WINDOW_MS, + refreshOnGet: false, + }); + const authSessions = state.authSessions; const authFailures = state.authFailures; + const hookSecretFailures = state.hookSecretFailures; - function sendAuthRateLimit(reply: FastifyReply, clientIp: string): void { - const remainingMs = authFailures.getRemainingTtl(clientIp) ?? AUTH_FAILURE_WINDOW_MS; + function sendAuthRateLimit( + reply: FastifyReply, + clientIp: string, + failures: StaleExpirationMap = authFailures + ): void { + const remainingMs = failures.getRemainingTtl(clientIp) ?? AUTH_FAILURE_WINDOW_MS; const retryAfterSeconds = Math.max(1, Math.ceil(remainingMs / 1000)); reply.header('Retry-After', String(retryAfterSeconds)); reply.code(429).send('Too Many Requests — try again later'); @@ -118,15 +135,15 @@ export function registerAuthMiddleware( done(); return; } - // Wrong/absent secret while tunneled — treat as a failed auth attempt so the - // per-IP rate limiter (below) throttles brute-force/abuse of this route. + // Wrong/absent secret while tunneled — rate-limit per IP in the DEDICATED + // hook bucket (never authFailures, which would lock out the login path). const hookIp = req.ip; - const hookFailures = authFailures.get(hookIp) ?? 0; + const hookFailures = hookSecretFailures.get(hookIp) ?? 0; if (hookFailures >= AUTH_FAILURE_MAX) { - sendAuthRateLimit(reply, hookIp); + sendAuthRateLimit(reply, hookIp, hookSecretFailures); return; } - authFailures.set(hookIp, hookFailures + 1); + hookSecretFailures.set(hookIp, hookFailures + 1); reply.code(401).send('Unauthorized: hook secret required'); return; } diff --git a/src/web/server.ts b/src/web/server.ts index fa46cc3b..d316e447 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -41,6 +41,7 @@ import fs from 'node:fs/promises'; import { execSync } from 'node:child_process'; import { hostname as getHostname } from 'node:os'; import { dataPath } from '../config/instance.js'; +import { getHookSecret } from '../config/hook-secret.js'; import { EventEmitter } from 'node:events'; import { Session, isExternalCliMode, type BackgroundTask } from '../session.js'; import type { ClaudeMode, SessionState } from '../types.js'; @@ -253,6 +254,7 @@ export class WebServer extends EventEmitter { private authSessions: StaleExpirationMap | null = null; private authFailures: StaleExpirationMap | null = null; private qrAuthFailures: StaleExpirationMap | null = null; + private hookSecretFailures: StaleExpirationMap | null = null; private pushStore: PushSubscriptionStore = new PushSubscriptionStore(); private teamWatcher: TeamWatcher = new TeamWatcher(); private _orchestratorLoop: import('../orchestrator-loop.js').OrchestratorLoop | null = null; @@ -608,6 +610,7 @@ export class WebServer extends EventEmitter { this.authSessions = authState.authSessions; this.authFailures = authState.authFailures; this.qrAuthFailures = authState.qrAuthFailures; + this.hookSecretFailures = authState.hookSecretFailures; } // WebSocket support (terminal I/O — low-latency bidirectional channel) @@ -1816,6 +1819,10 @@ export class WebServer extends EventEmitter { this.host === '0.0.0.0' || this.host === 'localhost' || this.host === '::1' ? '127.0.0.1' : this.host; process.env.CODEMAN_API_URL = `${protocol}://${apiHost}:${this.port}`; + // Ensure the COD-54 hook secret exists on disk before any session exports + // $CODEMAN_HOOK_SECRET_FILE — hook curls cat that path at execution time. + getHookSecret(); + // Start scheduled runs cleanup timer this.cleanup.setInterval( () => { @@ -2292,6 +2299,10 @@ export class WebServer extends EventEmitter { this.qrAuthFailures.dispose(); this.qrAuthFailures = null; } + if (this.hookSecretFailures) { + this.hookSecretFailures.dispose(); + this.hookSecretFailures = null; + } this.activePlanOrchestrators.clear(); this.cleaningUp.clear(); diff --git a/test/cod54-hook-event-auth.test.ts b/test/cod54-hook-event-auth.test.ts index b3f2ad55..ea2f64bf 100644 --- a/test/cod54-hook-event-auth.test.ts +++ b/test/cod54-hook-event-auth.test.ts @@ -147,4 +147,40 @@ describe('COD-54 hook-event auth — rate limiting', () => { } expect(saw429).toBe(true); }); + + it('hook-secret failures do NOT lock out the Basic-Auth login path (separate bucket)', async () => { + // The previous test exhausted the hook bucket for 127.0.0.1. Legacy (pre-secret) + // hooks fire constantly, so if they shared authFailures, every cookie-less + // request from loopback would now 429 — locking out login (and, via a tunnel, + // every client). Assert the login path is unaffected: + // 1. A credential-less request still gets a 401 challenge, NOT 429. + const unauthed = await fetch(`${baseUrl}/api/status`); + expect(unauthed.status).toBe(401); + // 2. Correct Basic credentials still authenticate. + const authed = await fetch(`${baseUrl}/api/status`, { + headers: { Authorization: 'Basic ' + Buffer.from(`${TEST_USER}:${TEST_PASS}`).toString('base64') }, + }); + expect(authed.status).toBe(200); + }); +}); + +describe('COD-54 secret delivery — generated hooks + session env present the secret', () => { + it('generated hook curl commands send the secret header, read from the file at exec time', async () => { + const { generateHooksConfig } = await import('../src/hooks-config.js'); + const config = generateHooksConfig(); + const commands = JSON.stringify(config); + // Header present, value sourced from $CODEMAN_HOOK_SECRET_FILE (not embedded). + expect(commands).toContain(HOOK_SECRET_HEADER); + expect(commands).toContain('$CODEMAN_HOOK_SECRET_FILE'); + expect(commands).not.toContain(getHookSecret()); + }); + + it('session env builders export CODEMAN_HOOK_SECRET_FILE (path only, never the value)', async () => { + const { buildClaudeEnv, buildShellEnv } = await import('../src/session-cli-builder.js'); + const claudeEnv = buildClaudeEnv('test-session'); + const shellEnv = buildShellEnv('test-session'); + expect(claudeEnv.CODEMAN_HOOK_SECRET_FILE).toMatch(/hook-secret$/); + expect(shellEnv.CODEMAN_HOOK_SECRET_FILE).toMatch(/hook-secret$/); + expect(JSON.stringify(claudeEnv)).not.toContain(getHookSecret()); + }); });