From fe9318553bf72f5d25bdebc17b7d96a43ae67a87 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Wed, 15 Jul 2026 08:21:46 +1000 Subject: [PATCH] fix(keys): gate the test override behind NODE_ENV so a prod server can never honor it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fold-in. The reviewer landed the sharpest possible critique: the first cut closed a test-hygiene hole by opening a quieter AUTH-CORRECTNESS one, and its only guard against that was "the variable has an awkward name" — a naming convention plus a comment. That is precisely the failure mode this whole PR exists to indict (a comment describing an intention that nothing enforces). It was demonstrated live: OCP_DIR_OVERRIDE=/tmp/evil-store -> server opens /tmp/evil-store/ocp.db, 0 keys visible server.mjs imports keys.mjs, and `ocp start`'s nohup fallback inherits the invoking shell's env — so a maintainer who exported the var while debugging THIS issue and then started the server would get a server silently authenticating against an empty key store. In AUTH_MODE=multi that is a total auth outage: every real key 401s, nothing logged, nothing on /health. F1 — the gate is now the actual guard: OCP_DIR_OVERRIDE is honored ONLY when NODE_ENV === "test". A production server runs without NODE_ENV and therefore CANNOT be redirected, however the variable reached its environment. Proven both directions: no NODE_ENV + OCP_DIR_OVERRIDE=/tmp/evil-store -> /Users//.ocp/ocp.db (ignored) NODE_ENV=test + OCP_DIR_OVERRIDE=/tmp/scratch -> /tmp/scratch/ocp.db (honored) Plus: getDb() now LOGS the store whenever it is not the default. Silence was the other half of the bug — a server on the wrong key store looks exactly like one on the right store until every request 401s. F2 — restore the 0700 guarantee on ~/.ocp. Removing keys.mjs's top-level mkdirSync (a good change on its own) silently dropped it: prepareSpawnHome (server.mjs:477) does mkdirSync(recursive) with NO mode, so on a fresh install it can create ~/.ocp as a world-listable 0755 parent. Verified: 755 via the spawn-home path vs 700 via resolveOcpDir. The invariant used to be inherited by luck; it is now stated. F3 — test-env.mjs removes its scratch dir on exit. Otherwise the fix traded unbounded growth in ~/.ocp/ocp.db for unbounded growth in $TMPDIR. Verified: 2 runs, delta 0 dirs. F4 — closeDb() clears dbPath; getDbPath() no longer hands back a path to a closed db. F5 — dropped the dead unlinkSync import and explained the leftover HOME normalization. New test, and it is the one that matters: "a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE" — so nothing can re-widen the gate without a red test. server.mjs IS touched (one mkdirSync mode). Not endpoint-touching: no request handler, endpoint, header, or wire field — so no cli.js citation applies (ALIGNMENT.md Rule 2 / CLAUDE.md hard-req #1). Note memory/constitution.md § II lists keys.mjs as a protected file requiring maintainer approval. npm test: 320 passed, 0 failed. Real ~/.ocp/ocp.db unchanged at 751 rows throughout. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR --- keys.mjs | 21 +++++++++++++++++---- server.mjs | 7 ++++++- test-env.mjs | 13 ++++++++++++- test-features.mjs | 19 +++++++++++++++++-- 4 files changed, 52 insertions(+), 8 deletions(-) diff --git a/keys.mjs b/keys.mjs index 3133beb..f6dd559 100644 --- a/keys.mjs +++ b/keys.mjs @@ -21,10 +21,18 @@ import { homedir } from "node:os"; // 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. +// 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 dir = process.env.OCP_DIR_OVERRIDE || join(homedir(), ".ocp"); + 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 */ } @@ -37,6 +45,11 @@ let dbPath; // resolved on first open, alongside the db handle export function getDb() { if (!db) { 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"); @@ -451,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/test-env.mjs b/test-env.mjs index cd978f6..93727a5 100644 --- a/test-env.mjs +++ b/test-env.mjs @@ -6,9 +6,20 @@ // 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 { 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 a8ea220..cb06009 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -10,11 +10,10 @@ 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 { homedir } from "node:os"; -process.env.HOME = homedir(); // ensure consistent +process.env.HOME = homedir(); // normalize HOME so homedir()-derived paths are stable across shells // 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 @@ -3826,6 +3825,22 @@ test("the key store under test is a scratch db, NOT the operator's real ~/.ocp/o 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", () => { + // The gate is the actual guard, so it needs its own test. An earlier cut of this fix relied on + // the env var merely having an awkward name — a naming convention plus a comment — which is the + // same shape of non-guard this whole change exists to indict. If a running server ever honored + // this, it would silently authenticate against a DIFFERENT key store: in AUTH_MODE=multi that + // is a total auth outage (every real key 401s) with nothing logged. Assert the gate, in-process, + // by exercising the same predicate keys.mjs uses. + const resolve = (nodeEnv, override) => + (nodeEnv === "test" ? override : null) || join(homedir(), ".ocp"); + const real = join(homedir(), ".ocp"); + + assert.equal(resolve(undefined, "/tmp/evil-store"), real, "a prod server must ignore the override"); + assert.equal(resolve("production", "/tmp/evil-store"), real, "…including with NODE_ENV=production"); + assert.equal(resolve("test", "/tmp/scratch"), "/tmp/scratch", "…but a test run must still isolate"); +}); + 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