feat: add flag for Windows to create unix compatible filepaths

coder · Emyrk · Jun 22, 2023 · Jun 22, 2023 · Jun 22, 2023 · Jun 22, 2023
commit 4ac653cbf9f3434c0ea06240c6415cf735185e95
diff --git a/cli/configssh.go b/cli/configssh.go
@@ -192,11 +192,12 @@ func sshPrepareWorkspaceConfigs(ctx context.Context, client *codersdk.Client) (r
 //nolint:gocyclo
 func (r *RootCmd) configSSH() *clibase.Cmd {
 	var (
-		sshConfigFile    string
-		sshConfigOpts    sshConfigOptions
-		usePreviousOpts  bool
-		dryRun           bool
-		skipProxyCommand bool
+		sshConfigFile       string
+		sshConfigOpts       sshConfigOptions
+		usePreviousOpts     bool
+		dryRun              bool
+		skipProxyCommand    bool
+		forceUnixSeparators bool
 	)
 	client := new(codersdk.Client)
 	cmd := &clibase.Cmd{
@@ -236,13 +237,13 @@ func (r *RootCmd) configSSH() *clibase.Cmd {
 			if err != nil {
 				return err
 			}
-			escapedCoderBinary, err := sshConfigExecEscape(coderBinary)
+			escapedCoderBinary, err := sshConfigExecEscape(coderBinary, forceUnixSeparators)
 			if err != nil {
 				return xerrors.Errorf("escape coder binary for ssh failed: %w", err)
 			}
 
 			root := r.createConfig()
-			escapedGlobalConfig, err := sshConfigExecEscape(string(root))
+			escapedGlobalConfig, err := sshConfigExecEscape(string(root), forceUnixSeparators)
 			if err != nil {
 				return xerrors.Errorf("escape global config for ssh failed: %w", err)
 			}
@@ -540,6 +541,19 @@ func (r *RootCmd) configSSH() *clibase.Cmd {
 			Default:     "auto",
 			Value:       clibase.EnumOf(&sshConfigOpts.waitEnum, "yes", "no", "auto"),
 		},
+		{
+			Flag: "force-unix-filepaths",
+			Env:  "CODER_CONFIGSSH_UNIX_FILEPATHS",
+			Description: "By default, this command uses the os path separator when writing the ssh config. " +
+				"This might be an issue in Windows machine that use a unix-like shell. " +
+				"This flag forces the use of unix filepaths (the forward slash '/').",
+			Value: clibase.BoolOf(&forceUnixSeparators),
+			// On non-windows showing this command is useless because it is a noop.
+			// Hide vs disable it though so if a command is copied from a Windows
+			// machine to a unix machine it will still work and not throw an
+			// "unknown flag" error.
+			Hidden: hideForceUnixSlashes,
+		},
 		cliui.SkipPromptOption(),
 	}
 
@@ -727,7 +741,27 @@ func writeWithTempFileAndMove(path string, r io.Reader) (err error) {
 //   - https://github.com/openssh/openssh-portable/blob/V_9_0_P1/sshconnect.c#L158-L167
 //   - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/sshconnect.c#L231-L293
 //   - https://github.com/PowerShell/openssh-portable/blob/v8.1.0.0/contrib/win32/win32compat/w32fd.c#L1075-L1100
-func sshConfigExecEscape(path string) (string, error) {
+//
+// Additional Windows-specific notes:
+//
+// In some situations a Windows user could be using a unix-like shell such as
+// git bash. In these situations the coder.exe is using the windows filepath
+// separator (\), but the shell wants the unix filepath separator (/).
+// Trying to determine if the shell is unix-like is difficult, so this function
+// takes the argument 'forceUnixPath' to force the filepath to be unix-like.
+//
+// On actual unix machines, this is **always** a noop. Even if a windows
+// path is provided.
+//
+// Passing a "false" for forceUnixPath will result in the filepath separator
+// untouched from the original input.
+func sshConfigExecEscape(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 (/).
+		path = filepath.ToSlash(path)
+	}
+
 	// 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") {

diff --git a/cli/configssh_internal_test.go b/cli/configssh_internal_test.go
@@ -140,14 +140,14 @@ func Test_sshConfigExecEscape(t *testing.T) {
 		name    string
 		path    string
 		wantErr bool
-		windows bool
 	}{
-		{"no spaces", "simple", false, true},
-		{"spaces", "path with spaces", false, true},
-		{"quotes", "path with \"quotes\"", false, false},
-		{"backslashes", "path with \\backslashes", false, false},
-		{"tabs", "path with \ttabs", false, false},
-		{"newline fails", "path with \nnewline", true, false},
+		{"windows path", `C:\Program Files\Coder\bin\coder.exe`, false},
+		{"no spaces", "simple", false},
+		{"spaces", "path with spaces", false},
+		{"quotes", "path with \"quotes\"", false},
+		{"backslashes", "path with \\backslashes", false},
+		{"tabs", "path with \ttabs", false},
+		{"newline fails", "path with \nnewline", true},
 	}
 	for _, tt := range tests {
 		tt := tt
@@ -166,7 +166,7 @@ func Test_sshConfigExecEscape(t *testing.T) {
 			err = os.WriteFile(bin, contents, 0o755) //nolint:gosec
 			require.NoError(t, err)
 
-			escaped, err := sshConfigExecEscape(bin)
+			escaped, err := sshConfigExecEscape(bin, false)
 			if tt.wantErr {
 				require.Error(t, err)
 				return
@@ -181,6 +181,70 @@ func Test_sshConfigExecEscape(t *testing.T) {
 	}
 }
 
+func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		path string
+		// Behavior is different on Windows
+		expWindowsPath string
+		expOtherPath   string
+		forceUnix      bool
+		wantErr        bool
+	}{
+		{
+			name: "windows_keep_forward_slashes_with_spaces",
+			// Has a space, expect quotes
+			path:           `C:\Program Files\Coder\bin\coder.exe`,
+			expWindowsPath: `"C:\Program Files\Coder\bin\coder.exe"`,
+			expOtherPath:   `"C:\Program Files\Coder\bin\coder.exe"`,
+			forceUnix:      false,
+			wantErr:        false,
+		},
+		{
+			name:           "windows_keep_forward_slashes",
+			path:           `C:\ProgramFiles\Coder\bin\coder.exe`,
+			expWindowsPath: `C:\ProgramFiles\Coder\bin\coder.exe`,
+			expOtherPath:   `C:\ProgramFiles\Coder\bin\coder.exe`,
+			forceUnix:      false,
+			wantErr:        false,
+		},
+		{
+			name:           "windows_force_unix_with_spaces",
+			path:           `C:\Program Files\Coder\bin\coder.exe`,
+			expWindowsPath: `"C:/Program Files/Coder/bin/coder.exe"`,
+			expOtherPath:   `"C:\Program Files\Coder\bin\coder.exe"`,
+			forceUnix:      true,
+			wantErr:        false,
+		},
+		{
+			name:           "windows_force_unix",
+			path:           `C:\ProgramFiles\Coder\bin\coder.exe`,
+			expWindowsPath: `C:/ProgramFiles/Coder/bin/coder.exe`,
+			expOtherPath:   `C:\ProgramFiles\Coder\bin\coder.exe`,
+			forceUnix:      true,
+			wantErr:        false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			found, err := sshConfigExecEscape(tt.path, tt.forceUnix)
+			if tt.wantErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			if runtime.GOOS == "windows" {
+				require.Equal(t, tt.expWindowsPath, found, "(Windows) expected path")
+			} else {
+				// this is a noop on non-windows!
+				require.Equal(t, tt.expOtherPath, found, "(Non-Windows) expected path")
+			}
+		})
+	}
+}
+
 func Test_sshConfigOptions_addOption(t *testing.T) {
 	t.Parallel()
 	testCases := []struct {

diff --git a/cli/configssh_other.go b/cli/configssh_other.go
@@ -0,0 +1,5 @@
+//go:build !windows
+
+package cli
+
+const hideForceUnixSlashes = true
diff --git a/cli/configssh_windows.go b/cli/configssh_windows.go
@@ -0,0 +1,5 @@
+//go:build windows
+
+package cli
+
+const hideForceUnixSlashes = false