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

Account for Windows and MSFT Edge limitations when loading WebViews #42

Merged
merged 9 commits into from
Sep 27, 2024

Conversation

shruti0085
Copy link
Contributor

@shruti0085 shruti0085 commented Sep 26, 2024

Description of changes:
This change introduces some updates to account for development limitations when using WebViews in Eclipse with Windows and MSFT Edge as the backing browser technology.
Changes:

  • File permissions added to the node executable have been removed for Windows because it otherwise fails to load
  • SWT.Edge browser style is used for Windows (More details here)
  • Edge/Chrome does not support directly loading a local resource on disk into html. This is security limitation present in most modern browsers. To address this issue and for better future scalability and maintenance,
    • a virtual host mapping has been created for the locations on disk where js files are referenced in the html loaded with WebViews.
    • Jetty server is used to create virtual servers for toolkit login view and amazon q chat view respectively. Ports for each are auto-assigned by Jetty based on system availability.
    • Localhost is the only virtual host that works rn, will investigate with a customized name as a follow-up

In addition some security recommendations has been added for chat webview CSP meta

  • default-src 'none'; - By default, do not allow any resources to load
  • script-src {javascriptFilePath} 'unsafe-inline' - Allow inline JS to load in http://{virtualhost}/amazonq-ui.js, which is required for mynah-ui to run
  • style-src {javascriptFilePath} 'unsafe-inline'; - Allow inline css to load in http://{virtualhost}/amazonq-ui.js, which is required for mynah-ui to run
  • img-src 'self' data: - Allow inline images (svg in particular) that start with data: to load in the same origin
  • object-src 'none'; base-uri 'none'; recommended by security internally
  • Added upgrade-insecure-requests to CSP

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Comment on lines 66 to 68
if (platform == PluginPlatform.WINDOWS) {
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's try checking for a POSIX compatible file system for a more robust check:

    public static boolean hasPosixFilePermissions(Path path) {
        return path.getFileSystem().supportedFileAttributeViews().contains("posix");
    }

if (platform == PluginPlatform.WINDOWS) {
return SWT.EDGE;
}
return SWT.NATIVE;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make the default explicitly SWT.WEBKIT. The other types seem to be deprecated but we don't want to leave the mapping up to chance.

* @param jsPath
* @return server launched
*/
protected Server setupVirtualServer(final String jsPath) {
Copy link
Contributor

Choose a reason for hiding this comment

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

My preference would be for this piece to live in a separate class that abstracts away the internal Server - could be WebviewAssetServer with a resolve(path) method (and stop).

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 like the idea. Updated

protected Server setupVirtualServer(final String jsPath) {
Server server = null;
try {
server = new Server(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Glad this worked.

@breedloj breedloj merged commit 5714b53 into main Sep 27, 2024
1 check passed
@breedloj breedloj deleted the shruti0085/setupQ branch September 27, 2024 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants