1013559 - Support both CSP parsers coexisting

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Garrett Robinson [:grobinson]]

Part of our landing plan for CSP is the we should be able to parse 1.x headers with the new parser while still supporting parsing X-CSP with the old parser. The current code prevents this behavior because it aborts on receiving X-CSP if the newBackend pref is flipped (currently on nsDocument.cpp:2808, may rot). We should change this so we use the old backend for X-CSP and the new backend for CSP (if the newBackend pref is true).

Comment 1

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1013559.patch

Uses the new (C++) backend (if available) for CSP 1.* policies while falling back to the old (JS) implementation for XCSP headers.

Comment 2

[Garrett Robinson [:grobinson]]

Comment on attachment 8425820 [details] [diff] [review]
bug_1013559.patch

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

lgtm! Consider removing the unnecessary PR logging, but r=me either way.

::: content/base/src/nsDocument.cpp
@@ +2816,5 @@
>  #ifdef PR_LOGGING
>        PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> +             "This document uses the deprecated x-content-security-policy header",
> +             "which will not be supported in the future."
> +             "Please update the document to use CSP 1.* compliant headers."));

I don't think we need to PRLOG about deprecated headers. The audience for PRLOG is Firefox developers, while the people who are typically concerned with setting headers are web developers. We already log to the Web Console for their benefit if they use deprecated CSP headers, on nsDocument.cpp:2730.

Comment 3

[Christoph Kerschbaumer [:ckerschb}]

(In reply to Garrett Robinson [:grobinson] from comment #2)
> Comment on attachment 8425820 [details] [diff] [review]
> bug_1013559.patch
> 
> Review of attachment 8425820 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm! Consider removing the unnecessary PR logging, but r=me either way.
> 
> ::: content/base/src/nsDocument.cpp
> @@ +2816,5 @@
> >  #ifdef PR_LOGGING
> >        PR_LOG(gCspPRLog, PR_LOG_DEBUG, ("%s %s %s",
> > +             "This document uses the deprecated x-content-security-policy header",
> > +             "which will not be supported in the future."
> > +             "Please update the document to use CSP 1.* compliant headers."));
> 
> I don't think we need to PRLOG about deprecated headers. The audience for
> PRLOG is Firefox developers, while the people who are typically concerned
> with setting headers are web developers. We already log to the Web Console
> for their benefit if they use deprecated CSP headers, on nsDocument.cpp:2730.

Haha, I really thought is console logging. Will remove that right away. Thanks!

Comment 4

[Sid Stamm [:geekboy or :sstamm]]

Comment on attachment 8425820 [details] [diff] [review]
bug_1013559.patch

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

I strongly encourage running this through try.

Comment 5

[Christoph Kerschbaumer [:ckerschb}]

Attachment: bug_1013559_v2.patch

Carrying over the r+ from grobinson and sstamm.
Even though this is a one line change, we are going to run this everywhere on try to make sure it's working as expected:
https://tbpl.mozilla.org/?tree=Try&rev=e16633a51898

Comment 6

[Christoph Kerschbaumer [:ckerschb}]

Comment on attachment 8425820 [details] [diff] [review]
bug_1013559.patch

Marking the first patch as obsolete.

Comment 7

[Christoph Kerschbaumer [:ckerschb}]

Try looks good to me, this is ready to land!

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97826428737a

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/97826428737a