Closed Bug 1140638 Opened 7 years ago Closed 7 years ago
Disable <meta referrer> on 37 due to outstanding middle click bug and risk to uplifting its fix
The fix for bug 1113431 (flaw in <meta referrer>) hasn't landed yet, and it will be too risky to uplift to 37 this late in the cycle if it does land soon. We need to disable <meta referrer> handling and ALSO the CSP referrer directive (since it uses the same logic for enforcing referrer policies).
bz: would you prefer we take the same "disable it" patch you wrote for bug 1134606 to disable this again, or should I put it behind a pref (or some other way) to disable the feature?
Whatever is simpler, though if we think we'll need it again for 38 a pref might be nice...
The patch from bug 1134606 applied to beta 37, but required a small (trivial) merge. This is it after the merge.
Chris, we need to disable the referrer directive in beta 37 (as we discussed). Does this patch look appropriate?
Attachment #8574312 - Flags: review?(mozilla)
Comment on attachment 8574312 [details] [diff] [review] disable csp referrer Review of attachment 8574312 [details] [diff] [review]: ----------------------------------------------------------------- Hey Sid, while your patch definitely serves the purpose of disabling the CSP referrer directive, I would also comment out: a) referrerDirectiveValue() in the *.h and *.cpp file of the Parser, as well as b) the only callsite of that method here: http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCSPParser.cpp#931 It's up to you, but in case someone browses the code it's not immediately obvious why our CSP implementation does not accept the referrer directive in our CSP implementation.
Attachment #8574312 - Flags: review?(mozilla) → review+
Tracking 37 as we need to disable this feature. Sid - With the r+ I think we're ready to land for Beta 4 (gtb today). If that's correct, can you please add a Beta approval request to this bug?
Added early return for the csp referrer parsing. Approval Request Comment [Feature/regressing bug #]: See comment 0, this is disabling an incomplete feature before it merges to release (removal from beta 37 only). [User impact if declined]: users will have an incomplete (and possibly problematic) implementation of a feature that should be held back. [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b342bd33a7b [Risks and why]: riskier to let the feature merge to release than to disable it like this. [String/UUID change made/needed]: None.
Comment on attachment 8574311 [details] [diff] [review] disable_meta Approval Request Comment [Feature/regressing bug #]: see bug 1113431 and comment 0 (incomplete feature, fix didn't land quick enough) [User impact if declined]: improper and problematic feature implementation will ship [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b342bd33a7b [Risks and why]: none significant (this was held back from 36 already) [String/UUID change made/needed]: none Note: this patch is a carry over from bug 1134606 and only has minor bitrot, so carrying over r+.
Lawrence: please flag checkin-needed or find ckerschb if you want to check this in today; I'm not likely available to do the checkin for the rest of the day.
Comment on attachment 8574311 [details] [diff] [review] disable_meta As previously discussed with the sec team, we are disabling this feature in 37. Beta+
Attachment #8574311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8574760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.