Make webviews load locally by code-asher · Pull Request #16 · coder/vscode

code-asher · 2021-11-17T00:46:25Z

I also had to revert path changes (from using includes) which also fixed a couple other bugs as well (see commits).

Instead of using Microsoft's hosted endpoint. This also fixes issues when using code-server over http since some browsers refuse to load http content (like when using localhost) in https contexts (since the iframe was always https).

code-asher · 2021-11-17T00:54:00Z

 					webEndpointUrlTemplate: this.createRequestUrl(req, parsedUrl, '/static').toString(),
+					webviewContentExternalBaseUrlTemplate: './webview/{{uuid}}/',

 					updateUrl: this.createRequestUrl(req, parsedUrl, '/update/check').toString(),


@jsjoeio For the proxy fixes it is probably sufficient to make these relative. The webview endpoints can probably just use ./ but the update and logout might be either ./ or ./../ depending on whether VS Code is being accessed from / or /vscode. We could copy the function here: https://github.com/cdr/code-server/blob/cd26f84bc6e0e6f5c17d847d9fdd718980b657a8/src/node/http.ts#L109-L121

Nice find! Looks like that would be an easy fix to copy in

We cannot use `includes` because this causes problems with endpoints that may share portions of their paths. For example /webview/uuid/service-worker.js was colliding with the /service-worker.js but moving it before caused static assets that load from a directory called /webview to start failing. We could move static up as well but this may just cause other conflicts. The standard ways of handling base paths seem to be to either ask the user for the base path or require that a rewriting proxy be put in front of the application (which rewrites /base/path/my/path to /my/path before it ever gets to the application). code-server currently requires the latter and does not support the former but either way the code will look similar (in the former case we would just strip the base path first, maybe before calling `handle`). For reference, the proxy in the Coder product is a rewriting proxy. This also fixes issues when the web server is behind multiple Express endpoints. With the existing code /anything/works/ rather than just the endpoints you specify (/vscode and / in our case) which is unexpected. It also makes it possible to browse without a trailing slash, for example /vscode, although that is currently broken for other reasons.

jsjoeio

Nice work! 👏 Thanks for seeing this through.

jsjoeio · 2021-11-17T17:04:15Z

+			// The webview endpoint contains variables in the format {{var}} so decode
+			// them as `new URI` will encode them.


great comment 👏

jsjoeio · 2021-11-17T17:08:28Z

+			webviewContentExternalBaseUrlTemplate: decodeURIComponent(
+				new URL(
+					config.productConfiguration?.webviewContentExternalBaseUrlTemplate ?? "",
+					window.location.toString(), // This works without toString() but TypeScript thinks otherwise.
+				).toString(),
+			),


❓ so here, we are decoding a new URL after constructing it with the webviewContentExternalBaseUrlTemplate and the current window.location as the base. I think

window.location.toString(), // This works without toString() but TypeScript thinks otherwise.

I believe window.location returns an object in some browsers but calling toString() returns the current URL, which is what we want. My guess is TS is actually correct

This works in the browsers I tested:

new URL("some/path", window.location)

So TypeScript appears to be wrong about it needing to be a string. MDN tells us that any value you pass will be stringified automatically. In particular URL (and Location, it seems) will use the href property (toString() just returns href in these cases I believe).

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

Note: The url and base arguments will each be stringified from whatever value you pass, just like with other Web APIs that accept USVString. In particular, you can use an existing URL object for either argument, and it will stringify to the object's href property.

I see what you're referring to now - TIL!

It's interesting though, according to the typedefs for new URL() it will only accept 'string | URL | undefined.

I wonder then if I should make an issue upstream to update the typedefs here to accept Location. What do you think?

I should also note that TypeScript does allow URL for the base but not Location for some reason. 🤷

I should also note that TypeScript does allow URL for the base

Right! I see that in the typedefs. Weird, it doesn't allow Location. I think I'll open up an issue and see if that's a bug.

Actually, looks like someone already did!

Oh snap! Nice find.

jsjoeio · 2021-11-17T17:09:07Z

-
-			if (['favicon.ico', 'code-192.png', 'code-512.png'].includes(parsedPath.base)) {
+			if (pathname === '/favicon.ico' || pathname === '/manifest.json' || pathname === '/code-192.png' || pathname === '/code-512.png') {
 				// always serve icons/manifest, even without a token


I guess serving icons without a token means they will always load 👍

Yup, assets like these cannot use tokens otherwise they would not work on the login page.

jsjoeio · 2021-11-17T17:09:27Z

+			if (pathname === '/favicon.ico' || pathname === '/manifest.json' || pathname === '/code-192.png' || pathname === '/code-512.png') {
 				// always serve icons/manifest, even without a token
-				return serveFile(this._logService, req, res, join(APP_ROOT, 'resources', 'server', parsedPath.base));
+				return serveFile(this._logService, req, res, join(APP_ROOT, 'resources', 'server', pathname.substr(1)));


❓ why do we do pathname.substr(1) here?

This is from upstream. It appears to be removing the leading slash but I am not sure why. Maybe their version of join breaks when there are leading slashes?

jsjoeio · 2021-11-17T17:09:50Z

+			/**
+			 * Add an endpoint for self-hosting webviews.  This must be unique per
+			 * webview as the code relies on unique service workers.  In our case we
+			 * use /webview/{{uuid}}.
+			 *
+			 * @author coder
+			 */


Will this PR resolve coder/code-server#4483 then?

jsjoeio · 2021-11-17T17:11:20Z

+			 * @author coder
+			 */
+			if (/^\/webview\//.test(pathname)) {
+				// always serve webview requests, even without a token


❓ why do we always serve webview requests? wouldn't we want to only serve with the token?

Good question, I think you might be right. I mostly have it this way since VS Code's appears to behave this way but adding the token is probably smart.

Currently we skip the token anyway but I will make a note on the issue we have for it to block webviews once we support it.

I should note that it is just serving assets so it should not be a security risk but at the same time it is only needed once you log in so might as well lock it up.

Well...one more thing haha. You can actually access these files from the /static endpoint already so I guess locking it up would not achieve anything since the static endpoint does not require auth.

jsjoeio · 2021-11-17T17:12:24Z

+		const relativeFilePath = normalize(normalizedPathname.split('/').splice(3).join('/'));
+
+		const filePath = join(APP_ROOT, 'out/vs/workbench/contrib/webview/browser/pre', relativeFilePath);
+		if (!isEqualOrParent(filePath, APP_ROOT, !isLinux)) {


❓ do you know why we pass the !isLinux as the third argument here?

The function signature looks like this:

function isEqualOrParent(base: string, parentCandidate: string, ignoreCase?: boolean, separator = sep): boolean

Linux is case sensitive so it looks like this is saying to be case sensitive on Linux platforms.

Edit: corrected, Linux is case sensitive, not insensitive. 🤦

jsjoeio · 2021-11-17T17:13:52Z

+	 * A unique path is required for every webview service worker.
+	 */
+	private async _handleWebview(req: http.IncomingMessage, res: http.ServerResponse, parsedUrl: url.UrlWithParsedQuery): Promise<void> {
+		const headers: Record<string, string> = Object.create(null);


! I find the type here Record<string, string> interesting but I guess that's cause TS can't infer the correct type because we initialize as an empty object?

Yeah I am not completely sure (this is copied from upstream) but without some kind of cast this would just be an empty object so you might run into trouble adding headers afterward, for example with:

headers["my-header"] = "my-value";

I am not sure but TypeScript might say something like "my-header is not a property of object".

I do not know why they use Record<string, string> over { [key: string]: string } though. 🤔

Ah gotcha. That makes sense.

I do not know why they use Record<string, string> over { [key: string]: string } though. 🤔

Yeah, wish I knew where to find out why they do that 🤔

jsjoeio · 2021-11-17T17:14:02Z

+	private async _handleWebview(req: http.IncomingMessage, res: http.ServerResponse, parsedUrl: url.UrlWithParsedQuery): Promise<void> {
+		const headers: Record<string, string> = Object.create(null);
+
+		// support paths that are uri-encoded (e.g. spaces => %20)


great comment 👏

jsjoeio · 2021-11-17T17:14:27Z

 			`script-src 'self' 'unsafe-eval' ${this._getScriptCspHashes(data).join(' ')} 'sha256-cb2sg39EJV8ABaSNFfWu/ou8o1xVXYK7jp90oZ9vpcg=';`,
 			'child-src \'self\';',
-			`frame-src 'self' https://*.vscode-webview.net ${this._productService.webEndpointUrl || ''} data:;`,
+			`frame-src 'self' ${this._productService.webEndpointUrl || ''} data:;`,


was this the missing puzzle piece? (I guess one of the bigger ones)

Nope, this is technically unnecessary but it does restrict the CSP scope which to me feels more secure.

The really important part was setting webviewContentExternalBaseUrlTemplate so it uses the same domain you are using to access code-server and creating an endpoint for it (/webview/uuid) in order to get unique service workers per webview.

Ah gotcha!

unique service workers per webview

Sounds like this was the main key to fix this!

jsjoeio · 2021-11-18T17:13:43Z

@code-asher anything preventing us from merging this?

code-asher requested a review from jsjoeio November 17, 2021 00:46

Make webviews load locally

15435fc

Instead of using Microsoft's hosted endpoint. This also fixes issues when using code-server over http since some browsers refuse to load http content (like when using localhost) in https contexts (since the iframe was always https).

code-asher commented Nov 17, 2021

View reviewed changes

jsjoeio approved these changes Nov 17, 2021

View reviewed changes

jsjoeio mentioned this pull request Nov 19, 2021

Update VS Code coder/code-server#4494

Closed

GirlBossRush approved these changes Nov 19, 2021

View reviewed changes

GirlBossRush merged commit 2a3c835 into coder:code-server-v2 Nov 19, 2021

jsjoeio mentioned this pull request Nov 22, 2021

Theme images on the welcome page are broken coder/code-server#4483

Closed

code-asher deleted the code-server-v2 branch December 10, 2021 20:53

DJlee139 mentioned this pull request Apr 25, 2023

IDE Theme PNG files broken when opening Welcome Screen in HTTPS with Docker when using VS Code Server microsoft/vscode#180853

Closed

		// The webview endpoint contains variables in the format {{var}} so decode
		// them as `new URI` will encode them.

Conversation

code-asher commented Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

code-asher Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsjoeio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

code-asher Nov 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsjoeio commented Nov 18, 2021

code-asher commented Nov 17, 2021 •

edited

Loading

code-asher Nov 17, 2021 •

edited

Loading

code-asher Nov 17, 2021 •

edited

Loading

code-asher Nov 17, 2021 •

edited

Loading

code-asher Nov 17, 2021 •

edited

Loading

code-asher Nov 17, 2021 •

edited

Loading