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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle URL and paths escaping in Windows #146

Merged
merged 4 commits into from Oct 15, 2019
Merged

Handle URL and paths escaping in Windows #146

merged 4 commits into from Oct 15, 2019

Conversation

herberttn
Copy link
Contributor

@herberttn herberttn commented Aug 14, 2019

This fixes an old problem that has been widely reported but haven't had a simple and concrete solution yet. This is my attempt on making it happen 馃檭

To sum up the problem: on windows, ampersand (&) characters were being escaped with caret (^) characters due to windows's cmd interpreting them as a command would break (as in command not found) or would forward a partial argument only (as in URLs with query arguments that contain spaces).

The solution: JavaScript's standard encodeURI function would solve the problem for URLs, but not for file paths (especially on windows). So we leverage the fact that window's cmd interprets a double-quoted argument as a plain text argument just by quoting it (like Node already does). For that to be consistent we need to disable Node's default quoting and escaping behavior (which is conditional) with its own windowsVerbatimArguments flag, and do it ourselves.

Tested in Windows with both Google Chrome and Internet Explorer, using query strings, spaces, pipes, fragments and multiple combinations of all of them.


IssueHunt Summary

Referenced issues

This pull request has been submitted to:


IssueHunt has been backed by the following sponsors. Become a sponsor

@herberttn
Copy link
Contributor Author

herberttn commented Aug 27, 2019

@sindresorhus Need me to do something else? Can we get a new version with this fix?

index.js Show resolved Hide resolved
@sindresorhus
Copy link
Owner

Sorry for the slow response. I have a lot to catch up on after the summer.

Stanzilla referenced this pull request in vercel/hyper Sep 12, 2019
@Stanzilla
Copy link

Does this also fix the problem in #98?

@herberttn
Copy link
Contributor Author

Does this also fix the problem in #98?

Hello @Stanzilla, yes it does!
This one fixes/closes all of the issues/pulls listed in the description!

Since I'm here: Hey @sindresorhus, when you have some spare time, this is ready for another review ;)

@sindresorhus sindresorhus merged commit 7ef15d2 into sindresorhus:master Oct 15, 2019
@sindresorhus
Copy link
Owner

@herberttn Thanks for your awesome work on this 馃檶

@sindresorhus
Copy link
Owner

https://github.com/sindresorhus/open/releases/tag/v7.0.0

@herberttn
Copy link
Contributor Author

@herberttn Thanks for your awesome work on this 馃檶

馃帀

@baoti
Copy link

baoti commented Oct 17, 2019

Maybe need some test for urls contains '%' (e.g. https://cn.bing.com/search?q=%2520 https://cn.bing.com/search?q=%E5%A5%BD). I think encodeURI will break url which is already url encoded.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment