Update CSPParser to handle 'sandbox', 'require-sri' and 'report-uri' with no valid srcs correctly

RESOLVED FIXED in Firefox 51

Status

()

P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla51
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments)

(Assignee)

Description

2 years ago
We should update TestCSPParser to include a testcase for adding a CSP Policy of:
* 'sandbox'
* 'report-uri'
* 'require-sri'
without any srcs.

All three of the above mentioned directives need special handling within the CSPParser. In fact we shouldn't handle the srcs of those directives within directiveValue() but rather in directive() itself.
(Assignee)

Updated

2 years ago
Blocks: 1231788
Priority: -- → P3
Whiteboard: [domsecurity-backlog1]
(Assignee)

Updated

2 years ago
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-backlog1] → [domsecurity-active]
(Assignee)

Comment 1

2 years ago
Created attachment 8781654 [details] [diff] [review]
bug_1290560_update_csp_parser.patch

Dan, I slightly refactored those bits in the parser so that directives which do not contain 'regular' srcs follow the same schemata. Those are:
* UPGRADE_IF_INSECURE_DIRECTIVE
* REQUIRE_SRI_FOR
* REPORT_URI_DIRECTIVE
* SANDBOX_DIRECTIVE

Further, I fixed the memory leak within requireSRIForDirectiveValue.

While updating the parser I also made sure we have enough test coverage for the parser for all of the directives mentioned above. I figured that we don't convert to lower case for sandbox and also referrer, but we do for all other directives, hence I also updated that within that patch so that all of the four special directives behalf the same way.
Attachment #8781654 - Flags: review?(dveditz)
(Assignee)

Comment 2

2 years ago
Created attachment 8781655 [details] [diff] [review]
bug_1290560_update_csp_parser_tests.patch
Attachment #8781655 - Flags: review?(dveditz)
(Assignee)

Comment 3

2 years ago
Comment on attachment 8781655 [details] [diff] [review]
bug_1290560_update_csp_parser_tests.patch

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

::: dom/security/test/TestCSPParser.cpp
@@ +531,5 @@
>      { "script-src https://foo.com/%$",
>        "script-src 'none'" },
>      { "require-SRI-for script elephants",
>        "require-sri-for script"},
> +    { "sandbox    foo", // allow-popups

I don't know how '// allow-popups' ended up there but I'll remove it for the final patch.

@@ -527,5 @@
>      { "script-src https://foo.com/%$",
>        "script-src 'none'" },
>      { "require-SRI-for script elephants",
>        "require-sri-for script"},
> -    { "require-sri-for paul",

That test actually moved into TestBadPolicies and I replaced 'paul' with foo.
Comment on attachment 8781654 [details] [diff] [review]
bug_1290560_update_csp_parser.patch

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

r=dveditz
Attachment #8781654 - Flags: review?(dveditz) → review+
Attachment #8781655 - Flags: review?(dveditz) → review+

Comment 5

2 years ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a62fbb0065fc
Update CSPParser to handle 'sandbox', 'require-sri' and 'report-uri' with no valid srcs correctly. r=dveditz
https://hg.mozilla.org/integration/mozilla-inbound/rev/82bbabfdb3c2
Update TestCSPParser to include 'sandbox', 'require-sri' and 'report-uri' with no valid srcs. r=dveditz

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a62fbb0065fc
https://hg.mozilla.org/mozilla-central/rev/82bbabfdb3c2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.