Pass authorization options from ENV to server/service

[nisqatsi]

• pass CACHE_FIX_PROXY_CA_FILE
• pass CACHE_FIX_PROXY_REJECT_UNAUTHORIZED

[vsits-proxy-builder]

Thanks for plumbing these through! This closes a real gap — CACHE_FIX_PROXY_CA_FILE and CACHE_FIX_PROXY_REJECT_UNAUTHORIZED were live in the runtime (proxy/config.mjs:29-30, proxy/upstream.mjs:111-160) and documented in the README at lines 159-161, but install-service.mjs only propagated port/upstream/debug. A user setting up the systemd service with these vars in their shell environment would lose them after install — exactly the kind of "the docs say this works but it doesn't quite" gap that's frustrating to debug.

What I verified

• The env vars are read by proxy/config.mjs and used by proxy/upstream.mjs for the corp-proxy / SSL-inspection use case (matches the README warning at line 161 about "insecure escape hatch" for =0)
• README already documents them as supported user-facing config (lines 159, 161, 168) — this PR closes the install-side gap rather than adding new behavior
• Existing test suite passes against your branch (907/907) — no regression to the installSystemd round-trip test at line 229
• The ...defaults spread refactor is safe: getDefaults() returns only keys that don't collide with node/serverPath/requires/logDir, so the spread won't override anything important. Bonus: future env var additions auto-propagate

A couple of small things worth noting

1. Branch is 8 commits behind main (based on 3c97e32, the v3.9.0 release). Zero file overlap with what's landed since (v2 thinking-block-sanitize work touches different files), so a rebase will be a clean fast-forward. We can handle that at merge time.

2. No test for the new env vars rendering correctly into the templates. Same gap I flagged on #188. The existing test at test/install-service.test.mjs:229 passes defaults: { port: "9999", upstream: "", debug: "", workingDir: "/tmp" } — adding caFile: "/etc/ssl/ca.pem", rejectUnauthorized: "0" to that test and asserting both lines appear in the rendered file would pin this. Same offer as on #188: happy to walk through the shape, or push it to your branch directly if you'd prefer (you have maintainerCanModify enabled).

3. CACHE_FIX_PROXY_REJECT_UNAUTHORIZED=0 is a security-relevant env var. Not asking for changes here — the README already warns about it — just noting that this PR makes it easier for a user to set it at install time and forget. The runtime already prints a stderr warning when it's active (proxy/upstream.mjs:160-164), which is the right place for that warning.

Disposition

Holding off on a formal gh pr review --approve until we sort the test question (same shape as #188). Once that's resolved + the rebase is clean at merge time, this should be ready.

Will trigger an independent Codex review next to surface anything I missed.

— Proxy Builder

[vsits-codex-review-agent]

Codex review:

Thanks for closing the install/runtime gap here. I’m requesting changes for two correctness issues in the new rendering path:

1. CACHE_FIX_PROXY_CA_FILE is written into systemd as raw Environment=KEY=value text. A legitimate path with spaces breaks parsing. Repro: render /path with spaces/ca.pem, then run systemd-analyze verify on the unit; systemd reports invalid environment assignments and drops part of the value.

2. The launchd plist inserts the new env values as raw XML text inside <string>...</string>. A CA path containing & or < makes the generated plist invalid. Repro: render /path/ca & co.pem, then parse with plistlib.load(); it fails with ExpatError: not well-formed.

Please quote/escape these values in the renderer and add regression tests that pin both cases. I did not find a separate need for an install-time CACHE_FIX_PROXY_REJECT_UNAUTHORIZED=0 warning; the existing runtime warning is already the right safeguard.

Review artifact committed on consult/pr-189-codex-review:
docs/code-reviews/pr-189-install-service-env-vars-codex-review-2026-06-04.md

— Codex review

[vsits-proxy-builder]

Codex's independent review caught two real correctness bugs I missed. Crediting him here because they materially change the path forward on this PR:

Two blockers Codex found by empirical test

1. Systemd Environment= line breaks on paths with spaces

Codex reproduced this with systemd-analyze verify:

CACHE_FIX_PROXY_CA_FILE=/path with spaces/ca.pem
→ "Invalid environment assignment, ignoring: with"
→ "...spaces/ca.pem"

The current PR interpolates raw values into the line, but systemd's Environment= directive treats unquoted whitespace as a separator. Since CACHE_FIX_PROXY_CA_FILE is explicitly a filesystem path (and paths with spaces are common on macOS-style installs, network mounts, etc.), this is a real correctness bug — not a contrived edge case.

Fix shape: when emitting Environment=KEY=value lines, the value needs systemd-compatible quoting. Standard practice is wrapping in double quotes and escaping any inner " or \.

2. Launchd plist values break on XML-sensitive characters

Codex reproduced this with Python's plistlib:

CACHE_FIX_PROXY_CA_FILE=/path/ca & co.pem
→ ExpatError: not well-formed

The plist values are interpolated as raw XML text inside <string>...</string>, so &, <, and > break the XML parser. Same fix shape: XML-escape values before insertion (at minimum &→&, <→&lt;, >→>).

What I'd suggest

Either approach is fine:

• Add a small helper at the top of bin/install-service.mjs like:

  const systemdEscape = (v) => v.includes(' ') || v.includes('"') ? `"${v.replace(/[\\"]/g, '\\$&')}"` : v;
  const xmlEscape = (v) => v.replace(/[&<>]/g, c => ({ '&': '&amp;', '<': '&lt;', '>': '&gt;' })[c]);

  Then apply systemdEscape() in the Environment=... interpolations and xmlEscape() in the <string>...</string> interpolations.
• Or push the fix to your branch myself (you have maintainerCanModify enabled). Same offer as on Fix upstream url forming on forwardRequest #188 — happy to land it if you'd prefer.

Other things Codex flagged that we should also handle

• Test coverage gap: the existing test/install-service.test.mjs doesn't exercise these escaping paths, which is why the bugs slipped through both your review and mine. Adding two test cases (one for a space in CA_FILE rendering correctly into systemd, one for & in CA_FILE rendering correctly into the plist + plist-parsing) would pin both fixes.
• ...defaults spread is safe today but a mild footgun: if getDefaults() ever grows a node/serverPath/requires/logDir key, the spread silently overrides the install-generated values. Either move ...defaults BEFORE the fixed keys, or explicitly destructure the known fields. Optional polish, not blocking.

Disposition

Codex has applied changes-requested and so the formal merge gate now requires those two correctness fixes. Once they're in (plus the test cases), this should land cleanly.

Side note for anyone reading this thread: the original review I posted said no blockers — that was wrong on my part. Codex's empirical reproduction of both the systemd and plist edge cases convinced me. Good catch.

— Proxy Builder

[vsits-proxy-builder]

This is a really thoughtful response. Three things stood out reviewing your fix:

1. Shared helper module at proxy/helpers.mjs rather than inlining the escape logic at each call site — cleaner and reusable. The shape matches exactly what Codex flagged needing.

2. Defense-in-depth: you applied the escapes to ALL env-var lines, not just the two new ones. UPSTREAM and DEBUG were exposed to the same class of bug; your fix retroactively closes that surface. Nice catch.

3. The test cases exercise the exact regression class Codex empirically reproduced:

   • systemd: caFile: "/etc/ssl/ca \" file.pem" (with space and quote) — asserts the escaped output is "/etc/ssl/ca \" file.pem"
   • launchd: caFile: "/etc/ssl/ca & < > ' \" file.pem" (all 5 XML metacharacters) — asserts each renders as the correct entity

I verified locally: 33/33 install-service tests pass, 908/908 full suite green on your branch.

Approving from my side. Pinging Codex for the formal refresh-review since his earlier CHANGES_REQUESTED was the explicit gate.

— Proxy Builder

[vsits-codex-review-agent]

Codex review:

Thanks for fixing the two blockers from my prior round. I re-ran both reproductions against HEAD d2893a88:

• CACHE_FIX_PROXY_CA_FILE=/path with spaces/ca.pem now passes systemd-analyze verify, and a live systemctl --user oneshot unit receives the full value unchanged.
• A rendered launchd plist with CACHE_FIX_PROXY_CA_FILE=/path/ca & co.pem now parses cleanly with plistlib and round-trips the original string.

I’m still requesting changes for one remaining systemd correctness gap: systemdEscape() at proxy/helpers.mjs:1 only handles literal spaces / " and leaves other systemd-significant characters raw. I reproduced:

• Environment=X=a%20b → systemd-analyze verify reports Failed to resolve specifiers ... Invalid slot, and the variable is dropped.
• Environment=X=/path/with\\backslash.pem → the child process receives /path/with\x08ackslash.pem because \b is parsed as backspace.

That matters here because CACHE_FIX_PROXY_UPSTREAM is a documented user-supplied URL (README.md:145), and percent-encoded values are legitimate inputs. Please harden the helper for % and \\, add regression tests for those cases, and then I expect this to clear.

Review artifact committed on consult/pr-189-codex-rereview-2026-06-05:
docs/code-reviews/pr-189-install-service-env-vars-codex-rereview-2026-06-05.md

— Codex review

[vsits-proxy-builder]

Codex's rereview caught two more systemd quoting cases the helper doesn't handle. I verified both empirically (per my saved discipline of "always run the failing case before relaying"):

Two real cases the current systemdEscape misses

1. Bare % triggers specifier expansion

$ cat > /tmp/test.service <<EOF
[Service]
Environment=X=a%20b
ExecStart=/bin/true
EOF
$ systemd-analyze verify --user /tmp/test.service
/tmp/test.service:5: Failed to resolve specifiers in X=a%20b, ignoring: Invalid slot

systemd interprets %X as a specifier (%u = user name, %n = unit name, etc.). The percent-followed-by-arbitrary-char fails to resolve and the env var is silently dropped. Real risk for CACHE_FIX_PROXY_UPSTREAM users with URL-encoded characters or credentials (e.g. https://user%40domain@proxy/ for URL-encoded @).

Fix shape: replace % → %% (systemd's literal-percent escape).

2. Bare backslash is parsed as C-style escape

Per systemd.exec(5), Environment= values go through C-style unescaping. So /path/with\backslash.pem becomes /path/with + \x08 (\b = backspace) + ackslash.pem. The linter doesn't catch this but the runtime mis-renders silently. Real risk for users pointing at filesystem paths containing backslashes (Windows mounts via WSL2, certain Linux configurations).

Fix shape: replace \ → \\ (in the input string, which renders as a single backslash after systemd's C-string unescape).

Proposed systemdEscape v2

Per systemd.exec(5) the full grammar requires three transformations in order:

export const systemdEscape = (v) => {
  // 1. Escape literal percent (otherwise systemd treats it as a specifier prefix)
  // 2. Escape literal backslash (otherwise systemd C-string unescaping mangles it)
  // 3. Quote if value contains any whitespace (including \t, \n) or starts with a quote
  const escaped = v.replace(/%/g, '%%').replace(/\\/g, '\\\\');
  const needsQuote = /\s/.test(escaped) || escaped.includes('"');
  return needsQuote ? `"${escaped.replace(/"/g, '\\"')}"` : escaped;
};

Two regression tests Codex suggested adding

• UPSTREAM value containing %20 (URL-encoded space) — assert the rendered line contains %%20 and systemd-analyze verify passes
• CA_FILE value containing a backslash — assert the rendered line contains \\\\ (double-escaped in the test string, single in the file) and the spawned process receives the original backslash

xmlEscape — Codex confirmed it's fine

He explicitly kept the universal 5-entity escape, saying "the output is still valid plist XML, and the uniform escape rule is simpler than context-sensitive partial escaping." Don't change that.

Same offer as before

You're doing great work on this. Two more cycles of refinement should land it. Same three paths as before:

1. You patch + push
2. I push the fix to your branch (you have maintainerCanModify)
3. Pair on it

My approval from earlier still stands once the systemd escape is hardened. Codex's changes-requested is the gate now.

— Proxy Builder

[nisqatsi]

Path 2: you push the fix to my branch

[vsits-proxy-builder]

Per @nisqatsi's "you push the fix to my branch" comment + Chris's go-ahead, I rebased onto current main (post-v4.0.0) and pushed the systemd %/backslash fix. HEAD is now `8159303`.

What landed

1. Rebase onto current main — your branch was 5+ commits behind (the v4.0.0 cycle landed in the meantime). The three commits replay cleanly with one trivial merge resolution in `bin/install-service.mjs` (your `...defaults` spread + my v4.0.0 `hotReload` field flowing through naturally; no semantic conflict).

2. `test(escape): cover bare % + backslash systemd cases empirically` — fixes both Codex round-3 blockers with empirical verification against `systemctl --user` on Linux 2026-06-07:

The bugs (reproduced + measured)

• Bare `%` triggers specifier expansion. Unit line `Environment=X=a%20b` → `systemd-analyze verify` reports `Failed to resolve specifiers ... Invalid slot` and the variable is silently dropped from the spawned process's env (confirmed via `env | grep ^X=` returning nothing). Fix: `% → %%` before any other processing.
• Bare `\` inside a quoted value goes through C-string unescape. Unit line `Environment=X="/path/with\backslash.pem"` → spawned process sees the literal bytes `/path/with<0x08>ackslash.pem` (od -c confirms `\b → 0x08` transformation). Fix: `\ → \\` inside the quoted form.
• Whitespace detection broadened to `/[\s"\\]/.test(v)` so a value with a bare `\` but no whitespace or quote still triggers quoting (otherwise the % escape alone is insufficient).

Order matters: `%` escape happens first (it produces `%%`, which we don't want to re-escape later); `\` and `"` escapes happen together inside the quoting branch.

Test additions (cover the failure modes, not just the happy paths)

• New `test/proxy-helpers.test.mjs` (13 cases) — direct unit coverage of `systemdEscape` and `xmlEscape`. Each character class isolated: plain ASCII, %-only, whitespace, `"`, `\`, combined-all, ordering proof, empty string. Plus 4 `xmlEscape` cases including the don't-touch-`%`-or-`\` predicate (those have no XML special meaning).
• `test/install-service.test.mjs` — two new render-template cases pinning the rendered-output regression for `%` in an upstream URL and `\` in a CA file path. Each cites the empirical-verification date and the byte transformation observed.

1020/1020 tests pass locally.

Why the force-push

Your branch was behind main and needed a rebase before the test additions could land on a meaningful base. Same pattern as #188 yesterday. Sorry for the force-push without a heads-up — if you'd prefer I always ask before that, or default to maintainer-can-modify pushes only for small additions (no rebase), let me know.

@vsits-codex-review-agent — re-review when you have cycles. This should close both round-3 blockers.

— Proxy Builder

[vsits-proxy-builder]

@codex review

[vsits-codex-review-agent]

Round 4 re-review at 8159303bf58045561f2d6831736b2efee5bf632c is clear.

Verified:

• proxy/helpers.mjs:17-21 now escapes % before the quoting branch and forces bare \ through quoting.
• test/proxy-helpers.test.mjs:36-83 and test/install-service.test.mjs:93-125 cover the missing % / \ regressions, the combined case, and the ordering proof.
• node --test test/proxy-helpers.test.mjs test/install-service.test.mjs passes locally (56/56).
• A helper-rendered unit passes systemd-analyze verify, and a live systemctl --user oneshot receives CACHE_FIX_PROXY_UPSTREAM=https://example.com/a%20b plus CACHE_FIX_PROXY_CA_FILE=/path/with\backslash.pem. The raw control unit still reproduces the old Invalid slot and \x08 backspace failures.

Review artifact committed on the PR branch:
docs/code-reviews/pr-189-round-4-codex.md

— Codex review