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.

  • Fixes #17: URL fragments
  • Fixes #44: URL query strings (ampersand escaping)
  • Fixes #55: URL query strings with local HTML files (ampersand escaping)
  • Fixes #77: File system path (ampersand escaping)
  • Fixes #101: URL query strings with local HTML files (ampersand escaping)
  • Fixes #115: Shell option is not needed since windowsVerbatimArguments is now used explicitly
  • Resolves #74: File system path (ampersand escaping)
  • Resolves #98: URL with pipe characters

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 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

@sindresorhus sindresorhus commented Sep 11, 2019

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
This reverts commit 84bab83.
@Stanzilla
Copy link

@Stanzilla Stanzilla commented Oct 9, 2019

Does this also fix the problem in #98?

@herberttn
Copy link
Contributor Author

@herberttn herberttn commented Oct 10, 2019

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
1 check passed
1 check passed
@travis-ci
Travis CI - Pull Request Build Passed
Details
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Oct 15, 2019

@herberttn Thanks for your awesome work on this 馃檶

@herberttn
Copy link
Contributor Author

@herberttn herberttn commented Oct 16, 2019

@herberttn Thanks for your awesome work on this 馃檶

馃帀

@baoti
Copy link

@baoti 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