disable nodeintegration #453

n-riesco · 2018-06-05T16:05:11Z

The main purpose of this PR is to disable nodeIntegration and 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:

2e09698 (issue Query panel doesn't open for non-sql databases #451, query panel doesn't open for non-sql connectors)
2344f2c (rebased PR electron-builder: force dtrace-provider rebuild #422))

@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-id and db-connector-user over 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:
- ce0be62

Fixes https://github.com/plotly/streambed/issues/11017

* 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.

* See https://github.com/plotly/streambed/issues/10436

* 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

Fixes #451

Closes #421 Closes #422

scjody

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.

n-riesco · 2018-06-05T20:14:46Z

@scjody yes, db-connector-auth-enabled works as you described.

tarzzz · 2018-06-06T16:05:38Z

backend/main.development.js

@@ -65,6 +65,12 @@ app.on('ready', () => {
        if (!url.startsWith(HTTP_URL)) event.preventDefault();
    });

+    // prevent navigation out of HTTP_URL
+    // see https://electronjs.org/docs/api/web-contents#event-will-navigate


Minor 🐐 The link is wrong .. should link to "new-window"

tarzzz · 2018-06-06T16:06:12Z

backend/main.development.js

@@ -68,7 +68,8 @@ app.on('ready', () => {
    // prevent navigation out of HTTP_URL
    // see https://electronjs.org/docs/api/web-contents#event-will-navigate
    mainWindow.webContents.on('new-window', (event, url) => {
-        if (!url.startsWith(HTTP_URL)) event.preventDefault();
+        event.preventDefault();
+        shell.openExternal(url);


tarzzz · 2018-06-06T16:20:03Z

app/components/Login.react.js

 class Login extends Component {
    constructor(props) {
        super(props);
        this.state = {
+            clientId: cookie.load('db-connector-client-id'),


its oauth2 client id, so we can probably name it like that. DB connector in itself doesn't have a notion of client-id.

tarzzz · 2018-06-06T16:20:26Z

app/components/Login.react.js

@@ -124,25 +101,41 @@ class Login extends Component {

    }
    buildOauthUrl() {
-        const {domain} = this.state;
-        /* global PLOTLY_ENV */
-        const oauthClientId = PLOTLY_ENV.OAUTH2_CLIENT_ID;


Good to get rid of this.. #Cleanup.. :)

tarzzz · 2018-06-06T16:25:36Z

backend/preload.js

+            }
+        },
+        ...propertyOptions
+    });


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.. !!

@tarzzz
This change only affects to the electron app.
What checks do you have in mind?

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

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 · 2018-06-06T18:21:19Z

I've checked login to Plotly Cloud works in the desktop and web apps.

n-riesco added 11 commits May 31, 2018 16:42

app: prevent new windows out of initial origin

5fff066

Fixes https://github.com/plotly/streambed/issues/11017

app: replace 'new-window` with shell.openExternal

90e5f7d

* This will help get rid of `shell.openExternal` in the frontend.

Settings: silence uncaught promise

dbd1e82

routes: fix URL to csv file returned by /datacache

8b16fc0

* Remove extra slash and ensure an absolute path is returned

Login: don't use shell.openExternal

ea0ffa7

Login: remove build-time global variables

6100bba

* See https://github.com/plotly/streambed/issues/10436

oauth: fix authorisation in the web app

f362921

* 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

app: disable nodeIntegration

ce0be62

* 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

Query: enable query panel for non-sql connectors

2e09698

Fixes #451

electron-builder: force dtrace-provider rebuild

2344f2c

Closes #421 Closes #422

scjody reviewed Jun 5, 2018

View reviewed changes

scjody approved these changes Jun 6, 2018

View reviewed changes

tarzzz reviewed Jun 6, 2018

View reviewed changes

tarzzz approved these changes Jun 6, 2018

View reviewed changes

n-riesco added 2 commits June 6, 2018 18:16

doc: fix link

f35bbca

oauth2: rename cookie

0ee49a2

* 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 June 6, 2018 18:13

n-riesco merged commit 27f04fd into master Jun 6, 2018

n-riesco deleted the security/disable-nodeintegration branch June 6, 2018 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disable nodeintegration #453

disable nodeintegration #453

n-riesco commented Jun 5, 2018

scjody left a comment

n-riesco commented Jun 5, 2018

tarzzz Jun 6, 2018

tarzzz Jun 6, 2018

tarzzz Jun 6, 2018

tarzzz Jun 6, 2018

tarzzz Jun 6, 2018

n-riesco Jun 6, 2018

tarzzz Jun 6, 2018

tarzzz left a comment

n-riesco commented Jun 6, 2018

disable nodeintegration #453

disable nodeintegration #453

Conversation

n-riesco commented Jun 5, 2018

scjody left a comment

Choose a reason for hiding this comment

n-riesco commented Jun 5, 2018

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

n-riesco Jun 6, 2018

Choose a reason for hiding this comment

tarzzz Jun 6, 2018

Choose a reason for hiding this comment

tarzzz left a comment

Choose a reason for hiding this comment

n-riesco commented Jun 6, 2018