Remove support for -forbid- lists in Safe Browsing

RESOLVED FIXED in Firefox 50

Status

()

Toolkit
Safe Browsing
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: francois, Unassigned)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment, 8 obsolete attachments)

70.79 KB, patch
allstars
: review+
Details | Diff | Splinter Review
(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Yoshi, is this something you'd like to take on?
My pleasure :D
Assignee: nobody → allstars.chh
(Assignee)

Updated

2 years ago
Blocks: 999484
(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.
(Reporter)

Comment 4

2 years ago
(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.
Created attachment 8761089 [details] [diff] [review]
Part 1: remove -forbid- SB list type.
Created attachment 8761090 [details] [diff] [review]
Part 2: update the tests for -forbid- SB list type.
Created attachment 8761091 [details] [diff] [review]
Part 3: remove XML entry on Fennect for -forbid- SB list.
Created attachment 8761092 [details] [diff] [review]
Part 4: remove text and style of 'forbidden' page.
Created attachment 8761093 [details] [diff] [review]
Part 5: Remove text and style of "forbidden" page.
There are still some failures on try, I am fixing them 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=da760e808229
Status: NEW → ASSIGNED
(Reporter)

Updated

2 years ago
Depends on: 1279637
(Reporter)

Comment 11

2 years ago
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.
Attachment #8761089 - Flags: feedback+
(Reporter)

Comment 12

2 years ago
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".
Attachment #8761090 - Flags: feedback-
(Reporter)

Updated

2 years ago
Attachment #8761091 - Flags: feedback+
(Reporter)

Updated

2 years ago
Attachment #8761092 - Flags: feedback+
(Reporter)

Updated

2 years ago
Attachment #8761093 - Flags: feedback+
(Reporter)

Comment 13

2 years ago
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 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
(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.
(Reporter)

Comment 16

2 years ago
(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.
Created attachment 8766160 [details] [diff] [review]
Part 1: remove -forbid- SB list type. v2

Addressed comment 11
Attachment #8761089 - Attachment is obsolete: true
Created attachment 8766161 [details] [diff] [review]
Part 2: update the tests for -forbid- SB list type. v2

addressed comment 12
Attachment #8761090 - Attachment is obsolete: true
Created attachment 8766162 [details] [diff] [review]
Part 6: update remaining tests.
(Assignee)

Updated

2 years ago
Attachment #8766160 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8766161 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8761091 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8761092 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8761093 - Flags: review?(francois)
(Assignee)

Updated

2 years ago
Attachment #8766162 - Flags: review?(francois)
(Reporter)

Updated

2 years ago
Attachment #8761091 - Flags: review?(francois) → review+
(Reporter)

Updated

2 years ago
Attachment #8761092 - Flags: review?(francois) → review+
(Reporter)

Updated

2 years ago
Attachment #8761093 - Flags: review?(francois) → review+
(Reporter)

Comment 21

2 years ago
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
Attachment #8766160 - Flags: review?(francois) → review+
(Reporter)

Updated

2 years ago
Attachment #8766161 - Flags: review?(francois) → review+
(Reporter)

Updated

2 years ago
Attachment #8766162 - Flags: review?(francois) → review+
Created attachment 8769583 [details] [diff] [review]
Patch.

Addressed comment 21 and merged them into one patch.
Attachment #8761091 - Attachment is obsolete: true
Attachment #8761092 - Attachment is obsolete: true
Attachment #8761093 - Attachment is obsolete: true
Attachment #8766160 - Attachment is obsolete: true
Attachment #8766161 - Attachment is obsolete: true
Attachment #8766162 - Attachment is obsolete: true
Attachment #8769583 - Flags: review+

Comment 23

2 years ago
Pushed by yhuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a8fe507301
remove -forbid- list from SafeBrowsing. r=francois

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b7a8fe507301
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Blocks: 1293104
You need to log in before you can comment on or make changes to this bug.