adopted-ember-addons / ember-electron

:zap: Build, test, compile and package desktop apps with Ember and Electron
https://ember-electron.js.org/
Other
805 stars 109 forks source link

Modify `app/index.html` with Content-Security-Policy #341

Closed jacobq closed 6 years ago

jacobq commented 6 years ago

Electron v2.x emits a security warning in the console if the page does not define a Content-Security-Policy. Would it be possible to add something like the code below to app/index.html (e.g. above {{content-for "head"}})? I think this would make it easier for users to discover how to address this warning without breaking their code. (Or maybe we do want to break potentially insecure code by default and require opt-out of protection?)

    <!-- To eliminate the security warning in Electron 2.x, remove 'unsafe-eval'
         from the Content-Security-Policy specified in the <meta> below.
         Note that this may break your application if your code (or a dependency) requires these capabilities.
         In that case, you may see an error similar to this:
         Uncaught EvalError: Refused to evaluate a string as JavaScript because 'unsafe-eval'
         is not an allowed source of script in the following Content Security Policy directive...
    -->
    <meta http-equiv="Content-Security-Policy" content="script-src 'unsafe-inline' 'unsafe-eval' serve://dist/">
bendemboski commented 6 years ago

These security warnings are the result of changes made to Electron and have nothing really to do with ember-electron. If you think they need more clarification or something, I would think you'd want to file that with Electron, not ember-electron. I could potentially see an FAQ section about this in ember-electron's documentation, possibly pointing the user to ember-cli-content-security-policy, but it doesn't seem right to me to modify our code because we think Electron's warnings aren't informative enough...

jacobq commented 6 years ago

I don't think I've articulated myself very well here, so I'll try again.

jacobq commented 6 years ago

I was not aware of ember-cli-content-security-policy. Would it make sense for ember-electron to use this addon? That would solve this nicely, I think.

bendemboski commented 6 years ago

Okay, here's what I'm having trouble understanding. Content security policy is a general web standard -- not specific to Electron in any way. Users of Ember that are security-conscious need to worry about it, and users of Electron that are security conscious need to worry about it. ember-electron integrates Ember and Electron, so what it is about that integration that introduces some new problems or complexity or something such that we need to educate users in additional ways to how Ember and Electron educate users?

Put another way, if this warning is good enough for vanilla-Electron users, why isn't it also good enough for ember-electron users? Wouldn't a user that creates a simple Electron project without adding a CSP meta tag be in exactly the same situation? Does ember-electron do something specific that triggers CSP warnings that we should be trying to mitigate? Or does some collision of Ember behavior and Electron behavior cause additional problems?

jacobq commented 6 years ago

Your point is well taken. No, I do not think there are any "new problems" created here. It's also not necessarily the responsibility of this project to educate users about these things. However, the flip side is that it isn't really anyone's responsibility to teach them, so I suspect that most (especially those of us who are not primarily web developers) just end up spending some time searching the Internet to find out what they need to know. While this is OK in so much as there is nothing stopping people from figuring it out, I think it's fair to say (as I am still a fledgling ember developer) that it is not an ideal experience for new developers (i.e. that time could have been better spent elsewhere).

For example, even after learning what the various CSP options do and how to set them via <meta>, not only was I not sure where to set them (app/index.html, environment.js, what about tests, etc.? ), but it didn't occur to me that there might be an addon available to solve this exact problem.

If you approve, I will create a PR to add a "security" page to the FAQ with an entry mentioning the warning and how it can be avoided by using ember-cli-content-security-policy (because that is what I wish I would've known when I was trying to solve this on my own). I appreciate hearing your thoughts on this, and you've convinced me through our discussion thus far that ember-electron itself shouldn't mess with this.

bendemboski commented 6 years ago

Yeah, I think an FAQ is a great idea. I also think it's worthwhile to hear from @felixrieseberg on this -- I believe he's been driving a lot of the focus on security education/warnings in Electron, so he might have some thoughts on if/what we should do in ember-electron beyond relying on Electron's warnings + an FAQ.