- Notifications
You must be signed in to change notification settings - Fork905
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
6f0ac31 tocaee378CompareThere 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.
Thanks for the research and docs here. Although this is indisputably gnarly, I much prefer it to the alternative of reducing functionality. Kudos on finding this hack 👍🏻
Please note QQ in inline comment, but other than that, approved!
| // 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. |
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.
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?
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've looked into this, and OpenSSH on Windows callssystem, which seems to always becmd.exe.
https://learn.microsoft.com/en-us/cpp/c-language/system-function?view=msvc-170
| ifstrings.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. |
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.
💫😵💫 %%%%%%%%%%%%%%%%%%%%%%%% 💫😵💫
d47a53d intomainUh oh!
There was an error while loading.Please reload this page.
fixes#18199
Corrects handling of paths with spaces in the
Match !execclause we use to determine whether Coder Connect is running. This is handled differently than the ProxyCommand, so we have a different escape routine, which also varies by OS.On Windows, we resort to a pretty gnarly hack, but it does work and I feel the only other option would be to reduce functionality such that we could not detect the Coder Connect state.