Closed
Bug 1247459
Opened 9 years ago
Closed 8 years ago
Meta and header CSP are merged without a semicolon
Categories
(Core :: DOM: Security, defect)
Core
DOM: Security
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)
(deleted),
patch
|
arroway
:
review+
|
Details | Diff | Splinter Review |
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)
Reporter | ||
Comment 1•9 years ago
|
||
Chrome 48 behaves as expected.
Comment 2•9 years ago
|
||
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)
Updated•9 years ago
|
Whiteboard: [domsecurity-backlog]
Assignee | ||
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
Updated•9 years ago
|
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
Assignee | ||
Updated•9 years ago
|
Assignee | ||
Comment 5•9 years ago
|
||
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 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
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.
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
Attachment #8755735 -
Attachment is obsolete: true
Attachment #8757889 -
Flags: review+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 11•8 years ago
|
||
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 13•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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
Comment 15•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in
before you can comment on or make changes to this bug.
Description
•