Ian Melven :imelven Assignee
	Description • 11 years ago

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)

	Ian Melven :imelven Assignee
	Comment 2 • 11 years ago

https://tbpl.mozilla.org/?tree=Try&rev=c81e900306d1

	Ian Melven :imelven Assignee
	Comment 3 • 11 years ago

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.

	Ian Melven :imelven Assignee
	Comment 4 • 11 years ago

Last try push ran into a problem with 763879, which I have now hopefully fixed.

https://tbpl.mozilla.org/?tree=Try&rev=67641f5d771f

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 5 • 11 years ago

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?

	Ian Melven :imelven Assignee
	Comment 6 • 11 years ago

(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 ?

	Ian Melven :imelven Assignee
	Comment 7 • 11 years ago

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 ;)

	Johnny Stenback (:jst)
	Comment 8 • 11 years ago

Comment on attachment 716254 [details] [diff] [review]
patch v1

Sorry for the delay here, r=jst

	Ian Melven :imelven Assignee
	Comment 9 • 11 years ago

(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+ :)

	Ian Melven :imelven Assignee
	Comment 10 • 11 years ago

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.

	Ian Melven :imelven Assignee
	Comment 11 • 11 years ago

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/8be85024c315

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 12 • 11 years ago

(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

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6b181afc9fad

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 14 • 11 years ago

Do we want to consider an uplift here?  What is the risk involved?

	Ian Melven :imelven Assignee
	Comment 15 • 11 years ago

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 :)

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 16 • 11 years ago

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.

	Daniel Veditz [:dveditz]
	Comment 17 • 11 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 18 • 11 years ago

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?

	Sid Stamm [:geekboy or :sstamm]
	Comment 19 • 11 years ago

Dan, are you willing to spend the time rebasing the patch from bug 763879 and uplifting it?

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 20 • 11 years ago

(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

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 21 • 11 years ago

(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

	Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
	Comment 22 • 11 years ago

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

	Ian Melven :imelven Assignee
	Comment 23 • 11 years ago

(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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 24 • 11 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/61f8d384403d

	Ian Melven :imelven Assignee
	Comment 26 • 11 years ago

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.

	Paul Silaghi, QA [:pauly]
	Comment 27 • 11 years ago

Verified security.csp.speccompliant=true by default in FF 23b10.

	Paul Silaghi, QA [:pauly]
	Comment 28 • 11 years ago

(In reply to Paul Silaghi [QA] from comment #27)
> Verified security.csp.speccompliant=true by default in FF 23b10.
Verified FF 24b4