fix: disable nodeIntegration & insecure resource warnings for localhost #18814

jerry1100 · 2019-06-15T09:23:45Z

Description of Change

This PR removes the "node integration with remote content" and "loading insecure content" warning messages when loading from localhost.

A lot of electron projects use webpack-dev-server for development, which involves "remote" content being loaded over an insecure connection from localhost, resulting in at least two warning messages in the console.

The warning messages are annoying and they confuse people. Worst, they can actually be counterproductive as people are resorting to disabling the security check just to get rid of the warnings, which means they won't see them when it really matters.

Note: I explicitly chose not to test against 127.0.0.1 because although it should be the same as localhost, the latter is much more prevalent. Also, that would break a lot of tests.

Other changes:

added isLocalhost() to check if the current window's hostname is localhost
also ignoring enabled remote module with remote content when loading from localhost

Checklist

npm test passes (I'm having issues with setting up the project, I'll leave it up to CI)
tests are changed or added
PR title follows semantic commit guidelines
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: "Node integration with remote content" and "loading insecure content" warning messages are suppressed for localhost connections.

welcome · 2019-06-15T09:23:48Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

jerry1100 · 2019-06-15T10:33:54Z

Looks like the tests are failing.

"Node integration with remote content" is failing because it's waiting for a console message that never comes (since it's suppressed). I'm not sure how to test this but here are some ideas:

Keep the console-message listener and add a did-finish-load listener that will pass the test. The idea is that the warning, if it were to come, will come before the did-finish-load event. Not sure if that's guaranteed to be the case though.
Pass the test after a fixed amount of time. Perhaps 10s? Something way longer than it would take the warning to show.

not ok 1124 security warnings should not warn about Node.js integration with remote content from localhost
  Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (C:\projects\src\electron\spec\security-warnings-spec.js)
      at Test.Runnable._timeoutError (C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:440:10)
      at C:\projects\src\electron\spec\node_modules\mocha\lib\runnable.js:251:24

"Loading insecure resources" is failing because the resource that's being loaded (which is correctly filtered) is loading a stylesheet from 127.0.0.1, which is not === 'localhost', and thus the warning message shows. To get around this, we can remove the stylesheet since it's redundant to have two insecure resources for testing.

not ok 1132 security warnings without sandbox should not warn about insecure resources from localhost
  AssertionError: expected '%cElectron Security Warning (Insecure Resources)' to not include 'Insecure Resources'
      at C:\projects\src\electron\spec\security-warnings-spec.js:210:34
      at CallbacksRegistry.apply (electron/js2c/renderer_init.js:1752:29)
      at electron/js2c/renderer_init.js:1553:21
      at EventEmitter.<anonymous> (electron/js2c/renderer_init.js:1543:7)
      at EventEmitter.emit (events.js:194:13)
      at Object.onMessage (electron/js2c/renderer_init.js:2343:16)

jerry1100 · 2019-06-16T08:45:26Z

For the "node integration with remote content" test, I opted in for using did-finish-load since I didn't like the idea of using a fixed delay. However, like I mentioned in my above post, this runs the risk of passing early if did-finish-load gets fired before console-message (though in my testing it does not).

For the "Loading insecure resources" test, I modified the test to check that the resource it's loading from localhost is not included in the warning message.

jerry1100 · 2019-06-16T20:01:44Z

An unrelated test failed. Does someone mind restarting it?

MarshallOfSound · 2019-06-16T21:01:48Z

@jerry1100

Error: No valid signing identity available to run autoUpdater specs

That spec failure is expected at the moment for fork PRs, we're working on making it work for PRs from forks but for now that failure is expected and ok 👍

spec/security-warnings-spec.js

zcbenz · 2019-06-17T12:27:52Z

Would like to hear more voices from @electron/wg-security before merging.

In warnAboutNodeWithRemoteContent(), add a check to see if the hostname is "localhost" and prevent the warning message if it is.

In warnAboutInsecureResources(), filter out resources from localhost since they are most likely not a threat.

Add tests for ignoring warning messages for the following scenarios: 1. node integration with remote content from localhost 2. loading insecure resources from localhost

Instead of relying on the "did-finish-load" event, which may result in a race condition, add an "onload" handler that logs "loaded" to the console. This will execute _after_ the nodeIntegration check, so it can be safely used as a signal to indicate that the test is done.

jerry1100 · 2019-06-18T04:32:51Z

Made some minor updates:

Added isLocalhost() to reduce code duplication
Handled the newly added "enabled remote module with remote content" warning

jerry1100 · 2019-06-30T05:57:47Z

Bump. Any input from @electron/wg-security?

felixrieseberg · 2019-07-01T23:25:28Z

Hi, here from the security wg. I'm okay with this, this seems like a solid change that doesn't undermine the spirit of warning people when they're about to shoot themselves in the foot.

MarshallOfSound

The only case this will stop warning about but is still valid is folks creating a localhost server to serve content in production. But that's a separate problem that this warning isn't attempting to prevent so 👍 from me

release-clerk · 2019-07-02T10:36:53Z

Release Notes Persisted

"Node integration with remote content" and "loading insecure content" warning messages are suppressed for localhost connections.

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 15, 2019

miniak requested a review from a team June 15, 2019 21:33

electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 16, 2019

zcbenz requested changes Jun 17, 2019

View reviewed changes

spec/security-warnings-spec.js Outdated Show resolved Hide resolved

zcbenz approved these changes Jun 17, 2019

View reviewed changes

jerry1100 added 11 commits June 17, 2019 20:31

fix: disable remote host nodeIntegration warning for localhost

0645d96

In warnAboutNodeWithRemoteContent(), add a check to see if the hostname is "localhost" and prevent the warning message if it is.

fix: disable loading insecure resources warning for localhost

d4844ef

In warnAboutInsecureResources(), filter out resources from localhost since they are most likely not a threat.

test: add tests for ignoring security warnings when using localhost

7ad94a5

Add tests for ignoring warning messages for the following scenarios: 1. node integration with remote content from localhost 2. loading insecure resources from localhost

test: fix insecure resource test

c632828

test: pass nodeIntegration with remote test on did-finish-load

460c196

test: maybe fix node integration test (error w/ conv circular struct)

fac14a1

test: update test description

7380ca9

test: rename base-page-security-load-message.html

2928a33

fix: ignore enabled remote module warning for localhost

212a12b

refactor: add isLocalhost()

d7819de

zcbenz approved these changes Jun 18, 2019

View reviewed changes

felixrieseberg approved these changes Jul 1, 2019

View reviewed changes

MarshallOfSound approved these changes Jul 1, 2019

View reviewed changes

zcbenz merged commit dee3315 into electron:master Jul 2, 2019

jerry1100 deleted the remove-warnings-localhost branch July 3, 2019 04:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: disable nodeIntegration & insecure resource warnings for localhost #18814

fix: disable nodeIntegration & insecure resource warnings for localhost #18814

jerry1100 commented Jun 15, 2019 •

edited

welcome bot commented Jun 15, 2019

jerry1100 commented Jun 15, 2019

jerry1100 commented Jun 16, 2019

jerry1100 commented Jun 16, 2019

MarshallOfSound commented Jun 16, 2019

zcbenz commented Jun 17, 2019

jerry1100 commented Jun 18, 2019

jerry1100 commented Jun 30, 2019

felixrieseberg commented Jul 1, 2019

MarshallOfSound left a comment

release-clerk bot commented Jul 2, 2019

fix: disable nodeIntegration & insecure resource warnings for localhost #18814

fix: disable nodeIntegration & insecure resource warnings for localhost #18814

Conversation

jerry1100 commented Jun 15, 2019 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Jun 15, 2019

jerry1100 commented Jun 15, 2019

jerry1100 commented Jun 16, 2019

jerry1100 commented Jun 16, 2019

MarshallOfSound commented Jun 16, 2019

zcbenz commented Jun 17, 2019

jerry1100 commented Jun 18, 2019

jerry1100 commented Jun 30, 2019

felixrieseberg commented Jul 1, 2019

MarshallOfSound left a comment

Choose a reason for hiding this comment

release-clerk bot commented Jul 2, 2019

jerry1100 commented Jun 15, 2019 •

edited