mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-19 09:44:07 +00:00
fix(test): make the F1 gate test REAL — it was theatre, and proved it
The reviewer deleted the entire NODE_ENV gate from keys.mjs and the suite still reported
320 passed, 0 failed. The one test written to stop this bug recurring was the one thing in
the PR that would have let it recur — and it would have merged green, with a false sense of
coverage.
Why it was worthless: it re-implemented the predicate INSIDE THE TEST BODY —
const resolve = (nodeEnv, override) =>
(nodeEnv === "test" ? override : null) || join(homedir(), ".ocp");
— and never called resolveOcpDir(), getDb(), or getDbPath(). It asserted that a closure
defined three lines above behaved as written. A copy of the predicate is not the predicate.
Its own comment said "exercising the same predicate keys.mjs uses" — that phrase was the tell.
This is the same failure class the PR exists to indict (an assertion of an intention that
nothing enforces), reproduced one layer up, in the fix for it. Fourth time in this repo that
a correctly-named test has vouched for nothing.
The real test must run OUT OF PROCESS: the parent is irreversibly NODE_ENV=test by the time
any test runs (test-env.mjs sets it before keys.mjs is imported), so the production path is
simply unreachable in-process. It now spawns 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 asserts what the
REAL keys.mjs actually did.
MUTATION-PROVEN, against the exact revert that used to pass:
delete the whole NODE_ENV gate -> 319 passed, 1 failed
✗ a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE
restore -> 320 passed, 0 failed
Also folded in: setup.mjs created ~/.ocp at the umask default (755, world-listable) on a fresh
install via the logs dir — pre-existing, self-healing on first server start, now stated
explicitly (mode 0700) rather than left to luck. Same class as the F2 fix.
npm test: 320 passed, 0 failed. Real ~/.ocp/ocp.db unchanged at 751 rows.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR
This commit is contained in:
@@ -390,7 +390,7 @@ 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 });
|
||||
if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true, mode: 0o700 }) // 0700: this call can create ~/.ocp itself on a fresh install;
|
||||
|
||||
// Uninstall legacy service names if present (upgrade path)
|
||||
if (platform === "darwin") {
|
||||
|
||||
+25
-13
@@ -11,6 +11,8 @@ import { createSerialMutex, createTtlCache, isTokenExpiring, orderLabelsLastGood
|
||||
import { createHash } from "node:crypto";
|
||||
import { strict as assert } from "node:assert";
|
||||
import { join } from "node:path";
|
||||
import { pathToFileURL } from "node:url";
|
||||
import { execFileSync } from "node:child_process";
|
||||
import { homedir } from "node:os";
|
||||
|
||||
process.env.HOME = homedir(); // normalize HOME so homedir()-derived paths are stable across shells
|
||||
@@ -3826,19 +3828,29 @@ test("the key store under test is a scratch db, NOT the operator's real ~/.ocp/o
|
||||
});
|
||||
|
||||
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");
|
||||
// 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;
|
||||
const probe = `import { getDb, getDbPath, closeDb } from ${JSON.stringify(keysUrl)};
|
||||
getDb(); process.stdout.write(getDbPath()); closeDb();`;
|
||||
const env = { ...process.env, HOME: home, OCP_DIR_OVERRIDE: evil };
|
||||
delete env.NODE_ENV; // a production server has no NODE_ENV
|
||||
const out = execFileSync(process.execPath, ["--input-type=module", "-e", probe],
|
||||
{ env, encoding: "utf8" }).trim();
|
||||
assert.equal(out, join(home, ".ocp", "ocp.db"), "a prod process must open HOME/.ocp/ocp.db");
|
||||
assert.ok(!out.startsWith(evil), "a prod process must NEVER honor OCP_DIR_OVERRIDE");
|
||||
} 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", () => {
|
||||
|
||||
Reference in New Issue
Block a user