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

Warn if loading remote content with nodeIntegration #10708

Merged
merged 2 commits into from Oct 6, 2017

Conversation

Projects
None yet
7 participants
@felixrieseberg
Member

felixrieseberg commented Oct 6, 2017

I'm concerned about more and more users loading remote content with nodeIntegration enabled. To make sure that developers are educated, I'd like to add the following warning:

screen shot 2017-10-06 at 1 02 41 pm

@felixrieseberg felixrieseberg requested a review from electron/reviewers as a code owner Oct 6, 2017

@codebytere

codebytere approved these changes Oct 6, 2017 edited

definitely an important change given the recent proliferation of apps doing this 👍

@felixrieseberg felixrieseberg merged commit 1761d5d into master Oct 6, 2017

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-mas-x64 Build #5336 succeeded in 13 min
Details
electron-osx-x64 Build #5324 succeeded in 13 min
Details

@felixrieseberg felixrieseberg deleted the remote-node-warning branch Oct 6, 2017

@zeke

This comment has been minimized.

Member

zeke commented Oct 7, 2017

👍

@alespergl

My instinct tells me that it would be preferable to do multiple string comparisons instead of the regex - for performance reasons mostly, as parsing and matching the regex will be orders of magnitude slower. Also, the regex should match only the beginning of the string by having a ^ at the front.

@felixrieseberg

This comment has been minimized.

Member

felixrieseberg commented Oct 7, 2017

Fair point! #10713

@ckerr

Nice change. 👍

@jeeftor

This comment has been minimized.

jeeftor commented Jun 1, 2018

Is there a way to turn this warning off? its kind of annoying?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Jun 1, 2018

@jeeftor The only way to make the warning go away is to follow it's advice, disable nodeIntegration for remote content. Loading remote content with node intergration enabled is an incredibly dangerous and insecure thing to do, we added these warnings for a reason

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment