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..4bcca7ec2 --- /dev/null +++ b/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]) + }) + } +} diff --git a/internal/upstream/core/connection_docker.go b/internal/upstream/core/connection_docker.go index 7fec7ff94..257c55a21 100644 --- a/internal/upstream/core/connection_docker.go +++ b/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 " +// 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