1394335 - goog-unwanted-shavar is unexpectedly added to malware list in V4 only

Status: VERIFIED FIXED

(Toolkit :: Safe Browsing, defect, P1)

Description

[Thomas Nguyen (:tnguyen)]

http://searchfox.org/mozilla-central/rev/18c16ebf818abb86805ce08a6e537e4cd826f044/browser/components/preferences/in-content/security.js#205
When users disable and enable unwanted list in V4 only, then goog-unwanted-shavar is added to the malware list. That may trigger V2 update

Comment 1

[Thomas Nguyen (:tnguyen)]

Attachment: Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

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

Comment 2

[Thomas Nguyen (:tnguyen)]

Hmm, the patch only works for explicit google list only,  I think we have to move unwanted list to a separated preference. In that way, we can add/remove correctly all unwanted lists of all providers (if there's any)

Comment 3

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since we are planning rollout for this feature for 56.

Comment 4

[François Marier [:francois]]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review178784

I think we should add a little more logic here in order to support both V2 and V4. See comments below.

::: browser/components/preferences/in-content-new/privacy.js:1105
(Diff revision 1)
>        blockUnwantedPref.value = blockUncommonUnwanted.checked;
>        blockUncommonPref.value = blockUncommonUnwanted.checked;
>  
>        let malware = malwareTable.value
>          .split(",")
> -        .filter(x => x !== "goog-unwanted-shavar" && x !== "test-unwanted-simple");
> +        .filter(x => x !== "goog-unwanted-proto" && x !== "test-unwanted-simple");

Let's filter out both "goog-unwanted-proto" and "goog-unwanted-shavar"

::: browser/components/preferences/in-content-new/privacy.js:1108
(Diff revision 1)
>        let malware = malwareTable.value
>          .split(",")
> -        .filter(x => x !== "goog-unwanted-shavar" && x !== "test-unwanted-simple");
> +        .filter(x => x !== "goog-unwanted-proto" && x !== "test-unwanted-simple");
>  
>        if (blockUncommonUnwanted.checked) {
> -        malware.push("goog-unwanted-shavar");
> +        malware.push("goog-unwanted-proto");

Here we should look for "goog-malware-shavar" in the array and if it's there, then we add "goog-unwanted-shavar".

If it's not there, we assume we're on V4 and then add "goog-unwanted-proto" instead.

Comment 5

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

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

Comment 6

[Thomas Nguyen (:tnguyen)]

Thanks Francois,
I updated the patch, assuming Google is the only provider which provides -unwanted- list.

Comment 7

[François Marier [:francois]]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review178868

Comment 8

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

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

Comment 9

[Thomas Nguyen (:tnguyen)]

Updated the patch because I think we may have to update a test for the new logic.
Try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0e40a6c2f50b84f259e77189c5cdbd4229b1472

Comment 10

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

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

Comment 11

[François Marier [:francois]]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review179312

Let's be more explicit about pref setting in the test so that it's less fragile.

::: browser/components/preferences/in-content-new/tests/browser_security-2.js:68
(Diff revision 4)
>  add_task(async function() {
> -  async function checkPrefSwitch(val1, val2) {
> +  async function checkPrefSwitch(val1, val2, isV2) {
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", val1);
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_uncommon", val2);
> +    let malwareTable = Services.prefs.getCharPref("urlclassifier.malwareTable");
> +    if (isV2 && malwareTable.indexOf("goog-malware-shavar") ==-1) {

This is a bit hacky because it's doing the minimum to make the logic of the checkbox work.

We may as well overwrite `urlclassifier.malwareTable` completely:

    malwareTable = "goog-malware-" + isV2 ? "shavar" : "proto"
    if val1 && val2:
        malwareTable += ",goog-unwanted-" ? isV2 : "shavar" : "proto"
    malwareTable += ",test-malware-simple"
    if val1 && val2:
        malwareTable += ",test-unwanted-simple"

so that it's set to a value that is closer to the real one.

::: browser/components/preferences/in-content/tests/browser_security.js:107
(Diff revision 4)
>  add_task(async function() {
> -  async function checkPrefSwitch(val1, val2) {
> +  async function checkPrefSwitch(val1, val2, isV2) {
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_potentially_unwanted", val1);
>      Services.prefs.setBoolPref("browser.safebrowsing.downloads.remote.block_uncommon", val2);
> +    let malwareTable = Services.prefs.getCharPref("urlclassifier.malwareTable");
> +    if (isV2 && malwareTable.indexOf("goog-malware-shavar") ==-1) {

ditto

Comment 12

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

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

Comment 13

[Thomas Nguyen (:tnguyen)]

Sure, thanks for your review

Comment 14

[François Marier [:francois]]

Comment on attachment 8901743 [details]
Bug 1394335 - Add correct unwanted list when users enable uncommon and unwanted software from preference page

https://reviewboard.mozilla.org/r/173192/#review179350

Thanks Thomas!

::: browser/components/preferences/in-content-new/tests/browser_security-2.js:62
(Diff revisions 4 - 5)
>  
>    await checkPrefSwitch(true);
>    await checkPrefSwitch(false);
>  });
>  
> +requestLongerTimeout(2);

Is that in seconds? Should we make it 5 seconds instead to make sure it doesn't turn into an intermittent when run on slow build machines?

::: browser/components/preferences/in-content/tests/browser_security.js:100
(Diff revision 5)
>  
>    await checkPrefSwitch(true);
>    await checkPrefSwitch(false);
>  });
>  
> +requestLongerTimeout(2);

ditto

Comment 15

[Thomas Nguyen (:tnguyen)]

That means 2*45s

Comment 16

[Thomas Nguyen (:tnguyen)]

Thanks for your review.
Trigger a try runnning
https://treeherder.mozilla.org/#/jobs?repo=try&revision=87ef67f29cdacc437e23c90c9b604fdf32392425

Comment 17

[Thomas Nguyen (:tnguyen)]

Attachment: Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

MozReview-Commit-ID: Ju0M47AWGVA

Comment 18

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Beta release patch, but need to wait for the patch landing in m-c first

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c80c4a770752
Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c2a787cc5a2
Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois

Comment 21

[Thomas Nguyen (:tnguyen)]

Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Approval Request Comment
[Feature/Bug causing the regression]: Enable V4 in FF 56 bug 1366920
[User impact if declined]: V2 update in google list is still getting around
[Is this code covered by automated tests?]: N
[Has the fix been verified in Nightly?]: Not yet
[Needs manual test from QE? If yes, steps to reproduce]: 
Start with an official build of Firefox (e.g. Nightly).

1. Go to about:preferences#privacy
2. Make sure all the check boxes in Security->Phishing Protection are shown
3. Uncheck/check the check boxes "Warn you about unwanted and uncommon software"
4. Go to about:config and search urlclassifier.malwaretable
5. Make sure in V4 only, only goog-unwanted-proto is added/removed

Repeat from step 3 several times

[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only flip correct preference 
[String changes made/needed]: No

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/3c2a787cc5a2

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, can your team verify the fix ? Thanks!

Comment 24

[François Marier [:francois]]

Small correction to the testing steps:

(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #21)
> 5. Make sure in V4 only, only goog-unwanted-proto is added/removed

goog-unwanted-proto AND test-unwanted-simple will be added/removed as the "Warn you about unwanted and uncommon software" checkbox is toggled.

Comment 25

[Wesly Huang (EPM)]

Timea, maybe you can help check this as well since you are executing the sign-off test this week? Thanks!

Comment 26

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox  57.0a1 (2017.08.28) on Win 8.1 x64.
I can confirm this issue is fixed, I verified using Firefox 57.0a1 (2017.09.05) Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.

Comment 27

[Wesly Huang (EPM)]

(thanks Timea for the timely help!)

Hi Liz:

would you kindly approve the uplift? Thanks.

(In reply to Timea Zsoldos from comment #26)
> I have reproduced this issue using Firefox  57.0a1 (2017.08.28) on Win 8.1
> x64.
> I can confirm this issue is fixed, I verified using Firefox 57.0a1
> (2017.09.05) Win 8.1 x64, Mac OS X 10.11 and Ubuntu 14.04 x64.

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8902605 [details] [diff] [review]
Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page

Fix the issue related to Safe Browsing and was verified. Beta56+.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90926a141c6d

Comment 30

[Timea Zsoldos [:tzsoldos], Desktop QA]

I can confirm this issue is fixed, I verified using Firefox 56.0b11 Win 8.1 x64, Mac OS X 10.10 and Ubuntu 14.04 x64.