Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

fix: handle paths with spaces in Match exec clause of SSH config#18266

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
spikecurtis merged 5 commits intomainfromspike/18199-path-with-spaces
Jun 6, 2025
Merged
Show file tree
Hide file tree
Changes fromall commits
Commits
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletionscli/configssh.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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 forssh failed: %w", err)
return xerrors.Errorf("escape coder binary forProxyCommand 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)
Expand DownExpand Up@@ -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")
}
Expand All@@ -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")
}
Expand DownExpand Up@@ -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)`.
Expand DownExpand Up@@ -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
funcsshConfigExecEscape(path string, forceUnixPath bool) (string, error) {
funcsshConfigProxyCommandEscape(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 (/).
Expand All@@ -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 unlikelyeven that a path contains quotes, they must be
// In the unlikelyevent 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, "\"", "\\\"")
Expand Down
63 changes: 60 additions & 3 deletionscli/configssh_internal_test.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
funcTest_sshConfigExecEscape(t *testing.T) {
funcTest_sshConfigProxyCommandEscape(t *testing.T) {
t.Parallel()

tests := []struct {
Expand DownExpand Up@@ -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
Expand All@@ -186,6 +186,63 @@ 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
wantErrOther bool
wantErrWindows bool
}{
{"no spaces", "simple", false, false},
{"spaces", "path with spaces", false, false},
{"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},
}
// 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)
bin := filepath.Join(dir, "coder.bat") // Windows will treat it as batch, Linux doesn't care
escaped, err := sshConfigMatchExecEscape(bin)
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))
require.Equal(t, "yay", got)
})
}
}

func Test_sshConfigExecEscapeSeparatorForce(t *testing.T) {
t.Parallel()

Expand DownExpand Up@@ -236,7 +293,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
Expand Down
31 changes: 31 additions & 0 deletionscli/configssh_other.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,4 +2,35 @@

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, 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
}
53 changes: 53 additions & 0 deletionscli/configssh_windows.go
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -2,5 +2,58 @@

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
// - 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 on Windows.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Thanks for the great write-up! ❤️

QQ: Is there a chance the "default shell" that OpenSSH uses can be configured to be powershell instead of cmd.exe? And if yes, would the for loop break?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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 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)
}

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

💫😵💫 %%%%%%%%%%%%%%%%%%%%%%%% 💫😵💫

}
return path, nil
}
Loading
[8]ページ先頭
©2009-2025 Movatter.jp