From 42f0b28c75389c8de042050d3520f8b39ad743bb Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Wed, 10 Jun 2026 12:25:26 -0400 Subject: [PATCH 1/2] feat(security): hook-event auth secret + tunnel password guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two hardening fixes for the public-tunnel exposure path (COD-54 / COD-55). COD-54 — gate the /api/hook-event localhost bypass when a tunnel is up: `cloudflared --url http://127.0.0.1:port` proxies internet traffic INTO the loopback origin, so a tunneled hook request arrives with req.ip === 127.0.0.1 and the old bare-localhost bypass would pass it unauthenticated. Now: - tunnel running → bypass requires a shared per-instance hook secret (X-Codeman-Hook-Secret header; constant-time compare) + per-IP rate limiting - tunnel not running (loopback-only, the normal case) → unchanged, so already-deployed credential-less hooks keep working. New src/config/hook-secret.ts; auth middleware takes a getTunnelRunning probe (wired from server.ts via tunnelManager.isRunning()). COD-55 — refuse starting the Cloudflare tunnel without auth: enabling the tunnel publishes full terminal control to a public URL; with no CODEMAN_PASSWORD the auth middleware is inactive and the bind guard never trips (tunnel binds loopback). PUT /api/settings now refuses tunnelEnabled:true with a 403 (before persisting) unless CODEMAN_PASSWORD is set or CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK=1 is acknowledged. New isUnauthenticatedNetworkAcknowledged() in network-auth-policy; settings-ui surfaces the refusal as an error toast and reverts the toggle. Scope: the always-on CSRF/Origin guard, Host-header allowlist, and network-auth-policy itself are already upstream (#113) and not re-proposed here. Verification: tsc, eslint, prettier, check:frontend-syntax clean; full test:ci green (2723 passed), incl. test/cod54-hook-event-auth and test/routes/system-routes-tunnel-guard. --- src/config/hook-secret.ts | 67 ++++++++ src/web/middleware/auth.ts | 55 ++++++- src/web/network-auth-policy.ts | 10 ++ src/web/public/settings-ui.js | 57 ++++++- src/web/routes/system-routes.ts | 21 +++ src/web/server.ts | 2 +- test/cod54-hook-event-auth.test.ts | 150 ++++++++++++++++++ .../routes/system-routes-tunnel-guard.test.ts | 133 ++++++++++++++++ 8 files changed, 485 insertions(+), 10 deletions(-) create mode 100644 src/config/hook-secret.ts create mode 100644 test/cod54-hook-event-auth.test.ts create mode 100644 test/routes/system-routes-tunnel-guard.test.ts diff --git a/src/config/hook-secret.ts b/src/config/hook-secret.ts new file mode 100644 index 000000000..2f0f86055 --- /dev/null +++ b/src/config/hook-secret.ts @@ -0,0 +1,67 @@ +/** + * @fileoverview Per-instance shared hook secret (COD-54). + * + * Claude Code hooks POST to `/api/hook-event` with no Basic-Auth credentials, + * relying on a localhost bypass in `web/middleware/auth.ts`. That bypass is safe + * for loopback-only deploys, but a `cloudflared --url http://127.0.0.1:port` + * tunnel proxies internet traffic INTO the loopback origin, so tunneled requests + * arrive with `req.ip === 127.0.0.1` and would otherwise pass the bypass and + * drive respawn/Ralph signals unauthenticated. + * + * To close that hole WITHOUT breaking the loop's own (credential-less) hook + * channel, every locally-generated hook command now presents a per-instance + * shared secret in the `X-Codeman-Hook-Secret` header. The middleware requires + * a matching secret for the bypass WHEN A TUNNEL IS RUNNING. Tunneled internet + * traffic can't know the secret; local hooks (which we generate) do. + * + * Storage mirrors the VAPID-key pattern in `push-store.ts`: a small file under + * the instance data dir (`dataPath('hook-secret')`), read-if-present / + * generate-if-missing, stable across restarts. 256 bits of hex. + */ + +import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs'; +import { randomBytes } from 'node:crypto'; +import { getDataDir, dataPath } from './instance.js'; + +/** HTTP header local hooks use to present the shared secret. */ +export const HOOK_SECRET_HEADER = 'X-Codeman-Hook-Secret'; + +/** Number of random bytes in the secret (256 bits → 64 hex chars). */ +const SECRET_BYTES = 32; + +let cachedSecret: string | null = null; + +/** + * Return this instance's hook secret, generating and persisting it on first use. + * Stable across restarts. Cached in-process after the first read. + */ +export function getHookSecret(): string { + if (cachedSecret) return cachedSecret; + + const secretFile = dataPath('hook-secret'); + + if (existsSync(secretFile)) { + try { + const raw = readFileSync(secretFile, 'utf-8').trim(); + if (raw) { + cachedSecret = raw; + return cachedSecret; + } + // Empty/whitespace file — fall through and regenerate. + } catch { + // Unreadable — fall through and regenerate. + } + } + + const secret = randomBytes(SECRET_BYTES).toString('hex'); + try { + mkdirSync(getDataDir(), { recursive: true }); + // Owner-only perms — the secret gates the hook bypass. + writeFileSync(secretFile, secret, { mode: 0o600 }); + } catch { + // Best-effort persistence: even if the write fails we still return a usable + // secret for this process so hooks/middleware agree within this run. + } + cachedSecret = secret; + return cachedSecret; +} diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index 1e2a6b27c..4671823d4 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -19,6 +19,7 @@ import { AUTH_FAILURE_MAX, AUTH_FAILURE_WINDOW_MS, } from '../../config/auth-config.js'; +import { getHookSecret, HOOK_SECRET_HEADER } from '../../config/hook-secret.js'; // Auth session cookie name export const AUTH_COOKIE_NAME = 'codeman_session'; @@ -34,9 +35,20 @@ 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. * @returns AuthState for lifecycle management (dispose on server stop) */ -export function registerAuthMiddleware(app: FastifyInstance, https: boolean): AuthState { +export function registerAuthMiddleware( + app: FastifyInstance, + https: boolean, + getTunnelRunning: () => boolean = () => false +): AuthState { const state: AuthState = { authSessions: null, authFailures: null, @@ -78,13 +90,44 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean): Au } app.addHook('onRequest', (req, reply, done) => { - // Hook events come from local Claude Code hooks (curl from localhost) — no auth headers available. - // Safe: validated by HookEventSchema, only triggers broadcasts. - // Security: restrict bypass to localhost only — prevents forged hook events via tunnel/LAN. + // Hook events come from local Claude Code hooks (curl from localhost) — no + // Basic-Auth credentials available. Validated downstream by HookEventSchema. + // + // 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. if (req.url === '/api/hook-event' && req.method === 'POST') { const ip = req.ip; - if (ip === '127.0.0.1' || ip === '::1' || ip === '::ffff:127.0.0.1') { - done(); + 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). + 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 — treat as a failed auth attempt so the + // per-IP rate limiter (below) throttles brute-force/abuse of this route. + const hookIp = req.ip; + const hookFailures = authFailures.get(hookIp) ?? 0; + if (hookFailures >= AUTH_FAILURE_MAX) { + sendAuthRateLimit(reply, hookIp); + return; + } + authFailures.set(hookIp, hookFailures + 1); + reply.code(401).send('Unauthorized: hook secret required'); return; } // Non-localhost hook requests fall through to normal auth diff --git a/src/web/network-auth-policy.ts b/src/web/network-auth-policy.ts index 91240a744..a7e73423f 100644 --- a/src/web/network-auth-policy.ts +++ b/src/web/network-auth-policy.ts @@ -6,6 +6,16 @@ export function isExplicitlyEnabled(value: string | undefined): boolean { return value !== undefined && EXPLICIT_TRUE_VALUES.has(value.trim().toLowerCase()); } +/** + * True when unauthenticated network exposure is acceptable: either a password is + * set (auth active) or the operator explicitly acknowledged it. Used by the + * tunnel-enable guard (COD-55) to refuse publishing an unauthenticated public URL. + */ +export function isUnauthenticatedNetworkAcknowledged(allowFlag = false): boolean { + if (process.env.CODEMAN_PASSWORD) return true; + return allowFlag || isExplicitlyEnabled(process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK); +} + export function isLoopbackBindHost(host: string): boolean { const normalized = host .trim() diff --git a/src/web/public/settings-ui.js b/src/web/public/settings-ui.js index db3e47b55..f36573dc1 100644 --- a/src/web/public/settings-ui.js +++ b/src/web/public/settings-ui.js @@ -844,11 +844,18 @@ Object.assign(CodemanApp.prototype, { btn.disabled = true; try { const newEnabled = !isActive; - await fetch('/api/settings', { + const res = await fetch('/api/settings', { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ tunnelEnabled: newEnabled }), }); + // COD-55: server refuses an unauthenticated public tunnel (403). Surface it. + if (newEnabled && (await this._handleTunnelEnableRefusal(res))) { + this._dismissTunnelConnecting(); + this._updateWelcomeTunnelBtn(false); + btn.disabled = false; + return; + } if (newEnabled) { this._showTunnelConnecting(); // Poll tunnel status as fallback in case SSE event is missed @@ -1148,13 +1155,40 @@ Object.assign(CodemanApp.prototype, { return `${Math.floor(hrs / 24)}d ago`; }, + /** + * COD-55: detect the server's refusal to start an unauthenticated public tunnel. + * The PUT /api/settings route returns a 4xx with { success:false, error } when no + * CODEMAN_PASSWORD is set and the unauthenticated-network opt-in is not acknowledged. + * Shows the server's (actionable) message as an error toast. + * @param {Response|null} res - the fetch Response from the settings PUT + * @returns {Promise} true if the tunnel-enable was refused (caller should abort) + */ + async _handleTunnelEnableRefusal(res) { + if (!res || res.ok) return false; + let message = 'Tunnel refused: set CODEMAN_PASSWORD before exposing Codeman publicly.'; + try { + const body = await res.json(); + if (body && body.error) message = body.error; + } catch { + /* non-JSON body — use the default message */ + } + this._dismissTunnelConnecting?.(); + this.showToast(message, 'error'); + return true; + }, + async _tunnelPanelToggle(enable) { try { - await fetch('/api/settings', { + const res = await fetch('/api/settings', { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ tunnelEnabled: enable }), }); + // COD-55: server refuses an unauthenticated public tunnel (403). Surface it. + if (enable && (await this._handleTunnelEnableRefusal(res))) { + this.closeTunnelPanel(); + return; + } if (enable) { this._updateTunnelIndicator(false); const indicator = document.getElementById('tunnelIndicator'); @@ -1473,7 +1507,24 @@ Object.assign(CodemanApp.prototype, { // Strip device-specific keys — localEchoEnabled/cjkInputEnabled are per-platform const { localEchoEnabled: _leo, cjkInputEnabled: _cjk, extendedKeyboardBar: _ekb, ...serverSettings } = settings; try { - await this._apiPut('/api/settings', { ...serverSettings, notificationPreferences: notifPrefsToSave, voiceSettings }); + const res = await this._apiPut('/api/settings', { + ...serverSettings, + notificationPreferences: notifPrefsToSave, + voiceSettings, + }); + + // COD-55: the server refuses an unauthenticated public tunnel with a 403 — which + // rejects the WHOLE settings PUT. Surface the message and revert the tunnel toggle + // (in the UI + localStorage) so it doesn't look enabled. Other settings persisted + // to localStorage above still apply locally. + if (settings.tunnelEnabled && (await this._handleTunnelEnableRefusal(res))) { + settings.tunnelEnabled = false; + this.saveAppSettingsToStorage(settings); + const cb = document.getElementById('appSettingsTunnelEnabled'); + if (cb) cb.checked = false; + this.closeAppSettings(); + return; + } // Save model configuration separately await this.saveModelConfigFromSettings(); diff --git a/src/web/routes/system-routes.ts b/src/web/routes/system-routes.ts index e3bd2f492..0c2f6379c 100644 --- a/src/web/routes/system-routes.ts +++ b/src/web/routes/system-routes.ts @@ -14,6 +14,7 @@ import { execSync, spawn } from 'node:child_process'; import { randomBytes } from 'node:crypto'; import { dataPath } from '../../config/instance.js'; import { ApiErrorCode, createErrorResponse, getErrorMessage, type NiceConfig } from '../../types.js'; +import { isUnauthenticatedNetworkAcknowledged } from '../network-auth-policy.js'; import { ConfigUpdateSchema, SettingsUpdateSchema, @@ -498,6 +499,26 @@ export function registerSystemRoutes( app.put('/api/settings', async (req) => { const settings = parseBody(SettingsUpdateSchema, req.body, 'Invalid settings') as Record; + // COD-55: enabling the Cloudflare tunnel publishes the whole app (full terminal + // control = effectively RCE) to a public *.trycloudflare.com URL. Because the + // tunnel binds to loopback, server.ts's non-loopback bind guard never trips, and + // with no CODEMAN_PASSWORD the auth middleware is inactive — so the tunnel URL is + // unauthenticated. Refuse to start a tunnel unless auth is configured OR the + // operator has acknowledged unauthenticated-network exposure. A public tunnel is + // higher-stakes than a LAN bind, so this is REFUSE (vs the bind guard's warn). + // Guard runs BEFORE persisting so a refused tunnelEnabled:true is not saved. + if (settings.tunnelEnabled === true && !ctx.tunnelManager.isRunning() && !isUnauthenticatedNetworkAcknowledged()) { + const msg = + 'Refusing to start the Cloudflare tunnel without authentication: it would publish ' + + 'full terminal control to a public URL with no password. Set CODEMAN_PASSWORD to ' + + 'require login, or set CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK=1 to acknowledge an ' + + 'unauthenticated public tunnel.'; + throw Object.assign(new Error(msg), { + statusCode: 403, + body: createErrorResponse(ApiErrorCode.OPERATION_FAILED, msg), + }); + } + try { const dir = dirname(SETTINGS_PATH); if (!existsSync(dir)) { diff --git a/src/web/server.ts b/src/web/server.ts index 3b91feb72..fa46cc3b2 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -603,7 +603,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); + const authState = registerAuthMiddleware(this.app, this.https, () => this.tunnelManager.isRunning()); if (authState) { this.authSessions = authState.authSessions; this.authFailures = authState.authFailures; diff --git a/test/cod54-hook-event-auth.test.ts b/test/cod54-hook-event-auth.test.ts new file mode 100644 index 000000000..b3f2ad55f --- /dev/null +++ b/test/cod54-hook-event-auth.test.ts @@ -0,0 +1,150 @@ +/** + * @fileoverview COD-54 — hook-event auth bypass hardening. + * + * 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. + * + * 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) + * - rate limiting: rapid unauthorized hook POSTs eventually 429 + * + * Port: 3230 (tunnel-running), 3231 (tunnel-down), 3232 (rate-limit) + */ +import { describe, it, expect, beforeAll, afterAll, vi } from 'vitest'; +import { WebServer } from '../src/web/server.js'; +import { TmuxManager } from '../src/tmux-manager.js'; +import { TunnelManager } from '../src/tunnel-manager.js'; +import { getHookSecret, HOOK_SECRET_HEADER } from '../src/config/hook-secret.js'; +import { AUTH_FAILURE_MAX } from '../src/config/auth-config.js'; + +const TUNNEL_UP_PORT = 3230; +const TUNNEL_DOWN_PORT = 3231; +const RATE_LIMIT_PORT = 3232; +const TEST_USER = 'admin'; +const TEST_PASS = 'cod54-test-password'; + +vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true); + +function hookBody(): string { + return JSON.stringify({ event: 'stop', sessionId: 'nonexistent-session', data: {} }); +} + +async function postHook(baseUrl: string, headers: Record = {}): Promise { + return fetch(`${baseUrl}/api/hook-event`, { + method: 'POST', + headers: { 'Content-Type': 'application/json', ...headers }, + body: hookBody(), + }); +} + +describe('COD-54 hook-event auth — tunnel running requires secret', () => { + let server: WebServer; + let baseUrl: string; + let isRunningSpy: ReturnType; + + beforeAll(async () => { + process.env.CODEMAN_PASSWORD = TEST_PASS; + process.env.CODEMAN_USERNAME = TEST_USER; + // Force the middleware's tunnel check to report "running". + isRunningSpy = vi.spyOn(TunnelManager.prototype, 'isRunning').mockReturnValue(true); + server = new WebServer(TUNNEL_UP_PORT, false, true); + await server.start(); + baseUrl = `http://localhost:${TUNNEL_UP_PORT}`; + }); + + afterAll(async () => { + await server.stop(); + isRunningSpy.mockRestore(); + delete process.env.CODEMAN_PASSWORD; + delete process.env.CODEMAN_USERNAME; + }); + + it('rejects a localhost hook POST WITHOUT the secret header (closes the tunnel hole)', async () => { + const res = await postHook(baseUrl); + expect(res.status).toBe(401); + }); + + it('rejects a localhost hook POST with a WRONG secret', async () => { + const res = await postHook(baseUrl, { [HOOK_SECRET_HEADER]: 'wrong-secret-value' }); + expect(res.status).toBe(401); + }); + + it('allows a localhost hook POST WITH the correct secret', async () => { + const res = await postHook(baseUrl, { [HOOK_SECRET_HEADER]: getHookSecret() }); + // Passes auth (may 200 with success:false for unknown session) — key is NOT 401. + expect(res.status).not.toBe(401); + }); +}); + +describe('COD-54 hook-event auth — tunnel down keeps localhost bypass (back-compat)', () => { + let server: WebServer; + let baseUrl: string; + let isRunningSpy: ReturnType; + + beforeAll(async () => { + process.env.CODEMAN_PASSWORD = TEST_PASS; + process.env.CODEMAN_USERNAME = TEST_USER; + // Tunnel NOT running — loopback-only normal prod case. + isRunningSpy = vi.spyOn(TunnelManager.prototype, 'isRunning').mockReturnValue(false); + server = new WebServer(TUNNEL_DOWN_PORT, false, true); + await server.start(); + baseUrl = `http://localhost:${TUNNEL_DOWN_PORT}`; + }); + + afterAll(async () => { + await server.stop(); + isRunningSpy.mockRestore(); + delete process.env.CODEMAN_PASSWORD; + delete process.env.CODEMAN_USERNAME; + }); + + it('still allows a localhost hook POST WITHOUT a secret (existing hooks + loop channel keep working)', async () => { + const res = await postHook(baseUrl); + expect(res.status).not.toBe(401); + }); +}); + +describe('COD-54 hook-event auth — rate limiting', () => { + let server: WebServer; + let baseUrl: string; + let isRunningSpy: ReturnType; + + beforeAll(async () => { + process.env.CODEMAN_PASSWORD = TEST_PASS; + process.env.CODEMAN_USERNAME = TEST_USER; + // Tunnel running so unauthorized (no-secret) hook POSTs are rejected and counted. + isRunningSpy = vi.spyOn(TunnelManager.prototype, 'isRunning').mockReturnValue(true); + server = new WebServer(RATE_LIMIT_PORT, false, true); + await server.start(); + baseUrl = `http://localhost:${RATE_LIMIT_PORT}`; + }); + + afterAll(async () => { + await server.stop(); + isRunningSpy.mockRestore(); + delete process.env.CODEMAN_PASSWORD; + delete process.env.CODEMAN_USERNAME; + }); + + it('eventually returns 429 for rapid unauthorized hook POSTs', async () => { + let saw429 = false; + // A few more than the failure max to cross the threshold. + for (let i = 0; i < AUTH_FAILURE_MAX + 3; i++) { + const res = await postHook(baseUrl); + if (res.status === 429) { + saw429 = true; + expect(res.headers.get('retry-after')).toMatch(/^\d+$/); + break; + } + expect(res.status).toBe(401); + } + expect(saw429).toBe(true); + }); +}); diff --git a/test/routes/system-routes-tunnel-guard.test.ts b/test/routes/system-routes-tunnel-guard.test.ts new file mode 100644 index 000000000..01efdf5e1 --- /dev/null +++ b/test/routes/system-routes-tunnel-guard.test.ts @@ -0,0 +1,133 @@ +/** + * @fileoverview COD-55 — tunnel password guard. + * + * Enabling the Cloudflare tunnel publishes the whole app (full terminal control = + * effectively RCE) to a public *.trycloudflare.com URL. When no CODEMAN_PASSWORD + * is set, requests through that URL are unauthenticated. These tests assert the + * PUT /api/settings tunnel-enable path REFUSES to start the tunnel unless a + * password is set OR the unauthenticated-network opt-in is acknowledged. + * + * Uses app.inject() — no real HTTP ports needed. Port: N/A. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { createRouteTestHarness, type RouteTestHarness } from './_route-test-utils.js'; +import { registerSystemRoutes } from '../../src/web/routes/system-routes.js'; + +// Settings are written to disk via fs/promises — stub so the guard test never +// touches the real settings.json, and so we can assert "not persisted on refusal". +vi.mock('node:fs/promises', () => ({ + default: { + readFile: vi.fn(async () => '{}'), + writeFile: vi.fn(async () => undefined), + }, +})); + +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + existsSync: vi.fn(() => true), + mkdirSync: vi.fn(), + readdirSync: vi.fn(() => []), + }; +}); + +import fs from 'node:fs/promises'; +const mockedWriteFile = vi.mocked(fs.writeFile); + +/** Build a tunnelManager stub the route's ctx can use. */ +function makeTunnelManager(running = false) { + return { + start: vi.fn(), + stop: vi.fn(), + isRunning: vi.fn(() => running), + getUrl: vi.fn(() => null), + getStatus: vi.fn(() => ({ running })), + }; +} + +describe('COD-55 tunnel password guard (PUT /api/settings tunnelEnabled)', () => { + let harness: RouteTestHarness; + let tunnel: ReturnType; + const savedPassword = process.env.CODEMAN_PASSWORD; + const savedOptIn = process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; + + beforeEach(async () => { + harness = await createRouteTestHarness(registerSystemRoutes); + vi.clearAllMocks(); + mockedWriteFile.mockResolvedValue(undefined); + tunnel = makeTunnelManager(false); + // tunnelManager is null in the default mock ctx — inject our spy. + (harness.ctx as unknown as { tunnelManager: unknown }).tunnelManager = tunnel; + delete process.env.CODEMAN_PASSWORD; + delete process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; + }); + + afterEach(async () => { + await harness.app.close(); + if (savedPassword === undefined) delete process.env.CODEMAN_PASSWORD; + else process.env.CODEMAN_PASSWORD = savedPassword; + if (savedOptIn === undefined) delete process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; + else process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK = savedOptIn; + }); + + it('REFUSES tunnel-enable with no password and no opt-in (4xx, start not called)', async () => { + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { tunnelEnabled: true }, + }); + + expect(res.statusCode).toBeGreaterThanOrEqual(400); + expect(res.statusCode).toBeLessThan(500); + const body = JSON.parse(res.body); + expect(body.success).toBe(false); + // Message should tell the user how to fix it. + expect(body.error).toMatch(/CODEMAN_PASSWORD|CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK/); + // The tunnel must NOT have been started. + expect(tunnel.start).not.toHaveBeenCalled(); + // And tunnelEnabled:true must NOT have been persisted. + expect(mockedWriteFile).not.toHaveBeenCalled(); + }); + + it('ALLOWS tunnel-enable when CODEMAN_PASSWORD is set (start called, 200)', async () => { + process.env.CODEMAN_PASSWORD = 'hunter2'; + + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { tunnelEnabled: true }, + }); + + expect(res.statusCode).toBe(200); + expect(tunnel.start).toHaveBeenCalledTimes(1); + }); + + it('ALLOWS tunnel-enable with the unauthenticated-network opt-in acknowledged (start called, 200)', async () => { + process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK = '1'; + + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { tunnelEnabled: true }, + }); + + expect(res.statusCode).toBe(200); + expect(tunnel.start).toHaveBeenCalledTimes(1); + }); + + it('does not guard tunnel-disable (tunnelEnabled:false always allowed)', async () => { + tunnel = makeTunnelManager(true); + (harness.ctx as unknown as { tunnelManager: unknown }).tunnelManager = tunnel; + + const res = await harness.app.inject({ + method: 'PUT', + url: '/api/settings', + payload: { tunnelEnabled: false }, + }); + + expect(res.statusCode).toBe(200); + expect(tunnel.stop).toHaveBeenCalledTimes(1); + }); +}); From aa4e1ce9cfdc7a3ecf8ad7a640791d3daf762cff Mon Sep 17 00:00:00 2001 From: arkon Date: Wed, 10 Jun 2026 22:31:09 +0200 Subject: [PATCH 2/2] fix(security): deliver the hook secret to hooks + isolate its rate-limit bucket MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes for COD-54: - Generated hook curl commands now present X-Codeman-Hook-Secret, read from the secret file AT EXECUTION TIME via $CODEMAN_HOOK_SECRET_FILE (exported into every managed session's env by tmux buildEnvExports / the direct-PTY env builders). Without this, every local hook 401'd the moment a managed tunnel came up — the enforcement existed but nothing presented the secret. Path-not-value keeps the secret off command lines and out of config files, and running sessions pick up a newly generated secret with no respawn; server.start() ensures the file exists up front. - Hook-secret failures now count into a DEDICATED per-IP bucket (hookSecretFailures) instead of the shared authFailures map. Legacy (pre-secret) hook configs fire constantly from 127.0.0.1; counting their 401s against the shared bucket would 429 every cookie-less loopback request — locking out the Basic-Auth login path (and, through a tunnel, every client, since tunneled traffic also arrives as 127.0.0.1). - docs/security-architecture.md: secret-gated hook exemption, dedicated bucket, COD-55 refusal, and the residual caveat for EXTERNAL loopback proxies (user-run cloudflared / tailscale serve), which the managed-tunnel probe cannot see. - test/cod54-hook-event-auth.test.ts: +3 tests — login path unaffected after hook-bucket exhaustion; generated hooks reference the header + $CODEMAN_HOOK_SECRET_FILE without embedding the value; env builders export the path only. Co-Authored-By: Claude Fable 5 --- docs/security-architecture.md | 28 ++++++++++++++++++----- src/hooks-config.ts | 12 ++++++++-- src/session-cli-builder.ts | 5 +++++ src/tmux-manager.ts | 3 +++ src/web/middleware/auth.ts | 31 +++++++++++++++++++------ src/web/server.ts | 11 +++++++++ test/cod54-hook-event-auth.test.ts | 36 ++++++++++++++++++++++++++++++ 7 files changed, 111 insertions(+), 15 deletions(-) diff --git a/docs/security-architecture.md b/docs/security-architecture.md index a2a3300be..c52453e85 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 29f6df110..f1d8bb480 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 e39c9e95b..ad6ed85fb 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 fa46cc3b2..d316e4473 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 b3f2ad55f..ea2f64bfe 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()); + }); });