Closed Bug 1388574 Opened 2 years ago Closed 2 years ago

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

Categories

(Toolkit :: Safe Browsing, defect, P3)

defect

Tracking

()

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- verified
firefox57 --- verified

People

(Reporter: francois, Assigned: francois)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Currently we download the Application Reputation lists even if the download protection feature is disabled (via browser.safebrowsing.downloads.enabled), which is unnecessary.
Depends on: 1385484
Attached patch WIP patch (obsolete) — Splinter Review
Attachment #8900058 - Attachment is obsolete: true
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
Attachment #8900446 - Flags: review?(hchang)
(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).
Attachment #8900446 - Flags: review?(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 :)
Attachment #8900446 - Flags: review?(hchang) → review+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c3cb4f92cea
Use the correct pref to gate the Application Reputation lists. r=hchang
https://hg.mozilla.org/mozilla-central/rev/4c3cb4f92cea
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Blocks: 1394053
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
Attachment #8900446 - Flags: approval-mozilla-beta?
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')
Flags: needinfo?(francois)
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on the latest Nightly build? Thanks!
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
Here's a rebased patch for Beta.
Flags: needinfo?(francois)
Attachment #8901987 - Flags: approval-mozilla-beta?
Attachment #8900446 - Flags: approval-mozilla-beta?
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?
Flags: needinfo?(brindusa.tot) → needinfo?(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.
Flags: needinfo?(francois)
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 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!
Attachment #8901987 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I can confirm this issue is fixed, I verified using Firefox 56.0b9 (20170903140023) on Ubuntu 14.04 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Depends on: 1431192
You need to log in before you can comment on or make changes to this bug.