1366496 - Switch usages of browser.safebrowsing.enabled to browser.safebrowsing.phishing.enabled after bug 1025965

Status: RESOLVED FIXED

(SeaMonkey :: Security, enhancement)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #1025965 +++

"In bug 1025358, the reporter believed that setting the pref browser.safebrowsing.enabled to false would disable safebrowsing entirely, whereas in fact it only controls the safebrowsing phishing protection, and browser.safebrowsing.malware.enabled needed to be set to false too."

Consequently, browser.safebrowsing.enabled was renamed browser.safebrowsing.phishing.enabled for consistency. We are now exposing the wrong pref in the Privacy & Security prefpane and for sync.

This issue affects 2.48 already but should definitely be fixed for 2.49.x (checkbox "Block reported web forgeries" is /not/ checked despite being enabled, no way to disable through prefpane).

Comment 1

[rsx11m]

Attachment: Straight-forward fix [2.48 only]

No other change necessary to make this work again.

Comment 2

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

Looks good.

I added a _migrateUI to nsSuiteGlue.js in bug 1331477 to avoid running migrations every time on startup. How about bumping the version to 2 and putting the new pref copy in there? f+ for now.

Comment 3

[rsx11m]

The other migrations should go there as well, let's do this in a follow-up bug as suggested over IRC?

Comment 4

[rsx11m]

Attachment: Use _migrateUI() instead

On the other hand, you are right; to avoid confusion with the old migration code to be moved in the follow-up bug, I shouldn't change it as this would be UI version #0. Thus, moving this to _migrateUI() as suggested and bumping the version to keep everything consistent from now on.

[Approval Request Comment]
Regression caused by (bug #): bug 1025965
User impact if declined: security pref not observed if other than default, unusable UI
Testing completed (on m-c, etc.): works on 2.48b1
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 5

[rsx11m]

(In reply to rsx11m from comment #3)
> The other migrations should go there as well, let's do this in a follow-up
> bug as suggested over IRC?

Filed bug 1366593.

Comment 6

[rsx11m]

(In reply to rsx11m from comment #4)
> Testing completed (on m-c, etc.): works on 2.48b1

Not quite, actually - needs bug 1331477 attachment 8838931 [details] [diff] [review] to work properly.
As fallback, we could use attachment 8869736 [details] [diff] [review] for the 2.48 release.

Comment 7

[rsx11m]

Still weird - it gets to setting suite.migration.version = 2 with both patches from bug 1331477 applied in a /new/ profile but not in an old one. Does the suite.migration.version = 1 migration apply to 2.48 already or only to 2.49?

If that's the case, let's indeed go with attachment 8869736 [details] [diff] [review] for 2.48 off comm-release.

Comment 8

[rsx11m]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

(Quoting rsx11m from bug 1331477 comment #21)
> Ok, so bug 1329494 and bug 1330640 landed on 52.0+ only, so this shouldn't
> apply for 51.0 = SM 2.48. Sorry for the noise...

Meaning, we still need *this* version for 2.48 given that _migrateUI() doesn't exist yet.

Comment 9

[rsx11m]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

On a third thought, the issue here started to apply to 2.47 first, thus logically before bug 1331477, and as such can be categorized as a "pre-2.49" migration to be put in _updatePrefs().

Thus, one could argue that attachment 8869736 [details] [diff] [review] should be applied to /all/ branches (which also makes the fix consistent among branches), then bug 1331477 goes on top of that, followed by bug 1366593 (the patch there can be easily un-unbitrotted).

Anyway, so whatever you (frg and IanN) decide, I'm flexible. Please set approval flags as you see fit.

Comment 10

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8869825 [details] [diff] [review]
Use _migrateUI() instead

Thanks. One NIT comment in nsSuiteGlue.js should end with a fullstop. 

I need to do 3 checkins and will add this one when the trees are open. I will fix it with the checkin.

Comment 11

[Frank-Rainer Grahl (:frg)]

https://hg.mozilla.org/comm-central/rev/2adf96bc4bf9690ed1da624951c990d8c69dbab8

Wontfix for 2.50 or not?

Comment 12

[rsx11m]

(In reply to Frank-Rainer Grahl from comment #11)
> https://hg.mozilla.org/comm-central/rev/2adf96bc4bf9690ed1da624951c990d8c69dbab8

Thanks!

> Wontfix for 2.50 or not?

Yes, the c-r request on attachment 8869736 [details] [diff] [review] is for the 2.48 release branch, unless IanN wants to have it on the default branch as well (which I'd think is redundant at this point).

Comment 13

[Frank-Rainer Grahl (:frg)]

Thought so. Just wanted to be sure. 2.50 is good for nothing at this point with the broken TB and our mail locales.

Comment 14

[Ian Neal]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

Assuming we do a 2.48 release a=me

Comment 15

[Ian Neal]

Comment on attachment 8869825 [details] [diff] [review]
Use _migrateUI() instead

[Triage Comment]
a=me for all repos

Comment 16

[rsx11m]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

Pushed on SEA248b1_2017021701_RELBRANCH
https://hg.mozilla.org/releases/comm-release/rev/99c5e35f2ea3

ewong: please transplant as necessary if a new relbranch is opened

Comment 17

[rsx11m]

Comment on attachment 8869825 [details] [diff] [review]
Use _migrateUI() instead

https://hg.mozilla.org/releases/comm-beta/rev/6b3779b32c4a
https://hg.mozilla.org/releases/comm-esr52/rev/67506db1dbfb

Comment 18

[rsx11m]

Comment on attachment 8869736 [details] [diff] [review]
Straight-forward fix [2.48 only]

Relanded on SEA_COMM510_20170330_RELBRANCH by frg,
https://hg.mozilla.org/releases/comm-release/rev/648006aa3339