diff --git a/sdk/typescript/src/deduplication/codex-review.ts b/sdk/typescript/src/deduplication/codex-review.ts index e8e01eddb..00cd0ed67 100644 --- a/sdk/typescript/src/deduplication/codex-review.ts +++ b/sdk/typescript/src/deduplication/codex-review.ts @@ -10,6 +10,7 @@ import { createInterface } from "node:readline"; import { comparisonEnvironment, disabledMcpServers, + environmentEntry, } from "../scan-comparison.js"; import { codexSecurityCredentialHome, @@ -81,8 +82,10 @@ export class CodexReviewRunner { environment, { workingDirectory: this.workingDirectory, signal: this.signal }, ); - const apiKey = - environment["OPENAI_API_KEY"] ?? environment["CODEX_API_KEY"]; + const apiKey = [ + environmentEntry(environment, "OPENAI_API_KEY"), + environmentEntry(environment, "CODEX_API_KEY"), + ].find((value) => value?.trim()); const args = ["app-server", "--stdio", "--disable", "plugins"]; const stateDatabase = join( codexSecurityStateDirectory(environment), @@ -90,10 +93,12 @@ export class CodexReviewRunner { ); const privatePaths = new Set( [ - environment["CODEX_HOME"] ?? join(homedir(), ".codex"), + environmentEntry(environment, "CODEX_HOME") || + join(homedir(), ".codex"), codexSecurityCredentialHome(environment), join(homedir(), ".ssh"), - environment["GH_CONFIG_DIR"] ?? join(homedir(), ".config", "gh"), + environmentEntry(environment, "GH_CONFIG_DIR") || + join(homedir(), ".config", "gh"), stateDatabase, `${stateDatabase}-wal`, `${stateDatabase}-shm`, diff --git a/sdk/typescript/src/scan-comparison.ts b/sdk/typescript/src/scan-comparison.ts index baa0f7e19..5eb86725c 100644 --- a/sdk/typescript/src/scan-comparison.ts +++ b/sdk/typescript/src/scan-comparison.ts @@ -459,7 +459,7 @@ export async function comparisonEnvironment( return environment; } -function environmentEntry( +export function environmentEntry( environment: Record, requested: string, ): string | undefined { diff --git a/sdk/typescript/tests-ts/codex-review.test.ts b/sdk/typescript/tests-ts/codex-review.test.ts index e56d6bb02..4c09ea556 100644 --- a/sdk/typescript/tests-ts/codex-review.test.ts +++ b/sdk/typescript/tests-ts/codex-review.test.ts @@ -1,44 +1,87 @@ import { spawn, type ChildProcessWithoutNullStreams } from "node:child_process"; import { existsSync } from "node:fs"; import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; -import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { homedir, tmpdir } from "node:os"; +import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; -import { expect, test } from "bun:test"; +import { expect, mock, test } from "bun:test"; import { CodexReviewRunner } from "../src/deduplication/codex-review.js"; +import { environmentEntry } from "../src/scan-comparison.js"; +import { runTestInSubprocess } from "./support/test-subprocess.js"; const fixture = fileURLToPath( new URL("fixtures/codex-review.mjs", import.meta.url), ); -for (const scenario of [ - "correction", - "text-only", - "failed-turn", - "exit", - "cancel", -]) { - test(`Codex review transport: ${scenario}`, async () => { +const transportCases: { + scenario: string; + name?: string; + environmentNames?: readonly [string, string, string]; + extraEnvironment?: Record; + windowsOnly?: boolean; +}[] = [ + ...["correction", "text-only", "failed-turn", "exit", "cancel"].map( + (scenario) => ({ scenario }), + ), + { + scenario: "correction", + name: "lowercase Windows environment", + environmentNames: ["codex_home", "openai_api_key", "gh_config_dir"], + windowsOnly: true, + }, + { + scenario: "correction", + name: "mixed-case Windows environment", + environmentNames: ["Codex_Home", "Codex_Api_Key", "Gh_Config_Dir"], + windowsOnly: true, + }, + ...["", " \t"].map((value) => ({ + scenario: "correction", + name: `${value === "" ? "empty" : "blank"} OpenAI key uses Codex key`, + environmentNames: ["CODEX_HOME", "CODEX_API_KEY", "GH_CONFIG_DIR"] as const, + extraEnvironment: { OPENAI_API_KEY: value }, + })), + { + scenario: "correction", + name: "empty Windows OpenAI alias uses Codex key", + environmentNames: ["Codex_Home", "Codex_Api_Key", "Gh_Config_Dir"], + extraEnvironment: { openai_api_key: "" }, + windowsOnly: true, + }, +]; + +for (const { + scenario, + name = scenario, + environmentNames = ["CODEX_HOME", "OPENAI_API_KEY", "GH_CONFIG_DIR"], + extraEnvironment, + windowsOnly = false, +} of transportCases) { + const runCase = test.skipIf(windowsOnly && process.platform !== "win32"); + runCase(`Codex review transport: ${name}`, async () => { const modelHome = await mkdtemp(join(tmpdir(), "codex-review-test-")); const checkout = await mkdtemp(join(tmpdir(), "codex-review-source-")); + const ghConfig = await mkdtemp(join(tmpdir(), "codex-review-gh-")); const transcript = join(modelHome, "messages.jsonl"); let child: ChildProcessWithoutNullStreams | undefined; let directory: string | undefined; let args: readonly string[] = []; const controller = new AbortController(); try { - await writeFile( - join(modelHome, "config.toml"), - '[mcp_servers.synthetic]\ncommand = "synthetic-unused-command"\n', - ); + const configuration = + '[mcp_servers.synthetic]\ncommand = "synthetic-unused-command"\n'; + await writeFile(join(modelHome, "config.toml"), configuration); + const [homeName, keyName, ghName] = environmentNames; const runner = new CodexReviewRunner( { PATH: process.env["PATH"], SystemRoot: process.env["SystemRoot"], TEMP: process.env["TEMP"], TMP: process.env["TMP"], - CODEX_HOME: modelHome, - OPENAI_API_KEY: "synthetic-review-key", + [homeName]: modelHome, + [keyName]: "synthetic-review-key", + [ghName]: ghConfig, + ...extraEnvironment, }, (_command, commandArgs, options) => { args = commandArgs; @@ -95,10 +138,29 @@ for (const scenario of [ } expect(args).toContain('cli_auth_credentials_store="ephemeral"'); expect(args.join(" ")).not.toContain("synthetic-review-key"); - if (scenario !== "cancel") - expect(await readFile(transcript, "utf8")).toContain( - '"method":"account/login/start"', - ); + const permissions = args.find((argument) => + argument.startsWith("permissions.codex_security_review="), + ); + expect(permissions).toContain( + `${JSON.stringify(resolve(modelHome))}="deny"`, + ); + expect(permissions).toContain( + `${JSON.stringify(resolve(ghConfig))}="deny"`, + ); + if (scenario !== "cancel") { + const loginRequest = (await readFile(transcript, "utf8")) + .trim() + .split("\n") + .map( + (line) => + JSON.parse(line) as { + method?: string; + params?: { apiKey?: string }; + }, + ) + .find((message) => message.method === "account/login/start"); + expect(loginRequest?.params?.apiKey).toBe("synthetic-review-key"); + } expect(existsSync(join(modelHome, "auth.json"))).toBe(false); expect(child!.exitCode !== null || child!.signalCode !== null).toBe(true); expect(existsSync(directory!)).toBe(false); @@ -106,6 +168,94 @@ for (const scenario of [ } finally { await rm(modelHome, { recursive: true, force: true }); await rm(checkout, { recursive: true, force: true }); + await rm(ghConfig, { recursive: true, force: true }); } }); } + +test("empty credential paths use default directories without denying cwd", async () => { + if ( + runTestInSubprocess( + import.meta.path, + "empty credential paths use default directories without denying cwd", + ) + ) { + return; + } + const comparison = { ...(await import("../src/scan-comparison.js")) }; + const root = await mkdtemp(join(tmpdir(), "codex-review-empty-paths-")); + const checkout = await mkdtemp(join(tmpdir(), "codex-review-source-")); + // An empty CODEX_HOME makes native Codex use the real user profile. + mock.module("../src/scan-comparison.js", () => ({ + ...comparison, + disabledMcpServers: async () => ({}), + })); + try { + const names: [string, string][] = [["CODEX_HOME", "GH_CONFIG_DIR"]]; + if (process.platform === "win32") + names.push(["codex_home", "Gh_Config_Dir"]); + for (const [homeName, ghName] of names) { + let args: readonly string[] = []; + const runner = new CodexReviewRunner( + { + CODEX_CLI_PATH: process.execPath, + CODEX_SECURITY_STATE_DIR: join(root, "state"), + OPENAI_API_KEY: "synthetic-review-key", + [homeName]: "", + [ghName]: "", + }, + (_command, commandArgs) => { + args = commandArgs; + throw new Error("Synthetic stop after permission configuration"); + }, + undefined, + checkout, + ); + await expect( + runner.run({ + model: "gpt-5.6-sol", + effort: "ultra", + prompt: "Review the supplied synthetic reports.", + schema: {}, + validate: (value) => value, + }), + ).rejects.toThrow( + "Codex did not complete a validated deduplication review", + ); + const permissions = args.find((argument) => + argument.startsWith("permissions.codex_security_review="), + ); + for (const path of [ + join(homedir(), ".codex"), + join(homedir(), ".config", "gh"), + ]) { + expect(permissions).toContain( + `${JSON.stringify(resolve(path))}="deny"`, + ); + } + expect(permissions).not.toContain( + `${JSON.stringify(resolve(""))}="deny"`, + ); + } + } finally { + mock.module("../src/scan-comparison.js", () => comparison); + await rm(root, { recursive: true, force: true }); + await rm(checkout, { recursive: true, force: true }); + } +}); + +test("environment lookups preserve platform case rules and exact-key precedence", () => { + const aliases = { codex_home: "synthetic-alias" }; + expect(environmentEntry(aliases, "CODEX_HOME")).toBe( + process.platform === "win32" ? "synthetic-alias" : undefined, + ); + expect( + environmentEntry( + { ...aliases, CODEX_HOME: "synthetic-exact" }, + "CODEX_HOME", + ), + ).toBe("synthetic-exact"); + expect(environmentEntry({ ...aliases, CODEX_HOME: "" }, "CODEX_HOME")).toBe( + "", + ); +});