diff --git a/internal/shellwrap/gitbash_windows_path_test.go b/internal/shellwrap/gitbash_windows_path_test.go new file mode 100644 index 000000000..e9bd7f348 --- /dev/null +++ b/internal/shellwrap/gitbash_windows_path_test.go @@ -0,0 +1,31 @@ +package shellwrap + +import ( + "os/exec" + "runtime" + "testing" +) + +// Regression test for the mcpproxy wrapper-server outage on Windows: Git +// Bash could not exec a backslash-style Windows path even though +// WrapWithUserShell correctly single-quoted it, because MSYS's exec layer +// only resolves POSIX-style paths. A backslash path falls through to +// bash's PATH lookup, which fails with "command not found" and mangles the +// path in its own error rendering (backslashes silently dropped). See +// toBashPath in WrapWithUserShell. +func TestWrapWithUserShell_GitBashCanExecWindowsPath(t *testing.T) { + if runtime.GOOS != "windows" { + t.Skip("Windows-only: exercises the real Git Bash exec path") + } + bashPath := `C:\Program Files\Git\bin\bash.exe` + if _, err := exec.Command(bashPath, "--version").Output(); err != nil { + t.Skipf("Git Bash not available at %s: %v", bashPath, err) + } + + shell, args := WrapWithUserShell(nil, `C:\Windows\System32\whoami.exe`, nil) + cmd := exec.Command(shell, args...) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("bash failed to exec a Windows-style path: %v\nargs=%v\noutput=%s", err, args, out) + } +} diff --git a/internal/shellwrap/shellwrap.go b/internal/shellwrap/shellwrap.go index 362f434b8..a4d7b3e45 100644 --- a/internal/shellwrap/shellwrap.go +++ b/internal/shellwrap/shellwrap.go @@ -36,14 +36,26 @@ const ( // This mirrors the implementation in internal/upstream/core so both code paths // can converge on one function. func Shellescape(s string) string { + return shellescapeFor(s, runtime.GOOS == osWindows) +} + +// shellescapeFor is Shellescape parameterized by which quoting dialect to +// use, rather than assuming GOOS decides it. GOOS is the wrong signal when +// the shell that will actually interpret the string is Git Bash / MSYS on +// Windows: cmd.exe-style quoting leaves backslashes unescaped, and bash then +// consumes every backslash in a Windows path (C:\ProgramData\... becomes +// C:ProgramDataQalatCyber... before the child ever sees it). Callers pick +// the dialect by asking "is the target shell cmd.exe-like", not "am I on +// Windows". +func shellescapeFor(s string, windowsStyle bool) string { if s == "" { - if runtime.GOOS == osWindows { + if windowsStyle { return `""` } return "''" } - if runtime.GOOS == osWindows { + if windowsStyle { // Windows cmd.exe special characters. if !strings.ContainsAny(s, " \t\n\r\"&|<>()^%") { return s @@ -98,10 +110,33 @@ func resolveLoginShell() string { func WrapWithUserShell(logger *zap.Logger, command string, args []string) (shell string, shellArgs []string) { shell = resolveLoginShell() + // The escaping dialect must match the shell that will actually parse + // commandString, not the host OS. $SHELL=Git-Bash on Windows is the + // common case this diverges from GOOS: the string is fed to bash -c, + // so it needs POSIX single-quoting, not cmd.exe quoting. This mirrors + // the same isBash check used below to pick -l -c vs /c. + isBash := isBashLikeShell(shell) + windowsStyle := runtime.GOOS == osWindows && !isBash + + // Git Bash / MSYS on Windows cannot exec a backslash-style Windows path + // (C:\ProgramData\...) even when it is correctly single-quoted: MSYS's + // own exec layer only resolves POSIX-style paths, so it falls through to + // bash's PATH lookup, which reports "command not found" using its own + // mangled rendering of the argv word (backslashes silently dropped). A + // forward-slash path (C:/ProgramData/...) is accepted by both Windows' + // CreateProcess and MSYS's exec layer, so convert before quoting when + // we're about to run a bash-like shell on Windows. + toBashPath := func(s string) string { + if runtime.GOOS == osWindows && isBash { + return strings.ReplaceAll(s, `\`, "/") + } + return s + } + parts := make([]string, 0, len(args)+1) - parts = append(parts, Shellescape(command)) + parts = append(parts, shellescapeFor(toBashPath(command), windowsStyle)) for _, a := range args { - parts = append(parts, Shellescape(a)) + parts = append(parts, shellescapeFor(toBashPath(a), windowsStyle)) } commandString := strings.Join(parts, " ") @@ -127,8 +162,7 @@ func WrapWithUserShell(logger *zap.Logger, command string, args []string) (shell zap.String("shell", shell)) } - isBash := isBashLikeShell(shell) - if runtime.GOOS == osWindows && !isBash { + if windowsStyle { // Windows cmd.exe: /c to execute a command string. return shell, []string{"/c", commandString} } diff --git a/internal/upstream/core/connection_docker_test.go b/internal/upstream/core/connection_docker_test.go index ae9493b8c..9b2d91bbc 100644 --- a/internal/upstream/core/connection_docker_test.go +++ b/internal/upstream/core/connection_docker_test.go @@ -275,7 +275,14 @@ func TestSetupDockerIsolationShellWrapsWhenDaemonEnvMissingNonDarwin(t *testing. "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, + // When the resolved login shell is bash-like on Windows (Git Bash/MSYS), + // WrapWithUserShell rewrites backslashes to forward slashes before + // quoting: MSYS's exec layer cannot run a backslash-style Windows path + // even when it is correctly single-quoted (it falls through to bash's + // PATH lookup and reports "command not found"). Compare against both + // separator styles so this assertion holds regardless of which shell + // dialect resolved on the test host. + assert.Contains(t, strings.ReplaceAll(cmdStr, "/", string(filepath.Separator)), 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)