Skip to content

Commit 5cc9eee

Browse files
DumbrisDumbris
authored
fix(upstream): direct-exec resolved docker binary for user-supplied docker run (#714)
User-supplied `docker run …` upstreams (config command IS `docker`) resolved the bundled Docker Desktop CLI for the status field but still shell-wrapped bare `docker` via `$SHELL -l -c "docker run …"` at spawn. On a default Docker Desktop macOS install the CLI lives only inside the app bundle and is not on the login-shell PATH, so the spawn failed with `zsh:1: command not found: docker` (GH #696, still broken on v0.41.0). PR #703's direct-exec fix covered only the isolation-injection path (setupDockerIsolation). Extract its resolve→spawn-decision logic into a shared `resolveDockerSpawn` method and reuse it on the user-supplied `docker run` branch in connectStdio, inserting --cidfile via the helper that matches the spawn shape (args-based on direct-exec, string-based on the login-shell fallback) and keeping the existing -e env injection. setupDockerIsolation behavior is unchanged (now delegates to the shared method). Docs updated to note direct-exec now covers both Docker spawn paths. Related #696 Related #712
1 parent eba4a85 commit 5cc9eee

4 files changed

Lines changed: 188 additions & 55 deletions

File tree

‎docs/docker-isolation.md‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -252,9 +252,11 @@ docker stats
252252
admin-gated "install CLI tools" step. When mcpproxy is launched from a
253253
LaunchAgent / tray, the captured login-shell `PATH` may omit this directory.
254254
- mcpproxy resolves the `docker` binary to its **absolute path** and then
255-
**exec's it directly** (no login-shell wrap) when spawning an isolated server,
256-
so the spawn bypasses `PATH` entirely and works even without the CLI-tools
257-
step. (The enhanced spawn `PATH` still includes the bundle bin dir as a
255+
**exec's it directly** (no login-shell wrap) when spawning a Docker upstream —
256+
both servers that mcpproxy *isolates* into `docker run` (uvx/npx) and upstreams
257+
whose config `command` **is** `docker` (a user-supplied `docker run …`) — so
258+
the spawn bypasses `PATH` entirely and works even without the CLI-tools step.
259+
(The enhanced spawn `PATH` still includes the bundle bin dir as a
258260
belt-and-suspenders measure.) Earlier builds resolved the absolute path but
259261
still routed the spawn through `$SHELL -l -c "<docker> run …"`, where the
260262
login shell re-derived `PATH` from rc files and could drop the bundle dir —

‎internal/upstream/core/connection_docker.go‎

Lines changed: 65 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -69,44 +69,71 @@ func (c *Client) setupDockerIsolation(command string, args []string) (dockerComm
6969
zap.String("container_command", containerCommand))
7070
}
7171

72-
// CRITICAL FIX (#696 / MCP-2744 / MCP-2753): resolve `docker` to an ABSOLUTE
73-
// path and exec it DIRECTLY — no login-shell wrap. Docker Desktop installed
74-
// the default way on macOS (without the optional, admin-gated "install CLI
75-
// tools" step) leaves the docker CLI only inside the app bundle at
76-
// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on
77-
// any standard PATH dir nor on the (often unreliable) login-shell PATH a
78-
// LaunchAgent captures.
79-
//
80-
// #697 resolved the absolute path but still routed the spawn through
81-
// `$SHELL -l -c "<docker> run …"`. There the absolute path is only a token
82-
// inside the -c string: the login shell re-derives $PATH from rc files and
83-
// can drop the bundle dir, so the bug persisted. Exec'ing the absolute path
84-
// directly (mirroring newDockerCmd's exec.CommandContext(dockerBin, …))
85-
// bypasses PATH entirely.
86-
//
87-
// Two preconditions gate the direct-exec:
88-
//
89-
// 1. The resolved value must be a VERIFIED absolute executable
90-
// (isDirectExecutable). ResolveDockerPath's last resort runs
91-
// `command -v docker` in the login shell, which can emit a shell
92-
// function name, an alias, or a bare command name rather than a real
93-
// binary path; direct-exec'ing that would fail with "no such file".
94-
//
95-
// 2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be
96-
// guaranteed in the spawn env WITHOUT the login shell
97-
// (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops
98-
// rc-file env inheritance. On macOS the startup hydration (MCP-2751)
99-
// already captured DOCKER_* from the login shell into os.Environ(), and
100-
// secureenv forwards them — but that hydration is Darwin-only. On Linux
101-
// a rootless/remote daemon whose DOCKER_HOST lives only in .profile would
102-
// be lost by direct-exec (the regression #699 kept the shell wrap to
103-
// avoid). So on non-Darwin we only direct-exec when DOCKER_HOST/
104-
// DOCKER_CONTEXT are already in os.Environ(); otherwise we keep the
105-
// login-shell wrap so `docker run` still finds the daemon.
106-
//
107-
// When we fall back to the shell wrap we still prefer the resolved absolute
108-
// path as the wrapped command (it sidesteps the login shell's PATH
109-
// re-derivation), dropping to bare "docker" only when resolution failed.
72+
// Resolve `docker` to an absolute path and decide between a direct-exec and a
73+
// login-shell wrap. Shared with the user-supplied `docker run` path
74+
// (connectStdio) via resolveDockerSpawn so both spawn paths behave identically
75+
// (#696 / MCP-2868).
76+
return c.resolveDockerSpawn(finalArgs)
77+
}
78+
79+
// resolveDockerSpawn decides how to spawn `docker` given the fully built docker
80+
// run argv (finalArgs, beginning with the "run" token). It resolves `docker` to
81+
// an absolute path and, when safe, returns that absolute path for a DIRECT exec
82+
// (shellWrapped == false); otherwise it returns a login-shell wrap
83+
// (shellWrapped == true). It is shared by BOTH docker spawn paths:
84+
//
85+
// - setupDockerIsolation — uvx/npx servers wrapped INTO `docker run` by the
86+
// isolation manager (isolation-injection path).
87+
// - connectStdio's user-supplied `docker run` branch — upstreams whose config
88+
// Command IS `docker` (GH #696 / MCP-2868). Before this extraction that path
89+
// shell-wrapped bare `docker`, so on a default Docker Desktop macOS install
90+
// (docker CLI only inside the app bundle, not on the login-shell PATH) it
91+
// failed with `zsh:1: command not found: docker`.
92+
//
93+
// shellWrapped governs how the caller inserts --cidfile: a direct-exec spawn
94+
// (shellWrapped == false) uses the args-based insertCidfileIntoDockerArgs, while
95+
// the login-shell fallback (shellWrapped == true) uses the string-based
96+
// insertCidfileIntoShellDockerCommand.
97+
//
98+
// CRITICAL FIX (#696 / MCP-2744 / MCP-2753 / MCP-2868): resolve `docker` to an
99+
// ABSOLUTE path and exec it DIRECTLY — no login-shell wrap. Docker Desktop
100+
// installed the default way on macOS (without the optional, admin-gated "install
101+
// CLI tools" step) leaves the docker CLI only inside the app bundle at
102+
// /Applications/Docker.app/Contents/Resources/bin/docker, which is NOT on any
103+
// standard PATH dir nor on the (often unreliable) login-shell PATH a LaunchAgent
104+
// captures.
105+
//
106+
// #697 resolved the absolute path but still routed the spawn through
107+
// `$SHELL -l -c "<docker> run …"`. There the absolute path is only a token
108+
// inside the -c string: the login shell re-derives $PATH from rc files and can
109+
// drop the bundle dir, so the bug persisted. Exec'ing the absolute path directly
110+
// (mirroring newDockerCmd's exec.CommandContext(dockerBin, …)) bypasses PATH
111+
// entirely.
112+
//
113+
// Two preconditions gate the direct-exec:
114+
//
115+
// 1. The resolved value must be a VERIFIED absolute executable
116+
// (isDirectExecutable). ResolveDockerPath's last resort runs
117+
// `command -v docker` in the login shell, which can emit a shell function
118+
// name, an alias, or a bare command name rather than a real binary path;
119+
// direct-exec'ing that would fail with "no such file".
120+
//
121+
// 2. The docker daemon-config env (DOCKER_HOST/DOCKER_CONTEXT) must be
122+
// guaranteed in the spawn env WITHOUT the login shell
123+
// (dockerDaemonEnvGuaranteed). Dropping the `$SHELL -l` wrap also drops
124+
// rc-file env inheritance. On macOS the startup hydration (MCP-2751) already
125+
// captured DOCKER_* from the login shell into os.Environ(), and secureenv
126+
// forwards them — but that hydration is Darwin-only. On Linux a
127+
// rootless/remote daemon whose DOCKER_HOST lives only in .profile would be
128+
// lost by direct-exec (the regression #699 kept the shell wrap to avoid). So
129+
// on non-Darwin we only direct-exec when DOCKER_HOST/DOCKER_CONTEXT are
130+
// already in os.Environ(); otherwise we keep the login-shell wrap so
131+
// `docker run` still finds the daemon.
132+
//
133+
// When we fall back to the shell wrap we still prefer the resolved absolute path
134+
// as the wrapped command (it sidesteps the login shell's PATH re-derivation),
135+
// dropping to bare "docker" only when resolution failed.
136+
func (c *Client) resolveDockerSpawn(finalArgs []string) (dockerCommand string, dockerArgs []string, shellWrapped bool) {
110137
resolved, resErr := resolveDockerBinary(c.logger)
111138
switch {
112139
case resErr == nil && isDirectExecutable(resolved) && dockerDaemonEnvGuaranteed():

‎internal/upstream/core/connection_docker_test.go‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,23 @@ func newIsolatedTestClient() *Client {
2727
}
2828
}
2929

30+
// newUserSuppliedDockerTestClient builds a Client whose config Command IS
31+
// `docker` (a user-supplied `docker run …` upstream, as opposed to a uvx/npx
32+
// server wrapped INTO docker by the isolation manager). This is the GH #696 /
33+
// MCP-2868 path.
34+
func newUserSuppliedDockerTestClient() *Client {
35+
return &Client{
36+
config: &config.ServerConfig{
37+
Name: "user-docker-server",
38+
Command: "docker",
39+
Args: []string{"run", "-i", "--rm", "mcp/example"},
40+
Env: map[string]string{"SLACK_TOKEN": "xoxb-secret"},
41+
},
42+
logger: zap.NewNop(),
43+
isolationManager: NewIsolationManager(config.DefaultDockerIsolationConfig()),
44+
}
45+
}
46+
3047
// forceDockerDaemonEnvGOOS overrides the GOOS the daemon-env guard sees, so the
3148
// Darwin (env-guaranteed-by-hydration) and non-Darwin branches can both be
3249
// exercised on a single CI host. Restored on cleanup.
@@ -290,6 +307,81 @@ func TestSetupDockerIsolationDirectExecsWhenDockerHostInEnv(t *testing.T) {
290307
assert.Equal(t, cmdRun, args[0])
291308
}
292309

310+
// TestResolveDockerSpawnUserSuppliedDockerRunDirectExec is the MCP-2868
311+
// root-cause assertion. A USER-SUPPLIED `docker run` upstream (config.Command IS
312+
// `docker`) must reuse the SAME resolve→direct-exec decision as the
313+
// isolation-injection path. With docker resolved to a verified absolute
314+
// executable and the daemon env guaranteed (macOS), the spawn must direct-exec
315+
// the ABSOLUTE docker path (no `$SHELL -l -c` wrap), carry the injected
316+
// `-e KEY=VALUE` env flags, and accept an args-based cidfile insertion. Before
317+
// this fix the user path shell-wrapped bare `docker`, producing GH #696's
318+
// `zsh:1: command not found: docker` on a default Docker Desktop macOS install
319+
// (docker CLI only inside the app bundle, not on the login-shell PATH).
320+
func TestResolveDockerSpawnUserSuppliedDockerRunDirectExec(t *testing.T) {
321+
fakeDocker := writeFakeDockerExecutable(t)
322+
forceDockerDaemonEnvGOOS(t, osDarwin) // daemon env guaranteed via startup hydration
323+
324+
orig := resolveDockerBinary
325+
t.Cleanup(func() { resolveDockerBinary = orig })
326+
resolveDockerBinary = func(_ *zap.Logger) (string, error) { return fakeDocker, nil }
327+
328+
c := newUserSuppliedDockerTestClient()
329+
// Mimic connectStdio's user-supplied docker branch: inject env vars as -e
330+
// flags into the docker run argv before deciding how to spawn.
331+
argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env)
332+
333+
cmd, args, shellWrapped := c.resolveDockerSpawn(argsToWrap)
334+
335+
assert.False(t, shellWrapped, "verified absolute docker must direct-exec, not shell-wrap")
336+
assert.Equal(t, fakeDocker, cmd,
337+
"user docker must be exec'd by its resolved absolute path, not bare 'docker', got: %s", cmd)
338+
require.NotEmpty(t, args)
339+
assert.Equal(t, cmdRun, args[0],
340+
"first arg must be the raw 'run' token (not a shell -c string), got: %v", args)
341+
// The injected -e env flag must survive into the docker run argv.
342+
assert.Subset(t, args, []string{"-e", "SLACK_TOKEN=xoxb-secret"},
343+
"injected -e env flags must be present in the direct-exec argv, got: %v", args)
344+
for _, a := range args {
345+
assert.NotContains(t, a, " run ",
346+
"args must be raw argv tokens, not a single shell command string, got: %v", args)
347+
}
348+
349+
// On the direct-exec path the caller inserts --cidfile via the args-based
350+
// helper (right after the "run" token).
351+
withCid := c.insertCidfileIntoDockerArgs(args, "/tmp/cid.txt")
352+
require.GreaterOrEqual(t, len(withCid), 3)
353+
assert.Equal(t, []string{cmdRun, "--cidfile", "/tmp/cid.txt"}, withCid[:3],
354+
"cidfile must be inserted right after 'run' on the direct-exec path, got: %v", withCid)
355+
}
356+
357+
// TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap verifies the
358+
// resolution-failure fallback for a user-supplied `docker run`: the spawn must
359+
// shell-wrap bare `docker` (preserving login-shell PATH resolution as a last
360+
// resort) and the cidfile is inserted via the string-based helper.
361+
func TestResolveDockerSpawnUserSuppliedFallsBackToShellWrap(t *testing.T) {
362+
orig := resolveDockerBinary
363+
t.Cleanup(func() { resolveDockerBinary = orig })
364+
resolveDockerBinary = func(_ *zap.Logger) (string, error) {
365+
return "", fmt.Errorf("docker not found")
366+
}
367+
368+
c := newUserSuppliedDockerTestClient()
369+
argsToWrap := c.injectEnvVarsIntoDockerArgs(c.config.Args, c.config.Env)
370+
371+
cmd, shellArgs, shellWrapped := c.resolveDockerSpawn(argsToWrap)
372+
373+
require.True(t, shellWrapped, "on resolution failure the user docker spawn must be shell-wrapped")
374+
assert.NotEqual(t, cmdDocker, cmd, "shell-wrap fallback exec's the login shell, not bare 'docker'")
375+
require.NotEmpty(t, shellArgs)
376+
cmdStr := shellArgs[len(shellArgs)-1]
377+
assert.True(t, strings.HasPrefix(cmdStr, "docker run"),
378+
"on resolution failure the spawn must fall back to bare 'docker', got: %s", cmdStr)
379+
// The real command string still carries the actual env value (redaction is a
380+
// log-only concern, not a spawn concern).
381+
assert.Contains(t, cmdStr, "-e SLACK_TOKEN=xoxb-secret",
382+
"injected -e env flag must survive into the shell command string, got: %s", cmdStr)
383+
}
384+
293385
// TestDockerDaemonEnvGuaranteed unit-tests the gate directly.
294386
func TestDockerDaemonEnvGuaranteed(t *testing.T) {
295387
t.Run("darwin is always guaranteed (startup hydration)", func(t *testing.T) {

‎internal/upstream/core/connection_stdio.go‎

Lines changed: 26 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -157,32 +157,44 @@ func (c *Client) connectStdio(ctx context.Context) error {
157157
finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile)
158158
}
159159
}
160-
} else {
161-
// For direct docker commands, inject env vars as -e flags before shell wrapping
160+
} else if isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun; isDirectDockerRun {
161+
// USER-SUPPLIED `docker run …` upstream (config.Command IS `docker`).
162+
// Inject env vars as -e flags, then reuse the SAME resolve→spawn decision
163+
// as the isolation path (resolveDockerSpawn) so this path also direct-execs
164+
// the resolved ABSOLUTE docker binary instead of shell-wrapping bare
165+
// `docker` — the GH #696 / MCP-2868 fix. Previously this branch always
166+
// called wrapWithUserShell("docker", …), which failed with
167+
// `command not found: docker` on a default Docker Desktop macOS install.
168+
c.isDockerCommand = true
169+
162170
argsToWrap := args
163-
isDirectDockerRun := (c.config.Command == cmdDocker || strings.HasSuffix(c.config.Command, "/"+cmdDocker)) && len(args) > 0 && args[0] == cmdRun
164-
if isDirectDockerRun && len(c.config.Env) > 0 {
171+
if len(c.config.Env) > 0 {
165172
argsToWrap = c.injectEnvVarsIntoDockerArgs(args, c.config.Env)
166173
c.logger.Debug("Injected env vars into direct docker command",
167174
zap.String("server", c.config.Name),
168175
zap.Int("env_count", len(c.config.Env)),
169176
zap.Strings("modified_args", shellwrap.RedactDockerArgs(argsToWrap)))
170177
}
171178

172-
// Use shell wrapping for environment inheritance
173-
// This fixes issues when mcpproxy is launched via Launchd and doesn't inherit
174-
// user's shell environment (like PATH customizations from .bashrc, .zshrc, etc.)
175-
finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, argsToWrap)
176-
c.isDockerCommand = false
179+
var dockerShellWrapped bool
180+
finalCommand, finalArgs, dockerShellWrapped = c.resolveDockerSpawn(argsToWrap)
177181

178-
// Handle explicit docker commands
179-
if isDirectDockerRun {
180-
c.isDockerCommand = true
181-
if cidFile != "" {
182-
// For shell-wrapped Docker commands, we need to modify the shell command string
182+
// Insert --cidfile via the helper that matches how we spawn: args-based on
183+
// the direct-exec path, string-based on the login-shell fallback.
184+
if cidFile != "" {
185+
if dockerShellWrapped {
183186
finalArgs = c.insertCidfileIntoShellDockerCommand(finalArgs, cidFile)
187+
} else {
188+
finalArgs = c.insertCidfileIntoDockerArgs(finalArgs, cidFile)
184189
}
185190
}
191+
} else {
192+
// Plain (non-docker) stdio command. Use shell wrapping for environment
193+
// inheritance. This fixes issues when mcpproxy is launched via Launchd and
194+
// doesn't inherit the user's shell environment (PATH customizations from
195+
// .bashrc, .zshrc, etc.).
196+
finalCommand, finalArgs = c.wrapWithUserShell(c.config.Command, args)
197+
c.isDockerCommand = false
186198
}
187199

188200
// Upstream transport with working directory support and process group management

0 commit comments

Comments
 (0)