Check the path for spaces before sending. Avoids filenames to be inte…#56966
Check the path for spaces before sending. Avoids filenames to be inte…#56966alexr00 merged 6 commits intomicrosoft:masterfrom
Conversation
…rpreted as file and arguments
| } | ||
| instance.sendText(uri.fsPath, true); | ||
| let uriPath: string = uri.fsPath; | ||
| if (uriPath.indexOf(' ') !== -1) { |
There was a problem hiding this comment.
A space on Mac/Linux should be escaped with a \\ in front of it.
We should also consider and handle paths that contain the " character.
There was a problem hiding this comment.
The " character is not allowed in Windows.
I thought it was the same in Unixes, but you're right! Some shells will not allow it, but at least the major filesystems allow it (ext3, ext4). I'll get educated on Mac, and I'll change the PR.
Thanks!
There was a problem hiding this comment.
Mac allows it, just testing Linux and treating mac the same should be enough 👍
There was a problem hiding this comment.
Looks like on Linux space can also be dealt with by surrounding with single quotes.
|
I tested using WSL and this path: ~tes"t/test script.bash |
|
I thought of doing the single quote thing, but I found out that some filesystems (ext4 being a very widely used one) allow all characters except '' and null. Though I doubt escaping non-printable characters is a practical solution. I think that for all practical purposes, the single quote suffices. Just wanted to drop this piece of data :) |
| if (isWindows) { | ||
| uriPath = '"' + uriPath + '"'; | ||
| } else { | ||
| uriPath = '\'' + uriPath + '\''; |
There was a problem hiding this comment.
Escaping on macOS and Linux would probably be better done by escaping each space, this is more typical than quoting the whole path. This is what happens when you drag a file in that contains spaces: /Users/daimms/Desktop/Screen\ Shot\ 2018-09-14\ at\ 12.39.14\ PM.png
| if (!hasSpace && hasDoubleQuote) { | ||
| uriPath = '\'' + uriPath + '\''; | ||
| } else if (hasSpace) { | ||
| uriPath = uriPath.replace(' ', '\\ '); |
There was a problem hiding this comment.
.replace only replaces the first instance.
I think we also need to handle \\ -> \\\\
|
|
|
@luiszun could you please sign the CLA mentioned in #56966 (comment) so we can get this merged? |
|
Looks good to me, just waiting on the CLA |
|
Supposedly I didn't have to sign the CLA if I linked my Microsoft corp account to this profile. Anyway, just signed it. Sorry for the delay |
5 participants
Check the path for spaces before sending. Avoids filenames to be interpreted as file and arguments
Closes #56109