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

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

Closed
jacobq opened this issue Mar 1, 2018 · 6 comments
Closed

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

jacobq opened this issue Mar 1, 2018 · 6 comments

Comments

@jacobq
Copy link
Collaborator

jacobq commented Mar 1, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

jacobq commented Mar 1, 2018

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

  • Though these security warnings are only shown in new versions of Electron, the vulnerabilities to which they draw attention pre-date the warnings, so users stand to benefit from heeding them.
  • The goal of this issue is not to suppress/hide the warning (could do that via process.env.ELECTRON_DISABLE_SECURITY_WARNINGS = true) but to address the underlying risk.
  • The suggested way to address the "Insecure Content-Security-Policy" warning (which will be shown in a new/empty ember-electron app using electron 2.x) is to set a Content-Security-Policy. However, some developers may not be familiar with this (I had to do some reading to understand it), so it would be nice if we could help direct them.
  • Your idea about an FAQ entry seems like a good alternative to actually changing code. Maybe that's the best path.
  • Regarding why this applies to ember-electron, there is nothing that should be changed upstream in electron (the warnings are a good thing). Similarly, it doesn't make much sense to change the blueprint in ember-cli/ember since non-electron apps have other ways of dealing with this (i.e. HTTP headers).
  • What I'm suggesting is some sort of check during ember install ember-electron that looks for a <meta> setting Content-Security-Policy. If one is not found, the user should be prompted to add one and given an appropriate link for more info (either electron security info page or ember-electron FAQ)

@jacobq
Copy link
Collaborator Author

jacobq commented Mar 1, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

jacobq commented Mar 1, 2018

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
Copy link
Member

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.

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

No branches or pull requests

2 participants