From 7fe593a2adc3ca9dc9526d0d2d92c7ebbdfe3d09 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 17 Jun 2026 14:36:39 +0300 Subject: [PATCH 1/2] fix(docker): direct-exec resolved docker binary for isolated spawn (MCP-2753) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #697 resolved the docker CLI to an absolute path but still routed the isolated spawn through `$SHELL -l -c " 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 --- docs/docker-isolation.md | 18 +- internal/upstream/core/connection_docker.go | 109 +++++++++-- .../upstream/core/connection_docker_test.go | 176 +++++++++++++++--- internal/upstream/core/connection_launcher.go | 9 +- internal/upstream/core/connection_stdio.go | 16 +- 5 files changed, 269 insertions(+), 59 deletions(-) diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index 550a23c16..b40bf07d1 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -251,11 +251,19 @@ docker stats it is **not** on a standard `PATH` dir unless you ran the optional, 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** before - spawning an isolated server (and also adds the bundle bin dir to the enhanced - spawn `PATH`), so isolation works even without the CLI-tools step. If you - still see this error, confirm the binary exists at the bundle path above, or - run Docker Desktop's "install CLI tools". +- 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 + 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 — + so the error persisted; direct exec fixes that. Direct exec is used only when + the resolved value is a verified absolute executable; a non-absolute result + (e.g. a shell function/alias from `command -v docker`) falls back to the + login-shell wrap of bare `docker`. If you still see this error, confirm the + binary exists at the bundle path above, or run Docker Desktop's "install CLI + tools". - `upstream_servers list` reports `docker_status.docker_path` (the resolved binary) and reports `docker_status.available` / per-server `docker_available` as `true` **only** when the CLI is actually resolvable *and* `docker info` diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index e2d882b45..d204bf7bc 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -2,6 +2,9 @@ package core import ( "fmt" + "os" + "path/filepath" + "runtime" "strings" "github.com/smart-mcp-proxy/mcpproxy-go/internal/shellwrap" @@ -15,8 +18,12 @@ import ( var resolveDockerBinary = shellwrap.ResolveDockerPath // setupDockerIsolation configures Docker isolation for the MCP server process. -// Returns the docker command and arguments to execute. -func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string) { +// Returns the docker command, arguments, and whether the returned command was +// wrapped in the user's login shell. 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. +func (c *Client) setupDockerIsolation(command string, args []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) { // Detect the runtime type from the command runtimeType := c.isolationManager.DetectRuntimeType(command) c.logger.Debug("Detected runtime type for Docker isolation", @@ -30,7 +37,8 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm c.logger.Error("Failed to build Docker args, falling back to shell wrapping", zap.String("server", c.config.Name), zap.Error(err)) - return c.wrapWithUserShell(command, args) + shellCmd, shellArgs := c.wrapWithUserShell(command, args) + return shellCmd, shellArgs, true } // Extract container name from Docker args for tracking @@ -61,27 +69,90 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm zap.String("container_command", containerCommand)) } - // CRITICAL FIX (#696): resolve `docker` to an ABSOLUTE path before shell-wrapping. - // 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. Invoking docker by absolute path — mirroring - // newDockerCmd — bypasses PATH entirely so isolated servers spawn successfully. - // Fall back to the bare "docker" command only when resolution fails. + // 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. // - // We still wrap in the user login shell so env-var inheritance and the - // existing cidfile insertion (which scans the command string for "docker run") - // keep working; the trailing "docker run" substring matches the absolute path. - dockerBin := cmdDocker - if resolved, resErr := resolveDockerBinary(c.logger); resErr == nil && resolved != "" { - dockerBin = resolved + // #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. The spawn env already carries the enhanced PATH + // and the hydrated DOCKER_* daemon vars (BuildSecureEnvironment with + // EnhancePath=true, after the startup login-shell hydration in MCP-2751), so + // no login shell is needed for env inheritance either. + // + // We only direct-exec a VERIFIED absolute executable. 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". For any + // non-absolute / non-executable result we fall back to a login-shell wrap of + // bare "docker", giving the interactive PATH a last-resort chance to find it. + if resolved, resErr := resolveDockerBinary(c.logger); resErr == nil && isDirectExecutable(resolved) { + return resolved, finalArgs, false } else if resErr != nil { - c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' (isolated server may fail if docker is not on the spawn PATH)", + c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' via login shell (isolated server may fail if docker is not on the spawn PATH)", zap.String("server", c.config.Name), zap.Error(resErr)) + } else if resolved != "" { + c.logger.Warn("Resolved docker value is not a verified absolute executable; falling back to bare 'docker' via login shell", + zap.String("server", c.config.Name), + zap.String("resolved", resolved)) + } + shellCmd, shellArgs := c.wrapWithUserShell(cmdDocker, finalArgs) + return shellCmd, shellArgs, true +} + +// isDirectExecutable reports whether path is safe to hand to exec directly +// (without a login-shell wrap): it must be an ABSOLUTE path to an existing, +// non-directory file that is executable. This is the Codex round-3 guard for +// MCP-2753 — a resolved value such as a shell function/alias name or a bare +// command name (which `command -v docker` can emit) is rejected so it is +// shell-wrapped instead of failing a direct exec. +func isDirectExecutable(path string) bool { + if path == "" || !filepath.IsAbs(path) { + return false + } + info, err := os.Stat(path) + if err != nil || info.IsDir() { + return false + } + if runtime.GOOS == "windows" { + // Windows file mode bits don't convey the executable flag; an absolute + // path to a regular file is the strongest portable signal here. + return true + } + return info.Mode().Perm()&0o111 != 0 +} + +// insertCidfileIntoDockerArgs inserts "--cidfile " immediately after the +// "run" token in a direct-exec docker argument slice (no shell wrap). It is the +// args-based sibling of insertCidfileIntoShellDockerCommand used on the +// direct-exec spawn path (MCP-2753). Operating on argv (rather than a shell +// command string) makes it platform-agnostic and sidesteps the -c vs /c shell +// quirk #697 had to patch. +func (c *Client) insertCidfileIntoDockerArgs(args []string, cidFile string) []string { + if len(args) == 0 || args[0] != cmdRun { + c.logger.Warn("Could not find 'run' as the first docker arg for cidfile insertion - container ID tracking may be limited", + zap.String("server", c.config.Name), + zap.Strings("args", args)) + return args } - return c.wrapWithUserShell(dockerBin, finalArgs) + + newArgs := make([]string, 0, len(args)+2) + newArgs = append(newArgs, args[0]) // "run" + newArgs = append(newArgs, "--cidfile", cidFile) + newArgs = append(newArgs, args[1:]...) + + c.logger.Debug("Inserted cidfile into direct-exec docker args", + zap.String("server", c.config.Name), + zap.String("cid_file", cidFile)) + return newArgs } // injectEnvVarsIntoDockerArgs injects environment variables as -e flags into Docker run args diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index 42ff2d0a1..f6e1e4a78 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -2,6 +2,9 @@ package core import ( "fmt" + "os" + "path/filepath" + "runtime" "strings" "testing" @@ -24,68 +27,181 @@ func newIsolatedTestClient() *Client { } } -// TestSetupDockerIsolationUsesResolvedAbsolutePath verifies the root-cause fix -// for #696: an isolated server is spawned with docker invoked by its resolved -// ABSOLUTE path (bypassing PATH), not the bare "docker" command that the -// login-shell PATH may be unable to resolve. +// writeFakeDockerExecutable writes a real, executable file under a fresh temp +// dir and returns its absolute path. The direct-exec guard (MCP-2753) only +// trusts a resolved value that is an ABSOLUTE path to an actual executable, so +// tests that exercise the direct-exec branch must point at a binary that +// genuinely exists with the exec bit set — a string that merely *looks* like a +// path is (correctly) rejected and shell-wrapped instead. +func writeFakeDockerExecutable(t *testing.T) string { + t.Helper() + dir := t.TempDir() + name := "docker" + if runtime.GOOS == "windows" { + name = "docker.exe" + } + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("write fake docker: %v", err) + } + return p +} + +// TestSetupDockerIsolationExecsResolvedBinaryDirectly is the root-cause +// assertion for MCP-2753: on successful resolution to a verified absolute +// executable, the isolated server must be spawned by exec'ing the resolved +// docker binary DIRECTLY — no `$SHELL -l -c " run …"` indirection. A +// login shell re-derives PATH from rc files and can drop the Docker Desktop +// bundle dir, so a shell-wrapped absolute path is only a token whose lookup the +// shell can still override. Exec'ing the absolute path bypasses PATH entirely. +func TestSetupDockerIsolationExecsResolvedBinaryDirectly(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + + assert.False(t, shellWrapped, "verified absolute executable must NOT shell-wrap the spawn") + assert.Equal(t, fakeDocker, cmd, + "docker must be exec'd by its resolved absolute path directly, got command: %s", cmd) + require.NotEmpty(t, args, "docker args must not be empty") + assert.Equal(t, cmdRun, args[0], + "first docker arg must be 'run' (raw argv, not a shell -c string), got: %v", args) + // No element may be a single shell command string wrapping the whole command. + for _, a := range args { + assert.NotContains(t, a, " run ", + "args must be raw argv tokens, not a single shell command string, got: %v", args) + } +} + +// TestSetupDockerIsolationUsesResolvedAbsolutePath verifies the resolved +// ABSOLUTE path is used as the exec command (bypassing PATH), not the bare +// "docker" command that the login-shell PATH may be unable to resolve. func TestSetupDockerIsolationUsesResolvedAbsolutePath(t *testing.T) { - const fakeDocker = "/opt/fake/Docker.app/Contents/Resources/bin/docker" + fakeDocker := writeFakeDockerExecutable(t) orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - shellCmd, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) + cmd, _, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) - require.NotEmpty(t, shellCmd, "shell command must not be empty") - require.NotEmpty(t, shellArgs, "shell args must not be empty") + assert.False(t, shellWrapped) + assert.Equal(t, fakeDocker, cmd, + "docker must be invoked by its resolved absolute path, got: %s", cmd) + assert.NotEqual(t, cmdDocker, cmd, + "docker must not be invoked as the bare 'docker' command, got: %s", cmd) +} - // The shell-wrapped command string is the last shell arg. +// TestSetupDockerIsolationShellWrapsNonAbsoluteResolved is the Codex round-3 +// P2 #1 guard: 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 an absolute path. Direct-exec'ing such a value would +// fail with "no such file or directory", so a non-absolute resolved value MUST +// fall back to the login-shell wrap (which can still resolve it interactively). +func TestSetupDockerIsolationShellWrapsNonAbsoluteResolved(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + // e.g. a shell builtin/alias/function name, or a bare command name. + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return "docker", nil } + + c := newIsolatedTestClient() + cmd, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.True(t, shellWrapped, "a non-absolute resolved value must be shell-wrapped, not direct-exec'd") + assert.NotEqual(t, "docker", cmd, "must not direct-exec a bare command name") + require.NotEmpty(t, shellArgs) cmdStr := shellArgs[len(shellArgs)-1] - assert.Contains(t, cmdStr, fakeDocker, - "docker must be invoked by its resolved absolute path, got: %s", cmdStr) - assert.False(t, strings.HasPrefix(cmdStr, "docker run"), - "docker must not be invoked as the bare 'docker' command, got: %s", cmdStr) + assert.True(t, strings.HasPrefix(cmdStr, "docker run"), + "shell fallback must wrap the bare 'docker' command, got: %s", cmdStr) +} + +// TestSetupDockerIsolationShellWrapsNonExecutableResolved is the other half of +// the P2 #1 guard: an absolute path that does not exist (or is not executable) +// must not be direct-exec'd — it falls back to the login-shell wrap. +func TestSetupDockerIsolationShellWrapsNonExecutableResolved(t *testing.T) { + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { + return "/no/such/path/docker", nil + } + + c := newIsolatedTestClient() + _, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.True(t, shellWrapped, "a non-existent absolute path must be shell-wrapped, not direct-exec'd") + require.NotEmpty(t, shellArgs) + cmdStr := shellArgs[len(shellArgs)-1] + assert.True(t, strings.HasPrefix(cmdStr, "docker run"), + "shell fallback must wrap the bare 'docker' command, got: %s", cmdStr) } // TestSetupDockerIsolationCidfileInsertionWithAbsolutePath verifies the -// existing cidfile-insertion logic still matches when docker is referenced by -// its absolute path (the trailing "docker run" substring is preserved). +// direct-exec cidfile-insertion logic inserts --cidfile right after the "run" +// token in the raw docker argv. func TestSetupDockerIsolationCidfileInsertionWithAbsolutePath(t *testing.T) { - const fakeDocker = "/opt/fake/Docker.app/Contents/Resources/bin/docker" + fakeDocker := writeFakeDockerExecutable(t) orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } c := newIsolatedTestClient() - _, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) - withCid := c.insertCidfileIntoShellDockerCommand(shellArgs, "/tmp/cid.txt") + cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + require.False(t, shellWrapped) + require.Equal(t, fakeDocker, cmd) + + 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 the 'run' token, got: %v", withCid) +} - cmdStr := withCid[len(withCid)-1] - assert.Contains(t, cmdStr, fakeDocker+" run --cidfile /tmp/cid.txt", - "cidfile must be inserted right after the absolute-path docker run, got: %s", cmdStr) +// TestInsertCidfileIntoDockerArgs unit-tests the args-based cidfile helper used +// on the direct-exec spawn path. It is platform-agnostic (operates on argv), +// sidestepping the -c vs /c shell quirk the shell-wrapped path had to handle. +func TestInsertCidfileIntoDockerArgs(t *testing.T) { + c := newIsolatedTestClient() + + t.Run("inserts after run", func(t *testing.T) { + args := []string{"run", "-i", "--rm", "mcp/duckduckgo"} + got := c.insertCidfileIntoDockerArgs(args, "/tmp/cid.txt") + assert.Equal(t, []string{"run", "--cidfile", "/tmp/cid.txt", "-i", "--rm", "mcp/duckduckgo"}, got) + }) + + t.Run("no-op when first arg is not run", func(t *testing.T) { + args := []string{"version"} + got := c.insertCidfileIntoDockerArgs(args, "/tmp/cid.txt") + assert.Equal(t, args, got, "must not mutate args that don't start with 'run'") + }) + + t.Run("no-op on empty args", func(t *testing.T) { + got := c.insertCidfileIntoDockerArgs(nil, "/tmp/cid.txt") + assert.Empty(t, got) + }) } // TestInsertCidfileWindowsCmdFormat verifies cidfile insertion works with the // Windows cmd.exe shell-wrap format: ["/c", "docker run …"] (second-to-last -// arg is "/c", not "-c"). This is a cross-platform unit test for the guard -// that previously rejected the /c flag as an unrecognised format. +// arg is "/c", not "-c"). This is the shell-fallback path's helper. func TestInsertCidfileWindowsCmdFormat(t *testing.T) { - const fakeDocker = "/opt/fake/bin/docker" c := newIsolatedTestClient() // Simulate Windows cmd.exe output: shell returns ["/c", cmdStr] - windowsShellArgs := []string{"/c", fakeDocker + " run --rm -i ghcr.io/some/image python -m srv"} + windowsShellArgs := []string{"/c", "docker run --rm -i ghcr.io/some/image python -m srv"} withCid := c.insertCidfileIntoShellDockerCommand(windowsShellArgs, "/tmp/cid.txt") cmdStr := withCid[len(withCid)-1] - assert.Contains(t, cmdStr, fakeDocker+" run --cidfile /tmp/cid.txt", + assert.Contains(t, cmdStr, "docker run --cidfile /tmp/cid.txt", "cidfile must be inserted in Windows cmd /c format too, got: %s", cmdStr) } // TestSetupDockerIsolationFallsBackToBareDocker verifies that when docker -// cannot be resolved to an absolute path, the spawn falls back to the bare -// "docker" command (preserving prior behavior / login-shell resolution). +// cannot be resolved to an absolute path, the spawn falls back to a login-shell +// wrap of the bare "docker" command (preserving prior behavior / login-shell +// PATH resolution as a last resort). func TestSetupDockerIsolationFallsBackToBareDocker(t *testing.T) { orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) @@ -94,8 +210,10 @@ func TestSetupDockerIsolationFallsBackToBareDocker(t *testing.T) { } c := newIsolatedTestClient() - _, shellArgs := c.setupDockerIsolation(c.config.Command, c.config.Args) + _, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + require.True(t, shellWrapped, "on resolution failure the spawn must be shell-wrapped") + 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) diff --git a/internal/upstream/core/connection_launcher.go b/internal/upstream/core/connection_launcher.go index 0d3807e55..898cd1630 100644 --- a/internal/upstream/core/connection_launcher.go +++ b/internal/upstream/core/connection_launcher.go @@ -247,9 +247,14 @@ func (c *Client) buildLauncherCmd(_ context.Context, willUseDocker bool) (*exec. var finalArgs []string if c.isolationManager != nil && c.isolationManager.ShouldIsolate(c.config) { - finalCommand, finalArgs = c.setupDockerIsolation(c.config.Command, args) + var dockerShellWrapped bool + finalCommand, finalArgs, dockerShellWrapped = c.setupDockerIsolation(c.config.Command, args) if cidFile != "" { - finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) + if dockerShellWrapped { + finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) + } else { + finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) + } } } else { argsToWrap := args diff --git a/internal/upstream/core/connection_stdio.go b/internal/upstream/core/connection_stdio.go index de8daa01f..bf7594a38 100644 --- a/internal/upstream/core/connection_stdio.go +++ b/internal/upstream/core/connection_stdio.go @@ -141,13 +141,21 @@ func (c *Client) connectStdio(ctx context.Context) error { zap.String("server", c.config.Name), zap.String("original_command", c.config.Command)) - // Use Docker isolation (now shell-wrapped for PATH inheritance) - finalCommand, finalArgs = c.setupDockerIsolation(c.config.Command, args) + // Use Docker isolation. On resolution to a verified absolute executable + // this direct-execs the docker binary (no shell wrap); otherwise it falls + // back to a login-shell wrap. dockerShellWrapped selects the matching + // cidfile helper. + var dockerShellWrapped bool + finalCommand, finalArgs, dockerShellWrapped = c.setupDockerIsolation(c.config.Command, args) c.isDockerCommand = true - // Add cidfile to shell-wrapped Docker command if we have one + // Add cidfile to the Docker command if we have one if cidFile != "" { - finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) + if dockerShellWrapped { + finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile) + } else { + finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile) + } } } else { // For direct docker commands, inject env vars as -e flags before shell wrapping From 04dbe4b085132385739640ea8a84b0ff8339a74d Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Wed, 17 Jun 2026 15:38:13 +0300 Subject: [PATCH 2/2] fix(docker): gate direct-exec on guaranteed daemon env (non-Darwin rootless regression) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/docker-isolation.md | 15 ++- internal/upstream/core/connection_docker.go | 84 ++++++++++++--- .../upstream/core/connection_docker_test.go | 102 ++++++++++++++++++ 3 files changed, 180 insertions(+), 21 deletions(-) diff --git a/docs/docker-isolation.md b/docs/docker-isolation.md index b40bf07d1..1c95e62db 100644 --- a/docs/docker-isolation.md +++ b/docs/docker-isolation.md @@ -259,11 +259,16 @@ docker stats 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 — so the error persisted; direct exec fixes that. Direct exec is used only when - the resolved value is a verified absolute executable; a non-absolute result - (e.g. a shell function/alias from `command -v docker`) falls back to the - login-shell wrap of bare `docker`. If you still see this error, confirm the - binary exists at the bundle path above, or run Docker Desktop's "install CLI - tools". + (a) the resolved value is a verified absolute executable and (b) the docker + daemon-config env is guaranteed without the login shell — on macOS via the + startup login-shell hydration, or on any platform when `DOCKER_HOST` / + `DOCKER_CONTEXT` are already exported into mcpproxy's environment. A + non-absolute result (e.g. a shell function/alias from `command -v docker`), or + a rootless/remote daemon on Linux whose `DOCKER_HOST` lives only in the + login-shell rc, falls back to the `$SHELL -l` wrap (still using the resolved + absolute path when one was found) so `docker run` keeps inheriting the daemon + config. If you still see this error, confirm the binary exists at the bundle + path above, or run Docker Desktop's "install CLI tools". - `upstream_servers list` reports `docker_status.docker_path` (the resolved binary) and reports `docker_status.available` / per-server `docker_available` as `true` **only** when the CLI is actually resolvable *and* `docker info` diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index d204bf7bc..0e53de8f6 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -82,29 +82,57 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm // 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. The spawn env already carries the enhanced PATH - // and the hydrated DOCKER_* daemon vars (BuildSecureEnvironment with - // EnhancePath=true, after the startup login-shell hydration in MCP-2751), so - // no login shell is needed for env inheritance either. + // bypasses PATH entirely. // - // We only direct-exec a VERIFIED absolute executable. 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". For any - // non-absolute / non-executable result we fall back to a login-shell wrap of - // bare "docker", giving the interactive PATH a last-resort chance to find it. - if resolved, resErr := resolveDockerBinary(c.logger); resErr == nil && isDirectExecutable(resolved) { + // 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. + resolved, resErr := resolveDockerBinary(c.logger) + switch { + case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed(): return resolved, finalArgs, false - } else if resErr != nil { + case resErr != nil: c.logger.Warn("Could not resolve docker to an absolute path; falling back to bare 'docker' via login shell (isolated server may fail if docker is not on the spawn PATH)", zap.String("server", c.config.Name), zap.Error(resErr)) - } else if resolved != "" { - c.logger.Warn("Resolved docker value is not a verified absolute executable; falling back to bare 'docker' via login shell", + case !isDirectExecutable(resolved): + c.logger.Warn("Resolved docker value is not a verified absolute executable; falling back to login-shell wrap", zap.String("server", c.config.Name), zap.String("resolved", resolved)) + default: + // Verified absolute executable, but the daemon env is not guaranteed + // without the login shell (non-Darwin, no DOCKER_HOST/DOCKER_CONTEXT in + // os.Environ()). Keep the shell wrap so rc-file DOCKER_* are inherited. + c.logger.Debug("docker daemon env not guaranteed in process env; keeping login-shell wrap so rc-file DOCKER_* are inherited", + zap.String("server", c.config.Name), + zap.String("resolved", resolved)) + } + + dockerBin := cmdDocker + if isDirectExecutable(resolved) { + dockerBin = resolved } - shellCmd, shellArgs := c.wrapWithUserShell(cmdDocker, finalArgs) + shellCmd, shellArgs := c.wrapWithUserShell(dockerBin, finalArgs) return shellCmd, shellArgs, true } @@ -122,7 +150,7 @@ func isDirectExecutable(path string) bool { if err != nil || info.IsDir() { return false } - if runtime.GOOS == "windows" { + if runtime.GOOS == osWindows { // Windows file mode bits don't convey the executable flag; an absolute // path to a regular file is the strongest portable signal here. return true @@ -130,6 +158,30 @@ func isDirectExecutable(path string) bool { return info.Mode().Perm()&0o111 != 0 } +// dockerDaemonEnvGOOS mirrors runtime.GOOS; a package var so tests can exercise +// the Darwin branch on a non-Darwin CI host. +var dockerDaemonEnvGOOS = runtime.GOOS + +// dockerDaemonEnvGuaranteed reports whether the docker daemon-config env +// (DOCKER_HOST/DOCKER_CONTEXT) is guaranteed to reach a direct-exec'd docker +// process WITHOUT a login-shell wrap. +// +// On macOS the startup login-shell hydration (shellwrap.HydrateFromLoginShell, +// MCP-2751) captures DOCKER_* into os.Environ() — its gate fires whenever a +// docker var is missing — so the secureenv allow-list forwards them and the +// shell wrap is unnecessary. That hydration is Darwin-only, so on other +// platforms direct-exec is only safe to skip the shell wrap when DOCKER_HOST or +// DOCKER_CONTEXT are ALREADY exported into mcpproxy's own environment. When they +// are configured only in the user's login-shell rc (Codex's non-Darwin rootless +// regression on PR #703), we must keep the `$SHELL -l` wrap so `docker run` +// inherits them. +func dockerDaemonEnvGuaranteed() bool { + if dockerDaemonEnvGOOS == osDarwin { + return true + } + return os.Getenv("DOCKER_HOST") != "" || os.Getenv("DOCKER_CONTEXT") != "" +} + // insertCidfileIntoDockerArgs inserts "--cidfile " immediately after the // "run" token in a direct-exec docker argument slice (no shell wrap). It is the // args-based sibling of insertCidfileIntoShellDockerCommand used on the diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index f6e1e4a78..27b14227d 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -27,6 +27,16 @@ func newIsolatedTestClient() *Client { } } +// 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. +func forceDockerDaemonEnvGOOS(t *testing.T, goos string) { + t.Helper() + orig := dockerDaemonEnvGOOS + t.Cleanup(func() { dockerDaemonEnvGOOS = orig }) + dockerDaemonEnvGOOS = goos +} + // writeFakeDockerExecutable writes a real, executable file under a fresh temp // dir and returns its absolute path. The direct-exec guard (MCP-2753) only // trusts a resolved value that is an ABSOLUTE path to an actual executable, so @@ -56,6 +66,7 @@ func writeFakeDockerExecutable(t *testing.T) string { // shell can still override. Exec'ing the absolute path bypasses PATH entirely. func TestSetupDockerIsolationExecsResolvedBinaryDirectly(t *testing.T) { fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, osDarwin) // daemon env guaranteed via startup hydration orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) @@ -82,6 +93,7 @@ func TestSetupDockerIsolationExecsResolvedBinaryDirectly(t *testing.T) { // "docker" command that the login-shell PATH may be unable to resolve. func TestSetupDockerIsolationUsesResolvedAbsolutePath(t *testing.T) { fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, osDarwin) orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) @@ -145,6 +157,7 @@ func TestSetupDockerIsolationShellWrapsNonExecutableResolved(t *testing.T) { // token in the raw docker argv. func TestSetupDockerIsolationCidfileInsertionWithAbsolutePath(t *testing.T) { fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, osDarwin) orig := resolveDockerBinary t.Cleanup(func() { resolveDockerBinary = orig }) @@ -218,3 +231,92 @@ func TestSetupDockerIsolationFallsBackToBareDocker(t *testing.T) { assert.True(t, strings.HasPrefix(cmdStr, "docker run"), "on resolution failure the spawn must fall back to bare 'docker', got: %s", cmdStr) } + +// TestSetupDockerIsolationShellWrapsWhenDaemonEnvMissingNonDarwin is the +// Codex round-4 (PR #703) regression guard. On non-Darwin hosts the startup +// login-shell hydration (MCP-2751) does NOT run, so a rootless/remote daemon +// whose DOCKER_HOST is exported only by the login-shell rc (.profile) is not in +// mcpproxy's os.Environ(). Direct-exec drops the `$SHELL -l` wrap and would lose +// that daemon config — the exact DOCKER_* loss #699 kept the shell wrap to +// avoid. So even with a verified absolute docker binary, the spawn must stay +// shell-wrapped (which sources the rc and recovers DOCKER_HOST), and it must +// wrap the resolved ABSOLUTE path (not bare 'docker'). +func TestSetupDockerIsolationShellWrapsWhenDaemonEnvMissingNonDarwin(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "") // not exported into mcpproxy's own env + t.Setenv("DOCKER_CONTEXT", "") // only present in the user's login-shell rc + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + cmd, shellArgs, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + + require.True(t, shellWrapped, + "non-Darwin with no DOCKER_HOST in env must keep the login-shell wrap to inherit rc-file DOCKER_*") + require.NotEmpty(t, shellArgs) + cmdStr := shellArgs[len(shellArgs)-1] + assert.Contains(t, cmdStr, fakeDocker, + "shell fallback should still use the resolved absolute path, got: %s", cmdStr) + assert.False(t, strings.HasPrefix(cmdStr, "docker run"), + "shell fallback must not degrade to bare 'docker' when an absolute path resolved, got: %s", cmdStr) + // Sanity: the wrapped command must reference the absolute path, not be the + // raw binary handed to direct exec. + assert.NotEqual(t, fakeDocker, cmd) +} + +// TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv proves the non-Darwin +// happy path: when DOCKER_HOST is already exported into mcpproxy's environment, +// the daemon config is guaranteed without the login shell, so the verified +// absolute binary is direct-exec'd (no shell wrap) even off macOS. +func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) { + fakeDocker := writeFakeDockerExecutable(t) + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "tcp://127.0.0.1:2375") + + orig := resolveDockerBinary + t.Cleanup(func() { resolveDockerBinary = orig }) + resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil } + + c := newIsolatedTestClient() + cmd, args, shellWrapped := c.setupDockerIsolation(c.config.Command, c.config.Args) + + assert.False(t, shellWrapped, + "DOCKER_HOST in os.Environ() guarantees daemon config — direct-exec is safe off macOS") + assert.Equal(t, fakeDocker, cmd) + require.NotEmpty(t, args) + assert.Equal(t, cmdRun, args[0]) +} + +// TestDockerDaemonEnvGuaranteed unit-tests the gate directly. +func TestDockerDaemonEnvGuaranteed(t *testing.T) { + t.Run("darwin is always guaranteed (startup hydration)", func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, osDarwin) + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "") + assert.True(t, dockerDaemonEnvGuaranteed()) + }) + + t.Run("non-darwin without DOCKER_HOST/DOCKER_CONTEXT is not guaranteed", func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "") + assert.False(t, dockerDaemonEnvGuaranteed()) + }) + + t.Run("non-darwin with DOCKER_HOST is guaranteed", func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "tcp://127.0.0.1:2375") + t.Setenv("DOCKER_CONTEXT", "") + assert.True(t, dockerDaemonEnvGuaranteed()) + }) + + t.Run("non-darwin with DOCKER_CONTEXT is guaranteed", func(t *testing.T) { + forceDockerDaemonEnvGOOS(t, "linux") + t.Setenv("DOCKER_HOST", "") + t.Setenv("DOCKER_CONTEXT", "colima") + assert.True(t, dockerDaemonEnvGuaranteed()) + }) +}