feat(shellwrap): hydrate login-shell env once at startup (macOS GUI/launchd) — MCP-2751#701
Conversation
…aunchd) 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
Deploying mcpproxy-docs with
|
| Latest commit: |
5f0f861
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c42d0d9b.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-mcp-2751-login-shell-env.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27684497751 --repo smart-mcp-proxy/mcpproxy-go
|
🤖 Codex review — REQUEST_CHANGES (1×P2)[P2] Respect explicitly-empty operator env vars —
|
…ration 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>
Addressed — Codex P2 (set-if-empty vs intentionally-empty)Valid catch, fixed in e42f22f. The curated-key guard used Fix: switched the guard to Test: added
|
🤖 Codex re-review (round 2) — REQUEST_CHANGES (1×P2)[P2] Treat proxy spellings as aliases before hydrating —
|
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>
Addressed — Codex round-2 P2 (proxy case-aliasing)Correct again, fixed in 3983b7e. Since HTTP clients honor either spelling, the two case variants of each proxy var are one logical var. The per-key Fix: split the allow-list into single-spelling keys (DOCKER_, tool-home) and an alias-aware proxy trio. For each logical proxy var I now check both Test: added
|
🤖 Codex re-review (round 3) — REQUEST_CHANGES (2×P2)[P2] Don't gate hydration on PATH alone —
|
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>
|
Round-3 fix pushed ( Two changes as directed: 1. Proxy vars dropped entirely
2. Launch gate fixed for pre-seeded PATH
Scope: PATH + DOCKER_ + tool-homes only.* Local: |
🤖 Codex review — ACCEPT ✅
No P0–P2 findings after the scope reduction (proxy vars dropped → no credential-leak surface; launch gate no longer relies on PATH alone). Login-shell hydration of PATH + Reviewed via |
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
…704) * feat(secureenv): opt-in proxy env forwarding to upstreams (MCP-2769) Split out of MCP-2751 login-shell hydration: forwarding HTTP(S)/proxy env vars to every stdio upstream by default leaks credential-bearing proxy URLs (http://user:pass@proxy). Keep proxy vars OUT of the default allow-list and gate forwarding behind an explicit opt-in. - config: new global `forward_proxy_env` (default false). - secureenv: when opted in, BuildSecureEnvironment forwards the ambient proxy vars (HTTP_PROXY/HTTPS_PROXY/NO_PROXY/ALL_PROXY/FTP_PROXY, both upper- and lower-case spellings) to spawned stdio upstreams. userinfo (credentials) is redacted from each value; the proxy host/port is preserved. Case-alias aware: an explicitly configured spelling suppresses forwarding of the ambient alias. Forwarding is independent of inherit_system_safe. - upstream/core: thread the flag into the per-server env config for stdio spawns. - docs/configuration.md: document the opt-in + the never-by-default rationale. Tests: redaction unit table, default-off leak guard, opt-in redacted forward, explicit-wins-over-alias, and an end-to-end wiring test through managed.NewClient. Related #701 * fix(secureenv): redact credentials from schemeless proxy values (MCP-2769) Codex PR #704 review (P2, security): redactProxyCredentials missed schemeless proxy values like "user:pass@proxy:8080" (no scheme). url.Parse reads "user" as the scheme and leaves Userinfo nil, so the original credential-bearing value was forwarded to upstreams despite the redaction guarantee. Fix: when the value has no "://" but contains "@", re-parse with a dummy "http://" scheme so url.Parse recognizes the userinfo, strip it, then remove the dummy scheme. An "@" inside a path (host/path@x) keeps Userinfo nil and is left intact. Adds table cases (schemeless user:pass / user-only / no-userinfo / at-in-path) and an end-to-end forwarding test asserting no creds reach upstream. Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
…CP-2753) (#703) * fix(docker): direct-exec resolved docker binary for isolated spawn (MCP-2753) PR #697 resolved the docker CLI to an absolute path but still routed the isolated spawn through `$SHELL -l -c "<docker> run …"`, where the login shell re-derives $PATH from rc files and can drop the Docker Desktop bundle dir. The absolute path was only a token in the -c string, so docker-isolated servers still failed with `command not found: docker` when the CLI was not on the LaunchAgent/tray spawn PATH — even though `upstream_servers list` reported a good docker_path. On successful resolution to a VERIFIED absolute executable, setupDockerIsolation now returns the resolved binary as the exec command with raw docker argv (no shell wrap), mirroring the management path's newDockerCmd. It returns a shellWrapped flag so callers pick the matching cidfile helper: the new args-based insertCidfileIntoDockerArgs inserts `--cidfile` after the `run` token (platform-agnostic, sidestepping the -c vs /c quirk), while the login-shell fallback keeps the string-based insertCidfileIntoShellDockerCommand. The direct-exec branch is gated by isDirectExecutable (filepath.IsAbs + os.Stat + exec-bit). ResolveDockerPath's last resort runs `command -v docker` in the login shell, which can emit a shell function/alias/bare name; such a value is rejected and shell-wrapped instead of failing a direct exec. Login-shell wrap of bare `docker` is also kept on the resolution-failure fallback. The DOCKER_* daemon-env half of MCP-2753 is already satisfied by the startup login-shell hydration shipped in MCP-2751 (#701): DOCKER_* are hydrated into os.Environ() and carried through BuildSecureEnvironment's default allow-list, so the direct-exec'd docker process still reaches Colima/rootless/remote daemons without a per-spawn shell capture. Related #699 Related #696 * fix(docker): gate direct-exec on guaranteed daemon env (non-Darwin rootless regression) Codex round-4 (PR #703 REQUEST_CHANGES, P2): the direct-exec branch regressed non-Darwin rootless/remote Docker. Dropping the `$SHELL -l` wrap also drops rc-file env inheritance, and HydrateFromLoginShell (MCP-2751) is Darwin-only while BuildSecureEnvironment only forwards DOCKER_* already in os.Environ(). So on Linux a daemon whose DOCKER_HOST/DOCKER_CONTEXT lives only in .profile would be lost by direct-exec — the same DOCKER_* loss that made #699 keep the shell wrap. Add a second precondition (dockerDaemonEnvGuaranteed) alongside the verified- absolute-executable check: direct-exec only when the daemon env is guaranteed without the login shell — macOS (startup hydration captured DOCKER_* into os.Environ()) OR DOCKER_HOST/DOCKER_CONTEXT already present in the process env on any platform. Otherwise keep the `$SHELL -l` wrap so `docker run` inherits the rc-file daemon config; the wrap now prefers the resolved absolute path (still sidestepping the login shell's PATH re-derivation) and drops to bare "docker" only when resolution failed. dockerDaemonEnvGOOS is a package var so the Darwin branch is testable on a non-Darwin CI host. New tests: non-Darwin + DOCKER_HOST-only-in-rc stays shell-wrapped with the absolute path; non-Darwin + DOCKER_HOST in env direct- execs; and a table test for the gate. Related #703 Related #699 Co-Authored-By: Paperclip <noreply@paperclip.ing> --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
2 participants
Summary
Fixes the root-cause class behind the recent docker/PATH point-fixes (#697 resolve-absolute docker path, #699 DOCKER_* capture): when mcpproxy is launched from a macOS GUI/launchd context (Launchpad/Dock, the
SMAppServicelogin 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,docker/uvx/npxon PATH, and exported vars likeDOCKER_HOST/DOCKER_CONTEXT. Launched from a terminal it's healthy.This adds a one-time, allow-listed login-shell environment hydration at startup so every spawn path inherits a correct environment without per-spawn
$SHELL -lcwrapping — the durable consolidation that supersedes per-var capture.What changed
internal/shellwrap/hydrate.go(new):captureLoginShellEnv()sources$SHELL -l -c 'env -0'once (NUL-delimited so multiline/=-containing values survive; marker-bracketed to strip rc-file banner noise; 5s timeout;sync.Once) and caches the parsed env.LoginShellPATHnow reads PATH from this shared capture — one shell fork, not two.HydrateFromLoginShell(logger): gates on macOS and launchd-minimal PATH (no-op on terminal launches and non-macOS), mergesPATHlogin-first, and applies a curated allow-list set-if-empty viaos.Setenv:DOCKER_HOST,DOCKER_CONTEXT,DOCKER_CONFIG,DOCKER_CERT_PATH,DOCKER_TLS_VERIFYHTTP_PROXY/HTTPS_PROXY/NO_PROXY(+ lowercase)NVM_DIR,ASDF_DIR,PYENV_ROOT,VOLTA_HOME,HOMEBREW_PREFIX,COLIMA_HOMEinternal/secureenv/manager.go: extend the default allow-list with the curated keys so the hydrated vars survive the filter into upstream stdio/docker spawns. ScannerMinimalEnvstays narrow (credential-free).cmd/mcpproxy/main.go: callHydrateFromLoginShellafter logger setup, before any manager readsos.Environ()— oneos.Setenvhydration propagates to all spawn paths.MCPX_STDIO_SPAWN_ENOENT.mdupdated to describe the automatic startup hydration.Security / privacy
envimport — secrets (AWS_*,GITHUB_TOKEN,ANTHROPIC_API_KEY, …) are never pulled into the daemon or subprocesses (unit-tested).HOME/USER/SHELLnever touched.Relationship to #697 / #699
DOCKER_*into the process env; per-spawn capture is redundant. (Absolute-path resolve + negative-cache fix remain.)Tests / verification
internal/shellwrap: gate no-op (non-darwin + comprehensive PATH), curated-set hydration, set-if-empty never-clobber, never-touch HOME/USER/SHELL, secret-exclusion, privacy (no values logged), capture-failure no-op. Gate seam (hydrationGOOS) exercises the logic on Linux CI.internal/secureenv: curated DOCKER_*/proxy vars pass the allow-list while secrets stay filtered; existingTestLaunchdMinimalPath_*gate guarantees preserved.go test ./internal/shellwrap/... ./internal/secureenv/... ./internal/upstream/core/... -race— green.golangci-lint v2(CI config) — 0 issues. Both editions build.PATH→ hydration fires and enriches PATH; launched with a comprehensivePATH→ no-op (gate confirmed).Related #699