Last Comment Bug 842657 - Flip the pref to enable the CSP 1.0 parser for Firefox
: Flip the pref to enable the CSP 1.0 parser for Firefox
Status: VERIFIED FIXED
:
Product: Firefox
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 24
Assigned To: Ian Melven :imelven
:
Mentors:
Depends on: 746978 763879 783049
Blocks: CSP csp-w3c-1.0 821877 873187
  Show dependency treegraph
 
Reported: 2013-02-19 10:47 PST by Ian Melven :imelven
Modified: 2014-06-04 20:37 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified


Attachments
patch v1 (5.69 KB, patch)
2013-02-20 14:48 PST, Ian Melven :imelven
jst: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Ian Melven :imelven 2013-02-19 10:47:08 PST
We have a CSP implementation that complies with the CSP 1.0 spec.

It's behind a pref currently that is only turned on during mochitests.

When the last piece of the CSP 1.0 work (bug 763879) lands (there is an r+ patch currently), we want to flip this pref in Nightly and start honoring unprefixed Content-Security-Policy headers (as described in bug 783049)
Comment 1 Ian Melven :imelven 2013-02-20 14:48:38 PST
Created attachment 716254 [details] [diff] [review]
patch v1
Comment 2 Ian Melven :imelven 2013-02-20 16:03:17 PST
https://tbpl.mozilla.org/?tree=Try&rev=c81e900306d1
Comment 3 Ian Melven :imelven 2013-02-21 11:51:38 PST
Comment on attachment 716254 [details] [diff] [review]
patch v1

Our plan is to flip this pref on Nightly when we land 763879. This will turn on using the new, CSP 1.0 spec compliant parser for CSP's delivered via the unprefixed Content-Security-Policy header. Bug 783049 contains further details of how we handle getting both the prefixed X- header and the unprefixed header, but tl;dr we ignore the prefixed header in that case and log a warning.
Comment 4 Ian Melven :imelven 2013-02-21 14:12:52 PST
Last try push ran into a problem with 763879, which I have now hopefully fixed.

https://tbpl.mozilla.org/?tree=Try&rev=67641f5d771f
Comment 5 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-02-21 14:55:24 PST
I don't have nearly enough context here to usefully review this.

Is this really a Firefox-specific pref, or should it live in all.js instead?

Who else is familiar with this work and can sign off on it being enabled by default?
Comment 6 Ian Melven :imelven 2013-02-21 15:01:09 PST
(In reply to :Gavin Sharp (use gavin@gavinsharp.com for email) from comment #5)
> I don't have nearly enough context here to usefully review this.

Fair enough.
 
> Is this really a Firefox-specific pref, or should it live in all.js instead?

For now, I think we just want it turned on in Firefox Desktop. The situation in B2G is a little more complex and I want to talk to a couple of folks over there to see what they'd like to do (it also would require some more code changes to get apps to use the new parser) and when it makes sense in their schedule to do it.

> Who else is familiar with this work and can sign off on it being enabled by
> default?

Sid Stamm is the closest to the work IMO - would it be ok for him to r? it ?
Comment 7 Ian Melven :imelven 2013-02-21 15:05:28 PST
Comment on attachment 716254 [details] [diff] [review]
patch v1

Sid and I ambushed jst and asked him if he could maybe take a look at this ;)
Comment 8 Johnny Stenback (:jst, jst@mozilla.com) 2013-03-11 19:42:17 PDT
Comment on attachment 716254 [details] [diff] [review]
patch v1

Sorry for the delay here, r=jst
Comment 9 Ian Melven :imelven 2013-03-11 19:53:37 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #8)
> Comment on attachment 716254 [details] [diff] [review]
> patch v1
> 
> Sorry for the delay here, r=jst

No problem, I've been slowly figuring out what the right thing is to do for bug 763879 is in the meantime - thanks for the r+ :)
Comment 10 Ian Melven :imelven 2013-05-16 08:16:56 PDT
I thought this was ready to go but the r+ for 763879 changed to an r-, see
https://bugzilla.mozilla.org/show_bug.cgi?id=763879#c123

I landed this in inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/254c1ac4ab8b but we are going to back it out.
Comment 11 Ian Melven :imelven 2013-05-16 08:17:37 PDT
Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8be85024c315
Comment 12 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-16 17:42:52 PDT
(In reply to Ian Melven :imelven from comment #10)
> I landed this in inbound as
> https://hg.mozilla.org/integration/mozilla-inbound/rev/254c1ac4ab8b but we
> are going to back it out.

hg export -r 254c1ac4ab8b > ian-flip.patch
hg import ian-flip.patch
hg push -r . m-i

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b181afc9fad
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-05-17 09:54:11 PDT
https://hg.mozilla.org/mozilla-central/rev/6b181afc9fad
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-22 10:59:43 PDT
Do we want to consider an uplift here?  What is the risk involved?
Comment 15 Ian Melven :imelven 2013-05-22 11:26:06 PDT
Uplifting this would depend on also uplifting bug 763879 which adds inline style blocking to our CSP implementation.

What risk is involved is a good question. We have tests that the old, prefixed header supported in Fx 23 and earlier will still be handled as it always was. We also have tests to check that when both headers are sent, the new, unprefixed header is preferred. In general, I'd say the biggest risk is probably that of confusing people who don't expect Firefox to handle the unprefixed header. Freddy Braun and I are both working on blog posts to let people know about CSP 1.0 landing and what it means, and I'm going to be reaching out to the documentation folks as well to get MDN updated with the current state of things. Given that we just moved 23 to Aurora, I think we still would have enough time to get the word out before this hits Beta/Release (and even then, I think the risk of unexpected breakage is pretty low, since Chrome has been honoring the unprefixed header since Chrome 25, which was released in February.)

I've asked Sid for his thoughts as well :)
Comment 16 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-22 11:45:46 PDT
from comment 15 and conversation in IRC the best option here is to wait until 24 and ride the trains, get more time to do outreach & awareness building, untracking for 23 given that.
Comment 17 Daniel Veditz [:dveditz] 2013-05-22 13:59:31 PDT
I'd like to appeal the wontfix for Fx23 and ask that we reconsider uplifting.

There are so few sites on the internet using this feature (waiting for our implementation to match Chrome's) that we won't get any real usage testing without a beta-sized audience. Waiting another cycle for more Aurora testing won't help much.

Lengthy outreach is not urgent because we will continue to support the old headers so we don't need to convince people to change. In convincing people to switch we also have Google evangelizing the 1.0-compliant headers, and they've been doing that for a while now.

The risk is relatively low, because worst comes to worst we just flip the pref. No need for a scary detangling and backout. We could even flip the pref with the hotfix addon if necessary, but since there are so few sites using this feature it's hard to imagine needing it.
Comment 18 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-23 16:54:03 PDT
Dan makes persuasive arguments, should we discover fallout we can hotfix this pref back to disabled - can we get this nominated & uplifted sooner rather than later to get the most bake time in Aurora/Beta?
Comment 19 Sid Stamm [:geekboy or :sstamm] 2013-05-23 17:07:44 PDT
Dan, are you willing to spend the time rebasing the patch from bug 763879 and uplifting it?
Comment 20 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-23 20:10:34 PDT
(In reply to Sid Stamm [:geekboy or :sstamm] from comment #19)
> Dan, are you willing to spend the time rebasing the patch from bug 763879
> and uplifting it?

Assuming there is not anything non-obviousl, then the merge took less than a minute:

hg graft 9eb574cd8faf
<fix a merge conflict in a single file>
hg graft 6b181afc9fad

https://tbpl.mozilla.org/?tree=Try&rev=23055644a92c
Comment 21 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-23 21:25:00 PDT
(In reply to Brian Smith (:bsmith) from comment #20)
> (In reply to Sid Stamm [:geekboy or :sstamm] from comment #19)
> > Dan, are you willing to spend the time rebasing the patch from bug 763879
> > and uplifting it?
> 
> Assuming there is not anything non-obviousl, then the merge took less than a
> minute:
> 
> hg graft 9eb574cd8faf
> <fix a merge conflict in a single file>
> hg graft 6b181afc9fad
> 
> https://tbpl.mozilla.org/?tree=Try&rev=23055644a92c

Seems I guessed wrong on that merge. Let's try again:
https://tbpl.mozilla.org/?tree=Try&rev=eeddd214c0f7
Comment 22 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2013-05-24 10:54:16 PDT
Comment on attachment 716254 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature
User impact if declined: CSP 1.0 policies with the standard syntax/semantics are will not be supported.
Testing completed (on m-c, etc.): This landed on m-c a week ago, seemingly without problems.
Risk to taking this patch (and alternatives if risky): Other people should comment on the risk as far as coding change risk is concerned. There is some compatibility risk for a small number of websites that are using CSP 1.0. However, it will be easy to revert this change if we run into problems.
String or IDL/UUID changes made by this patch: None
Comment 23 Ian Melven :imelven 2013-05-24 18:15:27 PDT
(In reply to Brian Smith (:bsmith) from comment #22)
> Comment on attachment 716254 [details] [diff] [review]
> patch v1
> 
> Risk to taking this patch (and alternatives if risky): Other people should
> comment on the risk as far as coding change risk is concerned. There is some
> compatibility risk for a small number of websites that are using CSP 1.0.
> However, it will be easy to revert this change if we run into problems.

I think Dan makes some very good points in comment 17.

The only real risk should be for sites using CSP 1.0 headers, as Brian said above. We even have data on how few sites are using these headers so far : 6 of the top Alexa 1 million - source : http://www.veracode.com/blog/2013/03/security-headers-on-the-top-1000000-websites-march-2013-report/

Also, I'll again mention there's a fair amount of tests around these changes as well, see my comment 15
Comment 24 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-27 19:11:40 PDT
Comment on attachment 716254 [details] [diff] [review]
patch v1

I'm OK with that risk since the payoff as Dan describes it sounds worthwhile. Approving for Aurora.
Comment 25 Ryan VanderMeulen [:RyanVM] 2013-05-28 12:43:09 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/61f8d384403d
Comment 26 Ian Melven :imelven 2013-06-10 15:42:40 PDT
CSP 1.0 support is a good candidate for a release note IMO. I'm going to push out a blog post about it very soon as well.
Comment 27 Paul Silaghi, QA [:pauly] 2013-07-30 06:37:19 PDT
Verified security.csp.speccompliant=true by default in FF 23b10.
Comment 28 Paul Silaghi, QA [:pauly] 2013-08-20 04:46:40 PDT
(In reply to Paul Silaghi [QA] from comment #27)
> Verified security.csp.speccompliant=true by default in FF 23b10.
Verified FF 24b4

Note You need to log in before you can comment on or make changes to this bug.