core,mini-browser: server enhancements

[paul-marechal]

mini-browser: serve on separate origin

The mini-browser currently hosts its services on the same origin than
Theia's main origin. This commit makes the mini-browser serve on its
own origin: {{uuid}}.mini-browser.{{hostname}} by default. Can be
configured with a THEIA_MINI_BROWSER_HOST_PATTERN environment
variable.

core: validate ws upgrade origin

Hosting the mini-browser on its own origin prevents cross-origin
requests from happening easily, but WebSockets don't benefit from the
same protection. We need to allow/disallow upgrade requests in the
backend application ourselves.

This means that in order to know who to refuse, we need to know where we
are hosted. This is done by specifying the THEIA_HOSTS environment
variable. If left empty, no check will be done. But if set, nothing
besides what is written in this var will be allowed. See
BackendApplicationHosts to get the hosts extracted from this var. Note
that the latter is not set when running Electron, since there's no need
to deal with arbitrary origins, everything should be local.

No check is done on the HTTP(s) endpoints because we'll rely on the
browser's SOP and CORS policies to take effect.

A new contribution point is added: WsRequestValidatorContribution.
An extender can implement this contribution to allow or not WebSocket
connections. Internally used to filter origins and Electron's token as
well as checking the origin of requests to prevent some type of XSS.

Signed-off-by: Paul Maréchal [email protected]

Fixes: https://bugs.eclipse.org/bugs/show_bug.cgi?id=568018

How to test

• The mini-browser should work like before:
  • Preview a .html file, open the dev-tools and make sure that the file is served from mini-browser.localhost:3000.
  • Copy the url of the iframe and open it in a new tab.
  • Open the dev-tools and try opening a WebSocket connection to the main Theia application on ws://localhost:3000/services.
  • It should fail.
• The backend should be able to refuse connections from wrong hosts:
  • Shutdown the backend, set THEIA_HOSTS=patate.local:3000.
  • Edit your hosts file to add an entry like 127.0.0.1 patate.local.
  • Run the backend and try accessing the app on http://localhost:3000.
  • It should not work due to the WebSocket connection being refused.
  • Access the app on http://patate.local:3000.
  • It should work.
• Electron should work like before:
  • Should not be affected by THEIA_HOSTS.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[akosyakov]

But it is a hack, no? The point of reimplementing with webview or as a VS Code extension is to solve all other issues with the mini browser, like not playing nicely with the shell, focus, keyboard/mouse events propagations and so on without any new configurations. With this PR we don't solve anything else except making one component secure at cost making the system more complex. It's also try partially reinvent webviews with securing resource access, i.e. solving what already solved. I don't think it is the right direction.

[paul-marechal]

@akosyakov No this is not a hack. Surely, the mini-browser can be implemented somehow else, but this is not the goal of this PR. The current changes to the mini-browser extension are minimal enough to fix what is mentioned in the bugzilla issue by serving content from a different origin. The system is considerably simplier than what is done for WebViews, because once again the goal of this PR isn't to re-implement but to fix.

The most complex thing that was added is the concept of WebSocket validators and adding the notion of known host domains for the backend to validate the origin of incoming WebSocket connections. See this post for more information. This fix is independent from changing how the mini-browser is implemented/hooks into Theia's shell. Right now our main focus is to make the required server changes to fix the bugzilla issue and the re-implementation can be tackled with subsequent work.

We'd really like to have those concerns addressed by merging this PR before next release if possible.

[akosyakov]

Please add the breaking changes section in CHANGELOG, explainign how downstreams should deal with a change.

[paul-marechal]

@akosyakov I just double-checked the changes and I don't think anything is broken, besides ElectronMessagingContribution that is no longer used so I deprecated it. There's already a changelog entry for it. It only affects people that extended this class. Is what I wrote enough or should I write something else?

[akosyakov]

@marechal-p Did you try to run mini browser from Gitpod? Should not all downstreams now reconfigure their proxies, dns servers and so on?

[akosyakov]

I could not start it by the way from Gitpod at all:

gitpod /workspace/theia/examples/browser $ yarn start ../.. --hostname 0.0.0.0
yarn run v1.22.4
$ theia start --plugins=local-dir:../../plugins ../.. --hostname 0.0.0.0
Failed to start the backend application.
Error: Cannot find module '@theia/core/src/node/ws-request-validators'

[paul-marechal]

Silly import src, this is my bad. I only tried my latest updates on Electron and the modules must be ok but not for browser. Will fix.

edit: Forgot to push, it was fixed locally

[paul-marechal]

@marechal-p Did you try to run mini browser from Gitpod? Should not all downstreams now reconfigure their proxies, dns servers and so on?

Downstream don't need to do a thing. Everything should work like before. Except that now one can set THEIA_HOSTS to secure his application. One has to put the domain name where the app is served from, port included if not 80 or 443.

[akosyakov]

@marechal-p but it does not work out of box in Gitpod, only on localhost:

It has to be configured properly.

[paul-marechal]

@akosyakov Ok I now understand the situation in which gitpod is: your DNS doesn't resolve mini-browser.{{hostname}} despite resolving {{uuid}}.webviews.{{hostname}} for WebViews?

A temporary hack, and it would be a hack in this case, would be to make the default pattern like mini-browser.webviews.{{hostname}}. The goal would be to make the already existing DNS rules correctly resolve the hostname.

[akosyakov]

You cannot assume that everybody is using {{uuid}}.webviews.{{hostname}}. It can be {{uuid}}-webviews-{{hostname}} or something else. Please document it as a new independent breaking change.

[paul-marechal]

Just found out about CSP and it looks like it could prevent having to validate origins as the browser would prevent the connections from happening automatically. I'll take some time going through this to see if that's the solution.

[akosyakov]

Just found out about CSP and it looks like it could prevent having to validate origins as the browser would prevent the connections from happening automatically. I'll take some time going through this to see if that's the solution.

We should not mess with remote content but load it as is. If we impose some restrictions on it, it will break user experience. We've been already though all of this while working on webviews. There is no another way than giving own origin to isolate one context from another.

[paul-marechal]

• Added {{uuid}} to the mini-browser host
• Added breaking change in CHANGELOG
• Fixed naming
• Fixed location mapping

[azatsarynnyy]

It would be nice to update the PR's description as well, with the default origin - {{uuid}}.mini-browser.{{hostname}}

[akosyakov]

@vince-fugnitto @marcdumais-work Could you test it please? I will try to find time to check code tomorrow. Hope we land it before release.

[paul-marechal]

• Fixed logic for Electron
• @azatsarynnyy fixed comments, nice catch.

[vince-fugnitto]

@marechal-p I performed a subsequent functional review 👍

• confirmed that the bugzilla .html file is successfully handled, while on master it does not.
• confirmed that the bugzilla extension is successfully handled, while on master it does not.
• confirmed that there are no regressions on both the browser and electron targets when opening html and svg files

[akosyakov]

@vince-fugnitto Could you check please also that dev servers are still working? see #8759 (comment)

[vince-fugnitto]

@vince-fugnitto Could you check please also that dev servers are still working? see #8759 (comment)

@akosyakov I confirmed that the Preview: Open URL command still works correctly with the changes:

[akosyakov]

@marechal-p Could you rebase please? Please also check launch.json and add THEIA_MINI_BROWSER_HOST_PATTERN to env variables similar as we do for webviews.

[akosyakov]

LGTM