fix(daemon): forward proxy env vars into managed daemon service definitions#322
Conversation
|
Thanks for this @TheIanLi - the proxy-forwarding fix is genuinely useful and it's properly raised via no-mistakes. One thing before it can merge: it's red on Windows CI, and the failure is in the new proxy-env code. Because Windows env-var names are case-insensitive, |
6cc4177 to
66768e3
Compare
|
Thanks @kunchenguid — addressed and re-run through no-mistakes. Your ask (Windows dedup): A few things slightly beyond the original ask — all surfaced by the no-mistakes review, pure correctness/security, no behaviour change:
Deliberately left out: I did not change the env-less restart behaviour — a Green locally ( |
cadce24 to
1a10d94
Compare
|
Quick follow-up: running the suite on real Windows surfaced that three service-file mode tests — |
ae1a2cd to
fff02f6
Compare
|
Follow-up + a correction to my earlier note, @kunchenguid. In an earlier comment I said an env-less Fixed in the latest commits: the drift render and the install write path now inherit the proxy already baked into the on-disk unit/plist when the current environment has none — mirroring the existing executable inheritance in Regression tests cover the systemd and launchd drift no-op (env-less restart no longer strips/churns), the write-path inheritance (e.g. a binary upgrade keeps the proxy + 0600), and the parser round-trips. I also corrected a couple of now-stale doc comments ( So the corrected behavior: an env-less restart now preserves the forwarded proxy instead of dropping it. I deliberately left the systemd |
|
Quick heads-up on the red ❌ in the checks panel, @kunchenguid — it's 4 cancelled checks, not failures: The substantive CI is green on the current head ( A re-run should go green on content: the PR only touches |
A daemon started by systemd/launchd inherits only HOME and a curated PATH, so when the user is behind an HTTP(S) proxy the daemon - and the agents it spawns (e.g. `claude --print`) - cannot reach the network and fail with "403 Request not allowed". Forward HTTP(S)_PROXY / NO_PROXY / ALL_PROXY (both upper- and lower-case spellings) present at install time into the generated systemd unit and launchd plist. When no proxy variables are set the generated definition is byte-for-byte unchanged, so users without a proxy are unaffected. Windows schtasks already inherits the user's logon environment and is unaffected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…arded Addresses a review note: forwarded proxy values are baked into the generated systemd unit / launchd plist, and a proxy URL can embed credentials (http://user:pass@host). When any proxy variable is forwarded, write the service definition owner-only (0600) instead of 0644 so those values are not world-readable. When no proxy is set the mode stays 0644, keeping non-proxy installs byte-for-byte and mode-for-mode unchanged. An explicit chmod enforces the mode on re-install when the file already exists. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…oolproof Optimization-only follow-up to the proxy-forwarding change; no behavior change. - writeServiceFile now takes a render callback and resolves the proxy environment itself, exactly once per install, feeding that same value to both the renderer and the 0600/0644 mode decision. This drops the duplicate serviceProxyEnv() lookup that happened on every install and makes it structurally impossible to bake proxy credentials into the content yet write it under the world-readable 0644 mode. - renderLaunchAgent/renderSystemdUnit keep their proxy-resolving signatures for standalone callers (drift detection in selfexec.go, tests) and delegate to new *WithProxyEnv variants used on the install path. - Assemble the launchd EnvironmentVariables <dict> in a single strings.Builder instead of splicing a proxy fragment via "%s </dict>"; rendered output is byte-for-byte unchanged. - Add table tests covering every proxyEnvKeys entry (ALL_PROXY, the lower-case spellings, NO_PROXY) for both the launchd and systemd renderers. - Document why the Windows schtasks path forwards no proxy env: an ONLOGON task inherits the user's interactive session environment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
serviceProxyEnv() iterated both the upper- and lower-case spellings of each proxy variable. On Windows, where environment-variable names are case-insensitive, HTTP_PROXY and http_proxy resolve to the same variable, so both spellings were baked into the rendered launchd/systemd service definition as duplicate entries, failing TestRenderLaunchAgentForwardsEveryProxyEnvKey on Windows CI. De-duplicate the spellings case-insensitively only on Windows; on case-sensitive platforms (macOS, Linux) keep forwarding every spelling verbatim so a value exported only as lower-case http_proxy still reaches lower-case-only consumers such as curl (which ignores HTTP_PROXY for plain-HTTP requests to avoid the CGI "httpoxy" issue). Drive the renderer drift tests off an injected proxyEnv so they no longer depend on the host's env-var case sensitivity, and add focused serviceProxyEnv tests for the Windows dedup, the both-spellings case, and the lower-only regression. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…se proxy-credential TOCTOU
… to survive specifier expansion
Go reports 0666 for regular files on Windows, so the 0644/0600 assertions in the service-file mode tests cannot hold there; the proxy-bearing 0600 service file is only generated on macOS/Linux (managedServiceInstallPath returns "" on Windows). Found by cross-compiling the daemon tests and running them on Windows via WSL interop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Drift detection rendered the managed service definition from the live environment, so a `daemon start` from a shell without the proxy variables exported rendered a no-proxy target, falsely detected drift, reinstalled, and stripped the proxy baked in at install time - re-breaking the daemon with "403 Request not allowed". The drift render now inherits proxy entries already present in the on-disk unit/plist when the current environment has none, mirroring the existing executable inheritance. The install write path does the same so a legitimate reinstall (e.g. a binary upgrade) preserves the forwarded proxy and its owner-only 0600 mode instead of dropping it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-less daemon refreshes
…ance renderSystemdUnit/renderLaunchAgent are now test-only convenience wrappers: drift detection switched to the *WithProxyEnv variants, so the comments claiming they are the drift-detection entry point were stale. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
fff02f6 to
ca611d8
Compare
|
Thanks @TheIanLi - merged! Really appreciate the contribution. |
How this was found
While running
no-mistakesfrom a WSL shell behind a local HTTP proxy, every agent step (review, test) failed withFailed to authenticate. API Error: 403 Request not allowed— even thoughclaude --printworked fine in an interactive shell. Reproducing the daemon-spawned agent's minimal environment (env -i HOME=… PATH=… claude --print) reproduced the 403; adding the proxy variables back made it succeed. The managed daemon's systemd unit only exportsHOMEandPATH, so the proxy configuration the user relies on never reaches the daemon — or the agents it spawns. This PR forwards it.What Changed
HTTPS_PROXY/HTTP_PROXY/NO_PROXY(and lowercase variants) from the install-time environment and bake them into the generated systemd unit and launchd plist for the managed daemon service, so the background daemon inherits the user's proxy configuration.internal/daemon/service.gothat collects only set, non-empty proxy variables, leaving service definitions byte-for-byte unchanged when no proxy is configured; wired it into both the systemd and launchd renderers.Risk Assessment
✅ Low: Small, additive, well-tested change that only augments generated service definitions with proxy env vars; empty-case formatting and the Windows path are correctly handled, with the only note being an intentional install-time capture tradeoff.
Testing
Ran the new and full daemon package tests (all green) and produced product-level evidence by rendering the actual systemd unit and launchd plist the installer writes: with a proxy configured, both definitions gain the proxy Environment/EnvironmentVariables entries (e.g.Environment="HTTPS_PROXY=http://127.0.0.1:7897"and the plist<key>HTTPS_PROXY</key>entries); with no proxy set the output is byte-for-byte identical to the base commit for both backends. This is a non-UI backend change, so the reviewer-visible artifacts are the generated service-definition files rather than screenshots. Throwaway evidence test and temporary base worktree were removed; working tree is clean.Evidence: systemd unit + launchd plist diffs (no-proxy vs with-proxy)
Evidence: Generated systemd unit with proxy forwarded
Evidence: Generated launchd plist with proxy forwarded
Evidence: Baseline systemd unit (no proxy)
Evidence: Baseline launchd plist (no proxy)
Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
internal/daemon/service.go:56- Proxy values are captured from the install-time environment and baked statically into the systemd/launchd service files (user-scoped, 0644). This is intentional and documented, but means (a) a proxy URL with embedded credentials (http://user:pass@host) is written in plaintext to the service definition, and (b) later proxy changes require reinstalling the service to take effect. Both are acceptable tradeoffs given the comments, just noting the new exposure surface.✅ **Test** - passed
✅ No issues found.
go test -race ./internal/daemon/ -run 'TestRenderLaunchAgentForwardsProxyEnv|TestServiceProxyEnvSkipsUnsetAndEmpty|TestRenderSystemdUnitForwardsProxyEnv' -v— all passgo test ./internal/daemon/(full package, no regressions)Rendered real systemd unit + launchd plist withHTTPS_PROXY/HTTP_PROXY/NO_PROXYset and captured the generated definitions as evidenceDiffed no-proxy vs with-proxy output for both backends — proxy lines appear only when setConfirmed the 'byte-for-byte unchanged when no proxy' claim by rendering the no-proxy output and diffing against the base commit 0e07573 for both systemd and launchd (IDENTICAL)✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.