From 7091958848ba5701bbb34fcfbea6caffddbf6d97 Mon Sep 17 00:00:00 2001 From: lidge-jun Date: Tue, 15 Sep 2026 14:20:58 +0900 Subject: [PATCH] fix(tests): stop a quota test from deleting the real OpenCodex home tests/usage/quota-reset-seen-store.test.ts forces a write failure by removing the config directory and putting a regular file in its place. It resolved that directory with getConfigDir(), which is the process-global home, so the removal followed whatever OPENCODEX_HOME happened to be. That is only bounded while the preload has installed a sandbox, and the preload is reached through bunfig.toml, which Bun resolves from the current working directory. Started from outside the repository the run loads no preload at all: OPENCODEX_HOME is unset, the guard is disarmed, and getConfigDir() returns the developer's real ~/.opencodex. On 2026-09-15 such a run deleted one, taking auth.json, codex-accounts.json, the service tokens and a 372MB usage ledger with it; every OAuth login on the machine was gone. assertNotRealHomeUnderTest could not help, because it guards writers and rmSync is not one. The file now creates its own home with mkdtempSync, pins OPENCODEX_HOME to it for the duration, restores the previous value afterwards, and names that directory in the destructive case instead of asking for the global one. tests/ci-workflows/test-home-guard.test.ts gains the invariant, asserted on the test sources because the directory is gone before any guarded call could run: no test may hand the process-global config directory to a destructive fs call. It was driven red against the original line and names the offending file. The claims in bunfig.toml and tests/preload.ts that the preload covers EVERY invocation are corrected to say what it actually covers, since believing them is how a bare getConfigDir() in a test looked safe. --- bunfig.toml | 15 +++++-- tests/ci-workflows/test-home-guard.test.ts | 52 ++++++++++++++++++++++ tests/preload.ts | 12 ++++- tests/usage/quota-reset-seen-store.test.ts | 40 +++++++++++++++-- 4 files changed, 110 insertions(+), 9 deletions(-) diff --git a/bunfig.toml b/bunfig.toml index 318845b44a..1a6ced2c5d 100644 --- a/bunfig.toml +++ b/bunfig.toml @@ -9,8 +9,15 @@ # The npm script already uses `bun test ./tests/`; this makes a bare `bun test` behave the same. [test] root = "tests" -# Sandboxes HOME/OPENCODEX_HOME/CODEX_HOME and arms the real-home write guard for EVERY -# invocation, including a bare `bun test ` that skips `scripts/test.ts`. A test -# once overwrote a real user config through that unwrapped path; see -# devlog/_fin/260730_codex_rs_upstream_v2_live_handoff/070. +# Sandboxes HOME/OPENCODEX_HOME/CODEX_HOME and arms the real-home write guard for every +# invocation THAT READS THIS FILE, including a bare `bun test ` that skips +# `scripts/test.ts`. A test once overwrote a real user config through that unwrapped path; +# see devlog/_fin/260730_codex_rs_upstream_v2_live_handoff/070. +# +# That qualifier is load-bearing. Bun resolves bunfig.toml from the CURRENT WORKING +# DIRECTORY, so `cd /tmp && bun test /tests/x.test.ts` loads no preload at all: nothing +# is sandboxed, the guard stays disarmed, and every `getConfigDir()` in that process returns +# the developer's real `~/.opencodex`. On 2026-09-15 such a run deleted one. A test that needs +# a config directory pins its own OPENCODEX_HOME instead of trusting this line, and +# `tests/ci-workflows/test-home-guard.test.ts` enforces that for the destructive case. preload = ["./tests/preload.ts"] diff --git a/tests/ci-workflows/test-home-guard.test.ts b/tests/ci-workflows/test-home-guard.test.ts index 47369ed663..acc8661c54 100644 --- a/tests/ci-workflows/test-home-guard.test.ts +++ b/tests/ci-workflows/test-home-guard.test.ts @@ -535,4 +535,56 @@ const canSymlink = (() => { expect(JSON.parse(probe.stdout.trim())).toEqual({ armed: true, rejected: true }); }); + /* + * The guard covers WRITERS, so a test that removes the config directory outright never + * reaches it: rmSync is plain node:fs, not a guarded writer. And the sandbox that would + * otherwise make the removal harmless is not universal — Bun resolves bunfig.toml, and with + * it the preload, from the CURRENT WORKING DIRECTORY. A run started outside the repository + * arms nothing, leaves OPENCODEX_HOME unset, and getConfigDir() then returns the developer's + * real ~/.opencodex. On 2026-09-15 a test did exactly that and deleted a live home: every + * OAuth login, the Codex account store, the service tokens and a 372MB usage ledger. + * + * Nothing runtime can be asserted here — the directory is gone before any guarded call runs + * — so the invariant is asserted on the test sources. A test that needs a config directory + * pins its own OPENCODEX_HOME and names that directory; none may hand the process-global one + * to a destructive fs call. + */ + test("no test file hands the process-global config directory to a destructive fs call", async () => { + const DESTRUCTIVE = "rmSync|rmdirSync|unlinkSync|renameSync|cpSync"; + const direct = new RegExp("\\b(?:" + DESTRUCTIVE + ")\\(\\s*getConfigDir\\(\\)"); + const bound = new RegExp("\\bconst\\s+([A-Za-z_$][\\w$]*)\\s*=\\s*getConfigDir\\(\\)"); + + // A line that merely NAMES the pattern is not a call: tests/cli/uninstall.test.ts asserts + // the CLI does not contain it, and the oracle at the end of this test is a literal. Both + // carry a quote on the line; a destructive call on a directory variable does not. + const isCode = (line: string): boolean => { + const trimmed = line.trim(); + if (trimmed.startsWith("//") || trimmed.startsWith("*") || trimmed.startsWith("/*")) return false; + return !trimmed.includes('"') && !trimmed.includes("'"); + }; + + const offenders = new Set(); + const testsDir = join(repoRoot(), "tests"); + for await (const relative of new Bun.Glob("**/*.test.ts").scan({ cwd: testsDir })) { + const lines = (await Bun.file(join(testsDir, relative)).text()).split("\n"); + const names = new Set(); + for (const line of lines) { + const found = bound.exec(line); + if (found) names.add(found[1]); + } + for (const line of lines) { + if (!isCode(line)) continue; + if (direct.test(line)) offenders.add(relative + ": getConfigDir() passed directly"); + for (const name of names) { + const viaName = new RegExp("\\b(?:" + DESTRUCTIVE + ")\\(\\s*" + name + "\\b"); + if (viaName.test(line)) offenders.add(relative + ": config dir removed via " + name); + } + } + } + + // The matcher must be able to see the shape it looks for, so an empty result is evidence + // rather than a silently broken regex. + expect(direct.test("rmSync(getConfigDir(), { recursive: true })")).toBe(true); + expect([...offenders].sort()).toEqual([]); + }); }); diff --git a/tests/preload.ts b/tests/preload.ts index a848a4aaf0..b8b0a83245 100644 --- a/tests/preload.ts +++ b/tests/preload.ts @@ -4,8 +4,16 @@ * `bun run test` already sandboxes HOME/OPENCODEX_HOME/CODEX_HOME through * `scripts/test.ts`. The incident this file prevents happened under a bare * `bun test ` — the command anyone reaches for while iterating on one test — - * which gets no wrapper and therefore had no isolation at all. A preload runs for - * EVERY invocation, so the protection no longer depends on remembering the wrapper. + * which gets no wrapper and therefore had no isolation at all. A preload runs for every + * invocation that READS bunfig.toml, so the protection no longer depends on remembering + * the wrapper. + * + * It does still depend on WHERE the run starts. Bun resolves bunfig.toml from the current + * working directory, so a run launched outside the repository never loads this file: no + * sandbox, no arming, and getConfigDir() resolves the real ~/.opencodex. On 2026-09-15 a + * run of that shape deleted a live home. Nothing here can close that hole from inside, so + * a test that needs a config directory pins its own OPENCODEX_HOME rather than inheriting + * one, and tests/ci-workflows/test-home-guard.test.ts enforces it for destructive calls. * (devlog `_plan/260730_codex_rs_upstream_v2_live_handoff/070`.) * * Import order below is load-bearing: importing the guard captures the real home at diff --git a/tests/usage/quota-reset-seen-store.test.ts b/tests/usage/quota-reset-seen-store.test.ts index 6689c26326..7594ed13b1 100644 --- a/tests/usage/quota-reset-seen-store.test.ts +++ b/tests/usage/quota-reset-seen-store.test.ts @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, test } from "bun:test"; -import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; import { join } from "node:path"; import { getConfigDir } from "../../src/config"; import type { QuotaResetEvent } from "../../src/quota/reset-detector"; @@ -15,6 +16,36 @@ import { swapLastObservedWindows, } from "../../src/quota/reset-seen-store"; +/** + * This file owns its config directory instead of inheriting one. + * + * Every case here resolves the process-global config home, and one of them DELETES it to + * force a write failure. That is bounded only while OPENCODEX_HOME points at a sandbox, and + * the preload that normally guarantees it does not cover every way this file can be run: Bun + * resolves `bunfig.toml` — and therefore its `preload = ["./tests/preload.ts"]` — from the + * CURRENT WORKING DIRECTORY. A run started outside the repository loads no preload, leaves + * OPENCODEX_HOME unset and the guard disarmed, and `getConfigDir()` then resolves the + * developer's real `~/.opencodex`. + * + * On 2026-09-15 exactly that invocation ran this file and deleted a live home. auth.json, + * codex-accounts.json, the service tokens and a 372MB usage ledger went with it; every OAuth + * login on the machine was gone, and only an unrelated three-week-old copy made any of it + * recoverable. The write guard could not help: `assertNotRealHomeUnderTest` covers + * writers, and `rmSync` is not one. + * + * Pinning the home here is what makes the deletion below safe under EITHER invocation. The + * previous value is restored afterwards because Bun reuses one process for several files. + */ +const PREVIOUS_OPENCODEX_HOME = process.env.OPENCODEX_HOME; +const ISOLATED_HOME = mkdtempSync(join(tmpdir(), "quota-reset-seen-store-")); +process.env.OPENCODEX_HOME = ISOLATED_HOME; + +afterAll(() => { + if (PREVIOUS_OPENCODEX_HOME === undefined) delete process.env.OPENCODEX_HOME; + else process.env.OPENCODEX_HOME = PREVIOUS_OPENCODEX_HOME; + rmSync(ISOLATED_HOME, { recursive: true, force: true }); +}); + const DAY = 24 * 60 * 60_000; /** * Real wall clock, not a fixed constant. @@ -75,7 +106,10 @@ describe("quota reset claim store", () => { // still reported a durable claim and the next start re-notified. // atomicWriteFile writes a sibling temp file in the config dir, so replacing that // directory with a regular file makes the real write fail without touching the module. - const configDir = getConfigDir(); + // This file's OWN directory, named directly: the store resolves the same path, and a + // destructive call must never be able to follow a config home it did not create. + const configDir = ISOLATED_HOME; + expect(getConfigDir()).toBe(configDir); rmSync(configDir, { recursive: true, force: true }); writeFileSync(configDir, "not a directory"); try {