fix: fix handling coder path with spaces in config-ssh

spikecurtis · spikecurtis · commit 69112fa5bf9f · 2025-06-06T11:24:58.000+04:00
diff --git 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
@@ -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
@@ -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
+}
