fix(docker): insert --cidfile after run for docker.exe shell commands

internal/upstream/core/cidfile_docker_exe_test.go

@@ -0,0 +1,91 @@
+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"))
+}
+
+// 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])
+		})
+	}
+}

internal/upstream/core/connection_docker.go

@@ -370,13 +370,16 @@ 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).
+	// 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
@@ -389,13 +392,111 @@ 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)
 }
 
+// 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 <file>"
+// 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
+	}
+}
+
 // configuredDockerBinary returns the server's own command when it is a
 // user-supplied `docker run` naming the docker CLI by an absolute or relative
 // path to an executable, so that binary is spawned instead of whichever docker the