smart-mcp-proxy
diff --git a/‎internal/security/scanner/docker.go‎
Lines changed: 43 additions & 9 deletions b/‎internal/security/scanner/docker.go‎
Lines changed: 43 additions & 9 deletions
diff --git a/‎internal/security/scanner/docker_test.go‎
Lines changed: 39 additions & 0 deletions b/‎internal/security/scanner/docker_test.go‎
Lines changed: 39 additions & 0 deletions
diff --git a/‎internal/shellwrap/shellwrap.go‎
Lines changed: 224 additions & 0 deletions b/‎internal/shellwrap/shellwrap.go‎
Lines changed: 224 additions & 0 deletions
@@ -10,10 +10,23 @@ import (
 	"strings"
 	"time"
 
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap"
 	"go.uber.org/zap"
 )
 
-// DockerRunner executes Docker operations for scanner containers
+// DockerRunner executes Docker operations for scanner containers.
+//
+// Security note: the scanner runs untrusted container images (vulnerability
+// scanners, SBOM generators, etc). It MUST NOT inherit the user's ambient
+// credentials (AWS, GitHub, Anthropic tokens, …). We therefore:
+//  1. Resolve the docker binary once via shellwrap.ResolveDockerPath — this
+//     picks up Homebrew / Docker Desktop / Colima installs even when mcpproxy
+//     was launched from a GUI with a minimal PATH, WITHOUT re-spawning a
+//     login shell on every invocation (important for hot paths like the
+//     ~2s health check loop).
+//  2. Invoke docker directly with cmd.Env set to a minimal PATH+HOME
+//     allowlist via shellwrap.MinimalEnv, so environment-based secrets do
+//     not leak into the docker CLI's subprocess tree.
 type DockerRunner struct {
 	logger *zap.Logger
 }
@@ -23,9 +36,30 @@ func NewDockerRunner(logger *zap.Logger) *DockerRunner {
 	return &DockerRunner{logger: logger}
 }
 
+// getDockerCmd creates an exec.Cmd for Docker with a minimal, allow-listed
+// environment so ambient secrets (AWS creds, GitHub tokens, …) cannot leak
+// into the security scanner's subprocess. The docker binary is resolved once
+// via shellwrap and then cached for the process lifetime.
+func (d *DockerRunner) getDockerCmd(ctx context.Context, args ...string) *exec.Cmd {
+	dockerBin, err := shellwrap.ResolveDockerPath(d.logger)
+	if err != nil || dockerBin == "" {
+		// Fall back to plain "docker" so exec returns a clear ENOENT the
+		// caller can surface. We still set the minimal env to avoid leaking
+		// secrets even in the error path.
+		dockerBin = "docker"
+		if d.logger != nil {
+			d.logger.Debug("scanner docker: falling back to bare 'docker' lookup",
+				zap.Error(err))
+		}
+	}
+	cmd := exec.CommandContext(ctx, dockerBin, args...)
+	cmd.Env = shellwrap.MinimalEnv()
+	return cmd
+}
+
 // IsDockerAvailable checks if Docker daemon is running
 func (d *DockerRunner) IsDockerAvailable(ctx context.Context) bool {
-	cmd := exec.CommandContext(ctx, "docker", "info")
+	cmd := d.getDockerCmd(ctx, "info")
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 	return cmd.Run() == nil
@@ -34,7 +68,7 @@ func (d *DockerRunner) IsDockerAvailable(ctx context.Context) bool {
 // PullImage pulls a Docker image with progress logging
 func (d *DockerRunner) PullImage(ctx context.Context, image string) error {
 	d.logger.Info("Pulling Docker image", zap.String("image", image))
-	cmd := exec.CommandContext(ctx, "docker", "pull", image)
+	cmd := d.getDockerCmd(ctx, "pull", image)
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
 	if err := cmd.Run(); err != nil {
@@ -46,15 +80,15 @@ func (d *DockerRunner) PullImage(ctx context.Context, image string) error {
 
 // ImageExists checks if a Docker image exists locally
 func (d *DockerRunner) ImageExists(ctx context.Context, image string) bool {
-	cmd := exec.CommandContext(ctx, "docker", "image", "inspect", image)
+	cmd := d.getDockerCmd(ctx, "image", "inspect", image)
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 	return cmd.Run() == nil
 }
 
 // RemoveImage removes a Docker image
 func (d *DockerRunner) RemoveImage(ctx context.Context, image string) error {
-	cmd := exec.CommandContext(ctx, "docker", "rmi", "-f", image)
+	cmd := d.getDockerCmd(ctx, "rmi", "-f", image)
 	var stderr bytes.Buffer
 	cmd.Stderr = &stderr
 	if err := cmd.Run(); err != nil {
@@ -156,7 +190,7 @@ func (d *DockerRunner) RunScanner(ctx context.Context, cfg ScannerRunConfig) (st
 		defer cancel()
 	}
 
-	cmd := exec.CommandContext(runCtx, "docker", args...)
+	cmd := d.getDockerCmd(runCtx, args...)
 	var stdoutBuf, stderrBuf bytes.Buffer
 	cmd.Stdout = &stdoutBuf
 	cmd.Stderr = &stderrBuf
@@ -186,7 +220,7 @@ func (d *DockerRunner) KillContainer(ctx context.Context, name string) error {
 	if name == "" {
 		return nil
 	}
-	cmd := exec.CommandContext(ctx, "docker", "kill", name)
+	cmd := d.getDockerCmd(ctx, "kill", name)
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 	return cmd.Run()
@@ -197,7 +231,7 @@ func (d *DockerRunner) StopContainer(ctx context.Context, name string, timeout i
 	if name == "" {
 		return nil
 	}
-	cmd := exec.CommandContext(ctx, "docker", "stop", "-t", fmt.Sprintf("%d", timeout), name)
+	cmd := d.getDockerCmd(ctx, "stop", "-t", fmt.Sprintf("%d", timeout), name)
 	cmd.Stdout = nil
 	cmd.Stderr = nil
 	return cmd.Run()
@@ -224,7 +258,7 @@ func (d *DockerRunner) ReadReportFile(reportDir string) ([]byte, error) {
 
 // GetImageDigest returns the Docker image digest
 func (d *DockerRunner) GetImageDigest(ctx context.Context, image string) (string, error) {
-	cmd := exec.CommandContext(ctx, "docker", "inspect", "--format", "{{.Id}}", image)
+	cmd := d.getDockerCmd(ctx, "inspect", "--format", "{{.Id}}", image)
 	var stdout bytes.Buffer
 	cmd.Stdout = &stdout
 	if err := cmd.Run(); err != nil {
 
@@ -225,3 +225,42 @@ func TestStopContainerEmptyName(t *testing.T) {
 		t.Errorf("StopContainer with empty name should return nil, got: %v", err)
 	}
 }
+
+// TestDockerRunnerDoesNotLeakAmbientSecrets verifies that the scanner's
+// Docker subprocess is invoked with a minimal, allow-listed environment so
+// credentials in the parent process environment (AWS, GitHub, Anthropic,
+// …) cannot leak into containers that run untrusted scanner images.
+func TestDockerRunnerDoesNotLeakAmbientSecrets(t *testing.T) {
+	t.Setenv("AWS_ACCESS_KEY_ID", "AKIA_test_dummy_value_00000000")
+	t.Setenv("GITHUB_TOKEN", "ghp_test_dummy_token_1234567890abcdef")
+	t.Setenv("ANTHROPIC_API_KEY", "sk-ant-test-dummy-not-real")
+
+	logger := zap.NewNop()
+	d := NewDockerRunner(logger)
+
+	cmd := d.getDockerCmd(context.Background(), "version")
+
+	if cmd.Env == nil {
+		t.Fatal("cmd.Env must be explicitly set (non-nil) so it does not inherit os.Environ()")
+	}
+
+	joined := strings.Join(cmd.Env, "\n")
+	forbidden := []string{"AWS_ACCESS_KEY_ID", "GITHUB_TOKEN", "ANTHROPIC_API_KEY"}
+	for _, key := range forbidden {
+		if strings.Contains(joined, key) {
+			t.Errorf("scanner docker command leaked %q into cmd.Env", key)
+		}
+	}
+
+	// PATH must still be present so the docker binary is actually runnable.
+	hasPath := false
+	for _, kv := range cmd.Env {
+		if strings.HasPrefix(kv, "PATH=") {
+			hasPath = true
+			break
+		}
+	}
+	if !hasPath {
+		t.Error("scanner docker command must retain PATH so 'docker' is discoverable")
+	}
+}
@@ -0,0 +1,224 @@
+// Package shellwrap provides platform-level helpers for wrapping commands in
+// the user's login shell and for resolving tool binaries (e.g. docker) with
+// PATH caching.
+//
+// It exists so that both the upstream proxy code (internal/upstream/core) and
+// the security scanner (internal/security/scanner) can share a single,
+// well-tested implementation of shell quoting + login-shell wrapping instead
+// of each rolling their own.
+package shellwrap
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"runtime"
+	"strings"
+	"sync"
+	"time"
+
+	"go.uber.org/zap"
+)
+
+const (
+	osWindows = "windows"
+	// defaultUnixShell is used when $SHELL is unset on a Unix-like system.
+	defaultUnixShell = "/bin/bash"
+	// defaultWindowsShell is used when neither $ComSpec nor $SHELL is set.
+	defaultWindowsShell = "cmd"
+)
+
+// Shellescape escapes a single argument for safe inclusion in a shell command
+// string. On Unix it uses POSIX single-quoting; on Windows it performs a
+// best-effort cmd.exe quoting.
+//
+// This mirrors the implementation in internal/upstream/core so both code paths
+// can converge on one function.
+func Shellescape(s string) string {
+	if s == "" {
+		if runtime.GOOS == osWindows {
+			return `""`
+		}
+		return "''"
+	}
+
+	if runtime.GOOS == osWindows {
+		// Windows cmd.exe special characters.
+		if !strings.ContainsAny(s, " \t\n\r\"&|<>()^%") {
+			return s
+		}
+		// cmd.exe does not use backslash as an escape character. If the
+		// caller supplied embedded double quotes we strip them — this is
+		// the same behaviour the upstream helper has used since PR #195.
+		cleaned := strings.Trim(s, `"`)
+		return `"` + cleaned + `"`
+	}
+
+	// Unix shell special characters: if none present, return as-is.
+	if !strings.ContainsAny(s, " \t\n\r\"'\\$`;&|<>(){}[]?*~") {
+		return s
+	}
+	// Use single quotes and escape any embedded single quotes.
+	return "'" + strings.ReplaceAll(s, "'", "'\"'\"'") + "'"
+}
+
+// isBashLikeShell mirrors the detection logic in connection_stdio.go so that
+// Git Bash / MSYS on Windows uses the Unix-style -l -c flags.
+func isBashLikeShell(shell string) bool {
+	lower := strings.ToLower(shell)
+	return strings.Contains(lower, "bash") || strings.Contains(lower, "sh")
+}
+
+// resolveLoginShell returns the user's preferred login shell, respecting the
+// $SHELL environment variable and falling back to platform defaults.
+func resolveLoginShell() string {
+	shell := os.Getenv("SHELL")
+	if shell != "" {
+		return shell
+	}
+	if runtime.GOOS == osWindows {
+		if cs := os.Getenv("ComSpec"); cs != "" {
+			return cs
+		}
+		return defaultWindowsShell
+	}
+	return defaultUnixShell
+}
+
+// WrapWithUserShell wraps a command and its arguments in the user's login
+// shell so the child process inherits the interactive PATH (important when
+// mcpproxy is launched from a GUI / LaunchAgent on macOS).
+//
+// It returns the shell to exec and the shell arguments (e.g. ["-l", "-c",
+// "docker run ..."] on Unix, ["/c", "docker run ..."] on Windows cmd).
+//
+// logger may be nil; when non-nil a debug line is emitted mirroring the
+// existing upstream/core helper.
+func WrapWithUserShell(logger *zap.Logger, command string, args []string) (shell string, shellArgs []string) {
+	shell = resolveLoginShell()
+
+	parts := make([]string, 0, len(args)+1)
+	parts = append(parts, Shellescape(command))
+	for _, a := range args {
+		parts = append(parts, Shellescape(a))
+	}
+	commandString := strings.Join(parts, " ")
+
+	if logger != nil {
+		logger.Debug("shellwrap: wrapping command with user login shell",
+			zap.String("original_command", command),
+			zap.Strings("original_args", args),
+			zap.String("shell", shell),
+			zap.String("wrapped_command", commandString))
+	}
+
+	isBash := isBashLikeShell(shell)
+	if runtime.GOOS == osWindows && !isBash {
+		// Windows cmd.exe: /c to execute a command string.
+		return shell, []string{"/c", commandString}
+	}
+	// Unix shells and Git Bash on Windows: -l for login env, -c for command.
+	return shell, []string{"-l", "-c", commandString}
+}
+
+// --- Docker path resolution ---------------------------------------------
+
+var (
+	dockerPathOnce sync.Once
+	dockerPath     string
+	dockerPathErr  error
+)
+
+// ResolveDockerPath returns the absolute path to the `docker` binary. The
+// result is cached for the process lifetime so that repeated calls from hot
+// paths (health checks, connection diagnostics) do not re-spawn a login shell
+// on every invocation.
+//
+// Resolution order:
+//  1. exec.LookPath("docker") — cheap, works when mcpproxy was started from
+//     a terminal or when the LaunchAgent PATH already contains docker.
+//  2. Fallback: ask the user's login shell `command -v docker` so we pick up
+//     Homebrew / Colima / Docker Desktop installs that only exist in the
+//     interactive PATH. This fallback is only run once.
+func ResolveDockerPath(logger *zap.Logger) (string, error) {
+	dockerPathOnce.Do(func() {
+		// Fast path: ask Go's standard PATH lookup first.
+		if p, err := exec.LookPath("docker"); err == nil && p != "" {
+			dockerPath = p
+			if logger != nil {
+				logger.Debug("shellwrap: resolved docker via PATH", zap.String("path", p))
+			}
+			return
+		}
+
+		// Slow path: shell out once via the user's login shell.
+		if runtime.GOOS == osWindows {
+			dockerPathErr = fmt.Errorf("docker not found in PATH")
+			return
+		}
+
+		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+		defer cancel()
+
+		shell, shellArgs := WrapWithUserShell(logger, "command", []string{"-v", "docker"})
+		cmd := exec.CommandContext(ctx, shell, shellArgs...)
+		out, err := cmd.Output()
+		if err != nil {
+			dockerPathErr = fmt.Errorf("login-shell docker lookup failed: %w", err)
+			return
+		}
+		resolved := strings.TrimSpace(string(out))
+		if resolved == "" {
+			dockerPathErr = fmt.Errorf("docker not found in login shell PATH")
+			return
+		}
+		dockerPath = resolved
+		if logger != nil {
+			logger.Debug("shellwrap: resolved docker via login shell",
+				zap.String("path", resolved))
+		}
+	})
+	return dockerPath, dockerPathErr
+}
+
+// resetDockerPathCacheForTest is used by tests to probe the sync.Once
+// behaviour. It is intentionally unexported and only referenced from
+// shellwrap_test.go.
+func resetDockerPathCacheForTest() {
+	dockerPathOnce = sync.Once{}
+	dockerPath = ""
+	dockerPathErr = nil
+}
+
+// --- Minimal environment for scanner subprocesses ------------------------
+
+// MinimalEnv returns a minimal, allow-listed environment suitable for
+// subprocesses that must NOT inherit the user's ambient credentials (e.g.
+// AWS_ACCESS_KEY_ID, GITHUB_TOKEN, etc). It includes PATH + HOME on Unix and
+// PATH + USERPROFILE on Windows so that `docker` itself still functions.
+//
+// Callers that need TLS or Docker-specific variables (DOCKER_HOST,
+// DOCKER_CONFIG, …) should append them explicitly.
+func MinimalEnv() []string {
+	env := make([]string, 0, 8)
+	if v := os.Getenv("PATH"); v != "" {
+		env = append(env, "PATH="+v)
+	}
+	if runtime.GOOS == osWindows {
+		if v := os.Getenv("USERPROFILE"); v != "" {
+			env = append(env, "USERPROFILE="+v)
+		}
+		if v := os.Getenv("SystemRoot"); v != "" {
+			env = append(env, "SystemRoot="+v)
+		}
+		if v := os.Getenv("ComSpec"); v != "" {
+			env = append(env, "ComSpec="+v)
+		}
+	} else {
+		if v := os.Getenv("HOME"); v != "" {
+			env = append(env, "HOME="+v)
+		}
+	}
+	return env
+}