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

electron: only allow browser-window #7205

Merged
merged 1 commit into from Feb 25, 2020
Merged

electron: only allow browser-window #7205

merged 1 commit into from Feb 25, 2020

Conversation

@marechal-p
Copy link
Member

marechal-p commented Feb 24, 2020

What it does

Only allow http request from Electron's own browser-window. Token is
generated within electron-main, which also sets it as a cookie within
browser-windows. Token is passed to the backend via environment
variables. The backend is looking for this specific token to authorize
requests.

How to test

Everything should keep working when running Electron. Make sure we can also display html previews. You should not be able to load http://localhost:<dynamic-port> in your browser anymore.

Review checklist

Reminder for reviewers

@marechal-p marechal-p added the electron label Feb 24, 2020
@marechal-p marechal-p requested review from kittaakos and akosyakov Feb 24, 2020
@marechal-p marechal-p force-pushed the mp/mini-browser branch from a6def7c to 4994c00 Feb 24, 2020
@akosyakov akosyakov added the security label Feb 24, 2020
@akosyakov

This comment has been minimized.

Copy link
Member

akosyakov commented Feb 24, 2020

It works! Cool!
Screenshot 2020-02-24 at 21 16 32

I wonder whether it is possible to enable security for ws upgrade request on express level, not on level of ws? If not I think the current solution is alright, we just should say that MessagingContribution should be always used in Theia, not a custom ws endpoints. Otherwise it will cause security issues for electron.

@akosyakov

This comment has been minimized.

Copy link
Member

akosyakov commented Feb 24, 2020

@marechal-p Could you reference a relevant bugzilla please? i.e. https://bugs.eclipse.org/bugs/show_bug.cgi?id=551747

@akosyakov akosyakov requested a review from thegecko Feb 24, 2020
@marechal-p marechal-p force-pushed the mp/mini-browser branch 2 times, most recently from 9a0a909 to 2af89f6 Feb 25, 2020
Copy link
Member

akosyakov left a comment

It works good for me. Please look at comments though.

Only allow http request from Electron's own browser-window. Token is
generated within electron-main, which also sets it as a cookie within
browser-windows. Token is passed to the backend via environment
variables. The backend is looking for this specific token to authorize
requests.

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

Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@marechal-p marechal-p force-pushed the mp/mini-browser branch from 2af89f6 to 7cc8b32 Feb 25, 2020
@marechal-p marechal-p merged commit b212d07 into master Feb 25, 2020
3 checks passed
3 checks passed
Gitpod Open an online workspace in Gitpod
Details
Travis CI - Pull Request Build Passed
Details
eclipsefdn/eca The author(s) of the pull request is covered by necessary legal agreements in order to proceed!
Details
@marechal-p marechal-p deleted the mp/mini-browser branch Feb 25, 2020
}

isTokenValid(token: ElectronSecurityToken): boolean {
return typeof token === 'object' && token.value === this.electronSecurityToken!.value;

This comment has been minimized.

Copy link
@phosphore

phosphore Mar 10, 2020

I would prefer a constant-time comparison algorithm here to prevent timing attacks (e.g. crypto.timingSafeEqual https://nodejs.org/api/crypto.html#crypto_crypto_timingsafeequal_a_b)

@marechal-p marechal-p mentioned this pull request Mar 10, 2020
1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.