1269773 - Remove Kinto and Safe-Browsing preferences from SeaMonkey's browser-prefs.js after they were moved to global all.js

Status: RESOLVED FIXED

(SeaMonkey :: Security, defect)

Description

[rsx11m]

In bug 1259947 and bug 1266967, several preferences were added related to the new cloud-based Kinto blocklist service to mirror firefox.js; bug 1266235 now intends to make those global in all.js, thus no longer requiring each application to make respective adjustments in their own preferences.

Consequently, we can remove again the related prefs in browser-pref.js, depending on which ones are made global in all.js; this is not critical as we are just replicating those prefs, but it's good to avoid redundancies.

Still waiting for approval of attachment 8747050 [details] there, pending any changes for security.onecrl.maximum_staleness_in_seconds as suggested in bug 1266235 comment #11.

Comment 1

[Philip Chee]

Bug 1033450 - consolidate safebrowsing prefs in all.js
https://hg.mozilla.org/mozilla-central/rev/84bc3fe034f2

We should probably keep the following in browser-prefs.js:
> //Theoretically the "client ID" sent in updates should be appinfo.name but
> //anything except "Firefox" or "navclient-auto-ffox" will cause safebrowsing
> //updates to fail. So we pretend to be Firefox here.
> pref("browser.safebrowsing.id", "navclient-auto-ffox");

Comment 2

[rsx11m]

Looks like spring cleaning in progress. ;-)
Sure, let's add those to the patch.

Comment 3

[rsx11m]

Attachment: Possible patch

This is not yet for review given that bug 1266235 is still work in progress, but you can have a look at the safebrowsing prefs in the first hunk already.

(In reply to Philip Chee from bug 920951 comment #24)
> Comment on attachment 8722583 [details] [diff] [review]
> 920951-safebrowsing_lists-V3.patch
> 
> > +pref("browser.safebrowsing.downloads.remote.block_dangerous",            true);
> > +pref("browser.safebrowsing.downloads.remote.block_dangerous_host",       true);
> > +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
> > +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);

The defaults for "potentially unwanted" and "uncommon" are both true now as was checked in by bug 1033450. I've added an override for these prefs, in case there was a good reason, but if catching those cases is desired as well, easy to remove that.

All other prefs seem to match what's defined in all.js, and I've retained two prefs used locally in suite/common/utilityOverlay.js.

Comment 4

[Philip Chee]

>> +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
>> +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);
Unfortunately we don't have the UI for this so users won't know why their downloads failed.

Comment 5

[Frank-Rainer Grahl (:frg)]

Comment on attachment 8748409 [details] [diff] [review]
Possible patch

As ratty said no ui yet so it can stay false. With the old download manager seems to be unused but when we sitch to the new one it might get used.

>> // Those are handled in suite/ code thus need to be defined here

I think they can go too because they are not suite specific. If FF changed them I would need to look at the code and remove or alter them too.

FRG

Comment 6

[rsx11m]

(In reply to Frank-Rainer Grahl from comment #5)
> I think they can go too because they are not suite specific. If FF changed
> them I would need to look at the code and remove or alter them too.

Looking at http://mxr.mozilla.org/comm-central/source/suite/common/utilityOverlay.js#1175 those are for handling the "Why is this site blocked" button in the UI when the page is reported, thus I'd think we still need those.

There is no occurrence of either browser.safebrowsing.warning.infoURL or .controlledAccess.infoURL in the mozilla/ tree. https://support.mozilla.org/kb/controlledaccess now points to an Firefox for Android article "Firefox family-friendly browsing in a restricted profile" thus may need to be revised (whatever you wanted to say there). If so, this should go into a follow-up bug, unless you have a URL readily available for me to plug in instead.

Comment 7

[rsx11m]

Annoyingly, the http://mozilla.org/firefox/its-an-attack.html and http://mozilla.org/firefox/its-a-trap.html test pages are no longer around or have been moved (both are dead links now).

Comment 8

[rsx11m]

Here we go, sorry for the noise...

Found https://support.mozilla.org/en-US/kb/how-does-phishing-and-malware-protection-work which in turn points to http://itisatrap.org/firefox/its-a-trap.html http://itisatrap.org/firefox/its-an-attack.html and http://itisatrap.org/firefox/unwanted.html (where the latter presents me with a funny combination of both "Reported Web Forgery!" and "Reported Attack Page!" warnings).

Clicking "Why was this page blocked" on neither page does actually use the values in the prefs, one points to the exact same page that I've looked up, the other to a Google page. Thus, I'd think that those prefs likely can go along with the unused code in utilityOverlay.js, *but* in a separate bug.

Comment 9

[Frank-Rainer Grahl (:frg)]

Just for reference here are the 4 safebrowsing pages I test with:

http://itisatrap.org/firefox/unwanted.html
http://itisatrap.org/firefox/its-a-trap.html
http://itisatrap.org/firefox/its-an-attack.html
http://itisatrap.org/firefox/forbidden.html (set browser.safebrowsing.forbiddenURIs.enabled)

With the current prefs all is well in 2.46a1 but as has been remarked some of the info urls go the firefox specific help pages.

Comment 10

[rsx11m]

Attachment: Proposed patch (v2)

> Still waiting for approval of attachment 8747050 [details] there, pending any changes for
> security.onecrl.maximum_staleness_in_seconds as suggested in bug 1266235 comment #11.

The patch there got approval now without that pref being moved, thus all that needs to be done here is to update the comments with respect to the discussion on the prefs defined or overridden.

I have opened bug 1270168 for the "Why was this page blocked" button.

Comment 11

[rsx11m]

Per bug 1266235 comment #19, security.onecrl.maximum_staleness_in_seconds will be moved after all.
Thus, I'll wait with adjusting the patch and requesting review until that has checked in.

Comment 12

[rsx11m]

Attachment: Updated patch (v3)

Remove security.onecrl.maximum_staleness_in_seconds as well.

Comment 13

[rsx11m]

Comment on attachment 8750741 [details] [diff] [review]
Updated patch (v3)

Ref. https://hg.mozilla.org/integration/fx-team/rev/f684fac95bd9

Comment 14

[Philip Chee]

Comment on attachment 8750741 [details] [diff] [review]
Updated patch (v3)

> -pref("browser.safebrowsing.provider.google.updateURL", "https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%");
> -pref("browser.safebrowsing.provider.google.gethashURL", "https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");

Bug 1077874 - Only expose Major version of Firefox in SafeBrowsing
In all.js it is appver=%MAJOR_VERSION% which in our case means "2" for the foreseeable future.
We don't seem to have the browser.safebrowsing.provider.mozilla.* prefs but if we do want them we should override those affected by Bug 1077874.

> -pref("browser.safebrowsing.provider.google.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=Firefox&hl=%LOCALE%&site=");
in all.js client=%NAME% will return SeaMonkey :P Do we still want to pretend to be Firefox?

Comment 15

[Frank-Rainer Grahl (:frg)]

>> in all.js client=%NAME% will return SeaMonkey :P Do we still want to pretend to be Firefox?

Not if it works with Seamonkey. Unlike the downloads and gethash url this one seems to work so imho %NAME% should be it.

Comment 16

[rsx11m]

(In reply to Philip Chee from comment #14)
> Bug 1077874 - Only expose Major version of Firefox in SafeBrowsing
> In all.js it is appver=%MAJOR_VERSION% which in our case means "2" for the
> foreseeable future.

I see, bummer. Easy to add that back.

> We don't seem to have the browser.safebrowsing.provider.mozilla.* prefs but
> if we do want them we should override those affected by Bug 1077874.

Do we want them? I've noticed that they were missing in our implementation, but don't know what they are doing. Since we now have them with all.js, probably can't hurt to override them as well.

> > -pref("browser.safebrowsing.provider.google.reportURL", "https://safebrowsing.google.com/safebrowsing/diagnostic?client=Firefox&hl=%LOCALE%&site=");
> in all.js client=%NAME% will return SeaMonkey :P Do we still want to pretend to be Firefox?

So, what's the verdict? Either way is ok with me, not having to override another pref seems to be preferable (and that's only for the report function anyway, thus shouldn't affect safe browsing as such).

Comment 17

[Frank-Rainer Grahl (:frg)]

>> Do we want them? I've noticed that they were missing in our implementation, but don't know 
>> what they are doing. Since we now have them with all.js, probably can't hurt to override them 
>> as well.

Maybe Mozilla is slowly moving away from google. Good idea. I would put them im.

>> So, what's the verdict? Either way is ok with me,

Ratty should decide but if it works with Seamonkey Seamonkey it should be. 

FRG

Comment 18

[Philip Chee]

> Ratty should decide but if it works with Seamonkey Seamonkey it should be.
If SeaMonkey works, then that's fine to use the defaults in all.js. IIRC (it was a long time ago when I last looked into this) using "Firefox" gives a different Firefox branded page with more options.

Comment 19

[rsx11m]

I get exactly the same page with the same content using either of those:
> http://safebrowsing.clients.google.com/safebrowsing/diagnostic?client=Firefox&hl=en-US&site=http://itisatrap.org/firefox/its-an-attack.html
> http://safebrowsing.clients.google.com/safebrowsing/diagnostic?client=SeaMonkey&hl=en-US&site=http://itisatrap.org/firefox/its-an-attack.html

Comment 20

[rsx11m]

Attachment: Some overrides retained (v4)

Comment #14 addressed.

Comment 21

[Philip Chee]

Comment on attachment 8753089 [details] [diff] [review]
Some overrides retained (v4)

r=me a=me if needed.

Comment 22

[rsx11m]

Thanks, bug 1266235 attachment 8747050 [details] still hasn't re-landed, thus can't push this yet.

Comment 23

[rsx11m]

https://hg.mozilla.org/integration/fx-team/rev/a5aad78f70ea has landed and thus far didn't get backed out again. :-)

Comment 24

[rsx11m]

Core patch was allowed to land on mozilla-central and is thus effective.
Pushed this patch as http://hg.mozilla.org/comm-central/rev/d5c2a0c809d1