The main use case for this new type of Safe Browsing list is a "parental mode" which allows parents to prevent their children from accessing certain websites. It will work like the malware and phishing lists except that there will be no way for users to click through the warning.

gcp, does this look like a reasonable approach to you? It will be the basis of the family-friendly blocking code in Fennec.

Matej, I went for some pretty basic copy here since I didn't have much to say about this generic blocking of content. I welcome any feedback you may have. The two other strings included in this new feature are: - "The site at %S is currently forbidden by your browser configuration and has been blocked." - "The browser prevented this page from loading because it is configured to block it." The wording is different in an effort to match the wording of other strings in the same context.

Comment on attachment 8688764 [details] [diff] [review] Initial working patch Review of attachment 8688764 [details] [diff] [review]: ----------------------------------------------------------------- Some considerations: 1) As the number of blocklists increases, there's quite a bit of code that could use cleanup instead of copy-pastaing on more cases. 2) "ffb" should probably just be named forbidden or something. 3) Are you only going to have a single blocklist? I imagine that in reality you're going to have age categories, and considerations that the "family-friendliness" of some subjects like sex and violence is going to be very locale dependent. But I'm not sure that matters for this code, as we can swap in different lists with prefs. That would mean we commit to serve the cross-product of every possible blocklist you would want, though. ::: browser/base/content/blockedSite.xhtml @@ +168,5 @@ > + > + /** > + * Initialize custom strings and functionality for blocked forbidden case > + */ > + function initPage_forbidden() We're accumulating technical debt here due to the way these test pages are constructed. ::: browser/base/content/browser.js @@ +3018,5 @@ > bucketName += isTopFrame ? "TOP_" : "FRAME_"; > switch (elementId) { > case "getMeOutButton": > + if (sendTelemetry) { > + secHistogram.add(nsISecTel[bucketName + "GET_ME_OUT_OF_HERE"]); So the bucket is empty for this new category? Looks strange. ::: modules/libpref/init/all.js @@ +4835,5 @@ > pref("urlclassifier.trackingTable", "test-track-simple,mozstd-track-digest256"); > pref("urlclassifier.trackingWhitelistTable", "test-trackwhite-simple,mozstd-trackwhite-digest256"); > > +// The table and global pref for the family-friendly browsing feature > +pref("browser.safebrowsing.ffb.enabled", false); ffb isn't a great name for that pref.

Would being able to swap the phishing and malware lists into the forbidden pref solve the CESG-GCHQ thing being discussed on dev.security?

(In reply to François Marier [:francois] from comment #2) > Created attachment 8688770 [details] > forbidden_error_page.png > > Matej, I went for some pretty basic copy here since I didn't have much to > say about this generic blocking of content. I welcome any feedback you may > have. The copy in the attachment looks good, though "Web" should be capitalized. > The two other strings included in this new feature are: > > - "The site at %S is currently forbidden by your browser configuration and > has been blocked." It sounds odd to me to use "forbidden," but then talk about configuration, which makes it seem like something the user has done. Maybe it could just be: "The site at %S has been blocked by your browser configuration." > - "The browser prevented this page from loading because it is configured to > block it." This looks good to me. Thanks.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #3) > 1) As the number of blocklists increases, there's quite a bit of code that > could use cleanup instead of copy-pastaing on more cases. Indeed. I've bitten the bullet and refactored this. > 2) "ffb" should probably just be named forbidden or something. I've changed it to browser.safebrowsing.forbidden.enabled but that's not great either because "enabled" and "forbidden" sound contradictory. > 3) Are you only going to have a single blocklist? I imagine that in reality > you're going to have age categories, and considerations that the > "family-friendliness" of some subjects like sex and violence is going to be > very locale dependent. But I'm not sure that matters for this code, as we > can swap in different lists with prefs. That would mean we commit to serve > the cross-product of every possible blocklist you would want, though. Right now, we're looking at a single blocklist (see bug 1225499). If that got split into a number of *-forbid-shavar lists in the future, that would be fine too, we could just have urlclassifier.forbiddenTable = "sex-forbid-shavar,violence-forbid-shavar,..." > ::: browser/base/content/browser.js > @@ +3018,5 @@ > > bucketName += isTopFrame ? "TOP_" : "FRAME_"; > > switch (elementId) { > > case "getMeOutButton": > > + if (sendTelemetry) { > > + secHistogram.add(nsISecTel[bucketName + "GET_ME_OUT_OF_HERE"]); > > So the bucket is empty for this new category? Looks strange. There's no bucket for this in Telemetry because "sendTelemetry" is false in that case. I'm pretty sure we don't want to collect metrics on how often children try to access a forbidden site. (In reply to Gian-Carlo Pascutto [:gcp] from comment #4) > Would being able to swap the phishing and malware lists into the forbidden > pref solve the CESG-GCHQ thing being discussed on dev.security? I'm planning on addressing that separately because I don't want to confuse these two fairly different use cases.

Comment on attachment 8689897 [details] [diff] [review] Code changes Olli, would you mind taking a look at this patch? It contains a few minor DocShell and DOM changes.

Comment on attachment 8689897 [details] [diff] [review] Code changes browser.safebrowsing.forbidden.enabled sounds a bit odd pref to me. If I just read that I think it would mean something like disabling safebrowsing is prevented. But the pref is actually about something totally different. s/forbidden.enabled/forbiddenURIs.enabled/ in the pref? would that be better? That is after all what the error name is, NS_ERROR_FORBIDDEN_URI mCheckForbidden should also be renamed. I guess I'd like to see the naming be consistently clear what the 'forbidden' is about.

There was a file missing in the original version of this patch.

Comment on attachment 8689897 [details] [diff] [review] Code changes Review of attachment 8689897 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/blockedSite.xhtml @@ +120,2 @@ > > + if (error === "forbidden") { Swap this around so its symmetric with the other cases, i.e. error !== ::: browser/base/content/browser.js @@ +3001,5 @@ > onAboutBlocked: function (elementId, reason, isTopFrame, location) { > // Depending on what page we are displaying here (malware/phishing/unwanted) > // use the right strings and links for each. > + let bucketName = ""; > + let sendTelemetry = false; I guess the reason we don't want to add a bucket for this is because the block will be very specific to some users' settings and consequently it wouldn't tell us much? ::: mobile/android/chrome/content/blockedSite.xhtml @@ +118,5 @@ > + el = document.getElementById("errorLongDescText_unwanted"); > + el.parentNode.removeChild(el); > + } > + > + if (error === "forbidden") { Same remark as before.

Comment on attachment 8690106 [details] [diff] [review] Tests Review of attachment 8690106 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/safebrowsing/content/test/head.js @@ +18,5 @@ > + * @return {Promise} resolved when the event is handled. > + * @resolves to the received event > + * @rejects if a valid load event is not received within a meaningful interval > + */ > +function promiseTabLoadEvent(tab, url, eventType="load") Bit sad this is duplicated throughout the tree.

Same patch but with the if statement reversed as requested by gcp and an improved pref name.

Same tests but with the new pref name. Carrying gcp's r+.

(In reply to Gian-Carlo Pascutto [:gcp] from comment #12) > ::: browser/base/content/browser.js > @@ +3001,5 @@ > > onAboutBlocked: function (elementId, reason, isTopFrame, location) { > > // Depending on what page we are displaying here (malware/phishing/unwanted) > > // use the right strings and links for each. > > + let bucketName = ""; > > + let sendTelemetry = false; > > I guess the reason we don't want to add a bucket for this is because the > block will be very specific to some users' settings and consequently it > wouldn't tell us much? I see two reasons: 1. Given the content of the forbid lists, this is sensitive data. 2. I don't see why we would need to know how often our users visit such forbidden sites.

Comment on attachment 8690261 [details] [diff] [review] Code changes I would use forbiddenURI as the term everywhere consistently, but either way.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c0dbcf2a51 https://hg.mozilla.org/integration/mozilla-inbound/rev/0b2a4b521e6b

https://hg.mozilla.org/mozilla-central/rev/d7c0dbcf2a51 https://hg.mozilla.org/mozilla-central/rev/0b2a4b521e6b

Can someone explain how to test this for localization? I tried http://www.itisatrap.org/firefox/forbidden.html but I don't get any block.

(In reply to Francesco Lodolo [:flod] from comment #20) > Can someone explain how to test this for localization? I tried > http://www.itisatrap.org/firefox/forbidden.html but I don't get any block. To enable it, you need to manually toggle the "browser.safebrowsing.forbiddenURIs.enabled" pref. It's like Safe Browsing except without the ability to ignore the warnings.

(In reply to François Marier [:francois] from comment #21) > To enable it, you need to manually toggle the > "browser.safebrowsing.forbiddenURIs.enabled" pref. Thanks. Toggled the pref (also restarted nightly to be sure) but it doesn't seem to work nonetheless (no warning).

(In reply to Francesco Lodolo [:flod] from comment #22) > Thanks. Toggled the pref (also restarted nightly to be sure) but it doesn't > seem to work nonetheless (no warning). Argh, never mind. A forced refresh loaded the warning page.