From 1d1d80c3f9066933e78438917e408fe21347779b Mon Sep 17 00:00:00 2001 From: Colin McDonnell Date: Thu, 8 Jan 2026 14:57:47 -0800 Subject: [PATCH] Additional testing with codex --- agents/codex.ts | 108 +++++++++++++++++++++++++---------------------- docs/security.md | 40 ++++++++++++++---- entry | 91 +++++++++++++++++++++------------------ main.ts | 1 + play.ts | 13 +++--- 5 files changed, 143 insertions(+), 110 deletions(-) diff --git a/agents/codex.ts b/agents/codex.ts index 8c9ca7b..9a7fd5a 100644 --- a/agents/codex.ts +++ b/agents/codex.ts @@ -1,15 +1,55 @@ -import { spawnSync } from "node:child_process"; -import { mkdirSync } from "node:fs"; +import { mkdirSync, writeFileSync } from "node:fs"; import { join } from "node:path"; +import type { McpHttpServerConfig } from "@anthropic-ai/claude-agent-sdk"; import { Codex, type CodexOptions, type ThreadEvent } from "@openai/codex-sdk"; import { log } from "../utils/cli.ts"; import { addInstructions } from "./instructions.ts"; -import { - agent, - type ConfigureMcpServersParams, - installFromNpmTarball, - setupProcessAgentEnv, -} from "./shared.ts"; +import { agent, installFromNpmTarball, setupProcessAgentEnv } from "./shared.ts"; + +interface WriteCodexConfigParams { + tempHome: string; + mcpServers: Record; + isPublicRepo: boolean; +} + +function writeCodexConfig({ tempHome, mcpServers, isPublicRepo }: WriteCodexConfigParams): string { + const codexDir = join(tempHome, ".codex"); + mkdirSync(codexDir, { recursive: true }); + const configPath = join(codexDir, "config.toml"); + + // build MCP servers section + const mcpServerSections: string[] = []; + for (const [name, config] of Object.entries(mcpServers)) { + if (config.type !== "http") continue; + log.info(`» Adding MCP server '${name}' at ${config.url}`); + mcpServerSections.push(`[mcp_servers.${name}]\nurl = "${config.url}"`); + } + + // SECURITY: for public repos, enforce env filtering via shell_environment_policy + // this prevents vuln if user's ~/.codex/config.toml has ignore_default_excludes=true + // for private repos, no filtering - agents use native shell with full env access + const shellPolicy = isPublicRepo + ? `[shell_environment_policy] +ignore_default_excludes = false` + : ""; + + writeFileSync( + configPath, + `# written by pullfrog +${shellPolicy} + +${mcpServerSections.join("\n\n")} +`.trim() + "\n" + ); + + if (isPublicRepo) { + log.info(`» Codex config written to ${configPath} (env filtering: enabled)`); + } else { + log.info(`» Codex config written to ${configPath} (private repo: no env filtering)`); + } + + return codexDir; +} export const codex = agent({ name: "codex", @@ -21,18 +61,24 @@ export const codex = agent({ }); }, run: async ({ payload, mcpServers, apiKey, cliPath, repo }) => { - // create config directory for codex before setting HOME const tempHome = process.env.PULLFROG_TEMP_DIR!; + + // create config directory for codex before setting HOME const configDir = join(tempHome, ".config", "codex"); mkdirSync(configDir, { recursive: true }); + const codexDir = writeCodexConfig({ + tempHome, + mcpServers, + isPublicRepo: repo.isPublic, + }); + setupProcessAgentEnv({ OPENAI_API_KEY: apiKey, HOME: tempHome, + CODEX_HOME: codexDir, // point Codex to our config directory }); - configureCodexMcpServers({ mcpServers, cliPath }); - // Configure Codex const codexOptions: CodexOptions = { apiKey, @@ -43,16 +89,6 @@ export const codex = agent({ log.info("🔒 sandbox mode enabled: restricting to read-only operations"); } - // SECURITY NOTE: Codex SDK does not have an option to disable native shell commands. - // For public repos, we rely on instructions telling the agent to use MCP bash instead. - // The MCP bash tool filters sensitive environment variables. - // This is not as robust as Claude/Cursor/OpenCode which can explicitly disable native bash. - if (repo.isPublic) { - log.info( - "🔒 public repo: instructions direct to MCP bash (native shell NOT disabled in SDK)" - ); - } - const codex = new Codex(codexOptions); const thread = codex.startThread( payload.sandbox @@ -195,33 +231,3 @@ const messageHandlers: { log.error(`Error: ${event.message}`); }, }; - -/** - * Configure MCP servers for Codex using the CLI. - * For HTTP-based servers, use: codex mcp add --url - */ -function configureCodexMcpServers({ mcpServers, cliPath }: ConfigureMcpServersParams): void { - for (const [serverName, serverConfig] of Object.entries(mcpServers)) { - if (serverConfig.type === "http") { - // HTTP-based MCP server - use --url flag - const addArgs = ["mcp", "add", serverName, "--url", serverConfig.url]; - - log.info(`Adding MCP server '${serverName}' at ${serverConfig.url}...`); - const addResult = spawnSync("node", [cliPath, ...addArgs], { - stdio: "pipe", - encoding: "utf-8", - }); - - if (addResult.status !== 0) { - throw new Error( - `codex mcp add failed: ${addResult.stderr || addResult.stdout || "Unknown error"}` - ); - } - log.info(`✓ MCP server '${serverName}' configured`); - } else { - throw new Error( - `Unsupported MCP server type for Codex: ${(serverConfig as any).type || "unknown"}` - ); - } - } -} diff --git a/docs/security.md b/docs/security.md index e6a23d8..63bda04 100644 --- a/docs/security.md +++ b/docs/security.md @@ -174,8 +174,8 @@ const bashPermission = isPublicRepo ? "deny" : "allow"; // Gemini - uses excludeTools in ~/.gemini/settings.json newSettings.excludeTools = ["run_shell_command"]; -// Codex - NO SDK mechanism to disable native shell -// Relies on instructions only (limitation) +// Codex - CLI internally scrubs env before spawning shell +// No SDK-level config needed; Codex handles this automatically ``` For **private repos**, native bash is allowed for all agents. @@ -263,15 +263,37 @@ This is a blocklist approach which explicitly excludes the shell tool while allo Additionally, Gemini has built-in CI detection that filters shell env when `GITHUB_SHA` is set. -### Codex (Limitation) +### Codex -**⚠️ Codex SDK does not support disabling native shell commands.** The SDK only offers `sandboxMode` options which control filesystem access, not specific tool availability. +Codex CLI filters out env vars matching `KEY`, `SECRET`, or `TOKEN` (case-insensitive) by default via `shell_environment_policy.ignore_default_excludes = false`. -For public repos, we rely on: -1. **Instructions** telling the agent to use MCP bash instead of native shell -2. **MCP bash tool** being available as an alternative +**Vulnerability**: If a user's `~/.codex/config.toml` has `ignore_default_excludes = true`, secrets will leak to shell commands. -This is a known limitation. Codex may still use native shell if it doesn't follow instructions. +**Our mitigation**: We set `CODEX_HOME` to a temp directory and write our own `config.toml` with `ignore_default_excludes = false` to enforce filtering regardless of what config exists in the user's `~/.codex/`. + +```typescript +// set CODEX_HOME to override user's config +setupProcessAgentEnv({ CODEX_HOME: codexDir }); + +// write secure config to $CODEX_HOME/config.toml +writeFileSync(join(codexDir, "config.toml"), ` +[shell_environment_policy] +ignore_default_excludes = false +`); +``` + +See [GitHub Issue #3064](https://github.com/openai/codex/issues/3064) and [config docs](https://github.com/openai/codex/blob/main/docs/config.md#shell_environment_policy). + +**Verified behavior** (tested via `pnpm play codex-env-test.ts`): +- Default (no config): ✅ secrets filtered +- `ignore_default_excludes = false`: ✅ secrets filtered +- `ignore_default_excludes = true`: ❌ secrets leak + +Example output when running `env | grep TEST` with our config: +``` +TEST_SAFE_VAR=VISIBLE-SAFE-VALUE +# FAKE_SECRET_KEY and TEST_API_TOKEN are NOT visible (filtered) +``` ### Summary by Agent @@ -281,4 +303,4 @@ This is a known limitation. Codex may still use native shell if it doesn't follo | Cursor | Native shell **disabled** | Native shell allowed | | OpenCode | Native bash **disabled** | Native bash allowed | | Gemini | Native shell **disabled** (via excludeTools) | Native bash allowed | -| Codex | Instructions only (**⚠️ not enforced**) | Native bash allowed | +| Codex | Native shell allowed (CLI scrubs env internally) | Native bash allowed | diff --git a/entry b/entry index a1ab4b0..6a9c718 100755 --- a/entry +++ b/entry @@ -104876,8 +104876,7 @@ var messageHandlers = { }; // agents/codex.ts -import { spawnSync as spawnSync2 } from "node:child_process"; -import { mkdirSync as mkdirSync3 } from "node:fs"; +import { mkdirSync as mkdirSync3, writeFileSync } from "node:fs"; import { join as join6 } from "node:path"; // ../node_modules/.pnpm/@openai+codex-sdk@0.58.0/node_modules/@openai/codex-sdk/dist/index.js @@ -105204,6 +105203,34 @@ var Codex = class { }; // agents/codex.ts +function writeCodexConfig({ tempHome, mcpServers, isPublicRepo }) { + const codexDir = join6(tempHome, ".codex"); + mkdirSync3(codexDir, { recursive: true }); + const configPath = join6(codexDir, "config.toml"); + const mcpServerSections = []; + for (const [name, config4] of Object.entries(mcpServers)) { + if (config4.type !== "http") continue; + log.info(`\xBB Adding MCP server '${name}' at ${config4.url}`); + mcpServerSections.push(`[mcp_servers.${name}] +url = "${config4.url}"`); + } + const shellPolicy = isPublicRepo ? `[shell_environment_policy] +ignore_default_excludes = false` : ""; + writeFileSync( + configPath, + `# written by pullfrog +${shellPolicy} + +${mcpServerSections.join("\n\n")} +`.trim() + "\n" + ); + if (isPublicRepo) { + log.info(`\xBB Codex config written to ${configPath} (env filtering: enabled)`); + } else { + log.info(`\xBB Codex config written to ${configPath} (private repo: no env filtering)`); + } + return codexDir; +} var codex = agent({ name: "codex", install: async () => { @@ -105217,11 +105244,17 @@ var codex = agent({ const tempHome = process.env.PULLFROG_TEMP_DIR; const configDir = join6(tempHome, ".config", "codex"); mkdirSync3(configDir, { recursive: true }); + const codexDir = writeCodexConfig({ + tempHome, + mcpServers, + isPublicRepo: repo.isPublic + }); setupProcessAgentEnv({ OPENAI_API_KEY: apiKey, - HOME: tempHome + HOME: tempHome, + CODEX_HOME: codexDir + // point Codex to our config directory }); - configureCodexMcpServers({ mcpServers, cliPath }); const codexOptions = { apiKey, codexPathOverride: cliPath @@ -105229,11 +105262,6 @@ var codex = agent({ if (payload.sandbox) { log.info("\u{1F512} sandbox mode enabled: restricting to read-only operations"); } - if (repo.isPublic) { - log.info( - "\u{1F512} public repo: instructions direct to MCP bash (native shell NOT disabled in SDK)" - ); - } const codex2 = new Codex(codexOptions); const thread = codex2.startThread( payload.sandbox ? { @@ -105354,32 +105382,10 @@ var messageHandlers2 = { log.error(`Error: ${event.message}`); } }; -function configureCodexMcpServers({ mcpServers, cliPath }) { - for (const [serverName, serverConfig] of Object.entries(mcpServers)) { - if (serverConfig.type === "http") { - const addArgs = ["mcp", "add", serverName, "--url", serverConfig.url]; - log.info(`Adding MCP server '${serverName}' at ${serverConfig.url}...`); - const addResult = spawnSync2("node", [cliPath, ...addArgs], { - stdio: "pipe", - encoding: "utf-8" - }); - if (addResult.status !== 0) { - throw new Error( - `codex mcp add failed: ${addResult.stderr || addResult.stdout || "Unknown error"}` - ); - } - log.info(`\u2713 MCP server '${serverName}' configured`); - } else { - throw new Error( - `Unsupported MCP server type for Codex: ${serverConfig.type || "unknown"}` - ); - } - } -} // agents/cursor.ts import { spawn as spawn3 } from "node:child_process"; -import { mkdirSync as mkdirSync4, writeFileSync } from "node:fs"; +import { mkdirSync as mkdirSync4, writeFileSync as writeFileSync2 } from "node:fs"; import { homedir as homedir2 } from "node:os"; import { join as join7 } from "node:path"; var cursor = agent({ @@ -105554,7 +105560,7 @@ function configureCursorMcpServers({ mcpServers }) { url: serverConfig.url }; } - writeFileSync(mcpConfigPath, JSON.stringify({ mcpServers: cursorMcpServers }, null, 2), "utf-8"); + writeFileSync2(mcpConfigPath, JSON.stringify({ mcpServers: cursorMcpServers }, null, 2), "utf-8"); log.info(`\xBB MCP config written to ${mcpConfigPath}`); } function configureCursorSandbox({ @@ -105577,14 +105583,14 @@ function configureCursorSandbox({ deny: denyShell } }; - writeFileSync(cliConfigPath, JSON.stringify(config4, null, 2), "utf-8"); + writeFileSync2(cliConfigPath, JSON.stringify(config4, null, 2), "utf-8"); log.info( `\xBB CLI config written to ${cliConfigPath} (sandbox: ${sandbox}, isPublicRepo: ${isPublicRepo})` ); } // agents/gemini.ts -import { mkdirSync as mkdirSync5, readFileSync as readFileSync2, writeFileSync as writeFileSync2 } from "node:fs"; +import { mkdirSync as mkdirSync5, readFileSync as readFileSync2, writeFileSync as writeFileSync3 } from "node:fs"; import { homedir as homedir3 } from "node:os"; import { join as join8 } from "node:path"; @@ -105869,12 +105875,12 @@ function configureGeminiMcpServers({ mcpServers, isPublicRepo }) { if (isPublicRepo) { newSettings.excludeTools = ["run_shell_command"]; } - writeFileSync2(settingsPath, JSON.stringify(newSettings, null, 2), "utf-8"); + writeFileSync3(settingsPath, JSON.stringify(newSettings, null, 2), "utf-8"); log.info(`\xBB MCP config written to ${settingsPath}`); } // agents/opencode.ts -import { mkdirSync as mkdirSync6, writeFileSync as writeFileSync3 } from "node:fs"; +import { mkdirSync as mkdirSync6, writeFileSync as writeFileSync4 } from "node:fs"; import { join as join9 } from "node:path"; var opencode = agent({ name: "opencode", @@ -106051,7 +106057,7 @@ function configureOpenCode({ mcpServers, sandbox, isPublicRepo }) { }; const configJson = JSON.stringify(config4, null, 2); try { - writeFileSync3(configPath, configJson, "utf-8"); + writeFileSync4(configPath, configJson, "utf-8"); } catch (error50) { log.error( `failed to write OpenCode config to ${configPath}: ${error50 instanceof Error ? error50.message : String(error50)}` @@ -136258,14 +136264,14 @@ var FastMCP = class extends FastMCPEventEmitter { }; // mcp/checkout.ts -import { writeFileSync as writeFileSync4 } from "node:fs"; +import { writeFileSync as writeFileSync5 } from "node:fs"; import { join as join10 } from "node:path"; // utils/shell.ts -import { spawnSync as spawnSync3 } from "node:child_process"; +import { spawnSync as spawnSync2 } from "node:child_process"; function $(cmd, args3, options) { const encoding = options?.encoding ?? "utf-8"; - const result = spawnSync3(cmd, args3, { + const result = spawnSync2(cmd, args3, { stdio: ["ignore", "pipe", "pipe"], encoding, cwd: options?.cwd, @@ -136453,7 +136459,7 @@ ${diffPreview}`); ); } const diffPath = join10(tempDir, `pr-${pull_number}.diff`); - writeFileSync4(diffPath, diffContent); + writeFileSync5(diffPath, diffContent); log.debug(`wrote diff to ${diffPath} (${diffContent.length} bytes)`); return { success: true, @@ -138529,6 +138535,7 @@ function resolveAgent({ repoSettings }) { const agentOverride = process.env.AGENT_OVERRIDE; + log.debug(`\xBB determineAgent: agentOverride=${agentOverride}, payload.agent=${payload.agent}, repoSettings.defaultAgent=${repoSettings.defaultAgent}`); const configuredAgentName = agentOverride || payload.agent || repoSettings.defaultAgent || null; if (configuredAgentName) { const agent3 = agents[configuredAgentName]; diff --git a/main.ts b/main.ts index d596a26..c50d0d7 100644 --- a/main.ts +++ b/main.ts @@ -370,6 +370,7 @@ function resolveAgent({ repoSettings: RepoSettings; }): Agent { const agentOverride = process.env.AGENT_OVERRIDE as AgentName | undefined; + log.debug(`» determineAgent: agentOverride=${agentOverride}, payload.agent=${payload.agent}, repoSettings.defaultAgent=${repoSettings.defaultAgent}`); const configuredAgentName = agentOverride || payload.agent || repoSettings.defaultAgent || null; if (configuredAgentName) { diff --git a/play.ts b/play.ts index 8992928..ce5ae3b 100644 --- a/play.ts +++ b/play.ts @@ -108,9 +108,10 @@ Examples: const passArgs = process.argv.slice(2); const nodeCmd = `node play.ts ${passArgs.join(" ")}`; - // pass .env file directly to Docker - const envFile = join(process.cwd(), "..", ".env"); - const envFlags = existsSync(envFile) ? ["--env-file", envFile] : []; + // pass all env vars to docker + const envFlags = Object.entries(process.env).flatMap(([key, value]) => + value !== undefined ? ["-e", `${key}=${value}`] : [] + ); // SSH for git - mount individual SSH files to avoid permission issues const sshFlags: string[] = []; @@ -145,17 +146,13 @@ Examples: "pullfrog-action-node-modules:/app/action/node_modules", "-w", "/app/action", - "-e", - "GITHUB_ACTIONS=true", - "-e", - "CI=true", ...envFlags, ...sshFlags, "--cap-add", "SYS_ADMIN", "--security-opt", "seccomp:unconfined", - "node:22", + "node:24", "bash", "-c", `corepack enable pnpm >/dev/null 2>&1 && pnpm install --frozen-lockfile && ${nodeCmd}`,