1388574 - Application reputation lists should be gated by the safebrowsing.dowloads.enabled pref

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P3)

Description

[François Marier [:francois]]

Currently we download the Application Reputation lists even if the download protection feature is disabled (via browser.safebrowsing.downloads.enabled), which is unnecessary.

Comment 1

[François Marier [:francois]]

Attachment: WIP patch

Comment 2

[François Marier [:francois]]

Attachment: Bug 1388574 - Use the correct pref to gate the Application Reputation lists.

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

Comment 3

[François Marier [:francois]]

Comment on attachment 8900446 [details]
Bug 1388574 - Use the correct pref to gate the Application Reputation lists.

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

Comment 4

[Henry Chang [:hchang]]

Comment on attachment 8900446 [details]
Bug 1388574 - Use the correct pref to gate the Application Reputation lists.

https://reviewboard.mozilla.org/r/171800/#review177170

The changes look good but would this patch unexpectedly disable download protection on Fennec?
It was behind the pref 'browser.safebrowsing.malware.enabled' (which is 'true' on Fennec)
but it's false because of

http://searchfox.org/mozilla-central/rev/48ea452803907f2575d81021e8678634e8067fc2/mobile/android/app/mobile.js#643

Comment 5

[François Marier [:francois]]

(In reply to Henry Chang [:hchang] from comment #4)
> The changes look good but would this patch unexpectedly disable download
> protection on Fennec?
> It was behind the pref 'browser.safebrowsing.malware.enabled' (which is
> 'true' on Fennec)
> but it's false because of

Download protection is disabled on purpose on Fennec because it's missing UI (bug 1239094).

Comment 6

[Henry Chang [:hchang]]

Comment on attachment 8900446 [details]
Bug 1388574 - Use the correct pref to gate the Application Reputation lists.

https://reviewboard.mozilla.org/r/171800/#review177248

Got it! Thanks :)

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c3cb4f92cea
Use the correct pref to gate the Application Reputation lists. r=hchang

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4c3cb4f92cea

Comment 9

[François Marier [:francois]]

Comment on attachment 8900446 [details]
Bug 1388574 - Use the correct pref to gate the Application Reputation lists.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1394053
[User impact if declined]: Safe Browsing broken on most Linux distributions
[Is this code covered by automated tests?]: Only SV smoketests
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Yes, https://testrail.stage.mozaws.net/index.php?/cases/view/3955
[List of other uplifts needed for the feature/fix]: This is a dependency of the fix we'll add in bug 1394053.
[Is the change risky?]: No
[Why is the change risky/not risky?]: If the gating mechanism doesn't work the lists won't be downloaded and the above smoketest will fail.
[String changes made/needed]: None

Comment 10

[Sylvestre Ledru [:Sylvestre]]

Doesn't apply to beta:

grafting 419097:4c3cb4f92cea "Bug 1388574 - Use the correct pref to gate the Application Reputation lists. r=hchang"
merging toolkit/components/url-classifier/SafeBrowsing.jsm
 warning: conflicts while merging toolkit/components/url-classifier/SafeBrowsing.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 11

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!

Comment 12

[François Marier [:francois]]

Attachment: Patch reviewed by Henry but rebased onto beta

Here's a rebased patch for Beta.

Comment 13

[Brindusa Tot, Desktop QA]

I verified the steps from https://testrail.stage.mozaws.net/index.php?/cases/view/3955 on Ubuntu 16.04 with the latest Nightly 57.0a1, Build ID 20170829100404(with the fix) and also on an older Nightly build from 08/08/2017 without the fix, Build ID 20170808114032, and could not see any difference between the two runs. 

Francois, what exactly should we observe with this fix?

Comment 14

[François Marier [:francois]]

(In reply to Brindusa Tot[:brindusat] from comment #13)
> I verified the steps from
> https://testrail.stage.mozaws.net/index.php?/cases/view/3955 on Ubuntu 16.04
> with the latest Nightly 57.0a1, Build ID 20170829100404(with the fix) and
> also on an older Nightly build from 08/08/2017 without the fix, Build ID
> 20170808114032, and could not see any difference between the two runs. 

That's good, it means the feature is still working fine :)

> Francois, what exactly should we observe with this fix?

In order to see a difference, you can do the following:

1. Create a new profile and start the browser.
2. Wait 5 minutes.
3. Go to https://testsafebrowsing.appspot.com/ and test #1 of the "Desktop Download Warnings"
4. Close the browser
5. Look in ~/.cache/mozilla/firefox/XXXX/safebrowsing/google4/
6. The goog-downloadwhite-proto.pset and goog-badbinurl-proto.pset files should be present.

That's just to make sure that everything is working. It should be the same before and after the fix.

Then, for the real test:

1. Create a new profile and start the browser.
2. Open the Preferences and go to Privacy & Security | Security | Phishing Protection.
3. Untick the "Block dangerous downloads" option
4. Close the browser
5. Delete ~/.cache/mozilla/firefox/XXXX/safebrowsing/google4/
6. Open the browser again, with the same profile
7. Go into about:url-classifier and trigger an update of the "google4" provider
8. "Last update status" should say "success" after a few seconds.
9. Close the browser
10. Look in ~/.cache/mozilla/firefox/XXXX/safebrowsing/google4/

Prior to the fix, the goog-downloadwhite-proto.pset and goog-badbinurl-proto.pset files should be present.

With this fix in place, these files will not be downloaded when "Block dangerous downloads" is disabled.

Comment 15

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  56.0b7 because the fix is not there yet, on Ubuntu 14.04 x64. I can confirm this issue is fixed, I verified using Firefox nightly 57.0a1 (20170829100404) on Ubuntu 14.04 x64.

Comment 16

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8901987 [details] [diff] [review]
Patch reviewed by Henry but rebased onto beta

Please uplift for beta 8.  Thanks for the verification in nightly!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/c7485146e243

Comment 18

[Timea Zsoldos [:zstimi/tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 56.0b9 (20170903140023) on Ubuntu 14.04 x64.