873187 - Back out 821877 until 842657 lands to turn on CSP 1.0

Status: RESOLVED INVALID

(Core :: Security, defect)

Description

[Garrett Robinson [:grobinson]]

Started by re-opening 821877, but bz told me it's better to create a new bug to track backouts. Description copied from comment on 821877:

Bug 821877 landed with the assumption that we would be able to get the CSP 1.0 implementation completed and flip the pref to turn it on by default in time for mozilla23. This did not happen (progress is being tracked in Bug 842657). The result is that this warning message advises developers to use the unprefixed Content-Security-Policy header, which is silently ignored (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#2463) if the pref is not set. This is bad advice and we need to back out the patch until the pref is set so it is not telling people to do the wrong thing.

Comment 1

[Garrett Robinson [:grobinson]]

Attachment: Patch 1

This patch backs out both changesets from Bug 821877. This removes the test that was added in 821877, so the backout should not cause any test failures.

Backed changes out of mozilla-central and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=24036e0f212b

Backed changes out of Aurora and pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=e9d94d3e4f83

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

I do not understand why we would back out the patch.

If the problem is that the error message text is confusing/misleading, then why don't we just fix that text?

In particular, having the messages go to the web console is a BIG improvement and I don't see any reason to back that part out.

Comment 3

[Garrett Robinson [:grobinson]]

(In reply to Brian Smith (:bsmith) from comment #2)
> I do not understand why we would back out the patch.
> 
> If the problem is that the error message text is confusing/misleading, then
> why don't we just fix that text?
> 
> In particular, having the messages go to the web console is a BIG
> improvement and I don't see any reason to back that part out.

The error message in question ("You're using the deprecated X-prefixed header; you should use the 1.0 spec header instead") doesn't make sense unless the CSP 1.0 pref is on. Otherwise, it actually *reduces* site security because a web dev responding to the warning might switch to the new header, which is silently ignored. I can't think of anything useful to change the error message to.

Backing this out does not remove the new security pane from the Web Console; just reporting for this error.

Since the pref is now flipped, I am just going to back this out of Aurora, but leave it in m-c.

If anybody has more questions or ideas, please let me know! This is the first patch I've had to back out and I am trying to figure out the best way to handle this, for now and in the future.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Garrett Robinson [:grobinson] from comment #3)
> Backing this out does not remove the new security pane from the Web Console;
> just reporting for this error.

Gotcha. I misread it. Carry on.

Comment 5

[Garrett Robinson [:grobinson]]

Attachment: Patch 2

Attached a revised backout patch for Aurora and have verified that it at least builds. Try run: https://tbpl.mozilla.org/?tree=Try&rev=89977dbc887f

Comment 6

[Garrett Robinson [:grobinson]]

Comment on attachment 751934 [details] [diff] [review]
Patch 2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 821877
User impact if declined: Misleading message in Web Console
Testing completed (on m-c, etc.): try run green on aurora
Risk to taking this patch (and alternatives if risky): Low - scope limited to CSP
String or IDL/UUID changes made by this patch: None

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 751934 [details] [diff] [review]
Patch 2

Approving the backout from Aurora - and tracking all these bugs so we can keep an eye on the timing here for either 23 (could still end up in there if low risk uplift for bug 842657 is nominated) or 24 as a backup.

Comment 8

[Garrett Robinson [:grobinson]]

I need this backout landed on Aurora. bug 842657 has landed, so I think it should be ok to leave the original patch in mozilla-central. If this is wrong, please correct me!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7c7e6e87fe76

Comment 10

[Garrett Robinson [:grobinson]]

This backout landed briefly in firefox23, but was then backed out by Bug 878790 after Bug 842657 landed. So it has been returned to firefox23, and was never taken out of central.

Comment 11

[bhavana bajaj [:bajaj]]

needinfo on :grobinson to help understand if there is anything that needs to be done for Fx24 here given the current status "status-firefox24: affected ", Or should that be flipped to fixed ?

Comment 12

[Garrett Robinson [:grobinson]]

Since the backout was never applied to central, so the fix for Bug 888172 should have remained in central and would have moved naturally into FF24 (currently Aurora). I double-checked this by pulling aurora-src and building, then running `mach mochitest-plain content/base/test/test_CSP_bug888172.html`. The test passed, so I don't think anything needs to be done for FF24 and am clearing the status-firefox24 flag.