diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 4b7adfc3..ffc8b840 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -243,6 +243,43 @@ export async function writeHooksConfig(casePath: string): Promise { }); } +/** + * Self-heal a case's hooks block so the COD-91 unconditional hook-secret gate keeps + * accepting its hook events. + * + * `writeHooksConfig` only runs when a case is first CREATED. Cases created before the + * X-Codeman-Hook-Secret header was added (COD-54, 2026-06-10) keep hook curls in their + * settings.local.json that POST to /api/hook-event WITHOUT the secret — which, once the + * gate requires it unconditionally (COD-91), silently 401 on a password-protected + * install. This refreshes the hooks block so those stale curls regain the header. + * + * Deliberately surgical: regenerates ONLY when settings.local.json already contains + * Codeman's own hook curls (they target `/api/hook-event`) that lack the secret header. + * No-op when the file/hooks are absent (we never impose hooks on a user who removed + * them), when the hooks aren't ours, or when the secret is already present — so it never + * clobbers a user's customizations and is cheap enough to call on every Claude spawn. + */ +export async function refreshStaleHookSecret(casePath: string): Promise { + const settingsPath = join(casePath, '.claude', 'settings.local.json'); + if (!existsSync(settingsPath)) return; + await withSettingsLock(settingsPath, async () => { + let existing: Record; + try { + existing = JSON.parse(await readFile(settingsPath, 'utf-8')); + } catch { + return; // malformed — leave it untouched (case-create owns the happy path) + } + const hooksJson = JSON.stringify(existing.hooks ?? null); + const isOurs = hooksJson.includes('/api/hook-event'); + // The generated curl carries this header literal (see generateHooksConfig); its + // absence on our own hooks means they predate COD-54 and need regenerating. + const hasSecret = hooksJson.includes('X-Codeman-Hook-Secret'); + if (!isOurs || hasSecret) return; + const merged = { ...existing, ...generateHooksConfig() }; + await writeFile(settingsPath, JSON.stringify(merged, null, 2) + '\n'); + }); +} + /** Unique marker identifying Codeman's own statusLine command (vs a user's). */ const STATUSLINE_MARKER = '/api/status-telemetry'; diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index 92674347..56dde1d7 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -36,20 +36,12 @@ interface AuthState { * Register HTTP Basic Auth middleware with session cookies and rate limiting. * Only active when CODEMAN_PASSWORD is set. * - * @param getTunnelRunning - returns true while a managed tunnel is active. Used - * to gate the `/api/hook-event` localhost bypass: when a tunnel is up, tunneled - * internet traffic reaches the loopback origin with `req.ip === 127.0.0.1`, so - * the bypass additionally requires the shared hook secret (COD-54). When no - * tunnel is running (loopback-only, the normal case) the plain localhost bypass - * is kept so already-deployed (pre-secret) hooks + the loop channel keep working. - * Optional; defaults to "no tunnel" (unchanged behavior) when omitted. + * The `/api/hook-event` + `/api/status-telemetry` localhost bypass requires the + * shared hook secret unconditionally (COD-91) — see the onRequest hook below. + * * @returns AuthState for lifecycle management (dispose on server stop) */ -export function registerAuthMiddleware( - app: FastifyInstance, - https: boolean, - getTunnelRunning: () => boolean = () => false -): AuthState { +export function registerAuthMiddleware(app: FastifyInstance, https: boolean): AuthState { const state: AuthState = { authSessions: null, authFailures: null, @@ -114,30 +106,26 @@ export function registerAuthMiddleware( // COD-54: the bare localhost bypass is unsafe while a tunnel is running, because // `cloudflared --url http://127.0.0.1:port` proxies internet traffic INTO the // loopback origin, so a tunneled request arrives with req.ip === 127.0.0.1 and - // would pass. So: - // - tunnel running → bypass requires the shared hook secret (local hooks present - // it via the X-Codeman-Hook-Secret header; internet traffic can't know it), - // - tunnel not running (loopback-only, the normal case) → keep the plain - // localhost bypass so already-deployed (pre-secret) hooks + the loop's own - // credential-less hook channel keep working. + // would pass. COD-91: require the shared hook secret on the loopback bypass + // UNCONDITIONALLY (not just while the managed tunnel is up). Codeman can't detect + // a user's own loopback reverse proxy (their own `cloudflared --url`, `tailscale + // serve`, nginx → 127.0.0.1), so tunnel-gating left that path with the unsafe plain + // bypass. Managed-session hooks always present the secret (X-Codeman-Hook-Secret, + // from $CODEMAN_HOOK_SECRET_FILE — generated for every instance), so requiring it + // always closes the gap without breaking the legitimate hook channel. if ((req.url === '/api/hook-event' || req.url === '/api/status-telemetry') && req.method === 'POST') { const ip = req.ip; const isLoopback = ip === '127.0.0.1' || ip === '::1' || ip === '::ffff:127.0.0.1'; if (isLoopback) { - if (!getTunnelRunning()) { - // Loopback-only: unchanged behavior. - done(); - return; - } - // Tunnel up: require the shared secret (constant-time compare). + // Always require the shared secret (constant-time compare). const presented = Buffer.from(req.headers[HOOK_SECRET_HEADER.toLowerCase()]?.toString() ?? ''); const expected = Buffer.from(getHookSecret()); if (presented.length === expected.length && timingSafeEqual(presented, expected)) { done(); return; } - // Wrong/absent secret while tunneled — rate-limit per IP in the DEDICATED - // hook bucket (never authFailures, which would lock out the login path). + // Wrong/absent secret — rate-limit per IP in the DEDICATED hook bucket + // (never authFailures, which would lock out the login path). const hookIp = req.ip; const hookFailures = hookSecretFailures.get(hookIp) ?? 0; if (hookFailures >= AUTH_FAILURE_MAX) { diff --git a/src/web/routes/session-routes.ts b/src/web/routes/session-routes.ts index 7174d2d8..f5348f30 100644 --- a/src/web/routes/session-routes.ts +++ b/src/web/routes/session-routes.ts @@ -45,7 +45,13 @@ import { validatePathWithinBase, } from '../route-helpers.js'; import { AUTH_COOKIE_NAME } from '../middleware/auth.js'; -import { writeHooksConfig, updateCaseModel, stripCaseEnvKeys, applyStatusLineConfig } from '../../hooks-config.js'; +import { + writeHooksConfig, + updateCaseModel, + stripCaseEnvKeys, + applyStatusLineConfig, + refreshStaleHookSecret, +} from '../../hooks-config.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; import { imageWatcher } from '../../image-watcher.js'; import { getLifecycleLog } from '../../session-lifecycle-log.js'; @@ -312,6 +318,13 @@ export function registerSessionRoutes( await applyStatusLineConfig(workingDir, true); } + // COD-91 self-heal: refresh a pre-secret hooks block in an existing case so the now + // unconditional hook-secret gate keeps accepting its hook events. No-op for fresh + // cases (writeHooksConfig already wrote the secret) and for non-Codeman/absent hooks. + if ((body.mode ?? 'claude') === 'claude') { + await refreshStaleHookSecret(workingDir).catch(() => {}); + } + // Check OpenCode availability if requested if (body.mode === 'opencode') { const { isOpenCodeAvailable } = await import('../../utils/opencode-cli-resolver.js'); @@ -1279,6 +1292,11 @@ export function registerSessionRoutes( } catch (err) { return createErrorResponse(ApiErrorCode.OPERATION_FAILED, `Failed to create case: ${getErrorMessage(err)}`); } + } else if (mode !== 'opencode') { + // COD-91 self-heal for an EXISTING case: refresh a pre-secret hooks block so the + // now-unconditional hook-secret gate keeps accepting its hook events. No-op when + // the hooks aren't ours or already carry the secret. + await refreshStaleHookSecret(casePath).catch(() => {}); } // Strip stale disk entries for keys this request is actively setting (Claude only — diff --git a/src/web/server.ts b/src/web/server.ts index cb07d9e4..8eada0d1 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -630,7 +630,7 @@ export class WebServer extends EventEmitter { registerHostGuard(this.app, () => this.getHostPolicy()); // Auth middleware (Basic Auth + session cookies + rate limiting) - const authState = registerAuthMiddleware(this.app, this.https, () => this.tunnelManager.isRunning()); + const authState = registerAuthMiddleware(this.app, this.https); if (authState) { this.authSessions = authState.authSessions; this.authFailures = authState.authFailures; diff --git a/test/auth-security.test.ts b/test/auth-security.test.ts index 8a22282c..e7a52c25 100644 --- a/test/auth-security.test.ts +++ b/test/auth-security.test.ts @@ -14,6 +14,7 @@ import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } import { WebServer } from '../src/web/server.js'; import { TmuxManager } from '../src/tmux-manager.js'; import { SettingsUpdateSchema } from '../src/web/schemas.js'; +import { getHookSecret, HOOK_SECRET_HEADER } from '../src/config/hook-secret.js'; const AUTH_PORT = 3160; const NOAUTH_PORT = 3161; @@ -250,28 +251,28 @@ describe('Auth Security', () => { }); describe('Hook Event Endpoint', () => { - it('should allow hook events from localhost without auth', async () => { + it('should allow hook events from localhost with the hook secret (no Basic auth)', async () => { const res = await fetch(`${baseUrl}/api/hook-event`, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', [HOOK_SECRET_HEADER]: getHookSecret() }, body: JSON.stringify({ event: 'stop', sessionId: 'nonexistent-session', data: {}, }), }); - // Should pass auth (localhost bypass) but may 404 on session — that's fine - // The key assertion is it does NOT return 401 + // Should pass auth (localhost bypass + hook secret) but may 404 on session — that's fine. + // The key assertion is it does NOT return 401 (COD-91: secret required even with no tunnel). expect(res.status).not.toBe(401); }); it('should reject hook events with invalid schema', async () => { const res = await fetch(`${baseUrl}/api/hook-event`, { method: 'POST', - headers: { 'Content-Type': 'application/json' }, + headers: { 'Content-Type': 'application/json', [HOOK_SECRET_HEADER]: getHookSecret() }, body: JSON.stringify({ invalid: 'data' }), }); - // Schema validation should catch this + // Past the auth gate (valid secret) → schema validation should catch this (not a 401). expect(res.status).not.toBe(401); // Not an auth error }); }); diff --git a/test/cod54-hook-event-auth.test.ts b/test/cod54-hook-event-auth.test.ts index ea2f64bf..10144207 100644 --- a/test/cod54-hook-event-auth.test.ts +++ b/test/cod54-hook-event-auth.test.ts @@ -4,15 +4,19 @@ * The `/api/hook-event` localhost bypass let tunnel traffic (cloudflared * --url http://127.0.0.1:port) reach the loopback origin with req.ip === * 127.0.0.1 and drive respawn/Ralph signals unauthenticated. The fix gates - * the bypass behind a shared hook secret WHEN A TUNNEL IS RUNNING, while - * keeping the plain localhost bypass for the normal loopback-only case so - * already-deployed (pre-secret) hooks and the loop's own channel keep working. + * the bypass behind a shared hook secret. COD-91 makes that requirement + * UNCONDITIONAL — the loopback bypass requires the secret whether or not a + * managed tunnel is running, because Codeman can't detect a user's own loopback + * reverse proxy (own cloudflared / `tailscale serve` / nginx → 127.0.0.1). + * Managed-session hooks always present the secret, so the legitimate channel + * keeps working. * * Tests: * - tunnel running + no secret → 401 (closes the hole) * - tunnel running + bad secret → 401 * - tunnel running + good secret → not 401 (allowed) - * - tunnel NOT running + no secret → not 401 (back-compat regression guard) + * - tunnel NOT running + no secret → 401 (COD-91: secret required unconditionally) + * - tunnel NOT running + good secret → not 401 (allowed) * - rate limiting: rapid unauthorized hook POSTs eventually 429 * * Port: 3230 (tunnel-running), 3231 (tunnel-down), 3232 (rate-limit) @@ -83,7 +87,7 @@ describe('COD-54 hook-event auth — tunnel running requires secret', () => { }); }); -describe('COD-54 hook-event auth — tunnel down keeps localhost bypass (back-compat)', () => { +describe('COD-91 hook-event auth — tunnel down ALSO requires the secret', () => { let server: WebServer; let baseUrl: string; let isRunningSpy: ReturnType; @@ -105,8 +109,13 @@ describe('COD-54 hook-event auth — tunnel down keeps localhost bypass (back-co delete process.env.CODEMAN_USERNAME; }); - it('still allows a localhost hook POST WITHOUT a secret (existing hooks + loop channel keep working)', async () => { + it('rejects a localhost hook POST WITHOUT a secret even with no tunnel (COD-91)', async () => { const res = await postHook(baseUrl); + expect(res.status).toBe(401); + }); + + it('allows a localhost hook POST WITH the correct secret when no tunnel is running', async () => { + const res = await postHook(baseUrl, { [HOOK_SECRET_HEADER]: getHookSecret() }); expect(res.status).not.toBe(401); }); }); diff --git a/test/hook-secret-selfheal.test.ts b/test/hook-secret-selfheal.test.ts new file mode 100644 index 00000000..f3c54b48 --- /dev/null +++ b/test/hook-secret-selfheal.test.ts @@ -0,0 +1,106 @@ +/** + * COD-91 — `refreshStaleHookSecret` self-heal. + * + * Making the hook-event secret unconditionally required (PR #127) would silently 401 the + * hook curls baked into cases created before the secret header existed (COD-54). Those + * curls live in `.claude/settings.local.json` and `writeHooksConfig` only runs at case + * CREATION, so existing cases never refresh. `refreshStaleHookSecret` regenerates the + * hooks block on session spawn — but ONLY when the case already holds Codeman's own + * pre-secret hook curls, never clobbering a user's customizations. + * + * Pure filesystem logic against a temp dir — no port / server / tmux. + */ + +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, readFileSync, existsSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import { refreshStaleHookSecret } from '../src/hooks-config.js'; + +const SECRET_HEADER = 'X-Codeman-Hook-Secret'; + +// A faithful pre-secret Codeman hook curl (what cases created before COD-54 contain): +// targets /api/hook-event, but with NO X-Codeman-Hook-Secret header. +function staleCodemanHooks() { + return { + Stop: [ + { + matcher: '', + hooks: [ + { + type: 'command', + command: + "HOOK_DATA=$(cat 2>/dev/null || echo '{}'); " + + 'printf \'{"event":"stop","sessionId":"%s","data":%s}\' "$CODEMAN_SESSION_ID" "$HOOK_DATA" | ' + + 'curl -s -X POST "$CODEMAN_API_URL/api/hook-event" -H \'Content-Type: application/json\' --data @- 2>/dev/null || true', + timeout: 5, + }, + ], + }, + ], + }; +} + +describe('refreshStaleHookSecret', () => { + let dir: string; + let settingsPath: string; + + beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), 'codeman-selfheal-')); + mkdirSync(join(dir, '.claude'), { recursive: true }); + settingsPath = join(dir, '.claude', 'settings.local.json'); + }); + + afterEach(() => { + rmSync(dir, { recursive: true, force: true }); + }); + + it('adds the secret header to a stale Codeman hooks block and preserves other keys', async () => { + writeFileSync( + settingsPath, + JSON.stringify({ env: { CLAUDE_CODE_FOO: '1' }, model: 'opus', hooks: staleCodemanHooks() }, null, 2) + ); + await refreshStaleHookSecret(dir); + + const after = JSON.parse(readFileSync(settingsPath, 'utf-8')); + expect(JSON.stringify(after.hooks)).toContain(SECRET_HEADER); + expect(JSON.stringify(after.hooks)).toContain('CODEMAN_HOOK_SECRET_FILE'); + // sibling keys untouched + expect(after.env).toEqual({ CLAUDE_CODE_FOO: '1' }); + expect(after.model).toBe('opus'); + }); + + it('leaves a hooks block that already carries the secret unchanged', async () => { + // Seed with a current block by healing a stale one first, then re-heal: second pass must no-op. + writeFileSync(settingsPath, JSON.stringify({ hooks: staleCodemanHooks() }, null, 2)); + await refreshStaleHookSecret(dir); + const healed = readFileSync(settingsPath, 'utf-8'); + expect(healed).toContain(SECRET_HEADER); + + await refreshStaleHookSecret(dir); + expect(readFileSync(settingsPath, 'utf-8')).toBe(healed); // byte-identical: no rewrite + }); + + it('does not touch hooks that are not Codeman’s (no /api/hook-event)', async () => { + const foreign = JSON.stringify( + { hooks: { Stop: [{ matcher: '', hooks: [{ type: 'command', command: 'echo hi', timeout: 5 }] }] } }, + null, + 2 + ); + writeFileSync(settingsPath, foreign); + await refreshStaleHookSecret(dir); + expect(readFileSync(settingsPath, 'utf-8')).toBe(foreign); + }); + + it('is a no-op when settings.local.json is absent (does not create one)', async () => { + await refreshStaleHookSecret(dir); + expect(existsSync(settingsPath)).toBe(false); + }); + + it('leaves a malformed settings file untouched', async () => { + const garbage = '{ not valid json'; + writeFileSync(settingsPath, garbage); + await refreshStaleHookSecret(dir); + expect(readFileSync(settingsPath, 'utf-8')).toBe(garbage); + }); +});