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

URI.parse should not use decodeURIComponent when parsing the querystring of a clicked link #53824

Closed
bpartridge opened this issue Jul 9, 2018 · 2 comments
Assignees
Labels
info-needed Issue requires more information from poster uri

Comments

@bpartridge
Copy link

  • VSCode Version: 1.25.0-insider b7e6e04
  • OS Version: Mac OS 10.12.6

Steps to Reproduce:

  1. In a comment in any code file, type https://google.com/?a=%2B
  2. Command-clicking it should open https://google.com/?a=%2B in a browser
  3. Instead, it loads https://google.com/?a=+ which is not the same URL (an HTTP server would parse this as {a: ' '} rather than {a: '+'} as intended)

Does this issue occur when all extensions are disabled?: Yes

Root cause seems to be that https://github.com/Microsoft/vscode/blob/6fae52876b1f9e549a6e1b91789d5fff82d61f0c/src/vs/base/common/uri.ts#L228 calls decodeURIComponent on the query string.

This is incorrect behavior, as the query string '+' is distinct from the query string '%2B'. For instance, if you have a script that, when run, prints a URL that url-encodes an email address like [email protected] in a query parameter, when clicked, the browser would send a query parameter that would decode on the server to test [email protected].

The correct fix would be to have the URI class represent query strings in their encoded form, only decoding them as needed. But this might cause problems with other users of the class that have relied on the assumption that the query string is decoded.

(Note that it would be incorrect to do the inverse; calling encodeURIComponent on the decoded query string would change all the ampersands delimiting query parameters into their encoded equivalents.)

This would likely fix #32026 but this problem is more general.

@weinand weinand removed their assignment Jul 9, 2018
@jrieken jrieken added the uri label Jul 9, 2018
@jrieken
Copy link
Member

jrieken commented Jul 9, 2018

This has nothing to with parse but with toString, esp. toString(skipEncoding: boolean) which is how we call it when opening uris for the browser (which will then do the http-specific encoding).

@bpartridge What is your issue here? Is this just a dupe of #32026?

@jrieken jrieken added the info-needed Issue requires more information from poster label Jul 9, 2018
@jrieken jrieken self-assigned this Jul 9, 2018
@vscodebot vscodebot bot closed this as completed Jul 16, 2018
@vscodebot
Copy link

vscodebot bot commented Jul 16, 2018

This issue has been closed automatically because it needs more information and has not had recent activity. See also our issue reporting guidelines.

Happy Coding!

@vscodebot vscodebot bot locked and limited conversation to collaborators Aug 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
info-needed Issue requires more information from poster uri
Projects
None yet
Development

No branches or pull requests

3 participants