fix(lib/vscode): patch authority in asWebviewUri #3895

jsjoeio · 2021-08-03T22:03:23Z

This PR patches asWebviewUri in lib/vscode/src/vs/workbench/api/common/shared/webview.ts to remove the port number from the authority field used to create the URI.

For now, we found that removing the port fixes the issue (resources load as expected in webviews), but long-term, we want to understand how this is meant to be used (having the port in the URI authority field).

Here is a video of an extension using a webview which loads both CSS and JS working.

Screen.Recording.2021-08-04.at.2.06.20.PM.mov

Fixes N/A

TODOS

fix audit-ci vulnerability in separate PR chore: bump tar with resolutions #3899
clone vscode
get tests working
submit new test for asWebviewUri fix: handle localhost and port in asWebviewUri microsoft/vscode#130148

codecov · 2021-08-03T22:10:26Z

Codecov Report

Merging #3895 (3ceb4cb) into main (56f0c4e) will not change coverage.
The diff coverage is n/a.

❗ Current head 3ceb4cb differs from pull request most recent head 99503fb. Consider uploading reports for the commit 99503fb to get more accurate results

@@           Coverage Diff           @@
##             main    #3895   +/-   ##
=======================================
  Coverage   63.51%   63.51%           
=======================================
  Files          36       36           
  Lines        1872     1872           
  Branches      379      379           
=======================================
  Hits         1189     1189           
  Misses        580      580           
  Partials      103      103

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56f0c4e...99503fb. Read the comment docs.

jawnsy · 2021-08-03T22:18:02Z

lib/vscode/src/vs/workbench/api/common/shared/webview.ts

 	return URI.from({
 		scheme: Schemas.https,
-		authority: `${resource.scheme}+${resource.authority}.${webviewRootResourceAuthority}`,
+		authority: `${resource.scheme}+${resource.authority.split(':')[0]}.${webviewRootResourceAuthority}`,


Be careful with this - authority could be an IPv6 address, which will contain colons and possibly also a port number, for example: https://[2607:f8b0:4005:80f::200e]:443 (this is Google's IPv6 address)

It might be preferable to use something like new URL("https://localhost:8443/test/abc").hostname to do this instead?

> new URL("https://localhost:8443/test/abc").hostname "localhost" > new URL("https://[2607:f8b0:4005:80f::200e]:443/").hostname "[2607:f8b0:4005:80f::200e]"

Oh, didn't know or consider that. Okay, maybe we should do something like this instead?

+${new URL(`http://${resource.authority}/test/abc``).hostname} -${resource.authority.split(':')[0]}

Would that be okay?

Note - the above doesn't account for any exceptions thrown from the URL constructor. I don't know if we want to try and handle that?

https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

Good call @vapurrmaid - let's have @code-asher chime in with his thoughts. I think he's in favor of the least intrusive patch.

All good, it's probably benign - just something to weigh

Ahhhh IPv6 is an interesting edge case.

To think out loud and summarize a bit, what happens here is that VS Code has a service worker that intercepts all requests in the web view (the iframe). It then extracts the "real" URI from this fake one and sends it to the parent (via postMessage) where the actual request is made after which the response is sent back (again through postMessage) for the service worker to resolve with.

So it doesn't matter too much what this URL is as long as:

The browser thinks it is valid and so actually makes the request inside the iframe for the service worker to then catch.

The URL has the info in it required to reconstruct the real URL and complete the real request successfully.

On the first point, the browser does nothing at all if there is a colon anywhere in the hostname (encoding it doesn't work either). For example, these all silently do nothing (no error at all from the browser indicating these are invalid):

<script src="https://test:5000.com/test.js"></script> <script src="https://test%3A5000.com/test.js"></script> <script src="https://test:.com/test.js"></script> <script src="https://test.com:d/test.js"></script>

Maybe there is an argument to be made here that VS Code's URI class should have thrown an error rather than spit out an invalid URL but I'm not sure.

In any case it seems that the solution must involve removing the port from the domain. new URL(url).hostname feels like an excellent solution to this. I think we will be safe here since we use the host header as the remote authority so it should be valid.

That leads to the second point. Is that port necessary for VS Code to reconstruct the real URL? If it is, we should add it back in the way @jawnsy is proposing. This may mean we also have to tweak the reconstruction side of things to correctly extract the port assuming VS Code hasn't accounted for this case.

But since it apparently works without the port (and without the authority entirely), do we even need to worry about it? Actually I'm confused why the authority would be required at all. The scheme and path should be all VS Code needs since it already knows where the backend is.

Anyway, the reconstructions seems to happen here:

https://github.com/cdr/code-server/blob/570cb6983281abbad617a8bbc9e1aa6a3d0048b5/lib/vscode/src/vs/workbench/contrib/webview/browser/pre/service-worker.js#L257-L271

So sure enough if we strip the port then this authority here will be port-less but we could patch this to add it back in. This is probably the "correct" solution. I wonder why it works without the authority though? Note the may be empty in the above code.

Well if we chase this down it eventually gets passed through to our file provider through the file provider channel. This channel (like all the channels) are set up during the initial connection and remain open so the authority has no use here. The file provider simply uses fsPath off the URI and ignores all the other properties of the URI.

In fact if you log all the requests to the file provider you'll see that many of them don't have the authority at all. So maybe we could just remove it but...

That doesn't mean there is absolutely no case where the authority won't be used. For example if a plugin implements their own scheme handler and they try to read the authority for some reason, maybe that could be an issue.

So I think the most correct solution is to stick the port in the proper place then add it back to the authority during the reconstruction linked above.

Something like this maybe (and add the port to asWebviewUri):

const authority = firstHostSegment.slice(scheme.length + 1) + requestUrl.port ? (":" + requestUrl.port) : ""

@code-asher all that makes sense to me. Should we still submit a patch upstream?

It would be nice if we could get it fixed upstream. I suppose worst thing that happens is that it gets closed.

Cool cool! Let me fix this up here then re-request a review from everyone and then I'll look into following up with a PR upstream to fix there too

lib/vscode/src/vs/workbench/api/common/shared/webview.ts

lib/vscode/src/vs/workbench/contrib/webview/browser/pre/service-worker.js

jsjoeio · 2021-08-04T20:54:28Z

Something strange is happening so converting back to a draft

jsjoeio · 2021-08-04T21:07:38Z

All fixed - ready for review!

jawnsy · 2021-08-05T01:47:55Z

lib/vscode/src/vs/workbench/api/common/shared/webview.ts

@@ -55,9 +55,12 @@ export function asWebviewUri(
 		});
 	}

+	// NOTE@coder: Add the port separately because if the port is in the domain the
+	// URL will be invalid and the browser will not request it.
+	const authorityUrl = new URL(`http://${resource.authority}`);


Are we guaranteed that the incoming authority is a valid URL? otherwise as Grey mentioned before, we'd have to try/catch a possible error right?

Also it does't matter since we're not using the scheme part, but you might want to consider using https here just because it looks a little nicer

AFAIK authority may represent any fully qualified host e.g. localhost:8080, example.com/any/path. The scheme may not necessarily be http

Good call! I'll use the resource.scheme here instead of http or https

We set the remote authority here:

https://github.com/cdr/code-server/blob/cde28a0a0e29381fa5a631da092f7065bde35ccf/src/node/routes/vscode.ts#L34

So potentially it could be blank although I'm not sure how often the host header is missing.

Personally I think letting the error throw is fine although maybe we should catch and re-throw it with a more helpful message ("host header may be invalid" or something like that).

Although VS Code does not make the authority mandatory so we should probably gracefully handle a blank authority.

jawnsy · 2021-08-05T01:48:25Z

LGTM. Are we still planning to offer this patch upstream?

GirlBossRush · 2021-08-05T04:49:02Z

lib/vscode/src/vs/workbench/api/common/shared/webview.ts

 	return URI.from({
 		scheme: Schemas.https,
-		authority: `${resource.scheme}+${resource.authority}.${webviewRootResourceAuthority}`,
+		authority: `${resource.scheme}+${authorityUrl.hostname}.${webviewRootResourceAuthority}${authorityUrl.port ? (':' + authorityUrl.port) : ''}`,


@jsjoeio IMO the bug stems from modifying URLs as strings, rather than leaning on the browser’s innate URL constructor Would something like this help with URL?

// e.g. new URL(‘abc://localhost:3000/foo/bar?baz=qux') // The scheme isn’t too big of a deal here but it helps to lean on what’s known from above. const authorityUrl = new URL(`${resource.scheme}://${resource.authority}`); authorityUrl.hostname = `${authorityUrl.hostname}+${resource.authority}.${webviewRootResourceAuthority}`; // Remove the protocol const authority = authorityUrl.toString().slice(`${authorityUrl.protocol}//`.length); return URI.from({ scheme: Schemas.https, authority, path: resource.path, fragment: resource.fragment, query: resource.query, });

That might work! It sounds like a more complete fix. I'll give it a try and report back! (thanks for the through codeblock too!

Converting to draft stage and then i'll re-request a review

Should we try/catch the URL construction?

On second thought, I think we'll leave as is and let it bubble up.

Okay...looking at your solution, I don't think I fully understand how it helps (more on my lack of understanding, not critiquing your code 😅). I tried it, but I think either the hostname or the port gets mangled (though I might be doing something else wrong). I'm going to leave as is for now. And if you want to follow-up with more details or have me submit another PR to improve this, happy to do so!

harrylepotter-win · 2021-08-05T06:41:52Z

@jsjoeio thankyou so much for this.

jsjoeio · 2021-08-05T18:58:00Z

LGTM. Are we still planning to offer this patch upstream?

Yessir! Added to the PR description just now: microsoft/vscode#130148

We do this because resources in webviews don't load properly when the port is used in the resource uri. We're not sure why this is happening but adding this fix to get webviews working like before.

I made Asher's requested changes but he is taking PTO so dismissing (Jonathan double-checked)

With #3895, we caused a regression where the Content-Security-Policy prevented images in the previewer to not work due to the ports in the resource URI. This modifies the CSP in the webview to make sure images are not blocked by CSP. I assume once we upgrade VS Code, we will revert this change.

jsjoeio self-assigned this Aug 3, 2021

jsjoeio added the bug Something isn't working label Aug 3, 2021

jsjoeio added this to the 3.11.1 milestone Aug 3, 2021

jsjoeio force-pushed the jsjoeio-fix-webview-uri branch from 692d4e4 to 6c18207 Compare August 3, 2021 22:07

jsjoeio marked this pull request as ready for review August 3, 2021 22:12

jsjoeio requested a review from a team as a code owner August 3, 2021 22:12

jawnsy reviewed Aug 3, 2021

View reviewed changes

code-asher previously requested changes Aug 4, 2021

View reviewed changes

lib/vscode/src/vs/workbench/api/common/shared/webview.ts Outdated Show resolved Hide resolved

lib/vscode/src/vs/workbench/contrib/webview/browser/pre/service-worker.js Outdated Show resolved Hide resolved

jsjoeio force-pushed the jsjoeio-fix-webview-uri branch from 4e36e44 to f5df6a7 Compare August 4, 2021 20:24

jsjoeio requested review from code-asher, greyscaled and jawnsy August 4, 2021 20:25

jsjoeio marked this pull request as draft August 4, 2021 20:54

jsjoeio force-pushed the jsjoeio-fix-webview-uri branch from f5df6a7 to ead6c5a Compare August 4, 2021 21:07

jsjoeio marked this pull request as ready for review August 4, 2021 21:09

jsjoeio mentioned this pull request Aug 4, 2021

fix: handle localhost and port in asWebviewUri microsoft/vscode#130148

Closed

jawnsy reviewed Aug 5, 2021

View reviewed changes

jawnsy approved these changes Aug 5, 2021

View reviewed changes

GirlBossRush reviewed Aug 5, 2021

View reviewed changes

jsjoeio marked this pull request as draft August 5, 2021 18:58

greyscaled removed their request for review August 5, 2021 22:07

jsjoeio marked this pull request as ready for review August 5, 2021 23:21

jsjoeio mentioned this pull request Aug 5, 2021

Add VSCODE_PROXY_URI env var to extension host and terminal #3621

Closed

4 tasks

fix(lib/vscode): patch authority in asWebviewUri

99503fb

We do this because resources in webviews don't load properly when the port is used in the resource uri. We're not sure why this is happening but adding this fix to get webviews working like before.

jsjoeio force-pushed the jsjoeio-fix-webview-uri branch from ead6c5a to 99503fb Compare August 5, 2021 23:26

jsjoeio enabled auto-merge August 5, 2021 23:27

jsjoeio merged commit f75edc2 into main Aug 5, 2021

jsjoeio deleted the jsjoeio-fix-webview-uri branch August 5, 2021 23:47

jsjoeio mentioned this pull request Aug 6, 2021

Image previewer does not work #3826

Closed

ahmadyahya11 mentioned this pull request Aug 8, 2021

v3.11.1 #3925

Closed

This was referenced Aug 10, 2021

Revert "fix(lib/vscode): patch authority in asWebviewUri" #3948

Closed

Cannot open jpg, gif, or png in code-server web IDE #3936

Closed

jsjoeio mentioned this pull request Sep 3, 2021

vsix extensions that utilise webviews are blocked by CSP #4098

Closed

jsjoeio mentioned this pull request Sep 10, 2021

fix(lib/vscode): update csp in webview to allow ports #4131

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(lib/vscode): patch authority in asWebviewUri #3895

fix(lib/vscode): patch authority in asWebviewUri #3895

jsjoeio commented Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 3, 2021 •

edited

Loading

jawnsy Aug 3, 2021 •

edited

Loading

jsjoeio Aug 3, 2021

greyscaled Aug 3, 2021 •

edited

Loading

jsjoeio Aug 3, 2021

greyscaled Aug 3, 2021

code-asher Aug 4, 2021

code-asher Aug 4, 2021 •

edited

Loading

jsjoeio Aug 4, 2021

code-asher Aug 4, 2021

jsjoeio Aug 4, 2021

jsjoeio commented Aug 4, 2021

jsjoeio commented Aug 4, 2021

jawnsy Aug 5, 2021

GirlBossRush Aug 5, 2021

jsjoeio Aug 5, 2021

code-asher Aug 10, 2021

code-asher Aug 10, 2021 •

edited

Loading

code-asher Aug 10, 2021

jawnsy commented Aug 5, 2021

GirlBossRush Aug 5, 2021

jsjoeio Aug 5, 2021

jsjoeio Aug 5, 2021

greyscaled Aug 5, 2021

jsjoeio Aug 5, 2021 •

edited

Loading

jsjoeio Aug 5, 2021 •

edited

Loading

harrylepotter-win commented Aug 5, 2021

jsjoeio commented Aug 5, 2021

fix(lib/vscode): patch authority in asWebviewUri #3895

fix(lib/vscode): patch authority in asWebviewUri #3895

Conversation

jsjoeio commented Aug 3, 2021 • edited Loading

Video

codecov bot commented Aug 3, 2021 • edited Loading

Codecov Report

jawnsy Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greyscaled Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-asher Aug 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjoeio commented Aug 4, 2021

jsjoeio commented Aug 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

code-asher Aug 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jawnsy commented Aug 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsjoeio Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

jsjoeio Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

harrylepotter-win commented Aug 5, 2021

jsjoeio commented Aug 5, 2021

jsjoeio commented Aug 3, 2021 •

edited

Loading

codecov bot commented Aug 3, 2021 •

edited

Loading

jawnsy Aug 3, 2021 •

edited

Loading

greyscaled Aug 3, 2021 •

edited

Loading

code-asher Aug 4, 2021 •

edited

Loading

code-asher Aug 10, 2021 •

edited

Loading

jsjoeio Aug 5, 2021 •

edited

Loading

jsjoeio Aug 5, 2021 •

edited

Loading