diff --git a/pkg/commands/oscommands/cmd_obj_builder.go b/pkg/commands/oscommands/cmd_obj_builder.go index fde64258210..9084fd994cf 100644 --- a/pkg/commands/oscommands/cmd_obj_builder.go +++ b/pkg/commands/oscommands/cmd_obj_builder.go @@ -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 ""`. 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) + } + 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('\\') } - return quote + message + quote + b.WriteByte('"') + return b.String() } diff --git a/pkg/commands/oscommands/new_shell_windows_test.go b/pkg/commands/oscommands/new_shell_windows_test.go new file mode 100644 index 00000000000..c7fe5aef116 --- /dev/null +++ b/pkg/commands/oscommands/new_shell_windows_test.go @@ -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 // 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 "" placeholder; it gets +// substituted with the resolved target file path before comparison. +func TestNewShell_VarietyOfEditorTemplates(t *testing.T) { + const filePlaceholder = "" + + cases := []struct { + name string + template string // stands for the fake editor's full path + line string + wantArgs []string + }{ + { + name: "vim/nvim style: +line filename", + template: `"" +{{line}} {{filename}}`, + line: "42", + wantArgs: []string{"+42", filePlaceholder}, + }, + { + name: "emacs-like with explicit +N", + template: `"" +{{line}} -nw {{filename}}`, + line: "7", + wantArgs: []string{"+7", "-nw", filePlaceholder}, + }, + { + name: "long flag with =value", + template: `"" --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: `"" -a -b -c --foo --bar -n{{line}} {{filename}}`, + line: "42", + wantArgs: []string{"-a", "-b", "-c", "--foo", "--bar", "-n42", filePlaceholder}, + }, + { + name: "flag after filename", + template: `"" {{filename}} --readonly`, + line: "", + wantArgs: []string{filePlaceholder, "--readonly"}, + }, + { + name: "single short flag attached to value", + template: `"" -n{{line}} {{filename}}`, + line: "1", + wantArgs: []string{"-n1", filePlaceholder}, + }, + { + name: "no flags, just filename", + template: `"" {{filename}}`, + line: "", + wantArgs: []string{filePlaceholder}, + }, + { + name: "flag with separate value (space-separated)", + template: `"" --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, "", 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) +} diff --git a/pkg/commands/oscommands/os_default_platform.go b/pkg/commands/oscommands/os_default_platform.go index 06684434e7e..5e73c994f30 100644 --- a/pkg/commands/oscommands/os_default_platform.go +++ b/pkg/commands/oscommands/os_default_platform.go @@ -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 diff --git a/pkg/commands/oscommands/os_test.go b/pkg/commands/oscommands/os_test.go index 54d9f3a80d4..1ccfbc02580 100644 --- a/pkg/commands/oscommands/os_test.go +++ b/pkg/commands/oscommands/os_test.go @@ -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 diff --git a/pkg/commands/oscommands/os_windows.go b/pkg/commands/oscommands/os_windows.go index 605ed768275..bd4cc515190 100644 --- a/pkg/commands/oscommands/os_windows.go +++ b/pkg/commands/oscommands/os_windows.go @@ -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", diff --git a/pkg/commands/oscommands/os_windows_test.go b/pkg/commands/oscommands/os_windows_test.go index 60ba495bf4e..495e23f7257 100644 --- a/pkg/commands/oscommands/os_windows_test.go +++ b/pkg/commands/oscommands/os_windows_test.go @@ -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) },