1033450 - consolidate safebrowsing prefs in all.js

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

They are currently in firefox.js, mobile.js, and b2g.js(!). Some are wrapped in ifdef MOZ_SAFE_BROWSING and some are not. Update URLs are only defined in firefox.js and mobile.js, though some safebrowsing prefs only(!) exist in b2g.js.

pref("browser.safebrowsing.enabled", true);
pref("browser.safebrowsing.malware.enabled", true);
pref("browser.safebrowsing.debug", true);

pref("browser.safebrowsing.updateURL", "https://safebrowsing.google.com/safebrowsing/downloads?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2&key=%GOOGLE_API_KEY%");
pref("browser.safebrowsing.gethashURL", "https://safebrowsing.google.com/safebrowsing/gethash?client=SAFEBROWSING_ID&appver=%VERSION%&pver=2.2");
pref("browser.safebrowsing.reportURL", "https://safebrowsing.google.com/safebrowsing/report?");
pref("browser.safebrowsing.reportGenericURL", "http://%LOCALE%.phish-generic.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportErrorURL", "http://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportPhishURL", "http://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportMalwareURL", "http://%LOCALE%.malware-report.mozilla.com/?hl=%LOCALE%");
pref("browser.safebrowsing.reportMalwareErrorURL", "http://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%");

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Francois, not sure how important that is. Can you please apply a priority or mark as wontfix if not needed anymore.

Comment 2

[François Marier [:francois]]

Good first bug since it's easy to fix but requires testing the various ways in which Safe Browsing is used.

Comment 3

[Dimi Lee [:dimi]]

I would like to work on this bug.

Comment 4

[Dimi Lee [:dimi]]

Attachment: (WIP) P1. Consolidate safebrowsing prefs in all.js

Comment 5

[François Marier [:francois]]

Comment on attachment 8731545 [details] [diff] [review]
(WIP) P1. Consolidate safebrowsing prefs in all.js

Review of attachment 8731545 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/app/mobile.js
@@ -653,5 @@
>  
>  // optimize images memory usage
>  pref("image.downscale-during-decode.enabled", true);
>  
>  #ifdef MOZ_SAFE_BROWSING

It would be interesting as part of this bug to verify whether or not we can build Firefox without MOZ_SAFE_BROWSING.

@@ -660,1 @@
>  pref("browser.safebrowsing.downloads.enabled", false);

Note: we'll need to confirm (by building on Fennec and testing manually) that this pref override takes precedence over the default that's set in all.js because Fennec doesn't yet have the UI to handle blocked downloads.

Comment 6

[Dimi Lee [:dimi]]

Attachment: (WIP) P2. Remove MOZ_SAFE_BROWSING

Only run try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=90ffe3429e15

Not yet verify on various platform

Comment 7

[François Marier [:francois]]

You could move your second patch to bug 336290.

Comment 9

[Dimi Lee [:dimi]]

Hi François,
Sorry pending this for a while.

I have some question while testing, i guess i can check with you next week in taipei.
Just leave some note first

A. http://testsafebrowsing.appspot.com/
[1]Desktop Webpage Warnings - 4. Client-side phishing detection,
The page is not blocked, what is the expected behavior ?

[2]Desktop Download Warnings - 4,5,6,7 Windows platform download
Is there any other way to test it on ubuntu or mac ? or it can only be test on windows.

[3]Bad IP Warnings
After clicking link from http://testsafebrowsing.appspot.com/s/badip.html, i didn't see anything special, but i did see warning when use chrome.
What do we expect after navigating to thpse pages. 

B.https://wiki.mozilla.org/Security/Tracking_protection#QA
I turned on Pref privacy.trackingprotection.enabled
And the link from "Strict blacklist" still shows "Your browser is not set up for tracking protection. Please check your setup and try again" , is this correct ?

Comment 10

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #9)
> A. http://testsafebrowsing.appspot.com/
> [1]Desktop Webpage Warnings - 4. Client-side phishing detection,
> The page is not blocked, what is the expected behavior ?

Yes, we don't implement the client-side stuff in Firefox.

> [2]Desktop Download Warnings - 4,5,6,7 Windows platform download
> Is there any other way to test it on ubuntu or mac ? or it can only be test
> on windows.

You should be able to test them on Linux or Mac in Firefox.

> [3]Bad IP Warnings
> After clicking link from http://testsafebrowsing.appspot.com/s/badip.html, i
> didn't see anything special, but i did see warning when use chrome.
> What do we expect after navigating to thpse pages. 

We don't implement the Bad IP Warnings in Firefox. I've added a note on https://wiki.mozilla.org/Security/Safe_Browsing#QA.

> B.https://wiki.mozilla.org/Security/Tracking_protection#QA
> I turned on Pref privacy.trackingprotection.enabled
> And the link from "Strict blacklist" still shows "Your browser is not set up
> for tracking protection. Please check your setup and try again" , is this
> correct ?

In order for that one to work, you need to switch to the "Strict" tracking protection list:

about:preferences | Privacy | Use Tracking Protection | Change Block List | Disconnect.me strict protection.

Comment 11

[Dimi Lee [:dimi]]

Attachment: MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review commit: https://reviewboard.mozilla.org/r/45977/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45977/

Comment 12

[Dimi Lee [:dimi]]

I have test this patch manually on both desktop and fennec with following tests

[1]https://wiki.mozilla.org/Phishing_Protection
- Malware, phishing, unwanted software and forbidden

[2]http://testsafebrowsing.appspot.com/
- Desktop Webpage Warnings
- Desktop Download Warnings (enabled in desktop and disabled in fennec)

[3]http://itisatrap.org/firefox/forbidden.html

[4]https://wiki.mozilla.org/Security/Tracking_protection#QA
- Blacklist and whitelist using hardcoded values (Show tracker)
- Standard blacklist (default not enabled)
- Strict blacklist (default not enabled)

Also try looks ok:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ea3d5488b3d

Comment 13

[François Marier [:francois]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review42531

As discussed in person.

::: b2g/app/b2g.js:350
(Diff revision 1)
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -// Prevent loading of pages identified as malware
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", true);

We can move these ones to libpref as well and only override them in Fennec.

::: b2g/app/b2g.js:353
(Diff revision 1)
> -
> -pref("browser.safebrowsing.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
>  
>  pref("browser.safebrowsing.id", "Firefox");

This is the same as the Fennec's use of @MOZ_APP_UA_NAME@ so we don't need to hardcode "Firefox" here.

::: browser/app/profile/firefox.js:967
(Diff revision 1)
> -
> -pref("browser.safebrowsing.reportPhishMistakeURL", "https://%LOCALE%.phish-error.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportPhishURL", "https://%LOCALE%.phish-report.mozilla.com/?hl=%LOCALE%&url=");
> -pref("browser.safebrowsing.reportMalwareMistakeURL", "https://%LOCALE%.malware-error.mozilla.com/?hl=%LOCALE%&url=");
>  
>  #ifdef MOZILLA_OFFICIAL

Let's move this block to libpref and add:

    #else
    pref("browser.safebrowsing.id", "@MOZ_APP_UA_NAME@");
    #endif

so that the ID is the same on all platforms for official and non-official builds.

::: browser/app/profile/firefox.js:975
(Diff revision 1)
>  pref("browser.safebrowsing.id", "navclient-auto-ffox");
>  #endif
>  
>  // Name of the about: page contributed by safebrowsing to handle display of error
>  // pages on phishing/malware hits.  (bug 399233)
>  pref("urlclassifier.alternate_error_page", "blocked");

This can also move to libpref.

::: mobile/android/app/mobile.js:648
(Diff revision 1)
>  pref("image.downscale-during-decode.enabled", true);
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", false);

We need to make sure that this override still works (i.e. malware downloads are not blocked on Fennec).

::: mobile/android/app/mobile.js:649
(Diff revision 1)
>  
>  #ifdef MOZ_SAFE_BROWSING
> -pref("browser.safebrowsing.enabled", true);
> -pref("browser.safebrowsing.malware.enabled", true);
>  pref("browser.safebrowsing.downloads.enabled", false);
>  pref("browser.safebrowsing.downloads.remote.enabled", false);

This second pref is unnecessary given that `browser.safebrowsing.downloads.enabled` is already set to `false`.

::: modules/libpref/init/all.js:4950
(Diff revision 1)
>  pref("dom.inter-app-communication-api.enabled", false);
>  
>  // Disable mapped array buffer by default.
>  pref("dom.mapped_arraybuffer.enabled", false);
>  
> +#ifdef MOZ_SAFE_BROWSING

I think we should leave this out since it's being removed anyways in a different bug.

::: modules/libpref/init/all.js:4969
(Diff revision 1)
> +pref("urlclassifier.downloadBlockTable", "goog-badbinurl-shavar");
> +
> +// The number of random entries to send with a gethash request.
> +pref("urlclassifier.gethashnoise", 4);
> +
> +// Gethash timeout for Safebrowsing.	

nit: trailing whitespace

::: modules/libpref/init/all.js:5020
(Diff revision 1)
>  pref("browser.safebrowsing.provider.mozilla.lists.mozfull.name", "mozfullName");
>  pref("browser.safebrowsing.provider.mozilla.lists.mozfull.description", "mozfullDesc");
>  
>  // Allow users to ignore Safe Browsing warnings.
>  pref("browser.safebrowsing.allowOverride", true);
> +#endif

We also need to remove this since we'll leave the `ifdef` out.

Comment 14

[Dimi Lee [:dimi]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/1-2/

Comment 15

[François Marier [:francois]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review44003

That looks great Dimi!

If you haven't tested that Download Protection is disabled on Fennec (i.e. you can download all of the test malware binaries from <http://testsafebrowsing.appspot.com>), then please do that before landing.

Comment 16

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #15)
> If you haven't tested that Download Protection is disabled on Fennec (i.e.
> you can download all of the test malware binaries from
> <http://testsafebrowsing.appspot.com>), then please do that before landing.

Yes, I have test download protection on fennec, it is disabled.
The try result looks strange now, so i pull latest code and push to try again to see if anything different.

Comment 17

[Dimi Lee [:dimi]]

try looks good
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4835ed04327

Comment 18

[Pulsebot]

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

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f71acdbd0d45

Comment 20

[Ethan Tseng [:ethan]]

Good job! Thanks, Dimi and François!

Comment 21

[François Marier [:francois]]

The patch that landed on central (https://hg.mozilla.org/mozilla-central/rev/f71acdbd0d45) is not actually the right one, this needs to be backed out and re-done.

The first revision (https://reviewboard.mozilla.org/r/45977/diff/1/) is what landed whereas the second revision (https://reviewboard.mozilla.org/r/45977/diff/2/) is what should have landed.

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0b75a2580c13 (backout)
https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307

Comment 23

[Carsten Book [:Tomcat]]

backout and re-landed in  https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307 

François: could you take a look at this and verify i landed the correct patch ? Thanks!

Comment 24

[François Marier [:francois]]

(In reply to Carsten Book [:Tomcat] from comment #23)
> backout and re-landed in 
> https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307 
> 
> François: could you take a look at this and verify i landed the correct
> patch ? Thanks!

Looks great, thanks!

Comment 25

[Carsten Book [:Tomcat]]

sorry had to back this out again for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=8905525&repo=fx-team

Comment 26

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/b813b1e6e7e3

Comment 27

[:Paolo Amadini]

Since we would like to land bug 1265359 and bug 1265358 later today, if the two bugs land first I simply suggest rebasing this patch taking into consideration the simple pref changes made there.

As François pointed out, these changes can be moved to "all.js". As far as I can tell by looking at the patches, this might mean that these checks would be enabled on B2G as well, but not on Android where the entire feature is disabled. If this needs to be disabled for B2G, this could be handled later in a separate bug.

Comment 28

[:Paolo Amadini]

The pref changes landed in fx-team.

Comment 29

[François Marier [:francois]]

(In reply to :Paolo Amadini from comment #27)
> Since we would like to land bug 1265359 and bug 1265358 later today, if the
> two bugs land first I simply suggest rebasing this patch taking into
> consideration the simple pref changes made there.

Makes perfect sense. It looks like we have a bit more work to do looking into the test failures.

Comment 30

[François Marier [:francois]]

Dimi, one more thing we should add to the patch is to move this block from https://hg.mozilla.org/integration/fx-team/rev/2271b0c11307#l2.56:

  #ifdef XP_WIN
  // Only download the whitelist on Windows, since the whitelist is
  // only useful for suppressing remote lookups for signed binaries which we can
  // only verify on Windows (Bug 974579). Other platforms always do remote lookups.
  pref("urlclassifier.downloadAllowTable", "goog-downloadwhite-digest256");
  #endif

into modules/libpref/init/all.js too.

That way, all of the prefs are in the same place except for the overrides.

Comment 31

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #30)
> 
> That way, all of the prefs are in the same place except for the overrides.

Ok, i will add it to next patch

Comment 32

[Dimi Lee [:dimi]]

(In reply to Carsten Book [:Tomcat] from comment #25)
> sorry had to back this out again for failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=8905525&repo=fx-team

I think the root cause is that the annotation @PREF_NAME@ can only be used in fennec not in desktop.
This line : pref("browser.safebrowsing.id", @MOZ_APP_UA_NAME@);  in all.js cause this try error.

I am sorry that somehow i miss this change when push to try (Comment 17) so i thought everything is fine :(

Push to try again to see if everything works well after applying fix
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c55e06196947

Comment 33

[Dimi Lee [:dimi]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/2-3/

Comment 34

[Dimi Lee [:dimi]]

Hi Francois
Try looks good(Comment 32), so I update the patch to review again, following is the change in this patch:
1. Address Comment 30
2. Move pref("browser.safebrowsing.id", @MOZ_APP_UA_NAME@);  to mobile.js
3. In all.js, use

#ifdef MOZILLA_OFFICIAL
pref("browser.safebrowsing.id", "navclient-auto-ffox");
#else
pref("browser.safebrowsing.id", "Firefox");
#endif

Originally if MOZILLA_OFFICIAL is not defined, desktop will get id by |Services.appinfo.name|[1], which is "FireFox" for desktop.So i use |pref("browser.safebrowsing.id", "Firefox");| directly. Please let me know if you have any concern about this

[1]https://dxr.mozilla.org/mozilla-central/source/toolkit/components/url-classifier/SafeBrowsing.jsm#185

Comment 35

[François Marier [:francois]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review45915

That looks all good except for the default value of the UNCOMMON and UNWANTED prefs. These were just modified (when the UI landed in Fx48) so we need to set them both to true.

::: modules/libpref/init/all.js:5005
(Diff revision 3)
> +
> +// Name of the about: page contributed by safebrowsing to handle display of error
> +// pages on phishing/malware hits.  (bug 399233)
> +pref("urlclassifier.alternate_error_page", "blocked");
> +
> +pref("browser.safebrowsing.enabled", true);

nit: let's add "// Enable phishing protection" to the end of this line just to remind folks that it's not a global "disable all of safe browsing" switch.

::: modules/libpref/init/all.js:5014
(Diff revision 3)
> +pref("browser.safebrowsing.downloads.remote.enabled", true);
> +pref("browser.safebrowsing.downloads.remote.timeout_ms", 10000);
> +pref("browser.safebrowsing.downloads.remote.url", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +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);

This should now default to `true` (it just changed).

::: modules/libpref/init/all.js:5015
(Diff revision 3)
> +pref("browser.safebrowsing.downloads.remote.timeout_ms", 10000);
> +pref("browser.safebrowsing.downloads.remote.url", "https://sb-ssl.google.com/safebrowsing/clientreport/download?key=%GOOGLE_API_KEY%");
> +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);

This should also now default to `true`.

Comment 36

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #35)
> > +pref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", false);
> > +pref("browser.safebrowsing.downloads.remote.block_uncommon",             false);
> 
> This should also now default to `true`.

I found currently these two preferences still set to false for b2g & fennec
Is this intended ?

Comment 37

[François Marier [:francois]]

(In reply to Dimi Lee[:dimi][:dlee] from comment #36)
> > This should also now default to `true`.
> 
> I found currently these two preferences still set to false for b2g & fennec
> Is this intended ?

Fennec disables all of download protection by setting browser.safebrowsing.downloads.enabled to false (because it doesn't have UI for this), so we can flip these prefs to true and it won't be affected.

B2G doesn't disable download protection, so enabling more of it will presumably be fine.

Comment 38

[Dimi Lee [:dimi]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45977/diff/3-4/

Comment 39

[Dimi Lee [:dimi]]

(In reply to François Marier [:francois] from comment #37)
> Fennec disables all of download protection by setting
> browser.safebrowsing.downloads.enabled to false (because it doesn't have UI
> for this), so we can flip these prefs to true and it won't be affected.
> 
> B2G doesn't disable download protection, so enabling more of it will
> presumably be fine.

Ok, then i will just set those two preferences to true in all.js.
Thanks!

Comment 40

[François Marier [:francois]]

Comment on attachment 8740738 [details]
MozReview Request: Bug 1033450 - consolidate safebrowsing prefs in all.js. r?francois

https://reviewboard.mozilla.org/r/45977/#review46119

Comment 41

[Dimi Lee [:dimi]]

try :
https://treeherder.mozilla.org/#/jobs?repo=try&revision=220872e55454

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/84bc3fe034f2

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84bc3fe034f2

Comment 44

[Matt Wobensmith [:mwobensmith][:matt:]]

I verified by doing a test pass for Safe Browsing on latest Nightly builds, Mac/Win, 2016-05-10.

Comment 45

[Ethan Tseng [:ethan]]

(In reply to Matt Wobensmith [:mwobensmith][:matt] from comment #44)
> I verified by doing a test pass for Safe Browsing on latest Nightly builds,
> Mac/Win, 2016-05-10.

Matt, thanks for verifying this!