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
This commit is contained in:
kaitranntt committed 2026-01-08 09:15:42 -05:00
1 parent 204eea00ce
commit 87cfcc5b3c
3 files changed
+56 -8

No files matched your search

+6 -2
View File
@@ -712,7 +712,10 @@ export async function execClaudeWithCLIProxy(
// - Unknown → medium // - Unknown → medium
let codexReasoningProxy: CodexReasoningProxy | null = null; let codexReasoningProxy: CodexReasoningProxy | null = null;
let codexReasoningPort: number | null = null; let codexReasoningPort: number | null = null;
if (provider === 'codex' && envVars.ANTHROPIC_BASE_URL) { if (provider === 'codex') {
if (!envVars.ANTHROPIC_BASE_URL) {
log('ANTHROPIC_BASE_URL not set for Codex, reasoning proxy disabled');
} else {
try { try {
const traceEnabled = const traceEnabled =
process.env.CCS_CODEX_REASONING_TRACE === '1' || process.env.CCS_CODEX_REASONING_TRACE === '1' ||
@@ -722,7 +725,7 @@ export async function execClaudeWithCLIProxy(
verbose, verbose,
defaultEffort: 'medium', defaultEffort: 'medium',
traceFilePath: traceEnabled traceFilePath: traceEnabled
? `${process.env.HOME || ''}/.ccs/codex-reasoning-proxy.log` ? `${process.env.HOME || process.cwd()}/.ccs/codex-reasoning-proxy.log`
: '', : '',
modelMap: { modelMap: {
defaultModel: envVars.ANTHROPIC_MODEL, defaultModel: envVars.ANTHROPIC_MODEL,
@@ -744,6 +747,7 @@ export async function execClaudeWithCLIProxy(
} }
} }
} }
}
const effectiveEnvVars = const effectiveEnvVars =
codexReasoningProxy && codexReasoningPort codexReasoningProxy && codexReasoningPort
+30 -6
View File
@@ -24,7 +24,7 @@ function isNonEmptyString(value: unknown): value is string {
} }
function isRecord(value: unknown): value is Record<string, unknown> { function isRecord(value: unknown): value is Record<string, unknown> {
return typeof value === 'object' && value !== null; return typeof value === 'object' && value !== null && !Array.isArray(value);
} }
function parseModelEffortSuffix( function parseModelEffortSuffix(
@@ -141,13 +141,22 @@ export class CodexReasoningProxy {
if (!this.config.traceFilePath) return; if (!this.config.traceFilePath) return;
try { try {
// Intentionally best-effort: tracing must never break requests. // Intentionally best-effort: tracing must never break requests.
const fs = require('fs') as typeof import('fs'); const fs = require('fs') as typeof import('fs');
const pathMod = require('path') as typeof import('path');
const path = require('path') as typeof import('path'); // Security: validate trace path against safe directories to prevent path traversal
const dir = path.dirname(this.config.traceFilePath); 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.mkdirSync(dir, { recursive: true });
fs.appendFileSync(this.config.traceFilePath, line + '\n'); fs.appendFileSync(resolvedPath, line + '\n');
} catch { } catch {
// ignore // ignore
} }
@@ -198,6 +207,7 @@ export class CodexReasoningProxy {
req.on('data', (chunk: Buffer) => { req.on('data', (chunk: Buffer) => {
total += chunk.length; total += chunk.length;
if (total > maxSize) { if (total > maxSize) {
req.destroy(); // Signal client to stop sending
reject(new Error('Request body too large (max 10MB)')); reject(new Error('Request body too large (max 10MB)'));
return; return;
} }
@@ -293,10 +303,24 @@ export class CodexReasoningProxy {
): http.OutgoingHttpHeaders { ): http.OutgoingHttpHeaders {
const headers: 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)) { for (const [key, value] of Object.entries(originalHeaders)) {
if (!value) continue; if (!value) continue;
const lower = key.toLowerCase(); const lower = key.toLowerCase();
if (lower === 'host' || lower === 'content-length') continue; if (hopByHop.has(lower)) continue;
headers[key] = value; headers[key] = value;
} }
@@ -30,6 +30,16 @@ describe('Codex Reasoning Proxy', () => {
assert.strictEqual(map.get('same'), 'medium'); 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', () => { describe('getEffortForModel', () => {
@@ -37,6 +47,11 @@ describe('Codex Reasoning Proxy', () => {
const map = buildCodexModelEffortMap({ opusModel: 'm1' }); const map = buildCodexModelEffortMap({ opusModel: 'm1' });
assert.strictEqual(getEffortForModel('unknown', map, 'medium'), 'medium'); 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', () => { describe('injectReasoningEffortIntoBody', () => {
@@ -51,6 +66,11 @@ describe('Codex Reasoning Proxy', () => {
it('leaves non-object bodies unchanged', () => { it('leaves non-object bodies unchanged', () => {
assert.strictEqual(injectReasoningEffortIntoBody('x', 'medium'), 'x'); 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', () => { describe('model suffix aliasing', () => {