Uri#parse can break path-component

[antoinerousseau]

• VSCode Version: 1.21.0
• OS Version: macOS 10.13.3

Steps to Reproduce:

1. Insert this link in a JS file: https://firebasestorage.googleapis.com/v0/b/brewlangerie.appspot.com/o/products%2FzVNZkudXJyq8bPGTXUxx%2FBetterave-Sesame.jpg?alt=media&token=0b2310c4-3ea6-4207-bbde-9c3710ba0437
2. Cmd-click it to open it: the %2F are not preserved and are replaced with /, causing the request to fail

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

[humphd]

I dug into this a bit today, and it's caused by the decodeURIComponent() calls on the path in uri.ts:

https://github.com/Microsoft/vscode/blob/7d80b00cb0d051425ade0e19e925dfcc2fe4c45c/src/vs/base/common/uri.ts#L264-L276

I'm not sure what the right fix is, but special-casing http(s) schemes (vs. file), and not calling it for match[5] fixes it. Though this change does break a bunch of pure file:// based tests with special characters in the path that are percent-encoded.

Maybe the right thing is to not decode at all until you need to reproduce the URI as a string. I note that in the browser, this is what happens:

new URL("https://firebasestorage.googleapis.com/v0/b/brewlangerie.appspot.com/o/products%2FzVNZkudXJyq8bPGTXUxx%2FBetterave-Sesame.jpg?alt=media&token=0b2310c4-3ea6-4207-bbde-9c3710ba0437").pathname
"/v0/b/brewlangerie.appspot.com/o/products%2FzVNZkudXJyq8bPGTXUxx%2FBetterave-Sesame.jpg"

Same in node:

require('url').parse('https://firebasestorage.googleapis.com/v0/b/brewlangerie.appspot.com/o/products%2FzVNZkudXJyq8bPGTXUxx%2FBetterave-Sesame.jpg?alt=media&token=0b2310c4-3ea6-4207-bbde-9c3710ba0437').pathname
'/v0/b/brewlangerie.appspot.com/o/products%2FzVNZkudXJyq8bPGTXUxx%2FBetterave-Sesame.jpg'

[humphd]

cc'ing @jrieken, who git shows to have been active on this code, and might have thoughts.

[jrieken]

/duplicate of #25852

[jrieken]

Sorry, this actually is not a duplicate of #25852

[jrieken]

fixed via #73849

[jrieken]

This unfortunately had to be reverted and reopened. The fix introduced a bunch of bad regressions that we couldn't easily fix. We revert to the old behaviour and we need to get back to the drawing board, however no easy solution exists...

[jrieken]

Fixed via #83060. There is still a caveat when calling parse and accessing the components of an URI because those are decoded (see this test which documents the current behaviour). This is for compatibility reasons. Anyways, calling toString will return the correct result and do the right thing when opened in a browser etc

[jrieken]

re-opened via 58479e8

[jrieken]

/duplicate of #83645

Closing this as a duplicate of #83645 which is all about finding a way out of the URI nightmare.

[vscodebot]

Thanks for creating this issue! We figured it's covering the same as another one we already have. Thus, we closed this one as a duplicate. You can search for existing issues here. See also our issue reporting guidelines.

Happy Coding!