whitelist script tag inserted for test file loaded assertion using nonce #88

jelhan · 2019-02-15T23:53:10Z

This adds a nonce to <script> tag inserted by ember-cli for assertion that test file was loaded. Otherwise this violates default CSP in tests.

Nonce is always present in CSP header but only part of CSP meta tag injected into tests/index.html.

A nonce used in productive applications must be cryptographically random. But since this one is only used in local development and if executing tests, using a static string is fine.

This adds a nonce to <script> tag inserted by ember-cli for assertion that test file was loaded. Otherwise this violates default CSP in tests. Nonce is always present in CSP header but only part of CSP meta tag injected into `tests/index.html`. A nonce used in productive applications must be cryptographically random. But since this one is only used in local development and if executing tests, using a static string is fine.

sandstrom

I haven't used this addon in tests much, so I cannot really vouch for whether this will work or not.

But what you describe makes sense in theory.

Two small comments below!

index.js

sandstrom · 2019-02-18T09:36:52Z

index.js

@@ -109,6 +111,10 @@ module.exports = {
        return;
      }

+      if (policyObject) {


For clarity, how about checking for options.environment === 'test' here too

Actually this needs to be applied also for options.environment === 'development' cause tests may be executed by http://localhost:4200/tests. In that case option.environment is true. So would end up checking the request target also...

@jelhan Alright, I haven't used http://localhost:4200/tests and I believe you, but it sounds weird that they aren't running under the test environment. Is that by design?

Anyway, perhaps we could test for both then?

options.environment === 'test' && options.environment === 'development'

It's a complex topic for http://localhost:4200/tests and server middleware. The tests are running under test environment but the middleware is running under the environment used to start the server. So if we want to make sure that the nonce is only present in headers for testing we would end up with a check like this:

options.environment === 'test' || req.path === '/tests'

To be honest I haven't checked if ember test --server runs middleware with test environment but I expect it to do so.

But I don't see a big issue in having that nonce present always in request headers. They don't influence build result but only used for development server (ember serve).

Have done some quick tests and it seems like serverMiddleware hook is not executed if running ember test --server. CSP headers are not present at all.

@jelhan Got it, makes sense! I know the test server is only running under development and won't affect builds. I'm just thinking that making that clear in the if-statement will make it easier to reason about the code.

But you could also add a comment instead, something like:

// the local server will never run for production builds, so no danger in adding the nonce all the time

(I still would have gone with an if-statement, but I don't have a strong opinion on this)

jelhan · 2019-02-18T21:16:06Z

I haven't used this addon in tests much, so I cannot really vouch for whether this will work or not.

Steps to check this feature:

ember new ember-test-csp && cd ember-test-csp
ember install ember-cli-content-security-policy
ember serve and open http://localhost:4200/tests in browser

You will notice two CSP violations reported:

Content Security Policy violation:
{
  "csp-report": {
    "document-uri": "http://localhost:4200/tests?hidepassed",
    "referrer": "",
    "violated-directive": "script-src-elem",
    "effective-directive": "script-src-elem",
    "original-policy": "default-src 'none'; script-src 'self' localhost:4200 0.0.0.0:4200 undefined:4200; font-src 'self'; connect-src 'self' ws://localhost:4200 ws://0.0.0.0:4200 ws://undefined:4200 http://localhost:4200; img-src 'self'; style-src 'self'; media-src 'self'; report-uri http://localhost:4200/csp-report;",
    "disposition": "report",
    "blocked-uri": "inline",
    "line-number": 38,
    "source-file": "http://localhost:4200/tests?hidepassed",
    "status-code": 200,
    "script-sample": ""
  }
}

Content Security Policy violation:
{
  "csp-report": {
    "document-uri": "http://localhost:4200/tests?hidepassed",
    "referrer": "",
    "violated-directive": "style-src-attr",
    "effective-directive": "style-src-attr",
    "original-policy": "default-src 'none'; script-src 'self' localhost:4200 0.0.0.0:4200 undefined:4200; font-src 'self'; connect-src 'self' ws://localhost:4200 ws://0.0.0.0:4200 ws://undefined:4200 http://localhost:4200; img-src 'self'; style-src 'self'; media-src 'self'; report-uri http://localhost:4200/csp-report;",
    "disposition": "report",
    "blocked-uri": "inline",
    "line-number": 6012,
    "column-number": 23,
    "source-file": "http://localhost:4200/assets/test-support.js",
    "status-code": 200,
    "script-sample": ""
  }
}

The first one should be fixed by this PR, the second one is addressed by qunitjs/qunit#1369.

sandstrom · 2019-02-19T14:42:32Z

@jelhan Seems like some of the tests failed, would you like to have a look at it? https://travis-ci.com/rwjblue/ember-cli-content-security-policy/jobs/178664703

You can try to just force-push to re-trigger another test-run, sometimes it's because of timeouts.

@rwjblue I don't have any additional comments on this PR (in addition to those above). In my opinion it's mostly good to merge. But happy to hear your thoughts before I do anything.

jelhan · 2019-02-19T16:43:41Z

@jelhan Seems like some of the tests failed, would you like to have a look at it? https://travis-ci.com/rwjblue/ember-cli-content-security-policy/jobs/178664703

I don't think that issue for ember-beta and ember-canary is related to my changes. Seeing the same issues in #89. Actually it doesn't seem like there are any tests except for linting.

rwjblue · 2019-02-19T17:02:44Z

index.js

@@ -98,6 +100,8 @@ module.exports = {
    var options = config.options;
    var project = options.project;

+    console.log(options.environment);


Whooops, I missed this before I merged!

Arghs and I missed that I committed that one with the last inline comment. I'm very sorry about that. Do you want me to open a PR to remove it or are you doing it yourself?

index.js is not covered by ESLint 😭

I've removed the console.log call in 97cfed0 (hope that was okay @rwjblue)

@sandstrom Thanks a lot. I'm really sorry for the churn.

@jelhan No worries, thanks for making this code better! 😄

Added by mistake in #88

This has been fixed upstream by rwjblue/ember-cli-content-security-policy#88 and is part of ember-cli-content-security-policy@1.1.1 release.

sandstrom reviewed Feb 18, 2019

View reviewed changes

refactor: rename NONCE to STATIC_TEST_NONCE

c64c06a

add comment to make it clearer why it's safe to add nonce always

f596372

rwjblue merged commit d505477 into rwjblue:master Feb 19, 2019

rwjblue reviewed Feb 19, 2019

View reviewed changes

rwjblue added the enhancement label Feb 19, 2019

sandstrom pushed a commit that referenced this pull request Feb 20, 2019

Remove stray log call

97cfed0

Added by mistake in #88

jelhan mentioned this pull request May 29, 2019

Discuss issues in SSR in README (investigate mitigation) jelhan/ember-style-modifier#11

Closed

jelhan mentioned this pull request Jan 27, 2020

Problem with using 'unsafe-inline' in script-src #127

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

whitelist script tag inserted for test file loaded assertion using nonce #88

whitelist script tag inserted for test file loaded assertion using nonce #88

jelhan commented Feb 15, 2019

sandstrom left a comment

sandstrom Feb 18, 2019

jelhan Feb 18, 2019

sandstrom Feb 19, 2019

jelhan Feb 19, 2019

jelhan Feb 19, 2019

sandstrom Feb 19, 2019

jelhan Feb 19, 2019

jelhan commented Feb 18, 2019

sandstrom commented Feb 19, 2019 •

edited

Loading

jelhan commented Feb 19, 2019

rwjblue Feb 19, 2019

jelhan Feb 19, 2019

jelhan Feb 19, 2019

sandstrom Feb 20, 2019

jelhan Feb 20, 2019

sandstrom Feb 20, 2019

whitelist script tag inserted for test file loaded assertion using nonce #88

whitelist script tag inserted for test file loaded assertion using nonce #88

Conversation

jelhan commented Feb 15, 2019

sandstrom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelhan commented Feb 18, 2019

sandstrom commented Feb 19, 2019 • edited Loading

jelhan commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandstrom commented Feb 19, 2019 •

edited

Loading