From 87cfcc5b3cbecfdccaa56c1a02b24fb8b84eb654 Mon Sep 17 00:00:00 2001 From: kaitranntt Date: Thu, 8 Jan 2026 09:15:42 -0500 Subject: [PATCH] fix(codex-proxy): security hardening and edge case fixes - Fix path traversal in trace() via safe dir validation - Add RFC 7230 hop-by-hop header filtering - Fix isRecord() to exclude arrays - Add req.destroy() on oversized body rejection - Add missing ANTHROPIC_BASE_URL warning for Codex - Use cwd() fallback when HOME undefined - Add 4 edge case unit tests --- src/cliproxy/cliproxy-executor.ts | 62 ++++++++++--------- src/cliproxy/codex-reasoning-proxy.ts | 36 +++++++++-- .../cliproxy/codex-reasoning-proxy.test.js | 20 ++++++ 3 files changed, 83 insertions(+), 35 deletions(-) diff --git a/src/cliproxy/cliproxy-executor.ts b/src/cliproxy/cliproxy-executor.ts index 03241737..03c8c04f 100644 --- a/src/cliproxy/cliproxy-executor.ts +++ b/src/cliproxy/cliproxy-executor.ts @@ -712,35 +712,39 @@ export async function execClaudeWithCLIProxy( // - Unknown → medium let codexReasoningProxy: CodexReasoningProxy | null = null; let codexReasoningPort: number | null = null; - if (provider === 'codex' && envVars.ANTHROPIC_BASE_URL) { - try { - const traceEnabled = - process.env.CCS_CODEX_REASONING_TRACE === '1' || - process.env.CCS_CODEX_REASONING_TRACE === 'true'; - codexReasoningProxy = new CodexReasoningProxy({ - upstreamBaseUrl: envVars.ANTHROPIC_BASE_URL, - verbose, - defaultEffort: 'medium', - traceFilePath: traceEnabled - ? `${process.env.HOME || ''}/.ccs/codex-reasoning-proxy.log` - : '', - modelMap: { - defaultModel: envVars.ANTHROPIC_MODEL, - opusModel: envVars.ANTHROPIC_DEFAULT_OPUS_MODEL, - sonnetModel: envVars.ANTHROPIC_DEFAULT_SONNET_MODEL, - haikuModel: envVars.ANTHROPIC_DEFAULT_HAIKU_MODEL, - }, - }); - codexReasoningPort = await codexReasoningProxy.start(); - log( - `Codex reasoning proxy active: http://127.0.0.1:${codexReasoningPort}/api/provider/codex` - ); - } catch (error) { - const err = error as Error; - codexReasoningProxy = null; - codexReasoningPort = null; - if (verbose) { - console.error(warn(`Codex reasoning proxy disabled: ${err.message}`)); + if (provider === 'codex') { + if (!envVars.ANTHROPIC_BASE_URL) { + log('ANTHROPIC_BASE_URL not set for Codex, reasoning proxy disabled'); + } else { + try { + const traceEnabled = + process.env.CCS_CODEX_REASONING_TRACE === '1' || + process.env.CCS_CODEX_REASONING_TRACE === 'true'; + codexReasoningProxy = new CodexReasoningProxy({ + upstreamBaseUrl: envVars.ANTHROPIC_BASE_URL, + verbose, + defaultEffort: 'medium', + traceFilePath: traceEnabled + ? `${process.env.HOME || process.cwd()}/.ccs/codex-reasoning-proxy.log` + : '', + modelMap: { + defaultModel: envVars.ANTHROPIC_MODEL, + opusModel: envVars.ANTHROPIC_DEFAULT_OPUS_MODEL, + sonnetModel: envVars.ANTHROPIC_DEFAULT_SONNET_MODEL, + haikuModel: envVars.ANTHROPIC_DEFAULT_HAIKU_MODEL, + }, + }); + codexReasoningPort = await codexReasoningProxy.start(); + log( + `Codex reasoning proxy active: http://127.0.0.1:${codexReasoningPort}/api/provider/codex` + ); + } catch (error) { + const err = error as Error; + codexReasoningProxy = null; + codexReasoningPort = null; + if (verbose) { + console.error(warn(`Codex reasoning proxy disabled: ${err.message}`)); + } } } } diff --git a/src/cliproxy/codex-reasoning-proxy.ts b/src/cliproxy/codex-reasoning-proxy.ts index 657359ad..6412c93d 100644 --- a/src/cliproxy/codex-reasoning-proxy.ts +++ b/src/cliproxy/codex-reasoning-proxy.ts @@ -24,7 +24,7 @@ function isNonEmptyString(value: unknown): value is string { } function isRecord(value: unknown): value is Record { - return typeof value === 'object' && value !== null; + return typeof value === 'object' && value !== null && !Array.isArray(value); } function parseModelEffortSuffix( @@ -141,13 +141,22 @@ export class CodexReasoningProxy { if (!this.config.traceFilePath) return; try { // Intentionally best-effort: tracing must never break requests. - const fs = require('fs') as typeof import('fs'); + const pathMod = require('path') as typeof import('path'); - const path = require('path') as typeof import('path'); - const dir = path.dirname(this.config.traceFilePath); + // Security: validate trace path against safe directories to prevent path traversal + const resolvedPath = pathMod.resolve(this.config.traceFilePath); + const home = process.env.HOME || process.env.USERPROFILE || ''; + const safeDirs = ['/tmp/', '/var/log/', home ? `${home}/.ccs/` : '']; + const isSafe = safeDirs.some((base) => base && resolvedPath.startsWith(base)); + if (!isSafe) { + this.log(`[SECURITY] Trace path rejected (outside allowed dirs): ${resolvedPath}`); + return; + } + + const dir = pathMod.dirname(resolvedPath); fs.mkdirSync(dir, { recursive: true }); - fs.appendFileSync(this.config.traceFilePath, line + '\n'); + fs.appendFileSync(resolvedPath, line + '\n'); } catch { // ignore } @@ -198,6 +207,7 @@ export class CodexReasoningProxy { req.on('data', (chunk: Buffer) => { total += chunk.length; if (total > maxSize) { + req.destroy(); // Signal client to stop sending reject(new Error('Request body too large (max 10MB)')); return; } @@ -293,10 +303,24 @@ export class CodexReasoningProxy { ): http.OutgoingHttpHeaders { const headers: http.OutgoingHttpHeaders = {}; + // RFC 7230 hop-by-hop headers that should not be forwarded + const hopByHop = new Set([ + 'host', + 'content-length', + 'connection', + 'transfer-encoding', + 'keep-alive', + 'proxy-authenticate', + 'proxy-authorization', + 'te', + 'trailer', + 'upgrade', + ]); + for (const [key, value] of Object.entries(originalHeaders)) { if (!value) continue; const lower = key.toLowerCase(); - if (lower === 'host' || lower === 'content-length') continue; + if (hopByHop.has(lower)) continue; headers[key] = value; } diff --git a/tests/unit/cliproxy/codex-reasoning-proxy.test.js b/tests/unit/cliproxy/codex-reasoning-proxy.test.js index 9322f709..a9294ae7 100644 --- a/tests/unit/cliproxy/codex-reasoning-proxy.test.js +++ b/tests/unit/cliproxy/codex-reasoning-proxy.test.js @@ -30,6 +30,16 @@ describe('Codex Reasoning Proxy', () => { assert.strictEqual(map.get('same'), 'medium'); }); + + it('returns empty map when all models undefined', () => { + const map = buildCodexModelEffortMap({}); + assert.strictEqual(map.size, 0); + }); + + it('ignores empty string models', () => { + const map = buildCodexModelEffortMap({ defaultModel: '', opusModel: ' ' }); + assert.strictEqual(map.size, 0); + }); }); describe('getEffortForModel', () => { @@ -37,6 +47,11 @@ describe('Codex Reasoning Proxy', () => { const map = buildCodexModelEffortMap({ opusModel: 'm1' }); assert.strictEqual(getEffortForModel('unknown', map, 'medium'), 'medium'); }); + + it('handles null model', () => { + const map = buildCodexModelEffortMap({ opusModel: 'm1' }); + assert.strictEqual(getEffortForModel(null, map, 'high'), 'high'); + }); }); describe('injectReasoningEffortIntoBody', () => { @@ -51,6 +66,11 @@ describe('Codex Reasoning Proxy', () => { it('leaves non-object bodies unchanged', () => { assert.strictEqual(injectReasoningEffortIntoBody('x', 'medium'), 'x'); }); + + it('leaves array bodies unchanged (not treated as record)', () => { + const arr = [1, 2, 3]; + assert.deepStrictEqual(injectReasoningEffortIntoBody(arr, 'high'), arr); + }); }); describe('model suffix aliasing', () => {