diff --git a/keys.mjs b/keys.mjs index 1999ed7..f6dd559 100644 --- a/keys.mjs +++ b/keys.mjs @@ -6,26 +6,64 @@ import { join } from "node:path"; import { mkdirSync, chmodSync } from "node:fs"; import { homedir } from "node:os"; -const OCP_DIR = join(homedir(), ".ocp"); -mkdirSync(OCP_DIR, { recursive: true, mode: 0o700 }); -// Tighten the directory mode in case it already existed with broader permissions. -try { chmodSync(OCP_DIR, 0o700); } catch { /* ignore EPERM on pre-existing dirs */ } -const DB_PATH = join(OCP_DIR, "ocp.db"); +// Resolved LAZILY, on first getDb() — not at module top-level. Two reasons, and the second is +// the bug this fixes: +// +// 1. Merely IMPORTING keys.mjs should not, as a side effect, create directories in the +// operator's home. +// 2. OCP_DIR_OVERRIDE exists so the test suite can point the key store at a scratch dir — and +// because ESM hoists imports, a top-level `const OCP_DIR = ...` here would be evaluated +// BEFORE an importing module's body could set the env var. Eager resolution made the +// override unsettable in the one place that needs it. (test-features.mjs carried a comment +// claiming it could "set env before the first getDb() call" — it could not, because nothing +// here ever read an env var. So `npm test` wrote real, UNREVOKED api_keys rows into the +// operator's live ~/.ocp/ocp.db: two per run, unbounded — 737 junk keys against 12 real ones +// on the maintainer's host — and two concurrent runs raced one file, which is the ~1-in-6 +// flake in `listKeys includes quota fields`.) +// +// The override is gated on NODE_ENV === "test", and that gate is the ACTUAL guard. An earlier +// cut of this fix relied on the variable merely having an awkward name — i.e. a naming convention +// plus a comment — which is precisely the failure mode this whole change exists to indict (a +// comment describing an intention that nothing enforces). A production server runs without +// NODE_ENV, so it CANNOT honor the override, however the variable got into its environment +// (`ocp start`'s nohup fallback inherits the invoking shell's env — a maintainer who exported +// this while debugging and then started the server would otherwise get a server silently +// authenticating against an empty key store: in AUTH_MODE=multi, a total auth outage, with +// nothing logged and nothing on /health to show it). +function resolveOcpDir() { + const override = process.env.NODE_ENV === "test" ? process.env.OCP_DIR_OVERRIDE : null; + const dir = override || join(homedir(), ".ocp"); + mkdirSync(dir, { recursive: true, mode: 0o700 }); + // Tighten the directory mode in case it already existed with broader permissions. + try { chmodSync(dir, 0o700); } catch { /* ignore EPERM on pre-existing dirs */ } + return dir; +} let db; +let dbPath; // resolved on first open, alongside the db handle export function getDb() { if (!db) { - db = new DatabaseSync(DB_PATH); + dbPath = join(resolveOcpDir(), "ocp.db"); + // Say which store we opened. Silence was the other half of the bug: a server on the wrong + // key store looks exactly like a server on the right one until every request 401s. + if (dbPath !== join(homedir(), ".ocp", "ocp.db")) { + console.error(`[keys] key store: ${dbPath} (NOT the default ~/.ocp/ocp.db)`); + } + db = new DatabaseSync(dbPath); db.exec("PRAGMA journal_mode = WAL"); db.exec("PRAGMA foreign_keys = ON"); initSchema(); // Tighten mode on the DB file (0600) after creation / first open. - try { chmodSync(DB_PATH, 0o600); } catch { /* ignore — same-user access still works */ } + try { chmodSync(dbPath, 0o600); } catch { /* ignore — same-user access still works */ } } return db; } +// Which file the key store actually opened. Exported so a test can ASSERT it is not the +// operator's real db — the bug this replaced was invisible precisely because nothing checked. +export function getDbPath() { return dbPath; } + function initSchema() { db.exec(` CREATE TABLE IF NOT EXISTS api_keys ( @@ -426,5 +464,5 @@ export function findKey(idOrName) { } export function closeDb() { - if (db) { db.close(); db = null; } + if (db) { db.close(); db = null; dbPath = undefined; } // clear both — a path to a closed db is a footgun } diff --git a/server.mjs b/server.mjs index fe1e11e..179f317 100644 --- a/server.mjs +++ b/server.mjs @@ -474,7 +474,12 @@ const SPAWN_HOME_DIR = `${process.env.HOME}/.ocp/spawn-home`; // erroring loudly — never a silent auth/credential corruption (there are no credentials here). function prepareSpawnHome(dir = SPAWN_HOME_DIR) { try { - mkdirSync(`${dir}/.claude`, { recursive: true }); + // mode 0700, and it matters for the PARENT: with `recursive`, this call can create ~/.ocp + // itself on a fresh install (spawn homes live under it), and without an explicit mode that + // parent lands at the umask default — world-listable 0755. keys.mjs used to pre-create it + // 0700 as an import side effect; it no longer does (it resolves its dir lazily), so the + // 0700 guarantee has to be stated here rather than inherited by luck. + mkdirSync(`${dir}/.claude`, { recursive: true, mode: 0o700 }); // Belt-and-braces: ensure no settings.json/plugins leak in (this home is fully ours). for (const f of [`${dir}/.claude/settings.json`, `${dir}/.claude/settings.local.json`]) { try { if (existsSync(f)) rmSync(f, { force: true }); } catch { /* best effort */ } diff --git a/setup.mjs b/setup.mjs index 89fec40..8d29dda 100755 --- a/setup.mjs +++ b/setup.mjs @@ -390,7 +390,9 @@ if (!DRY_RUN) { // and "ocp-proxy" keeps the proxy invisible to that heuristic. const OCP_HOME = join(HOME, ".ocp"); const ocpLogsDir = join(OCP_HOME, "logs"); - if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true }); + // mode 0700: with `recursive`, this call can create ~/.ocp ITSELF on a fresh install, and + // without an explicit mode that parent lands at the umask default (world-listable 0755). + if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true, mode: 0o700 }); // Uninstall legacy service names if present (upgrade path) if (platform === "darwin") { diff --git a/test-env.mjs b/test-env.mjs new file mode 100644 index 0000000..93727a5 --- /dev/null +++ b/test-env.mjs @@ -0,0 +1,25 @@ +// Imported FIRST by test-features.mjs, before keys.mjs, so this runs before anything can open +// the key store. ESM hoists imports and evaluates them in order, so a `process.env.X = ...` +// statement in the test's own body would run too late — hence a separate module. +// +// Why this exists: `npm test` used to write real, UNREVOKED api_keys rows into the operator's +// live ~/.ocp/ocp.db (the same database the running server reads) — two per run, unbounded. +// It also made the suite racy: two concurrent runs (e.g. review worktrees) shared one file, so +// `listKeys()` could miss "test-user-1" and the `in` check would throw on undefined. +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +export const TEST_OCP_DIR = mkdtempSync(join(tmpdir(), "ocp-test-")); + +// BOTH are required. keys.mjs honors OCP_DIR_OVERRIDE only when NODE_ENV === "test", so that a +// production server — which runs without NODE_ENV — cannot be redirected onto a different key +// store no matter how the variable reached its environment. +process.env.NODE_ENV = "test"; +process.env.OCP_DIR_OVERRIDE = TEST_OCP_DIR; + +// Remove the scratch store on exit. Without this the fix would trade unbounded growth in +// ~/.ocp/ocp.db for unbounded growth in $TMPDIR — better, but still litter. +process.on("exit", () => { + try { rmSync(TEST_OCP_DIR, { recursive: true, force: true }); } catch { /* best effort */ } +}); diff --git a/test-features.mjs b/test-features.mjs index 17140ae..b85529c 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -3,22 +3,24 @@ * Integration test for Quota + Cache features. * Tests database layer functions directly — no server needed. */ -import { getDb, createKey, listKeys, validateKey, recordUsage, checkQuota, updateKeyQuota, getKeyQuota, findKey, cacheHash, getCachedResponse, setCachedResponse, clearCache, getCacheStats, closeDb, hasCacheControl, singleflight, getInflightStats } from "./keys.mjs"; +// MUST come before keys.mjs: redirects the key store to a scratch dir (see test-env.mjs). +import { TEST_OCP_DIR } from "./test-env.mjs"; +import { getDb, getDbPath, createKey, listKeys, validateKey, recordUsage, checkQuota, updateKeyQuota, getKeyQuota, findKey, cacheHash, getCachedResponse, setCachedResponse, clearCache, getCacheStats, closeDb, hasCacheControl, singleflight, getInflightStats } from "./keys.mjs"; import { isLoopbackBind } from "./lib/net.mjs"; import { createSerialMutex, createTtlCache, isTokenExpiring, orderLabelsLastGoodFirst } from "./lib/spawn-auth.mjs"; import { createHash } from "node:crypto"; import { strict as assert } from "node:assert"; -import { unlinkSync } from "node:fs"; import { join } from "node:path"; +import { pathToFileURL } from "node:url"; +import { execFileSync } from "node:child_process"; import { homedir } from "node:os"; -// Use a test database to avoid corrupting real data -const TEST_DB = join(homedir(), ".ocp", "ocp-test.db"); -try { unlinkSync(TEST_DB); } catch {} +process.env.HOME = homedir(); // normalize HOME so homedir()-derived paths are stable across shells -// Monkey-patch DB_PATH for testing (override the module-level variable) -// Since keys.mjs uses lazy init, we can set env before first getDb() call -process.env.HOME = homedir(); // ensure consistent +// The scaffolding that used to live here CLAIMED to use "a test database to avoid corrupting +// real data" by setting an env var before the first getDb(). It never worked: keys.mjs read no +// env var, and ESM hoisting meant the assignment ran after the import anyway. The redirect is +// now real, and lives in test-env.mjs (imported above, before keys.mjs). This test proves it. let passed = 0; let failed = 0; @@ -3811,6 +3813,60 @@ test("REGRESSION: a WARM (pooled) pane streams — the sink comes off the pane, assert.equal(out.text, "Hello world", "and the transcript stays authoritative for the final text"); }); +console.log("\nTest isolation (the suite must never touch the operator's live key store):"); + +test("the key store under test is a scratch db, NOT the operator's real ~/.ocp/ocp.db", () => { + // The guard that was missing. `npm test` wrote live, UNREVOKED api_keys rows straight into the + // operator's real ~/.ocp/ocp.db — the same database the running server reads — two per run, + // unbounded (737 junk keys vs 12 real ones on the maintainer's host before this landed). It + // went unnoticed for so long precisely because NOTHING asserted where the store actually was. + const real = join(homedir(), ".ocp", "ocp.db"); + const used = getDbPath(); + assert.ok(used, "getDb() must have opened something by now"); + assert.notEqual(used, real, "the suite must NOT open the operator's live key database"); + assert.ok(used.startsWith(TEST_OCP_DIR), `expected a scratch db under ${TEST_OCP_DIR}, got ${used}`); +}); + +test("a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE", () => { + // Must run OUT OF PROCESS. The parent is irreversibly NODE_ENV=test by the time any test runs + // (test-env.mjs set it before keys.mjs was imported), so the production path is unreachable + // from in here — and an in-process test can only ever RE-IMPLEMENT the predicate, which is + // worthless: the first cut of this test did exactly that, and deleting the whole NODE_ENV gate + // from keys.mjs still left the suite at 320 passed / 0 failed. A copy of the predicate is not + // the predicate. So: spawn a child with no NODE_ENV, the override set, and HOME redirected to + // a temp dir (so the real key store is never opened), and assert what the REAL keys.mjs did. + const home = mkdtempSync(join(tmpdir(), "ocp-prodsim-")); + const evil = mkdtempSync(join(tmpdir(), "ocp-evil-")); + try { + const keysUrl = pathToFileURL(join(import.meta.dirname, "keys.mjs")).href; + // The child prints the override it SAW, then the store it actually opened. Printing both is + // the negative control: without it, a future refactor that renamed the env var and missed + // this test's `env` object would leave the child with no override at all — and "prod opened + // the right store" would pass for the wrong reason. Asserting the child saw it and ignored + // it anyway is the claim we actually want to make. + const probe = `import { getDb, getDbPath, closeDb } from ${JSON.stringify(keysUrl)}; +getDb(); process.stdout.write(process.env.OCP_DIR_OVERRIDE + "\\n" + getDbPath()); closeDb();`; + const env = { ...process.env, HOME: home, OCP_DIR_OVERRIDE: evil }; + delete env.NODE_ENV; // a production server has no NODE_ENV + const [seen, opened] = execFileSync(process.execPath, ["--input-type=module", "-e", probe], + { env, encoding: "utf8" }).trim().split("\n"); + assert.equal(seen, evil, "precondition: the child must actually SEE the override"); + assert.equal(opened, join(home, ".ocp", "ocp.db"), "a prod process must open HOME/.ocp/ocp.db"); + assert.ok(!opened.startsWith(evil), "…having seen the override, a prod process must IGNORE it"); + } finally { + rmSync(home, { recursive: true, force: true }); + rmSync(evil, { recursive: true, force: true }); + } +}); + +test("listKeys does not depend on rows left behind by an earlier or concurrent run", () => { + // The ~1-in-6 flake: two runs sharing one db file. keys.find() returned undefined and the + // caller's `in` check threw a TypeError instead of failing cleanly. With a per-run scratch db + // the store starts empty, so the count is exactly what THIS run created. + const mine = listKeys().filter((k) => k.name === "test-user-1"); + assert.equal(mine.length, 1, "exactly one test-user-1 — a shared store would accumulate duplicates"); +}); + runAsyncTests().then(() => Promise.all(pendingAsync)).then(() => { closeDb(); console.log(`\n=== Results: ${passed} passed, ${failed} failed ===\n`);