fix(setup): never carry test-only key-store redirection vars into a server OCP launches (A4) (#165)

* fix(setup): never carry test-only key-store redirection vars into a server OCP launches (A4)

Defense-in-depth for the key-store isolation shipped in #163, plus a correction to the
overstated claim that fix's comments made. Surfaced by an independent (Codex) re-review.

Background: keys.mjs honors OCP_DIR_OVERRIDE only when NODE_ENV === "test", so the key store
can be pointed at a scratch dir for the test suite. If BOTH vars reached a production daemon's
environment, it would open a scratch/empty key store instead of ~/.ocp/ocp.db — in AUTH_MODE=multi
a silent total auth outage. #163's comments claimed a production server "runs without NODE_ENV, so
it CANNOT honor the override no matter how the variable got in." That is not something keys.mjs can
enforce — it is only true while the daemon's env happens to lack NODE_ENV=test. This PR makes it
true for every server OCP itself launches, and softens the docs to stop overclaiming.

Three parts (all in OCP's own launch/installer paths — no server.mjs change, no cli.js analogue):

1. scripts/lib/plist-merge.mjs — new exported NEVER_PRESERVE = {NODE_ENV, OCP_DIR_OVERRIDE},
   stripped from the preserved set in BOTH mergePlistEnv and mergeSystemdEnv. The preservation
   rule ("keys only in the EXISTING unit are kept verbatim") was the vector: a unit that once
   carried these test-only vars would otherwise survive every setup re-run. setup.mjs's template
   never injects them, so preservation was the only entry path, and this closes it.

2. ocp (cmd_restart manual fallback) — the one direct `node server.mjs` launch OCP controls now
   runs under `env -u NODE_ENV -u OCP_DIR_OVERRIDE`, so a maintainer who exported both while
   debugging and then restarted can't silently boot the daemon onto a scratch store.

3. keys.mjs + test-env.mjs — softened the overstated comments to state what is actually enforced
   (the two-key gate makes neither var alone do anything; OCP's launchers strip both) and to name
   the one residual path honestly: a hand-rolled `node server.mjs` with both vars explicitly
   exported, bypassing every launcher — for which the loud getDb() "NOT the default" log is the
   backstop. No library-level gate can catch an operator who both sets a test flag and bypasses
   the launchers; the honest fix is a non-silent wrong-store, which #163 already provides.

Severity: LOW (defense-in-depth; the default/shipped path was already safe). No behavior change on
any correctly-configured install.

ALIGNMENT.md: this PR does not touch server.mjs, so the cli.js-citation hard requirement does not
apply; and no cli.js operation is involved — key-store isolation and installer env hygiene are
entirely OCP-owned (no Class A / cli.js-mirror surface).

Tests: +4 mutation-proven (3 behavioral: drop the `!NEVER_PRESERVE.has(k)` guard in either merge
fn and they fail — verified 326 passed / 3 failed under mutation; restored). The `ocp` bash
`env -u` line is verified by `bash -n` + inspection (the suite does not exec the installer/daemon).
Full suite: 329 passed / 0 failed (was 325).

Version bump + CHANGELOG deferred to the later chore(release) PR, per the repo's #148/#149/#150 ->
#151 convention (matching PR #164).

Co-Authored-By: Claude <claude-opus-4-8> <noreply@anthropic.com>

* test(setup): assert NEVER_PRESERVE.size === 2 so the "exactly two" test matches its name

Reviewer nit (LOW): the membership assertion let a future spurious third entry slip past a
test whose name promises "exactly the two". Behavior stays guarded by the 3 mutation-proof
tests; this just makes the contract test honest.

Co-Authored-By: Claude <claude-opus-4-8> <noreply@anthropic.com>

---------

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude <claude-opus-4-8> <noreply@anthropic.com>

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
+// comment describing an intention that nothing enforces). The two-key gate means NEITHER var
-// NODE_ENV, so it CANNOT honor the override, however the variable got into its environment
+// alone does anything: a stray OCP_DIR_OVERRIDE with no NODE_ENV is inert, and NODE_ENV=test with
-// (`ocp start`'s nohup fallback inherits the invoking shell's env — a maintainer who exported
+// no override just resolves the default dir.
-// 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
+// This gate does NOT, by itself, prove a production daemon can't be redirected — an earlier
-// nothing logged and nothing on /health to show it).
+// 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");

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

scripts/lib/plist-merge.mjs

@@ -8,6 +8,19 @@
 //
 // No new dependencies — regex-based, plist <key>X</key><string>Y</string> 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 <string> 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;
 

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
+// BOTH are required. keys.mjs honors OCP_DIR_OVERRIDE only when NODE_ENV === "test", so neither
-// production server — which runs without NODE_ENV — cannot be redirected onto a different key
+// var alone redirects anything — a stray OCP_DIR_OVERRIDE in a production env is inert without
-// store no matter how the variable reached its environment.
+// 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;
 

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 = `<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
+<plist version="1.0">
+<dict>
+  <key>Label</key>
+  <string>dev.ocp.proxy</string>
+  <key>EnvironmentVariables</key>
+  <dict>
+    <key>CLAUDE_PROXY_PORT</key>
+    <string>3456</string>
+    <key>CLAUDE_CACHE_TTL</key>
+    <string>600</string>
+    <key>NODE_ENV</key>
+    <string>test</string>
+    <key>OCP_DIR_OVERRIDE</key>
+    <string>/tmp/scratch-store</string>
+  </dict>
+</dict>
+</plist>`;
+
+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, /<key>CLAUDE_CACHE_TTL<\/key>\s*<string>600<\/string>/, "a legit user key is still preserved");
+  assert.doesNotMatch(merged, /<key>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 = `<?xml version="1.0" encoding="UTF-8"?>
+<plist version="1.0">
+<dict>
+  <key>EnvironmentVariables</key>
+  <dict>
+    <key>CLAUDE_PROXY_PORT</key>
+    <string>3456</string>
+    <key>NODE_ENV</key>
+    <string>test</string>
+    <key>OCP_DIR_OVERRIDE</key>
+    <string>/tmp/scratch-store</string>
+  </dict>
+</dict>
+</plist>`;
+  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);