fix: CLI/installer hardening — restart labels, key permissions, unit-secret escaping (#113) (#120)

Three CLI/installer findings from the 2026-05-31 audit (no server.mjs):

1. ocp-plugin `cmdRestart` hardcoded uid 501 + the legacy `ai.openclaw.proxy`
   label, and fell through to a dangerous `pkill -f 'node.*server.mjs' && cd
   ~/.openclaw/projects/*/; node server.mjs &` that could kill an unrelated node
   process and relaunch an env-less ghost proxy from a glob-ambiguous dir. Now uses
   process.getuid() + the live labels (dev.ocp.proxy on macOS, ocp-proxy on Linux
   systemd; the OpenClaw gateway label is unchanged) and drops the pkill fallback
   entirely (returns a manual `ocp restart` message on failure).

2. ocp-connect wrote the quota key unquoted into rc files and a world-readable
   environment.d/ocp.conf. Now single-quotes the value and chmod 600s the rc files
   and ocp.conf (matching the existing auth-profiles.json 0o600 convention).

3. setup.mjs interpolated the injected service-unit secrets (CLAUDE_BIN,
   OCP_ADMIN_KEY, PROXY_ANONYMOUS_KEY) raw into plist <string> and systemd
   Environment= lines. Added xmlEscape() for all plist <string> values and
   assertSafeInjectValue() which rejects control characters (\x00-\x1f — newline,
   CR, tab) before any unit is written, blocking a newline-injected rogue
   Environment= directive. Spaces are intentionally allowed (CLAUDE_BIN paths may
   contain them). XML-escaping on write also resolves plist-merge.mjs's [^<]* regex
   concern transitively (no raw < reaches it) — comment added, logic unchanged.

ALIGNMENT.md: CLI/installer scripts only, no Anthropic operation forwarded → cli.js
citation N/A. No blacklisted tokens or port literals introduced; alignment.yml passes.

Independent fresh-context reviewer (opus): APPROVE (Iron Rule 10) — verified the
control-char regex byte-exact via od (no space-rejection regression), the pkill
fallback fully removed, restart labels match setup.mjs ground truth, OCP keys
(base64url) cannot break the single-quoting, and the validator runs before any write.

Closes #113.

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dtzp555-max
2026-05-31 22:56:47 +10:00
committed by GitHub
co-authored by taodeng Claude Opus 4.8
parent 4a7d79c330
commit 68d58e7df4
6 changed files with 113 additions and 15 deletions
+68
View File
@@ -858,6 +858,74 @@ test("gcSnapshots keeps last N regardless of age", () => {
rmSync(root, { recursive: true, force: true });
});
// ── setup.mjs helpers: xmlEscape + assertSafeInjectValue ──
// setup.mjs cannot be imported (top-level side effects run the installer).
// Replicated verbatim from setup.mjs for unit-testing — keep in sync with source.
console.log("\nsetup.mjs inject helpers:");
function xmlEscape(v) {
return String(v).replace(/&/g, "&amp;").replace(/</g, "&lt;").replace(/>/g, "&gt;").replace(/"/g, "&quot;").replace(/'/g, "&apos;");
}
function assertSafeInjectValueTest(name, v) {
if (v == null) return v;
// eslint-disable-next-line no-control-regex
if (/[\x00-\x1f]/.test(String(v))) {
throw new Error(`FATAL: ${name} contains a newline or control character`);
}
return v;
}
test("xmlEscape encodes all five special XML chars", () => {
assert.equal(xmlEscape('a<b>&"\''), "a&lt;b&gt;&amp;&quot;&apos;");
});
test("xmlEscape leaves normal ocp_ token untouched", () => {
assert.equal(xmlEscape("ocp_abc123"), "ocp_abc123");
});
test("assertSafeInjectValue rejects value with newline", () => {
assert.throws(() => assertSafeInjectValueTest("OCP_ADMIN_KEY", "a\nb"), /FATAL/);
});
test("assertSafeInjectValue rejects value with carriage return", () => {
assert.throws(() => assertSafeInjectValueTest("OCP_ADMIN_KEY", "a\rb"), /FATAL/);
});
test("assertSafeInjectValue rejects value with a tab (control char)", () => {
assert.throws(() => assertSafeInjectValueTest("OCP_ADMIN_KEY", "a\tb"), /FATAL/);
});
test("assertSafeInjectValue ACCEPTS a path with a space (CLAUDE_BIN may legitimately contain one)", () => {
assert.equal(assertSafeInjectValueTest("CLAUDE_BIN", "/Users/x/My Apps/node"), "/Users/x/My Apps/node");
});
test("assertSafeInjectValue accepts normal ocp_ token", () => {
assert.doesNotThrow(() => assertSafeInjectValueTest("OCP_ADMIN_KEY", "ocp_abc123"));
});
test("assertSafeInjectValue accepts null (omit path)", () => {
assert.doesNotThrow(() => assertSafeInjectValueTest("OCP_ADMIN_KEY", null));
});
test("plist-merge round-trips XML-escaped value correctly via mergePlistEnv", () => {
// A value written with xmlEscape must survive a merge cycle — the [^<]* regex in
// parsePlistEnv only sees the escaped form (no raw < reaches it), so round-trip is safe.
const escaped = xmlEscape("a<b>&\"'"); // "a&lt;b&gt;&amp;&quot;&apos;"
const template = `<?xml version="1.0" encoding="UTF-8"?>
<plist version="1.0">
<dict>
<key>EnvironmentVariables</key>
<dict>
<key>CLAUDE_AUTH_MODE</key>
<string>${escaped}</string>
</dict>
</dict>
</plist>`;
// mergePlistEnv with no existing plist returns template unchanged.
const merged = mergePlistEnv(null, template);
assert.ok(merged.includes(escaped), "escaped value should survive unchanged through plist merge");
});
test("gcSnapshots keeps snapshots newer than keepDays regardless of count", () => {
const root = mkdtempSync(testJoin(tmpdir(), "ocp-gc-days-"));
const dotOcp = testJoin(root, ".ocp");