diff --git a/bin/install-service.mjs b/bin/install-service.mjs index ce10f929..351d034e 100644 --- a/bin/install-service.mjs +++ b/bin/install-service.mjs @@ -13,6 +13,7 @@ import { spawn } from "node:child_process"; import { fileURLToPath } from "node:url"; import { dirname, resolve, join } from "node:path"; import { homedir, platform } from "node:os"; +import { systemdEscape, xmlEscape } from "../proxy/helpers.mjs"; const __dirname = dirname(fileURLToPath(import.meta.url)); const TEMPLATE_DIR = resolve(__dirname, "..", "templates"); @@ -22,6 +23,8 @@ function getDefaults() { return { port: validatePort(process.env.CACHE_FIX_PROXY_PORT || "9801"), upstream: process.env.CACHE_FIX_PROXY_UPSTREAM || "", + caFile: process.env.CACHE_FIX_PROXY_CA_FILE || "", + rejectUnauthorized: process.env.CACHE_FIX_PROXY_REJECT_UNAUTHORIZED || "", debug: process.env.CACHE_FIX_DEBUG || "", // Hot-reload is opt-in as of v4.0.0 (#196). Capture from env at install // time so the operator can bake `CACHE_FIX_HOT_RELOAD=on` into the @@ -93,10 +96,16 @@ function getPaths(plat = platform()) { function renderSystemdTemplate(template, vars) { const upstreamLine = vars.upstream - ? `Environment=CACHE_FIX_PROXY_UPSTREAM=${vars.upstream}` + ? `Environment=CACHE_FIX_PROXY_UPSTREAM=${systemdEscape(vars.upstream)}` + : ""; + const caFileLine = vars.caFile + ? `Environment=CACHE_FIX_PROXY_CA_FILE=${systemdEscape(vars.caFile)}` + : ""; + const rejectUnauthorizedLine = vars.rejectUnauthorized + ? `Environment=CACHE_FIX_PROXY_REJECT_UNAUTHORIZED=${systemdEscape(vars.rejectUnauthorized)}` : ""; const debugLine = vars.debug - ? `Environment=CACHE_FIX_DEBUG=${vars.debug}` + ? `Environment=CACHE_FIX_DEBUG=${systemdEscape(vars.debug)}` : ""; const hotReloadLine = vars.hotReload ? `Environment=CACHE_FIX_HOT_RELOAD=${vars.hotReload}` @@ -111,6 +120,8 @@ function renderSystemdTemplate(template, vars) { .replaceAll("{{SERVER_PATH}}", vars.serverPath) .replaceAll("{{PORT}}", vars.port) .replaceAll("{{UPSTREAM_LINE}}", upstreamLine) + .replaceAll("{{CA_FILE_LINE}}", caFileLine) + .replaceAll("{{REJECT_UNAUTHORIZED_LINE}}", rejectUnauthorizedLine) .replaceAll("{{DEBUG_LINE}}", debugLine) .replaceAll("{{HOT_RELOAD_LINE}}", hotReloadLine) .replaceAll("{{REQUIRES_LINE}}", requiresLine) @@ -121,10 +132,16 @@ function renderSystemdTemplate(template, vars) { function renderLaunchdTemplate(template, vars) { const upstreamPlist = vars.upstream - ? ` CACHE_FIX_PROXY_UPSTREAM\n ${vars.upstream}` + ? ` CACHE_FIX_PROXY_UPSTREAM\n ${xmlEscape(vars.upstream)}` + : ""; + const caFilePlist = vars.caFile + ? ` CACHE_FIX_PROXY_CA_FILE\n ${xmlEscape(vars.caFile)}` + : ""; + const rejectUnauthorizedPlist = vars.rejectUnauthorized + ? ` CACHE_FIX_PROXY_REJECT_UNAUTHORIZED\n ${xmlEscape(vars.rejectUnauthorized)}` : ""; const debugPlist = vars.debug - ? ` CACHE_FIX_DEBUG\n ${vars.debug}` + ? ` CACHE_FIX_DEBUG\n ${xmlEscape(vars.debug)}` : ""; const hotReloadPlist = vars.hotReload ? ` CACHE_FIX_HOT_RELOAD\n ${vars.hotReload}` @@ -134,6 +151,8 @@ function renderLaunchdTemplate(template, vars) { .replaceAll("{{SERVER_PATH}}", vars.serverPath) .replaceAll("{{PORT}}", vars.port) .replaceAll("{{UPSTREAM_PLIST}}", upstreamPlist) + .replaceAll("{{CA_FILE_PLIST}}", caFilePlist) + .replaceAll("{{REJECT_UNAUTHORIZED_PLIST}}", rejectUnauthorizedPlist) .replaceAll("{{DEBUG_PLIST}}", debugPlist) .replaceAll("{{HOT_RELOAD_PLIST}}", hotReloadPlist) .replaceAll("{{WORKING_DIR}}", vars.workingDir) @@ -186,12 +205,8 @@ async function installSystemd({ paths, defaults, force = false } = {}) { const rendered = renderSystemdTemplate(template, { node: process.execPath, serverPath: SERVER_PATH, - port: defaults.port, - upstream: defaults.upstream, - debug: defaults.debug, - hotReload: defaults.hotReload, - workingDir: defaults.workingDir, requires: "", + ...defaults, }); await mkdir(paths.configDir, { recursive: true }); await writeFile(targetPath, rendered); @@ -286,12 +301,8 @@ async function installLaunchd({ paths, defaults, force = false } = {}) { const rendered = renderLaunchdTemplate(template, { node: process.execPath, serverPath: SERVER_PATH, - port: defaults.port, - upstream: defaults.upstream, - debug: defaults.debug, - hotReload: defaults.hotReload, - workingDir: defaults.workingDir, logDir: paths.logDir, + ...defaults, }); await mkdir(paths.configDir, { recursive: true }); await writeFile(targetPath, rendered); diff --git a/docs/code-reviews/pr-189-round-4-codex.md b/docs/code-reviews/pr-189-round-4-codex.md new file mode 100644 index 00000000..94937844 --- /dev/null +++ b/docs/code-reviews/pr-189-round-4-codex.md @@ -0,0 +1,43 @@ +# Review: PR #189 + +Date: 2026-06-08 +Reviewed: PR #189 at `8159303bf58045561f2d6831736b2efee5bf632c` +Round: 4 +Label applied: approved-by-codex-agent + +## What Is Correct + +`systemdEscape()` now closes the remaining round-3 gap in the helper itself. It escapes `%` before entering the quoting branch, and the quote trigger now includes bare backslashes, so a value containing `\` is forced down the quote-and-escape path instead of being emitted raw. Confirmed in `proxy/helpers.mjs:17-21`. + +The new helper-level regression tests cover the right behaviors: bare `%`, bare `\`, a combined `%`/space/`\`/`"` case, and an explicit ordering proof that `%` escaping happens before quote-wrapping. Confirmed in `test/proxy-helpers.test.mjs:36-83`. + +The renderer-level tests pin the rendered `Environment=` lines for the two concrete PR #189 regressions: percent-encoded upstream URLs and backslashes in CA-file paths. Confirmed in `test/install-service.test.mjs:93-125`. + +I also re-ran the empirical checks against HEAD. A helper-rendered systemd unit passed `systemd-analyze verify`, and a live `systemctl --user` oneshot unit received: + +- `CACHE_FIX_PROXY_UPSTREAM=https://example.com/a%20b` +- `CACHE_FIX_PROXY_CA_FILE=/path/with\backslash.pem` + +For control, the raw unescaped unit still reproduced the old failures: the upstream var was dropped with `Failed to resolve specifiers ... Invalid slot`, and the CA path arrived as `/path/with<0x08>ackslash.pem`. + +`node --test test/proxy-helpers.test.mjs test/install-service.test.mjs` passed locally (56/56). + +## Blockers + +None. + +## What Needs Attention + +None. + +## Bloat / Non-Functional + +None. + +## Recommendations + +Approve and merge once the PR label state is updated. + +## Bottom Line + +The remaining round-3 systemd escaping gap is closed at `8159303`. The helper logic is now correct for `%` and `\`, the new regression tests cover the previously missing cases, and the live user-manager repro matches the intended behavior. This is ready for approval. diff --git a/proxy/helpers.mjs b/proxy/helpers.mjs new file mode 100644 index 00000000..d207cfcd --- /dev/null +++ b/proxy/helpers.mjs @@ -0,0 +1,30 @@ +// Escape a value for safe rendering into a systemd `Environment=KEY=VALUE` line. +// +// Per systemd.exec(5) Environment= and systemd.unit(5) Specifier Expansion: +// - Literal `%` is the specifier-expansion marker; to embed one in a value +// the unit file must write `%%`. Without escaping, `a%20b` is parsed as +// a failed `%20` specifier expansion, systemd logs "Invalid slot" and +// silently drops the variable (empirically reproduced 2026-06-07). +// - Backslash is a C-string escape inside quoted strings AND inside the +// Environment= value parser; `\b` becomes byte 0x08 (backspace), `\n` +// becomes LF, etc. To embed a literal `\` the unit must write `\\`. +// - `"` requires `\"` (after the backslash escape rule above). +// - Whitespace requires the whole value to be quoted (`"..."`). +// +// Order matters: escape `%` first (it produces `%%`, neither of which we +// want to re-escape later), then handle `\` and `"` together inside the +// quoting branch. +export const systemdEscape = (v) => { + const percentEscaped = v.replace(/%/g, '%%'); + const needsQuoting = /[\s"\\]/.test(v); + if (!needsQuoting) return percentEscaped; + return `"${percentEscaped.replace(/[\\"]/g, '\\$&')}"`; +}; + +export const xmlEscape = (v) => v.replace(/[&<>'"]/g, c => ({ + '&': '&', + '<': '<', + '>': '>', + "'": ''', + '"': '"' +})[c]); diff --git a/templates/cache-fix-proxy.service.template b/templates/cache-fix-proxy.service.template index ce88cfad..37b6e248 100644 --- a/templates/cache-fix-proxy.service.template +++ b/templates/cache-fix-proxy.service.template @@ -10,6 +10,8 @@ Restart=on-failure RestartSec=5 Environment=CACHE_FIX_PROXY_PORT={{PORT}} {{UPSTREAM_LINE}} +{{CA_FILE_LINE}} +{{REJECT_UNAUTHORIZED_LINE}} {{DEBUG_LINE}} {{HOT_RELOAD_LINE}} WorkingDirectory={{WORKING_DIR}} diff --git a/templates/com.cnighswonger.cache-fix-proxy.plist.template b/templates/com.cnighswonger.cache-fix-proxy.plist.template index 5d37f146..ef3eab21 100644 --- a/templates/com.cnighswonger.cache-fix-proxy.plist.template +++ b/templates/com.cnighswonger.cache-fix-proxy.plist.template @@ -14,6 +14,8 @@ CACHE_FIX_PROXY_PORT {{PORT}} {{UPSTREAM_PLIST}} +{{CA_FILE_PLIST}} +{{REJECT_UNAUTHORIZED_PLIST}} {{DEBUG_PLIST}} {{HOT_RELOAD_PLIST}} diff --git a/test/install-service.test.mjs b/test/install-service.test.mjs index 71212d00..9330c08b 100644 --- a/test/install-service.test.mjs +++ b/test/install-service.test.mjs @@ -38,6 +38,8 @@ const sampleVars = { serverPath: "/opt/cache-fix/proxy/server.mjs", port: "9801", upstream: "", + caFile: "", + rejectUnauthorized: "", debug: "", workingDir: "/opt/cache-fix", requires: "", @@ -58,20 +60,26 @@ test("renderSystemdTemplate: omits empty optional Environment lines", async () = const tpl = await readFile(join(TEMPLATE_DIR, "cache-fix-proxy.service.template"), "utf-8"); const out = renderSystemdTemplate(tpl, sampleVars); assert.ok(!out.includes("CACHE_FIX_PROXY_UPSTREAM")); + assert.ok(!out.includes("CACHE_FIX_PROXY_CA_FILE")); + assert.ok(!out.includes("CACHE_FIX_PROXY_REJECT_UNAUTHORIZED")); assert.ok(!out.includes("CACHE_FIX_DEBUG")); // No leftover empty placeholders assert.ok(!out.includes("{{")); assert.ok(!out.includes("}}")); }); -test("renderSystemdTemplate: includes UPSTREAM and DEBUG when set", async () => { +test("renderSystemdTemplate: includes UPSTREAM, CA_FILE, REJECT_UNAUTHORIZED and DEBUG when set", async () => { const tpl = await readFile(join(TEMPLATE_DIR, "cache-fix-proxy.service.template"), "utf-8"); const out = renderSystemdTemplate(tpl, { ...sampleVars, upstream: "http://127.0.0.1:8080", + caFile: "/etc/ssl/ca \" file.pem", // with space and " + rejectUnauthorized: "0", debug: "1", }); assert.ok(out.includes("Environment=CACHE_FIX_PROXY_UPSTREAM=http://127.0.0.1:8080")); + assert.ok(out.includes("Environment=CACHE_FIX_PROXY_CA_FILE=\"/etc/ssl/ca \\\" file.pem\"")); + assert.ok(out.includes("Environment=CACHE_FIX_PROXY_REJECT_UNAUTHORIZED=0")); assert.ok(out.includes("Environment=CACHE_FIX_DEBUG=1")); }); @@ -82,6 +90,40 @@ test("renderSystemdTemplate: requires line wires both Requires and After", async assert.ok(out.includes("After=llm-relay.service")); }); +// PR #189 regression — bare % triggers systemd specifier expansion and +// silently drops the variable. Verified 2026-06-07 against `systemctl --user`: +// the unit line `Environment=X=a%%20b` delivers `a%20b` to the spawned +// process, while `Environment=X=a%20b` (unescaped) delivers an empty string +// after a "Failed to resolve specifiers ... Invalid slot" log entry. +test("renderSystemdTemplate: bare % in upstream URL is escaped to %% (PR #189)", async () => { + const tpl = await readFile(join(TEMPLATE_DIR, "cache-fix-proxy.service.template"), "utf-8"); + const out = renderSystemdTemplate(tpl, { + ...sampleVars, + upstream: "http://10.0.0.1:8080/path%20with%20encoded", + }); + assert.ok( + out.includes("Environment=CACHE_FIX_PROXY_UPSTREAM=http://10.0.0.1:8080/path%%20with%%20encoded"), + "bare % must be escaped to %% in the rendered Environment= line", + ); + assert.ok(!/=http:\/\/10\.0\.0\.1:8080\/path%20/.test(out), "no unescaped %20 should appear"); +}); + +// PR #189 regression — bare \ triggers systemd C-string unescape and +// produces a control byte. Verified 2026-06-07: `Environment=X=/path/with\backslash.pem` +// delivers /path/with<0x08>ackslash.pem to the process; the quoted form +// `Environment=X="/path/with\\backslash.pem"` delivers the literal value. +test("renderSystemdTemplate: backslash in CA file path is escaped to \\\\ (PR #189)", async () => { + const tpl = await readFile(join(TEMPLATE_DIR, "cache-fix-proxy.service.template"), "utf-8"); + const out = renderSystemdTemplate(tpl, { + ...sampleVars, + caFile: "/etc/ssl/with\\backslash.pem", + }); + assert.ok( + out.includes('Environment=CACHE_FIX_PROXY_CA_FILE="/etc/ssl/with\\\\backslash.pem"'), + "bare \\ must be escaped to \\\\ inside the quoted Environment= value", + ); +}); + test("renderLaunchdTemplate: substitutes core fields and renders valid plist", async () => { const tpl = await readFile( join(TEMPLATE_DIR, "com.cnighswonger.cache-fix-proxy.plist.template"), @@ -99,7 +141,31 @@ test("renderLaunchdTemplate: substitutes core fields and renders valid plist", a assert.ok(!out.includes("{{")); }); -test("renderLaunchdTemplate: omits CACHE_FIX_PROXY_UPSTREAM/DEBUG when not set", async () => { +test("renderLaunchdTemplate: includes UPSTREAM, CA_FILE, REJECT_UNAUTHORIZED and DEBUG when set", async () => { + const tpl = await readFile( + join(TEMPLATE_DIR, "com.cnighswonger.cache-fix-proxy.plist.template"), + "utf-8", + ); + const out = renderLaunchdTemplate(tpl, { + ...sampleVars, + upstream: "http://127.0.0.1:8080", + caFile: "/etc/ssl/ca & < > ' \" file.pem", // with XLM spec symbols + rejectUnauthorized: "0", + debug: "1", + logDir: "/Users/test/Library/Logs", + }); + assert.ok(out.includes("com.cnighswonger.cache-fix-proxy")); + assert.ok(out.includes("/usr/local/bin/node")); + assert.ok(out.includes("/opt/cache-fix/proxy/server.mjs")); + assert.ok(out.includes("9801")); + assert.ok(out.includes("http://127.0.0.1:8080")); + assert.ok(out.includes("/etc/ssl/ca & < > ' " file.pem")); + assert.ok(out.includes("0")); + assert.ok(out.includes("/Users/test/Library/Logs/cache-fix-proxy.log")); + assert.ok(!out.includes("{{")); +}); + +test("renderLaunchdTemplate: omits CACHE_FIX_PROXY_UPSTREAM/CA_FILE/REJECT_UNAUTHORIZED/DEBUG when not set", async () => { const tpl = await readFile( join(TEMPLATE_DIR, "com.cnighswonger.cache-fix-proxy.plist.template"), "utf-8", @@ -109,6 +175,8 @@ test("renderLaunchdTemplate: omits CACHE_FIX_PROXY_UPSTREAM/DEBUG when not set", logDir: "/tmp/logs", }); assert.ok(!out.includes("CACHE_FIX_PROXY_UPSTREAM")); + assert.ok(!out.includes("CACHE_FIX_PROXY_CA_FILE")); + assert.ok(!out.includes("CACHE_FIX_PROXY_REJECT_UNAUTHORIZED")); assert.ok(!out.includes("CACHE_FIX_DEBUG")); }); @@ -268,10 +336,12 @@ test("installSystemd: writes file to configDir; uninstall removes it", async () healthcheckServiceFile: "cache-fix-proxy-healthcheck.service", healthcheckTimerFile: "cache-fix-proxy-healthcheck.timer", }; - const r1 = await installSystemd({ paths, defaults: { port: "9999", upstream: "", debug: "", workingDir: "/tmp" } }); + const r1 = await installSystemd({ paths, defaults: { port: "9999", upstream: "", caFile: "/etc/ssl/ca.pem", rejectUnauthorized: "0", debug: "", workingDir: "/tmp" } }); assert.ok(r1.ok); const onDisk = await readFile(join(dir, "cache-fix-proxy.service"), "utf-8"); assert.ok(onDisk.includes("CACHE_FIX_PROXY_PORT=9999")); + assert.ok(onDisk.includes("CACHE_FIX_PROXY_CA_FILE=/etc/ssl/ca.pem")); + assert.ok(onDisk.includes("CACHE_FIX_PROXY_REJECT_UNAUTHORIZED=0")); const r2 = await uninstallSystemd({ paths }); assert.ok(r2.ok); diff --git a/test/proxy-helpers.test.mjs b/test/proxy-helpers.test.mjs new file mode 100644 index 00000000..48bb46cb --- /dev/null +++ b/test/proxy-helpers.test.mjs @@ -0,0 +1,120 @@ +// Unit tests for the systemd / launchd escape helpers in proxy/helpers.mjs. +// +// Why these are unit-shaped: the install-service render tests exercise the +// helpers indirectly via the rendered output, but each helper has its own +// edge cases (specifier expansion, C-string escapes, XML entities) that +// deserve direct coverage so future refactors of the helper can't silently +// regress the per-character semantics. +// +// Each case in `systemdEscape` was empirically verified 2026-06-07 against +// `systemd --user` on a Linux host: the unit file rendered by the expected +// output delivers the original input to the spawned process byte-for-byte. + +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { systemdEscape, xmlEscape } from "../proxy/helpers.mjs"; + +// --- systemdEscape --- +// +// Per systemd.exec(5) Environment= and systemd.unit(5) Specifier Expansion: +// - literal % must be written %% (otherwise systemd tries specifier +// expansion, logs "Invalid slot", and silently drops the variable) +// - literal \ must be written \\ (otherwise systemd applies C-string +// escapes; \b → backspace, \n → newline, etc.) +// - literal " must be written \" +// - whitespace requires the value to be quoted + +test("systemdEscape: plain ASCII value with no special chars passes through unchanged", () => { + assert.equal(systemdEscape("plain"), "plain"); + assert.equal(systemdEscape("http://127.0.0.1:8080"), "http://127.0.0.1:8080"); + assert.equal(systemdEscape("/etc/ssl/ca.pem"), "/etc/ssl/ca.pem"); + assert.equal(systemdEscape("0"), "0"); + assert.equal(systemdEscape("1"), "1"); +}); + +test("systemdEscape: bare % is escaped to %% even without quoting (PR #189 regression — specifier expansion)", () => { + // Reproduced 2026-06-07: an Environment= line containing bare % causes + // systemd-analyze verify to report "Failed to resolve specifiers ... + // Invalid slot" and the variable to be silently dropped. + assert.equal(systemdEscape("a%20b"), "a%%20b"); + assert.equal(systemdEscape("100%"), "100%%"); + assert.equal(systemdEscape("%%"), "%%%%"); +}); + +test("systemdEscape: whitespace triggers quoting", () => { + assert.equal(systemdEscape("a b"), '"a b"'); + assert.equal(systemdEscape("/path with spaces/ca.pem"), '"/path with spaces/ca.pem"'); + // Tabs and newlines also count as whitespace. + assert.equal(systemdEscape("a\tb"), '"a\tb"'); +}); + +test("systemdEscape: literal \" triggers quoting and is escaped to \\\"", () => { + assert.equal(systemdEscape('a"b'), '"a\\"b"'); +}); + +test("systemdEscape: literal \\ is escaped to \\\\ inside quoted strings (PR #189 regression — C-string escape)", () => { + // Reproduced 2026-06-07: Environment=X=/path/with\backslash.pem causes + // systemd to deliver /path/with<0x08>ackslash.pem (the \b becomes a + // backspace byte) because Environment= values pass through the C-string + // unescape. + assert.equal( + systemdEscape("/path/with\\backslash.pem"), + '"/path/with\\\\backslash.pem"', + ); + // A bare backslash anywhere triggers quoting (it's a special char even + // without whitespace or other special chars present). + assert.equal(systemdEscape("a\\b"), '"a\\\\b"'); +}); + +test("systemdEscape: combined %, space, \\, \" all in one value", () => { + assert.equal( + systemdEscape('mixed%and spaces and \\ and " all'), + '"mixed%%and spaces and \\\\ and \\" all"', + ); +}); + +test("systemdEscape: order is %-escape first, then quote-wrap with \\ and \" escaped together", () => { + // % escaping must happen BEFORE the quoting branch's backslash escape, + // otherwise `%` in a value that also needs quoting would be missed. + // (Empirically observed during PR #189 round 1 — the prior helper + // didn't escape % at all, regardless of quoting state.) + assert.equal(systemdEscape("has% and space"), '"has%% and space"'); +}); + +test("systemdEscape: empty string passes through unchanged", () => { + assert.equal(systemdEscape(""), ""); +}); + +// --- xmlEscape --- +// +// For launchd plist values. The five XML predefined entities. + +test("xmlEscape: plain ASCII passes through unchanged", () => { + assert.equal(xmlEscape("plain"), "plain"); + assert.equal(xmlEscape("/etc/ssl/ca.pem"), "/etc/ssl/ca.pem"); +}); + +test("xmlEscape: all five predefined entities", () => { + assert.equal(xmlEscape("&"), "&"); + assert.equal(xmlEscape("<"), "<"); + assert.equal(xmlEscape(">"), ">"); + assert.equal(xmlEscape("'"), "'"); + assert.equal(xmlEscape('"'), """); +}); + +test("xmlEscape: combined all five in one value", () => { + assert.equal( + xmlEscape("/etc/ssl/ca & < > ' \" file.pem"), + "/etc/ssl/ca & < > ' " file.pem", + ); +}); + +test("xmlEscape: empty string passes through unchanged", () => { + assert.equal(xmlEscape(""), ""); +}); + +test("xmlEscape: does NOT touch % or \\ — those have no XML special meaning", () => { + assert.equal(xmlEscape("a%20b"), "a%20b"); + assert.equal(xmlEscape("/path/with\\backslash.pem"), "/path/with\\backslash.pem"); +});