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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1290560_update_csp_parser_tests.patch

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

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 4

[Daniel Veditz [:dveditz]]

Comment on attachment 8781654 [details] [diff] [review]
bug_1290560_update_csp_parser.patch

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

r=dveditz

Comment 5

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a62fbb0065fc
https://hg.mozilla.org/mozilla-central/rev/82bbabfdb3c2