diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index ca6a7ab05..bfffef41d 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -47,6 +47,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/logs" "github.com/smart-mcp-proxy/mcpproxy-go/internal/registries" "github.com/smart-mcp-proxy/mcpproxy-go/internal/server" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" "github.com/smart-mcp-proxy/mcpproxy-go/internal/telemetry" _ "github.com/smart-mcp-proxy/mcpproxy-go/oas" // Import generated swagger docs @@ -499,6 +500,17 @@ func runServer(cmd *cobra.Command, _ []string) error { zap.String("log_level", cmdLogLevel), zap.Bool("log_to_file", cmdLogToFile)) + // MCP-2751: when launched from a macOS GUI/launchd context (Launchpad, the + // SMAppService login item, or the tray spawning the core), the process + // inherits a launchd-minimal environment and never sources the user's + // login shell — so it lacks Homebrew/Docker PATH entries and exported vars + // like DOCKER_HOST. Hydrate a curated allow-list (PATH + DOCKER_*/proxy/ + // tool-home) once, before any manager reads os.Environ(), so every spawn + // path (docker, stdio servers, uvx/npx, ResolveDockerPath, + // secureenv.BuildSecureEnvironment) inherits a correct environment with no + // call-site changes. No-op on terminal launches and non-macOS. + shellwrap.HydrateFromLoginShell(logger) + // Pass edition and version to internal packages httpapi.SetEdition(Edition) server.SetMCPServerVersion(version) diff --git a/docs/errors/MCPX_STDIO_SPAWN_ENOENT.md b/docs/errors/MCPX_STDIO_SPAWN_ENOENT.md index 3098d853f..6455800fa 100644 --- a/docs/errors/MCPX_STDIO_SPAWN_ENOENT.md +++ b/docs/errors/MCPX_STDIO_SPAWN_ENOENT.md @@ -55,8 +55,17 @@ sudo apt install nodejs npm # Debian/Ubuntu ### 2. Make the GUI inherit the shell PATH (macOS) -The macOS tray launches mcpproxy with the user's login `PATH`, which is shorter -than your interactive shell `PATH`. Either: +When launched from a macOS GUI/launchd context (Launchpad, the login item, or +the tray spawning the core), mcpproxy inherits a launchd-minimal environment +rather than your interactive shell `PATH`. Since the daemon now **hydrates the +login-shell environment once at startup** — sourcing your login shell to merge +in `PATH` (and curated `DOCKER_*`/proxy/tool-home vars) — `uvx`/`npx` installed +in `/opt/homebrew/bin`, `~/.local/bin`, or a version-manager shim directory are +normally found automatically. (Hydration is a no-op when mcpproxy is started +from a terminal whose `PATH` is already comprehensive.) + +If the tool still isn't found — e.g. it lives in a directory your login shell +doesn't export, or your rc files don't run non-interactively — either: - Move the binary to a system directory: `sudo ln -s "$(which uvx)" /usr/local/bin/uvx`, or - Set an absolute path in the upstream config: `"command": "/Users/you/.local/bin/uvx"`. diff --git a/internal/secureenv/launchd_path_test.go b/internal/secureenv/launchd_path_test.go index 7f3165b73..8d1032e17 100644 --- a/internal/secureenv/launchd_path_test.go +++ b/internal/secureenv/launchd_path_test.go @@ -215,6 +215,54 @@ func TestLaunchdMinimalPath_AlreadyComprehensive(t *testing.T) { "comprehensive PATH must be returned unchanged — terminal-launched processes should not be polluted by login-shell capture") } +// TestBuildSecureEnvironment_AllowsHydratedDockerVars verifies the allow-list +// extension for MCP-2751: once shellwrap.HydrateFromLoginShell has placed +// curated DOCKER_*/tool-home vars into the process env, the default allow-list +// must pass them through to spawned upstreams — while secrets remain filtered. +// Proxy vars are intentionally excluded from the allow-list (separate follow-up). +func TestBuildSecureEnvironment_AllowsHydratedDockerVars(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("uses unix PATH semantics") + } + + originalEnv := os.Environ() + defer func() { + os.Clearenv() + for _, env := range originalEnv { + parts := strings.SplitN(env, "=", 2) + if len(parts) == 2 { + os.Setenv(parts[0], parts[1]) + } + } + }() + + t.Cleanup(withFakeLoginShellPath("")) + + os.Clearenv() + os.Setenv("PATH", "/usr/bin:/bin:/usr/sbin:/sbin") + os.Setenv("HOME", "/tmp/test-home") + os.Setenv("DOCKER_HOST", "unix:///Users/me/.docker/run/docker.sock") + os.Setenv("DOCKER_CONTEXT", "desktop-linux") + os.Setenv("HOMEBREW_PREFIX", "/opt/homebrew") + // Genuine secrets that must still be filtered out by the allow-list. + os.Setenv("AWS_ACCESS_KEY_ID", "AKIA_test_dummy_value_00000000") + os.Setenv("GITHUB_TOKEN", "ghp_dummy_test_token_1234567890abcdef") + // Proxy vars are excluded from the allow-list. + os.Setenv("HTTPS_PROXY", "http://proxy.corp:8080") + + manager := NewManager(DefaultEnvConfig()) + joined := strings.Join(manager.BuildSecureEnvironment(), "\n") + + assert.Contains(t, joined, "DOCKER_HOST=unix:///Users/me/.docker/run/docker.sock", + "curated DOCKER_HOST must survive the allow-list") + assert.Contains(t, joined, "DOCKER_CONTEXT=desktop-linux") + assert.Contains(t, joined, "HOMEBREW_PREFIX=/opt/homebrew") + + assert.NotContains(t, joined, "HTTPS_PROXY", "proxy vars must be filtered (out of scope)") + assert.NotContains(t, joined, "AWS_ACCESS_KEY_ID", "secrets must still be filtered out") + assert.NotContains(t, joined, "GITHUB_TOKEN", "secrets must still be filtered out") +} + // --- test helpers -------------------------------------------------------- // withFakeLoginShellPath swaps loginShellPATHFn for a stub returning `path`. diff --git a/internal/secureenv/manager.go b/internal/secureenv/manager.go index 341aa5fa5..a06d0c067 100644 --- a/internal/secureenv/manager.go +++ b/internal/secureenv/manager.go @@ -86,6 +86,16 @@ func DefaultEnvConfig() *EnvConfig { } allowedVars = append(allowedVars, localeVars...) + // Add container / tool-home passthrough variables (MCP-2751). These are NOT + // secrets; they mirror the curated set hydrated by shellwrap.HydrateFromLoginShell + // so the now-present vars survive this allow-list filter and reach upstream + // stdio/docker spawns. Proxy vars (HTTP_PROXY etc.) are intentionally excluded + // — proxy forwarding is a separate opt-in concern. + allowedVars = append(allowedVars, + "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG", "DOCKER_CERT_PATH", "DOCKER_TLS_VERIFY", + "NVM_DIR", "ASDF_DIR", "PYENV_ROOT", "VOLTA_HOME", "HOMEBREW_PREFIX", "COLIMA_HOME", + ) + return &EnvConfig{ InheritSystemSafe: true, AllowedSystemVars: allowedVars, diff --git a/internal/shellwrap/hydrate.go b/internal/shellwrap/hydrate.go new file mode 100644 index 000000000..aeba4bb1f --- /dev/null +++ b/internal/shellwrap/hydrate.go @@ -0,0 +1,306 @@ +package shellwrap + +import ( + "context" + "os" + "os/exec" + "runtime" + "sort" + "strings" + "sync" + "time" + + "go.uber.org/zap" +) + +// --- Unified login-shell environment capture ---------------------------- +// +// captureLoginShellEnv sources the user's login shell exactly once per process +// and returns the full environment it exports. It is the single shell fork +// shared by LoginShellPATH (PATH-only view) and HydrateFromLoginShell (curated +// env merge), so startup never forks the login shell more than once. +// +// Why this exists: when mcpproxy is launched from a macOS GUI / launchd context +// (Launchpad, Dock, the SMAppService login item, or the tray spawning the +// core), the Go process 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. Launched from a terminal it is healthy. See +// MCP-2751. + +// osDarwin is the runtime.GOOS value for macOS (osWindows is defined in +// shellwrap.go). +const osDarwin = "darwin" + +const ( + // loginShellEnvMarkerBegin / loginShellEnvMarkerEnd bracket the captured + // `env -0` output inside the shell's stdout. The markers let us pluck the + // real environment out of arbitrary banner / motd / oh-my-zsh / direnv + // output that login-rc files routinely emit on stdout (see issue #439). + // The strings are deliberately unlikely to appear in any rc-file banner. + loginShellEnvMarkerBegin = "__MCPPROXY_LOGIN_ENV_BEGIN__" + loginShellEnvMarkerEnd = "__MCPPROXY_LOGIN_ENV_END__" +) + +var ( + loginShellEnvOnce sync.Once + loginShellEnvVal map[string]string +) + +// captureLoginShellEnv runs ` -l -c 'env -0'` once, NUL-delimited so +// multiline / `=`-containing values survive, bracketed by unique markers, with +// a 5s timeout. The parsed environment is cached for the rest of the process +// lifetime. Returns nil on Windows or when the capture fails (the caller then +// falls back to the ambient process environment — no regression). +func captureLoginShellEnv(logger *zap.Logger) map[string]string { + loginShellEnvOnce.Do(func() { + loginShellEnvVal = doCaptureLoginShellEnv(logger) + }) + return loginShellEnvVal +} + +func doCaptureLoginShellEnv(logger *zap.Logger) map[string]string { + if runtime.GOOS == osWindows { + // Windows has no login shell; PATH lives in the registry and is handled + // separately by secureenv. Consistent with LoginShellPATH returning "". + return nil + } + + shell := resolveLoginShell() + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Bracket `env -0` with unique markers so rc-file banner output (welcome + // messages, oh-my-zsh updates, direnv hooks, motd, etc.) does NOT leak into + // the value we parse. We deliberately build the argv ourselves rather than + // going through WrapWithUserShell because shellescape would quote the + // command and suppress expansion. + script := `printf '` + loginShellEnvMarkerBegin + `'; env -0; printf '` + loginShellEnvMarkerEnd + `'` + cmd := exec.CommandContext(ctx, shell, "-l", "-c", script) + out, err := cmd.Output() + if err != nil { + if logger != nil { + logger.Debug("shellwrap: login-shell env capture failed", + zap.String("shell", shell), + zap.Error(err)) + } + return nil + } + + env := parseLoginShellEnv(string(out)) + if logger != nil { + // Never log values — key count only. + logger.Debug("shellwrap: captured login-shell env", + zap.String("shell", shell), + zap.Int("var_count", len(env))) + } + return env +} + +// parseLoginShellEnv extracts the marker-bracketed region of the shell stdout +// and parses the NUL-delimited `env -0` dump into a map. It returns nil if the +// markers are absent or the body is not NUL-delimited (e.g. an `env` without +// `-0` support on a very old OS), so the caller degrades to the ambient env. +func parseLoginShellEnv(stdout string) map[string]string { + i := strings.Index(stdout, loginShellEnvMarkerBegin) + if i < 0 { + return nil + } + body := stdout[i+len(loginShellEnvMarkerBegin):] + if j := strings.Index(body, loginShellEnvMarkerEnd); j >= 0 { + body = body[:j] + } + // env -0 emits NUL-separated records; absence of any NUL means `-0` was not + // honored — refuse to misparse a newline-joined blob. + if !strings.Contains(body, "\x00") { + return nil + } + + result := make(map[string]string, 48) + for _, pair := range strings.Split(body, "\x00") { + if pair == "" { + continue + } + eq := strings.IndexByte(pair, '=') + if eq <= 0 { + continue // no key, or empty key + } + key := pair[:eq] + if !isValidEnvKey(key) { + continue + } + result[key] = pair[eq+1:] + } + return result +} + +// isValidEnvKey guards against treating banner noise as an env record. +func isValidEnvKey(key string) bool { + for i := 0; i < len(key); i++ { + c := key[i] + switch { + case c >= 'A' && c <= 'Z': + case c >= 'a' && c <= 'z': + case c >= '0' && c <= '9': + case c == '_': + default: + return false + } + } + return key != "" +} + +// resetLoginShellEnvCacheForTest clears the unified capture cache. Only +// referenced from tests in this package. +func resetLoginShellEnvCacheForTest() { + loginShellEnvOnce = sync.Once{} + loginShellEnvVal = nil +} + +// --- Login-shell environment hydration ----------------------------------- + +// hydrationGOOS is a test seam so the macOS-only gate can be exercised on the +// Linux CI runners. Defaults to the real GOOS. +var hydrationGOOS = runtime.GOOS + +// curatedSingleKeys is the allow-list of environment variables hydrated from +// the login shell. It deliberately EXCLUDES secrets (AWS_*, GITHUB_TOKEN, +// ANTHROPIC_API_KEY, …) and proxy vars (HTTP_PROXY/HTTPS_PROXY — proxy +// forwarding is out of scope here; filed as a separate opt-in follow-up). +// HOME / USER / SHELL are never touched. +var curatedSingleKeys = []string{ + // Docker / container engine selection (supersedes the per-spawn capture in #699). + "DOCKER_HOST", "DOCKER_CONTEXT", "DOCKER_CONFIG", "DOCKER_CERT_PATH", "DOCKER_TLS_VERIFY", + // Tool-home roots that node/python version managers rely on. + "NVM_DIR", "ASDF_DIR", "PYENV_ROOT", "VOLTA_HOME", "HOMEBREW_PREFIX", "COLIMA_HOME", +} + +// HydrateFromLoginShell performs a one-time, allow-listed merge of the user's +// login-shell environment into the current process environment via os.Setenv, +// so that every downstream spawn path (docker lifecycle, stdio servers, +// uvx/npx, secureenv.BuildSecureEnvironment, ResolveDockerPath) inherits a +// correct PATH and curated vars with no call-site changes. +// +// Hydration is triggered on macOS when: +// - the ambient PATH looks launchd-minimal (lacks /usr/local/bin or +// /opt/homebrew/bin), OR +// - any DOCKER_* curated var is absent (a GUI launcher may pre-seed PATH +// via /etc/paths yet still not export DOCKER_HOST from rc files). +// +// PATH is merged login-first (enriching, never shrinking) only when the PATH +// is launchd-minimal. Curated keys are applied set-if-unset whenever hydration +// triggers. The returned snapshot maps each applied key to its value for +// diagnostics; this function never logs values (key names + lengths only). +func HydrateFromLoginShell(logger *zap.Logger) (applied bool, snapshot map[string]string) { + snapshot = make(map[string]string) + + if hydrationGOOS != osDarwin { + // Linux: systemd Environment=/EnvironmentFile=. Windows: registry PATH. + // Neither uses login-shell sourcing. + return false, snapshot + } + + pathIsMinimal := looksLikeLaunchdMinimalPath(os.Getenv("PATH")) + if !pathIsMinimal && !anyDockerVarMissing() { + // Healthy interactive launch with all docker vars present — no-op. + return false, snapshot + } + + env := captureLoginShellEnv(logger) + if len(env) == 0 { + return false, snapshot + } + + sep := string(os.PathListSeparator) + + // PATH: merge login-first so docker / uvx / npx resolve correctly. + // Only merge when PATH looks launchd-minimal — a comprehensive PATH is left + // untouched even if curated vars triggered hydration. + if pathIsMinimal { + if loginPath := env["PATH"]; loginPath != "" { + current := os.Getenv("PATH") + merged := mergePathUnique(loginPath, current, sep) + if merged != current { + _ = os.Setenv("PATH", merged) + snapshot["PATH"] = merged + } + } + } + + // Curated keys: set-if-unset only, never clobber an operator-set value + // (LookupEnv so an explicit empty value is preserved). + for _, key := range curatedSingleKeys { + setEnvIfUnset(env, key, snapshot) + } + + if len(snapshot) == 0 { + return false, snapshot + } + + if logger != nil { + logger.Info("shellwrap: hydrated login-shell environment for GUI/launchd launch", + zap.Strings("keys", sortedKeys(snapshot)), + zap.Int("count", len(snapshot))) + for _, k := range sortedKeys(snapshot) { + // Key name + value length only — never the value itself. + logger.Debug("shellwrap: hydrated env var", + zap.String("key", k), + zap.Int("value_length", len(snapshot[k]))) + } + } + return true, snapshot +} + +// anyDockerVarMissing reports whether any DOCKER_* entry from curatedSingleKeys +// is absent from the current process environment. Used as a secondary hydration +// trigger when PATH is already comprehensive but Docker connectivity vars are +// absent (common when a GUI launcher pre-seeds PATH via /etc/paths but rc files +// set DOCKER_HOST only in login-shell context). +func anyDockerVarMissing() bool { + for _, key := range curatedSingleKeys { + if strings.HasPrefix(key, "DOCKER_") { + if _, present := os.LookupEnv(key); !present { + return true + } + } + } + return false +} + +// setEnvIfUnset hydrates a single key from the captured login-shell env when it +// provides a non-empty value and the key is not already present in the process +// env. Records applied keys in snapshot. Uses LookupEnv so an explicitly +// set-empty operator value is preserved. +func setEnvIfUnset(loginEnv map[string]string, key string, snapshot map[string]string) { + val, ok := loginEnv[key] + if !ok || val == "" { + return + } + if _, present := os.LookupEnv(key); present { + return + } + _ = os.Setenv(key, val) + snapshot[key] = val +} + +// looksLikeLaunchdMinimalPath mirrors the secureenv heuristic +// (manager.go buildEnhancedPath): a PATH that already contains a common tool +// directory is a healthy interactive launch — hydration must be a no-op there. +func looksLikeLaunchdMinimalPath(path string) bool { + for _, dir := range []string{"/usr/local/bin", "/opt/homebrew/bin"} { + for _, p := range strings.Split(path, string(os.PathListSeparator)) { + if p == dir { + return false + } + } + } + return true +} + +func sortedKeys(m map[string]string) []string { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} diff --git a/internal/shellwrap/hydrate_test.go b/internal/shellwrap/hydrate_test.go new file mode 100644 index 000000000..a3016e491 --- /dev/null +++ b/internal/shellwrap/hydrate_test.go @@ -0,0 +1,314 @@ +package shellwrap + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" +) + +// --- helpers ------------------------------------------------------------- + +// writeFakeLoginEnvShell writes a POSIX shell that exports the supplied +// environment (simulating the user's rc files) BEFORE evaluating its `-c` +// command (captureLoginShellEnv's `env -0`). This lets tests assert that the +// capture/hydration picks up login-only vars that differ from the ambient +// process environment. PATH should include /usr/bin so the fake shell can +// locate the `env` binary. +func writeFakeLoginEnvShell(t *testing.T, dir string, overrides map[string]string) string { + t.Helper() + var b strings.Builder + b.WriteString("#!/bin/sh\n") + for k, v := range overrides { + b.WriteString("export " + k + "='" + v + "'\n") + } + b.WriteString(`while [ $# -gt 0 ]; do + case "$1" in + -l|--login) shift ;; + -c) shift; eval "$1"; shift ;; + *) shift ;; + esac +done +`) + p := filepath.Join(dir, "fake-login-env-shell") + require.NoError(t, os.WriteFile(p, []byte(b.String()), 0o755)) + return p +} + +// restoreEnvAfter snapshots the process environment and restores it on +// cleanup, since HydrateFromLoginShell mutates os env directly (not via +// t.Setenv, which auto-restores). +func restoreEnvAfter(t *testing.T) { + t.Helper() + saved := os.Environ() + t.Cleanup(func() { + os.Clearenv() + for _, kv := range saved { + if i := strings.IndexByte(kv, '='); i >= 0 { + _ = os.Setenv(kv[:i], kv[i+1:]) + } + } + }) +} + +// withHydrationGOOS overrides the macOS-only gate seam so the logic can be +// exercised on Linux CI runners. +func withHydrationGOOS(t *testing.T, goos string) { + t.Helper() + prev := hydrationGOOS + hydrationGOOS = goos + t.Cleanup(func() { hydrationGOOS = prev }) +} + +// --- tests --------------------------------------------------------------- + +func TestHydrate_GateNoOpOnNonDarwin(t *testing.T) { + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "linux") + + t.Setenv("PATH", "/usr/bin:/bin") // minimal, but non-darwin gate wins + + applied, snapshot := HydrateFromLoginShell(nil) + assert.False(t, applied, "hydration must be a no-op on non-darwin") + assert.Empty(t, snapshot) +} + +func TestHydrate_GateNoOpOnComprehensivePathAndAllDockerPresent(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + // Comprehensive PATH AND all DOCKER_* vars already present ⇒ neither gate + // condition triggers ⇒ hydration is a no-op. + t.Setenv("PATH", "/usr/local/bin:/usr/bin:/bin") + t.Setenv("DOCKER_HOST", "unix:///already/set.sock") + t.Setenv("DOCKER_CONTEXT", "desktop-linux") + t.Setenv("DOCKER_CONFIG", "/Users/me/.docker") + t.Setenv("DOCKER_CERT_PATH", "/Users/me/.docker/certs") + t.Setenv("DOCKER_TLS_VERIFY", "1") + + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "DOCKER_HOST": "unix:///should/not/be/used.sock", + }) + t.Setenv("SHELL", fake) + + applied, snapshot := HydrateFromLoginShell(nil) + assert.False(t, applied, "comprehensive PATH + all docker vars ⇒ no-op") + assert.Empty(t, snapshot) + assert.Equal(t, "unix:///already/set.sock", os.Getenv("DOCKER_HOST"), + "existing DOCKER_HOST must not be touched") +} + +func TestHydrate_ComprehensivePathStillHydratesDockerIfMissing(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + // PATH is comprehensive (pre-seeded by /etc/paths or similar), but DOCKER_HOST + // is absent — the second gate condition triggers hydration for curated vars + // but must NOT merge PATH. + t.Setenv("PATH", "/usr/local/bin:/usr/bin:/bin") + os.Unsetenv("DOCKER_HOST") + os.Unsetenv("DOCKER_CONTEXT") + os.Unsetenv("DOCKER_CONFIG") + os.Unsetenv("DOCKER_CERT_PATH") + os.Unsetenv("DOCKER_TLS_VERIFY") + + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "DOCKER_HOST": "unix:///Users/me/.docker/run/docker.sock", + }) + t.Setenv("SHELL", fake) + + applied, snapshot := HydrateFromLoginShell(nil) + require.True(t, applied, "missing DOCKER_HOST must trigger hydration") + + assert.Equal(t, "unix:///Users/me/.docker/run/docker.sock", os.Getenv("DOCKER_HOST")) + + // PATH must not be modified — it was already comprehensive. + assert.Equal(t, "/usr/local/bin:/usr/bin:/bin", os.Getenv("PATH"), + "comprehensive PATH must not be merged") + assert.NotContains(t, snapshot, "PATH", + "PATH must not appear in the snapshot when unchanged") + assert.Contains(t, snapshot, "DOCKER_HOST") +} + +func TestHydrate_MinimalPathHydratesCuratedSet(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + t.Setenv("PATH", "/usr/bin:/bin") // launchd-minimal + os.Unsetenv("DOCKER_HOST") + os.Unsetenv("HOMEBREW_PREFIX") + os.Unsetenv("GITHUB_TOKEN") + + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "DOCKER_HOST": "unix:///Users/me/.docker/run/docker.sock", + "HOMEBREW_PREFIX": "/opt/homebrew", + "GITHUB_TOKEN": "ghp_secret_must_not_be_imported", + // Proxy vars must NOT be imported — they are out of scope for this fix. + "HTTPS_PROXY": "http://proxy.corp:8080", + }) + t.Setenv("SHELL", fake) + + applied, snapshot := HydrateFromLoginShell(nil) + require.True(t, applied, "launchd-minimal macOS launch must hydrate") + + // PATH merged login-first, ambient preserved. + assert.True(t, strings.HasPrefix(os.Getenv("PATH"), "/opt/homebrew/bin:/usr/local/bin"), + "PATH must be enriched login-first, got %q", os.Getenv("PATH")) + assert.Contains(t, os.Getenv("PATH"), "/usr/bin") + + // Curated container / tool-home vars present in the process env. + assert.Equal(t, "unix:///Users/me/.docker/run/docker.sock", os.Getenv("DOCKER_HOST")) + assert.Equal(t, "/opt/homebrew", os.Getenv("HOMEBREW_PREFIX")) + + // Proxy vars are NOT in the allow-list and must never be hydrated. + assert.Empty(t, os.Getenv("HTTPS_PROXY"), "proxy vars must never be hydrated (out of scope)") + _, proxyLeaked := snapshot["HTTPS_PROXY"] + assert.False(t, proxyLeaked, "proxy var must not appear in the diagnostic snapshot") + + // Secrets are NOT in the allow-list and must never be hydrated. + assert.Empty(t, os.Getenv("GITHUB_TOKEN"), "secrets must never be hydrated into the daemon") + _, leaked := snapshot["GITHUB_TOKEN"] + assert.False(t, leaked, "secret must not appear in the diagnostic snapshot") + + assert.Contains(t, snapshot, "PATH") + assert.Contains(t, snapshot, "DOCKER_HOST") +} + +func TestHydrate_SetIfEmptyNeverClobbers(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + t.Setenv("PATH", "/usr/bin:/bin") + // Operator already set DOCKER_HOST on the daemon — hydration must not clobber. + t.Setenv("DOCKER_HOST", "tcp://operator-set:2375") + + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "DOCKER_HOST": "unix:///login/value.sock", + }) + t.Setenv("SHELL", fake) + + applied, snapshot := HydrateFromLoginShell(nil) + require.True(t, applied, "PATH enrichment still applies") + + assert.Equal(t, "tcp://operator-set:2375", os.Getenv("DOCKER_HOST"), + "operator-set DOCKER_HOST must never be clobbered") + _, inSnap := snapshot["DOCKER_HOST"] + assert.False(t, inSnap, "un-applied (clobber-skipped) key must not be in snapshot") +} + +func TestHydrate_NeverTouchesHomeUserShell(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + t.Setenv("PATH", "/usr/bin:/bin") + t.Setenv("HOME", "/real/home") + t.Setenv("USER", "realuser") + + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "HOME": "/evil/home", + "USER": "eviluser", + }) + t.Setenv("SHELL", fake) + + _, _ = HydrateFromLoginShell(nil) + + assert.Equal(t, "/real/home", os.Getenv("HOME"), "HOME must never be hydrated") + assert.Equal(t, "realuser", os.Getenv("USER"), "USER must never be hydrated") +} + +func TestHydrate_NeverLogsValues(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + t.Setenv("PATH", "/usr/bin:/bin") + os.Unsetenv("DOCKER_HOST") + + const secretVal = "unix:///super/secret/docker-socket-value.sock" + dir := t.TempDir() + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + "DOCKER_HOST": secretVal, + }) + t.Setenv("SHELL", fake) + + core, recorded := observer.New(zapcore.DebugLevel) + logger := zap.New(core) + + applied, _ := HydrateFromLoginShell(logger) + require.True(t, applied) + + for _, e := range recorded.All() { + assert.NotContains(t, e.Message, secretVal, "log message must never contain a hydrated value") + for k, v := range e.ContextMap() { + assert.NotContains(t, fmt.Sprintf("%v", v), secretVal, + "log field %q must never contain a hydrated value", k) + } + } +} + +func TestHydrate_CaptureFailureIsNoOp(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell hydration is unix-only") + } + resetLoginShellEnvCacheForTest() + t.Cleanup(resetLoginShellEnvCacheForTest) + restoreEnvAfter(t) + withHydrationGOOS(t, "darwin") + + t.Setenv("PATH", "/usr/bin:/bin") + t.Setenv("SHELL", "/nonexistent/shell-binary-does-not-exist") + + applied, snapshot := HydrateFromLoginShell(nil) + assert.False(t, applied, "a failed login-shell capture must degrade to a no-op") + assert.Empty(t, snapshot) +} diff --git a/internal/shellwrap/shellwrap.go b/internal/shellwrap/shellwrap.go index d7d6bbdc7..ee4517d59 100644 --- a/internal/shellwrap/shellwrap.go +++ b/internal/shellwrap/shellwrap.go @@ -309,15 +309,10 @@ func resetDockerPathCacheForTest() { // --- Login-shell PATH capture -------------------------------------------- -var ( - loginShellPathOnce sync.Once - loginShellPathVal string -) - // LoginShellPATH returns the PATH value emitted by the user's login shell. -// It is captured exactly once per process via -// ` -l -c 'printf %s "$PATH"'` and cached for the rest of the -// process lifetime. +// It is a thin view over captureLoginShellEnv (hydrate.go), which sources the +// login shell exactly once per process and caches the full environment — so +// PATH capture and env hydration share a single shell fork. // // Why this exists: when mcpproxy runs as a macOS App Bundle or LaunchAgent, // os.Getenv("PATH") is often `/usr/bin:/bin`. That is enough for Go's @@ -334,72 +329,12 @@ var ( // // Callers should treat an empty return value as "no override available" // and fall back to os.Getenv("PATH"). -// loginShellPATHMarkerBegin / loginShellPATHMarkerEnd bracket the captured -// PATH inside the shell's stdout. The markers let us pluck the real PATH -// out of arbitrary banner / motd / oh-my-zsh / direnv output that login-rc -// files routinely emit on stdout. See issue #439. -// -// The strings are deliberately unlikely to appear in any rc-file banner. -const ( - loginShellPATHMarkerBegin = "__MCPPROXY_LOGIN_PATH_BEGIN__" - loginShellPATHMarkerEnd = "__MCPPROXY_LOGIN_PATH_END__" -) - -// extractLoginShellPATH parses the stdout of the login-shell capture command -// and returns the PATH between the begin/end markers. If the markers are -// absent (e.g. test fakes that ignore `-c`), it falls back to the trimmed -// stdout for backward compatibility. Returns "" if no usable value is found. -func extractLoginShellPATH(stdout string) string { - if i := strings.Index(stdout, loginShellPATHMarkerBegin); i >= 0 { - rest := stdout[i+len(loginShellPATHMarkerBegin):] - if j := strings.Index(rest, loginShellPATHMarkerEnd); j >= 0 { - return rest[:j] - } - } - return strings.TrimSpace(stdout) -} - func LoginShellPATH(logger *zap.Logger) string { - loginShellPathOnce.Do(func() { - if runtime.GOOS == osWindows { - return - } - shell := resolveLoginShell() - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() - // Bracket the captured PATH with unique markers so banner output - // emitted by rc files (welcome messages, oh-my-zsh updates, - // direnv hooks, motd, etc.) does NOT leak into the value we - // hand back to callers. Without markers the captured stdout - // becomes "Welcome banner/usr/local/bin:…" — exec.LookPath then - // can't resolve anything. - // - // We deliberately build the argv ourselves rather than going - // through WrapWithUserShell because shellescape would quote - // the `$PATH` and suppress expansion. - script := `printf '` + loginShellPATHMarkerBegin + `%s` + loginShellPATHMarkerEnd + `' "$PATH"` - cmd := exec.CommandContext(ctx, shell, "-l", "-c", script) - out, err := cmd.Output() - if err != nil { - if logger != nil { - logger.Debug("shellwrap: login-shell PATH capture failed", - zap.String("shell", shell), - zap.Error(err)) - } - return - } - captured := extractLoginShellPATH(string(out)) - if captured == "" { - return - } - loginShellPathVal = captured - if logger != nil { - logger.Debug("shellwrap: captured login-shell PATH", - zap.String("shell", shell), - zap.Int("path_length", len(captured))) - } - }) - return loginShellPathVal + env := captureLoginShellEnv(logger) + if env == nil { + return "" + } + return env["PATH"] } // mergePathUnique joins two PATH-style strings into one, preserving the @@ -431,10 +366,10 @@ func mergePathUnique(primary, secondary, sep string) string { return strings.Join(parts, sep) } -// resetLoginShellPathCacheForTest is only referenced from shellwrap_test.go. +// resetLoginShellPathCacheForTest clears the shared login-shell capture cache. +// Only referenced from shellwrap_test.go. func resetLoginShellPathCacheForTest() { - loginShellPathOnce = sync.Once{} - loginShellPathVal = "" + resetLoginShellEnvCacheForTest() } // --- Minimal environment for scanner subprocesses ------------------------ diff --git a/internal/shellwrap/shellwrap_test.go b/internal/shellwrap/shellwrap_test.go index a05af95c8..809b8243e 100644 --- a/internal/shellwrap/shellwrap_test.go +++ b/internal/shellwrap/shellwrap_test.go @@ -420,7 +420,10 @@ func TestLoginShellPATH_CapturesFromShell(t *testing.T) { dir := t.TempDir() want := "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" - fake := writeFakeShell(t, dir, want) + // The login shell exports this PATH; captureLoginShellEnv's `env -0` then + // reports it back. PATH must include /usr/bin so the fake shell can locate + // the `env` binary. + fake := writeFakeLoginEnvShell(t, dir, map[string]string{"PATH": want}) t.Setenv("SHELL", fake) got := LoginShellPATH(nil) @@ -454,9 +457,13 @@ func TestMinimalEnv_PrefersLoginShellPATH(t *testing.T) { // Simulate a LaunchAgent-like ambient PATH (missing homebrew/local). t.Setenv("PATH", "/usr/bin:/bin") - // Point $SHELL at a fake shell that prints the real (enriched) PATH. + // Point $SHELL at a fake login shell that exports the real (enriched) PATH, + // so captureLoginShellEnv reports it back enriched while the ambient PATH + // stays minimal. dir := t.TempDir() - fake := writeFakeShell(t, dir, "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin") + fake := writeFakeLoginEnvShell(t, dir, map[string]string{ + "PATH": "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + }) t.Setenv("SHELL", fake) env := MinimalEnv()