Commit 90f6e7f
feat(shellwrap): hydrate login-shell env once at startup (macOS GUI/launchd) — MCP-2751 (#701)
* feat(shellwrap): hydrate login-shell env once at startup (macOS GUI/launchd)
When mcpproxy is launched from a macOS GUI/launchd context (Launchpad, the
SMAppService login item, or the tray spawning the core), the Go core inherits a
launchd-minimal environment: it never sources the user's login shell, so it
lacks Homebrew/Docker PATH entries and exported vars like DOCKER_HOST. This is
the root-cause class behind the recent docker/PATH point-fixes (#697, #699).
Add a one-time, allow-listed login-shell environment hydration at startup so
every spawn path (docker lifecycle, stdio servers, uvx/npx, ResolveDockerPath,
secureenv.BuildSecureEnvironment) inherits a correct environment with no
call-site changes:
- internal/shellwrap/hydrate.go: captureLoginShellEnv() sources `$SHELL -l -c
'env -0'` once (NUL-delimited, marker-bracketed, 5s timeout, sync.Once) and
caches the parsed env. LoginShellPATH now reads PATH from this shared capture
— one shell fork, not two.
- HydrateFromLoginShell() gates on macOS + launchd-minimal PATH (no-op on
terminal launches and non-macOS), merges PATH login-first, and applies a
curated allow-list (DOCKER_*, HTTP(S)/NO_PROXY, NVM_DIR/ASDF_DIR/PYENV_ROOT/
VOLTA_HOME/HOMEBREW_PREFIX/COLIMA_HOME) set-if-empty via os.Setenv. Secrets
are never imported; HOME/USER/SHELL are never touched; values are never
logged (key names + lengths only).
- secureenv: extend the default allow-list with the curated DOCKER_*/proxy/
tool-home keys so the hydrated vars survive the filter into upstream spawns.
Scanner MinimalEnv stays narrow (credential-free).
- cmd/mcpproxy: call HydrateFromLoginShell after logger setup, before any
manager reads os.Environ().
Supersedes the per-spawn DOCKER_* env capture; the absolute-path docker probe
(#697) and negative-cache fix (#699) remain as complementary fallbacks.
Related #699
* fix(shellwrap): preserve intentionally-empty operator vars during hydration
Codex review (PR #701, P2): the curated-key set-if-empty guard used
`os.Getenv(key) != ""`, which treats an explicitly set-empty value the same
as unset. An operator who sets `HTTPS_PROXY=` or `DOCKER_HOST=` to DISABLE an
inherited value would have it overwritten from the login shell — violating the
never-clobber-operator-values contract.
Use os.LookupEnv to distinguish unset from intentionally-empty: any present
key (even empty) is operator intent and is preserved. launchd never sets these
to empty, so present-but-empty is always deliberate. Adds
TestHydrate_PreservesIntentionallyEmptyVar.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
* fix(shellwrap): alias-aware set-if-unset for proxy var spellings
Codex round-2 (PR #701, P2): proxy vars (HTTP_PROXY/HTTPS_PROXY/NO_PROXY) are
case-insensitive to HTTP clients, so the per-key LookupEnv guard could still
clobber operator intent — e.g. operator sets `https_proxy=` (lowercase, to
disable) but the login shell exports `HTTPS_PROXY=...`; the exact-key check on
the uppercase spelling missed the lowercase presence and imported it anyway.
Split the curated allow-list into single-spelling keys (DOCKER_*, tool-home)
and an alias-aware proxy trio: for each logical proxy var, if EITHER spelling
is already present in the process env (including intentionally empty), skip
hydrating BOTH spellings. Adds TestHydrate_ProxyCaseAliasingPreservesOperatorIntent
(daemon https_proxy='' + login HTTPS_PROXY=http://x → neither imported).
Co-Authored-By: Paperclip <noreply@paperclip.ing>
* fix(shellwrap): drop proxy vars, fix launch gate for pre-seeded PATH
Codex round-3 scope reduction:
- Remove HTTP_PROXY/HTTPS_PROXY/NO_PROXY from hydration and from
secureenv DefaultEnvConfig allow-list. Proxy forwarding to every
stdio upstream is out of scope; filed as a separate opt-in follow-up.
- Fix the launch gate: hydration now triggers when PATH looks launchd-
minimal OR any DOCKER_* curated var is absent. A GUI launcher can
pre-seed PATH via /etc/paths yet still not export DOCKER_HOST from
rc files, so PATH-comprehensiveness alone is insufficient.
- PATH-merge stays gated on pathIsMinimal only — a comprehensive PATH
is not widened even when the DOCKER_* gate fires.
Tests:
- Rename GateNoOpOnComprehensivePath → GateNoOpOnComprehensivePathAndAllDockerPresent
(now requires both conditions, so all 5 DOCKER_* vars are pre-set)
- Add ComprehensivePathStillHydratesDockerIfMissing — verifies the new
secondary gate: comprehensive PATH, missing DOCKER_HOST → hydrates
curated vars but leaves PATH unmodified
- Drop PreservesIntentionallyEmptyVar and ProxyCaseAliasingPreservesOperatorIntent
(proxy vars are no longer hydrated)
- Update MinimalPathHydratesCuratedSet to assert proxy vars are NOT imported
Co-Authored-By: Paperclip <noreply@paperclip.ing>
---------
Co-authored-by: Paperclip <noreply@paperclip.ing>1 parent 6b1b346 commit 90f6e7f
8 files changed
Lines changed: 722 additions & 81 deletions
File tree
- cmd/mcpproxy
- docs/errors
- internal
- secureenv
- shellwrap
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
47 | 47 | | |
48 | 48 | | |
49 | 49 | | |
| 50 | + | |
50 | 51 | | |
51 | 52 | | |
52 | 53 | | |
| |||
499 | 500 | | |
500 | 501 | | |
501 | 502 | | |
| 503 | + | |
| 504 | + | |
| 505 | + | |
| 506 | + | |
| 507 | + | |
| 508 | + | |
| 509 | + | |
| 510 | + | |
| 511 | + | |
| 512 | + | |
| 513 | + | |
502 | 514 | | |
503 | 515 | | |
504 | 516 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
58 | | - | |
59 | | - | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
60 | 69 | | |
61 | 70 | | |
62 | 71 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
215 | 215 | | |
216 | 216 | | |
217 | 217 | | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
218 | 266 | | |
219 | 267 | | |
220 | 268 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
86 | 86 | | |
87 | 87 | | |
88 | 88 | | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
89 | 99 | | |
90 | 100 | | |
91 | 101 | | |
| |||
0 commit comments