Closed Bug 1140638 Opened 9 years ago Closed 9 years ago

Disable <meta referrer> on 37 due to outstanding middle click bug and risk to uplifting its fix


(Core :: DOM: Core & HTML, defect)

37 Branch
Not set



Tracking Status
firefox36 --- unaffected
firefox37 + fixed
firefox38 --- unaffected
firefox39 --- unaffected


(Reporter: geekboy, Assigned: geekboy)




(2 files, 1 obsolete file)

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?
Flags: needinfo?(bzbarsky)
Whatever is simpler, though if we think we'll need it again for 38 a pref might be nice...
Flags: needinfo?(bzbarsky)
Attached patch disable_metaSplinter Review
The patch from bug 1134606 applied to beta 37, but required a small (trivial) merge.  This is it after the merge.
Attached patch disable csp referrer (obsolete) — Splinter Review
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:

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]:
[Risks and why]: riskier to let the feature merge to release than to disable it like this.
[String/UUID change made/needed]: None.
Attachment #8574312 - Attachment is obsolete: true
Flags: needinfo?(sstamm)
Attachment #8574760 - Flags: review+
Attachment #8574760 - Flags: approval-mozilla-beta?
Comment on attachment 8574311 [details] [diff] [review]

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]:
[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+.
Attachment #8574311 - Flags: review+
Attachment #8574311 - Flags: approval-mozilla-beta?
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.
Flags: needinfo?(lmandel)
Comment on attachment 8574311 [details] [diff] [review]

As previously discussed with the sec team, we are disabling this feature in 37. Beta+
Flags: needinfo?(lmandel)
Attachment #8574311 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8574760 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.