From 7cb664f4acc42815e4e43466e89746307440f63e Mon Sep 17 00:00:00 2001 From: Rudy Celekli Date: Sat, 24 Oct 2026 17:36:30 -0400 Subject: [PATCH 1/2] fix(docker): insert --cidfile after run for docker.exe shell commands On Windows without DOCKER_HOST the docker spawn keeps the shell wrap, and the command string starts with the resolved docker.exe path. The literal "docker run" search missed it, so --cidfile was appended as extra shell argv where docker never saw it and the container could not be tracked for cleanup. Match the first token as docker or docker.exe (bare, path, or quoted) and insert --cidfile right after the run verb. --- .../upstream/core/cidfile_docker_exe_test.go | 52 ++++++++++ internal/upstream/core/connection_docker.go | 97 +++++++++++++++++-- 2 files changed, 141 insertions(+), 8 deletions(-) create mode 100644 internal/upstream/core/cidfile_docker_exe_test.go diff --git a/internal/upstream/core/cidfile_docker_exe_test.go b/internal/upstream/core/cidfile_docker_exe_test.go new file mode 100644 index 000000000..520b65557 --- /dev/null +++ b/internal/upstream/core/cidfile_docker_exe_test.go @@ -0,0 +1,52 @@ +package core + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// A Windows shell wrap names the CLI docker.exe, often as a quoted path +// (Docker Desktop installs under "Program Files"). --cidfile has to land +// immediately after "run". Appending it as extra shell argv leaves docker +// without a cidfile, so the container cannot be tracked for cleanup. +func TestInsertCidfile_DockerExe(t *testing.T) { + c := newIsolatedTestClient() + const cid = `C:\Temp\mcpproxy-cid.txt` + + cases := []struct { + name string + cmd string + want string + }{ + { + name: "bare docker.exe", + cmd: `docker.exe run --rm -i ghcr.io/some/image python -m srv`, + want: `docker.exe run --cidfile ` + cid + ` --rm -i ghcr.io/some/image python -m srv`, + }, + { + name: "quoted docker.exe path", + cmd: `"C:\Program Files\Docker\resources\bin\docker.exe" run --rm -i ghcr.io/some/image`, + want: `"C:\Program Files\Docker\resources\bin\docker.exe" run --cidfile ` + cid + ` --rm -i ghcr.io/some/image`, + }, + { + name: "unix docker still inserts after run", + cmd: `docker run --rm -i ghcr.io/some/image`, + want: `docker run --cidfile ` + cid + ` --rm -i ghcr.io/some/image`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + in := []string{"/c", tc.cmd} + out := c.insertCidfileIntoShellDockerCommand(in, cid) + require.Len(t, out, len(in), "cidfile must be inside the command string, not extra shell argv: %v", out) + assert.Equal(t, tc.want, out[len(out)-1]) + }) + } + + // A command that merely mentions docker must not be rewritten. + out := c.insertCidfileIntoShellDockerCommand([]string{"/c", "echo docker run"}, cid) + assert.False(t, strings.Contains(out[len(out)-1], "--cidfile")) +} diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 1630bd855..3b8333a74 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -367,13 +367,12 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile // Get the Docker command string (last argument) dockerCmd := shellArgs[len(shellArgs)-1] - // Insert --cidfile into the Docker command string - // Look for "docker run" and insert --cidfile right after - if strings.Contains(dockerCmd, "docker run") { - // Replace "docker run" with "docker run --cidfile /path/to/file" - dockerCmdWithCid := strings.Replace(dockerCmd, "docker run", fmt.Sprintf("docker run --cidfile %s", cidFile), 1) - - // Create new args with the modified command + // Insert --cidfile immediately after the run verb. The executable may be the + // bare name "docker", "docker.exe" (Windows), or a quoted path to either. + // A literal search for "docker run" misses docker.exe, and the fallback + // below then appends --cidfile as extra shell argv, where docker never + // sees it (it lands after the image and is passed into the container). + if dockerCmdWithCid, ok := insertCidfileAfterDockerRun(dockerCmd, cidFile); ok { newArgs := make([]string, len(shellArgs)) copy(newArgs, shellArgs) newArgs[len(newArgs)-1] = dockerCmdWithCid @@ -386,13 +385,95 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile return newArgs } - // If we can't find "docker run", fall back to appending + // If we can't find a docker run verb, fall back to appending c.logger.Warn("Could not find 'docker run' in shell command for cidfile insertion", zap.String("server", c.config.Name), zap.String("docker_cmd", logSafeCommand(dockerCmd))) return append(shellArgs, "--cidfile", cidFile) } +// insertCidfileAfterDockerRun returns the command with "--cidfile " +// placed immediately after the run verb of a docker or docker.exe invocation. +// The executable may be bare or a quoted path. ok is false when the command +// does not start with one. +func insertCidfileAfterDockerRun(dockerCmd, cidFile string) (string, bool) { + end, ok := dockerRunVerbEnd(dockerCmd) + if !ok { + return "", false + } + return dockerCmd[:end] + " --cidfile " + cidFile + dockerCmd[end:], true +} + +// dockerRunVerbEnd reports the index just past the "run" verb when cmd's first +// token is a docker or docker.exe executable (bare, a path, or quoted). +func dockerRunVerbEnd(cmd string) (int, bool) { + i := skipShellSpace(cmd, 0) + tok, next, ok := readShellToken(cmd, i) + if !ok || !isDockerExecutableToken(tok) { + return 0, false + } + i = skipShellSpace(cmd, next) + const verb = "run" + if i+len(verb) > len(cmd) || cmd[i:i+len(verb)] != verb { + return 0, false + } + end := i + len(verb) + if end < len(cmd) && cmd[end] != ' ' && cmd[end] != '\t' { + return 0, false + } + return end, true +} + +func skipShellSpace(s string, i int) int { + for i < len(s) && (s[i] == ' ' || s[i] == '\t') { + i++ + } + return i +} + +// readShellToken reads one shell word starting at i. A leading quote consumes +// until the matching closer; the returned token is the word without quotes. +func readShellToken(s string, i int) (string, int, bool) { + if i >= len(s) { + return "", i, false + } + if s[i] == '"' || s[i] == '\'' { + q := s[i] + j := i + 1 + for j < len(s) && s[j] != q { + j++ + } + if j >= len(s) { + return "", i, false + } + return s[i+1 : j], j + 1, true + } + j := i + for j < len(s) && s[j] != ' ' && s[j] != '\t' { + j++ + } + if j == i { + return "", i, false + } + return s[i:j], j, true +} + +// isDockerExecutableToken reports whether tok names the docker CLI, including +// a path and the Windows docker.exe form. Comparison is case-insensitive +// because Windows executable lookup is. +func isDockerExecutableToken(tok string) bool { + base := tok + if k := strings.LastIndexAny(base, `/\`); k >= 0 { + base = base[k+1:] + } + switch strings.ToLower(base) { + case "docker", "docker.exe": + return true + default: + return false + } +} + // extractContainerNameFromArgs extracts the container name from Docker run arguments func (c *Client) extractContainerNameFromArgs(dockerArgs []string) string { // Look for --name flag in the arguments From 5d0dcf47509a2dd67e068ea8ac3b5b620854fe62 Mon Sep 17 00:00:00 2001 From: Algis Dumbris Date: Mon, 26 Oct 2026 13:20:26 +0200 Subject: [PATCH 2/2] fix(docker): quote the cidfile path for the wrapping shell Review finding on #1773: the cidfile path was spliced unquoted into the string cmd.exe /c or bash -c re-parses. A %TEMP% with a space split the path under cmd.exe and broke container start; under Git Bash the Windows path lost its backslashes so the container ID was never tracked. Quote it per shell dialect, keyed on the /c vs -c flag. Co-Authored-By: Claude Opus 5.5 --- .../upstream/core/cidfile_docker_exe_test.go | 39 +++++++++++++++++++ internal/upstream/core/connection_docker.go | 22 ++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/internal/upstream/core/cidfile_docker_exe_test.go b/internal/upstream/core/cidfile_docker_exe_test.go index 520b65557..4bcca7ec2 100644 --- a/internal/upstream/core/cidfile_docker_exe_test.go +++ b/internal/upstream/core/cidfile_docker_exe_test.go @@ -50,3 +50,42 @@ func TestInsertCidfile_DockerExe(t *testing.T) { out := c.insertCidfileIntoShellDockerCommand([]string{"/c", "echo docker run"}, cid) assert.False(t, strings.Contains(out[len(out)-1], "--cidfile")) } + +// The cidfile path is spliced into a string the shell re-parses, so it must be +// quoted in that shell's dialect (review finding on #1773). +func TestInsertCidfile_QuotesPathForShell(t *testing.T) { + c := newIsolatedTestClient() + + cases := []struct { + name string + args []string + cid string + want string + }{ + { + name: "cmd.exe with a space in TEMP", + args: []string{"/c", `"C:\Program Files\Docker\docker.exe" run --rm -i img`}, + cid: `C:\Users\John Smith\AppData\Local\Temp\mcpproxy-cid-1.txt`, + want: `"C:\Program Files\Docker\docker.exe" run --cidfile "C:\Users\John Smith\AppData\Local\Temp\mcpproxy-cid-1.txt" --rm -i img`, + }, + { + name: "git bash keeps backslashes", + args: []string{"-l", "-c", `'C:/Program Files/Docker/docker.exe' run --rm -i img`}, + cid: `C:\Users\jsmith\AppData\Local\Temp\mcpproxy-cid-1.txt`, + want: `'C:/Program Files/Docker/docker.exe' run --cidfile 'C:\Users\jsmith\AppData\Local\Temp\mcpproxy-cid-1.txt' --rm -i img`, + }, + { + name: "plain unix path stays unquoted", + args: []string{"-l", "-c", `docker run --rm -i img`}, + cid: `/tmp/mcpproxy-cid-1.txt`, + want: `docker run --cidfile /tmp/mcpproxy-cid-1.txt --rm -i img`, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out := c.insertCidfileIntoShellDockerCommand(tc.args, tc.cid) + require.Len(t, out, len(tc.args)) + assert.Equal(t, tc.want, out[len(out)-1]) + }) + } +} diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 3b8333a74..61fe48403 100644 --- a/internal/upstream/core/connection_docker.go +++ b/internal/upstream/core/connection_docker.go @@ -372,7 +372,11 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile // A literal search for "docker run" misses docker.exe, and the fallback // below then appends --cidfile as extra shell argv, where docker never // sees it (it lands after the image and is passed into the container). - if dockerCmdWithCid, ok := insertCidfileAfterDockerRun(dockerCmd, cidFile); ok { + // The cidfile path is spliced into a string the shell re-parses, so quote it + // in that shell's dialect: a %TEMP% with a space would otherwise split the + // path under cmd.exe, and bash -c would strip a Windows path's backslashes. + quotedCid := quoteCidfileForShell(cidFile, secondToLast == "/c") + if dockerCmdWithCid, ok := insertCidfileAfterDockerRun(dockerCmd, quotedCid); ok { newArgs := make([]string, len(shellArgs)) copy(newArgs, shellArgs) newArgs[len(newArgs)-1] = dockerCmdWithCid @@ -392,6 +396,22 @@ func (c *Client) insertCidfileIntoShellDockerCommand(shellArgs []string, cidFile return append(shellArgs, "--cidfile", cidFile) } +// quoteCidfileForShell quotes cidFile for the shell that will parse the +// wrapped command string: cmd.exe (/c) or a POSIX shell (-c, including Git +// Bash on Windows). +func quoteCidfileForShell(cidFile string, cmdExe bool) string { + if cmdExe { + if strings.ContainsAny(cidFile, " \t&|<>()^%") { + return `"` + cidFile + `"` + } + return cidFile + } + if !strings.ContainsAny(cidFile, " \t\n\r\"'\\$`;&|<>(){}[]?*~") { + return cidFile + } + return "'" + strings.ReplaceAll(cidFile, "'", `'"'"'`) + "'" +} + // insertCidfileAfterDockerRun returns the command with "--cidfile " // placed immediately after the run verb of a docker or docker.exe invocation. // The executable may be bare or a quoted path. ok is false when the command