diff --git a/CHANGELOG.md b/CHANGELOG.md index 71d0a38..7914fc4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fix +- **#113** — CLI/installer hardening: ocp-plugin restart uses the live uid + `dev.ocp.proxy`/`ocp-proxy` labels and drops the unsafe pkill fallback; ocp-connect quotes + chmod 600 the persisted key; setup.mjs XML-escapes and newline-validates injected service-unit secrets. - **#112** — Recorded OAuth-host verification against compiled cli.js v2.1.154 (ALIGNMENT Class A); usage-probe and default request model now derive from `models.json` (ADR 0003 SPOT) instead of hardcoded IDs. ### Security diff --git a/ocp-connect b/ocp-connect index a6ced15..de268fa 100755 --- a/ocp-connect +++ b/ocp-connect @@ -634,11 +634,12 @@ PYEOF { echo "" echo "# OCP LAN (added by ocp connect)" - echo "export OPENAI_BASE_URL=$base_url/v1" + echo "export OPENAI_BASE_URL='$base_url/v1'" if [[ -n "$key" ]]; then - echo "export OPENAI_API_KEY=$key" + echo "export OPENAI_API_KEY='$key'" fi } >> "$rc_file" + chmod 600 "$rc_file" 2>/dev/null || true done echo " Shell config:" @@ -671,6 +672,7 @@ PYEOF echo "OPENAI_API_KEY=$key" fi } > "$env_dir/ocp.conf" + chmod 600 "$env_dir/ocp.conf" 2>/dev/null || true echo "" echo " System-level (systemd):" echo " ✓ $env_dir/ocp.conf" diff --git a/ocp-plugin/index.js b/ocp-plugin/index.js index 966a971..817ea10 100644 --- a/ocp-plugin/index.js +++ b/ocp-plugin/index.js @@ -208,31 +208,34 @@ async function cmdTest() { async function cmdRestart(args) { const target = (args || "").trim().toLowerCase(); const { execSync } = await import("node:child_process"); + const uid = typeof process.getuid === "function" ? process.getuid() : 501; + const macProxy = `launchctl kickstart -k gui/${uid}/dev.ocp.proxy`; + const macGateway = `launchctl kickstart -k gui/${uid}/ai.openclaw.gateway`; try { if (target === "gateway") { - execSync("launchctl kickstart -k gui/501/ai.openclaw.gateway", { timeout: 15000 }); + execSync(macGateway, { timeout: 15000 }); return "✓ Gateway restarted"; } else if (target === "all") { - execSync("launchctl kickstart -k gui/501/ai.openclaw.proxy", { timeout: 15000 }); + execSync(macProxy, { timeout: 15000 }); // Gateway restart will kill this plugin too, so do it last - execSync("launchctl kickstart -k gui/501/ai.openclaw.gateway", { timeout: 15000 }); + execSync(macGateway, { timeout: 15000 }); return "✓ Proxy + Gateway restarted"; } else { - execSync("launchctl kickstart -k gui/501/ai.openclaw.proxy", { timeout: 15000 }); + execSync(macProxy, { timeout: 15000 }); return "✓ Proxy restarted"; } } catch (e) { - // Try systemd for Linux + // Linux: systemd user services try { if (target === "gateway") { execSync("systemctl --user restart openclaw-gateway", { timeout: 15000 }); return "✓ Gateway restarted"; } else { - execSync("systemctl --user restart openclaw-proxy 2>/dev/null || pkill -f 'node.*server.mjs' && sleep 2 && cd ~/.openclaw/projects/*/; node server.mjs &", { timeout: 15000, shell: true }); + execSync("systemctl --user restart ocp-proxy", { timeout: 15000 }); return "✓ Proxy restarted"; } } catch (e2) { - return `✗ Restart failed: ${e2.message?.slice(0, 100)}`; + return `✗ Restart failed: ${e2.message?.slice(0, 100)}. Run \`ocp restart\` on the server host manually.`; } } } diff --git a/scripts/lib/plist-merge.mjs b/scripts/lib/plist-merge.mjs index 52683f5..7ef8ebc 100644 --- a/scripts/lib/plist-merge.mjs +++ b/scripts/lib/plist-merge.mjs @@ -9,6 +9,8 @@ // No new dependencies — regex-based, plist XY shape // is stable enough for our hand-written templates in setup.mjs. +// Note: setup.mjs XML-escapes all injected values before writing (via xmlEscape()), +// so raw `<` / `>` / `&` never appear in plist bodies — the [^<]* regex below is safe. const PLIST_KV_RE = /([^<]+)<\/key>\s*([^<]*)<\/string>/g; export function parsePlistEnv(plistContent) { diff --git a/setup.mjs b/setup.mjs index f356555..54c2fb1 100755 --- a/setup.mjs +++ b/setup.mjs @@ -65,6 +65,28 @@ const OCP_ADMIN_KEY_INJECT = process.env.OCP_ADMIN_KEY || null; // PROXY_ANONYMOUS_KEY — same pattern const PROXY_ANON_KEY_INJECT = process.env.PROXY_ANONYMOUS_KEY || null; +// ── Inject-value helpers ───────────────────────────────────────────────── +// Escape a value for safe inclusion in a plist … body. +function xmlEscape(v) { + return String(v).replace(/&/g, "&").replace(//g, ">").replace(/"/g, """).replace(/'/g, "'"); +} +// Validate an injected service value: no control chars (a newline would inject a +// rogue systemd Environment= directive; other control chars corrupt the unit/plist). +// Spaces are allowed — filesystem paths (CLAUDE_BIN) may legitimately contain them. +function assertSafeInjectValue(name, v) { + if (v == null) return v; + if (/[\x00-\x1f]/.test(String(v))) { + console.error(`FATAL: ${name} contains a newline or control character — refusing to write it into the service unit.`); + process.exit(1); + } + return v; +} + +// Validate all three INJECT values before they are written into any service unit. +assertSafeInjectValue("CLAUDE_BIN", CLAUDE_BIN_INJECT); +assertSafeInjectValue("OCP_ADMIN_KEY", OCP_ADMIN_KEY_INJECT); +assertSafeInjectValue("PROXY_ANONYMOUS_KEY", PROXY_ANON_KEY_INJECT); + // ── Models: derived from models.json (single source of truth) ────────── const modelsConfig = JSON.parse(readFileSync(join(__dirname, "models.json"), "utf-8")); @@ -403,17 +425,17 @@ if (!DRY_RUN) { EnvironmentVariables CLAUDE_PROXY_PORT - ${PORT} + ${xmlEscape(PORT)} CLAUDE_BIND - ${BIND_ADDRESS} + ${xmlEscape(BIND_ADDRESS)} CLAUDE_AUTH_MODE - ${AUTH_MODE_CONFIG}${CLAUDE_BIN_INJECT ? ` + ${xmlEscape(AUTH_MODE_CONFIG)}${CLAUDE_BIN_INJECT ? ` CLAUDE_BIN - ${CLAUDE_BIN_INJECT}` : ""}${OCP_ADMIN_KEY_INJECT ? ` + ${xmlEscape(CLAUDE_BIN_INJECT)}` : ""}${OCP_ADMIN_KEY_INJECT ? ` OCP_ADMIN_KEY - ${OCP_ADMIN_KEY_INJECT}` : ""}${PROXY_ANON_KEY_INJECT ? ` + ${xmlEscape(OCP_ADMIN_KEY_INJECT)}` : ""}${PROXY_ANON_KEY_INJECT ? ` PROXY_ANONYMOUS_KEY - ${PROXY_ANON_KEY_INJECT}` : ""} + ${xmlEscape(PROXY_ANON_KEY_INJECT)}` : ""} RunAtLoad diff --git a/test-features.mjs b/test-features.mjs index ffc36e8..c057897 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -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, "&").replace(//g, ">").replace(/"/g, """).replace(/'/g, "'"); +} +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&"\''), "a<b>&"'"); +}); + +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&\"'"); // "a<b>&"'" + const template = ` + + + EnvironmentVariables + + CLAUDE_AUTH_MODE + ${escaped} + + +`; + // 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");