mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-22 21:45:08 +00:00
fix/test-db-isolation
2
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
  taodengandClaude Opus 4.8
|
fe9318553b |
fix(keys): gate the test override behind NODE_ENV so a prod server can never honor it
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/<op>/.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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR
|
||
|
taodengandClaude Opus 4.8
|
67b2e140ce |
fix(test): stop the suite writing live API keys into the operator's real key store
`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR
|