diff --git a/plugins/codex/scripts/codex-companion.mjs b/plugins/codex/scripts/codex-companion.mjs index 35222fd..c676467 100644 --- a/plugins/codex/scripts/codex-companion.mjs +++ b/plugins/codex/scripts/codex-companion.mjs @@ -22,7 +22,7 @@ import { } from "./lib/codex.mjs"; import { readStdinIfPiped } from "./lib/fs.mjs"; import { collectReviewContext, ensureGitRepository, resolveReviewTarget } from "./lib/git.mjs"; -import { binaryAvailable, terminateProcessTree } from "./lib/process.mjs"; +import { binaryAvailable, sanitizeChildEnv, terminateProcessTree } from "./lib/process.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { generateJobId, @@ -642,7 +642,7 @@ function spawnDetachedTaskWorker(cwd, jobId) { const scriptPath = path.join(ROOT_DIR, "scripts", "codex-companion.mjs"); const child = spawn(process.execPath, [scriptPath, "task-worker", "--cwd", cwd, "--job-id", jobId], { cwd, - env: process.env, + env: sanitizeChildEnv(process.env), detached: true, stdio: "ignore", windowsHide: true diff --git a/plugins/codex/scripts/lib/app-server.mjs b/plugins/codex/scripts/lib/app-server.mjs index 127c837..aa1cb80 100644 --- a/plugins/codex/scripts/lib/app-server.mjs +++ b/plugins/codex/scripts/lib/app-server.mjs @@ -14,7 +14,7 @@ import { spawn } from "node:child_process"; import readline from "node:readline"; import { parseBrokerEndpoint } from "./broker-endpoint.mjs"; import { ensureBrokerSession, loadBrokerSession } from "./broker-lifecycle.mjs"; -import { terminateProcessTree } from "./process.mjs"; +import { sanitizeChildEnv, terminateProcessTree } from "./process.mjs"; const PLUGIN_MANIFEST_URL = new URL("../../.claude-plugin/plugin.json", import.meta.url); const PLUGIN_MANIFEST = JSON.parse(fs.readFileSync(PLUGIN_MANIFEST_URL, "utf8")); @@ -188,7 +188,7 @@ class SpawnedCodexAppServerClient extends AppServerClientBase { async initialize() { this.proc = spawn("codex", ["app-server"], { cwd: this.cwd, - env: this.options.env ?? process.env, + env: sanitizeChildEnv(this.options.env ?? process.env), stdio: ["pipe", "pipe", "pipe"], shell: process.platform === "win32" ? (process.env.SHELL || true) : false, windowsHide: true diff --git a/plugins/codex/scripts/lib/broker-lifecycle.mjs b/plugins/codex/scripts/lib/broker-lifecycle.mjs index ef76381..f640991 100644 --- a/plugins/codex/scripts/lib/broker-lifecycle.mjs +++ b/plugins/codex/scripts/lib/broker-lifecycle.mjs @@ -6,6 +6,7 @@ import process from "node:process"; import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { createBrokerEndpoint, parseBrokerEndpoint } from "./broker-endpoint.mjs"; +import { sanitizeChildEnv } from "./process.mjs"; import { resolveStateDir } from "./state.mjs"; export const PID_FILE_ENV = "CODEX_COMPANION_APP_SERVER_PID_FILE"; @@ -60,7 +61,7 @@ export function spawnBrokerProcess({ scriptPath, cwd, endpoint, pidFile, logFile const logFd = fs.openSync(logFile, "a"); const child = spawn(process.execPath, [scriptPath, "serve", "--endpoint", endpoint, "--cwd", cwd, "--pid-file", pidFile], { cwd, - env, + env: sanitizeChildEnv(env), detached: true, stdio: ["ignore", logFd, logFd] }); diff --git a/plugins/codex/scripts/lib/process.mjs b/plugins/codex/scripts/lib/process.mjs index af28d1c..002630e 100644 --- a/plugins/codex/scripts/lib/process.mjs +++ b/plugins/codex/scripts/lib/process.mjs @@ -1,10 +1,32 @@ import { spawnSync } from "node:child_process"; import process from "node:process"; +export const ROUTING_INTENT_ENV = "RUST_VERIFICATION_PRESERVE_ROUTING_ENV"; +// Keep exact-match entries uppercase because keys are normalized before lookup. +const SCRUBBED_ENV_EXACT_KEYS = new Set([ + "RUST_VERIFICATION_ROOT_BASE", + "RUST_VERIFICATION_REAL_CARGO", + ROUTING_INTENT_ENV, + "BOLT_RUST_VERIFICATION_ROOT" +]); +const SCRUBBED_ENV_PATTERN = /^CARGO_.*TARGET_DIR$/; + +export function sanitizeChildEnv(env = process.env) { + const sourceEnv = env ?? process.env; + const childEnv = { ...sourceEnv }; + for (const key of Object.keys(childEnv)) { + const normalizedKey = key.toUpperCase(); + if (SCRUBBED_ENV_PATTERN.test(normalizedKey) || SCRUBBED_ENV_EXACT_KEYS.has(normalizedKey)) { + delete childEnv[key]; + } + } + return childEnv; +} + export function runCommand(command, args = [], options = {}) { const result = spawnSync(command, args, { cwd: options.cwd, - env: options.env, + env: sanitizeChildEnv(options.env ?? process.env), encoding: "utf8", input: options.input, maxBuffer: options.maxBuffer, diff --git a/plugins/codex/scripts/stop-review-gate-hook.mjs b/plugins/codex/scripts/stop-review-gate-hook.mjs index 2346bdc..cba5d45 100644 --- a/plugins/codex/scripts/stop-review-gate-hook.mjs +++ b/plugins/codex/scripts/stop-review-gate-hook.mjs @@ -7,6 +7,7 @@ import { spawnSync } from "node:child_process"; import { fileURLToPath } from "node:url"; import { getCodexAvailability } from "./lib/codex.mjs"; +import { sanitizeChildEnv } from "./lib/process.mjs"; import { loadPromptTemplate, interpolateTemplate } from "./lib/prompts.mjs"; import { getConfig, listJobs } from "./lib/state.mjs"; import { sortJobsNewestFirst } from "./lib/job-control.mjs"; @@ -99,7 +100,7 @@ function runStopReview(cwd, input = {}) { const scriptPath = path.join(SCRIPT_DIR, "codex-companion.mjs"); const prompt = buildStopReviewPrompt(input); const childEnv = { - ...process.env, + ...sanitizeChildEnv(process.env), ...(input.session_id ? { [SESSION_ID_ENV]: input.session_id } : {}) }; const result = spawnSync(process.execPath, [scriptPath, "task", "--json", prompt], { diff --git a/tests/commands.test.mjs b/tests/commands.test.mjs index ef5adb0..2b039bb 100644 --- a/tests/commands.test.mjs +++ b/tests/commands.test.mjs @@ -165,9 +165,9 @@ test("result and cancel commands are exposed as deterministic runtime entrypoint const resultHandling = read("skills/codex-result-handling/SKILL.md"); assert.match(result, /disable-model-invocation:\s*true/); - assert.match(result, /codex-companion\.mjs" result \$ARGUMENTS/); + assert.match(result, /codex-companion\.mjs" result "\$ARGUMENTS"/); assert.match(cancel, /disable-model-invocation:\s*true/); - assert.match(cancel, /codex-companion\.mjs" cancel \$ARGUMENTS/); + assert.match(cancel, /codex-companion\.mjs" cancel "\$ARGUMENTS"/); assert.match(resultHandling, /do not turn a failed or incomplete Codex run into a Claude-side implementation attempt/i); assert.match(resultHandling, /if Codex was never successfully invoked, do not generate a substitute answer at all/i); }); diff --git a/tests/process.test.mjs b/tests/process.test.mjs index 80e0715..2f6ddfc 100644 --- a/tests/process.test.mjs +++ b/tests/process.test.mjs @@ -1,7 +1,214 @@ +import fs from "node:fs"; +import path from "node:path"; import test from "node:test"; import assert from "node:assert/strict"; +import process from "node:process"; +import { fileURLToPath } from "node:url"; -import { terminateProcessTree } from "../plugins/codex/scripts/lib/process.mjs"; +import { + runCommand, + sanitizeChildEnv, + terminateProcessTree +} from "../plugins/codex/scripts/lib/process.mjs"; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); +const SCRIPT_ROOT = path.join(ROOT, "plugins", "codex", "scripts"); + +function listScriptFiles(dir) { + const files = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const fullPath = path.join(dir, entry.name); + if (entry.isDirectory()) { + files.push(...listScriptFiles(fullPath)); + continue; + } + if (entry.isFile() && fullPath.endsWith(".mjs")) { + files.push(fullPath); + } + } + return files.sort(); +} + +test("sanitizeChildEnv strips routing env and cargo target overrides", () => { + const env = sanitizeChildEnv({ + KEEP: "ok", + CARGO_HOME: "/tmp/cargo-home", + CARGO_TARGET_DIR: "/tmp/target", + RUST_VERIFICATION_ROOT_BASE: "/tmp/root", + RUST_VERIFICATION_REAL_CARGO: "/tmp/cargo", + RUST_VERIFICATION_PRESERVE_ROUTING_ENV: "1", + BOLT_RUST_VERIFICATION_ROOT: "/tmp/bolt", + CARGO_BUILD_TARGET_DIR: "/tmp/poison" + }); + + assert.deepEqual(env, { + KEEP: "ok", + CARGO_HOME: "/tmp/cargo-home" + }); +}); + +test("sanitizeChildEnv strips scrubbed keys case-insensitively", () => { + const env = sanitizeChildEnv({ + keep: "ok", + Cargo_Home: "/tmp/cargo-home", + cargo_target_dir: "/tmp/target", + Rust_Verification_Preserve_Routing_Env: "1", + cargo_build_target_dir: "/tmp/poison" + }); + + assert.deepEqual(env, { + keep: "ok", + Cargo_Home: "/tmp/cargo-home" + }); +}); + +test("sanitizeChildEnv does not mutate its input", () => { + const input = { + KEEP: "ok", + CARGO_TARGET_DIR: "/tmp/target", + RUST_VERIFICATION_PRESERVE_ROUTING_ENV: "1" + }; + + const output = sanitizeChildEnv(input); + + assert.deepEqual(input, { + KEEP: "ok", + CARGO_TARGET_DIR: "/tmp/target", + RUST_VERIFICATION_PRESERVE_ROUTING_ENV: "1" + }); + assert.deepEqual(output, { + KEEP: "ok" + }); +}); + +test("sanitizeChildEnv treats null like inherited process env", () => { + const originalKeep = process.env.TEST_NULL_FALLBACK_KEEP; + const originalTarget = process.env.CARGO_TARGET_DIR; + process.env.TEST_NULL_FALLBACK_KEEP = "ok"; + process.env.CARGO_TARGET_DIR = "/tmp/target"; + + try { + const env = sanitizeChildEnv(null); + assert.equal(env.TEST_NULL_FALLBACK_KEEP, "ok"); + assert.equal(env.CARGO_TARGET_DIR, undefined); + } finally { + if (originalKeep === undefined) { + delete process.env.TEST_NULL_FALLBACK_KEEP; + } else { + process.env.TEST_NULL_FALLBACK_KEEP = originalKeep; + } + + if (originalTarget === undefined) { + delete process.env.CARGO_TARGET_DIR; + } else { + process.env.CARGO_TARGET_DIR = originalTarget; + } + } +}); + +test("runCommand sanitizes child env before spawning", () => { + const result = runCommand( + process.execPath, + [ + "-e", + "process.stdout.write(JSON.stringify({" + + "keep: process.env.KEEP ?? null," + + "cargoHome: process.env.CARGO_HOME ?? null," + + "routing: process.env.RUST_VERIFICATION_PRESERVE_ROUTING_ENV ?? null," + + "targetRoot: process.env.CARGO_TARGET_DIR ?? null," + + "target: process.env.CARGO_BUILD_TARGET_DIR ?? null" + + "}));" + ], + { + env: { + ...process.env, + KEEP: "ok", + CARGO_HOME: "/tmp/cargo-home", + CARGO_TARGET_DIR: "/tmp/target", + RUST_VERIFICATION_PRESERVE_ROUTING_ENV: "1", + CARGO_BUILD_TARGET_DIR: "/tmp/poison" + } + } + ); + + assert.equal(result.status, 0, result.stderr); + assert.deepEqual(JSON.parse(result.stdout), { + keep: "ok", + cargoHome: "/tmp/cargo-home", + routing: null, + targetRoot: null, + target: null + }); +}); + +test("runCommand sanitizes inherited process env when env is omitted", () => { + const originalKeep = process.env.TEST_INHERITED_KEEP; + const originalTarget = process.env.CARGO_TARGET_DIR; + process.env.TEST_INHERITED_KEEP = "ok"; + process.env.CARGO_TARGET_DIR = "/tmp/target"; + + try { + const result = runCommand(process.execPath, [ + "-e", + "process.stdout.write(JSON.stringify({" + + "keep: process.env.TEST_INHERITED_KEEP ?? null," + + "targetRoot: process.env.CARGO_TARGET_DIR ?? null" + + "}));" + ]); + + assert.equal(result.status, 0, result.stderr); + assert.deepEqual(JSON.parse(result.stdout), { + keep: "ok", + targetRoot: null + }); + } finally { + if (originalKeep === undefined) { + delete process.env.TEST_INHERITED_KEEP; + } else { + process.env.TEST_INHERITED_KEEP = originalKeep; + } + + if (originalTarget === undefined) { + delete process.env.CARGO_TARGET_DIR; + } else { + process.env.CARGO_TARGET_DIR = originalTarget; + } + } +}); + +test("all production spawn sites are covered by the sanitizer contract", () => { + const expectedSpawnFiles = [ + "codex-companion.mjs", + "lib/app-server.mjs", + "lib/broker-lifecycle.mjs", + "lib/process.mjs", + "stop-review-gate-hook.mjs" + ]; + + const spawnFiles = listScriptFiles(SCRIPT_ROOT) + .filter((file) => /\bspawn(?:Sync)?\(/.test(fs.readFileSync(file, "utf8"))) + .map((file) => path.relative(SCRIPT_ROOT, file)) + .sort(); + + assert.deepEqual(spawnFiles, expectedSpawnFiles); + + for (const relativePath of spawnFiles) { + const source = fs.readFileSync(path.join(SCRIPT_ROOT, relativePath), "utf8"); + if (relativePath === "lib/process.mjs") { + assert.match(source, /env:\s*sanitizeChildEnv\(options\.env \?\? process\.env\)/); + continue; + } + + if (relativePath === "stop-review-gate-hook.mjs") { + assert.match(source, /const childEnv = \{\s*\.\.\.sanitizeChildEnv\(process\.env\)/s); + assert.match(source, /env:\s*childEnv/); + continue; + } + + assert.match(source, /sanitizeChildEnv/); + assert.match(source, /env:\s*sanitizeChildEnv\(/); + } +}); test("terminateProcessTree uses taskkill on Windows", () => { let captured = null;