Closed
Bug 1394335
Opened 7 years ago
Closed 7 years ago
goog-unwanted-shavar is unexpectedly added to malware list in V4 only
Categories
(Toolkit :: Safe Browsing, defect, P1)
Toolkit
Safe Browsing
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | + | verified |
firefox57 | --- | verified |
People
(Reporter: tnguyen, Assigned: tnguyen)
References
Details
(Whiteboard: #sbv4-m9)
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
francois
:
review+
|
Details |
9.75 KB,
patch
|
gchang
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Assignee | ||
Updated•7 years ago
|
Priority: -- → P1
Whiteboard: #sbv4-m9
Assignee | ||
Updated•7 years ago
|
tracking-firefox56:
--- → ?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years ago
|
||
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)
Updated•7 years ago
|
Assignee: nobody → tnguyen
Status: NEW → ASSIGNED
Comment 4•7 years ago
|
||
mozreview-review |
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.
Attachment #8901743 -
Flags: review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
Thanks Francois, I updated the patch, assuming Google is the only provider which provides -unwanted- list.
Comment 7•7 years ago
|
||
mozreview-review |
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
Attachment #8901743 -
Flags: review?(francois) → review+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review |
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
Attachment #8901743 -
Flags: review+ → review-
Comment hidden (mozreview-request) |
Assignee | ||
Comment 13•7 years ago
|
||
Sure, thanks for your review
Comment 14•7 years ago
|
||
mozreview-review |
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
Attachment #8901743 -
Flags: review?(francois) → review+
Assignee | ||
Comment 15•7 years ago
|
||
That means 2*45s
Assignee | ||
Comment 16•7 years ago
|
||
Thanks for your review. Trigger a try runnning https://treeherder.mozilla.org/#/jobs?repo=try&revision=87ef67f29cdacc437e23c90c9b604fdf32392425
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 17•7 years ago
|
||
MozReview-Commit-ID: Ju0M47AWGVA
Assignee | ||
Comment 18•7 years ago
|
||
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
Attachment #8902605 -
Attachment description: Add correct unwanted list when users enable uncommon and unwanted software from preference page → Beta release patch - Add correct unwanted list when users enable uncommon and unwanted software from preference page
Comment 19•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c80c4a770752 Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois
Keywords: checkin-needed
Comment 20•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/3c2a787cc5a2 Add correct unwanted list when users enable uncommon and unwanted software from preference page r=francois
Assignee | ||
Comment 21•7 years ago
|
||
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
Attachment #8902605 -
Flags: approval-mozilla-beta?
Comment 22•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3c2a787cc5a2
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 23•7 years ago
|
||
Andrei, can your team verify the fix ? Thanks!
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Comment 24•7 years ago
|
||
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.
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Comment 25•7 years ago
|
||
Timea, maybe you can help check this as well since you are executing the sign-off test this week? Thanks!
Flags: needinfo?(timea.zsoldos)
Comment 26•7 years ago
|
||
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.
Flags: needinfo?(timea.zsoldos)
Comment 27•7 years ago
|
||
(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.
Flags: needinfo?(lhenry)
Comment 28•7 years ago
|
||
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+.
Flags: needinfo?(lhenry)
Attachment #8902605 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 29•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/90926a141c6d
Flags: in-testsuite+
Comment 30•7 years ago
|
||
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
You need to log in
before you can comment on or make changes to this bug.
Description
•