diff --git a/devlog/_plan/260819_unclaimed_bug_selection/140_final_audit.md b/devlog/_plan/260819_unclaimed_bug_selection/140_final_audit.md new file mode 100644 index 0000000000..c4c5c20fa2 --- /dev/null +++ b/devlog/_plan/260819_unclaimed_bug_selection/140_final_audit.md @@ -0,0 +1,100 @@ +# 140 — final audit of the merged stack, and what it caught + +Run after #2116/#2117/#2118/#2121 landed on `dev`. Verdict: **fail**, and the +reason was not one of the four fixes. + +## What the audit confirmed + +- The proxy-env leak is genuinely dead. The auditor re-ran the five affected + suites from a clean `git archive origin/dev` — 236 pass / 0 fail — **and then + re-ran them without `--isolate`**, the exact single-process condition that + produced the original 73 failures. Still green. That second run is the one + that matters; the first only proves isolation hides it. +- #2121's gate reason cannot fire on the turn-drain path. +- Escaping holds across all three builders under newline, quote and `%` + injection. +- Full suite on `dev` head `fbc6f26a2`: **13,501 pass / 0 fail**, run on + `ssh lidge`. + +## What it caught — P1, and it is real + +#2107 baked the proxy environment into the installed service definition. A proxy +URL routinely carries `user:password`, so that change quietly made those files +credential-bearing. They were still written with a bare `writeFileSync`. + +Measured, not assumed: umask 022, `writeFileSync` with no mode → **0644**. + +The precedent was already in the same file and was not followed — the service +API token (`service.ts:387`) and the install state (`:190`) both write +`{ mode: 0o600 }` plus a `chmodSync`. The repo also has an explicit convention +against leaking this exact value: `collectProxyEnv` reports proxy presence as a +boolean so the URL never escapes, pinned by a `doctor` test asserting the +serialized rows never contain `"secret"`. + +So the change wrote a credential to a world-readable file in a codebase that +already treats 0600 as the standard for precisely this data. + +**The uncomfortable part is procedural.** #2116's own body disclosed the risk +and offered to gate on redaction. That question was never adjudicated — the PR +merged at `REVIEW_REQUIRED` with only bot comments. `AGENTS.md` requires +explicit security review for credential handling. Disclosing a risk in a PR body +is not the same as discharging it, and self-merging past your own open question +is how a known risk becomes a shipped one. + +### The fix + +One `writeServiceDefinitionFile()` for the plist, the unit, and the Windows +scheduler assets: `{ mode: 0o600 }` plus `chmodSync`, plus the Windows ACL. + +The explicit `chmodSync` is not belt-and-braces. `mode` applies only at +creation, so an install over a definition an earlier version left at 0644 would +keep the loose mode — and that is the realistic upgrade path, not a hypothetical. + +Red-driven: with the mode argument removed, the three new assertions report +`644` against an expected `600`. + +## P2 — the untestable builder was left untested + +`buildWindowsServiceScript` was the only one of the three builders with no proxy +assertion, and the reason is instructive: the only way to reach it was to assign +`process.env`, which is the exact pattern whose leak this stack had just removed. +The refactor fixed the leak where a test existed and left the untestable builder +untested. + +It now takes the resolved entries like the other two, with a regression covering +the canonical-name rule. + +## P2 — the new fix reintroduced the same structural class + +`reportedFenceReasons` in #2121 is process-lifetime module state — structurally +the same hazard as the proxy leak, one abstraction away. Whichever file +constructs the error first consumes the one-shot warn, so a later file asserting +on it would see nothing and **pass vacuously**. + +Current suites pass in both file orders, so this was latent rather than live. The +reset is now documented as an order-sensitive contract and its caller resets on +both sides. + +## P3 — pin-to-line comments were already wrong at merge + +`auth-context.ts:326` (actual: 357/363/370), `lifecycle.ts:180`, +`native-profile-startup.ts:138-139` (actual: 142-143) and `:311` (actual: 315). +Replaced with symbol names, which do not drift when a file moves. + +## The honest gap that remains + +No commit in this stack has a green cross-platform CI run of its own — the runs +were cancelled by successive force-pushes, and `dev`'s own run was still in +flight. Both Windows-specific behaviors this stack shipped are unverified on +Windows: the Windows proxy path had no test until now, and `owner-unavailable` — +the branch #2108 most needs named — is a Windows icacls path asserted nowhere in +the suite. + +Stating it rather than filing it as done. + +## The lesson worth keeping + +An audit that only re-runs what the author ran finds nothing. This one found the +P1 by asking a question the author never asked — *what mode is that file?* — and +then measuring it instead of reasoning about it. + diff --git a/src/codex/auth-context.ts b/src/codex/auth-context.ts index c67844dd50..1f8b120112 100644 --- a/src/codex/auth-context.ts +++ b/src/codex/auth-context.ts @@ -161,7 +161,15 @@ function reportNativeMainFenceReason(reason: NativeMainStartupBlockReason): void ); } -/** Test-only: the dedup above is module state, so a second test would otherwise observe nothing. */ +/** + * Test-only reset for the dedup set above. + * + * The dedup is process-lifetime module state, so it is order-sensitive across test files + * sharing one Bun process: whichever file constructs this error first consumes the one-shot + * warn, and a later file asserting on it would see nothing and pass vacuously. Any test that + * asserts on the warn must call this first — an `afterEach` in the asserting file is not + * enough on its own, because the consuming file may not be the asserting one. + */ export function __resetNativeMainFenceReasonLog(): void { reportedFenceReasons.clear(); } diff --git a/src/service.ts b/src/service.ts index 37e77506dd..57610907b9 100644 --- a/src/service.ts +++ b/src/service.ts @@ -1541,7 +1541,11 @@ function taskXmlRunLevelAcceptable(principal: string): boolean { return value === "leastprivilege" || value === "highestavailable"; } -export function buildWindowsServiceScript(entry = cliEntry(), port = resolveServiceListenPort()): string { +export function buildWindowsServiceScript( + entry = cliEntry(), + port = resolveServiceListenPort(), + proxyEnv: { name: string; value: string }[] = resolvedProxyEnv(), +): string { // Provenance rides along with the entry: a second durableBunRuntime() call here could // resolve differently from the binary the caller actually baked. const { bun, bunRuntimeSource, cli } = entry; @@ -1559,7 +1563,7 @@ export function buildWindowsServiceScript(entry = cliEntry(), port = resolveServ windowsBatchSet("CODEX_HOME", process.env.CODEX_HOME?.trim(), "path"), windowsBatchSet("CODEX_SQLITE_HOME", currentCodexSqliteHomeAbsolute("windows"), "path"), windowsBatchSet("OPENCODEX_HOME", process.env.OPENCODEX_HOME?.trim(), "path"), - ...resolvedProxyEnv().map(({ name, value }) => windowsBatchSet(name, value)), + ...proxyEnv.map(({ name, value }) => windowsBatchSet(name, value)), windowsBatchSet("OCX_API_TOKEN_FILE", serviceApiTokenFilePath(), "path"), windowsBatchSet("OCX_SERVICE_LOG", serviceLogPath(), "path"), windowsBatchSet("OCX_BUN", bun, "path"), @@ -1881,7 +1885,7 @@ function installLaunchd(): void { // Capture this BEFORE writing: the write below makes the plist exist unconditionally, // so a post-write existsSync would call every fresh install an "installed" service. const wasInstalled = existsSync(p); - writeFileSync(p, buildPlist(), "utf8"); + writeServiceDefinitionFile(p, buildPlist(), "utf8"); // Best-effort: an absent job is fine here, and a failed unload is caught by the // load verification below with a better message than a raw unload error. runLaunchctl(["unload", p]); @@ -1944,6 +1948,27 @@ function uninstallLaunchd(): void { if (existsSync(p)) unlinkSync(p); } +/** + * Write a service definition with owner-only permissions. + * + * These files carry the outbound proxy environment (#2107), and a proxy URL routinely + * carries `user:password`. `writeFileSync` without a mode lands at 0644 under the default + * umask, so the credential would be world-readable on a shared host. Every other + * secret-bearing write in this file already uses 0600 — the service API token and the + * install state — and a service definition holding a proxy credential belongs in the same + * class. + * + * The explicit `chmodSync` is not redundant: `mode` only applies when the file is + * created, so an install over a definition left at 0644 by an earlier version would keep + * the loose mode. On Windows the POSIX bits are advisory, so the real ACL is applied + * there the same way the token file does it. + */ +export function writeServiceDefinitionFile(path: string, content: string, encoding: "utf8" | "utf16le"): void { + writeFileSync(path, content, { encoding, mode: 0o600 }); + try { chmodSync(path, 0o600); } catch { /* best-effort; the Windows ACL below is authoritative */ } + if (process.platform === "win32") hardenSecretPath(path, { required: false }); +} + // ── Windows (Task Scheduler) ── /** * In-place service-asset write that tolerates the transient EBUSY/EPERM/EACCES Windows @@ -1952,7 +1977,7 @@ function uninstallLaunchd(): void { function writeServiceAssetWithRetry(path: string, content: string, encoding: "utf8" | "utf16le"): void { for (let attempt = 0; ; attempt++) { try { - writeFileSync(path, content, encoding); + writeServiceDefinitionFile(path, content, encoding); return; } catch (err) { const code = (err as NodeJS.ErrnoException).code; @@ -2520,7 +2545,7 @@ function installSystemd(): void { recordOwnedConfigPath(getConfigDir(), serviceStatePath()); if (!existsSync(getConfigDir())) mkdirSync(getConfigDir(), { recursive: true }); writeServiceApiTokenFile(); - writeFileSync(unitPath(), buildUnit(), "utf8"); + writeServiceDefinitionFile(unitPath(), buildUnit(), "utf8"); sh("systemctl --user daemon-reload"); sh(`systemctl --user enable ${TASK}`); sh(`systemctl --user restart ${TASK}`); diff --git a/tests/codex-auth-context.test.ts b/tests/codex-auth-context.test.ts index e57bff1766..ade8606a40 100644 --- a/tests/codex-auth-context.test.ts +++ b/tests/codex-auth-context.test.ts @@ -1355,6 +1355,12 @@ describe("cooldown error surface", () => { // instead of remapping to Anthropic 529), and headers never survive to /api/logs, so stdout is // the only surface that reaches every path this fence fires on. describe("native-main fence names its gate reason", () => { + // Reset on BOTH sides: an afterEach only protects tests that run after this file, and the + // dedup is module state shared with every other file in the same process. + beforeEach(() => { + __resetNativeMainFenceReasonLog(); + }); + afterEach(() => { __resetNativeMainFenceReasonLog(); }); @@ -1394,7 +1400,7 @@ describe("native-main fence names its gate reason", () => { } }); - // auth-context.ts:326 throws the same error for the turn-drain fence (lifecycle.ts:180), which + // The claimMainProfile() site throws the same error for the turn-drain fence, which // is NOT the startup gate: the snapshot there reads `ready`. Inventing a reason for it would // send the next reboot report chasing a startup gate that never closed. test("the turn-drain fence stays silent instead of borrowing a startup reason", () => { diff --git a/tests/service.test.ts b/tests/service.test.ts index 32ba00c47c..04c8ef5ed3 100644 --- a/tests/service.test.ts +++ b/tests/service.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { isAbsolute, join, posix, win32 } from "node:path"; import * as serviceModule from "../src/service"; @@ -7,7 +7,7 @@ import { saveConfig } from "../src/config"; import { windowsEnvIndirectBatchValue } from "../src/lib/win-paths"; import { assertServiceAuthEnvironment, assertServiceEnvironmentMatchesInstall, bakedServicePathsDiagnostic, confirmServiceServing, launchdListenPort, systemdListenPort, buildPlist, buildUnit, buildWindowsLauncherVbs, buildWindowsSchtasksCreateArgs, buildWindowsSchtasksCreateArgsForXml, buildWindowsServiceScript, buildWindowsTaskXml, deriveWindowsServiceDiagnostic, installFreshWindowsSchedulerSafely, installServiceSafely, launchctlLoadFailed, launchdJobMatchesPlist, normalizeServiceSubcommand, parseServiceInstallState, prepareServiceInstall, readWindowsSchedulerXmlState, registerFreshWindowsSchedulerTask, removeNativeWindowsServiceForScheduler, repairService, resolveServiceListenPort, runLaunchctl, serviceLogPath, serviceStartableFromTray, serviceStatusReport, serviceRetryCommand, serviceStatusSummary, systemdNeedsDaemonReload, windowsListenPort, winswListenPort, startLaunchd, windowsTaskRegistrationHealthy } from "../src/service"; import type { ServiceDiagnostic } from "../src/service"; -import { resolvedProxyEnv } from "../src/service"; +import { resolvedProxyEnv, writeServiceDefinitionFile } from "../src/service"; import { buildWinswXml } from "../src/lib/winsw"; import { CONFIG_OWNER_FILE, CONFIG_UNINSTALL_MANIFEST, recordOwnedConfigPath, removeOwnedConfigState } from "../src/lib/config-ownership"; import { serviceApiTokenFilePath } from "../src/lib/service-secrets"; @@ -157,6 +157,23 @@ describe("systemd service unit", () => { expect(unit).not.toContain("http_proxy="); }); + test("the Windows wrapper bakes proxy env the same way the unit and plist do (#2107)", () => { + // This builder was the only one of the three with no proxy assertion, because the only way + // to reach it was to assign process.env — the pattern that leaked HTTP_PROXY across files. + const script = buildWindowsServiceScript( + { bun: "C:\\OpenCodex\\bun.exe", bunRuntimeSource: "bundled", cli: "C:\\OpenCodex\\cli.ts" }, + 10100, + resolvedProxyEnv({ HTTP_PROXY: "http://127.0.0.1:7890", no_proxy: "localhost" }), + ); + + expect(script).toContain("HTTP_PROXY=http://127.0.0.1:7890"); + // Lower-case spellings are baked under the canonical name, never both. + expect(script).toContain("NO_PROXY=localhost"); + expect(script).not.toContain("no_proxy="); + expect(script).not.toContain("HTTPS_PROXY="); + }); + + test("preserves custom Codex and OpenCodex homes", () => { const oldCodexHome = process.env.CODEX_HOME; const oldCodexSqliteHome = process.env.CODEX_SQLITE_HOME; @@ -199,7 +216,9 @@ describe("systemd service unit", () => { expect(startSystemd).toContain("ocx service install"); expect(startSystemd).toContain("process.exit(1)"); - const writeAt = installSystemd.indexOf('writeFileSync(unitPath(), buildUnit(), "utf8")'); + // The write goes through writeServiceDefinitionFile so the unit lands 0600: it can carry a + // proxy credential (#2107). What this test pins is the ORDER — write, then reload. + const writeAt = installSystemd.indexOf('writeServiceDefinitionFile(unitPath(), buildUnit(), "utf8")'); const reloadAt = installSystemd.indexOf("systemctl --user daemon-reload"); const enableAt = installSystemd.indexOf("systemctl --user enable"); const restartAt = installSystemd.indexOf("systemctl --user restart"); @@ -2152,3 +2171,52 @@ describe("service serving confirmation", () => { }); }); }); + +// #2107 baked the outbound proxy environment into the installed service definition, and a +// proxy URL routinely carries user:password. That made these files credential-bearing, so +// they must not be written at the umask default. +describe("service definitions are not world-readable", () => { + const modeOf = (path: string): string => (statSync(path).mode & 0o777).toString(8); + + test("a freshly written definition is owner-only", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-service-mode-")); + try { + const path = join(dir, "unit"); + writeServiceDefinitionFile(path, buildUnit(resolvedProxyEnv({ HTTP_PROXY: "http://u:p@127.0.0.1:7890" })), "utf8"); + + expect(modeOf(path)).toBe("600"); + // The credential is still written — this test pins who can read it, not that it is absent. + expect(readFileSync(path, "utf8")).toContain("u:p@127.0.0.1"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("an install over a loose definition from an older version tightens it", () => { + // `mode` applies only on creation, so a reinstall would otherwise leave 0644 standing. + const dir = mkdtempSync(join(tmpdir(), "ocx-service-mode-")); + try { + const path = join(dir, "plist"); + writeFileSync(path, "stale", { encoding: "utf8", mode: 0o644 }); + expect(modeOf(path)).toBe("644"); + + writeServiceDefinitionFile(path, buildPlist(resolvedProxyEnv({})), "utf8"); + + expect(modeOf(path)).toBe("600"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + test("utf16le scheduler assets take the same mode", () => { + const dir = mkdtempSync(join(tmpdir(), "ocx-service-mode-")); + try { + const path = join(dir, "task.xml"); + writeServiceDefinitionFile(path, "\uFEFF", "utf16le"); + + expect(modeOf(path)).toBe("600"); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); +});