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

[Security] Content-Security-Policy warnings with ember-electron #349

Closed
gossi opened this issue May 16, 2018 · 28 comments · Fixed by #695
Closed

[Security] Content-Security-Policy warnings with ember-electron #349

gossi opened this issue May 16, 2018 · 28 comments · Fixed by #695

Comments

@gossi
Copy link

gossi commented May 16, 2018

The recent releases of electron fixed a bunch of XSS vulnerabilities so I also updated my verison of electron-prebuilt-compile to 2.0.0. Which introduced a couple of security warnings, which I was able to fix, except one for CSP with ember-electron.

In the document about security for electron they mention a restricted policy for script tags. As suggested I installed the ember-cli-content-security-policy addon. It's great, it adds all the relevant live-reload related urls and stuff. By default it adds them as header when running ember s but fails adding this with ember electron and the serve protocol. It can be configured to use a <meta> element instead.

What I find out, regarding ember-electron. In order to make it work, you must set script-src to include 'unsafe-inline' because of the <script> element that is added to the body. Second is I had to add 'unsafe-eval' to script-src to make the actual ember code work in electron. I'm sceptical about this, whether I turned something wrong.

What I ended up with, was setting a CSP to a value that shouldn't be set as by the security guidelines of electron.

I would wish for some guidance on this topic, because we as developers are quick to lower the barriers to make things work but actually break security with that.

@jacobq
Copy link
Collaborator

jacobq commented May 17, 2018

It should be possible to run w/o needing to allow all of those "unsafe" things, but it depends a lot on your code and its dependencies. For example, I needed one because I use plotly.js, which currently needs this.
BTW, this issue looks like it is closely related to #341

@jacobq
Copy link
Collaborator

jacobq commented Jul 15, 2018

@gossi Have you learned more about this? Do you have any suggestions for how we could improve the ember-electron documentation or code in this regard?

@gossi
Copy link
Author

gossi commented Jul 16, 2018

No I haven't really. If there is some dependency (or transient dependency) it's basically fishing for the unknown for it and it is out of the responsibility of ones code, not much we can do about it.

What I can say for sure is: ember-cli-hot-loader doesn't work with ember-electron most basically because it's eval'ing in the new javascript send in

@jacobq
Copy link
Collaborator

jacobq commented Jul 16, 2018

ember-cli-hot-loader doesn't work with ember-electron most basically because it's eval'ing in the new javascript sent in.

I assume you mean that it doesn't work when using strict CSPs, right? (It could "work" with lax / insecure settings.) I guess what I'm trying to figure out is what the next steps are for resolving this issue -- do we just need improved docs (with which I feel qualified to help) or is there something in the code?

@gossi
Copy link
Author

gossi commented Jul 18, 2018

It didn't work with lax settings for me, I enabled 'unsafe-eval' and it didn't work.

Regarding your original question, I have no idea what can be done code-wise to tackle this. For documentation I think these things help:

  • I can be, that even your code looks fine, some dependencies trigger security concerns for electron, e.g. plotly.js for you. It is very hard to find those libraries (if at all). But a warning, your deps have an impact on this
  • Do use ember-cli-content-security-policy addon to handle CSP in a nice way

Does that help?

@jacobq
Copy link
Collaborator

jacobq commented Jul 18, 2018

@gossi Thanks for your reply; that helps clarify a little bit, but I still don't quite understand what constitutes "working" and "not working" in this context (e.g. if the app runs correctly but the security warnings appear in dev is it "working" or "not working"?)

As for documentation, did you see the security FAQ page about this? It shows how to set up ember-cli-content-security-policy using the <meta> tag option and reasonable default settings.

Also, I'm not entirely sure what is specific to ember-electron here. If you have a restrictive CSP (e.g. no unsafe-eval or unsafe-inline) in any web or electron app won't you have this same problem? (I was asking Ben about something like this once before, which was when I learned about the ember-cli-content-security-policy addon.)

If I understand you correctly, these are some of the current pain points:

  • Offending dependencies are hard to track down
  • It is often not possible to fix upstream problems except by reducing security via unsafe-inline and unsafe-eval
  • Some ember tools that normally work, like ember-cli-hot-loader don't work with ember-electron apps (I haven't seen this for myself, but it sounds like that was your experience)

@gossi
Copy link
Author

gossi commented Jul 18, 2018

I set up e-csp correctly using <meta> anyway it won't work with electron 😄

I had to add 'unsafe-eval'. Whenever you do this, electron will put up the warning message in the console (should be added to the docs as well), so for some reasons, you have to.

For ember-cli-hot-loader (I used the first public version) that did break my app. After reading about, it evals new code and also it was proclaimed it's highly unlikely to work with CSP in place (I read that somewhere). Maybe that's something, that can be worked out.

Anyway you summary nails it 👍

@evoactivity
Copy link
Contributor

unsafe-eval is most likely down to using ember-auto-import, you can configure it to not use eval by passing the forbidEval options. https://github.com/ef4/ember-auto-import#customizing-build-behavior

My CSP ended up looking like this

// ember-cli-content-security-policy
contentSecurityPolicy: {
  "default-src": ["'none'"],
  "script-src": ["'self' 'unsafe-inline' http://localhost:7020"],
  "font-src": ["'self'"],
  "connect-src": ["'self'"],
  "img-src": ["'self'"],
  "style-src": ["'self'"],
  "media-src": ["'self'"],
},
contentSecurityPolicyMeta: true,

I had to manually add the http://localhost:7020 to get the live reload script to load.

@RobbieTheWagner
Copy link
Member

@evoactivity this setup works to remove the warnings for you and you get a working app when you build for production?

@evoactivity
Copy link
Contributor

I'll let you know in the next day or so, I'm still getting a new project setup and have not tried a production build just yet.

@evoactivity
Copy link
Contributor

@rwwagner90 I've just tried ember electron:package and ran the resulting app, there were no warnings logged and the app ran as expected with the CSP settings outlined above and forbidEval set to true for ember-auto-import.

@RobbieTheWagner
Copy link
Member

@evoactivity nice! Would you be open to submitting a PR adding this info to the docs?

@RobbieTheWagner
Copy link
Member

@evoactivity does running ember t work for you? When I add in CSP, ember t seems to hang for me.

@RobbieTheWagner
Copy link
Member

False alarm, I had to remove frame-src and then tests ran. However, ember electron:test still fails. I guess we need extra config for Electron tests?

@RobbieTheWagner
Copy link
Member

@evoactivity can you confirm that ember electron:test hangs for you? My guess is we need something like http://testemserver/ in the CSP somewhere, so testem can connect, but I haven't been able to figure that out. @bendemboski any ideas?

@bendemboski
Copy link
Member

Yeah, we load testem.js from http://testemserver/testem.js, and then I think it dynamically creates an iframe that points to http://testemserver/connection.html or something like that. The test tooling that we install in the main process intercepts them and 304-redirects them to http://localhost:<testem's port>/..., but the browser first seems them as http://testemserver/... URLs.

I don't use CSP myself, so I can't really give any more specific info.

@evoactivity
Copy link
Contributor

Sorry for not replying sooner @rwwagner90, I've been away from my project using this for the last week or so, I should be back on it today and I'll let you know if I see the same behaviour or find a way around it.

@RobbieTheWagner
Copy link
Member

@bendemboski so we probably need to allow both http://testemserver and http://localhost:<testem's port>. Is there any way to know what testems port is going to be or set it explicitly?

@bendemboski
Copy link
Member

I think you can for all practical purposes count on the testem port being 7357. That's testem's default, and I don't think we have any tooling to override it except maybe testem-electron.js (although I'm not sure if you can even configure it there). It's complicated because ember-cli jumps through some hoops to support running the testem server and the Ember dev server run on the same port, but we don't have a dev server, so I forget exactly how all that fits together.

So I think you could get away with that. The only place I know of where we know the testem URL/port for sure (rather than assuming we're using testem's default port) is here, but that's running in the main process at runtime. Maybe if we rewrite tests/index.html dynamically at launch time (one of the things we discussed re: embroider support) then we could look for and rewrite the CSP meta tag.

So yeah, it's complicated™️ but you could probably get away with just assuming http://localhost:7357.

@RobbieTheWagner
Copy link
Member

@bendemboski hmm, no luck with this:

contentSecurityPolicy: {
      'default-src': ["'none'"],
      'script-src': [
        'http://localhost:7020',
        'http://localhost:7357',
        'http://testemserver',
        "'self'",
        "'unsafe-inline'"
      ],
      'font-src': ["'self'"],
      'connect-src': ["'self'"],
      'img-src': ['data:', "'self'"],
      'style-src': ["'self'", "'unsafe-inline'"],
      'media-src': ["'self'"]
    },
    contentSecurityPolicyMeta: true,

@bendemboski
Copy link
Member

I haven't CSP'ed in a while, but don't you see messages about the blocked content in the console, or can't you enable a warning mode or something to get the browser to tell what it's blocking and why?

@RobbieTheWagner
Copy link
Member

@bendemboski I am not sure. I think typically, when running the code in the browser, you get something in the console about it, but not sure if the same is true for Electron. I think it may not know it's blocking anything, since it's only blocking testem stuff, and the part it is blocking is only giving control back to the terminal.

@bendemboski
Copy link
Member

I'd be quite surprised if Electron suppressed that kind of console warning...if you run ember electron:test -s and then open the dev tools, do you see anything in the console?

@RobbieTheWagner
Copy link
Member

@bendemboski yeah, I did that yesterday and got all the ones I could fixed. I think the only ones not fixable are the contentFor ones. There are a couple open issues about that and here is one of them adopted-ember-addons/ember-cli-content-security-policy#149. I wonder if we should manually add in the script content via some blueprints instead of contentFor? Then we could add a nonce. Perhaps we could also add it via contentFor somehow, but that is unclear.

@RobbieTheWagner
Copy link
Member

With this PR #695 everything works with this config:

contentSecurityPolicy: {
      'default-src': ["'none'"],
      'script-src': [
        'http://localhost:7020',
        'http://localhost:7357',
        'http://testemserver',
        "'nonce-ember-electron-shim-head'",
        "'nonce-ember-electron-shim-footer'",
        "'nonce-ember-electron-shim-test-head'",
        "'self'",
        "'unsafe-inline'"
      ],
      'font-src': ["'self'"],
      'frame-src': ['http://localhost:7357', 'http://testemserver/', "'self'"],
      'connect-src': ["'self'"],
      'img-src': ['data:', "'self'"],
      'style-src': ["'self'", "'unsafe-inline'"],
      'media-src': ["'self'"]
    },
    contentSecurityPolicyMeta: true,
    ```

@jelhan
Copy link

jelhan commented Jan 21, 2021

Ember CLI Content Security Policy should inject the source required to run tests with testem and to support Ember CLI live reload automatically. I'm wondering why this is not working for Ember Electron apps. Is Ember Electron doing something special in regards to this two features?

You can find tests for both here:

Did you tested with latest pre-release of Ember CLI Content Security Policy? This is currently v2.0.0-2. The latest stable release (v1.1.1) is very old and had way less test coverage.

Sadly there is no good work-a-round for scripts injected by an addon in a contentFor hook yet. I discussed this here in detail: adopted-ember-addons/ember-cli-content-security-policy#149 (comment)

@RobbieTheWagner
Copy link
Member

@jelhan yeah, we have some hacks to make testem work. @bendemboski knows more than I do, but essentially since electron doesn't run a server and runs from file urls, we are not able to use the normal testem setup.

@jacobq
Copy link
Collaborator

jacobq commented May 10, 2022

In case anyone else stumbles upon this nowadays, I want to make a note that ember-cli-content-security-policy now uses config/content-security-policy.js rather than the environment for its configuration.

https://github.com/rwjblue/ember-cli-content-security-policy/blob/8f2bb08841c39973295d6129981f9fa07f3d8028/DEPRECATIONS.md

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 a pull request may close this issue.

6 participants