From 0862dd077ea8abd59fa107aba9f98832d5a312ac Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 6 Jun 2025 11:24:24 +0400 Subject: [PATCH 1/5] fix: fix handling coder path with spaces in config-ssh --- cli/configssh.go | 28 +++++++++++------- cli/configssh_internal_test.go | 4 +-- cli/configssh_windows.go | 54 ++++++++++++++++++++++++++++++++++ 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/cli/configssh.go b/cli/configssh.go index cfea6b377f6ee..c1be60b604a9e 100644 --- a/cli/configssh.go +++ b/cli/configssh.go @@ -112,14 +112,19 @@ func (o sshConfigOptions) equal(other sshConfigOptions) bool { } func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error { - escapedCoderBinary, err := sshConfigExecEscape(o.coderBinaryPath, o.forceUnixSeparators) + escapedCoderBinaryProxy, err := sshConfigProxyCommandEscape(o.coderBinaryPath, o.forceUnixSeparators) if err != nil { - return xerrors.Errorf("escape coder binary for ssh failed: %w", err) + return xerrors.Errorf("escape coder binary for ProxyCommand failed: %w", err) } - escapedGlobalConfig, err := sshConfigExecEscape(o.globalConfigPath, o.forceUnixSeparators) + escapedCoderBinaryMatchExec, err := sshConfigMatchExecEscape(o.coderBinaryPath) if err != nil { - return xerrors.Errorf("escape global config for ssh failed: %w", err) + return xerrors.Errorf("escape coder binary for Match exec failed: %w", err) + } + + escapedGlobalConfig, err := sshConfigProxyCommandEscape(o.globalConfigPath, o.forceUnixSeparators) + if err != nil { + return xerrors.Errorf("escape global config for ProxyCommand failed: %w", err) } rootFlags := fmt.Sprintf("--global-config %s", escapedGlobalConfig) @@ -155,7 +160,7 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error { _, _ = buf.WriteString("\t") _, _ = fmt.Fprintf(buf, "ProxyCommand %s %s ssh --stdio%s --ssh-host-prefix %s %%h", - escapedCoderBinary, rootFlags, flags, o.userHostPrefix, + escapedCoderBinaryProxy, rootFlags, flags, o.userHostPrefix, ) _, _ = buf.WriteString("\n") } @@ -174,11 +179,11 @@ func (o sshConfigOptions) writeToBuffer(buf *bytes.Buffer) error { // the ^^ options should always apply, but we only want to use the proxy command if Coder Connect is not running. if !o.skipProxyCommand { _, _ = fmt.Fprintf(buf, "\nMatch host *.%s !exec \"%s connect exists %%h\"\n", - o.hostnameSuffix, escapedCoderBinary) + o.hostnameSuffix, escapedCoderBinaryMatchExec) _, _ = buf.WriteString("\t") _, _ = fmt.Fprintf(buf, "ProxyCommand %s %s ssh --stdio%s --hostname-suffix %s %%h", - escapedCoderBinary, rootFlags, flags, o.hostnameSuffix, + escapedCoderBinaryProxy, rootFlags, flags, o.hostnameSuffix, ) _, _ = buf.WriteString("\n") } @@ -759,7 +764,8 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after [] return data, nil, nil, nil } -// sshConfigExecEscape quotes the string if it contains spaces, as per +// sshConfigProxyCommandEscape prepares the path for use in ProxyCommand. +// It quotes the string if it contains spaces, as per // `man 5 ssh_config`. However, OpenSSH uses exec in the users shell to // run the command, and as such the formatting/escape requirements // cannot simply be covered by `fmt.Sprintf("%q", path)`. @@ -804,7 +810,7 @@ func sshConfigSplitOnCoderSection(data []byte) (before, section []byte, after [] // This is a control flag, and that is ok. It is a control flag // based on the OS of the user. Making this a different file is excessive. // nolint:revive -func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) { +func sshConfigProxyCommandEscape(path string, forceUnixPath bool) (string, error) { if forceUnixPath { // This is a workaround for #7639, where the filepath separator is // incorrectly the Windows separator (\) instead of the unix separator (/). @@ -814,9 +820,9 @@ func sshConfigExecEscape(path string, forceUnixPath bool) (string, error) { // This is unlikely to ever happen, but newlines are allowed on // certain filesystems, but cannot be used inside ssh config. if strings.ContainsAny(path, "\n") { - return "", xerrors.Errorf("invalid path: %s", path) + return "", xerrors.Errorf("invalid path: %q", path) } - // In the unlikely even that a path contains quotes, they must be + // In the unlikely event that a path contains quotes, they must be // escaped so that they are not interpreted as shell quotes. if strings.Contains(path, "\"") { path = strings.ReplaceAll(path, "\"", "\\\"") diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index d3eee395de0a3..7fcc3eaad441d 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -171,7 +171,7 @@ func Test_sshConfigExecEscape(t *testing.T) { err = os.WriteFile(bin, contents, 0o755) //nolint:gosec require.NoError(t, err) - escaped, err := sshConfigExecEscape(bin, false) + escaped, err := sshConfigProxyCommandEscape(bin, false) if tt.wantErr { require.Error(t, err) return @@ -236,7 +236,7 @@ func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) { tt := tt t.Run(tt.name, func(t *testing.T) { t.Parallel() - found, err := sshConfigExecEscape(tt.path, tt.forceUnix) + found, err := sshConfigProxyCommandEscape(tt.path, tt.forceUnix) if tt.wantErr { require.Error(t, err) return diff --git a/cli/configssh_windows.go b/cli/configssh_windows.go index 642a388fc873c..d7fc9df5e3cf1 100644 --- a/cli/configssh_windows.go +++ b/cli/configssh_windows.go @@ -2,5 +2,59 @@ package cli +import ( + "fmt" + "strings" + + "golang.org/x/xerrors" +) + // Must be a var for unit tests to conform behavior var hideForceUnixSlashes = false + +// sshConfigMatchExecEscape prepares the path for use in `Match exec` statement. +// +// OpenSSH parses the Match line with a very simple tokenizer that accepts "-enclosed strings for the exec command, and +// has no supported escape sequences for ". This means we cannot include " within the command to execute. +// +// To make matters worse, on Windows, OpenSSH passes the string directly to cmd.exe for execution, and as far as I can +// tell, the only supported way to call a path that has spaces in it is to surround it with ". +// +// So, we can't actually include " directly, but here is a horrible workaround: +// +// "for /f %%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a connect exists %h" +// +// The key insight here is to store the character " in a variable (%a in this case, but the % itself needs to be +// escaped, so it becomes %%a), and then use that variable to construct the double-quoted path: +// +// %%aC:\Program Files\Coder\bin\coder.exe%%a. +// +// How do we generate a single " character without actually using that character? I couldn't find any command in cmd.exe +// to do it, but powershell.exe can convert ASCII to characters like this: `[char]34` (where 34 is the code point for "). +// +// Other notes: +// - @ in `@cmd.exe` suppresses echoing it, so you don't get this command printed +// - without another invocation of cmd.exe (e.g. `do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a`) then +// the double-quote gets interpreted as part of the path and you get: '"C:\Program' is not recognized. Constructing +// the string and then passing it to another instance of cmd.exe does this trick here. +// - OpenSSH passes the `Match exec` command to cmd.exe regardless of whether the user has a unix-like shell like +// git bash, so we don't have a `forceUnixPath` option like for the ProxyCommand which does respect the user's +// configured shell. +func sshConfigMatchExecEscape(path string) (string, error) { + // This is unlikely to ever happen, but newlines are allowed on + // certain filesystems, but cannot be used inside ssh config. + if strings.ContainsAny(path, "\n") { + return "", xerrors.Errorf("invalid path: %s", path) + } + // Windows does not allow double-quotes in paths. If we get one it is an error. + if strings.Contains(path, `"`) { + return "", xerrors.Errorf("path must not contain quotes: %q", path) + } + // A space or a tab requires quoting, but tabs must not be escaped + // (\t) since OpenSSH interprets it as a literal \t, not a tab. + if strings.ContainsAny(path, " \t") { + // c.f. function comment for how this works. + path = fmt.Sprintf("for /f %%%%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%%%a%s%%%%a", path) //nolint:gocritic // We don't want %q here. + } + return path, nil +} From c363edd337fe3f72911bcbcd2293b9223baae800 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 6 Jun 2025 09:33:14 +0000 Subject: [PATCH 2/5] add support for macOS & Linux --- cli/configssh_other.go | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/cli/configssh_other.go b/cli/configssh_other.go index fde7cc0e47e63..9f45f58e31adc 100644 --- a/cli/configssh_other.go +++ b/cli/configssh_other.go @@ -2,4 +2,34 @@ package cli +import ( + "strings" + + "golang.org/x/xerrors" +) + var hideForceUnixSlashes = true + +// sshConfigMatchExecEscape prepares the path for use in `Match exec` statement. +// +// OpenSSH parses the Match line with a very simple tokenizer that accepts "-enclosed strings for the exec command, and +// has no supported escape sequences for ". This means we cannot include " within the command to execute. +func sshConfigMatchExecEscape(path string) (string, error) { + // This is unlikely to ever happen, but newlines are allowed on + // certain filesystems, but cannot be used inside ssh config. + if strings.ContainsAny(path, "\n") { + return "", xerrors.Errorf("invalid path: %s", path) + } + // Quotes are allowed in path names on unix-like file systems, but OpenSSH's parsing of `Match exec` doesn't allow + // them. + if strings.Contains(path, `"`) { + return "", xerrors.Errorf("path must not contain quotes: %q", path) + } + + // OpenSSH passes the match exec string directly to the user's shell. sh, bash and zsh accept spaces and tabs + // simply escaped by a `\`. It's hard to predict exactly what more exotic shells might do, but this should work for + // macOS and most Linux distros in their default configuration. + path = strings.ReplaceAll(path, " ", "\\ ") + path = strings.ReplaceAll(path, "\t", "\\\t") + return path, nil +} From 970b11522cf4dd49427fdb8cec9be9071d1e844c Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 6 Jun 2025 09:51:07 +0000 Subject: [PATCH 3/5] add test, support backslashes --- cli/configssh_internal_test.go | 56 +++++++++++++++++++++++++++++++++- cli/configssh_other.go | 7 +++-- 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index 7fcc3eaad441d..3e9ad0d240343 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -139,7 +139,7 @@ func Test_sshConfigSplitOnCoderSection(t *testing.T) { // This test tries to mimic the behavior of OpenSSH // when executing e.g. a ProxyCommand. // nolint:tparallel -func Test_sshConfigExecEscape(t *testing.T) { +func Test_sshConfigProxyCommandEscape(t *testing.T) { t.Parallel() tests := []struct { @@ -186,6 +186,60 @@ func Test_sshConfigExecEscape(t *testing.T) { } } +// This test tries to mimic the behavior of OpenSSH +// when executing e.g. a match exec command. +// nolint:tparallel +func Test_sshConfigMatchExecEscape(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + path string + wantErr bool + }{ + {"no spaces", "simple", false}, + {"spaces", "path with spaces", false}, + {"quotes fails", "path with \"quotes\"", true}, + {"backslashes", "path with \\backslashes", false}, + {"tabs", "path with \ttabs", false}, + {"newline fails", "path with \nnewline", true}, + } + // nolint:paralleltest // Fixes a flake + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + cmd := "/bin/sh" + arg := "-c" + contents := []byte("#!/bin/sh\necho yay\n") + if runtime.GOOS == "windows" { + cmd = "cmd.exe" + arg = "/c" + contents = []byte("echo yay\n") + } + + dir := filepath.Join(t.TempDir(), tt.path) + err := os.MkdirAll(dir, 0o755) + require.NoError(t, err) + bin := filepath.Join(dir, "coder.bat") // Windows will treat it as batch, Linux doesn't care + + err = os.WriteFile(bin, contents, 0o755) //nolint:gosec + require.NoError(t, err) + + escaped, err := sshConfigMatchExecEscape(bin) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + + b, err := exec.Command(cmd, arg, escaped).CombinedOutput() //nolint:gosec + require.NoError(t, err) + got := strings.TrimSpace(string(b)) + require.Equal(t, "yay", got) + }) + } +} + func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) { t.Parallel() diff --git a/cli/configssh_other.go b/cli/configssh_other.go index 9f45f58e31adc..07417487e8c8f 100644 --- a/cli/configssh_other.go +++ b/cli/configssh_other.go @@ -26,9 +26,10 @@ func sshConfigMatchExecEscape(path string) (string, error) { return "", xerrors.Errorf("path must not contain quotes: %q", path) } - // OpenSSH passes the match exec string directly to the user's shell. sh, bash and zsh accept spaces and tabs - // simply escaped by a `\`. It's hard to predict exactly what more exotic shells might do, but this should work for - // macOS and most Linux distros in their default configuration. + // OpenSSH passes the match exec string directly to the user's shell. sh, bash and zsh accept spaces, tabs and + // backslashes simply escaped by a `\`. It's hard to predict exactly what more exotic shells might do, but this + // should work for macOS and most Linux distros in their default configuration. + path = strings.ReplaceAll(path, `\`, `\\`) // must be first, since later replacements add backslashes. path = strings.ReplaceAll(path, " ", "\\ ") path = strings.ReplaceAll(path, "\t", "\\\t") return path, nil From caee378e01d3780434dd6a06e162fc89f890ead5 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 6 Jun 2025 14:28:31 +0400 Subject: [PATCH 4/5] fixup test on Windows --- cli/configssh_internal_test.go | 37 ++++++++++++++++++---------------- cli/configssh_windows.go | 19 +++++++++-------- 2 files changed, 29 insertions(+), 27 deletions(-) diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index 3e9ad0d240343..f7748d9dfa038 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -193,16 +193,17 @@ func Test_sshConfigMatchExecEscape(t *testing.T) { t.Parallel() tests := []struct { - name string - path string - wantErr bool + name string + path string + wantErrOther bool + wantErrWindows bool }{ - {"no spaces", "simple", false}, - {"spaces", "path with spaces", false}, - {"quotes fails", "path with \"quotes\"", true}, - {"backslashes", "path with \\backslashes", false}, - {"tabs", "path with \ttabs", false}, - {"newline fails", "path with \nnewline", true}, + {"no spaces", "simple", false, false}, + {"spaces", "path with spaces", false, false}, + {"quotes", "path with \"quotes\"", false, true}, + {"backslashes", "path with\\backslashes", false, false}, + {"tabs", "path with \ttabs", false, true}, + {"newline fails", "path with \nnewline", true, true}, } // nolint:paralleltest // Fixes a flake for _, tt := range tests { @@ -214,24 +215,26 @@ func Test_sshConfigMatchExecEscape(t *testing.T) { if runtime.GOOS == "windows" { cmd = "cmd.exe" arg = "/c" - contents = []byte("echo yay\n") + contents = []byte("@echo yay\n") } dir := filepath.Join(t.TempDir(), tt.path) - err := os.MkdirAll(dir, 0o755) - require.NoError(t, err) bin := filepath.Join(dir, "coder.bat") // Windows will treat it as batch, Linux doesn't care - - err = os.WriteFile(bin, contents, 0o755) //nolint:gosec - require.NoError(t, err) - escaped, err := sshConfigMatchExecEscape(bin) - if tt.wantErr { + if (runtime.GOOS == "windows" && tt.wantErrWindows) || (runtime.GOOS != "windows" && tt.wantErrOther) { require.Error(t, err) return } require.NoError(t, err) + err = os.MkdirAll(dir, 0o755) + require.NoError(t, err) + + err = os.WriteFile(bin, contents, 0o755) //nolint:gosec + require.NoError(t, err) + + // OpenSSH processes %% escape sequences into % + escaped = strings.ReplaceAll(escaped, "%%", "%") b, err := exec.Command(cmd, arg, escaped).CombinedOutput() //nolint:gosec require.NoError(t, err) got := strings.TrimSpace(string(b)) diff --git a/cli/configssh_windows.go b/cli/configssh_windows.go index d7fc9df5e3cf1..5df0d6b50c00e 100644 --- a/cli/configssh_windows.go +++ b/cli/configssh_windows.go @@ -34,25 +34,24 @@ var hideForceUnixSlashes = false // // Other notes: // - @ in `@cmd.exe` suppresses echoing it, so you don't get this command printed -// - without another invocation of cmd.exe (e.g. `do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a`) then -// the double-quote gets interpreted as part of the path and you get: '"C:\Program' is not recognized. Constructing -// the string and then passing it to another instance of cmd.exe does this trick here. +// - we need another invocation of cmd.exe (e.g. `do @cmd.exe /c %%aC:\Program Files\Coder\bin\coder.exe%%a`). Without +// it the double-quote gets interpreted as part of the path, and you get: '"C:\Program' is not recognized. +// Constructing the string and then passing it to another instance of cmd.exe does this trick here. // - OpenSSH passes the `Match exec` command to cmd.exe regardless of whether the user has a unix-like shell like // git bash, so we don't have a `forceUnixPath` option like for the ProxyCommand which does respect the user's -// configured shell. +// configured shell on Windows. func sshConfigMatchExecEscape(path string) (string, error) { // This is unlikely to ever happen, but newlines are allowed on // certain filesystems, but cannot be used inside ssh config. if strings.ContainsAny(path, "\n") { return "", xerrors.Errorf("invalid path: %s", path) } - // Windows does not allow double-quotes in paths. If we get one it is an error. - if strings.Contains(path, `"`) { - return "", xerrors.Errorf("path must not contain quotes: %q", path) + // Windows does not allow double-quotes or tabs in paths. If we get one it is an error. + if strings.ContainsAny(path, "\"\t") { + return "", xerrors.Errorf("path must not contain quotes or tabs: %q", path) } - // A space or a tab requires quoting, but tabs must not be escaped - // (\t) since OpenSSH interprets it as a literal \t, not a tab. - if strings.ContainsAny(path, " \t") { + + if strings.ContainsAny(path, " ") { // c.f. function comment for how this works. path = fmt.Sprintf("for /f %%%%a in ('powershell.exe -Command [char]34') do @cmd.exe /c %%%%a%s%%%%a", path) //nolint:gocritic // We don't want %q here. } From 09692025375e0b3d18ef7579ead5117f33afe145 Mon Sep 17 00:00:00 2001 From: Spike Curtis Date: Fri, 6 Jun 2025 14:51:55 +0400 Subject: [PATCH 5/5] no quotes on non-Windows either --- cli/configssh_internal_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go index f7748d9dfa038..acf534e7ae157 100644 --- a/cli/configssh_internal_test.go +++ b/cli/configssh_internal_test.go @@ -200,7 +200,7 @@ func Test_sshConfigMatchExecEscape(t *testing.T) { }{ {"no spaces", "simple", false, false}, {"spaces", "path with spaces", false, false}, - {"quotes", "path with \"quotes\"", false, true}, + {"quotes", "path with \"quotes\"", true, true}, {"backslashes", "path with\\backslashes", false, false}, {"tabs", "path with \ttabs", false, true}, {"newline fails", "path with \nnewline", true, true},