diff --git a/eval.go b/eval.go index 23a6be5e..ab8f1b01 100644 --- a/eval.go +++ b/eval.go @@ -368,6 +368,12 @@ func (e *setExpr) eval(app *app, args []string) { return } gOpts.shellopts = strings.Split(e.val, ":") + case "shellcmd": + if e.val == "" { + gOpts.shellcmd = nil + return + } + gOpts.shellcmd = tokenize(e.val) case "sortby": method := sortMethod(e.val) if !isValidSortMethod(method) { diff --git a/misc.go b/misc.go index d877d642..b9df2d25 100644 --- a/misc.go +++ b/misc.go @@ -108,8 +108,31 @@ func unescape(s string) string { } // This function splits the given string by whitespaces. It is aware of escaped -// whitespaces so that they are not split unintentionally. +// whitespaces and quoted strings so that they are not split unintentionally. func tokenize(s string) []string { + quoted := false + esc := false + toks := strings.FieldsFunc(s, func(r rune) bool { + if r == '\'' || r == '"' || r == '`' { + quoted = !quoted + } + if esc { + esc = false + return false + } + if r == '\\' { + esc = true + return false + } + return !quoted && unicode.IsSpace(r) + }) + if len(toks) == 0 { + toks = append(toks, s) + } + return toks +} + +func tokenize_old(s string) []string { esc := false var buf []rune var toks []string diff --git a/misc_test.go b/misc_test.go index f4095603..ef0ba521 100644 --- a/misc_test.go +++ b/misc_test.go @@ -188,11 +188,12 @@ func TestTokenize(t *testing.T) { for _, test := range tests { if got := tokenize(test.s); !reflect.DeepEqual(got, test.exp) { - t.Errorf("at input '%v' expected '%v' but got '%v'", test.s, test.exp, got) + t.Errorf("at input '%#v' expected '%#v' but got '%#v'", test.s, test.exp, got) } } } + func TestSplitWord(t *testing.T) { tests := []struct { s string diff --git a/opts.go b/opts.go index 50db7b15..24d568a3 100644 --- a/opts.go +++ b/opts.go @@ -94,6 +94,7 @@ var gOpts struct { rulerfmt string preserve []string shellopts []string + shellcmd []string keys map[string]expr cmdkeys map[string]expr cmds map[string]expr @@ -240,6 +241,7 @@ func init() { gOpts.rulerfmt = " %a| %p| \033[7;31m %m \033[0m| \033[7;33m %c \033[0m| \033[7;35m %s \033[0m| \033[7;34m %f \033[0m| %i/%t" gOpts.preserve = []string{"mode"} gOpts.shellopts = nil + gOpts.shellcmd = nil gOpts.tempmarks = "'" gOpts.numberfmt = "\033[33m" gOpts.tagfmt = "\033[31m" diff --git a/os.go b/os.go index ac2d2f60..1b83ed92 100644 --- a/os.go +++ b/os.go @@ -135,16 +135,40 @@ func detachedCommand(name string, arg ...string) *exec.Cmd { return cmd } +func shellCommand2(s string, args []string) *exec.Cmd { + var words []string + for _, word := range gOpts.shellcmd { + switch word { + case "%a": + words = append(words, args...) + case "%c": + words = append(words, s) + default: + words = append(words, word) + } + } + cmd := exec.Command(words[0], words[1:]...) + return cmd +} + func shellCommand(s string, args []string) *exec.Cmd { + if len(gOpts.ifs) != 0 { s = fmt.Sprintf("IFS='%s'; %s", gOpts.ifs, s) } + if len(gOpts.shellcmd) > 0 { + return shellCommand2(s, args) + } + + // original legacy configuration which uses shell, shellopts and shellflag + args = append([]string{gOpts.shellflag, s, "--"}, args...) args = append(gOpts.shellopts, args...) - return exec.Command(gOpts.shell, args...) + cmd := exec.Command(gOpts.shell, args...) + return cmd } func shellSetPG(cmd *exec.Cmd) { diff --git a/os_test.go b/os_test.go new file mode 100644 index 00000000..64c3a6fe --- /dev/null +++ b/os_test.go @@ -0,0 +1,374 @@ +package main + +import ( + "bytes" + "fmt" + "io" + "os" + "os/exec" + "runtime" + "strings" + "testing" + + "github.com/gdamore/tcell/v2" + "golang.org/x/sys/windows" +) + +func createTestApp() *app { + screen := tcell.NewSimulationScreen("UTF-8") + err := screen.Init() + if err != nil { + panic(err) + } + ui := newUI(screen) + nav := newNav(ui.wins[0].h) + app := newApp(ui, nav) + return app +} + +func readConfig(app *app, config_string string) { + filepath := "lfrc" + white := func() { + rc, err := os.Create(filepath) + if err != nil { + panic(err) + } + defer rc.Close() + + _, err = rc.WriteString(config_string) + if err != nil { + panic(err) + } + _, err = rc.Seek(0, io.SeekStart) + if err != nil { + panic(err) + } + } + white() + app.readFile(filepath) +} + +func evalShellExpr(app *app, exp string) string { + p := newParser(strings.NewReader(exp)) + for p.parse() { + old := os.Stdout // keep backup of the real stdout + r, w, _ := os.Pipe() + os.Stderr = w + defer func() { os.Stderr = old }() // restoring the real stdout + + outC := make(chan string) + // copy the output in a separate goroutine so printing can't block indefinitely + go func() { + var buf bytes.Buffer + _, err := io.Copy(&buf, r) + if err != nil { + panic(err) + } + outC <- buf.String() + }() + + p.expr.eval(app, nil) + + // back to normal state + w.Close() + out := <-outC + return out + } + if p.err != nil { + panic(p.err) + } + return "" +} + +func cdToTempDir(t *testing.T) { + t.Helper() + tmpDir := t.TempDir() + wd, err := os.Getwd() + _ = wd + if err != nil { + panic(err) + } + err = os.Chdir(tmpDir) + if err != nil { + panic(err) + } + + t.Cleanup(func() { + err = os.Chdir(wd) + if err != nil { + panic(err) + } + }) +} + +func commandExists(cmd string) bool { + _, err := exec.LookPath(cmd) + return err == nil +} + +type testCmd struct { + rc_cmd string + ui_cmd string +} + +type cmdShells struct { + posix testCmd + pwsh testCmd + cmd testCmd + nu testCmd +} + +func sameTestCmd(rc_cmd string, ui_cmd string) cmdShells { + c := testCmd{rc_cmd, ui_cmd} + return cmdShells{c, c, c, c} +} + +type expected struct { + args string + exp string +} + +var gShellCommandtests = []struct { + name string + cmd cmdShells + exp []expected +}{ + // { + // "lfdoc", + // sameTestCmd(`lf -doc`, `lf -doc`), + // []expected{{"", genDocString}}, + // }, + { + "test_echo", + cmdShells{ + posix: testCmd{`printf '%s\n' "$@"`, `printf '%s\n'`}, + pwsh: testCmd{`Write-Output $args`, `Write-Output`}, + cmd: testCmd{`".\test echo.bat"`, `".\test echo.bat"`}, + nu: testCmd{`print`, `print`}, + }, + []expected{ + {`a "b c"`, "a\nb c\n"}, + {`a "'b'"`, "a\n'b'\n"}, + {`"iam|not a pipe"`, "iam|not a pipe\n"}, + {`"I'm Special"`, "I'm Special\n"}, + // {`"%NotAppData%"`, "%NotAppData%\n"}, + // {`"^NotEscaped"`, "^NotEscaped\n"}, + // {`"(NotAGroup)"`, "(NotAGroup)\n"}, + // {`"a testoutput + // cmd mkdir $ mkdir -v ...$args + // cmd doc $ lf -doc | ignore + // cmd toggle2 $ lf -remote "send $env.id toggle" + // `) +} + + +// When user invokes cmd from command line he already escapes all arguments himself +// so no need to escape already escaped arg + + + + + +func TestMyCmd(t *testing.T) { + cases := []string{ + `argument1 "she said, "you had me at hello"" "\some\path with\spaces`, + // We need to escape " -> \" inside "..." + `argument1 "she said, \"you had me at hello\"" "\some\path with\spaces`, + + // also for cmd.exe + `argument1 ^"she said, \^"you had me at hello\^"^" ^"\some\path with\spaces`, + + `argument1 "argument"2" argument3 argument4`, + `argument1 "argument\"2" argument3 argument4`, + // for cmd + `argument1 ^"argument\^"2^" argument3 argument4`, + + `"\some\directory with\spaces\" argument2`, + // We need to escape '\' only if it follows by " \" -> \\" + `"\some\directory with\spaces\\" argument2`, + // for cmd + `^"\some\directory with\spaces\\^" argument2`, + + `"malicious argument\" &whoami"`, + + `^"malicious argument\^" ^&whoami^"`, + } + for _, c := range cases { + _ = c + // for _, i := range []string{"echo.bat", "cecho"} { + // _ = i + cmd := exec.Command("cmd.exe") + cmd.SysProcAttr = &windows.SysProcAttr{ + CmdLine: fmt.Sprintf(`/D /c printf "%%s\n" %s`, c), + } + cmd.Dir = `W:\` + fmt.Println(cmd.SysProcAttr) + rtn, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%v %v", rtn, err) + } + fmt.Println(string(rtn)) + // } + } +} + diff --git a/os_windows.go b/os_windows.go index db9ef562..7033ab97 100644 --- a/os_windows.go +++ b/os_windows.go @@ -118,7 +118,269 @@ func detachedCommand(name string, arg ...string) *exec.Cmd { return cmd } +// https://github.com/ActiveState/cli/pull/3389/files +// makeCmdLine builds a command line out of args by escaping "special" +// characters and joining the arguments with spaces. +// Based on syscall\exec_windows.go + +// https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way + +// way around os.EscapeArg is = SysProcAttr.CmdLine +// Set to the misexec application, but don't pass command line arguments +// cmd := exec.Command("msiexec") +// +// // Manually set the command line arguments so they are not escaped +// cmd.SysProcAttr = &syscall.SysProcAttr{ +// HideWindow: false, +// CmdLine: fmt.Sprintf(` /a "%v" TARGETDIR="%v"`, msiFile, targetDir), // Leave a space at the beginning +// CreationFlags: 0, +// } + +// https://github.com/golang/go/issues/15566 syscall: exec_windows.go: arguments should not be escaped to work with msiexec +// argv windows https://daviddeley.com/autohotkey/parameters/parameters.htm#WIN the best source we have on this topic. + +// extensive testing here https://github.com/sergeymakinen/go-quote/blob/main/windows/argv.go + +// https://github.com/golang/go/issues/17149 os/exec: Cannot execute command with space in the name on Windows, when there are parameters. Big discussion around cmd + +// Go encodes child process parameters in a way that is understood by most programs. Go uses rules similar to what CommandLineToArgvW implements. +// +// Unfortunately, your child process is cmd.exe (cmd.exe is called to execute the batch file you've requested). And cmd.exe parses its input parameters differently. + +// manually set cmd: +// command to execute, may contain quotes, backslashes and spaces +// var commandLine = `"C:\Program Files\echo.bat" "hello friend"` +// +// var comSpec = os.Getenv("COMSPEC") +// if comSpec == "" { +// comSpec = os.Getenv("SystemRoot") + "\\System32\\cmd.exe" +// } +// childProcess = exec.Command(comSpec) +// childProcess.SysProcAttr = &syscall.SysProcAttr{CmdLine: comSpec + " /C \"" + commandLine + "\""} +// +// // Then execute and read the output +// out, _ := childProcess.CombinedOutput() +// fmt.Printf("Output: %s", out) + +// func makeCmd(name string, args ...string) (*exec.Cmd, error) { +// if len(args) == 0 { +// return exec.Command(name), nil +// } +// +// name, err := exec.LookPath(name) +// if err != nil { +// return nil, err +// } +// +// if !isBatchFile(name) { +// return exec.Command(name, args...), nil +// } +// +// argsEscaped := make([]string, len(args)+1) +// argsEscaped[0] = syscall.EscapeArg(name) +// for i, a := range args { +// argsEscaped[i+1] = syscall.EscapeArg(a) +// } +// +// shell := os.Getenv("COMSPEC") +// if shell == "" { +// shell = "cmd.exe" // Will be expanded by exec.LookPath in exec.Command +// } +// +// cmd := exec.Command(shell) +// cmd.Args = nil +// cmd.SysProcAttr = &syscall.SysProcAttr{ +// CmdLine: fmt.Sprintf(`%s /c "%s"`, syscall.EscapeArg(cmd.Path), strings.Join(argsEscaped, " ")), +// } +// +// return cmd, nil +// } +// +// func isBatchFile(path string) bool { +// ext := filepath.Ext(path) +// return strings.EqualFold(ext, ".bat") || strings.EqualFold(ext, ".cmd") +// } + +// +// Empirically, I can also confirm that if I force golang to pass nil for lpApplicationName here instead of argv0p, executing exec.Command(`C:\Program Files\echo.bat`, "hello world") works without resorting to using the SysProcAttr escape hatch. +// +// All that said: +// +// Specifying lpApplicationName for CreateProcess is cited as a security best-practice as a fail-safe should the caller fail to add quotes around the path of the executable in lpCommandLine. FWIW, dotnet ensures that the executable is quoted for this very reason, and it turns out golang does, too, via makeCmdLine's use of appendEscapeArg. +// I can't find any official documentation around this subtle behavior difference with CreateProcess as to why it works when NOT specifying lpApplicationName, so at this point it's all circumstantial. All I was able to find is someone else pointing out this suble behavioral difference way back in 2001. +// It's entirely possible I'm glossing over some other important detail. I'm stabbing in the dark at this point. 😅 + +// It still exists in golang 1.20, e.g: +// cmd.exe /c copy/b "input 1.ts"+"input 2.ts" ouput.ts +// I guess that golang can automatically add double quotes to paths that contain Spaces, but many commands have their own coding rules, such as copy/b. The "+" in copy/b means concatenating the input files, but golang cannot parse it and cannot add double quotes to paths of input files that contain Spaces correctly. +// + +// TODO: check this https://github.com/otm/gluash + +// https://github.com/golang/go/issues/68313 syscall/exec_windows.go: appendEscapeArg does not escape all necessary characters +// some related commits to go https://go-review.googlesource.com/c/go/+/30947 + +// https://github.com/golang/go/issues/27199 os/exec: execution of batch-files (.cmd/.bat) is vulnerable in go-lang for windows / insufficient escape + +// some workarounds = https://github.com/ActiveState/cli/pull/3389 adn functions for proper escapings + +// golang lib https://github.com/golang/go/blob/master/src/syscall/exec_windows.go#L84 + +// Based on https://github.com/sebres/PoC/blob/master/SB-0D-001-win-exec/SOLUTION.md#definition + +// TODO: syscall\exec_windows.go +// and https://github.com/ActiveState/cli/pull/3389/files +// and https://learn.microsoft.com/en-us/archive/blogs/twistylittlepassagesallalike/everyone-quotes-command-line-arguments-the-wrong-way +func escapeArg(s string) string { + const argvUnsafeChars = "\t \"<>&|^!()%" + if len(s) == 0 { + return `""` + } + + needsBackslash := strings.ContainsAny(s, `"\`) + // Based on https://github.com/sebres/PoC/blob/master/SB-0D-001-win-exec/SOLUTION.md#definition + //"\t \"" + needsQuotes := strings.ContainsAny(s, "\t \"<>&|^!()%") + + if !needsBackslash && !needsQuotes { + // No special handling required; normal case. + return s + } + + if !needsBackslash { + // just needsQuotes + return `"` + s + `"` + } + + var ( + buf strings.Builder + slashes int + ) + if needsQuotes { + buf.WriteByte('"') + } + for i := 0; i < len(s); i++ { + switch s[i] { + case '"': + for slashes++; slashes > 0; slashes-- { + buf.WriteByte('\\') + } + buf.WriteByte(s[i]) + case '\\': + slashes++ + buf.WriteByte(s[i]) + default: + slashes = 0 + buf.WriteByte(s[i]) + } + } + + if needsQuotes { + for ; slashes > 0; slashes-- { + buf.WriteByte('\\') + } + buf.WriteByte('"') + } + + return buf.String() +} + +// https://ss64.com/nt/syntax-esc.html +var cmdQuoteReplacer = strings.NewReplacer( + "\t", "^\t", + // TODO: do we need to escape Spaces??? + " ", "^ ", + "!", "^!", + "&", "^&", + "'", "^'", + `"`, `""`, + "+", "^+", + ",", "^,", + ";", "^;", + "<", "^<", + "|", "^|", + "=", "^=", + ">", "^>", + "[", "^[", + "]", "^]", + "^", "^^", + "`", "^`", + "{", "^{", + "}", "^}", + "~", "^~", +) + +func escapeCmd(s string) string { + const cmdUnsafeChars = "!\"&'+,;<=>[]^`{}~" + if strings.ContainsAny(s, cmdUnsafeChars) { + s = cmdQuoteReplacer.Replace(s) + } + return s +} + +// TODO: I we ran a command from UI it passes as entire string in `s` +// without args. Should we also escape `s`?? + +func shellCommand2(s string, args []string) *exec.Cmd { + for i := 0; i < len(args); i++ { + args[i] = escapeArg(args[i]) + } + + // s = escapeArg(s) + + // Windows CMD requires special handling to deal with quoted arguments + exeName := filepath.Base(strings.ToLower(gOpts.shellcmd[0])) + isCmd := exeName == "cmd" || strings.HasSuffix(exeName, ".bat") || strings.HasSuffix(exeName, ".cmd") + if isCmd { + // Go currently does not escape arguments properly on Windows, it account for spaces and tab characters, but not + // other characters that need escaping such as `<` and `>`. + // This can be dropped once we update to a Go version that fixes this bug: https://github.com/golang/go/issues/68313 + for i := 0; i < len(args); i++ { + args[i] = escapeCmd(args[i]) + } + + // in case the command has some malicious characters such as & + // TODO: or it should be handled by the user when they type the command from the UI?? + s = `"`+escapeCmd(s)+`"` + } + + var words []string + for _, word := range gOpts.shellcmd { + switch word { + case "%c": + words = append(words, s) + case "%a": + words = append(words, args...) + default: + words = append(words, word) + } + } + + cmd := exec.Command(words[0], words[1:]...) + + if true { + // If we have to deal with a different from Argv command-line quoting + // when starting processes on Windows, we need to to manually create a command-line + // via the CmdLine SysProcAttr + + cmd.SysProcAttr = &windows.SysProcAttr{ + CmdLine: strings.Join(words[1:], " "), + } + + log.Printf("- %v\n", cmd.SysProcAttr) + return cmd + } + log.Printf("- %v\n", cmd.Args) + return cmd +} + func shellCommand(s string, args []string) *exec.Cmd { + if len(gOpts.shellcmd) > 0 { + return shellCommand2(s, args) + } + + // original legacy configuration which uses shell, shellopts and shellflag + // Windows CMD requires special handling to deal with quoted arguments if strings.ToLower(gOpts.shell) == "cmd" { var builder strings.Builder @@ -131,12 +393,15 @@ func shellCommand(s string, args []string) *exec.Cmd { cmd := exec.Command(gOpts.shell) cmd.SysProcAttr = &windows.SysProcAttr{CmdLine: cmdline} + return cmd } args = append([]string{gOpts.shellflag, s}, args...) args = append(gOpts.shellopts, args...) - return exec.Command(gOpts.shell, args...) + cmd := exec.Command(gOpts.shell, args...) + + return cmd } func shellSetPG(cmd *exec.Cmd) { diff --git a/os_windows_test.go b/os_windows_test.go new file mode 100644 index 00000000..b540d035 --- /dev/null +++ b/os_windows_test.go @@ -0,0 +1,95 @@ +//go:build windows +// +build windows + +package main + +import ( + "fmt" + "os/exec" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +type EscapeTest struct { + args []string + expArgv string + expCmd string +} + +var escapeTests = []EscapeTest{ + {[]string{``}, "\"\"", `^"^"`}, + { + []string{`argument1`, `she said, "you had me at hello"`, `"\some\path with\spaces`}, + `argument1 "she said, \"you had me at hello\"" "\"\some\path with\spaces"`, + `argument1 ^"she^ said^,^ \^"you^ had^ me^ at^ hello\^"^" ^"\^"\some\path^ with\spaces^"`, + }, + { + []string{`argument1`, `argument"2`, `argument3`, `argument4`}, + `argument1 "argument\"2" argument3 argument4`, + `argument1 ^"argument\^"2^" argument3 argument4`, + }, + { + []string{`\some\directory with\spaces\`, `argument2`}, + `"\some\directory with\spaces\\" argument2`, + `^"\some\directory^ with\spaces\\^" argument2`, + }, + { + []string{`malicious argument" &whoami`}, + `"malicious argument\" &whoami"`, + `^"malicious^ argument\^"^ ^&whoami^"`, + }, +} + +func TestEscapeArg(t *testing.T) { + for _, test := range escapeTests { + t.Run("escapeArg", func(t *testing.T) { + t.Run("Argv", func(t *testing.T) { + var args []string + for _, a := range test.args { + args = append(args, escapeArg(a)) + } + arg := strings.Join(args, " ") + if arg != test.expArgv { + t.Errorf("expected `%#v` but got `%#v`", test.expArgv, arg) + } + }) + t.Run("Cmd", func(t *testing.T) { + var args []string + for _, a := range test.args { + args = append(args, escapeCmd(escapeArg(a))) + } + arg := strings.Join(args, " ") + if arg != test.expCmd { + t.Errorf("expected %#v but got %#v", test.expCmd, arg) + } + }) + }) + } +} + +func runCmd(c string, t *testing.T) { + t.Helper() + cmd := exec.Command("cmd.exe") + cmd.SysProcAttr = &windows.SysProcAttr{ + CmdLine: fmt.Sprintf(`/D /c cecho %s`, c), + } + cmd.Dir = `W:\` + rtn, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("%v %v", rtn, err) + } + fmt.Println(string(rtn)) +} + +func TestMyCmd2(t *testing.T) { + for _, test := range escapeTests { + args := []string{} + for _, arg := range test.args { + args = append(args, escapeCmd(escapeArg(arg))) + } + c := strings.Join(args, " ") + runCmd(c, t) + } +}