mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 22:49:41 +02:00
fix(security): deliver the hook secret to hooks + isolate its rate-limit bucket
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 <noreply@anthropic.com>
This commit is contained in:
@@ -29,6 +29,7 @@ interface AuthState {
|
||||
authSessions: StaleExpirationMap<string, AuthSessionRecord> | null;
|
||||
authFailures: StaleExpirationMap<string, number> | null;
|
||||
qrAuthFailures: StaleExpirationMap<string, number> | null;
|
||||
hookSecretFailures: StaleExpirationMap<string, number> | 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<string, number>({
|
||||
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<string, number> = 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;
|
||||
}
|
||||
|
||||
@@ -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<string, import('./ports/auth-port.js').AuthSessionRecord> | null = null;
|
||||
private authFailures: StaleExpirationMap<string, number> | null = null;
|
||||
private qrAuthFailures: StaleExpirationMap<string, number> | null = null;
|
||||
private hookSecretFailures: StaleExpirationMap<string, number> | 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();
|
||||
|
||||
|
||||
Reference in New Issue
Block a user