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

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

Merged
merged 11 commits into from Jul 2, 2019

Conversation

@jerry1100
Copy link
Contributor

jerry1100 commented Jun 15, 2019

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:

Checklist

Release Notes

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

@welcome

This comment has been minimized.

Copy link

welcome bot commented Jun 15, 2019

💖 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

This comment has been minimized.

Copy link
Contributor Author

jerry1100 commented Jun 15, 2019

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:

  1. 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.
  2. 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)
@miniak miniak requested a review from electron/wg-security Jun 15, 2019
@jerry1100

This comment has been minimized.

Copy link
Contributor Author

jerry1100 commented Jun 16, 2019

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.

@electron-cation electron-cation bot removed the new-pr 🌱 label Jun 16, 2019
@jerry1100

This comment has been minimized.

Copy link
Contributor Author

jerry1100 commented Jun 16, 2019

An unrelated test failed. Does someone mind restarting it?

@MarshallOfSound

This comment has been minimized.

Copy link
Member

MarshallOfSound commented Jun 16, 2019

@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 Outdated Show resolved Hide resolved
@zcbenz
zcbenz approved these changes Jun 17, 2019
@zcbenz

This comment has been minimized.

Copy link
Member

zcbenz commented Jun 17, 2019

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

jerry1100 added 9 commits Jun 15, 2019
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 jerry1100 force-pushed the jerry1100:remove-warnings-localhost branch from 553fc5d to 2928a33 Jun 18, 2019
@jerry1100

This comment has been minimized.

Copy link
Contributor Author

jerry1100 commented Jun 18, 2019

Made some minor updates:

  1. Added isLocalhost() to reduce code duplication
  2. Handled the newly added "enabled remote module with remote content" warning
@zcbenz
zcbenz approved these changes Jun 18, 2019
@jerry1100

This comment has been minimized.

Copy link
Contributor Author

jerry1100 commented Jun 30, 2019

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

@felixrieseberg

This comment has been minimized.

Copy link
Member

felixrieseberg commented Jul 1, 2019

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.

Copy link
Member

MarshallOfSound left a comment

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

@zcbenz zcbenz merged commit dee3315 into electron:master Jul 2, 2019
8 of 9 checks passed
8 of 9 checks passed
build-mac Workflow: build-mac
Details
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP Ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
build-linux Workflow: build-linux
Details
lint Workflow: lint
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

release-clerk bot commented Jul 2, 2019

Release Notes Persisted

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

@jerry1100 jerry1100 deleted the jerry1100:remove-warnings-localhost branch Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.