Closed Bug 1247459 Opened 4 years ago Closed 4 years ago

Meta and header CSP are merged without a semicolon

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: francois, Assigned: arroway)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file, 4 obsolete files)

Steps:

1. Create an HTML with an image and a meta CSP of "img-src 'none'"
2. Serve that page using a CSP header of "img-src 'none'"

Expected:

> Content Security Policy: The page's settings blocked the loading of a resource at http://example.com/ ("img-src 'none'").

Actual:

> Content Security Policy: The page's settings blocked the loading of a resource at http://example.com/ ("img-src 'none'").
> Content Security Policy: The page's settings blocked the loading of a resource at http://example.com/ ("img-src 'none'img-src 'none'").
Flags: needinfo?(mozilla)
Chrome 48 behaves as expected.
Thanks francois, we should get that fixed! I'll find someone to assign that bug in our next triage!
Blocks: csp-w3c-3
Flags: needinfo?(mozilla)
Whiteboard: [domsecurity-backlog]
Attached patch bug-1247459-fix.patch (obsolete) — Splinter Review
The bug is due to the string outViolatedDirective not being reset between to consecutive calls to nsCSPPolicy::permits in nsCSPUtils.cpp.
I added a line in permits() to do that. I've been looking for a place to write some tests, but found no obvious solution. Do you have any suggestion?
I'll push this to try as soon as I get my access back, but it didn't break stuff in test/csp/ so far.
Assignee: nobody → stephouillon
Status: NEW → ASSIGNED
Attachment #8741008 - Flags: feedback?(ckerschb)
Comment on attachment 8741008 [details] [diff] [review]
bug-1247459-fix.patch

Review of attachment 8741008 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for looking into this problem. I think it would be best if you add another devtool test, similar to [1]. Instead of adding ^header^ file though you could add two different meta CSP policies.

[1] http://mxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/browser_webconsole_bug_770099_violation.js#15

::: dom/security/nsCSPUtils.cpp
@@ +1072,5 @@
>  
>    NS_ASSERTION(aUri, "permits needs an uri to perform the check!");
>  
>    nsCSPDirective* defaultDir = nullptr;
> +  outViolatedDirective.AssignASCII("");

Please use outViolatedDirective.Truncate() and move it right after NS_ASSERTION().
Attachment #8741008 - Flags: feedback?(ckerschb) → feedback+
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
See Also: → 1264955
Attached patch bug-1247459-v2.patch (obsolete) — Splinter Review
I updated the code as asked and added a test for the webconsole (checking that the same message appears twice).

Try results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7125d91d3527f1c7bd5c2ad89b1e526425967393
Attachment #8741008 - Attachment is obsolete: true
Attachment #8741842 - Flags: review?(ckerschb)
Comment on attachment 8741842 [details] [diff] [review]
bug-1247459-v2.patch

Review of attachment 8741842 [details] [diff] [review]:
-----------------------------------------------------------------

Code and test looks good. Clearing the review only because I would like to understand why the test is disabled for e10s on some platforms. Please answer my question and re-flag me and I have the code reviewed in no time. Thanks for looking into this problem!

::: devtools/client/webconsole/test/browser.ini
@@ +289,5 @@
>  skip-if = e10s # Bug 1042253 - webconsole e10s tests (Linux debug intermittent)
>  [browser_webconsole_bug_1010953_cspro.js]
>  skip-if = e10s && (os == 'win' || os == 'mac') # Bug 1243967
> +[browser_webconsole_bug_1247459_violation.js]
> +skip-if = e10s && (os == 'win' || os == 'mac') # Bug 1264955

It wasn't obvious to me what the problem is for e10s on win and mac? Do you happen to know what's the exact problem on those platforms?

::: devtools/client/webconsole/test/browser_webconsole_bug_1247459_violation.js
@@ +3,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +// Tests that the Web Console CSP messages for two META policies
> +// are correclty displayed.

s/correclty/correctly.
Attachment #8741842 - Flags: review?(ckerschb)
Thanks for the feedback.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> 
> It wasn't obvious to me what the problem is for e10s on win and mac? Do you
> happen to know what's the exact problem on those platforms?
> 

It doesn't seem the reason has been identified, but I'll ask linclark who manages these bugs when she is back from pto this week to know for sure.
I updated the tests:
* replaced the double quotes with their Unicode representation because tests were failing
* removed the skip-if directive for e10s, and it seems to pass

https://treeherder.mozilla.org/#/jobs?repo=try&revision=35ede4a31e02&selectedJob=21279629
Attachment #8741842 - Attachment is obsolete: true
Attachment #8755735 - Flags: review?(ckerschb)
Comment on attachment 8755735 [details] [diff] [review]
bug-1247459-fix-merging-meta-header-violations.patch

Review of attachment 8755735 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, r=me

::: devtools/client/webconsole/test/browser_webconsole_bug_1247459_violation.js
@@ +13,5 @@
> +                       "webconsole/test/test_bug_1247459_violation.html";
> +const CSP_VIOLATION_MSG = "Content Security Policy: The page\u2019s settings " +
> +                          "blocked the loading of a resource at " +
> +                          "http://some.example.com/test.png (\u201cimg-src " +
> +                            "https://example.com\u201d).";

nit: indendation
Attachment #8755735 - Flags: review?(ckerschb) → review+
Backed out for failing its own test on Windows:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b0c9204fec7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c970fb57fedd4da27872c5139e09a6a1ce80e3e0
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=29072613&repo=mozilla-inbound


33 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_bug_1247459_violation.js | Test timed out -
38 INFO TEST-UNEXPECTED-FAIL | devtools/client/webconsole/test/browser_webconsole_bug_1247459_violation.js | failed to match rule: CSP policy URI warning displayed successfully -
Return code: 1
Flags: needinfo?(stephouillon)
I've been trying to fix the issue on Windows, but couldn't find what was wrong.
In the log, the webconsole messages are printed fine.

On Christoph advice, I'm temporarily disabling the test on windows to be able to land the patch before the merge on Monday.

Results of the try server are here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef5129a9672&selectedJob=21943949
Attachment #8757889 - Attachment is obsolete: true
Flags: needinfo?(stephouillon)
Attachment #8759824 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a22e275b759f
Meta and header CSP are merged without a semicolon. r=ckerschb
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a22e275b759f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.