diff --git a/internal/shellwrap/shellwrap.go b/internal/shellwrap/shellwrap.go index ddb4b0f1..aba9e389 100644 --- a/internal/shellwrap/shellwrap.go +++ b/internal/shellwrap/shellwrap.go @@ -191,6 +191,104 @@ func resetDockerPathCacheForTest() { dockerPathErr = nil } +// --- 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. +// +// 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 +// exec.LookPath to find a docker binary once shellwrap.ResolveDockerPath +// has cached its absolute path, but it is NOT enough for the docker CLI +// itself, which re-execs credential helpers like `docker-credential-desktop` +// via its own $PATH lookup. Those helpers typically live in +// /usr/local/bin or /opt/homebrew/bin — directories that only exist in +// the interactive login PATH. +// +// On Windows, this function returns "" (credential-helper PATH drift is +// not the same problem there, and interactive-shell PATH capture would +// require cmd.exe or PowerShell gymnastics we explicitly avoid). +// +// Callers should treat an empty return value as "no override available" +// and fall back to os.Getenv("PATH"). +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() + // `-l -c 'printf %s "$PATH"'` works on bash, zsh, dash, sh. + // We deliberately build the argv ourselves rather than going + // through WrapWithUserShell because shellescape would quote + // the `$PATH` and suppress expansion. + cmd := exec.CommandContext(ctx, shell, "-l", "-c", `printf %s "$PATH"`) + 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 := strings.TrimSpace(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 +} + +// mergePathUnique joins two PATH-style strings into one, preserving the +// order of `primary` (highest priority) followed by any entries of +// `secondary` that were not already present. Empty segments are dropped. +func mergePathUnique(primary, secondary, sep string) string { + if primary == "" { + return secondary + } + if secondary == "" { + return primary + } + seen := make(map[string]struct{}, 16) + parts := make([]string, 0, 16) + add := func(list string) { + for _, p := range strings.Split(list, sep) { + if p == "" { + continue + } + if _, ok := seen[p]; ok { + continue + } + seen[p] = struct{}{} + parts = append(parts, p) + } + } + add(primary) + add(secondary) + return strings.Join(parts, sep) +} + +// resetLoginShellPathCacheForTest is only referenced from shellwrap_test.go. +func resetLoginShellPathCacheForTest() { + loginShellPathOnce = sync.Once{} + loginShellPathVal = "" +} + // --- Minimal environment for scanner subprocesses ------------------------ // MinimalEnv returns a minimal, allow-listed environment suitable for @@ -200,10 +298,28 @@ func resetDockerPathCacheForTest() { // // Callers that need TLS or Docker-specific variables (DOCKER_HOST, // DOCKER_CONFIG, …) should append them explicitly. +// +// On Unix, PATH is built by merging the user's login-shell PATH +// (captured once via LoginShellPATH) with the process's ambient PATH. +// Login-shell entries come first so that docker's own credential-helper +// lookups can find binaries installed in /opt/homebrew/bin or +// /usr/local/bin even when mcpproxy was started from a LaunchAgent with +// a minimal inherited PATH. See issue #381. func MinimalEnv() []string { + return minimalEnvWithLogger(nil) +} + +// MinimalEnvWithLogger is MinimalEnv with an optional logger used while +// capturing the login-shell PATH on the first call. Subsequent calls +// return the cached value without logging. +func MinimalEnvWithLogger(logger *zap.Logger) []string { + return minimalEnvWithLogger(logger) +} + +func minimalEnvWithLogger(logger *zap.Logger) []string { env := make([]string, 0, 8) - if v := os.Getenv("PATH"); v != "" { - env = append(env, "PATH="+v) + if path := buildMinimalPath(logger); path != "" { + env = append(env, "PATH="+path) } if runtime.GOOS == osWindows { if v := os.Getenv("USERPROFILE"); v != "" { @@ -222,3 +338,20 @@ func MinimalEnv() []string { } return env } + +// buildMinimalPath returns the PATH value that MinimalEnv should set on +// child processes. On Unix it merges the login-shell PATH with ambient +// PATH so that docker credential helpers (e.g. docker-credential-desktop) +// installed in /opt/homebrew/bin or /usr/local/bin are resolvable — see +// issue #381. On Windows it returns the ambient PATH unchanged. +func buildMinimalPath(logger *zap.Logger) string { + ambient := os.Getenv("PATH") + if runtime.GOOS == osWindows { + return ambient + } + login := LoginShellPATH(logger) + if login == "" { + return ambient + } + return mergePathUnique(login, ambient, string(os.PathListSeparator)) +} diff --git a/internal/shellwrap/shellwrap_test.go b/internal/shellwrap/shellwrap_test.go index 726cc0aa..82951e34 100644 --- a/internal/shellwrap/shellwrap_test.go +++ b/internal/shellwrap/shellwrap_test.go @@ -1,6 +1,8 @@ package shellwrap import ( + "os" + "path/filepath" "runtime" "strings" "testing" @@ -110,3 +112,141 @@ func TestMinimalEnv_DropsSecrets(t *testing.T) { } assert.True(t, hasPath, "minimal env must contain PATH") } + +func TestMergePathUnique(t *testing.T) { + cases := []struct { + name string + primary, secondary, sep string + want string + }{ + {"empty primary", "", "/usr/bin:/bin", ":", "/usr/bin:/bin"}, + {"empty secondary", "/opt/homebrew/bin", "", ":", "/opt/homebrew/bin"}, + { + "dedup keeps primary order", + "/opt/homebrew/bin:/usr/local/bin", + "/usr/bin:/opt/homebrew/bin:/bin", + ":", + "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin", + }, + { + "drops empty segments", + "/opt/homebrew/bin::/usr/local/bin", + ":/bin:", + ":", + "/opt/homebrew/bin:/usr/local/bin:/bin", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, mergePathUnique(tc.primary, tc.secondary, tc.sep)) + }) + } +} + +// writeFakeShell writes a POSIX shell script at /fake-shell that +// ignores its arguments and echoes `wantPath` on stdout. It returns the +// absolute path to the script so tests can point $SHELL at it. +func writeFakeShell(t *testing.T, dir, wantPath string) string { + t.Helper() + script := "#!/bin/sh\nprintf %s '" + wantPath + "'\n" + p := filepath.Join(dir, "fake-shell") + require.NoError(t, os.WriteFile(p, []byte(script), 0o755)) + return p +} + +func TestLoginShellPATH_CapturesFromShell(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("login-shell PATH capture is Unix-only") + } + resetLoginShellPathCacheForTest() + t.Cleanup(resetLoginShellPathCacheForTest) + + dir := t.TempDir() + want := "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin" + fake := writeFakeShell(t, dir, want) + t.Setenv("SHELL", fake) + + got := LoginShellPATH(nil) + assert.Equal(t, want, got, "should capture the PATH printed by the login shell") + + // Second call must use the cache even if we break $SHELL. + t.Setenv("SHELL", "/nonexistent/shell") + got2 := LoginShellPATH(nil) + assert.Equal(t, want, got2, "second call must return the cached value") +} + +func TestLoginShellPATH_EmptyWhenShellFails(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("unix-only") + } + resetLoginShellPathCacheForTest() + t.Cleanup(resetLoginShellPathCacheForTest) + + t.Setenv("SHELL", "/nonexistent/shell-binary-does-not-exist") + got := LoginShellPATH(nil) + assert.Equal(t, "", got, "should return empty string when the login shell cannot be executed") +} + +func TestMinimalEnv_PrefersLoginShellPATH(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("unix-only") + } + resetLoginShellPathCacheForTest() + t.Cleanup(resetLoginShellPathCacheForTest) + + // 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. + dir := t.TempDir() + fake := writeFakeShell(t, dir, "/opt/homebrew/bin:/usr/local/bin:/usr/bin:/bin") + t.Setenv("SHELL", fake) + + env := MinimalEnv() + + var pathVal string + for _, kv := range env { + if strings.HasPrefix(kv, "PATH=") { + pathVal = strings.TrimPrefix(kv, "PATH=") + break + } + } + require.NotEmpty(t, pathVal, "MinimalEnv must include PATH") + + // The login-shell entries must come first so that docker's + // credential-helper lookup can find /opt/homebrew/bin and /usr/local/bin. + assert.True(t, strings.HasPrefix(pathVal, "/opt/homebrew/bin:/usr/local/bin"), + "login-shell PATH entries must come before ambient, got %q", pathVal) + assert.Contains(t, pathVal, "/usr/bin", "ambient PATH entries must still be present") + assert.Contains(t, pathVal, "/bin", "ambient PATH entries must still be present") + + // No duplicate segments. + segs := strings.Split(pathVal, ":") + seen := make(map[string]bool, len(segs)) + for _, s := range segs { + assert.False(t, seen[s], "duplicate PATH segment %q in %q", s, pathVal) + seen[s] = true + } +} + +func TestMinimalEnv_FallsBackToAmbientPATHWhenShellBroken(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("unix-only") + } + resetLoginShellPathCacheForTest() + t.Cleanup(resetLoginShellPathCacheForTest) + + t.Setenv("PATH", "/usr/bin:/bin") + t.Setenv("SHELL", "/nonexistent/shell-binary-does-not-exist") + + env := MinimalEnv() + + var pathVal string + for _, kv := range env { + if strings.HasPrefix(kv, "PATH=") { + pathVal = strings.TrimPrefix(kv, "PATH=") + break + } + } + assert.Equal(t, "/usr/bin:/bin", pathVal, "must fall back to ambient PATH when login shell fails") +}