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

feature: Hot security tips #11810

Merged
merged 13 commits into from
Feb 3, 2018
Merged

feature: Hot security tips #11810

merged 13 commits into from
Feb 3, 2018

Conversation

felixrieseberg
Copy link
Member

@felixrieseberg felixrieseberg commented Feb 1, 2018

Now that we have this fancy security checklist, we need to make sure that developers know that it's there.

This PR generalizes the one Electron Security Warning that we already have and adds new warnings for all of the checklist items that are easy to detect. The warnings are only emitted if the binary's name is electron, indicating strongly that a developer is looking at the console.

The detection and subsequent logs can be force-set with the environment variables ELECTRON_DISABLE_SECURITY_WARNINGS and ELECTRON_ENABLE_SECURITY_WARNINGS. One can also set those properties on the window object to enable or disable them per-renderer.

If you mess up greatly, you'd see something like this:

screen shot 2018-02-01 at 3 02 44 pm

@felixrieseberg felixrieseberg requested a review from a team February 1, 2018 23:03
@zeke
Copy link
Contributor

zeke commented Feb 2, 2018

🔥

@groundwater
Copy link
Contributor

I ❤️ this.

I would want to wrap this into a major version bump, even though you've taken a lot of care to be non-disruptive.

If we can get this into 2.0-beta I am 👍 but I would 👎 delaying the beta for this change. Since we're doing major bumps every chromium release a 3.0 isn't really far off anyways.

@ckerr
Copy link
Member

ckerr commented Feb 2, 2018

I don't know what timeline @felixrieseberg has in mind for this, eg tracking down the CI issue, but FWIW I'd be happy to merge this whenever it's ready, either in a 2.0 beta next week, or in 3.0

🔥

@felixrieseberg
Copy link
Member Author

felixrieseberg commented Feb 3, 2018

@ckerr Alright! Feeling pretty good about this now. I believe that the test failure on Linux x64 is unrelated, maybe we could restart it.

EDIT: Restarted the build for Linux 64.

@ckerr ckerr merged commit d586ef2 into master Feb 3, 2018
@ckerr ckerr deleted the security-hot-tips branch February 3, 2018 14:50
sethlu pushed a commit to sethlu/electron that referenced this pull request May 3, 2018
* 🔧 Add security issue detection (and logs)

* 🔧 Check for it on load

* 👷 Add some tests

* 👷 Make the linter happy

* 🔧 Allow them to be enabled by force

* 📝 Make message slightly prettier

* 🔧 Fix a typo in the code comment

* 🔧 Classic mistake

* 🚀 Optimize things a bit more

* 👷 Add tests, fix tests

* 📝 Document things

* 🔧 Make linter happy

* 🔧 One more piece of cleanup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants