disable nodeintegration #453
Conversation
* This will help get rid of `shell.openExternal` in the frontend.
* Remove extra slash and ensure an absolute path is returned
* Replace call to `shell.openExternal` with an `<a>` tag. * Don't use /datacache to save CSV files locally, because: - it's unnecessary, - and `<a>` can't be used to open `file://` URLs. * Convert CSV file to data URL. * Capture requests to open a data URL and show native a save dialog instead.
* Ensure the cookie db-connector-user is also passed to HTTP connections. * Trigger the reload of the web app when the authorisation popup is closed. Fixes #260
* Disables nodeIntegration by moving all the use of electron's API in the frontend to the object `window.$falcon` (that is setup inside the preload script). Closes #438
scjody
left a comment
scjody
left a comment
There was a problem hiding this comment.
To be clear: is the db-connector-auth-enabled cookie only used by the frontend to control what UI is shown? In other words I want to be sure it's not used by the backend to control whether or not auth is actually needed.
|
@scjody yes, |
backend/main.development.js
Outdated
| }); | ||
|
|
||
| // prevent navigation out of HTTP_URL | ||
| // see https://electronjs.org/docs/api/web-contents#event-will-navigate |
There was a problem hiding this comment.
Minor 🐐 The link is wrong .. should link to "new-window"
backend/main.development.js
Outdated
| mainWindow.webContents.on('new-window', (event, url) => { | ||
| if (!url.startsWith(HTTP_URL)) event.preventDefault(); | ||
| event.preventDefault(); | ||
| shell.openExternal(url); |
app/components/Login.react.js
Outdated
| constructor(props) { | ||
| super(props); | ||
| this.state = { | ||
| clientId: cookie.load('db-connector-client-id'), |
There was a problem hiding this comment.
its oauth2 client id, so we can probably name it like that. DB connector in itself doesn't have a notion of client-id.
| buildOauthUrl() { | ||
| const {domain} = this.state; | ||
| /* global PLOTLY_ENV */ | ||
| const oauthClientId = PLOTLY_ENV.OAUTH2_CLIENT_ID; |
There was a problem hiding this comment.
Good to get rid of this.. #Cleanup.. :)
| } | ||
| }, | ||
| ...propertyOptions | ||
| }); |
There was a problem hiding this comment.
This is a good idea to only expose the functionality we need, but would be great to check this with both electron and browser / onprem.. !!
There was a problem hiding this comment.
@tarzzz
This change only affects to the electron app.
What checks do you have in mind?
There was a problem hiding this comment.
This change only affects to the electron app.
Yep, I noted it now. Only electron-app testing then (which I can see you have already done)..
tarzzz
left a comment
tarzzz
left a comment
There was a problem hiding this comment.
Looks good.. Some minor comments and 💃 after testing..
Thanks for the cleanup .. !!
* Renamed cookie `db-connector-client-id` to `db-connector-oauth2-client-id`.
n-riesco
force-pushed
the
security/disable-nodeintegration
branch
from
e7ea62d to
0ee49a2
Compare
|
I've checked login to Plotly Cloud works in the desktop and web apps. |
The main purpose of this PR is to disable
nodeIntegrationand strengthen the security measures in Falcon's frontend. To achieve this, I had to refactor some of the login code (so I took the opportunity to fix #260).I've also included 2 other small fixes:
@tarzzz could you review this PR, please?
@scjody are you OK with the changes in 6100bba and f362921 ? With these changes, we send the cookies
db-connector-auth-enabled,db-connector-client-idanddb-connector-userover HTTP.@kndungu I think you'll be interested in reviewing the following commits (related to security in an electron app):
to capture the event
new-window:to disable
nodeIntegration: