Skip to content

Make webviews load locally #16

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

Merged
merged 2 commits into from
Nov 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions src/vs/code/browser/workbench/workbench.ts
Original file line number Diff line number Diff line change
Expand Up @@ -521,6 +521,25 @@ class WindowIndicator implements IWindowIndicator {
// Finally create workbench
create(document.body, {
...config,
/**
* Override relative URLs in the product configuration against the window
* location as necessary. Only paths that must be absolute need to be
* rewritten (for example the webview endpoint); the rest can be left
* relative (for example the update path).
*
* @author coder
*/
productConfiguration: {
...config.productConfiguration,
// The webview endpoint contains variables in the format {{var}} so decode
// them as `new URI` will encode them.
Comment on lines +534 to +535
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comment 👏

webviewContentExternalBaseUrlTemplate: decodeURIComponent(
new URL(
config.productConfiguration?.webviewContentExternalBaseUrlTemplate ?? "",
window.location.toString(), // This works without toString() but TypeScript thinks otherwise.
).toString(),
),
Comment on lines +536 to +541
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

image

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

image

Copy link
Member Author

@code-asher code-asher Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, looks like someone already did!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh snap! Nice find.

},
developmentOptions: {
logLevel: logLevel ? parseLogLevel(logLevel) : undefined,
...config.developmentOptions
Expand Down
85 changes: 58 additions & 27 deletions src/vs/server/webClientServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,51 +85,59 @@ export class WebClientServer {
private readonly _productService: IProductService
) { }

/**
* @coder Patched to handle an arbitrary path depth, such as in Coder Enterprise.
*/
async handle(req: http.IncomingMessage, res: http.ServerResponse, parsedUrl: url.UrlWithParsedQuery): Promise<void> {
try {
const pathname = parsedUrl.pathname!;
const parsedPath = path.parse(pathname);
const pathPrefix = getPathPrefix(pathname);

if (['manifest.json', 'webmanifest.json'].includes(parsedPath.base)) {
/**
* Add a custom manifest.
*
* @author coder
*/
if (pathname === '/manifest.json' || pathname === '/webmanifest.json') {
return this._handleManifest(req, res, parsedUrl);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}

if (parsedPath.base === this._environmentService.serviceWorkerFileName) {
return serveFile(this._logService, req, res, this._environmentService.serviceWorkerPath, {
'Service-Worker-Allowed': pathPrefix,
});
/**
* 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
*/
Comment on lines +104 to +110
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup!

if (/^\/webview\//.test(pathname)) {
// always serve webview requests, even without a token
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return this._handleWebview(req, res, parsedUrl);
}

if (parsedPath.dir.includes('/static/') && parsedPath.ext) {
parsedUrl.pathname = pathname.substring(pathname.indexOf('/static/'));
if (/^\/static\//.test(pathname)) {
// always serve static requests, even without a token
return this._handleStatic(req, res, parsedUrl);
}

if (parsedPath.base === 'callback') {
/**
* Move the service worker to the root. This makes the scope the root
* (otherwise we would need to include Service-Worker-Allowed).
*
* @author coder
*/
if (pathname === '/' + this._environmentService.serviceWorkerFileName) {
return serveFile(this._logService, req, res, this._environmentService.serviceWorkerPath);
}
if (pathname === '/') {
// the token handling is done inside the handler
return this._handleRoot(req, res, parsedUrl);
}
if (pathname === '/callback') {
// callback support
return this._handleCallback(req, res, parsedUrl);
}

if (parsedPath.base === 'fetch-callback') {
if (pathname === '/fetch-callback') {
// callback fetch support
return this._handleFetchCallback(req, res, parsedUrl);
}

if (pathname.endsWith('/')) {
// the token handling is done inside the handler
return this._handleRoot(req, res, parsedUrl);
}

const message = `"${parsedUrl.pathname}" not found.`;
const error = new Error(message);
req.emit('error', error);
Expand Down Expand Up @@ -240,6 +248,28 @@ export class WebClientServer {
return serveFile(this._logService, req, res, filePath, headers);
}

/**
* Handle HTTP requests for /webview/*
*
* 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);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@code-asher code-asher Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 🤔


// support paths that are uri-encoded (e.g. spaces => %20)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great comment 👏

const normalizedPathname = decodeURIComponent(parsedUrl.pathname!);

// Strip `/webview/{uuid}` from the path.
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)) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@code-asher code-asher Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return this.serveError(req, res, 400, `Bad request.`, parsedUrl);
}

return serveFile(this._logService, req, res, filePath, headers);
}

/**
* Handle HTTP requests for /
*/
Expand Down Expand Up @@ -318,6 +348,7 @@ export class WebClientServer {
logoutEndpointUrl: this.createRequestUrl(req, parsedUrl, '/logout').toString(),
webEndpointUrl: this.createRequestUrl(req, parsedUrl, '/static').toString(),
webEndpointUrlTemplate: this.createRequestUrl(req, parsedUrl, '/static').toString(),
webviewContentExternalBaseUrlTemplate: './webview/{{uuid}}/',

updateUrl: this.createRequestUrl(req, parsedUrl, '/update/check').toString(),
Copy link
Member Author

@code-asher code-asher Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
Expand All @@ -343,7 +374,7 @@ export class WebClientServer {
// the sha is the same as in src/vs/workbench/services/extensions/worker/httpWebWorkerExtensionHostIframe.html
`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:;`,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@code-asher code-asher Nov 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha!

unique service workers per webview

Sounds like this was the main key to fix this!

'worker-src \'self\' data:;',
'style-src \'self\' \'unsafe-inline\';',
'connect-src \'self\' ws: wss: https:;',
Expand Down
8 changes: 5 additions & 3 deletions src/vs/workbench/browser/web.main.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,13 +102,15 @@ class BrowserMain extends Disposable {
// Startup
const instantiationService = workbench.startup();

/** @coder Initialize our own client-side additions. */
/**
* Initialize our own client-side additions.
*
* @author Coder
*/
if (!this.configuration.productConfiguration) {
throw new Error('`productConfiguration` not present in workbench config');
}

const codeServerClientAdditions = this._register(instantiationService.createInstance(CodeServerClientAdditions, this.configuration.productConfiguration));

await codeServerClientAdditions.startup();

// Window
Expand Down