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

RESOLVED FIXED in seamonkey2.46

Status

defect
--
minor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

Tracking

(Blocks 2 bugs)

Trunk
seamonkey2.46
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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");
Depends on: 1033450
Looks like spring cleaning in progress. ;-)
Sure, let's add those to the patch.
Summary: Remove Kinto preferences from SeaMonkey's browser-prefs.js after bug 1266235 moved them to global all.js → Remove Kinto and Safe-Browsing preferences from SeaMonkey's browser-prefs.js after they were moved to global all.js
Posted patch Possible patch (obsolete) — Splinter Review
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.
Attachment #8748409 - Flags: feedback?(frgrahl)
Blocks: 920951
>> +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 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
Attachment #8748409 - Flags: feedback?(frgrahl) → feedback+
(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.
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).
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.
Blocks: 1270168
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.
Posted patch Proposed patch (v2) (obsolete) — Splinter Review
> 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.
Attachment #8748409 - Attachment is obsolete: true
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.
Posted patch Updated patch (v3) (obsolete) — Splinter Review
Remove security.onecrl.maximum_staleness_in_seconds as well.
Attachment #8749629 - Attachment is obsolete: true
Comment on attachment 8750741 [details] [diff] [review]
Updated patch (v3)

Ref. https://hg.mozilla.org/integration/fx-team/rev/f684fac95bd9
Attachment #8750741 - Flags: review?(philip.chee)
Blocks: 1266965
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?
Attachment #8750741 - Flags: review?(philip.chee) → review-
>> 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.
(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).
Flags: needinfo?(philip.chee)
>> 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
> 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.
Flags: needinfo?(philip.chee)
Comment #14 addressed.
Attachment #8750741 - Attachment is obsolete: true
Attachment #8753089 - Flags: review?(philip.chee)
Comment on attachment 8753089 [details] [diff] [review]
Some overrides retained (v4)

r=me a=me if needed.
Attachment #8753089 - Flags: review?(philip.chee) → review+
Thanks, bug 1266235 attachment 8747050 [details] still hasn't re-landed, thus can't push this yet.
https://hg.mozilla.org/integration/fx-team/rev/a5aad78f70ea has landed and thus far didn't get backed out again. :-)
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.46
You need to log in before you can comment on or make changes to this bug.