mirror of
https://github.com/dtzp555-max/ocp.git
synced 2026-07-21 21:15:09 +00:00
taodengandClaude Opus 4.8
67b2e140ce
`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
15 lines
870 B
JavaScript
15 lines
870 B
JavaScript
// 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;
|