Skip to content
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

fix: handle localhost and port in asWebviewUri #130148

Closed
wants to merge 2 commits into from

Conversation

jsjoeio
Copy link
Contributor

@jsjoeio jsjoeio commented Aug 4, 2021

This PR fixes #130151

We encountered this error in code-server (fix here: coder/code-server#3895) and noticed that resources weren't loading in webviews due to the wrong URI being returned from asWebviewUri.

I don't think Codespaces has the issue because it's always running on a standard port and so it won't have a port in the url.

Additional Context

We noticed this happening with a custom VS Code extension having issues loading resources (CSS & JS) in webviews.

I was going to try and reproduce with vscode by running yarn web but I'm not seeing the option to manually install VSIX files 🤔

image

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 4, 2021

Not sure how to fix these 😅

[build/lib/layersChecker.ts]: Reference to 'URL' from 'lib.dom.d.ts' violates layer '**/vs/**/common/**' (/home/runner/work/vscode/vscode/src/vs/workbench/api/common/shared/webview.ts (60,28)
[build/lib/layersChecker.ts]: Reference to 'hostname' from 'lib.dom.d.ts' violates layer '**/vs/**/common/**' (/home/runner/work/vscode/vscode/src/vs/workbench/api/common/shared/webview.ts (61,50)
[build/lib/layersChecker.ts]: Reference to 'port' from 'lib.dom.d.ts' violates layer '**/vs/**/common/**' (/home/runner/work/vscode/vscode/src/vs/workbench/api/common/shared/webview.ts (61,106)
[build/lib/layersChecker.ts]: Reference to 'port' from 'lib.dom.d.ts' violates layer '**/vs/**/common/**' (/home/runner/work/vscode/vscode/src/vs/workbench/api/common/shared/webview.ts (61,133)

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

let authority = `${resource.scheme}+${resource.authority}.${webviewRootResourceAuthority}`;
if (resource.authority.includes('localhost')) {
const authorityUrl = new URL(`http://${resource.authority}`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

To fix the layer errors, try using our URI class instead of the built-in URL class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I tried that before but the URI class doesn't have a port (though it does have it in the authority but then I'd have to split it on the :, right?) Unless I leave the port out entirely?

Copy link

Choose a reason for hiding this comment

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

@mjbvz The URI puts the authority section before the vscode-resource.vscode-webview.net. So the url becomes localhost:8080.vscode-resource.vscode-webview.net which is an invalid port.

Ideally, the URI package could parse the port like URL.

@jsjoeio jsjoeio marked this pull request as ready for review August 6, 2021 18:53
@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 13, 2021

@mjbvz is this patch/fix no longer needed thanks to 649dd18?

@mjbvz
Copy link
Collaborator

mjbvz commented Aug 16, 2021

Possibly? Although I didn't test ports specifically with that fix

Did you have a chance to test the issue from #130151?

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 16, 2021

I spoke to @code-asher who tested it and I believe it could fix it:

> encodeAuthority("localhost:8080")
"localhost-003a8080"

> decodeAuthority("localhost-003a8080")
"localhost:8080"

> new URL("http://localhost-003a8080")
URL { href: "http://localhost-003a8080/", origin: "http://localhost-003a8080", protocol: "http:", username: "", password: "", host: "localhost-003a8080", hostname: "localhost-003a8080", port: "", pathname: "/", search: "" }

I will have to double-check and follow-up. If it does, I could instead add new tests for URLs with ports

@jsjoeio
Copy link
Contributor Author

jsjoeio commented Aug 23, 2021

Closing in favor of #131447

@jsjoeio jsjoeio closed this Aug 23, 2021
@jsjoeio jsjoeio deleted the jsjoeio-fix-aswebviewuri branch August 23, 2021 22:09
@github-actions github-actions bot locked and limited conversation to collaborators Oct 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

asWebviewUri doesn't return the correct Uri when remote authority is localhost:[port]
3 participants