Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check the path for spaces before sending. Avoids filenames to be inte… #56966

Merged
merged 6 commits into from Sep 20, 2018

Conversation

Projects
None yet
5 participants
@luiszun
Copy link
Contributor

luiszun commented Aug 22, 2018

Check the path for spaces before sending. Avoids filenames to be interpreted as file and arguments

Closes #56109

@@ -638,7 +638,11 @@ export class RunActiveFileInTerminalAction extends Action {
this.notificationService.warn(nls.localize('workbench.action.terminal.runActiveFile.noFile', 'Only files on disk can be run in the terminal'));
return TPromise.as(void 0);
}
instance.sendText(uri.fsPath, true);
let uriPath: string = uri.fsPath;
if (uriPath.indexOf(' ') !== -1) {

This comment has been minimized.

@Tyriar

Tyriar Aug 23, 2018

Member

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.

This comment has been minimized.

@luiszun

luiszun Aug 31, 2018

Author Contributor

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!

This comment has been minimized.

@Tyriar

Tyriar Aug 31, 2018

Member

Mac allows it, just testing Linux and treating mac the same should be enough 👍

This comment has been minimized.

@alexr00

alexr00 Sep 14, 2018

Member

Looks like on Linux space can also be dealt with by surrounding with single quotes.

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Sep 14, 2018

I tested using WSL and this path: ~tes"t/test script.bash

@luiszun

This comment has been minimized.

Copy link
Contributor Author

luiszun commented Sep 14, 2018

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 + '\'';

This comment has been minimized.

@Tyriar

Tyriar Sep 14, 2018

Member

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(' ', '\\ ');

This comment has been minimized.

@Tyriar

Tyriar Sep 17, 2018

Member

.replace only replaces the first instance.

I think we also need to handle \\ -> \\\\

@msftclas

This comment has been minimized.

Copy link

msftclas commented Sep 18, 2018

CLA assistant check
All CLA requirements met.

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Sep 19, 2018

@luiszun could you please sign the CLA mentioned in #56966 (comment) so we can get this merged?

@Tyriar

Tyriar approved these changes Sep 19, 2018

@Tyriar

This comment has been minimized.

Copy link
Member

Tyriar commented Sep 19, 2018

Looks good to me, just waiting on the CLA

@luiszun

This comment has been minimized.

Copy link
Contributor Author

luiszun commented Sep 20, 2018

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

@alexr00 alexr00 merged commit 5867dc7 into Microsoft:master Sep 20, 2018

2 checks passed

VS Code #20180918.72 succeeded with issues
Details
license/cla All CLA requirements met.
Details

@Tyriar Tyriar added this to the September 2018 milestone Sep 21, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.