1274893 - Remove support for -forbid- lists in Safe Browsing

Status: RESOLVED FIXED

(Toolkit :: Safe Browsing, defect, P2)

Description

[François Marier [:francois]]

The blacklist that got added in bug 1216723 and bug 1225433 for the fennec
parental controls is no longer going to be used. We should therefore
remove everything that has to do with forbidden URIs (the "-forbid-" list type).

These are the relevant commits that will need to be partially or fully backed out:

https://hg.mozilla.org/mozilla-central/rev/62fc74be5c75
https://hg.mozilla.org/mozilla-central/rev/3d4671e701e4
https://hg.mozilla.org/mozilla-central/rev/03dee128e789
https://hg.mozilla.org/mozilla-central/rev/40245af65036
https://hg.mozilla.org/mozilla-central/rev/0b2a4b521e6b
https://hg.mozilla.org/mozilla-central/rev/d7c0dbcf2a51

but a dxr search for "forbidden" or "forbid" will also reveal a few more
places and tests where support for forbidden URIs was added since.

Comment 1

[François Marier [:francois]]

Yoshi, is this something you'd like to take on?

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

My pleasure :D

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to François Marier [:francois] from comment #0)
> https://hg.mozilla.org/mozilla-central/rev/03dee128e789
This should not be related to -forbid- list.

Comment 4

[François Marier [:francois]]

(In reply to Yoshi Huang[:allstars.chh] from comment #3)
> (In reply to François Marier [:francois] from comment #0)
> > https://hg.mozilla.org/mozilla-central/rev/03dee128e789
> This should not be related to -forbid- list.

You're right, that has nothing to do with -forbid-. It must have been a copy/paste error.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: remove -forbid- SB list type.

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: update the tests for -forbid- SB list type.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 3: remove XML entry on Fennect for -forbid- SB list.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 4: remove text and style of 'forbidden' page.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 5: Remove text and style of "forbidden" page.

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

There are still some failures on try, I am fixing them 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da760e808229

Comment 11

[François Marier [:francois]]

Comment on attachment 8761089 [details] [diff] [review]
Part 1: remove -forbid- SB list type.

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

::: modules/libpref/init/all.js
@@ -5039,5 @@
>  #else
>  pref("urlclassifier.downloadAllowTable", "");
>  #endif
>  
> -pref("urlclassifier.disallow_completions", "test-malware-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,test-forbid-simple,goog-downloadwhite-digest256,mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256,test-block-simple,mozplugin-block-digest256,mozplugin2-block-digest256");

nit: This is not required by this bug, but while we're changing this line, it would be nice to move "test-block-simple" to be at the end of the test lists instead of randomly mixed with the real lists.

Comment 12

[François Marier [:francois]]

Comment on attachment 8761090 [details] [diff] [review]
Part 2: update the tests for -forbid- SB list type.

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

::: browser/base/content/blockedSite.xhtml
@@ -137,5 @@
>            if (btn) {
>              btn.parentNode.removeChild(btn);
>            }
>          }
> -

I don't think you want to remove this one since it might be used by the other tests too...

::: toolkit/components/url-classifier/tests/unit/test_dbservice.js
@@ -54,5 @@
>  var chunk7 = chunk7Urls.join("\n");
>  
>  // we are going to add chunks 1, 2, 4, 5, and 6 to phish-simple,
> -// chunk 2 to malware-simple, chunk 3 to unwanted-simple,
> -// chunk 4 to forbid-simple, and chunk 7 to block-simple.

We should keep "chunk 7 to block-simple".

Comment 13

[François Marier [:francois]]

Thanks for the thorough job Yoshi. The thing I pointed out in comment 12 might be the cause of the test failures you have seen.

Suggestion: I think it might be worthwhile to merge all of the patches together even though it will be a pretty long one. That way the whole feature is removed in a single commit along with relevant tests and UI.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8761089 [details] [diff] [review]
Part 1: remove -forbid- SB list type.

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

::: modules/libpref/init/all.js
@@ -5039,5 @@
>  #else
>  pref("urlclassifier.downloadAllowTable", "");
>  #endif
>  
> -pref("urlclassifier.disallow_completions", "test-malware-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,test-forbid-simple,goog-downloadwhite-digest256,mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256,test-block-simple,mozplugin-block-digest256,mozplugin2-block-digest256");

I'll move test-block-simple to the end.

But did you notice that I've removed test-forbid-simple?
That's added in https://hg.mozilla.org/mozilla-central/diff/d7c0dbcf2a51/modules/libpref/init/all.js

Do you mean I don't have to remove 'test-forbid-simple'?

Thanks

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to François Marier [:francois] from comment #13)
> Suggestion: I think it might be worthwhile to merge all of the patches
> together even though it will be a pretty long one. That way the whole
> feature is removed in a single commit along with relevant tests and UI.
Great, so I'll merge them all when I push to inbound

So far for the review I'll still split into different parts, that's easier for reviewer and me to check those patches.

Comment 16

[François Marier [:francois]]

(In reply to Yoshi Huang[:allstars.chh] from comment #14)
> I'll move test-block-simple to the end.

Great, thanks!

> But did you notice that I've removed test-forbid-simple?
> That's added in
> https://hg.mozilla.org/mozilla-central/diff/d7c0dbcf2a51/modules/libpref/
> init/all.js
> 
> Do you mean I don't have to remove 'test-forbid-simple'?

Yes, removing test-forbid-simple is the right thing to do. We don't need it anymore.

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 1: remove -forbid- SB list type. v2

Addressed comment 11

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 2: update the tests for -forbid- SB list type. v2

addressed comment 12

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Part 6: update remaining tests.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=186b5dcd87fc

Comment 21

[François Marier [:francois]]

Comment on attachment 8766160 [details] [diff] [review]
Part 1: remove -forbid- SB list type. v2

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

r=me but please address my comment first.

::: modules/libpref/init/all.js
@@ +5058,5 @@
>  #else
>  pref("urlclassifier.downloadAllowTable", "");
>  #endif
>  
> +pref("urlclassifier.disallow_completions", "test-malware-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,goog-downloadwhite-digest256,mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256,mozplugin-block-digest256,mozplugin2-block-digest256,test-block-simple");

Sorry for the confusion. What I meant was moving "test-block-simple" to be after "test-trackwhite-simple".

So like this:

test-malware-simple,test-phish-simple,test-unwanted-simple,test-track-simple,test-trackwhite-simple,test-block-simple,goog-downloadwhite-digest256,mozstd-track-digest256,mozstd-trackwhite-digest256,mozfull-track-digest256,mozplugin-block-digest256,mozplugin2-block-digest256

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Addressed comment 21 and merged them into one patch.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a8fe507301
remove -forbid- list from SafeBrowsing. r=francois

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b7a8fe507301