-
Notifications
You must be signed in to change notification settings - Fork 12
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
I believe There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This works in the browsers I tested:
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I see what you're referring to now - TIL! It's interesting though, according to the typedefs for I wonder then if I should make an issue upstream to update the typedefs here to accept There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I should also note that TypeScript does allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right! I see that in the typedefs. Weird, it doesn't allow There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, looks like someone already did! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh snap! Nice find. |
||
}, | ||
developmentOptions: { | ||
logLevel: logLevel ? parseLogLevel(logLevel) : undefined, | ||
...config.developmentOptions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess serving icons without a token means they will always load 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ why do we do There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
} | ||
|
||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will this PR resolve coder/code-server#4483 then? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ! I find the type here There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah gotcha. That makes sense.
Yeah, wish I knew where to find out why they do that 🤔 |
||
|
||
// support paths that are uri-encoded (e.g. spaces => %20) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❓ do you know why we pass the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 / | ||
*/ | ||
|
@@ -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(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
}, | ||
|
@@ -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:;`, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The really important part was setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah gotcha!
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:;', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great comment 👏