diff --git a/keys.mjs b/keys.mjs index f6dd559..ee7a624 100644 --- a/keys.mjs +++ b/keys.mjs @@ -24,12 +24,22 @@ import { homedir } from "node:os"; // 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). +// comment describing an intention that nothing enforces). The two-key gate means NEITHER var +// alone does anything: a stray OCP_DIR_OVERRIDE with no NODE_ENV is inert, and NODE_ENV=test with +// no override just resolves the default dir. +// +// This gate does NOT, by itself, prove a production daemon can't be redirected — an earlier +// version of this comment overclaimed that ("a production server runs without NODE_ENV, so it +// CANNOT honor the override no matter how the variable got in"). That is only true while the +// daemon's env actually lacks NODE_ENV=test, which is an assumption, not something this file can +// enforce. What makes it hold in the shipped configuration is defense-in-depth in OCP's launchers: +// the plist/systemd units strip both vars on every (re)install (scripts/lib/plist-merge.mjs +// NEVER_PRESERVE), and `ocp` restart's manual nohup fallback strips them (`env -u`). So a server +// OCP itself started cannot carry the test-only redirection. The one residual path is an operator +// who hand-launches `node server.mjs` with BOTH vars explicitly exported, bypassing every +// launcher — a case no library-level gate can catch. The loud getDb() log below ("NOT the default +// ~/.ocp/ocp.db") is the backstop there: a wrong key store is at least never silent (in +// AUTH_MODE=multi that would otherwise be a total auth outage with nothing on /health to show it). function resolveOcpDir() { const override = process.env.NODE_ENV === "test" ? process.env.OCP_DIR_OVERRIDE : null; const dir = override || join(homedir(), ".ocp"); diff --git a/ocp b/ocp index a30e889..1bb4616 100755 --- a/ocp +++ b/ocp @@ -622,7 +622,12 @@ cmd_restart() { self_r="${BASH_SOURCE[0]}" while [[ -L "$self_r" ]]; do self_r="$(readlink "$self_r")"; done script_dir="$(cd "$(dirname "$self_r")" && pwd)" - DISABLE_AUTOUPDATER=1 nohup node "$script_dir/server.mjs" >> "$HOME/.ocp/logs/proxy.log" 2>&1 & + # env -u strips test-only key-store redirection vars (A4): if the invoking shell had + # NODE_ENV=test + OCP_DIR_OVERRIDE exported (e.g. from a debugging session), this manual + # fallback would otherwise inherit them and start the daemon against a scratch/empty key + # store — a silent auth outage in AUTH_MODE=multi. The plist/systemd paths strip these via + # plist-merge's NEVER_PRESERVE; this covers the one direct-launch path OCP controls. + DISABLE_AUTOUPDATER=1 env -u NODE_ENV -u OCP_DIR_OVERRIDE nohup node "$script_dir/server.mjs" >> "$HOME/.ocp/logs/proxy.log" 2>&1 & fi sleep 3 if curl -sf --max-time 5 "$PROXY/health" > /dev/null 2>&1; then diff --git a/scripts/lib/plist-merge.mjs b/scripts/lib/plist-merge.mjs index 7ef8ebc..821da3b 100644 --- a/scripts/lib/plist-merge.mjs +++ b/scripts/lib/plist-merge.mjs @@ -8,6 +8,19 @@ // // No new dependencies — regex-based, plist XY shape // is stable enough for our hand-written templates in setup.mjs. +// +// SECURITY DENYLIST (A4): keys that must NEVER be carried into a service unit, even when a +// prior unit already contained them. OCP's key store honors OCP_DIR_OVERRIDE only when +// NODE_ENV === "test" (keys.mjs). If BOTH somehow reached a daemon's environment, the server +// would open a scratch/empty key store instead of ~/.ocp/ocp.db — in AUTH_MODE=multi a silent +// total auth outage. The preservation rule below ("keys only in EXISTING are kept verbatim") +// is exactly a vector for that: a unit that once carried these test-only vars would otherwise +// survive every setup re-run. So we strip them from the preserved set unconditionally. This is +// defense-in-depth: setup.mjs's own template never injects them, so the only way they enter is +// preservation, and this closes it. (The residual path — a hand-rolled `node server.mjs` with +// both vars exported — is out of any launcher's reach; keys.mjs's loud "NOT the default" log is +// the backstop there.) +export const NEVER_PRESERVE = new Set(["NODE_ENV", "OCP_DIR_OVERRIDE"]); // Note: setup.mjs XML-escapes all injected values before writing (via xmlEscape()), // so raw `<` / `>` / `&` never appear in plist bodies — the [^<]* regex below is safe. @@ -36,7 +49,7 @@ export function mergePlistEnv(existing, template) { const preserved = {}; for (const [k, v] of Object.entries(existingEnv)) { - if (!KNOWN.has(k)) preserved[k] = v; + if (!KNOWN.has(k) && !NEVER_PRESERVE.has(k)) preserved[k] = v; } if (Object.keys(preserved).length === 0) return template; @@ -72,7 +85,7 @@ export function mergeSystemdEnv(existing, template) { const KNOWN = new Set(Object.keys(templateEnv)); const preservedLines = Object.entries(existingEnv) - .filter(([k]) => !KNOWN.has(k)) + .filter(([k]) => !KNOWN.has(k) && !NEVER_PRESERVE.has(k)) .map(([k, v]) => `Environment=${k}=${v}`); if (preservedLines.length === 0) return template; diff --git a/test-env.mjs b/test-env.mjs index 93727a5..2ee13de 100644 --- a/test-env.mjs +++ b/test-env.mjs @@ -12,9 +12,10 @@ 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. +// BOTH are required. keys.mjs honors OCP_DIR_OVERRIDE only when NODE_ENV === "test", so neither +// var alone redirects anything — a stray OCP_DIR_OVERRIDE in a production env is inert without +// NODE_ENV=test alongside it. (A daemon OCP launches never carries either: the service units and +// the `ocp` restart fallback strip both — see plist-merge NEVER_PRESERVE / keys.mjs's comment.) process.env.NODE_ENV = "test"; process.env.OCP_DIR_OVERRIDE = TEST_OCP_DIR; diff --git a/test-features.mjs b/test-features.mjs index c4c2f72..e190c26 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -537,7 +537,7 @@ async function runSingleflightTests() { await runSingleflightTests(); // ── Plist Env Merge Tests ── -import { mergePlistEnv, mergeSystemdEnv } from "./scripts/lib/plist-merge.mjs"; +import { mergePlistEnv, mergeSystemdEnv, NEVER_PRESERVE } from "./scripts/lib/plist-merge.mjs"; console.log("\nPlist env merge:"); @@ -651,6 +651,78 @@ test("mergePlistEnv is idempotent", () => { assert.equal(mergePlistEnv(r1, SAMPLE_TEMPLATE_PLIST), r1); }); +// ── A4: security denylist — test-only key-store redirection vars must NEVER survive a setup +// re-run, even when a prior unit already carried them. Mutation-proof: drop the +// `!NEVER_PRESERVE.has(k)` guard in either merge fn and these fail (the vars get preserved). +test("NEVER_PRESERVE denylists exactly the two key-store redirection vars", () => { + assert.ok(NEVER_PRESERVE.has("NODE_ENV") && NEVER_PRESERVE.has("OCP_DIR_OVERRIDE")); + assert.equal(NEVER_PRESERVE.size, 2, "exactly two — a new entry needs its own rationale + test"); +}); + +const PLIST_EXISTING_WITH_TEST_VARS = ` + + + + Label + dev.ocp.proxy + EnvironmentVariables + + CLAUDE_PROXY_PORT + 3456 + CLAUDE_CACHE_TTL + 600 + NODE_ENV + test + OCP_DIR_OVERRIDE + /tmp/scratch-store + + +`; + +test("mergePlistEnv strips test-only redirection vars (A4) but keeps legit user keys", () => { + const merged = mergePlistEnv(PLIST_EXISTING_WITH_TEST_VARS, SAMPLE_TEMPLATE_PLIST); + assert.match(merged, /CLAUDE_CACHE_TTL<\/key>\s*600<\/string>/, "a legit user key is still preserved"); + assert.doesNotMatch(merged, /NODE_ENV<\/key>/, "NODE_ENV must never reach a service unit"); + assert.doesNotMatch(merged, /OCP_DIR_OVERRIDE/, "OCP_DIR_OVERRIDE must never reach a service unit (key or value)"); +}); + +test("mergePlistEnv: an existing unit whose ONLY extras are denylisted → template unchanged", () => { + const existing = ` + + + EnvironmentVariables + + CLAUDE_PROXY_PORT + 3456 + NODE_ENV + test + OCP_DIR_OVERRIDE + /tmp/scratch-store + + +`; + assert.equal(mergePlistEnv(existing, SAMPLE_TEMPLATE_PLIST), SAMPLE_TEMPLATE_PLIST, "nothing left to preserve → clean template"); +}); + +const SYSTEMD_EXISTING_WITH_TEST_VARS = `[Unit] +Description=OCP — Open Claude Proxy + +[Service] +ExecStart=/usr/bin/node /home/u/ocp/server.mjs +Environment=CLAUDE_PROXY_PORT=3456 +Environment=CLAUDE_CACHE_TTL=600 +Environment=NODE_ENV=test +Environment=OCP_DIR_OVERRIDE=/tmp/scratch-store +Restart=always +`; + +test("mergeSystemdEnv strips test-only redirection vars (A4) but keeps legit user keys", () => { + const merged = mergeSystemdEnv(SYSTEMD_EXISTING_WITH_TEST_VARS, SAMPLE_TEMPLATE_SYSTEMD); + assert.match(merged, /Environment=CLAUDE_CACHE_TTL=600/, "a legit user key is still preserved"); + assert.doesNotMatch(merged, /Environment=NODE_ENV=/, "NODE_ENV must never reach a service unit"); + assert.doesNotMatch(merged, /OCP_DIR_OVERRIDE/, "OCP_DIR_OVERRIDE must never reach a service unit"); +}); + test("mergeSystemdEnv is idempotent", () => { const r1 = mergeSystemdEnv(SAMPLE_EXISTING_SYSTEMD, SAMPLE_TEMPLATE_SYSTEMD); assert.equal(mergeSystemdEnv(r1, SAMPLE_TEMPLATE_SYSTEMD), r1);