Closed
Bug 1394053
Opened 7 years ago
Closed 7 years ago
Having download protection enabled on non-official builds breaks all of Safe Browsing
Categories
(Toolkit :: Safe Browsing, defect, P1)
Tracking
()
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox55 | --- | unaffected |
firefox56 | + | verified |
firefox57 | --- | fixed |
People
(Reporter: francois, Assigned: tnguyen)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: #sbv4-m9)
Attachments
(3 files, 3 obsolete files)
59 bytes,
text/x-review-board-request
|
francois
:
review+
jaws
:
feedback+
|
Details |
59 bytes,
text/x-review-board-request
|
gcp
:
review+
|
Details |
17.76 KB,
patch
|
francois
:
review+
lizzard
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
We need to disable download protection when MOZ_OFFICIAL_BUILD != 1 just like we do for the phishing list: https://searchfox.org/mozilla-central/rev/5696c3e525fc8222674eed6a562f5fcbe804c4c7/modules/libpref/init/all.js#5343-5349 So we need to do two things: 1. set browser.safebrowsing.downloads.enabled = false when MOZILLA_OFFICIAL is not defined 2. remove the "Block dangerous downloads" and "Warn you about unwanted and uncommon software" from the Security section of about:preferences#privacy
Reporter | ||
Comment 1•7 years ago
|
||
This will be a problem for many Linux distributions so I'd like to uplift the fix on 56. The patch will not affect our official builds.
status-firefox56:
--- → affected
status-firefox57:
--- → affected
tracking-firefox56:
--- → ?
Summary: Disable download protection on non-official builds → Having download protection enabled on non-official builds breaks all of Safe Browsing
Reporter | ||
Updated•7 years ago
|
Blocks: 1366920
Keywords: regression
Reporter | ||
Comment 2•7 years ago
|
||
Nihanth, is there an established way to hide preferences in about:preferences when MOZILLA_OFFICIAL is not defined? (i.e. #2 in comment 0).
Flags: needinfo?(nhnt11)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → tnguyen
Comment 3•7 years ago
|
||
(In reply to François Marier [:francois] from comment #2) > Nihanth, is there an established way to hide preferences in > about:preferences when MOZILLA_OFFICIAL is not defined? (i.e. #2 in comment > 0). Closest precedent I can find is this Dev Edition specific pref: http://searchfox.org/mozilla-central/rev/89e125b817c5d493babbc58ea526be970bd3748e/browser/components/preferences/in-content-new/main.xul#287 We use ifdefs to hide prefs at build-time (as in that file); I don't know of any reason not to use an ifndef with MOZILLA_OFFICIAL.
Flags: needinfo?(nhnt11)
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to François Marier [:francois] from comment #1) > This will be a problem for many Linux distributions so I'd like to uplift > the fix on 56. > > The patch will not affect our official builds. Just curious, which problem will occur if we enable download protection on non-official builds?
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•7 years ago
|
||
I pushed a patch based on the idea on comment 0. Then, in the about:preference#privacy page, we only have a checkbox of SB enabled (which will enable SB -malware-, -phish-, -unwanted- lists). Francois, could you please take a look?
Reporter | ||
Comment 8•7 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #5) > (In reply to François Marier [:francois] from comment #1) > > This will be a problem for many Linux distributions so I'd like to uplift > > the fix on 56. > > > > The patch will not affect our official builds. > > Just curious, which problem will occur if we enable download protection on > non-official builds? If you enable that then the goog-downloadwhite-proto and goog-badbinurl-proto lists will be requested from the server whenever we try to do an update and that will make the whole update fail because these lists are not available.
Reporter | ||
Comment 9•7 years ago
|
||
(In reply to Nihanth Subramanya [:nhnt11] from comment #3) > Closest precedent I can find is this Dev Edition specific pref: > http://searchfox.org/mozilla-central/rev/ > 89e125b817c5d493babbc58ea526be970bd3748e/browser/components/preferences/in- > content-new/main.xul#287 Thanks for the pointer Nihanth!
Reporter | ||
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review178780 My apologies Thomas, I gave you the wrong information. It turns out that we can keep the third checkbox ("Warn you about unwanted and uncommon software") when `MOZILLA_OFFICIAL != 1`. Only the second one ("Block dangerous downloads") needs to be removed. The reason is twofold: 1. We need to remove `goog-unwanted-proto` from `urlclassifier.malwareTable` when "Warn you about unwanted and uncommon software" is unticked. 2. Flipping `browser.safebrowsing.downloads.remote.block_potentially_unwanted` and `browser.safebrowsing.downloads.remote.block_uncommon` when `browser.safebrowsing.downloads.enabled = false` doesn't do anything so it's perfectly safe to do. Your approach is totally fine, we just need to update the patch to only remove the second checkbox. Again, sorry for leading you astray. ::: browser/components/preferences/in-content-new/privacy.js:1105 (Diff revision 1) > - } else { > + } else { > - blockUncommonUnwanted.setAttribute("disabled", "true"); > + blockUncommonUnwanted.setAttribute("disabled", "true"); > - } > + } > - }); > + }); > > - blockUncommonUnwanted.addEventListener("command", function() { > + blockUncommonUnwanted.addEventListener("command", function() { We can do this also when `MOZILLA_OFFICIAL != 1`. ::: browser/components/preferences/in-content-new/privacy.js:1140 (Diff revision 1) > - blockDownloads.checked = blockDownloadsPref.value; > + blockDownloads.checked = blockDownloadsPref.value; > - if (!blockDownloadsPref.value) { > + if (!blockDownloadsPref.value) { > - blockUncommonUnwanted.setAttribute("disabled", "true"); > + blockUncommonUnwanted.setAttribute("disabled", "true"); > - } > + } > > - blockUncommonUnwanted.checked = blockUnwantedPref.value && blockUncommonPref.value; > + blockUncommonUnwanted.checked = blockUnwantedPref.value && blockUncommonPref.value; We should do this outside of the `if`. ::: browser/components/preferences/in-content-new/privacy.xul:739 (Diff revision 1) > +#ifdef MOZILLA_OFFICIAL > <vbox class="indent"> > <checkbox id="blockDownloads" > label="&blockDownloads.label;" > accesskey="&blockDownloads.accesskey;" /> > <checkbox id="blockUncommonUnwanted" We should keep this checkbox. ::: browser/components/preferences/in-content/security.js:202 (Diff revision 1) > - } else { > + } else { > - blockUncommonUnwanted.setAttribute("disabled", "true"); > + blockUncommonUnwanted.setAttribute("disabled", "true"); > - } > + } > - }); > + }); > > - blockUncommonUnwanted.addEventListener("command", function() { > + blockUncommonUnwanted.addEventListener("command", function() { We can do this also when `MOZILLA_OFFICIAL != 1`. ::: browser/components/preferences/in-content/security.js:237 (Diff revision 1) > - blockDownloads.checked = blockDownloadsPref.value; > + blockDownloads.checked = blockDownloadsPref.value; > - if (!blockDownloadsPref.value) { > + if (!blockDownloadsPref.value) { > - blockUncommonUnwanted.setAttribute("disabled", "true"); > + blockUncommonUnwanted.setAttribute("disabled", "true"); > - } > + } > > - blockUncommonUnwanted.checked = blockUnwantedPref.value && blockUncommonPref.value; > + blockUncommonUnwanted.checked = blockUnwantedPref.value && blockUncommonPref.value; We should do this outside of the `if`. ::: browser/components/preferences/in-content/security.xul:85 (Diff revision 1) > +#ifdef MOZILLA_OFFICIAL > <vbox class="indent"> > <checkbox id="blockDownloads" > label="&blockDownloads.label;" > accesskey="&blockDownloads.accesskey;" /> > <checkbox id="blockUncommonUnwanted" We should keep this checkbox.
Attachment #8901719 -
Flags: review?(francois) → review-
Reporter | ||
Updated•7 years ago
|
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•7 years ago
|
||
Updated the patch based on comment 10. Thanks Francois
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review178870 I just thought of something that might simplify the branching and reduce the size of the diff (which could make it easier to uplift). In the JS file, why not remove the checks against AppConstants and instead check whether or not the call to `document.getElementById("blockDownloads")` returned an element? So for example, this block: if (!AppConstants.MOZILLA_OFFICIAL) { enableSafeBrowsing.addEventListener("command", function() { safeBrowsingPhishingPref.value = enableSafeBrowsing.checked; safeBrowsingMalwarePref.value = enableSafeBrowsing.checked; if (enableSafeBrowsing.checked) { blockUncommonUnwanted.removeAttribute("disabled"); } else { blockUncommonUnwanted.setAttribute("disabled", "true"); } } else { ... } Would become something like: enableSafeBrowsing.addEventListener("command", function() { safeBrowsingPhishingPref.value = enableSafeBrowsing.checked; safeBrowsingMalwarePref.value = enableSafeBrowsing.checked; if (enableSafeBrowsing.checked) { if (blockDownloads) { blockDownloads.removeAttribute("disabled"); } blockUncommonUnwanted.removeAttribute("disabled"); } else { if (blockDownloads) { blockDownloads.removeAttribute("disabled"); } blockUncommonUnwanted.setAttribute("disabled", "true"); } Using the same pattern, we can remove all of runtime `AppConstants.MOZILLA_OFFICIAL` checks and instead rely on the `#ifdef` to remove the elements. What do you think?
Attachment #8901719 -
Flags: review?(francois) → review-
Assignee | ||
Comment 14•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review178870 Basically it's the same checking idea. We may use if (blockDownloads) instead of if (!AppConstants.MOZILLA_OFFICIAL). I think the different size of them does not have a noticeable impact. In the previous patch, I just moved all the UN_MOZILLA_OFFICE related changes into one if() condition (I am not a big fan of adding if (blockDownloads) everywhere). It looks better to trace (for me)
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review178870 And using if (!AppConstants.MOZILLA_OFFICIAL), for me, makes it clearer to understand why we have that change (although people can blame the code to see what happened :)), even at my first thought I intended to create a new function _initSafeBrowsing_non_official
Assignee | ||
Comment 16•7 years ago
|
||
MozReview-Commit-ID: 7crD3OcsmRm
Comment hidden (mozreview-request) |
Assignee | ||
Comment 18•7 years ago
|
||
Then, I updated the patch using your idea. People can blame the code to trace what happened anyway :)
Reporter | ||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review179194 r+ but you need to remove the pref patch from this before landing. ::: modules/libpref/init/all.js:5401 (Diff revision 3) > pref("browser.safebrowsing.id", "navclient-auto-ffox"); > #else > pref("browser.safebrowsing.id", "Firefox"); > #endif > > // Download protection You accidentally included the patch from bug 1394056 here.
Attachment #8901719 -
Flags: review?(francois) → review+
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds https://reviewboard.mozilla.org/r/173142/#review179324 ::: modules/libpref/init/all.js:5401 (Diff revision 3) > pref("browser.safebrowsing.id", "navclient-auto-ffox"); > #else > pref("browser.safebrowsing.id", "Firefox"); > #endif > > // Download protection Oops, thanks you. I removed it
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8902189 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 22•7 years ago
|
||
MozReview-Commit-ID: IZ7HhhBIrde Beta release patch, but need to wait for landing on m-c first
Comment 23•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/539b21e9b958 Disable download protection on non-official builds r=francois
Keywords: checkin-needed
Backed out for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=127131620&repo=autoland https://hg.mozilla.org/integration/autoland/rev/c0332f371039d2f10cd3a201caa5d9ce81c21805
Flags: needinfo?(tnguyen)
Assignee | ||
Comment 25•7 years ago
|
||
Thanks you for backing out, looks like the asan build does not include MOZILLA_OFFICIAL
Flags: needinfo?(tnguyen)
Comment 26•7 years ago
|
||
Pushed by kwierso@gmail.com: https://hg.mozilla.org/mozilla-central/rev/1575ef52aa22 Disable download protection on non-official builds r=francois
Comment hidden (mozreview-request) |
Assignee | ||
Comment 28•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=18987fa729941a3c1ec0c1a1cd280c1225496e9c
Comment hidden (mozreview-request) |
Assignee | ||
Comment 30•7 years ago
|
||
(In reply to Thomas Nguyen[:tnguyen] ni plz from comment #29) > Comment on attachment 8901719 [details] > Bug 1394053 - Disable download protection on non-official builds > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/173142/diff/5-6/ Rebased, fixed the test failure and re-pushed to try https://treeherder.mozilla.org/#/jobs?repo=try&revision=0bd1e9e998793d094afa2db926ba83aaa1ff7289
Comment hidden (mozreview-request) |
Assignee | ||
Comment 32•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b8c1addb7d0a207eaa6c356818a881224afcdef0
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Assignee | ||
Updated•7 years ago
|
Attachment #8902609 -
Attachment is obsolete: true
Assignee | ||
Comment 33•7 years ago
|
||
Patch for beta request
Comment 34•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s e62b89b4a1c0 -d e6189da47385: rebasing 417098:e62b89b4a1c0 "Bug 1394053 - Disable download protection on non-official builds r=francois" (tip) other [source] changed browser/components/preferences/in-content-new/privacy.js which local [dest] deleted use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u other [source] changed browser/components/preferences/in-content-new/privacy.xul which local [dest] deleted use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u other [source] changed browser/components/preferences/in-content/security.js which local [dest] deleted use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u other [source] changed browser/components/preferences/in-content/security.xul which local [dest] deleted use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u other [source] changed browser/components/preferences/in-content/tests/browser_security.js which local [dest] deleted use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u merging browser/components/preferences/in-content/tests/browser_security-1.js and browser/components/preferences/in-content-new/tests/browser_security-1.js to browser/components/preferences/in-content/tests/browser_security-1.js merging browser/components/preferences/in-content/tests/browser_security-2.js and browser/components/preferences/in-content-new/tests/browser_security-2.js to browser/components/preferences/in-content/tests/browser_security-2.js unresolved conflicts (see hg resolve, then hg rebase --continue)
Updated•7 years ago
|
Flags: needinfo?(tnguyen)
Keywords: checkin-needed
Assignee | ||
Comment 35•7 years ago
|
||
Did rebase but I don't see any diff from the last commit :(
Flags: needinfo?(tnguyen)
Assignee | ||
Comment 36•7 years ago
|
||
Well, I see bug 1349689 is still on progress to re-construct preference things on autoland but has not been landed on m-c yet. Let's wait for that bug to be completed
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment hidden (obsolete) |
Comment 39•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/05907bcce126 Disable download protection on non-official builds r=francois
Keywords: checkin-needed
Comment 40•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/05907bcce126
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 41•7 years ago
|
||
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds Since this is a front-end UI change, I think it requires at least a rubber-stamp from a peer of the Firefox module, in particular the preferences sub-module. If you have already talked out of band with someone from UX or engineering, you can indicate this by placing rs= in the commit message. Generally the idea seems good but since I didn't see any rubber-stamp indication, I'm flagging Jared to take a look.
Attachment #8901719 -
Flags: feedback?(jaws)
Comment 42•7 years ago
|
||
Comment on attachment 8901719 [details] Bug 1394053 - Disable download protection on non-official builds Nevermind the previous message, François is a Firefox peer. I used to know him for Platform work, and when looking up the list of peers using CTRL+F, only the entry for Firefox Accounts came up because it's written without the cedilla. Jared might still want to take a look as he is familiar with preferences.
Reporter | ||
Comment 43•7 years ago
|
||
Comment on attachment 8903056 [details] [diff] [review] Beta release - Disable download protection on non-official builds This patch is incomplete.
Attachment #8903056 -
Flags: approval-mozilla-beta?
Reporter | ||
Comment 44•7 years ago
|
||
This patch is incomplete, it removes the UI preferences for download protection without actually disabling it. The problem is that I made a mistake while reviewing revision 3: https://reviewboard.mozilla.org/r/173142/diff/3/ and lead Thomas to remove a part of the patch that was actually required. We need to actually disable browser.safebrowsing.downloads.enabled, just like we disabled goog-phish-proto in 1394056.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request) |
Comment 46•7 years ago
|
||
mozreview-review |
Comment on attachment 8904704 [details] Bug 1394053 - Disable download protection on non-official builds. https://reviewboard.mozilla.org/r/176492/#review181922
Attachment #8904704 -
Flags: review?(gpascutto) → review+
Comment 47•7 years ago
|
||
Pushed by fmarier@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/eb99a243bb9d Disable download protection on non-official builds. r=gcp
Updated•7 years ago
|
Attachment #8901719 -
Flags: feedback?(jaws) → feedback+
Reporter | ||
Comment 48•7 years ago
|
||
Thomas, could you please prepare a new Beta patch that combines our two patches?
Flags: needinfo?(tnguyen)
Assignee | ||
Comment 49•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8903056 -
Attachment is obsolete: true
Flags: needinfo?(tnguyen)
Reporter | ||
Comment 50•7 years ago
|
||
Comment on attachment 8905320 [details] [diff] [review] Disable download protection on non-official builds Review of attachment 8905320 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Can you please test it on beta with both official and non-official builds?
Attachment #8905320 -
Flags: review+
Assignee | ||
Comment 51•7 years ago
|
||
Comment on attachment 8905320 [details] [diff] [review] Disable download protection on non-official builds Rebuilt w/o MOZILLA_OFFICIAL and tested in my local machine Approval Request Comment [Feature/Bug causing the regression]: 1366920 [User impact if declined]: Update fails in many Linux distributions. [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: No. [Needs manual test from QE? If yes, steps to reproduce]: Go to about:preferences#privacy On non-offical build - Make sure "Block dangerous downloads" checkbox is hidden - Other checkboxes: "Warn you about unwanted and uncommon software" and "Block dangerous and deceptive content" should work as expected On official build - Make sure the change does not have any impact on official builf, all 3 checkboxes: "Block dangerous downloads", "Warn you about unwanted and uncommon software", "Block dangerous and deceptive content" should work as expected [List of other uplifts needed for the feature/fix]: No [Is the change risky?]: No [Why is the change risky/not risky?]: Only disable download protection on non-offical build. No impact on offical builds, [String changes made/needed]: No
Attachment #8905320 -
Flags: approval-mozilla-beta?
Comment 52•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/eb99a243bb9d
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
Comment 53•7 years ago
|
||
Comment on attachment 8905320 [details] [diff] [review] Disable download protection on non-official builds We don't want to ship this regression, let's uplift to beta 10 and verify it on beta.
Attachment #8905320 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 54•7 years ago
|
||
Andrei here is an issue for verification in beta after it lands. I don't think we need to verify it on both channels.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
Comment 55•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c2643f2df57e
Flags: in-testsuite+
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Version: unspecified → 56 Branch
Comment 56•7 years ago
|
||
Verified! It works. :) Step: 1. Download source code from Beta branch 2. Run ./mach build 3. Run ./mach run 4. Enter about:config in Url bar 5. Check the preference: browser.safebrowsing.downloads.enable 6. Enter preferences#privacy in Url bar 7. check the settings for Phishing protection Expected Result: - After step 5, browser.safebrowsing.downloads.enable is false - After Step 7, the checkbox "block dangerous downloads" is hidden Actual Result: - After step 5, browser.safebrowsing.downloads.enable is false - After Step 7, the checkbox "block dangerous downloads" is hidden OS: Windows 10 Pro Build info: 56.0b11 Hg log: changeset: 421677:6bd183fc6921
Status: RESOLVED → VERIFIED
Comment 57•7 years ago
|
||
Also can confirm that the fix has no impact on official builds: browser.safebrowsing.downloads.enabled is set to true, all 3 checkboxes: "Block dangerous downloads", "Warn you about unwanted and uncommon software", "Block dangerous and deceptive content" work as expected (run some smoke download protection and safe browsing testing). Verified on 56.0b11 build1 (20170911193316), using Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6.
Flags: needinfo?(andrei.vaida)
Comment 58•7 years ago
|
||
Glad to see this was fixed and verified. Thanks everyone for your effort! :)
Comment 59•7 years ago
|
||
I can confirm that the fix has no impact on official builds, I verified on Firefox 57.0b13 official build using Windows 8.1 x64, Mac 10.13 and Ubuntu 16.04 x86.
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•