electron: only allow browser-window #7205
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
paul-marechal
force-pushed
the
mp/mini-browser
branch
from
a6def7c
to
4994c00
Compare
|
It works! Cool! I wonder whether it is possible to enable security for ws upgrade request on express level, not on level of |
akosyakov
reviewed
Feb 24, 2020
packages/core/src/electron-node/token/electron-token-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/core/src/electron-node/token/electron-token-backend-contribution.ts
Show resolved
Hide resolved
packages/core/src/electron-node/token/electron-token-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/core/src/electron-node/token/electron-token-backend-contribution.ts
Outdated
Show resolved
Hide resolved
packages/core/src/electron-node/token/electron-token-backend-module.ts
Outdated
Show resolved
Hide resolved
packages/core/src/electron-node/token/electron-token-validator.ts
Outdated
Show resolved
Hide resolved
|
@marechal-p Could you reference a relevant bugzilla please? i.e. https://bugs.eclipse.org/bugs/show_bug.cgi?id=551747 |
paul-marechal
force-pushed
the
mp/mini-browser
branch
2 times, most recently
from
9a0a909
to
2af89f6
Compare
paul-marechal
commented
Feb 25, 2020
akosyakov
approved these changes
Feb 25, 2020
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>
paul-marechal
force-pushed
the
mp/mini-browser
branch
from
2af89f6
to
7cc8b32
Compare
phosphore
reviewed
Mar 10, 2020
| } | ||
|
|
||
| isTokenValid(token: ElectronSecurityToken): boolean { | ||
| return typeof token === 'object' && token.value === this.electronSecurityToken!.value; |
There was a problem hiding this comment.
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)
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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