From 333dc047c3973d984d4f19251f2a8c4fc82722c7 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Mon, 8 Jun 2026 10:06:34 -0400 Subject: [PATCH 1/5] fix: COD-29 fail closed for unauthenticated network binds --- src/cli.ts | 14 ++++++-- src/web/server.ts | 70 +++++++++++++++++++++++++++++++------- test/auth-security.test.ts | 66 +++++++++++++++++++++++++---------- test/cli-commands.test.ts | 50 ++++++++++++++++++--------- 4 files changed, 151 insertions(+), 49 deletions(-) diff --git a/src/cli.ts b/src/cli.ts index 520394c1..ea6536ac 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -483,21 +483,29 @@ program program .command('web') .description('Start the web interface') + .option('-H, --host ', 'Host to bind to', process.env.CODEMAN_HOST || '127.0.0.1') .option('-p, --port ', 'Port to listen on (env: CODEMAN_PORT)', process.env.CODEMAN_PORT || '3000') .option('--https', 'Enable HTTPS with self-signed certificate (only needed for remote access, not localhost)') .option('--title-hostname ', 'Override the hostname shown in the browser title') + .option( + '--allow-unauthenticated-network', + 'Allow non-loopback web access without CODEMAN_PASSWORD (dangerous; terminal control is exposed)' + ) .action(async (options) => { const { startWebServer } = await import('./web/server.js'); + const host = options.host; const port = parseInt(options.port, 10); const https = !!options.https; const titleHostname = options.titleHostname; + const allowUnauthenticatedNetwork = !!options.allowUnauthenticatedNetwork; const protocol = https ? 'https' : 'http'; + const displayHost = host === '0.0.0.0' ? 'localhost' : host; - console.log(chalk.cyan(`Starting Codeman web interface on port ${port}${https ? ' (HTTPS)' : ''}...`)); + console.log(chalk.cyan(`Starting Codeman web interface on ${displayHost}:${port}${https ? ' (HTTPS)' : ''}...`)); try { - const server = await startWebServer(port, https, false, titleHostname); - console.log(chalk.green(`\n✓ Web interface running at ${protocol}://localhost:${port}`)); + const server = await startWebServer(port, https, false, host, titleHostname, allowUnauthenticatedNetwork); + console.log(chalk.green(`\n✓ Web interface running at ${protocol}://${displayHost}:${port}`)); if (https) { console.log(chalk.yellow(' Note: Accept the self-signed certificate in your browser on first visit')); } diff --git a/src/web/server.ts b/src/web/server.ts index ad94e517..95eebb90 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -42,6 +42,7 @@ import { execSync } from 'node:child_process'; import { hostname as getHostname } from 'node:os'; import { dataPath } from '../config/instance.js'; import { EventEmitter } from 'node:events'; +import { isIP } from 'node:net'; import { Session, type BackgroundTask } from '../session.js'; import type { ClaudeMode, SessionState } from '../types.js'; import { RespawnController, RespawnConfig } from '../respawn-controller.js'; @@ -122,6 +123,26 @@ import { const __dirname = dirname(fileURLToPath(import.meta.url)); +const EXPLICIT_TRUE_VALUES = new Set(['1', 'true', 'yes', 'on']); + +function isExplicitlyEnabled(value: string | undefined): boolean { + return value !== undefined && EXPLICIT_TRUE_VALUES.has(value.trim().toLowerCase()); +} + +function isLoopbackBindHost(host: string): boolean { + const normalized = host + .trim() + .toLowerCase() + .replace(/^\[(.*)\]$/, '$1'); + if (normalized === 'localhost' || normalized === '::1' || normalized === '0:0:0:0:0:0:0:1') { + return true; + } + if (isIP(normalized) === 4 && normalized.startsWith('127.')) { + return true; + } + return normalized.startsWith('::ffff:127.'); +} + // Bounded, predictable shape for SSE client identifiers: alphanumerics, `_`, `-`. // Length range covers crypto.randomUUID() (36 chars) plus any short stable IDs, // while capping growth of `sseClientsById` and blocking pathological inputs. @@ -191,6 +212,7 @@ export class WebServer extends EventEmitter { private sse: SseStreamManager; private store = getStore(); private port: number; + private host: string; private https: boolean; private testMode: boolean; private mux: TerminalMultiplexer; @@ -232,6 +254,10 @@ export class WebServer extends EventEmitter { private pushStore: PushSubscriptionStore = new PushSubscriptionStore(); private teamWatcher: TeamWatcher = new TeamWatcher(); private _orchestratorLoop: import('../orchestrator-loop.js').OrchestratorLoop | null = null; + private readonly titleHostname: string; + private readonly windowTitle: string; + private readonly indexHtmlTemplate: string; + private readonly allowUnauthenticatedNetwork: boolean; private _pasteImageGcStop: (() => void) | null = null; private _eventLoopMonitor: EventLoopMonitorHandle | null = null; private teamWatcherHandlers: { @@ -240,15 +266,22 @@ export class WebServer extends EventEmitter { teamRemoved: (config: unknown) => void; taskUpdated: (data: unknown) => void; } | null = null; - private readonly titleHostname: string; - private readonly windowTitle: string; - private readonly indexHtmlTemplate: string; - constructor(port: number = 3000, https: boolean = false, testMode: boolean = false, titleHostname?: string) { + constructor( + port: number = 3000, + https: boolean = false, + testMode: boolean = false, + host: string = '127.0.0.1', + titleHostname?: string, + allowUnauthenticatedNetwork: boolean = false + ) { super(); this.setMaxListeners(0); + this.host = host; this.port = port; this.https = https; this.testMode = testMode; + this.allowUnauthenticatedNetwork = + allowUnauthenticatedNetwork || isExplicitlyEnabled(process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK); this.titleHostname = titleHostname || getHostname(); this.windowTitle = `codeman:${this.titleHostname}`; this.indexHtmlTemplate = readFileSync(join(__dirname, 'public', 'index.html'), 'utf-8'); @@ -1646,6 +1679,13 @@ export class WebServer extends EventEmitter { } async start(): Promise { + if (!isLoopbackBindHost(this.host) && !process.env.CODEMAN_PASSWORD && !this.allowUnauthenticatedNetwork) { + throw new Error( + 'Refusing to start Codeman on a non-loopback host without CODEMAN_PASSWORD. ' + + 'Set CODEMAN_PASSWORD or explicitly allow unauthenticated network access.' + ); + } + await this.setupRoutes(); const lifecycleLog = getLifecycleLog(); @@ -1672,19 +1712,23 @@ export class WebServer extends EventEmitter { this._eventLoopMonitor = startEventLoopMonitor(); } - await this.app.listen({ port: this.port, host: '0.0.0.0' }); + await this.app.listen({ port: this.port, host: this.host }); const protocol = this.https ? 'https' : 'http'; - console.log(`Codeman web interface running at ${protocol}://localhost:${this.port}`); + const displayHost = this.host === '0.0.0.0' ? 'localhost' : this.host; + console.log(`Codeman web interface running at ${protocol}://${displayHost}:${this.port}`); - // Security warning: server binds to 0.0.0.0 (all interfaces) — warn if no auth configured - if (!process.env.CODEMAN_PASSWORD) { + if (!isLoopbackBindHost(this.host) && !process.env.CODEMAN_PASSWORD && this.allowUnauthenticatedNetwork) { console.warn('\n⚠ WARNING: No CODEMAN_PASSWORD set — server is accessible without authentication.'); console.warn(' Anyone on your network can access and control Claude sessions.'); - console.warn(' Set CODEMAN_PASSWORD environment variable to enable auth.\n'); + console.warn( + ' This was explicitly allowed by --allow-unauthenticated-network or CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK.\n' + ); } // Set API URL for child processes (MCP server, spawned sessions) - process.env.CODEMAN_API_URL = `${protocol}://localhost:${this.port}`; + const apiHost = + 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}`; // Start scheduled runs cleanup timer this.cleanup.setInterval( @@ -2176,9 +2220,11 @@ export async function startWebServer( port: number = 3000, https: boolean = false, testMode: boolean = false, - titleHostname?: string + host: string = '127.0.0.1', + titleHostname?: string, + allowUnauthenticatedNetwork: boolean = false ): Promise { - const server = new WebServer(port, https, testMode, titleHostname); + const server = new WebServer(port, https, testMode, host, titleHostname, allowUnauthenticatedNetwork); await server.start(); return server; } diff --git a/test/auth-security.test.ts b/test/auth-security.test.ts index db3ed2d1..70a0241c 100644 --- a/test/auth-security.test.ts +++ b/test/auth-security.test.ts @@ -3,12 +3,12 @@ * 1. Timing-safe password comparison (timingSafeEqual) * 2. Hook event endpoint restricted to localhost * 3. Session cookie TTL refresh on access - * 4. Startup warning when no password configured + * 4. Startup fails closed when network-bound without auth * 5. SSE client limit enforcement * 6. Logout endpoint invalidates session * 7. Settings schema rejects unknown fields * - * Port: 3160 (auth tests), 3161 (no-auth tests) + * Port: 3160 (auth tests), 3161 (loopback no-auth tests), 3162 (network override tests) */ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { WebServer } from '../src/web/server.js'; @@ -16,6 +16,7 @@ import { SettingsUpdateSchema } from '../src/web/schemas.js'; const AUTH_PORT = 3160; const NOAUTH_PORT = 3161; +const NETWORK_OVERRIDE_PORT = 3162; const TEST_USER = 'admin'; const TEST_PASS = 'test-password-12345'; @@ -220,7 +221,7 @@ describe('Settings Schema Security', () => { it('should enforce tunnelEnabled as boolean', () => { const result = SettingsUpdateSchema.safeParse({ - tunnelEnabled: 'yes', // truthy string — should be rejected + tunnelEnabled: 'yes', // truthy string — should be rejected }); expect(result.success).toBe(false); }); @@ -264,40 +265,69 @@ describe('Settings Schema Security', () => { expect(validResult.success).toBe(true); const invalidResult = SettingsUpdateSchema.safeParse({ - nice: { enabled: true, niceValue: 100 }, // Out of range + nice: { enabled: true, niceValue: 100 }, // Out of range }); expect(invalidResult.success).toBe(false); }); }); -describe('No-Auth Server Warning', () => { +describe('No-Auth Server Startup Policy', () => { let server: WebServer; - let consoleWarnSpy: string[] = []; - const originalWarn = console.warn; beforeAll(async () => { delete process.env.CODEMAN_PASSWORD; delete process.env.CODEMAN_USERNAME; - consoleWarnSpy = []; - console.warn = (...args: unknown[]) => { - consoleWarnSpy.push(args.map(String).join(' ')); - }; - server = new WebServer(NOAUTH_PORT, false, true); + delete process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; + server = new WebServer(NOAUTH_PORT, false, true, '127.0.0.1'); await server.start(); }); afterAll(async () => { - console.warn = originalWarn; + delete process.env.CODEMAN_PASSWORD; + delete process.env.CODEMAN_USERNAME; + delete process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; await server.stop(); }); - it('should warn when no CODEMAN_PASSWORD is set', () => { - const hasWarning = consoleWarnSpy.some(msg => msg.includes('No CODEMAN_PASSWORD set')); - expect(hasWarning).toBe(true); + it('allows loopback requests without auth when no password is configured', async () => { + const res = await fetch(`http://localhost:${NOAUTH_PORT}/api/status`); + expect(res.status).toBe(200); }); - it('should allow requests without auth when no password configured', async () => { - const res = await fetch(`http://localhost:${NOAUTH_PORT}/api/status`); + it('rejects non-loopback startup without a password or explicit override', async () => { + const networkServer = new WebServer(0, false, true, '0.0.0.0'); + + await expect(networkServer.start()).rejects.toThrow(/CODEMAN_PASSWORD/); + + await networkServer.stop(); + }); + + it('allows non-loopback startup when CODEMAN_PASSWORD is configured', async () => { + process.env.CODEMAN_PASSWORD = TEST_PASS; + const networkServer = new WebServer(0, false, true, '0.0.0.0'); + + await networkServer.start(); + await networkServer.stop(); + + delete process.env.CODEMAN_PASSWORD; + }); + + it('allows non-loopback startup with the explicit unauthenticated-network override', async () => { + const networkServer = new WebServer(NETWORK_OVERRIDE_PORT, false, true, '0.0.0.0', undefined, true); + + await networkServer.start(); + const res = await fetch(`http://localhost:${NETWORK_OVERRIDE_PORT}/api/status`); expect(res.status).toBe(200); + await networkServer.stop(); + }); + + it('allows non-loopback startup with the explicit unauthenticated-network env override', async () => { + process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK = 'true'; + const networkServer = new WebServer(0, false, true, '0.0.0.0'); + + await networkServer.start(); + await networkServer.stop(); + + delete process.env.CODEMAN_ALLOW_UNAUTHENTICATED_NETWORK; }); }); diff --git a/test/cli-commands.test.ts b/test/cli-commands.test.ts index db903c41..ae65fb5d 100644 --- a/test/cli-commands.test.ts +++ b/test/cli-commands.test.ts @@ -5,6 +5,7 @@ */ import { describe, it, expect } from 'vitest'; +import { program } from '../src/cli.js'; describe('CLI Command Parsing', () => { describe('Command Structure', () => { @@ -72,11 +73,11 @@ describe('CLI Command Parsing', () => { ]; const findCommand = (name: string): Command | undefined => { - return commands.find(c => c.name === name || c.aliases.includes(name)); + return commands.find((c) => c.name === name || c.aliases.includes(name)); }; const findSubcommand = (parent: Command, name: string): Command | undefined => { - return parent.subcommands?.find(c => c.name === name || c.aliases.includes(name)); + return parent.subcommands?.find((c) => c.name === name || c.aliases.includes(name)); }; it('should find commands by name', () => { @@ -103,7 +104,7 @@ describe('CLI Command Parsing', () => { }); it('should have descriptions for all commands', () => { - commands.forEach(cmd => { + commands.forEach((cmd) => { expect(cmd.description).toBeTruthy(); }); }); @@ -267,13 +268,13 @@ describe('CLI Command Parsing', () => { }); it('should have defaults for web flags', () => { - webFlags.forEach(flag => { + webFlags.forEach((flag) => { expect(flag.default).toBeDefined(); }); }); it('should have defaults for tui flags', () => { - tuiFlags.forEach(flag => { + tuiFlags.forEach((flag) => { expect(flag.default).toBeDefined(); }); }); @@ -322,7 +323,7 @@ describe('CLI Command Parsing', () => { help += `${description}\n`; if (options.length > 0) { help += '\nOptions:\n'; - options.forEach(opt => { + options.forEach((opt) => { help += ` ${opt}\n`; }); } @@ -345,6 +346,15 @@ describe('CLI Command Parsing', () => { expect(help).toContain('--host'); }); + it('documents the unauthenticated network override in real web command help', () => { + const webCommand = program.commands.find((command) => command.name() === 'web'); + expect(webCommand).toBeDefined(); + + const help = webCommand!.helpInformation(); + expect(help).toContain('--allow-unauthenticated-network'); + expect(help).toMatch(/without\s+CODEMAN_PASSWORD/); + }); + it('should format properly', () => { const help = generateHelp('test', 'Test command', ['--flag']); const lines = help.split('\n'); @@ -474,22 +484,27 @@ describe('CLI Output Formatting', () => { } const formatRow = (values: string[], columns: Column[]): string => { - return values.map((val, i) => { - const width = columns[i]?.width || 10; - return val.padEnd(width).substring(0, width); - }).join(' '); + return values + .map((val, i) => { + const width = columns[i]?.width || 10; + return val.padEnd(width).substring(0, width); + }) + .join(' '); }; const formatTable = (headers: string[], rows: string[][], widths: number[]): string => { const columns = headers.map((h, i) => ({ header: h, width: widths[i] })); const headerRow = formatRow(headers, columns); - const separator = columns.map(c => '-'.repeat(c.width)).join(' '); - const dataRows = rows.map(row => formatRow(row, columns)); + const separator = columns.map((c) => '-'.repeat(c.width)).join(' '); + const dataRows = rows.map((row) => formatRow(row, columns)); return [headerRow, separator, ...dataRows].join('\n'); }; it('should format single row', () => { - const columns = [{ header: 'ID', width: 10 }, { header: 'Status', width: 8 }]; + const columns = [ + { header: 'ID', width: 10 }, + { header: 'Status', width: 8 }, + ]; const row = formatRow(['123', 'active'], columns); expect(row).toBe('123 active '); }); @@ -503,7 +518,10 @@ describe('CLI Output Formatting', () => { it('should format complete table', () => { const table = formatTable( ['ID', 'Status'], - [['1', 'active'], ['2', 'idle']], + [ + ['1', 'active'], + ['2', 'idle'], + ], [5, 8] ); expect(table).toContain('ID'); @@ -587,7 +605,7 @@ describe('CLI Output Formatting', () => { }); it('should format normal costs with 2 decimals', () => { - expect(formatCost(1.50)).toBe('$1.50'); + expect(formatCost(1.5)).toBe('$1.50'); expect(formatCost(0.05)).toBe('$0.05'); }); @@ -633,7 +651,7 @@ describe('CLI Output Formatting', () => { describe('List Formatting', () => { const formatList = (items: string[], bullet: string = '-'): string => { - return items.map(item => `${bullet} ${item}`).join('\n'); + return items.map((item) => `${bullet} ${item}`).join('\n'); }; const formatNumberedList = (items: string[]): string => { From dea015dc91083ca46afe830a2d534251ef347410 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Sat, 6 Jun 2026 23:55:49 -0400 Subject: [PATCH 2/5] COD-2 scope downloads to session workspace --- src/web/routes/file-routes.ts | 107 +++++++++++++++++++++++++++++++- test/routes/file-routes.test.ts | 59 ++++++++++++++++++ 2 files changed, 165 insertions(+), 1 deletion(-) diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index c9b495c1..d27418ef 100644 --- a/src/web/routes/file-routes.ts +++ b/src/web/routes/file-routes.ts @@ -4,7 +4,8 @@ */ import { FastifyInstance } from 'fastify'; -import { join } from 'node:path'; +import { basename as pathBasename, join } from 'node:path'; +import { homedir } from 'node:os'; import fs from 'node:fs/promises'; import { ApiErrorCode, createErrorResponse, getErrorMessage } from '../../types.js'; import { fileStreamManager } from '../../file-stream-manager.js'; @@ -380,4 +381,108 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort): void const closed = fileStreamManager.closeStream(streamId); return { success: closed }; }); + // Session-scoped file download. + // Uses the same realpath-based workspace boundary as file preview/raw routes; + // the sensitive-path blocklist remains defense-in-depth, not the primary boundary. + const SENSITIVE_PATTERNS: RegExp[] = [ + /^\/etc\/shadow$/, + /^\/etc\/gshadow$/, + /^\/etc\/master\.passwd$/, + new RegExp(`^${homedir().replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}\\/\\.ssh\\/`), + /\/\.env$/, + /\/\.env\./, + /\/credentials(\.json|\.yml|\.yaml|\.xml)?$/i, + /\/\.aws\/credentials$/, + /\/\.gcloud\/credentials\.db$/, + /\/\.docker\/config\.json$/, + ]; + + function isSensitivePath(absPath: string): boolean { + return SENSITIVE_PATTERNS.some((pattern) => pattern.test(absPath)); + } + + app.get('/api/download', async (req, reply) => { + const { path: filePath, sessionId } = req.query as { path?: string; sessionId?: string }; + + if (!filePath) { + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Missing path parameter')); + return; + } + + if (!sessionId) { + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Missing sessionId parameter')); + return; + } + + const session = findSessionOrFail(ctx, sessionId); + const validated = validateSessionFilePath(session.workingDir, filePath); + if (!validated) { + reply.code(404).send(createErrorResponse(ApiErrorCode.NOT_FOUND, 'File not found')); + return; + } + const { resolvedPath } = validated; + + // Check sensitive path blocklist + if (isSensitivePath(resolvedPath)) { + reply.code(403).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Access to this file is blocked')); + return; + } + + try { + const stat = await fs.stat(resolvedPath); + + if (!stat.isFile()) { + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Path is not a file')); + return; + } + + // 50MB size limit + const MAX_DOWNLOAD_SIZE = 50 * 1024 * 1024; + if (stat.size > MAX_DOWNLOAD_SIZE) { + reply + .code(400) + .send( + createErrorResponse( + ApiErrorCode.INVALID_INPUT, + `File too large (${Math.round(stat.size / 1024 / 1024)}MB > 50MB limit)` + ) + ); + return; + } + + const ext = filePath.split('.').pop()?.toLowerCase() || ''; + const mimeTypes: Record = { + png: 'image/png', + jpg: 'image/jpeg', + jpeg: 'image/jpeg', + gif: 'image/gif', + webp: 'image/webp', + svg: 'image/svg+xml', + pdf: 'application/pdf', + json: 'application/json', + txt: 'text/plain', + md: 'text/markdown', + csv: 'text/csv', + xml: 'application/xml', + zip: 'application/zip', + gz: 'application/gzip', + tar: 'application/x-tar', + }; + + const filename = pathBasename(resolvedPath); + const content = await fs.readFile(resolvedPath); + // Bypass Fastify compression — write directly to raw response + reply.raw.writeHead(200, { + 'Content-Type': mimeTypes[ext] || 'application/octet-stream', + 'Content-Disposition': `attachment; filename="${filename}"`, + 'Content-Length': content.length, + }); + reply.raw.end(content); + return; + } catch (err) { + reply + .code(500) + .send(createErrorResponse(ApiErrorCode.OPERATION_FAILED, `Failed to read file: ${getErrorMessage(err)}`)); + } + }); } diff --git a/test/routes/file-routes.test.ts b/test/routes/file-routes.test.ts index bb657733..d0f9a6f4 100644 --- a/test/routes/file-routes.test.ts +++ b/test/routes/file-routes.test.ts @@ -363,4 +363,63 @@ describe('file-routes', () => { expect(body.success).toBe(false); }); }); + + // ========== GET /api/download ========== + + describe('GET /api/download', () => { + it('requires a sessionId to scope downloads', async () => { + const res = await harness.app.inject({ + method: 'GET', + url: `/api/download?path=${encodeURIComponent('/tmp/test-workdir/report.txt')}`, + }); + + expect(res.statusCode).toBe(400); + }); + + it('downloads files scoped to the session working directory', async () => { + const content = Buffer.from('download content'); + mockedReadFile.mockResolvedValue(content as never); + mockedStat.mockResolvedValue({ size: content.length, isFile: () => true } as never); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/download?sessionId=${harness.ctx._sessionId}&path=report.txt`, + }); + + expect(res.statusCode).toBe(200); + expect(res.headers['content-disposition']).toContain('filename="report.txt"'); + expect(res.body).toBe('download content'); + }); + + it('rejects absolute paths outside the session working directory', async () => { + const res = await harness.app.inject({ + method: 'GET', + url: `/api/download?sessionId=${harness.ctx._sessionId}&path=${encodeURIComponent('/var/log/app.log')}`, + }); + + expect(res.statusCode).toBe(404); + }); + + it('rejects symlink targets that escape the session working directory', async () => { + mockedRealpathSync.mockReturnValue('/tmp/outside-workdir/link.log' as never); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/download?sessionId=${harness.ctx._sessionId}&path=${encodeURIComponent( + '/tmp/test-workdir/link.log' + )}`, + }); + + expect(res.statusCode).toBe(404); + }); + + it('blocks sensitive files even when they are inside the session working directory', async () => { + const res = await harness.app.inject({ + method: 'GET', + url: `/api/download?sessionId=${harness.ctx._sessionId}&path=.env`, + }); + + expect(res.statusCode).toBe(403); + }); + }); }); From a36543c1b90bd8e9181bf8b7aa2c46d47eed4c23 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Mon, 8 Jun 2026 10:09:27 -0400 Subject: [PATCH 3/5] fix: COD-29 harden downloads and extract auth policy --- src/web/network-auth-policy.ts | 21 +++++++++++++++++++++ src/web/routes/file-routes.ts | 13 +++++++------ src/web/server.ts | 22 +--------------------- test/network-auth-policy.test.ts | 26 ++++++++++++++++++++++++++ test/routes/file-routes.test.ts | 16 ++++++++++++++++ 5 files changed, 71 insertions(+), 27 deletions(-) create mode 100644 src/web/network-auth-policy.ts create mode 100644 test/network-auth-policy.test.ts diff --git a/src/web/network-auth-policy.ts b/src/web/network-auth-policy.ts new file mode 100644 index 00000000..161a263e --- /dev/null +++ b/src/web/network-auth-policy.ts @@ -0,0 +1,21 @@ +import { isIP } from 'node:net'; + +const EXPLICIT_TRUE_VALUES = new Set(['1', 'true', 'yes', 'on']); + +export function isExplicitlyEnabled(value: string | undefined): boolean { + return value !== undefined && EXPLICIT_TRUE_VALUES.has(value.trim().toLowerCase()); +} + +export function isLoopbackBindHost(host: string): boolean { + const normalized = host + .trim() + .toLowerCase() + .replace(/^\[(.*)\]$/, '$1'); + if (normalized === 'localhost' || normalized === '::1' || normalized === '0:0:0:0:0:0:0:1') { + return true; + } + if (isIP(normalized) === 4 && normalized.startsWith('127.')) { + return true; + } + return normalized.startsWith('::ffff:127.'); +} diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index d27418ef..ef9a9842 100644 --- a/src/web/routes/file-routes.ts +++ b/src/web/routes/file-routes.ts @@ -279,7 +279,6 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort): void jpeg: 'image/jpeg', gif: 'image/gif', webp: 'image/webp', - svg: 'image/svg+xml', ico: 'image/x-icon', bmp: 'image/bmp', mp4: 'video/mp4', @@ -293,19 +292,21 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort): void }; const content = await fs.readFile(resolvedPath); - if (download === 'true') { - const rawBasename = filePath!.split('/').pop() || 'download'; - // Sanitize filename for Content-Disposition header (prevent header injection) - const basename = rawBasename.replace(/["\\\r\n]/g, '_'); + const rawBasename = filePath!.split('/').pop() || 'download'; + // Sanitize filename for Content-Disposition header (prevent header injection) + const basename = rawBasename.replace(/["\\\r\n]/g, '_'); + if (download === 'true' || ext === 'svg') { reply.raw.writeHead(200, { - 'Content-Type': mimeTypes[ext] || 'application/octet-stream', + 'Content-Type': ext === 'svg' ? 'application/octet-stream' : mimeTypes[ext] || 'application/octet-stream', 'Content-Disposition': `attachment; filename="${basename}"`, 'Content-Length': content.length, + 'X-Content-Type-Options': 'nosniff', }); reply.raw.end(content); return; } reply.header('Content-Type', mimeTypes[ext] || 'application/octet-stream'); + reply.header('X-Content-Type-Options', 'nosniff'); reply.send(content); } catch (err) { reply diff --git a/src/web/server.ts b/src/web/server.ts index 95eebb90..1d624444 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -42,7 +42,6 @@ import { execSync } from 'node:child_process'; import { hostname as getHostname } from 'node:os'; import { dataPath } from '../config/instance.js'; import { EventEmitter } from 'node:events'; -import { isIP } from 'node:net'; import { Session, type BackgroundTask } from '../session.js'; import type { ClaudeMode, SessionState } from '../types.js'; import { RespawnController, RespawnConfig } from '../respawn-controller.js'; @@ -103,6 +102,7 @@ import { SseEvent } from './sse-events.js'; import type { ScheduledRun } from './ports/index.js'; import { registerAuthMiddleware, registerSecurityHeaders } from './middleware/auth.js'; import { installRouteErrorHandler } from './route-error-handler.js'; +import { isExplicitlyEnabled, isLoopbackBindHost } from './network-auth-policy.js'; import { registerPushRoutes, registerTeamRoutes, @@ -123,26 +123,6 @@ import { const __dirname = dirname(fileURLToPath(import.meta.url)); -const EXPLICIT_TRUE_VALUES = new Set(['1', 'true', 'yes', 'on']); - -function isExplicitlyEnabled(value: string | undefined): boolean { - return value !== undefined && EXPLICIT_TRUE_VALUES.has(value.trim().toLowerCase()); -} - -function isLoopbackBindHost(host: string): boolean { - const normalized = host - .trim() - .toLowerCase() - .replace(/^\[(.*)\]$/, '$1'); - if (normalized === 'localhost' || normalized === '::1' || normalized === '0:0:0:0:0:0:0:1') { - return true; - } - if (isIP(normalized) === 4 && normalized.startsWith('127.')) { - return true; - } - return normalized.startsWith('::ffff:127.'); -} - // Bounded, predictable shape for SSE client identifiers: alphanumerics, `_`, `-`. // Length range covers crypto.randomUUID() (36 chars) plus any short stable IDs, // while capping growth of `sseClientsById` and blocking pathological inputs. diff --git a/test/network-auth-policy.test.ts b/test/network-auth-policy.test.ts new file mode 100644 index 00000000..143c34a4 --- /dev/null +++ b/test/network-auth-policy.test.ts @@ -0,0 +1,26 @@ +import { describe, expect, it } from 'vitest'; +import { isExplicitlyEnabled, isLoopbackBindHost } from '../src/web/network-auth-policy.js'; + +describe('network auth policy', () => { + it.each(['localhost', '127.0.0.1', '127.42.0.9', '::1', '[::1]', '0:0:0:0:0:0:0:1', '::ffff:127.0.0.1'])( + 'treats %s as loopback', + (host) => { + expect(isLoopbackBindHost(host)).toBe(true); + } + ); + + it.each(['0.0.0.0', '192.168.1.10', '10.0.0.1', 'example.com', '::', '[::]', '::ffff:192.168.1.10'])( + 'treats %s as non-loopback', + (host) => { + expect(isLoopbackBindHost(host)).toBe(false); + } + ); + + it.each(['1', 'true', 'TRUE', ' yes ', 'on'])('treats %s as an explicit opt-in', (value) => { + expect(isExplicitlyEnabled(value)).toBe(true); + }); + + it.each([undefined, '', '0', 'false', 'no', 'off', 'enabled'])('does not treat %s as an explicit opt-in', (value) => { + expect(isExplicitlyEnabled(value)).toBe(false); + }); +}); diff --git a/test/routes/file-routes.test.ts b/test/routes/file-routes.test.ts index d0f9a6f4..903dc7f3 100644 --- a/test/routes/file-routes.test.ts +++ b/test/routes/file-routes.test.ts @@ -305,6 +305,22 @@ describe('file-routes', () => { expect(res.headers['content-type']).toBe('image/png'); }); + it('serves workspace SVG as an untrusted attachment instead of inline image/svg+xml', async () => { + const content = Buffer.from(''); + mockedReadFile.mockResolvedValue(content as never); + mockedStat.mockResolvedValue({ size: content.length } as never); + + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${harness.ctx._sessionId}/file-raw?path=malicious.svg`, + }); + + expect(res.statusCode).toBe(200); + expect(res.headers['content-type']).toBe('application/octet-stream'); + expect(res.headers['content-disposition']).toContain('attachment; filename="malicious.svg"'); + expect(res.headers['x-content-type-options']).toBe('nosniff'); + }); + it('rejects path traversal in raw file serving', async () => { mockedRealpathSync.mockReturnValue('/etc/shadow' as never); From da00fa6038fa455993080d614c8c963f6fe5a555 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Mon, 8 Jun 2026 10:10:20 -0400 Subject: [PATCH 4/5] fix: COD-29 relax auth lockout recovery --- src/web/middleware/auth.ts | 23 ++++++--- test/auth-security.test.ts | 101 ++++++++++++++++++++++++++++++------- 2 files changed, 99 insertions(+), 25 deletions(-) diff --git a/src/web/middleware/auth.ts b/src/web/middleware/auth.ts index 6b372ca5..ee60915f 100644 --- a/src/web/middleware/auth.ts +++ b/src/web/middleware/auth.ts @@ -8,7 +8,7 @@ * - CORS (localhost only) */ -import { FastifyInstance } from 'fastify'; +import type { FastifyInstance, FastifyReply } from 'fastify'; import { randomBytes, timingSafeEqual } from 'node:crypto'; import { StaleExpirationMap } from '../../utils/index.js'; import type { AuthSessionRecord } from '../ports/auth-port.js'; @@ -69,6 +69,13 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean): Au const authSessions = state.authSessions; const authFailures = state.authFailures; + function sendAuthRateLimit(reply: FastifyReply, clientIp: string): void { + const remainingMs = authFailures.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'); + } + 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. @@ -90,13 +97,6 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean): Au const clientIp = req.ip; - // Rate limit: reject if too many failed attempts from this IP - const failures = authFailures.get(clientIp) ?? 0; - if (failures >= AUTH_FAILURE_MAX) { - reply.code(429).send('Too Many Requests — try again later'); - return; - } - // Check session cookie first (avoids re-sending credentials on every request) // Use get() instead of has() so refreshOnGet extends the TTL on active sessions const sessionToken = req.cookies[AUTH_COOKIE_NAME]; @@ -140,6 +140,13 @@ export function registerAuthMiddleware(app: FastifyInstance, https: boolean): Au return; } + // Rate limit only requests that failed to authenticate on this attempt. + const failures = authFailures.get(clientIp) ?? 0; + if (failures >= AUTH_FAILURE_MAX) { + sendAuthRateLimit(reply, clientIp); + return; + } + // Auth failed — track failure count authFailures.set(clientIp, failures + 1); diff --git a/test/auth-security.test.ts b/test/auth-security.test.ts index 70a0241c..b4387f22 100644 --- a/test/auth-security.test.ts +++ b/test/auth-security.test.ts @@ -10,20 +10,53 @@ * * Port: 3160 (auth tests), 3161 (loopback no-auth tests), 3162 (network override tests) */ -import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import { describe, it, expect, beforeAll, afterAll, beforeEach, afterEach, vi } from 'vitest'; import { WebServer } from '../src/web/server.js'; +import { TmuxManager } from '../src/tmux-manager.js'; import { SettingsUpdateSchema } from '../src/web/schemas.js'; const AUTH_PORT = 3160; const NOAUTH_PORT = 3161; const NETWORK_OVERRIDE_PORT = 3162; +const AUTH_RATE_LIMIT_PORT = 3220; const TEST_USER = 'admin'; const TEST_PASS = 'test-password-12345'; +vi.spyOn(TmuxManager, 'isTmuxAvailable').mockReturnValue(true); + function basicAuthHeader(user: string, pass: string): string { return 'Basic ' + Buffer.from(`${user}:${pass}`).toString('base64'); } +async function startAuthServer(port: number): Promise<{ server: WebServer; baseUrl: string }> { + process.env.CODEMAN_PASSWORD = TEST_PASS; + process.env.CODEMAN_USERNAME = TEST_USER; + const server = new WebServer(port, false, true); + await server.start(); + return { server, baseUrl: `http://localhost:${port}` }; +} + +async function getSessionCookie(baseUrl: string): Promise { + const res = await fetch(`${baseUrl}/api/status`, { + headers: { Authorization: basicAuthHeader(TEST_USER, TEST_PASS) }, + }); + expect(res.status).toBe(200); + const setCookie = res.headers.get('set-cookie'); + expect(setCookie).toBeTruthy(); + const cookieMatch = setCookie!.match(/codeman_session=([^;]+)/); + expect(cookieMatch).toBeTruthy(); + return `codeman_session=${cookieMatch![1]}`; +} + +async function exhaustAuthFailures(baseUrl: string, prefix: string): Promise { + for (let i = 0; i < 10; i++) { + const res = await fetch(`${baseUrl}/api/status`, { + headers: { Authorization: basicAuthHeader(TEST_USER, `${prefix}-${i}`) }, + }); + expect(res.status).toBe(401); + } +} + describe('Auth Security', () => { let server: WebServer; let baseUrl: string; @@ -155,29 +188,63 @@ describe('Auth Security', () => { }); describe('Rate Limiting', () => { - it('should block after too many failed attempts', async () => { - // Send 10 failed attempts - for (let i = 0; i < 10; i++) { - await fetch(`${baseUrl}/api/status`, { - headers: { Authorization: basicAuthHeader(TEST_USER, 'wrong-' + i) }, - }); - } - - // 11th attempt should be rate-limited - const res = await fetch(`${baseUrl}/api/status`, { + let rateServer: WebServer; + let rateBaseUrl: string; + + beforeEach(async () => { + ({ server: rateServer, baseUrl: rateBaseUrl } = await startAuthServer(AUTH_RATE_LIMIT_PORT)); + }); + + afterEach(async () => { + await rateServer.stop(); + }); + + it('should rate-limit wrong credentials after too many failed attempts', async () => { + await exhaustAuthFailures(rateBaseUrl, 'cod21-wrong'); + + const res = await fetch(`${rateBaseUrl}/api/status`, { headers: { Authorization: basicAuthHeader(TEST_USER, 'wrong-again') }, }); + expect(res.status).toBe(429); + expect(res.headers.get('retry-after')).toMatch(/^\d+$/); }); - it('should rate-limit even with correct credentials after lockout', async () => { - // After being rate-limited, even correct credentials should fail - const res = await fetch(`${baseUrl}/api/status`, { + it('should allow an existing valid session cookie during auth failure lockout', async () => { + const cookie = await getSessionCookie(rateBaseUrl); + await exhaustAuthFailures(rateBaseUrl, 'cod21-cookie'); + + const res = await fetch(`${rateBaseUrl}/api/status`, { + headers: { Cookie: cookie }, + }); + + expect(res.status).toBe(200); + }); + + it('should allow correct credentials to recover from auth failure lockout', async () => { + await exhaustAuthFailures(rateBaseUrl, 'cod21-recover'); + + const res = await fetch(`${rateBaseUrl}/api/status`, { headers: { Authorization: basicAuthHeader(TEST_USER, TEST_PASS) }, }); - // Rate limit is per-IP and the previous test used the same IP - // This test verifies rate limiting isn't bypassed by correct creds - expect(res.status).toBe(429); + + expect(res.status).toBe(200); + expect(res.headers.get('set-cookie')).toContain('codeman_session='); + }); + + it('should clear failed attempt count after correct credentials recover access', async () => { + await exhaustAuthFailures(rateBaseUrl, 'cod21-clear'); + + const recoveryRes = await fetch(`${rateBaseUrl}/api/status`, { + headers: { Authorization: basicAuthHeader(TEST_USER, TEST_PASS) }, + }); + expect(recoveryRes.status).toBe(200); + + const wrongAfterRecovery = await fetch(`${rateBaseUrl}/api/status`, { + headers: { Authorization: basicAuthHeader(TEST_USER, 'wrong-after-recovery') }, + }); + + expect(wrongAfterRecovery.status).toBe(401); }); }); From 6ee88be5499a9ba4d0b63b09996f89d701d65b60 Mon Sep 17 00:00:00 2001 From: arkon Date: Mon, 8 Jun 2026 18:01:20 +0200 Subject: [PATCH 5/5] test: fix title tests for new host constructor arg + async renderIndexHtml The WebServer constructor now takes `host` as the 4th positional arg (titleHostname shifted to 5th), and renderIndexHtml became async (it reads settings.json for the gesture bundle) and cache-busts asset URLs. Update the two title tests accordingly: - pass '127.0.0.1' as the bind host so the title value lands in the 5th titleHostname slot (server-index-title + push-payload-host-title) - await renderIndexHtml and make the cases async - strip ?v= cache-bust params before the byte-identical assertion Co-Authored-By: Claude Opus 4.8 (1M context) --- test/push-payload-host-title.test.ts | 3 +- test/server-index-title.test.ts | 44 +++++++++++++++------------- 2 files changed, 26 insertions(+), 21 deletions(-) diff --git a/test/push-payload-host-title.test.ts b/test/push-payload-host-title.test.ts index 2ce1e7f5..b5c39d85 100644 --- a/test/push-payload-host-title.test.ts +++ b/test/push-payload-host-title.test.ts @@ -47,7 +47,8 @@ interface PushPayload { function makeServerWithHost(host: string): WebServer { // Constructor only assigns fields — no network/disk activity until start(). - const server = new WebServer(0, false, true, host); + // 4th arg is the bind host; the title hostname is the 5th arg. + const server = new WebServer(0, false, true, '127.0.0.1', host); // Stub push store: one subscription with all events enabled. const fakeSub = { endpoint: 'https://push.example.com/abc', diff --git a/test/server-index-title.test.ts b/test/server-index-title.test.ts index 85deba6c..76cd4f02 100644 --- a/test/server-index-title.test.ts +++ b/test/server-index-title.test.ts @@ -32,36 +32,38 @@ const __dirname = dirname(fileURLToPath(import.meta.url)); const indexHtmlPath = join(__dirname, '..', 'src', 'web', 'public', 'index.html'); const rawTemplate = readFileSync(indexHtmlPath, 'utf-8'); -function render(host?: string): string { - const server = new WebServer(0, false, true, host); - return (server as unknown as { renderIndexHtml: () => string }).renderIndexHtml(); +async function render(host?: string): Promise { + // 4th arg is the bind host; the title hostname is the 5th arg. + const server = new WebServer(0, false, true, '127.0.0.1', host); + // renderIndexHtml is async (it reads settings.json for the gesture bundle). + return (server as unknown as { renderIndexHtml: () => Promise }).renderIndexHtml(); } describe('WebServer index.html templating (#82)', () => { - it('substitutes the bare <title>Codeman with codeman:', () => { - const html = render('laptop'); + it('substitutes the bare Codeman with codeman:', async () => { + const html = await render('laptop'); expect(html).toContain('codeman:laptop'); expect(html).not.toContain('Codeman'); }); - it('defaults to os.hostname() when no titleHostname is supplied', () => { - const html = render(); + it('defaults to os.hostname() when no titleHostname is supplied', async () => { + const html = await render(); const expected = `codeman:${osHostname()}`; expect(html).toContain(expected); }); - it('treats an empty-string titleHostname as "not supplied" and falls back to os.hostname()', () => { + it('treats an empty-string titleHostname as "not supplied" and falls back to os.hostname()', async () => { // CLI normally guarantees a non-empty string, but the constructor's // `titleHostname || getHostname()` guard makes empty fall through — // pin that behavior so a future refactor doesn't accidentally ship // a `codeman:` to users. - const html = render(''); + const html = await render(''); expect(html).toMatch(/codeman:.+<\/title>/); expect(html).not.toContain('<title>codeman:'); }); - it('HTML-escapes < > & in the hostname so it cannot break out of the title tag', () => { - const html = render(''); + it('HTML-escapes < > & in the hostname so it cannot break out of the title tag', async () => { + const html = await render(''); expect(html).toContain('codeman:<script>alert(1)</script>'); // The raw closing from the injected payload must NOT appear // outside the actual title element — escape-then-substitute prevents @@ -69,16 +71,18 @@ describe('WebServer index.html templating (#82)', () => { expect(html).not.toContain('<script>alert(1)</script>'); }); - it('escapes an ampersand without double-encoding existing entities', () => { + it('escapes an ampersand without double-encoding existing entities', async () => { // The escaper replaces & first, then < and >. A hostname that already // contains a literal `&` should render as `&` once, not `&amp;`. - const html = render('a&b'); + const html = await render('a&b'); expect(html).toContain('codeman:a&b'); expect(html).not.toContain('&amp;'); }); - it('only substitutes the tag — the rest of the template is byte-for-byte identical', () => { - const html = render('laptop'); + it('only substitutes the <title> tag — the rest of the template is identical (modulo asset cache-busting)', async () => { + // renderIndexHtml also appends ?v=<mtime> cache-bust params to same-origin + // .js/.css refs; strip them so the title remains the only other change. + const html = (await render('laptop')).replace(/(\.(?:js|css))\?v=[^"]*/g, '$1'); const beforeTitle = rawTemplate.split('<title>Codeman')[0]; const afterTitle = rawTemplate.split('Codeman')[1]; expect(html.startsWith(beforeTitle)).toBe(true); @@ -88,8 +92,8 @@ describe('WebServer index.html templating (#82)', () => { expect(html.length - rawTemplate.length).toBe(expectedDelta); }); - it('replaces the <title> placeholder exactly once', () => { - const html = render('laptop'); + it('replaces the <title> placeholder exactly once', async () => { + const html = await render('laptop'); // Defense against a future regression where the template gains a // second `<title>Codeman` (e.g. inside a