fix(service): write service definitions owner-only, they can carry a proxy credential

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.
+

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();
 }

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}`);

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", () => {

tests/service.test.ts

@@ -1,13 +1,13 @@
 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";
 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<Task />", "utf16le");
+
+      expect(modeOf(path)).toBe("600");
+    } finally {
+      rmSync(dir, { recursive: true, force: true });
+    }
+  });
+});