Closed Bug 1394053 Opened 2 years ago Closed 2 years ago

Having download protection enabled on non-official builds breaks all of Safe Browsing

Categories

(Toolkit :: Safe Browsing, defect, P1)

56 Branch
defect

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)

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
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.
Summary: Disable download protection on non-official builds → Having download protection enabled on non-official builds breaks all of Safe Browsing
Blocks: 1366920
Keywords: regression
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: nobody → tnguyen
(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)
Track 56+ as regression.
(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?
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?
(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.
(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!
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-
Status: NEW → ASSIGNED
Updated the patch based on comment 10. Thanks Francois
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-
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)
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
MozReview-Commit-ID: 7crD3OcsmRm
Then, I updated the patch using your idea. People can blame the code to trace what happened anyway :)
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+
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
Attachment #8902189 - Attachment is obsolete: true
Keywords: checkin-needed
MozReview-Commit-ID: IZ7HhhBIrde


Beta release patch, but need to wait for landing on m-c first
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
Thanks you for backing out, looks like the asan build does not include MOZILLA_OFFICIAL
Flags: needinfo?(tnguyen)
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/1575ef52aa22
Disable download protection on non-official builds r=francois
(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
Keywords: checkin-needed
Attachment #8902609 - Attachment is obsolete: true
Patch for beta request
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)
Flags: needinfo?(tnguyen)
Keywords: checkin-needed
Did rebase but I don't see any diff from the last commit :(
Flags: needinfo?(tnguyen)
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
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/05907bcce126
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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 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.
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?
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 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+
Pushed by fmarier@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb99a243bb9d
Disable download protection on non-official builds. r=gcp
Thomas, could you please prepare a new Beta patch that combines our two patches?
Flags: needinfo?(tnguyen)
Attachment #8903056 - Attachment is obsolete: true
Flags: needinfo?(tnguyen)
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+
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?
https://hg.mozilla.org/mozilla-central/rev/eb99a243bb9d
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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+
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)
Version: unspecified → 56 Branch
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
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)
Glad to see this was fixed and verified.
Thanks everyone for your effort!  :)
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.