From 67b2e140ce2af35bf39186526420d259be7ad746 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Wed, 15 Jul 2026 08:10:37 +1000 Subject: [PATCH] fix(test): stop the suite writing live API keys into the operator's real key store MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `npm test` wrote real, UNREVOKED rows into ~/.ocp/ocp.db — the SAME sqlite database the running server reads — two per run, unbounded. On the maintainer's host that had accumulated **737 test-suite keys against 12 real operator keys** (749 rows total), all revoked=0. Not a credential leak: createKey() mints `randomBytes(24)` and the suite discards the plaintext, so nobody holds a usable token. But it is real damage: - the operator's key store grows by 2 rows on every test run, forever - `ocp keys list` is unusable (749 rows, 12 of them real) - the suite is RACY: two concurrent runs (e.g. two review worktrees) share one file, so listKeys() can miss "test-user-1" and `"quota_daily" in undefined` throws a TypeError rather than failing cleanly. That is the ~1-in-6 flake in `listKeys includes quota fields`, reported by a reviewer and initially not reproducible serially — it needs a concurrent run to surface, which is exactly what four parallel reviewers produced. Root cause: keys.mjs resolved `OCP_DIR`/`DB_PATH` at MODULE TOP-LEVEL and read no env var. test-features.mjs carried a comment claiming it could "set env before the first getDb() call" — it could not, on two counts: nothing in keys.mjs read an env var, and ESM hoists imports, so the assignment would have run after keys.mjs was already evaluated anyway. The isolation was never real; it just LOOKED real, which is why it survived. Fix: - keys.mjs resolves the dir lazily, inside getDb(), honoring OCP_DIR_OVERRIDE. Deliberately NOT a generic `OCP_DIR`: pointing a RUNNING server at a different key store silently changes which credentials authenticate, so this must be awkward to set by accident. - new test-env.mjs, imported BEFORE keys.mjs, mints a per-run scratch dir. A separate module is required — ESM hoisting means a statement in the test's own body is too late. - export getDbPath() so the store's location can be asserted. - as a side effect, importing keys.mjs no longer creates directories in the operator's home. Two guards added, both MUTATION-TESTED (revert the override → both fail, 317/2): - the key store under test is a scratch db, NOT the operator's real ~/.ocp/ocp.db - listKeys does not depend on rows left behind by an earlier or concurrent run Proven, not asserted: the real ~/.ocp/ocp.db held at 749 rows across two full test runs (it previously grew by 2 each run). The 737 existing junk rows are NOT cleaned up here — that is a destructive change to the maintainer's live database and is his call, not a side effect of a test fix. npm test: 319 passed, 0 failed (was 317). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR --- keys.mjs | 39 ++++++++++++++++++++++++++++++++------- test-env.mjs | 14 ++++++++++++++ test-features.mjs | 37 ++++++++++++++++++++++++++++++------- 3 files changed, 76 insertions(+), 14 deletions(-) create mode 100644 test-env.mjs diff --git a/keys.mjs b/keys.mjs index 1999ed7..3133beb 100644 --- a/keys.mjs +++ b/keys.mjs @@ -6,26 +6,51 @@ 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`.) +// +// Deliberately NOT a generic name like OCP_DIR: this must be awkward to set by accident, because +// pointing a RUNNING server at a different key store silently changes which credentials authenticate. +function resolveOcpDir() { + const dir = process.env.OCP_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"); + 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 ( diff --git a/test-env.mjs b/test-env.mjs new file mode 100644 index 0000000..cd978f6 --- /dev/null +++ b/test-env.mjs @@ -0,0 +1,14 @@ +// 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 } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +export const TEST_OCP_DIR = mkdtempSync(join(tmpdir(), "ocp-test-")); +process.env.OCP_DIR_OVERRIDE = TEST_OCP_DIR; diff --git a/test-features.mjs b/test-features.mjs index 17140ae..a8ea220 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -3,7 +3,9 @@ * 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"; @@ -12,14 +14,13 @@ import { unlinkSync } from "node:fs"; import { join } from "node:path"; 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 {} - -// 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 +3812,28 @@ 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("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`);