Closed Bug 874408 Opened 11 years ago Closed 8 years ago

Revisit the Safe Browsing preferences

Categories

(Firefox :: Settings UI, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 48
Iteration:
48.3 - Apr 25
Tracking Status
firefox48 --- verified
relnote-firefox --- 48+

People

(Reporter: gcp, Assigned: johannh)

References

Details

(Whiteboard: [fxprivacy])

Attachments

(3 files, 8 obsolete files)

117.83 KB, image/png
Details
62.24 KB, image/png
Details
20.45 KB, patch
past
: review+
Details | Diff | Splinter Review
If we're cleaning out the settings UI to lower the amount of checkboxes, then we might as well deal with these:

Options->Security:

[x] Block reported attack sites
[x] Block reported forgeries

I can't really see a use case where you want to have one and not the other, especially as the distinction is very slim and fuzzy. On top of that, I'm not even sure the backend code works correctly with one but not both enabled.

Suggest to make one checkbox:

[x] Block reported forgeries and attack sites
Something shorter would be good. How about?

[x] Block known malicious sites
Product: Firefox → Toolkit
Other things to consider:

- We should rename "web forgeries" to "deceptive websites" (see bug 1245992).
- Allow download protection to be configured separately from browsing protection.
- Allow users to turn on/off protection for unwanted & uncommon software.

In other words,

[x] Block dangerous and deceptive sites
[x] Block dangerous downloads
[x] Warn me about unwanted and uncommon software
Component: Safe Browsing → Preferences
OS: Windows 7 → All
Product: Toolkit → Firefox
Summary: Remove split preference for attack site & forgeries → Revisit the Safe Browsing preferences
Whiteboard: [fxprivacy]
Version: 21 Branch → unspecified
Added [fxprivacy] since Paolo is working on some improvements related to Safe Browsing and it could be a good time to revisit our obsolete preferences.
Whiteboard: [fxprivacy] → [fxprivacy] [triage]
Flags: qe-verify?
Priority: -- → P3
Whiteboard: [fxprivacy] [triage] → [fxprivacy]
Priority: P3 → P4
(In reply to François Marier [:francois] from comment #2)
> - Allow download protection to be configured separately from browsing
> protection.

Why do you think that would be useful for users? Like gcp I can't think of any use case where I'd want one but not the other.

> - Allow users to turn on/off protection for unwanted & uncommon software.

Same here. Both options seem to go against the point of this bug (and Firefox UX in general), which is to reduce checkboxes.

I'd also note that Chrome only contains a single checkbox in its settings panel, although I'm not sure if it disables everything or just a few parts when unchecked. In our case it seems more appropriate to have an all-or-nothing toggle, in order to cater to the super-privacy-sensitive crowd.
Flags: qe-verify? → qe-verify+
(In reply to Panos Astithas [:past] from comment #5)
> (In reply to François Marier [:francois] from comment #2)
> > - Allow download protection to be configured separately from browsing
> > protection.
> 
> Why do you think that would be useful for users? Like gcp I can't think of
> any use case where I'd want one but not the other.

My comment predates our adding Application Reputation/Download Protection to Firefox. Application Reputation sends file metadata (including URLs) to a remote, third party service. It also adds a bunch of new categories to warn about.

This has much more severe privacy implications than the two options we had before (which would still be joined in what Francois proposed).

> > - Allow users to turn on/off protection for unwanted & uncommon software.
> 
> Same here. Both options seem to go against the point of this bug (and
> Firefox UX in general), which is to reduce checkboxes.

I think the idea here is that those might give more false positives? On the other hand, users who are bothered with having to unblock those will just turn Download Protection/Application Reputation off I presume.

> I'd also note that Chrome only contains a single checkbox in its settings
> panel, although I'm not sure if it disables everything or just a few parts
> when unchecked. In our case it seems more appropriate to have an
> all-or-nothing toggle, in order to cater to the super-privacy-sensitive
> crowd.

Our implementation is more mindful of privacy, which is why it is conceivable you might enable one part but not the other. 

On the other hand I agree people who are privacy minded will just want to turn this off entirely and won't even care about us being careful there.
(In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> Application Reputation sends file metadata (including URLs) to a
> remote, third party service. It also adds a bunch of new categories to warn
> about.
> 
> This has much more severe privacy implications than the two options we had
> before (which would still be joined in what Francois proposed).

Fair enough, but if it's the privacy implication we're concerned about, then we should indicate it in the text making the two options unmistakably different.

My own view is that the distinction is not going to be understood by non-experts, therefore this is going to be UI for a tiny slice of our user base, who might as well just use about:config or an add-on.

> > > - Allow users to turn on/off protection for unwanted & uncommon software.
> > 
> > Same here. Both options seem to go against the point of this bug (and
> > Firefox UX in general), which is to reduce checkboxes.
> 
> I think the idea here is that those might give more false positives? On the
> other hand, users who are bothered with having to unblock those will just
> turn Download Protection/Application Reputation off I presume.

Agreed, and I'd also note that we will provide an unblocking option in the primary UI for the occasional override.

> > I'd also note that Chrome only contains a single checkbox in its settings
> > panel, although I'm not sure if it disables everything or just a few parts
> > when unchecked. In our case it seems more appropriate to have an
> > all-or-nothing toggle, in order to cater to the super-privacy-sensitive
> > crowd.
> 
> Our implementation is more mindful of privacy, which is why it is
> conceivable you might enable one part but not the other. 
> 
> On the other hand I agree people who are privacy minded will just want to
> turn this off entirely and won't even care about us being careful there.

Good point, I could see us using an unchecked checkbox to mean "no additional security beyond the really critical bits", since the UI should prioritize the needs of the majority of our user base.
(In reply to Panos Astithas [:past] from comment #7)
> (In reply to Gian-Carlo Pascutto [:gcp] from comment #6)
> > Application Reputation sends file metadata (including URLs) to a
> > remote, third party service. It also adds a bunch of new categories to warn
> > about.
> > 
> > This has much more severe privacy implications than the two options we had
> > before (which would still be joined in what Francois proposed).
> 
> Fair enough, but if it's the privacy implication we're concerned about, then
> we should indicate it in the text making the two options unmistakably
> different.

That's exactly what I had in mind. I want users who are concerned about the privacy implications of download protection to be able to disable it without disabling Safe Browsing entirely.

> My own view is that the distinction is not going to be understood by
> non-experts, therefore this is going to be UI for a tiny slice of our user
> base, who might as well just use about:config or an add-on.

That's true, but aren't preferences already just for a tiny slice of users? My (possibly mistaken) understanding is that the vast majority of users don't ever change preferences or look at them.

> > > > - Allow users to turn on/off protection for unwanted & uncommon software.
> > > 
> > > Same here. Both options seem to go against the point of this bug (and
> > > Firefox UX in general), which is to reduce checkboxes.
> > 
> > I think the idea here is that those might give more false positives? On the
> > other hand, users who are bothered with having to unblock those will just
> > turn Download Protection/Application Reputation off I presume.
> 
> Agreed, and I'd also note that we will provide an unblocking option in the
> primary UI for the occasional override.

I'm not entirely convinced we need that one as a separate option but I wanted to make sure we considered it in this overhaul of the preferences because it is somewhat different from phishing and malware.

> > > I'd also note that Chrome only contains a single checkbox in its settings
> > > panel, although I'm not sure if it disables everything or just a few parts
> > > when unchecked. In our case it seems more appropriate to have an
> > > all-or-nothing toggle, in order to cater to the super-privacy-sensitive
> > > crowd.
> > 
> > Our implementation is more mindful of privacy, which is why it is
> > conceivable you might enable one part but not the other. 
> > 
> > On the other hand I agree people who are privacy minded will just want to
> > turn this off entirely and won't even care about us being careful there.
> 
> Good point, I could see us using an unchecked checkbox to mean "no
> additional security beyond the really critical bits", since the UI should
> prioritize the needs of the majority of our user base.

Chrome doesn't actually have the ability to control the parts of Safe Browsing that are enabled. We offer more privacy controls and it would be good to expose some of them as a differentiator. I don't think we should just do the same as Chrome in this case.
Assignee: nobody → jhofmann
Attachment #8742357 - Attachment is obsolete: true
Comment on attachment 8742360 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

I went ahead and implemented François version, please correct me if there wasn't consensus on any of this. Also feel free to make me change the text in case it's not perfect.

gcp, do you have time to take a look at the patch? I had to add a bit of JS boilerplate to set two prefs with a single checkbox, is there a better way to do it?

Because I removed the direct preference binding this also doesn't automatically update when preferences are changed in about:config, although that would probably be easy to add.
Attachment #8742360 - Flags: review?(gpascutto)
Comment on attachment 8742360 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8742360 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.js
@@ +149,5 @@
> +    let enableSafeBrowsing = document.getElementById("enableSafeBrowsing");
> +    let blockDangerous = document.getElementById("blockDangerous");
> +    let blockUncommonUnwanted = document.getElementById("blockUncommonUnwanted");
> +
> +    let safeBrowsingPref = document.getElementById("browser.safebrowsing.enabled");

Let's call this `safeBrowsingPhishingPref` since, despite the pref being badly named, that's what it actually means.
Comment on attachment 8742360 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8742360 [details] [diff] [review]:
-----------------------------------------------------------------

f? to francois again, see my remark regarding the prefs.

::: browser/components/preferences/in-content/security.js
@@ +153,5 @@
> +    let safeBrowsingPref = document.getElementById("browser.safebrowsing.enabled");
> +    let safeBrowsingMalwarePref = document.getElementById("browser.safebrowsing.malware.enabled");
> +
> +    let blockDangerousPref = document.getElementById("browser.safebrowsing.downloads.remote.block_dangerous");
> +    let blockDangerousHostPref = document.getElementById("browser.safebrowsing.downloads.remote.block_dangerous_host");

I think this isn't doing what Francois intended. If none of the b.sb.downloads.remote.* options is enabled, then these should be set to false as well:

browser.safebrowsing.downloads.enabled
browser.safebrowsing.downloads.remote.enabled

These control the list updates and the sending to the remote server. The remote.block_* options control what we do with the result of that. But the sending to the remote server is the privacy sensitive part.

::: browser/locales/en-US/chrome/browser/preferences/security.dtd
@@ +6,5 @@
>  
>  <!ENTITY  warnAddonInstall.label        "Warn me when sites try to install add-ons">
>  <!ENTITY  warnAddonInstall.accesskey    "W">
>  
> +<!ENTITY  enableSafeBrowsing.label        "Block dangerous and deceptive sites">

You eliminated the localization note here, but note the restriction on our use of these lists:
https://developers.google.com/safe-browsing/developers_guide_v2#AcceptableUsage

Which emphasize that the language should not imply a "definite" verdict.
Attachment #8742360 - Flags: review?(gpascutto)
Attachment #8742360 - Flags: review+
Attachment #8742360 - Flags: feedback?(francois)
Comment on attachment 8742360 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8742360 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> I think this isn't doing what Francois intended. If none of the
> b.sb.downloads.remote.* options is enabled, then these should be set to
> false as well:
> 
> browser.safebrowsing.downloads.enabled
> browser.safebrowsing.downloads.remote.enabled
> 
> These control the list updates and the sending to the remote server. The
> remote.block_* options control what we do with the result of that. But the
> sending to the remote server is the privacy sensitive part.

Good catch, I overlooked that part.

The preferences should do the following:

1. Block dangerous and deceptive sites:
     - browser.safebrowsing.enabled = true
     - browser.safebrowsing.malware.enabled = true

2. Block dangerous downloads:
     - browser.safebrowsing.downloads.enabled = true
     (no need to toggle anything else)

3. Warn me about unwanted and uncommon software:
     - browser.safebrowsing.downloads.remote.block_potentially_unwanted = true
     - browser.safebrowsing.downloads.remote.block_uncommon = true

Ideally that last one should also remove "test-unwanted-simple" and "goog-unwanted-simple" from "urlclassifier.malwareTable" when it's disabled. If it's too hard though, we can probably ignore this.

> You eliminated the localization note here, but note the restriction on our
> use of these lists:
> https://developers.google.com/safe-browsing/
> developers_guide_v2#AcceptableUsage
> 
> Which emphasize that the language should not imply a "definite" verdict.

For what it's worth, Chrome uses "Protect you and your device from dangerous sites" in their Settings UI now.
Attachment #8742360 - Flags: feedback?(francois) → feedback-
 > The preferences should do the following:
> 
> 1. Block dangerous and deceptive sites:
>      - browser.safebrowsing.enabled = true
>      - browser.safebrowsing.malware.enabled = true
> 
> 2. Block dangerous downloads:
>      - browser.safebrowsing.downloads.enabled = true
>      (no need to toggle anything else)
> 
> 3. Warn me about unwanted and uncommon software:
>      - browser.safebrowsing.downloads.remote.block_potentially_unwanted =
> true
>      - browser.safebrowsing.downloads.remote.block_uncommon = true
> 

But wouldn't unticking #2 make #3 impossible then? Should I gray out #3 in case #2 is unticked?
Flags: needinfo?(francois)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #13)
> Comment on attachment 8742360 [details] [diff] [review]
> Join safe browsing, add download protection to security prefs
> 
> Review of attachment 8742360 [details] [diff] [review]:
> 
> ::: browser/locales/en-US/chrome/browser/preferences/security.dtd
> @@ +6,5 @@
> >  
> >  <!ENTITY  warnAddonInstall.label        "Warn me when sites try to install add-ons">
> >  <!ENTITY  warnAddonInstall.accesskey    "W">
> >  
> > +<!ENTITY  enableSafeBrowsing.label        "Block dangerous and deceptive sites">
> 
> You eliminated the localization note here, but note the restriction on our
> use of these lists:
> https://developers.google.com/safe-browsing/
> developers_guide_v2#AcceptableUsage
> 
> Which emphasize that the language should not imply a "definite" verdict.

Oh, I didn't know this existed. :) I am very skeptical if our current wording is in line with these policies, however. Who's the right person to look over that?
(In reply to Johann Hofmann [:johannh] from comment #15)
> But wouldn't unticking #2 make #3 impossible then? Should I gray out #3 in
> case #2 is unticked?

Yes to both.

>Oh, I didn't know this existed. :) I am very skeptical if our current wording is in line with these policies, however. Who's the right person to look over that?

Francois is in regular contact with the respective Google team, so he can help there. The observation about Chrome's language seems to suggest this isn't a big concern as far as the settings checkboxes are concerned, so your proposal is probably fine.
Attachment #8742360 - Attachment is obsolete: true
Attachment #8743231 - Flags: review?(gpascutto)
Would be cool if the two of you could give it another look :)
(In reply to Gian-Carlo Pascutto [:gcp] from comment #17)
> Francois is in regular contact with the respective Google team, so he can
> help there. The observation about Chrome's language seems to suggest this
> isn't a big concern as far as the settings checkboxes are concerned, so your
> proposal is probably fine.

Like gcp, I would assume that your wording is fine. I'll show them a screenshot next time I meet with them just to make sure they're not surprised by it, but I wouldn't expect them to ask for changes.
(In reply to Johann Hofmann [:johannh] from comment #15)
>  > The preferences should do the following:
> > 
> > 1. Block dangerous and deceptive sites:
> >      - browser.safebrowsing.enabled = true
> >      - browser.safebrowsing.malware.enabled = true
> > 
> > 2. Block dangerous downloads:
> >      - browser.safebrowsing.downloads.enabled = true
> >      (no need to toggle anything else)
> > 
> > 3. Warn me about unwanted and uncommon software:
> >      - browser.safebrowsing.downloads.remote.block_potentially_unwanted =
> > true
> >      - browser.safebrowsing.downloads.remote.block_uncommon = true
> > 
> 
> But wouldn't unticking #2 make #3 impossible then? Should I gray out #3 in
> case #2 is unticked?

Yes, I should have mentioned that:

- If Option #1 is unticked, gray out #2 and #3.
- If Option #2 is unticked, gray out #3.
Flags: needinfo?(francois)
Comment on attachment 8743231 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8743231 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to François Marier [:francois] from comment #21)
> Yes, I should have mentioned that:
> 
> - If Option #1 is unticked, gray out #2 and #3.
> - If Option #2 is unticked, gray out #3.

Actually, I take that back. Since you are editing urlclassifier.malwareTable, Option 3 is actually relevant even when Option 2 is unticked.

All we need to do I think is:

- If Option #1 is unticked, gray out #2 and #3.

::: browser/components/preferences/in-content/security.js
@@ +146,5 @@
>    },
>  
> +  _initSafeBrowsing() {
> +    let enableSafeBrowsing = document.getElementById("enableSafeBrowsing");
> +    let blockDangerous = document.getElementById("blockDangerous");

Perhaps that should be renamed to `blockDownloads`?

@@ +183,5 @@
> +
> +      if (blockUncommonUnwanted.checked) {
> +        malware.push("goog-unwanted-simple");
> +        malware.push("test-unwanted-simple");
> +      }

This is great!

One thing I'll note is that if you turn it on and off again, it will change the pref from the default of "" to "" which would needlessly mess with future pref updates.

How about updating "urlclassifier.malwareTable" to "goog-malware-shavar,test-malware-simple,goog-unwanted-shavar,test-unwanted-simple" in
 https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/modules/libpref/init/all.js#4960 as part of this patch too, to mitigate this potential issue?
(In reply to François Marier [:francois] from comment #22)
> One thing I'll note is that if you turn it on and off again, it will change
> the pref from the default of "" to "" which would needlessly mess with
> future pref updates.

That should have read: from the default of "goog-malware-shavar,goog-unwanted-shavar,test-malware-simple,test-unwanted-simple" to "goog-malware-shavar,test-malware-simple,goog-unwanted-shavar,test-unwanted-simple"
(In reply to François Marier [:francois] from comment #22)
> Comment on attachment 8743231 [details] [diff] [review]
> Join safe browsing, add download protection to security prefs
> 
> Review of attachment 8743231 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to François Marier [:francois] from comment #21)
> > Yes, I should have mentioned that:
> > 
> > - If Option #1 is unticked, gray out #2 and #3.
> > - If Option #2 is unticked, gray out #3.
> 
> Actually, I take that back. Since you are editing
> urlclassifier.malwareTable, Option 3 is actually relevant even when Option 2
> is unticked.
> 
> All we need to do I think is:
> 
> - If Option #1 is unticked, gray out #2 and #3.
> 

Even though #3 is changing stuff unrelated to #2, it will not work correctly if #2 is not ticked (afaik), so I'd say we go with your initial proposal.
Also just noting, it might confuse a lot of people that safe browsing (#1) and safe downloads (#2) are so tightly coupled you can't use #2 without #1.
Attachment #8743231 - Attachment is obsolete: true
Attachment #8743231 - Flags: review?(gpascutto)
Comment on attachment 8743723 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

This incorporates all the latest suggestions, except for francois' last point (see my comment), I'll add the hopefully last review flag once we get that sorted out :)

Thanks!
Attachment #8743723 - Flags: feedback?(gpascutto)
Attachment #8743723 - Flags: feedback?(francois)
Comment on attachment 8743231 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8743231 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.js
@@ +286,5 @@
>      gSubDialog.open("chrome://passwordmgr/content/passwordManager.xul");
> +  },
> +
> +  updateBlockUncommonUnwantedDownloads() {
> +    let checkbox = document.getElementById("blockUncommonUnwantedDownloads");

This appears to turn on SafeBrowsing (i.e. safeBrowsingPhishingPref.value and safeBrowsingMalwarePref.value) if the checkbox for UncommonUnwanted gets checked, but it's not possible to check it (because the checkbox would be disabled) unless they were already enabled.

Maybe I'm reading the logic wrong.
Attachment #8743231 - Attachment is obsolete: false
Comment on attachment 8743231 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

My bad, I had this review open but didn't finish it yesterday.
Attachment #8743231 - Attachment is obsolete: true
Comment on attachment 8743723 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8743723 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/preferences/in-content/security.js
@@ +301,5 @@
> +  updateBlockUncommonUnwantedDownloads() {
> +    let checkbox = document.getElementById("blockUncommonUnwantedDownloads");
> +
> +    safeBrowsingEnabled.value = checkbox.checked;
> +    safeBrowsingMalwareEnabled.value = checkbox.checked;

As said, this looks suspicious to me.

::: modules/libpref/init/all.js
@@ +4958,5 @@
>  pref("dom.mapped_arraybuffer.enabled", false);
>  
>  #ifdef MOZ_SAFE_BROWSING
>  // The tables used for Safebrowsing phishing and malware checks.
> +pref("urlclassifier.malwareTable", "goog-malware-shavar,test-malware-simple,goog-unwanted-shavar,test-unwanted-simple");

Does it make sense to just enforce sorting the names in this pref, to avoid the problem Francois talked about? That seems more sensible than a magic ordering.
Attachment #8743723 - Flags: feedback?(gpascutto) → feedback+
(In reply to Gian-Carlo Pascutto [:gcp] from comment #29)
> Comment on attachment 8743231 [details] [diff] [review]
> Join safe browsing, add download protection to security prefs
> 
> My bad, I had this review open but didn't finish it yesterday.

No wait you're right, that part somehow checked itself in although I deleted it. What the heck. I'll remove it, sorry!
Attachment #8743723 - Attachment is obsolete: true
Attachment #8743723 - Flags: feedback?(francois)
Comment on attachment 8743867 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Ok, so this version should be exorcised from any spirits of the past that plagued it before.

> Does it make sense to just enforce sorting the names in this pref, to avoid the problem Francois talked about? That seems more sensible than a magic ordering.

I can put an Array.sort into the function if that's what you mean. Should be fine and I don't think there are any performance implications we should worry about.
Attachment #8743867 - Flags: feedback?(francois)
Comment on attachment 8743723 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8743723 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/locales/en-US/chrome/browser/preferences/security.dtd
@@ +13,2 @@
>  -->
> +<!ENTITY  enableSafeBrowsing.label        "Block dangerous and deceptive sites">

You said that it might be unexpected that this option about "sites" also turns off the one about "downloads". So maybe we just need to make this description bigger: "Block dangerous and deceptive content". Or "Keep me safe on the internet".
Attachment #8743723 - Attachment is obsolete: false
(In reply to Gian-Carlo Pascutto [:gcp] from comment #34)
> Comment on attachment 8743723 [details] [diff] [review]
> Join safe browsing, add download protection to security prefs
> 
> Review of attachment 8743723 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/locales/en-US/chrome/browser/preferences/security.dtd
> @@ +13,2 @@
> >  -->
> > +<!ENTITY  enableSafeBrowsing.label        "Block dangerous and deceptive sites">
> 
> You said that it might be unexpected that this option about "sites" also
> turns off the one about "downloads". So maybe we just need to make this
> description bigger: "Block dangerous and deceptive content". Or "Keep me
> safe on the internet".

Mmh although I like the sound of "Keep me safe on the internet", "Block dangerous and deceptive content" might be the more sensible choice here.
Attachment #8743723 - Attachment is obsolete: true
Attachment #8743867 - Attachment is obsolete: true
Attachment #8743867 - Flags: feedback?(francois)
Sorry francois for the feedback? spam. I'll not reapply the flag, I suppose you know that I'd appreciate your opinion anyway ;)
Driveby comment:

The text in about:rights describes how to disable these options with the current strings (about:rights -> click "Service Terms" link -> click "here" link in "Instructions on how to disable a particular feature or Service may be found here.").

That text should be updated to reflect the new UI. Please file a legal bug blocking this one for getting signoff on updated about:rights text. (That should be a trivial process for an obvious change, so don't fear it. :)
(In reply to Johann Hofmann [:johannh] from comment #24)
> (In reply to François Marier [:francois] from comment #22)
> > All we need to do I think is:
> > 
> > - If Option #1 is unticked, gray out #2 and #3.
> > 
> 
> Even though #3 is changing stuff unrelated to #2, it will not work correctly
> if #2 is not ticked (afaik), so I'd say we go with your initial proposal.

Option #3 will have an effect (adding/removing unwanted software sites in the browsing protection part of Safe Browsing) even when download protection is off.

That said, it's a bit of an edge case, so I think your simplification is fine.
Comment on attachment 8743885 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8743885 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #8743885 - Flags: review+
Attachment #8743885 - Attachment is obsolete: true
I opened the legal bug, thanks Dolske for driving by :)

We talked about it and agreed that it might be sufficient to only adjust the name of the checkbox in about:legal, since that checkbox is the only the thing user needs to completely disable safe browsing, including download protection.
Attachment #8744266 - Attachment is obsolete: true
Attached image Settings pyramid
Attachment #8744382 - Attachment description: Screen Shot 2016-04-22 at 18.07.32.png → Settings pyramid
Attached image Non-pyramidy settings
Based on feedback in our meeting I added an indent to the two download protection settings. I didn't go for the full pyramid (see screenshots) because that looked a bit ridiculous to me.
(In reply to Johann Hofmann [:johannh] from comment #45)
> Created attachment 8744383 [details]
> Non-pyramidy settings

I agree that the pyramid looked pretty silly :)

Interestingly, the single level of indentation actually matches what I was suggesting in comment 39. Do you think it would make sense to only gray out #2 and #3 when #1 is disabled and make #2 and #3 independent of one another?

That makes sense internally but the layout also seems to suggest that this is how it works.
(In reply to François Marier [:francois] from comment #48)
> (In reply to Johann Hofmann [:johannh] from comment #45)
> > Created attachment 8744383 [details]
> > Non-pyramidy settings
> 
> I agree that the pyramid looked pretty silly :)
> 
> Interestingly, the single level of indentation actually matches what I was
> suggesting in comment 39. Do you think it would make sense to only gray out
> #2 and #3 when #1 is disabled and make #2 and #3 independent of one another?
> 
> That makes sense internally but the layout also seems to suggest that this
> is how it works.

I think it would indeed make sense, but #3 can currently not work on its own, at least not when I tried it. :/ 

If #2 (safebrowsing.downloads.enabled) is disabled, users will not see a warning for unknown downloads despite having #3 (safebrowsing.downloads.remote.block_potentially_unwanted) explicitly turned on. That was actually the reason why my very first patch didn't set the safebrowsing.downloads.enabled preference, to be able to control the type of warning that is shown more granularly.

I agree that this layout is slightly confusing, but for now I'd say it's the lesser evil. :)

We might consider changing the way the preference system for safebrowsing.downloads works.
(In reply to Johann Hofmann [:johannh] from comment #49)
> I think it would indeed make sense, but #3 can currently not work on its
> own, at least not when I tried it. :/ 
> 
> If #2 (safebrowsing.downloads.enabled) is disabled, users will not see a
> warning for unknown downloads despite having #3
> (safebrowsing.downloads.remote.block_potentially_unwanted) explicitly turned
> on.

You're right, they won't see warnings for unwanted software _downloads_ but they will see warnings for unwanted software _sites_ like http://itisatrap.org/firefox/unwanted.html.
Comment on attachment 8744381 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8744381 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/locales/en-US/chrome/global/aboutRights.dtd
@@ +62,5 @@
>  <!ENTITY rights.safebrowsing-a "SafeBrowsing: ">
>  <!ENTITY rights.safebrowsing-b "Disabling the Safe Browsing feature is not recommended as it may result in you going to unsafe sites.  If you wish to disable the feature completely, follow these steps:">
>  <!ENTITY rights.safebrowsing-term1 "Open the application preferences">
>  <!ENTITY rights.safebrowsing-term2 "Select the Security selection">
> +<!ENTITY rights.safebrowsing-term3 "Uncheck the option to &quot;&enableSafeBrowsing.label;&quot;">

We always change the key when modifying the value so that l10n tools can pick up the change. In this case you could make it rights2.safeBrowsing-term3.
(In reply to François Marier [:francois] from comment #50)
> (In reply to Johann Hofmann [:johannh] from comment #49)
> > I think it would indeed make sense, but #3 can currently not work on its
> > own, at least not when I tried it. :/ 
> > 
> > If #2 (safebrowsing.downloads.enabled) is disabled, users will not see a
> > warning for unknown downloads despite having #3
> > (safebrowsing.downloads.remote.block_potentially_unwanted) explicitly turned
> > on.
> 
> You're right, they won't see warnings for unwanted software _downloads_ but
> they will see warnings for unwanted software _sites_ like
> http://itisatrap.org/firefox/unwanted.html.

Fair enough, but you still have the problem that we'd be giving out false information to the user. We're claiming that uncommon downloads are detected when they are really not. Maybe I'm thinking this too far but if a user ticks this and trusts that we would warn e.g. on downloading .exe files and we really don't the best outcome is merely an unhappy thread on SUMO.

Maybe we could add another setting for turning on only warnings for websites? Sounds like a followup though. :)
Attachment #8744381 - Attachment is obsolete: true
Comment on attachment 8744589 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Panos, I changed the key, if you still want to land this for 48 :)

In that case, could you give it another quick r? There were only minor changes after gcp's r+, but I'd like to be sure.

(François, I'm assuming our conversation does not block this bug, I hope that's ok) :)
Attachment #8744589 - Flags: review?(past)
Thanks for doing this, but your patch highlights another problem: we are switching the text in about:rights on Android, but have we modified the preferences screen there accordingly?
(In reply to Panos Astithas [:past] from comment #55)
> Thanks for doing this, but your patch highlights another problem: we are
> switching the text in about:rights on Android, but have we modified the
> preferences screen there accordingly?

The about:rights section in Android is actually commented out, we might as well remove it.

"Safe Browsing cannot be disabled in Firefox Mobile; these instructions show how to do it on desktop"
Comment on attachment 8744589 [details] [diff] [review]
Join safe browsing, add download protection to security prefs

Review of attachment 8744589 [details] [diff] [review]:
-----------------------------------------------------------------

Great then, I just verified on an Android device that there is nothing else to do here. Let's land this!
Attachment #8744589 - Flags: review?(past) → review+
https://hg.mozilla.org/mozilla-central/rev/2df96c0510da
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
See Also: → 1267236
Blocks: 1216897
Iteration: --- → 48.3 - Apr 25
Priority: P4 → P1
QA Contact: paul.silaghi
Release Note Request (optional, but appreciated)
[Why is this notable]: Firefox 48 will ship with more protections against malware downloads (blocking potentially unwanted software and uncommon downloads by default, while additionally providing UI to unblock the downloads) and this change surfaces a user-facing control to toggle this behavior. We should only mention all these changes in one bullet point, so this seems as good as any.
[Suggested wording]: Enhanced protection against malware downloads
[Links (documentation, blog post, etc)]: I believe the platform security team is going to blog about this
relnote-firefox: --- → ?
See Also: → 1267574
A more thorough description of the changes in malware detection would include these points:

* Enabled blocking Potentially Unwanted Software and Uncommon Software categories (many times more blocks than what we had before).
* UI additions to allow a blocked download to proceed from primary UI with a few clicks.
* Added icon badges to the downloads button (or menu button when the downloads button is hidden there) when malware downloads are detected.
* Updated the about:preferences section that deals with the Safe Browsing service to provide users with more control over their experience.
A few issues:
1. Check all the 3 options, uncheck option #1 => options #2, #3 grayed out, but in about:config the prefs are still TRUE
2. Check all the 3 options, uncheck option #2 => option #3 grayed out, but in about:config the prefs are still TRUE
3. Check all the 3 options, uncheck option #2 => option #3 grayed out.
Then uncheck/recheck option #1 => option #3 enabled, even if option #2 is disabled
Flags: needinfo?(jhofmann)
Yes, as far as I understand (and also in my opinion) this is correct UX. The pref that option #1 triggers will invalidate prefs #2 and #3 no matter their state. Same with #2 making pref #3 useless no matter its state.

When the user decides to check #1 or #2 again they find their dependent prefs in the same state that they were in before checking #1 or #2. That's the reason we grey them out, to signify "this is not working right now but if you enable the parent option it will still be in the same state".
Flags: needinfo?(jhofmann)
Mmh there are actually different precedent cases in the preferences, some do it like in this patch and some also disable the child option. We should have some kind of guideline for this, I suppose.
(In reply to Johann Hofmann [:johannh] from comment #64)
> some also disable the child option
I'm talking about the correspondent prefs in about:config, and not necessarily the UI.
There shouldn't be a difference if an option is (checked and grayed out) vs (unchecked and grayed out).
(In reply to Paul Silaghi, QA [:pauly] from comment #65)
> (In reply to Johann Hofmann [:johannh] from comment #64)
> > some also disable the child option
> I'm talking about the correspondent prefs in about:config, and not
> necessarily the UI.
> There shouldn't be a difference if an option is (checked and grayed out) vs
> (unchecked and grayed out).

Here's an example of a setting that does this even worse: If you go to Search -> Provide search suggestions -> Show search suggestions in location bar results it will toggle the pref "browser.urlbar.suggest.searches" to true

Now untoggle "Provide search suggestions" and "Show search suggestions in location bar results" will be shown as unchecked as disabled even though the preference is still set. I'd say the pref behavior is ok, but it needs to signify that it's still toggled on (but disabled) in the UI. Maybe we should file a bug about that?

A setting that does it exactly like this patch is Advanced -> Update -> Automatically install updates (recommended: improved security) -> Warn me if this will disable any of my add-ons

where the pref that is toggled is app.update.mode

So yes, I think there should be a difference between (checked and grayed out) and (unchecked and grayed out), since there is already an implicit difference that the user would not perceive otherwise.
It sounds like we have not been very consistent in about:preferences, so a followup bug is probably warranted. But I would like Jared's opinion on how to handle this specific case. Should we make safe browsing prefs behave like updates or more like search suggestions?
Flags: needinfo?(jaws)
(In reply to Paul Silaghi, QA [:pauly] from comment #62)
> A few issues:
> 1. Check all the 3 options, uncheck option #1 => options #2, #3 grayed out,
> but in about:config the prefs are still TRUE
> 2. Check all the 3 options, uncheck option #2 => option #3 grayed out, but
> in about:config the prefs are still TRUE

I agree with what Johann said in comment 63: the prefs we toggle override the dependent ones so we shouldn't needlessly overwrite the prefs that a user (or an add-on) may have tweaked.

> 3. Check all the 3 options, uncheck option #2 => option #3 grayed out.
> Then uncheck/recheck option #1 => option #3 enabled, even if option #2 is
> disabled

That, on the other hand looks like a small bug in the graying logic. If we do gray out #3 when #2 is unchecked (which could change in bug 1267236), then toggling #1 should not reset that.
(In reply to François Marier [:francois] from comment #68)
> 
> > 3. Check all the 3 options, uncheck option #2 => option #3 grayed out.
> > Then uncheck/recheck option #1 => option #3 enabled, even if option #2 is
> > disabled
> 
> That, on the other hand looks like a small bug in the graying logic. If we
> do gray out #3 when #2 is unchecked (which could change in bug 1267236),
> then toggling #1 should not reset that.

Oh yes, I seem to have missed that. What's the default procedure for this? Make a new bug?
(In reply to François Marier [:francois] from comment #68)
> I agree with what Johann said in comment 63: the prefs we toggle override
> the dependent ones so we shouldn't needlessly overwrite the prefs that a
> user (or an add-on) may have tweaked.
Why do we want a pyramid model at all then?
Let's say I want to disable all the protections. Don't you find a little bizarre that, in order to do that, I need to uncheck pref #3, #2, #1, in exactly this order, because in any other order, some of them will still be active?

(In reply to Johann Hofmann [:johannh] from comment #69)
> Oh yes, I seem to have missed that. What's the default procedure for this?
> Make a new bug?
I'll file a follow-up ASAP.
(In reply to Paul Silaghi, QA [:pauly] from comment #70)
> (In reply to François Marier [:francois] from comment #68)
> > I agree with what Johann said in comment 63: the prefs we toggle override
> > the dependent ones so we shouldn't needlessly overwrite the prefs that a
> > user (or an add-on) may have tweaked.
> Why do we want a pyramid model at all then?
> Let's say I want to disable all the protections. Don't you find a little
> bizarre that, in order to do that, I need to uncheck pref #3, #2, #1, in
> exactly this order, because in any other order, some of them will still be
> active?

This is a common pattern that we apply in a lot of other places in the settings as well (see above). My previous statement that the precedent in settings is mixed was actually confused by the bug around Search. From playing around with the settings a bit more I can gather that this is actually the default pattern for *all* child settings.

Do you know any cases where child/dependent settings are cleared by unchecking their parent or any indication that this is better UX? If so, we will need to change this in many other settings than this one. 

The pyramid model is still valid, since the setting that users made will no longer be active if the parent is disabled. Not because the pref is removed, but because a "higher-level" pref that the parent setting has set makes it so. :)
I'm pretty sure it's standard platform UX at least on Windows and Mac OS X to just grey out settings (potentially checked/enabled) if the parent setting is disabled, and have this convey "everything is disabled due to the parent".

e.g. http://www.designinginterfaces.com/firstedition/Patterns/Responsive_Enabling/sysprefs.gif

That being said, our "grey" is so close to black that I almost can't see the difference between disabled and active state. And that's on a *good* screen...
Added to the release notes with your (great) wording
Depends on: 1268848
Panos, thanks for flagging me. This is indeed a very confusing UI and should be restructured in Firefox 48.

The prefs are set up like so:
A
  AB
  AC

But since AC actually is dependent on AB, AC should be indented below AB like so:
A
  AB
    AC

Visually this isn't seen often though, and is probably the reason why it wasn't created this way in the first place. Do we really need this much fine-grained control over these features?

Who is going to enable "Block dangerous and deceptive content" but also disable "Warn me about unwanted and uncommon software"? What does it mean to "warn" about unwanted and uncommon software? Is a warning some sort of temporary block? Is that an interstitial page? If it is basically another form of blocking, then we can remove the "warn me about unwanted ..." checkbox and just include that functionality with the "Block dangerous and deceptive content".
Flags: needinfo?(past)
Flags: needinfo?(jhofmann)
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #74)
> Who is going to enable "Block dangerous and deceptive content" but also
> disable "Warn me about unwanted and uncommon software"?

The assumption here is that "unwanted" and "uncommon" contain more false positive and also involve a policy decision by Google (https://www.google.com/intl/en/about/company/unwanted-software-policy.html). Whether or not that's a distinction we need to expose to users is a good question, but this would be the reason to enable one and not the other.

> What does it mean to "warn" about unwanted and uncommon software? Is a warning
> some sort of temporary block? Is that an interstitial page?

It's two things:

- a much softer version of the download blocking for unwanted and uncommon downloads
- a new interstitial page when visiting sites that host potentially unwanted software
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #74)
> Do we really need this much fine-grained control over these features?

Comment 8 makes a few compelling arguments about not simplifying away areas we're trying to differentiate our product on.
I'm marking this bug as verified fixed on 48.0a2 (2016-05-04) based on comments 62 and below.
Status: RESOLVED → VERIFIED
I believe comment 75 and comment 76 provide the necessary background here. FWIW my opinion is that it's fine to not make any further changes in followup bugs, as the potential for confusion is limited to a very extreme case (but kudos to pauly for discovering it!) that is already of interest to a tiny minority of our users.
Flags: needinfo?(past)
I agree with Panos and everyone else in favor of keeping the three options. It feels like the right amount of granular control. The potential for confusion would be even less if we had the "pyramid" layout, but that would just look ugly IMO. We have Bug 1267236 to deal with the settings not only affecting downloads, but that's the only followup I think we need right now.
Flags: needinfo?(jhofmann)
i'm not sure if this is the right place to inquire about this (other candidates include bug 1111741 and bug 974579), but i'm wondering why download signature verification is disabled on Linux and Mac? Since Linux and Mac do download malware detection on Windows executables since FF 39, why don't they also check the signatures for Windows executables so that lookups don't need to be done for known good publishers (reducing privacy impact)?
Flags: needinfo?(jhofmann)
I don't think this bug is the right thread for that, you might want to ask :gcp or :francois in the #security IRC channel before discussing this in Bugzilla :)

https://wiki.mozilla.org/IRC
Flags: needinfo?(jhofmann)
>Since Linux and Mac do download malware detection on Windows executables since FF 39, why don't they also check the signatures for Windows executables

Because extracting the signatures from a Windows binary is done using Windows (Authenticode) APIs:

https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/netwerk/base/BackgroundFileSaver.cpp#733
https://dxr.mozilla.org/mozilla-central/rev/b69a5bbb5e40bd426e35222baa600b481e50d265/netwerk/base/BackgroundFileSaver.cpp#825
thanks, johannh and gcp -- moved to bug 1283475
You need to log in before you can comment on or make changes to this bug.