Move inline scripts to public folder, document CSP setup

[RobbieTheWagner]

Closes #349

[jelhan]

Adding a static nonce breaks the security model of Content Security Policy.

Content Security Policy Level 3

Editor’s Draft, 11 January 2021

§ 7. Security and Privacy Considerations

§ 7.1. Nonce Reuse

Nonces override the other restrictions present in the directive in which they’re delivered. It is critical, then, that they remain unguessable, as bypassing a resource’s policy is otherwise trivial.

If a server delivers a nonce-source expression as part of a policy, the server MUST generate a unique value each time it transmits a policy. The generated value SHOULD be at least 128 bits long (before encoding), and SHOULD be generated via a cryptographically secure random number generator in order to ensure that the value is difficult for an attacker to predict.

https://w3c.github.io/webappsec-csp/#security-nonces

[RobbieTheWagner]

@jelhan this is just to make running tests work. It shouldn't even need any of this.

[jelhan]

Is this for tests of Ember Electron only? In that case I would recommend to include hashes of the injected scripts in CSP configuration of dummy app (tests/dummy/config/content-security-policy.js).

[RobbieTheWagner]

@jelhan no, this is how we can allow users to run tests. Without this ember electron:test in any app using ember-electron will always hang and fail.

[jelhan]

Do I get it right that you are planning to ask your users to include the nonce in the script-src directive of their CSP? If so I see the big risk that they include it not only in development and testing builds. If the nonce is included in production builds it would be a security issue.

Couldn't you ask your users to include a hash of the file content in script-src directive of their CSP instead? Are you expecting the content of these files to change that regularly?

[bendemboski]

@rwwagner90 wait, is there also a problem running the application (not just tests)? Because two of the three scripts you nonced are always used, and only one is test-only.

I have an idea for how we could do the testem hack without a script tag, but I'm less sure of the other two.

[bendemboski]

Couldn't you ask your users to include a hash of the file content in script-src directive of their CSP instead? Are you expecting the content of these files to change that regularly?

We don't change them very often, but they have changed occasionally, and I'm kinda reluctant to put ourselves in a situation where any such change would be breaking and require a major version bump.

[RobbieTheWagner]

@rwwagner90 wait, is there also a problem running the application (not just tests)? Because two of the three scripts you nonced are always used, and only one is test-only.

I have an idea for how we could do the testem hack without a script tag, but I'm less sure of the other two.

@bendemboski I believe the application still works, but I am not sure why. Perhaps it only works because I am running in dev mode, and if I built for production it would stop working.

I think we could write some scripts to manually put this code into users' index.html rather than relying on contentFor, right?

[jelhan]

I think we could write some scripts to manually put this code into users' index.html rather than relying on contentFor, right?

It does not matter how the code gets into index.html. As long as it's inline JavaScript it violates a CSP unless script-src directive contains either "unsafe-inline", a hash of the script content or a nonce, which is attached to the script.

Did you consider loading the script from an external file instead of injecting it?

[bendemboski]

Did you consider loading the script from an external file instead of injecting it?

Yes, I was just starting to think about that, although got bogged down in wondering why we don't need to have some directive in the CSP allowing file: URLs...I guess maybe because index.html is loaded via file: URL and they are all relative? I haven't CSP'ed in a while, so it's workings isn't readily available in my head!

[jelhan]

got bogged down in wondering why we don't need to have some directive in the CSP allowing file: URLs...I guess maybe because index.html is loaded via file: URL and they are all relative?

I guess the CSP's script-src directive contains "self". The "self" keyword points to the origin of the document. E.g. if a website is served from https://examples.com/bar a CSP directive script-src: "self" allows all scripts served from https://examples.com. I guess it's similar for the file: protocol. But I'm not used to serving applications via file protocol. Especially I'm not sure about the origin concept for file:.

[RobbieTheWagner]

@bendemboski @jelhan so sounds like we should move this to an external file? We would still have to inline that external file right? Would love to figure out a solution here.

[bendemboski]

So I think we would move the shim-*.js scripts from lib/resources to public so they end up at /assets/ember-electron/shim-*.js (and maybe implement treeForPublic() to filter out shim-test-head.js when building for a non-test environment), and then change the contentFor hook to insert:

• <script src="assets/shim-head.js"></script> for head

• <script src="../assets/shim-head.js"></script> for test-head

• <script src="assets/shim-footer.js"></script> for body-footer

• <script src="../assets/shim-footer.js"></script> for test-body-footer

• <script src="/assets/shim-head.js"></script> for head

• <script src="/assets/shim-test-head.js"></script> for test-head

• <script src="/assets/shim-footer.js"></script> for body-footer

Then none of the scripts will be inline, and should behave just like <app-name>.js and vendor.js, and not require any special policy directives.

[RobbieTheWagner]

@bendemboski seems reasonable. Did you have a concrete implementation in mind or do you want me to take a stab at it?

[bendemboski]

I think you'd just need to move the files to public/ and then re-implement injectScript() to be

function injectScript(scriptName) {
  return `<script src="/ember-electron/${scriptName}"></script>`;
}

We probably don't need to worry about filtering out shim-test-head.js in non-test environments -- it's such a tiny script that it just sitting around in the app package (but never being loaded) shouldn't cause any issues.

[RobbieTheWagner]

@bendemboski moving the scripts to public seems to have worked. @jelhan any issues with keeping the testem server stuff in the CSP? This is what I have now:

    contentSecurityPolicy: {
      'default-src': ["'none'"],
      'script-src': [
        'http://localhost:7020',
        'http://localhost:7357',
        'http://testemserver',
        "'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,

[bendemboski]

Nice, looks good to me! Maybe we should add a section to the documentation with instructions for enabling CSP (mainly with that working config you pasted above)?

[jelhan]

@jelhan any issues with keeping the testem server stuff in the CSP?

I would recommend to add it only for test environment. V2 allows environment specific configuration. But beside that it should be fine.