diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index cc9de6d42..775437f92 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -53,6 +53,10 @@ - Fixed auth-gateway request cancellation for requests that are already aborted before dispatch. - Fixed `/login` and `/logout` provider selector overflowing tall provider lists off-screen on small terminals. The selector now scrolls a 10-item window centered on the highlighted entry, shows a `(n/total)` indicator when windowed, and accepts PageUp/PageDown for faster navigation. +### Fixed + +- Fixed `.env` loading so malformed variable names and NUL-containing values are ignored before they can poison `Bun.env` and break bash/external process execution with `nul byte found in provided data`. + ## [15.1.2] - 2026-05-15 ### Fixed diff --git a/packages/utils/src/env.ts b/packages/utils/src/env.ts index f2efbac5f..ed219cd56 100644 --- a/packages/utils/src/env.ts +++ b/packages/utils/src/env.ts @@ -3,13 +3,50 @@ import * as os from "node:os"; import * as path from "node:path"; import { getAgentDir, getConfigRootDir } from "./dirs"; +const ENV_NAME_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; + +/** + * Strict shell-identifier shape. Used for dotenv keys we accept into + * `Bun.env` — those should be referenceable as `$NAME` from POSIX shells, + * so we reject anything outside `[A-Za-z_][A-Za-z0-9_]*`. + */ +export function isValidEnvName(name: string): boolean { + return ENV_NAME_RE.test(name); +} + +/** + * The only names that are genuinely unsafe to forward to a native `execve` + * spawn: empty, containing `=` (would corrupt the `KEY=VALUE` framing) or + * NUL (terminates the C string mid-entry). Windows ships standard variables + * whose names contain parentheses (e.g. `ProgramFiles(x86)`, `CommonProgramFiles(x86)`) + * — those MUST survive the scrub so downstream resolvers (Git Bash discovery + * in `procmgr.ts`, etc.) can still read them. + */ +export function isSafeEnvName(name: string): boolean { + return name.length > 0 && !name.includes("=") && !name.includes("\0"); +} + +export function isSafeEnvValue(value: string): boolean { + return !value.includes("\0"); +} + +export function filterProcessEnv(env: Record): Record { + const result: Record = {}; + for (const key in env) { + const value = env[key]; + if (!isSafeEnvName(key) || value === undefined || !isSafeEnvValue(value)) continue; + result[key] = value; + } + return result; +} + /** * Parses a .env file synchronously and extracts key-value string pairs. * Ignores lines that are empty or start with '#'. Trims whitespace. * Allows values to be quoted with single or double quotes. * Returns an object of key-value pairs. */ -function parseEnvFile(filePath: string): Record { +export function parseEnvFile(filePath: string): Record { const result: Record = {}; try { const content = fs.readFileSync(filePath, "utf-8"); @@ -22,12 +59,15 @@ function parseEnvFile(filePath: string): Record { if (eqIndex === -1) continue; const key = trimmed.slice(0, eqIndex).trim(); + if (!isValidEnvName(key)) continue; + let value = trimmed.slice(eqIndex + 1).trim(); // Remove surrounding quotes (" or ') if ((value.startsWith('"') && value.endsWith('"')) || (value.startsWith("'") && value.endsWith("'"))) { value = value.slice(1, -1); } + if (!isSafeEnvValue(value)) continue; result[key] = value; } @@ -51,10 +91,17 @@ const piEnv = parseEnvFile(path.join(getConfigRootDir(), ".env")); const agentEnv = parseEnvFile(path.join(getAgentDir(), ".env")); const projectEnv = parseEnvFile(path.join(process.cwd(), ".env")); +for (const key of Object.keys(Bun.env)) { + const value = Bun.env[key]; + if (!isSafeEnvName(key) || value === undefined || !isSafeEnvValue(value)) { + delete Bun.env[key]; + } +} + for (const file of [projectEnv, agentEnv, piEnv, homeEnv]) { - for (const [key, value] of Object.entries(file)) { + for (const key in file) { if (!Bun.env[key]) { - Bun.env[key] = value; + Bun.env[key] = file[key]; } } } diff --git a/packages/utils/src/procmgr.ts b/packages/utils/src/procmgr.ts index 04612283b..32b4aef8f 100644 --- a/packages/utils/src/procmgr.ts +++ b/packages/utils/src/procmgr.ts @@ -2,7 +2,7 @@ import * as fs from "node:fs"; import * as path from "node:path"; import { Process, ProcessStatus } from "@oh-my-pi/pi-natives"; import type { Subprocess } from "bun"; -import { $env } from "./env"; +import { $env, filterProcessEnv } from "./env"; import { $which } from "./which"; export interface ShellConfig { @@ -45,7 +45,7 @@ function isExecutable(path: string): boolean { function buildSpawnEnv(shell: string): Record { const noCI = $env.PI_BASH_NO_CI || $env.CLAUDE_BASH_NO_CI; return { - ...Bun.env, + ...filterProcessEnv(Bun.env), SHELL: shell, GIT_EDITOR: "true", GPG_TTY: "not a tty", diff --git a/packages/utils/test/env.test.ts b/packages/utils/test/env.test.ts new file mode 100644 index 000000000..a4997c185 --- /dev/null +++ b/packages/utils/test/env.test.ts @@ -0,0 +1,84 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as fs from "node:fs"; +import * as os from "node:os"; +import * as path from "node:path"; +import { filterProcessEnv, parseEnvFile } from "../src/env"; + +const tempDirs: string[] = []; + +afterEach(() => { + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { force: true, recursive: true }); + } +}); + +function writeTempEnv(content: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "pi-utils-env-")); + tempDirs.push(dir); + const filePath = path.join(dir, ".env"); + fs.writeFileSync(filePath, content); + return filePath; +} + +describe("parseEnvFile", () => { + it("ignores malformed names and nul-containing values", () => { + const filePath = writeTempEnv( + [ + "GOOD=value", + "_ALSO_GOOD='quoted value'", + "1BAD=value", + "BAD-NAME=value", + "BAD NAME=value", + "BAD_VALUE=before\0after", + "# comment", + "NO_EQUALS", + ].join("\n"), + ); + + expect(parseEnvFile(filePath)).toEqual({ + GOOD: "value", + _ALSO_GOOD: "quoted value", + }); + }); + + it("mirrors valid OMP_ variables to PI_ variables", () => { + const filePath = writeTempEnv("OMP_FEATURE=enabled\nOMP_BAD=before\0after\n"); + + expect(parseEnvFile(filePath)).toEqual({ + OMP_FEATURE: "enabled", + PI_FEATURE: "enabled", + }); + }); +}); + +describe("filterProcessEnv", () => { + it("drops entries that cannot be passed to process spawn env", () => { + expect( + filterProcessEnv({ + GOOD: "value", + EMPTY: "", + "BAD=NAME": "value", + BAD_VALUE: "before\0after", + MISSING: undefined, + }), + ).toEqual({ + GOOD: "value", + EMPTY: "", + }); + }); + + it("preserves Windows-style variable names containing parentheses", () => { + // `ProgramFiles(x86)` and friends are standard on Windows and must + // survive the scrub so Git Bash discovery in procmgr.ts can resolve + // 32-bit Program Files installations. + expect( + filterProcessEnv({ + "ProgramFiles(x86)": "C:\\Program Files (x86)", + "CommonProgramFiles(x86)": "C:\\Program Files (x86)\\Common Files", + }), + ).toEqual({ + "ProgramFiles(x86)": "C:\\Program Files (x86)", + "CommonProgramFiles(x86)": "C:\\Program Files (x86)\\Common Files", + }); + }); +});