diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index 1c95e62d..c65364ff 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -252,9 +252,11 @@ docker stats admin-gated "install CLI tools" step. When mcpproxy is launched from a LaunchAgent / tray, the captured login-shell `PATH` may omit this directory. - mcpproxy resolves the `docker` binary to its **absolute path** and then - **exec's it directly** (no login-shell wrap) when spawning an isolated server, - so the spawn bypasses `PATH` entirely and works even without the CLI-tools - step. (The enhanced spawn `PATH` still includes the bundle bin dir as a + **exec's it directly** (no login-shell wrap) when spawning a Docker upstream — + both servers that mcpproxy *isolates* into `docker run` (uvx/npx) and upstreams + whose config `command` **is** `docker` (a user-supplied `docker run …`) — so + the spawn bypasses `PATH` entirely and works even without the CLI-tools step. + (The enhanced spawn `PATH` still includes the bundle bin dir as a belt-and-suspenders measure.) Earlier builds resolved the absolute path but still routed the spawn through `$SHELL -l -c " run …"`, where the login shell re-derived `PATH` from rc files and could drop the bundle dir — diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index fd97b620..97effdfe 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -69,44 +69,71 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm zap.String("container_command", containerCommand)) } - // CRITICAL FIX (#696 / MCP-2744 / MCP-2753): resolve `docker` to an ABSOLUTE - // path and exec it DIRECTLY — no login-shell wrap. Docker Desktop installed - // the default way on macOS (without the optional, admin-gated "install CLI - // tools" step) leaves the docker CLI only inside the app bundle at - // /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on - // any standard PATH dir nor on the (often unreliable) login-shell PATH a - // LaunchAgent captures. - // - // #697 resolved the absolute path but still routed the spawn through - // `$SHELL -l -c " run …"`. There the absolute path is only a token - // inside the -c string: the login shell re-derives $PATH from rc files and - // can drop the bundle dir, so the bug persisted. Exec'ing the absolute path - // directly (mirroring newDockerCmd's exec.CommandContext(dockerBin, …)) - // bypasses PATH entirely. - // - // Two preconditions gate the direct-exec: - // - // 1. The resolved value must be a VERIFIED absolute executable - // (isDirectExecutable). ResolveDockerPath's last resort runs - // `command -v docker` in the login shell, which can emit a shell - // function name, an alias, or a bare command name rather than a real - // binary path; direct-exec'ing that would fail with "no such file". - // - // 2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be - // guaranteed in the spawn env WITHOUT the login shell - // (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops - // rc-file env inheritance. On macOS the startup hydration (MCP-2751) - // already captured DOCKER_* from the login shell into os.Environ(), and - // secureenv forwards them — but that hydration is Darwin-only. On Linux - // a rootless/remote daemon whose DOCKER_HOST lives only in .profile would - // be lost by direct-exec (the regression #699 kept the shell wrap to - // avoid). So on non-Darwin we only direct-exec when DOCKER_HOST/ - // DOCKER_CONTEXT are already in os.Environ(); otherwise we keep the - // login-shell wrap so `docker run` still finds the daemon. - // - // When we fall back to the shell wrap we still prefer the resolved absolute - // path as the wrapped command (it sidesteps the login shell's PATH - // re-derivation), dropping to bare "docker" only when resolution failed. + // Resolve `docker` to an absolute path and decide between a direct-exec and a + // login-shell wrap. Shared with the user-supplied `docker run` path + // (connectStdio) via resolveDockerSpawn so both spawn paths behave identically + // (#696 / MCP-2868). + return c.resolveDockerSpawn(finalArgs) +} + +// resolveDockerSpawn decides how to spawn `docker` given the fully built docker +// run argv (finalArgs, beginning with the "run" token). It resolves `docker` to +// an absolute path and, when safe, returns that absolute path for a DIRECT exec +// (shellWrapped == false); otherwise it returns a login-shell wrap +// (shellWrapped == true). It is shared by BOTH docker spawn paths: +// +// - setupDockerIsolation — uvx/npx servers wrapped INTO `docker run` by the +// isolation manager (isolation-injection path). +// - connectStdio's user-supplied `docker run` branch — upstreams whose config +// Command IS `docker` (GH #696 / MCP-2868). Before this extraction that path +// shell-wrapped bare `docker`, so on a default Docker Desktop macOS install +// (docker CLI only inside the app bundle, not on the login-shell PATH) it +// failed with `zsh:1: command not found: docker`. +// +// shellWrapped governs how the caller inserts --cidfile: a direct-exec spawn +// (shellWrapped == false) uses the args-based insertCidfileIntoDockerArgs, while +// the login-shell fallback (shellWrapped == true) uses the string-based +// insertCidfileIntoShellDockerCommand. +// +// CRITICAL FIX (#696 / MCP-2744 / MCP-2753 / MCP-2868): resolve `docker` to an +// ABSOLUTE path and exec it DIRECTLY — no login-shell wrap. Docker Desktop +// installed the default way on macOS (without the optional, admin-gated "install +// CLI tools" step) leaves the docker CLI only inside the app bundle at +// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on any +// standard PATH dir nor on the (often unreliable) login-shell PATH a LaunchAgent +// captures. +// +// #697 resolved the absolute path but still routed the spawn through +// `$SHELL -l -c " run …"`. There the absolute path is only a token +// inside the -c string: the login shell re-derives $PATH from rc files and can +// drop the bundle dir, so the bug persisted. Exec'ing the absolute path directly +// (mirroring newDockerCmd's exec.CommandContext(dockerBin, …)) bypasses PATH +// entirely. +// +// Two preconditions gate the direct-exec: +// +// 1. The resolved value must be a VERIFIED absolute executable +// (isDirectExecutable). ResolveDockerPath's last resort runs +// `command -v docker` in the login shell, which can emit a shell function +// name, an alias, or a bare command name rather than a real binary path; +// direct-exec'ing that would fail with "no such file". +// +// 2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be +// guaranteed in the spawn env WITHOUT the login shell +// (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops +// rc-file env inheritance. On macOS the startup hydration (MCP-2751) already +// captured DOCKER_* from the login shell into os.Environ(), and secureenv +// forwards them — but that hydration is Darwin-only. On Linux a +// rootless/remote daemon whose DOCKER_HOST lives only in .profile would be +// lost by direct-exec (the regression #699 kept the shell wrap to avoid). So +// on non-Darwin we only direct-exec when DOCKER_HOST/DOCKER_CONTEXT are +// already in os.Environ(); otherwise we keep the login-shell wrap so +// `docker run` still finds the daemon. +// +// When we fall back to the shell wrap we still prefer the resolved absolute path +// as the wrapped command (it sidesteps the login shell's PATH re-derivation), +// dropping to bare "docker" only when resolution failed. +func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) { resolved, resErr := resolveDockerBinary(c.logger) switch { case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed(): diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index 27b14227..124a6db4 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -27,6 +27,23 @@ func newIsolatedTestClient() *Client { } } +// newUserSuppliedDockerTestClient builds a Client whose config Command IS +// `docker` (a user-supplied `docker run …` upstream, as opposed to a uvx/npx +// server wrapped INTO docker by the isolation manager). This is the GH #696 / +// MCP-2868 path. +func newUserSuppliedDockerTestClient() *Client { + return &Client{ + config: &config.ServerConfig{ + Name: "user-docker-server", + Command: "docker", + Args: []string{"run", "-i", "--rm", "mcp/example"}, + Env: map[string]string{"SLACK_TOKEN": "xoxb-secret"}, + }, + logger: zap.NewNop(), + isolationManager: NewIsolationManager(config.DefaultDockerIsolationConfig()), + } +} + // forceDockerDaemonEnvGOOS overrides the GOOS the daemon-env guard sees, so the // Darwin (env-guaranteed-by-hydration) and non-Darwin branches can both be // exercised on a single CI host. Restored on cleanup. @@ -290,6 +307,81 @@ func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) { assert.Equal(t, cmdRun, args[0]) } +// TestResolveDockerSpawnUserSuppliedDockerRunDirectExec is the MCP-2868 +// root-cause assertion. A USER-SUPPLIED `docker run` upstream (config.Command IS +// `docker`) must reuse the SAME resolve→direct-exec decision as the +// isolation-injection path. With docker resolved to a verified absolute +// executable and the daemon env guaranteed (macOS), the spawn must direct-exec +// the ABSOLUTE docker path (no `$SHELL -l -c` wrap), carry the injected +// `-e KEY=VALUE` env flags, and accept an args-based cidfile insertion. Before +// this fix the user path shell-wrapped bare `docker`, producing GH #696's +// `zsh:1: command not found: docker` on a default Docker Desktop macOS install +// (docker CLI only inside the app bundle, not on the login-shell PATH). +func TestResolveDockerSpawnUserSuppliedDockerRunDirectExec(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, osDarwin) // daemon env guaranteed via startup hydration + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newUserSuppliedDockerTestClient() + // Mimic connectStdio's user-supplied docker branch: inject env vars as -e + // flags into the docker run argv before deciding how to spawn. + argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) + + cmd, args, shellWrapped := c.resolveDockerSpawn(argsToWrap) + + assert.False(t, shellWrapped, "verified absolute docker must direct-exec, not shell-wrap") + assert.Equal(t, fakeDocker, cmd, + "user docker must be exec'd by its resolved absolute path, not bare 'docker', got: %s", cmd) + require.NotEmpty(t, args) + assert.Equal(t, cmdRun, args[0], + "first arg must be the raw 'run' token (not a shell -c string), got: %v", args) + // The injected -e env flag must survive into the docker run argv. + assert.Subset(t, args, []string{"-e", "SLACK_TOKEN=xoxb-secret"}, + "injected -e env flags must be present in the direct-exec argv, got: %v", args) + for _, a := range args { + assert.NotContains(t, a, " run ", + "args must be raw argv tokens, not a single shell command string, got: %v", args) + } + + // On the direct-exec path the caller inserts --cidfile via the args-based + // helper (right after the "run" token). + withCid := c.insertCidfileIntoDockerArgs(args, "/tmp/cid.txt") + require.GreaterOrEqual(t, len(withCid), 3) + assert.Equal(t, []string{cmdRun, "--cidfile", "/tmp/cid.txt"}, withCid[:3], + "cidfile must be inserted right after 'run' on the direct-exec path, got: %v", withCid) +} + +// TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap verifies the +// resolution-failure fallback for a user-supplied `docker run`: the spawn must +// shell-wrap bare `docker` (preserving login-shell PATH resolution as a last +// resort) and the cidfile is inserted via the string-based helper. +func TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { + return "", fmt.Errorf("docker not found") + } + + c := newUserSuppliedDockerTestClient() + argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env) + + cmd, shellArgs, shellWrapped := c.resolveDockerSpawn(argsToWrap) + + require.True(t, shellWrapped, "on resolution failure the user docker spawn must be shell-wrapped") + assert.NotEqual(t, cmdDocker, cmd, "shell-wrap fallback exec's the login shell, not bare 'docker'") + require.NotEmpty(t, shellArgs) + cmdStr := shellArgs[len(shellArgs)-1] + assert.True(t, strings.HasPrefix(cmdStr, "docker run"), + "on resolution failure the spawn must fall back to bare 'docker', got: %s", cmdStr) + // The real command string still carries the actual env value (redaction is a + // log-only concern, not a spawn concern). + assert.Contains(t, cmdStr, "-e SLACK_TOKEN=xoxb-secret", + "injected -e env flag must survive into the shell command string, got: %s", cmdStr) +} + // TestDockerDaemonEnvGuaranteed unit-tests the gate directly. func TestDockerDaemonEnvGuaranteed(t *testing.T) { t.Run("darwin is always guaranteed (startup hydration)", func(t *testing.T) { diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index 095d0ed5..94faa6c3 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -157,11 +157,18 @@ func (c *Client) connectStdio(ctx context.Context) error { finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } - } else { - // For direct docker commands, inject env vars as -e flags before shell wrapping + } else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun { + // USER-SUPPLIED `docker run …` upstream (config.Command IS `docker`). + // Inject env vars as -e flags, then reuse the SAME resolve→spawn decision + // as the isolation path (resolveDockerSpawn) so this path also direct-execs + // the resolved ABSOLUTE docker binary instead of shell-wrapping bare + // `docker` — the GH #696 / MCP-2868 fix. Previously this branch always + // called wrapWithUserShell("docker", …), which failed with + // `command not found: docker` on a default Docker Desktop macOS install. + c.isDockerCommand = true + argsToWrap := args - isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun - if isDirectDockerRun && len(c.config.Env) > 0 { + if len(c.config.Env) > 0 { argsToWrap = c.injectEnvVarsIntoDockerArgs(args, c.config.Env) c.logger.Debug("Injected env vars into direct docker command", zap.String("server", c.config.Name), @@ -169,20 +176,25 @@ func (c *Client) connectStdio(ctx context.Context) error { zap.Strings("modified_args", shellwrap.RedactDockerArgs(argsToWrap))) } - // Use shell wrapping for environment inheritance - // This fixes issues when mcpproxy is launched via Launchd and doesn't inherit - // user's shell environment (like PATH customizations from .bashrc, .zshrc, etc.) - finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, argsToWrap) - c.isDockerCommand = false + var dockerShellWrapped bool + finalCommand, finalArgs, dockerShellWrapped = c.resolveDockerSpawn(argsToWrap) - // Handle explicit docker commands - if isDirectDockerRun { - c.isDockerCommand = true - if cidFile != "" { - // For shell-wrapped Docker commands, we need to modify the shell command string + // Insert --cidfile via the helper that matches how we spawn: args-based on + // the direct-exec path, string-based on the login-shell fallback. + if cidFile != "" { + if dockerShellWrapped { finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) + } else { + finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) } } + } else { + // Plain (non-docker) stdio command. Use shell wrapping for environment + // inheritance. This fixes issues when mcpproxy is launched via Launchd and + // doesn't inherit the user's shell environment (PATH customizations from + // .bashrc, .zshrc, etc.). + finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, args) + c.isDockerCommand = false } // Upstream transport with working directory support and process group management