Bug 86323 – Content Security Policy console error messages should include the violated directive.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

Firefox's error console gives a bit more detail regarding a blocked resource than WebKit currently does. Crucially, it includes the text of the directive that's violated, which gives the developer some chance of understanding what's going on. For example, a blocked image might generate the following error:

    CSP: Directive "img-src http://example.com" violated by http://notexample.com/image.jpg.

That's awkwardly worded, but more helpful than WebKit's current:

    Refused to load image from 'https://www.google.de/logos/2012/mothersday12-res.png' because of Content-Security-Policy.

I like WebKit's error structure, but would recommend adding the violated directive explicitly. I'll throw a strawman patch up in a few that would read:

    Refused to load the image 'http://notexample.com/image.jpg', as it violates the following Content Security Policy directive: "img-src http://example.com/"

That might be too long, but it makes the relevant detail more understandable to developers. I'm open to ideas.

If this is acceptable, I'll update the CSP tests accordingly.

(As a drive by, I'd also like to correct "Refused to load connect from SOURCE" to something more coherent. :) )

[Mike West]

Created attachment 141612 [details]
Patch

[Darin Adler]

Comment on attachment 141612 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=141612&action=review

> Source/WebCore/page/ContentSecurityPolicy.cpp:661
> +    const String verb = type == "connect" ? ("connect to") : ("load the");

Should just be String rather than const String. Also no need for those parentheses; it’s not a conventional style for something like this. Maybe there’s another way to do this without constructing this extra string that we have to destroy, but I can’t think of anything offhand.

> Source/WebCore/page/ContentSecurityPolicy.cpp:662
> +    reportViolation(directive->text(), "Refused to " + verb + " " + type + " '" + url.string() + "' as it violates the following Content Security Policy directive: \"" + directive->text() + "\".\n", url);

I’m not too happy with the phrase “refused to x as it violates y”; the word “as” there does not seem right to me. I think the word “because” is far clearer. Or maybe “since”.

[WebKit Review Bot]

Comment on attachment 141612 [details]
Patch

Attachment 141612 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12682305

New failing tests:
http/tests/security/contentSecurityPolicy/javascript-url-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-xmlhttprequest-blocked.html
http/tests/security/sandboxed-iframe-origin-add.html
http/tests/security/contentSecurityPolicy/inline-style-attribute-blocked.html
http/tests/security/contentSecurityPolicy/image-blocked.html
http/tests/security/contentSecurityPolicy/directive-parsing-03.html
http/tests/security/contentSecurityPolicy/inline-script-blocked.html
http/tests/security/contentSecurityPolicy/inline-script-blocked-goofy.html
http/tests/security/contentSecurityPolicy/eval-scripts-setInterval-blocked.html
fast/canvas/webgl/shader-precision-format.html
http/tests/security/contentSecurityPolicy/inline-script-blocked-javascript-url.html
http/tests/security/contentSecurityPolicy/directive-parsing-01.html
http/tests/security/contentSecurityPolicy/block-mixed-content-hides-warning.html
http/tests/security/contentSecurityPolicy/inline-style-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-websocket-blocked.html
http/tests/security/contentSecurityPolicy/frame-src-blocked.html
http/tests/security/contentSecurityPolicy/combine-multiple-policies.html
http/tests/security/contentSecurityPolicy/directive-parsing-02.html
http/tests/security/contentSecurityPolicy/object-src-no-url-blocked.html
http/tests/security/contentSecurityPolicy/default-src-inline-blocked.html
http/tests/security/contentSecurityPolicy/media-src-blocked.html
http/tests/security/contentSecurityPolicy/connect-src-eventsource-blocked.html
http/tests/security/contentSecurityPolicy/eval-scripts-setTimeout-blocked.html
http/tests/inspector/resource-har-pages.html

[WebKit Review Bot]

Created attachment 141635 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

@Mike: You'll probably need to update the expected results for many of the CSP tests because they contain the console message in the output.  You can do that using the --reset-results flag for run-webkit-tests.  (Note: You can also tell run-webkit-tests to just run the tests in the LayoutTests/http/tests/security/contentSecurityPolicy directory.)

[Mike West]

Created attachment 141643 [details]
Patch

[Mike West]

Thanks Darin and Adam. The new patch changes "as" to "because", drops `const` and parens, and resets the tests (`--result-results` is a life-saver... I've been doing those edits by hand!).

[WebKit Review Bot]

Comment on attachment 141643 [details]
Patch

Attachment 141643 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12685237

New failing tests:
http/tests/security/contentSecurityPolicy/source-list-parsing.html
media/csp-blocks-video.html

[WebKit Review Bot]

Created attachment 141649 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mike West]

Created attachment 141655 [details]
Patch

[Mike West]

Missed a test: the latest patch updates `media/csp-blocks-video.html`.

`http/tests/security/contentSecurityPolicy/source-list-parsing.html` looks like it's loading the iframes in a different order than expected. The results are all there, and correct so far as I can tell, but mixed up. Adam, any ideas about how I could address that?

[WebKit Review Bot]

Comment on attachment 141655 [details]
Patch

Attachment 141655 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12679432

New failing tests:
http/tests/security/contentSecurityPolicy/source-list-parsing.html
media/csp-blocks-video.html

[WebKit Review Bot]

Created attachment 141661 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Mike West]

Created attachment 141668 [details]
Patch

[WebKit Review Bot]

Comment on attachment 141668 [details]
Patch

Attachment 141668 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12682393

New failing tests:
http/tests/security/contentSecurityPolicy/source-list-parsing.html

[WebKit Review Bot]

Created attachment 141696 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Adam Barth]

> `http/tests/security/contentSecurityPolicy/source-list-parsing.html` looks like it's loading the iframes in a different order than expected. The results are all there, and correct so far as I can tell, but mixed up. Adam, any ideas about how I could address that?

Is the test non-deterministic?  If so, we might need to improve the test to eliminate any race conditions.  It looks like the test is slightly flaky:

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showExpectations=true&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Fsource-list-parsing.html

However, your patch seems to cause it to fail more consistently.

[Mike West]

(In reply to comment #17)
> Is the test non-deterministic?  If so, we might need to improve the test to eliminate any race conditions.  It looks like the test is slightly flaky:

It's loading 18 iframes, and they don't all seem to load in the same order every time it's run. I'm not entirely sure how we can ensure that they load in the correct order... Chaining `document.write` calls via `onload` events seems like a bad solution.

Splitting this up into 18 tests would certainly solve the problem, but seems like overkill.

Are there other tests of this sort that I could take a look at for ideas?

[Adam Barth]

> It's loading 18 iframes, and they don't all seem to load in the same order every time it's run. I'm not entirely sure how we can ensure that they load in the correct order... Chaining `document.write` calls via `onload` events seems like a bad solution.

I bet these all used to produce the same output but now they produce different output.  Previously, it didn't matter what order they ran in, but now it does.

> Splitting this up into 18 tests would certainly solve the problem, but seems like overkill.

Yeah, we either need to serialize the order using onload and appendChild or shard the test.  We probably should do a little of both.  Maybe four per test?

[Mike West]

Created attachment 141783 [details]
Patch

[Mike West]

(In reply to comment #19)
> Yeah, we either need to serialize the order using onload and appendChild or shard the test.  We probably should do a little of both.  Maybe four per test?

I've taken a stab at this, splitting it into 4 tests: two with 5, two with 4. This seems to work locally, let's see if the bots are happy.

[WebKit Review Bot]

Comment on attachment 141783 [details]
Patch

Clearing flags on attachment: 141783

Committed r117006: <http://trac.webkit.org/changeset/117006>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kent Tamura]

(In reply to comment #22)
> (From update of attachment 141783 [details])
> Clearing flags on attachment: 141783
> 
> Committed r117006: <http://trac.webkit.org/changeset/117006>

After this, script-src-redirect.html has been really flaky.
I wonder if we should roll it out.

[Kent Tamura]

> After this, script-src-redirect.html has been really flaky.
> I wonder if we should roll it out.

http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=script-src-redirect.html

[Mike West]

Looks like `script-src-redirect.html` is another iframe ordering test. I'll rewrite it to use the same methodology as `source-list-parsing.html`: https://bugs.webkit.org/show_bug.cgi?id=86433

[Chris Dumez]

It looks like the the global expectation was not updated for the following test:
  http/tests/security/contentSecurityPolicy/media-src-blocked.html

You only updated the Chromium expectation.

[Chris Dumez]

Similarly, the expectation for the following test needs to be updated:
http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html

[Chris Dumez]

Created attachment 141858 [details]
Additional patch to update expectations

Update the expectations for:
  http/tests/security/contentSecurityPolicy/media-src-blocked.html
  http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked.html

[Chris Dumez]

Reopening the bug until all expectations are updated.

[WebKit Review Bot]

Comment on attachment 141858 [details]
Additional patch to update expectations

Rejecting attachment 141858 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ecurityPolicy/media-src-blocked-expected.txt
patching file LayoutTests/http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked-expected.txt
Hunk #1 FAILED at 1.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/http/tests/security/contentSecurityPolicy/shared-worker-connect-src-blocked-expected.txt.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12684697

[Chris Dumez]

Created attachment 141863 [details]
Patch

Could someone please cq+?

[WebKit Review Bot]

Comment on attachment 141863 [details]
Patch

Clearing flags on attachment: 141863

Committed r117035: <http://trac.webkit.org/changeset/117035>

[Mike West]

Expectations were updated in May. Closing. :)