From f0f43ddbadb71a4cbac7351a16d1ddcd8794d904 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sun, 14 Jun 2026 12:46:58 -0400 Subject: [PATCH 1/2] Require the hook-event secret unconditionally, not only under a managed tunnel MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit COD-54 gated the /api/hook-event + /api/status-telemetry localhost bypass behind the shared X-Codeman-Hook-Secret only WHILE a managed tunnel was running, keeping a plain localhost bypass otherwise. But Codeman can't detect a user's OWN loopback reverse proxy (their own `cloudflared --url`, `tailscale serve`, nginx -> 127.0.0.1), which proxies internet traffic into the loopback origin with req.ip === 127.0.0.1 — so that setup kept the unsafe plain bypass. Require the secret on the loopback bypass unconditionally. Managed-session hooks already always present it (X-Codeman-Hook-Secret from $CODEMAN_HOOK_SECRET_FILE, generated for every instance), so the legitimate hook channel is unaffected; only the previously-unguarded own-proxy path is now rejected. Drops the now-unused getTunnelRunning param from registerAuthMiddleware. Tests: cod54-hook-event-auth (tunnel-down now also requires the secret, plus a good-secret positive case); auth-security (hook tests present the secret to reach schema validation). --- src/web/middleware/auth.ts | 40 +++++++++++------------------- src/web/server.ts | 2 +- test/auth-security.test.ts | 13 +++++----- test/cod54-hook-event-auth.test.ts | 21 +++++++++++----- 4 files changed, 37 insertions(+), 39 deletions(-) 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/server.ts b/src/web/server.ts index fa32a1df..9ca0c935 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); }); }); From 21fbff4d8a489c91399abd368bf461c3f9d94181 Mon Sep 17 00:00:00 2001 From: "Claude (Codeman maintainer)" Date: Sun, 14 Jun 2026 22:35:29 +0200 Subject: [PATCH 2/2] fix(hooks): self-heal stale pre-secret hook configs so COD-91 doesn't 401 them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making the hook-event secret unconditionally required closes the own-loopback-proxy gap, but it would also silently 401 the hook curls baked into cases created BEFORE the secret header existed (COD-54, 2026-06-10): writeHooksConfig only runs at case CREATION, so an existing/linked case on a password-protected install keeps secret-less curls that the new gate rejects (degrading idle/stop/teammate/task signalling with no error surfaced). No-password installs are unaffected — the gate isn't registered without CODEMAN_PASSWORD. Add `refreshStaleHookSecret(casePath)` and call it on Claude-mode spawns in POST /api/sessions and POST /api/quick-start (existing-case branch). It regenerates the hooks block ONLY when settings.local.json already holds Codeman's own hook curls (they target /api/hook-event) that lack the X-Codeman-Hook-Secret header — a no-op when the hooks are absent, not ours, or already current, so it never clobbers user customizations and is cheap on every spawn. Fresh cases are unaffected (writeHooksConfig already wrote the secret). withSettingsLock serializes it with the model/statusLine writers. Verified: new test/hook-secret-selfheal.test.ts 5/5 (heal + key-preservation + no-op on current/foreign/absent/malformed); the PR's cod54 + auth-security suites still pass (36); tsc, lint, format:check, and npm run build all clean (symbol present in dist). Co-Authored-By: Claude Opus 4.8 (1M context) --- src/hooks-config.ts | 37 +++++++++++ src/web/routes/session-routes.ts | 20 +++++- test/hook-secret-selfheal.test.ts | 106 ++++++++++++++++++++++++++++++ 3 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 test/hook-secret-selfheal.test.ts 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/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/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); + }); +});