Fix quoting of shell commands on Windows (#5704)

Lazygit builds a shell command by interpolating Quote'd arguments into a
template and running the result via `cmd /c`. Several things were wrong
on Windows:

- Quote emitted bash-style `\"…\"` quoting, which cmd.exe doesn't
understand. Making it usable at all previously required a fragile
round-trip through str.ToArgv and re-escaping.
- The assembled command line was handed to `cmd /c` without `/s`, so
cmd's default rules stripped the wrong quotes once the line contained
more than two of them (e.g. a quoted editor path at a location with
spaces, plus a quoted filename that also contains spaces).
- Shell metacharacters were escaped with `^` (`&` → `^&`, etc.), which
neutralised command chaining, pipes, redirection and `%VAR%` expansion
in custom commands.

Quote now emits the standard Windows convention directly, and NewShell
hands cmd.exe the fully-assembled line verbatim via SysProcAttr.CmdLine,
wrapped as `cmd /s /c "<command>"`. The /s flag strips exactly the outer
quote pair we add, leaving each argument's own quoting intact. With the
`^` escaping gone, metacharacters in a custom command reach cmd as the
author intended; this also removes the spurious `^` reported in #3092.

Fixes #5560
Fixes #2427
Fixes #4147
This commit is contained in:
Stefan Haller 2026-06-23 14:14:07 +02:00 committed by GitHub
commit 9f6db03fea
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 495 additions and 35 deletions

View file

@ -48,26 +48,34 @@ func (self *CmdObjBuilder) NewShell(commandStr string, shellFunctionsFile string
if len(shellFunctionsFile) > 0 {
commandStr = fmt.Sprintf("%ssource %s\n%s", self.platform.PrefixForShellFunctionsFile, shellFunctionsFile, commandStr)
}
quotedCommand := self.quotedCommandString(commandStr)
if self.platform.OS == "windows" {
return self.newWindowsShell(commandStr)
}
quotedCommand := self.Quote(commandStr)
cmdArgs := str.ToArgv(fmt.Sprintf("%s %s %s", self.platform.Shell, self.platform.ShellArg, quotedCommand))
return self.New(cmdArgs)
}
func (self *CmdObjBuilder) quotedCommandString(commandStr string) string {
// Windows does not seem to like quotes around the command
if self.platform.OS == "windows" {
return strings.NewReplacer(
"^", "^^",
"&", "^&",
"|", "^|",
"<", "^<",
">", "^>",
"%", "^%",
).Replace(commandStr)
}
// newWindowsShell wraps the command in `cmd.exe /s /c "<command>"`. The /s
// flag tells cmd to strip exactly the outermost pair of quotes and pass the
// rest through unchanged, which preserves any quoting the command itself
// contains (e.g. `"C:\Program Files\my-editor.exe" file.txt`). Without /s,
// cmd's default rules drop the wrong quotes once the command line contains
// more than two of them.
//
// We bypass Go's standard arg quoting via SysProcAttr.CmdLine: it follows the
// CommandLineToArgvW convention (`\"` for inner quotes), but cmd.exe doesn't.
func (self *CmdObjBuilder) newWindowsShell(commandStr string) *CmdObj {
args := []string{self.platform.Shell, "/s", self.platform.ShellArg, commandStr}
cmdObj := self.New(args)
return self.Quote(commandStr)
cmdLine := fmt.Sprintf(`%s /s %s "%s"`, self.platform.Shell, self.platform.ShellArg, commandStr)
setRawCmdLine(cmdObj.GetCmd(), cmdLine)
return cmdObj
}
func (self *CmdObjBuilder) CloneWithNewRunner(decorate func(ICmdObjRunner) ICmdObjRunner) *CmdObjBuilder {
@ -80,21 +88,47 @@ func (self *CmdObjBuilder) CloneWithNewRunner(decorate func(ICmdObjRunner) ICmdO
}
func (self *CmdObjBuilder) Quote(message string) string {
var quote string
if self.platform.OS == "windows" {
quote = `\"`
message = strings.NewReplacer(
`"`, `"'"'"`,
`\"`, `\\"`,
).Replace(message)
} else {
quote = `"`
message = strings.NewReplacer(
`\`, `\\`,
`"`, `\"`,
`$`, `\$`,
"`", "\\`",
).Replace(message)
return quoteForWindows(message)
}
return quote + message + quote
message = strings.NewReplacer(
`\`, `\\`,
`"`, `\"`,
`$`, `\$`,
"`", "\\`",
).Replace(message)
return `"` + message + `"`
}
// quoteForWindows encodes a value using the standard Windows command-line
// convention (the algorithm behind syscall.EscapeArg, reimplemented here so
// it's available on all platforms). The result is always wrapped in double
// quotes so cmd.exe and CommandLineToArgvW treat it as a single argument
// regardless of what shell metacharacters it contains.
func quoteForWindows(s string) string {
var b strings.Builder
b.WriteByte('"')
slashes := 0
for i := range len(s) {
c := s[i]
switch c {
case '\\':
slashes++
b.WriteByte(c)
case '"':
for ; slashes > 0; slashes-- {
b.WriteByte('\\')
}
b.WriteByte('\\')
b.WriteByte(c)
default:
slashes = 0
b.WriteByte(c)
}
}
for ; slashes > 0; slashes-- {
b.WriteByte('\\')
}
b.WriteByte('"')
return b.String()
}

View file

@ -0,0 +1,389 @@
//go:build windows
package oscommands
import (
"os"
"os/exec"
"path/filepath"
"strings"
"sync"
"testing"
"github.com/jesseduffield/lazygit/pkg/utils"
"github.com/stretchr/testify/assert"
)
// These tests run only on Windows because they exercise real cmd.exe
// quote-parsing behaviour, which has only been a problem on Windows.
// makeWindowsShellBuilder returns a CmdObjBuilder configured for a real
// Windows cmd shell, bypassing the test "dummy" platform (which is darwin).
func makeWindowsShellBuilder() *CmdObjBuilder {
log := utils.NewDummyLog()
return &CmdObjBuilder{
runner: &cmdObjRunner{log: log, guiIO: NewNullGuiIO(log)},
platform: &Platform{OS: "windows", Shell: "cmd", ShellArg: "/c"},
}
}
// fakeEditorSrc is a minimal Go program that records the args it received,
// one per line, to marker.txt in its own directory. Using a real .exe (not a
// .bat) means args are parsed by Go's runtime via CommandLineToArgvW — the
// same algorithm used by ~all real Windows GUI editors. A .bat would parse
// args via cmd.exe's own rules, which can hide bugs that affect editors.
const fakeEditorSrc = `package main
import (
"os"
"path/filepath"
"strings"
)
func main() {
exe, err := os.Executable()
if err != nil {
os.Exit(2)
}
marker := filepath.Join(filepath.Dir(exe), "marker.txt")
body := strings.Join(os.Args[1:], "\n")
if err := os.WriteFile(marker, []byte(body), 0o644); err != nil {
os.Exit(3)
}
}
`
var (
fakeEditorOnce sync.Once
fakeEditorBytes []byte
fakeEditorErr error
)
// loadFakeEditorBytes builds the fake editor exactly once per test process
// and returns its bytes. Tests then drop a copy at a path containing spaces.
func loadFakeEditorBytes(t *testing.T) []byte {
t.Helper()
fakeEditorOnce.Do(func() {
buildDir, err := os.MkdirTemp("", "lazygit-fake-editor-build-*")
if err != nil {
fakeEditorErr = err
return
}
defer os.RemoveAll(buildDir)
srcPath := filepath.Join(buildDir, "main.go")
binPath := filepath.Join(buildDir, "fake-editor.exe")
if err := os.WriteFile(srcPath, []byte(fakeEditorSrc), 0o644); err != nil {
fakeEditorErr = err
return
}
cmd := exec.Command("go", "build", "-o", binPath, srcPath)
cmd.Stderr = os.Stderr
if err := cmd.Run(); err != nil {
fakeEditorErr = err
return
}
fakeEditorBytes, fakeEditorErr = os.ReadFile(binPath)
})
if fakeEditorErr != nil {
t.Fatalf("failed to build fake editor helper: %v", fakeEditorErr)
}
return fakeEditorBytes
}
// placeFakeEditor builds the fake editor and places it at a path containing
// a space (mirroring `C:\Program Files\...`). The marker the editor writes
// lives next to the exe.
func placeFakeEditor(t *testing.T) (exe, markerFile string) {
t.Helper()
bin := loadFakeEditorBytes(t)
exeDir := filepath.Join(t.TempDir(), "Program Files", "FakeEditor")
if err := os.MkdirAll(exeDir, 0o755); err != nil {
t.Fatalf("mkdir exeDir: %v", err)
}
exe = filepath.Join(exeDir, "fake-editor.exe")
markerFile = filepath.Join(exeDir, "marker.txt")
if err := os.WriteFile(exe, bin, 0o755); err != nil {
t.Fatalf("write fake editor: %v", err)
}
return exe, markerFile
}
// placeTargetFile creates a file at <freshTempDir>/<dirName>/<basename> with
// a trivial body. Use a dirName containing a space (e.g. "my repo") to put
// the file at a path with spaces.
func placeTargetFile(t *testing.T, dirName, basename string) string {
t.Helper()
dir := filepath.Join(t.TempDir(), dirName)
if err := os.MkdirAll(dir, 0o755); err != nil {
t.Fatalf("mkdir target dir: %v", err)
}
target := filepath.Join(dir, basename)
if err := os.WriteFile(target, []byte("hello"), 0o644); err != nil {
t.Fatalf("write target: %v", err)
}
return target
}
// setupFakeEditor is a convenience wrapper for the common case: editor at a
// spacey path AND target file at a spacey path — the conditions that
// trigger the cmd.exe quote-stripping bug.
func setupFakeEditor(t *testing.T) (fakeExe, targetFile, markerFile string) {
t.Helper()
fakeExe, markerFile = placeFakeEditor(t)
targetFile = placeTargetFile(t, "my repo", "file.txt")
return fakeExe, targetFile, markerFile
}
// resolveTemplate mirrors what pkg/commands/git_commands/file.go does: it
// substitutes {{filename}} with the Windows-quoted filename and {{line}}
// with a line number.
func resolveTemplate(builder *CmdObjBuilder, template, filename, line string) string {
out := strings.ReplaceAll(template, "{{filename}}", builder.Quote(filename))
out = strings.ReplaceAll(out, "{{line}}", line)
return out
}
// readMarkerArgs reads the args the fake editor recorded. Each arg is on its
// own line (so an arg that itself contains a space stays one element). An
// empty file means the editor ran with zero args.
func readMarkerArgs(t *testing.T, markerFile string) []string {
t.Helper()
data, err := os.ReadFile(markerFile)
if err != nil {
t.Fatalf("marker file was not written; the fake editor never ran: %v", err)
}
s := strings.TrimRight(string(data), "\r\n")
if s == "" {
return nil
}
return strings.Split(s, "\n")
}
func TestNewShell_QuotedExePath_FilenameWithSpaces_EditAtLineAndWait(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, targetFile, markerFile := setupFakeEditor(t)
template := `"` + fakeExe + `" -multiInst -nosession -noPlugin -n{{line}} {{filename}}`
cmdStr := resolveTemplate(builder, template, targetFile, "42")
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
assert.Equal(t,
[]string{"-multiInst", "-nosession", "-noPlugin", "-n42", targetFile},
readMarkerArgs(t, markerFile),
)
}
func TestNewShell_QuotedExePath_FilenameWithSpaces_EditAtLine(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, targetFile, markerFile := setupFakeEditor(t)
template := `"` + fakeExe + `" -n{{line}} {{filename}}`
cmdStr := resolveTemplate(builder, template, targetFile, "42")
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
assert.Equal(t,
[]string{"-n42", targetFile},
readMarkerArgs(t, markerFile),
)
}
func TestNewShell_QuotedExePath_FilenameWithSpaces_Edit(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, targetFile, markerFile := setupFakeEditor(t)
template := `"` + fakeExe + `" {{filename}}`
cmdStr := resolveTemplate(builder, template, targetFile, "")
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
assert.Equal(t,
[]string{targetFile},
readMarkerArgs(t, markerFile),
)
}
// Sanity check: for a filename WITHOUT spaces the same templates already work,
// because the resulting cmd.exe line has exactly two quote characters and
// cmd /c keeps them. This pins the difference down to filename quoting and
// guards against a regression where the no-spaces case starts failing too.
func TestNewShell_QuotedExePath_FilenameWithoutSpaces_StillWorks(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, markerFile := placeFakeEditor(t)
plainTarget := placeTargetFile(t, "repo", "plain.txt") // no-space dir
template := `"` + fakeExe + `" -multiInst -nosession -noPlugin -n{{line}} {{filename}}`
cmdStr := resolveTemplate(builder, template, plainTarget, "42")
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
assert.Equal(t,
[]string{"-multiInst", "-nosession", "-noPlugin", "-n42", plainTarget},
readMarkerArgs(t, markerFile),
)
}
// TestNewShell_VarietyOfEditorTemplates exercises NewShell with a range of
// realistic editor templates, all with the trigger conditions of the bug
// (quoted exe at a spacey path + filename at a spacey path). Each subtest
// asserts the editor receives the exact args lazygit intended.
//
// Args in `wantArgs` may use the literal "<file>" placeholder; it gets
// substituted with the resolved target file path before comparison.
func TestNewShell_VarietyOfEditorTemplates(t *testing.T) {
const filePlaceholder = "<file>"
cases := []struct {
name string
template string // <exe> stands for the fake editor's full path
line string
wantArgs []string
}{
{
name: "vim/nvim style: +line filename",
template: `"<exe>" +{{line}} {{filename}}`,
line: "42",
wantArgs: []string{"+42", filePlaceholder},
},
{
name: "emacs-like with explicit +N",
template: `"<exe>" +{{line}} -nw {{filename}}`,
line: "7",
wantArgs: []string{"+7", "-nw", filePlaceholder},
},
{
name: "long flag with =value",
template: `"<exe>" --line={{line}} --tab-size=4 {{filename}}`,
line: "42",
wantArgs: []string{"--line=42", "--tab-size=4", filePlaceholder},
},
{
name: "many short and long flags before filename",
template: `"<exe>" -a -b -c --foo --bar -n{{line}} {{filename}}`,
line: "42",
wantArgs: []string{"-a", "-b", "-c", "--foo", "--bar", "-n42", filePlaceholder},
},
{
name: "flag after filename",
template: `"<exe>" {{filename}} --readonly`,
line: "",
wantArgs: []string{filePlaceholder, "--readonly"},
},
{
name: "single short flag attached to value",
template: `"<exe>" -n{{line}} {{filename}}`,
line: "1",
wantArgs: []string{"-n1", filePlaceholder},
},
{
name: "no flags, just filename",
template: `"<exe>" {{filename}}`,
line: "",
wantArgs: []string{filePlaceholder},
},
{
name: "flag with separate value (space-separated)",
template: `"<exe>" --goto {{line}} {{filename}}`,
line: "42",
wantArgs: []string{"--goto", "42", filePlaceholder},
},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, targetFile, markerFile := setupFakeEditor(t)
template := strings.ReplaceAll(tc.template, "<exe>", fakeExe)
cmdStr := resolveTemplate(builder, template, targetFile, tc.line)
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
want := make([]string, len(tc.wantArgs))
for i, a := range tc.wantArgs {
want[i] = strings.ReplaceAll(a, filePlaceholder, targetFile)
}
assert.Equal(t, want, readMarkerArgs(t, markerFile))
})
}
}
// TestNewShell_FilenameSpecialCharacters varies the basename of the target
// file across characters that are legal in Windows filenames but might
// interact badly with cmd.exe / Quote(): parentheses, brackets, single
// quote, comma, semicolon, equals, etc. The exe is at a spacey path and
// the target dir has spaces, so the bug-trigger conditions are still met.
func TestNewShell_FilenameSpecialCharacters(t *testing.T) {
cases := []struct {
name string
basename string
}{
{"parens", "file (1).txt"},
{"brackets", "file[v2].txt"},
{"single quote", "it's a file.txt"},
{"comma", "a,b,c.txt"},
{"semicolon", "a;b.txt"},
{"equals", "key=value.txt"},
{"plus", "a+b.txt"},
{"hash", "issue#42.txt"},
{"at sign", "user@host.txt"},
{"tilde", "~backup.txt"},
{"dot leading", ".gitignore.txt"},
{"multiple dots", "v1.2.3.txt"},
{"dash leading", "-flag-looking.txt"},
}
for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) {
builder := makeWindowsShellBuilder()
fakeExe, markerFile := placeFakeEditor(t)
targetFile := placeTargetFile(t, "my repo", tc.basename)
template := `"` + fakeExe + `" -n{{line}} {{filename}}`
cmdStr := resolveTemplate(builder, template, targetFile, "42")
out, err := builder.NewShell(cmdStr, "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
assert.Equal(t,
[]string{"-n42", targetFile},
readMarkerArgs(t, markerFile),
)
})
}
}
// Command chaining with && must work: cmd /s /c runs the assembled line verbatim,
// so cmd treats && as a separator and runs both commands. The two echoes
// therefore produce two separate output lines.
func TestNewShell_CommandChaining(t *testing.T) {
builder := makeWindowsShellBuilder()
out, err := builder.NewShell("echo first&&echo second", "").GetCmd().CombinedOutput()
if err != nil {
t.Fatalf("shell command failed: %v\ncmd.exe output:\n%s", err, string(out))
}
normalized := strings.ReplaceAll(string(out), "\r\n", "\n")
lines := strings.Split(strings.TrimSpace(normalized), "\n")
assert.Equal(t, []string{"first", "second"}, lines)
}

View file

@ -40,6 +40,11 @@ func (c *OSCommand) UpdateWindowTitle() error {
return nil
}
// setRawCmdLine is the non-Windows no-op counterpart of the Windows shim
// (see the comment there). NewShell's shell-building logic is portable, so
// this call is reached on every host; only the Windows build does anything.
func setRawCmdLine(cmd *exec.Cmd, cmdLine string) {}
func TerminateProcessGracefully(cmd *exec.Cmd) error {
if cmd.Process == nil {
return nil

View file

@ -75,11 +75,26 @@ func TestOSCommandQuoteWindows(t *testing.T) {
actual := osCommand.Quote(`hello "test" 'test2'`)
expected := `\"hello "'"'"test"'"'" 'test2'\"`
expected := `"hello \"test\" 'test2'"`
assert.EqualValues(t, expected, actual)
}
// On Windows, NewShell must hand the command to cmd.exe verbatim.
func TestNewShellWindowsPassesMetacharactersVerbatim(t *testing.T) {
osCommand := NewDummyOSCommand()
platform := &Platform{OS: "windows", Shell: "cmd", ShellArg: "/c"}
osCommand.Platform = platform
osCommand.Cmd.platform = platform
command := `echo a && echo b | sort > out.txt < in.txt %PATH%`
assert.Equal(t,
[]string{"cmd", "/s", "/c", command},
osCommand.Cmd.NewShell(command, "").Args(),
)
}
func TestOSCommandFileType(t *testing.T) {
type scenario struct {
path string

View file

@ -5,8 +5,25 @@ import (
"os"
"os/exec"
"path/filepath"
"syscall"
)
// setRawCmdLine hands cmd.exe the exact command line we built, bypassing
// os/exec's default composition (which quotes args with the
// CommandLineToArgvW `\"` convention that cmd.exe doesn't understand).
//
// The shell-building logic in NewShell is portable and dispatches on
// platform.OS, which keeps it (and its quoting) unit-testable on any host.
// Assigning SysProcAttr.CmdLine is the only step that needs a Windows-only
// field, so it's the single piece split out behind a build tag; every other
// platform gets the no-op in os_default_platform.go.
func setRawCmdLine(cmd *exec.Cmd, cmdLine string) {
if cmd.SysProcAttr == nil {
cmd.SysProcAttr = &syscall.SysProcAttr{}
}
cmd.SysProcAttr.CmdLine = cmdLine
}
func GetPlatform() *Platform {
return &Platform{
OS: "windows",

View file

@ -20,7 +20,7 @@ func TestOSCommandOpenFileWindows(t *testing.T) {
{
filename: "test",
runner: NewFakeRunner(t).
ExpectArgs([]string{"cmd", "/c", "start", "", "test"}, "", errors.New("error")),
ExpectArgs([]string{"cmd", "/s", "/c", `start "" "test"`}, "", errors.New("error")),
test: func(err error) {
assert.Error(t, err)
},
@ -28,7 +28,7 @@ func TestOSCommandOpenFileWindows(t *testing.T) {
{
filename: "test",
runner: NewFakeRunner(t).
ExpectArgs([]string{"cmd", "/c", "start", "", "test"}, "", nil),
ExpectArgs([]string{"cmd", "/s", "/c", `start "" "test"`}, "", nil),
test: func(err error) {
assert.NoError(t, err)
},
@ -36,7 +36,7 @@ func TestOSCommandOpenFileWindows(t *testing.T) {
{
filename: "filename with spaces",
runner: NewFakeRunner(t).
ExpectArgs([]string{"cmd", "/c", "start", "", "filename with spaces"}, "", nil),
ExpectArgs([]string{"cmd", "/s", "/c", `start "" "filename with spaces"`}, "", nil),
test: func(err error) {
assert.NoError(t, err)
},
@ -44,7 +44,7 @@ func TestOSCommandOpenFileWindows(t *testing.T) {
{
filename: "let's_test_with_single_quote",
runner: NewFakeRunner(t).
ExpectArgs([]string{"cmd", "/c", "start", "", "let's_test_with_single_quote"}, "", nil),
ExpectArgs([]string{"cmd", "/s", "/c", `start "" "let's_test_with_single_quote"`}, "", nil),
test: func(err error) {
assert.NoError(t, err)
},
@ -52,7 +52,7 @@ func TestOSCommandOpenFileWindows(t *testing.T) {
{
filename: "$USER.txt",
runner: NewFakeRunner(t).
ExpectArgs([]string{"cmd", "/c", "start", "", "$USER.txt"}, "", nil),
ExpectArgs([]string{"cmd", "/s", "/c", `start "" "$USER.txt"`}, "", nil),
test: func(err error) {
assert.NoError(t, err)
},