From 143c928b81d2f05629240af5fbfb95ccfa9ffb5a Mon Sep 17 00:00:00 2001 From: Ploie77 Date: Sat, 19 Sep 2026 22:55:36 +0400 Subject: [PATCH] fix(shellwrap): Git Bash on Windows cannot exec backslash-style paths WrapWithUserShell correctly single-quotes a backslash-style Windows path (e.g. C:\ProgramData\foo\bar.cmd) when the resolved login shell is bash-like, but Git Bash / MSYS still cannot execute it: MSYS's own 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). This breaks every stdio server configured with an absolute Windows-path command (wrapper .cmd/.bat scripts are the common case) on any Windows host where the resolved shell is bash-like, e.g. Git Bash / VS Code integrated terminal / MSYS2 set as $SHELL. Fix: convert backslashes to forward slashes in the command and args before shell-escaping, but only on the Windows + bash-like-shell branch. Windows' CreateProcess and MSYS's exec layer both accept forward-slash paths, so this is safe in both directions. Verified against a real config with 14 wrapper-.cmd-based stdio servers that were all failing with this exact error: connected server count went from 13 to 27, tool count from 249 to 403, after rebuilding with this fix. Fixes #1318 --- .../shellwrap/gitbash_windows_path_test.go | 31 +++++++++++++ internal/shellwrap/shellwrap.go | 46 ++++++++++++++++--- .../upstream/core/connection_docker_test.go | 9 +++- 3 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 internal/shellwrap/gitbash_windows_path_test.go 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)