From 163692a19b48ade546845dc2d5ef5c37ce55952b Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 00:10:32 +0900 Subject: [PATCH 1/2] fix(service): write service definitions owner-only, they can carry a proxy credential MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #2107 baked the outbound proxy environment into the installed service definition. A proxy URL routinely carries user:password, which quietly made those files credential-bearing — and they were still written with a bare writeFileSync, so under the default umask 022 they landed at 0644, world-readable on a shared host. The precedent was already in the same file and was not followed: the service API token and the install state both write { mode: 0o600 } plus a chmodSync. This routes the plist, the systemd unit, and the Windows scheduler assets through one writeServiceDefinitionFile() that does the same. The explicit chmodSync is not redundant. mode applies only when a file is created, so an install over a definition left at 0644 by an earlier version would otherwise keep the loose mode — which is the realistic upgrade path here, not a hypothetical. Also in this change, both from the same audit: buildWindowsServiceScript now takes the resolved proxy entries the way buildUnit and buildPlist already do. It was the only one of the three builders with no proxy assertion at all, because the only way to reach it was to assign process.env — the exact pattern whose leak this stack just finished removing. It now has a regression covering the canonical-name rule. __resetNativeMainFenceReasonLog is documented as an order-sensitive contract rather than a convenience, and its caller resets on both sides. The dedup set is process-lifetime module state: whichever file constructs the error first consumes the one-shot warn, so an afterEach in the asserting file would not have saved a later assertion from passing vacuously. Verification: red-driven — with the mode argument removed the three new assertions report 644 against an expected 600. After: 333 pass / 0 fail across service, codex-auth-context, the three Lab suites, api-key-attribution and doctor. tsc --noEmit exit 0. privacy:scan exit 0. --- src/codex/auth-context.ts | 10 ++++- src/service.ts | 35 ++++++++++++--- tests/codex-auth-context.test.ts | 8 +++- tests/service.test.ts | 74 ++++++++++++++++++++++++++++++-- 4 files changed, 117 insertions(+), 10 deletions(-) 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 }); + } + }); +}); From 52f85f6652852097e5aa7ea2e40cd87431204e1e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Thu, 20 Aug 2026 00:11:02 +0900 Subject: [PATCH 2/2] docs(devlog): record the final audit and the 0644 credential exposure it caught --- .../140_final_audit.md | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 devlog/_plan/260819_unclaimed_bug_selection/140_final_audit.md 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. +