- Notifications
You must be signed in to change notification settings - Fork928
fix: Use smarter quoting for ProxyCommand in config-ssh#3755
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
cli/configssh.go Outdated
| func sshConfigExecEscape(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) | ||
| } | ||
| // In the unlikely even 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, "\"", "\\\"") | ||
| } | ||
| // 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") { | ||
| path = fmt.Sprintf("\"%s\"", path) | ||
| } | ||
| return path, nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
So --- and hear me out here 😳 --- could we test this by creating a hello-world batch or shell script (platform-dependent) under a temp path that contains a space, attempting to execute/bin/bash -c '<path of thing>', and validating that it executes successfully?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see why not, good idea! In theory, we'd want to test all the shells.. but then again, I think we're unlikely to run into that many edge cases with this unless people do really fringe stuff with their filesystems/folder structures. 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think testing/bin/sh and/orcmd.exe is all we should do tbh
a5ae539 to73e6a84CompareThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
🚀
This change takes better into account how OpenSSH executes
ProxyCommands and applies quoting accordingly.This supercedes#3664, which was reverted.
Fixes#2853
Bonus commit:
~/.sshdirectory exists