Closed
Bug 1274893
Opened 8 years ago
Closed 8 years ago
Remove support for -forbid- lists in Safe Browsing
Categories
(Toolkit :: Safe Browsing, defect, P2)
Toolkit
Safe Browsing
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: francois, Assigned: allstars.chh)
References
Details
Attachments
(1 file, 8 obsolete files)
70.79 KB,
patch
|
allstars.chh
:
review+
|
Details | Diff | Splinter Review |
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•8 years ago
|
||
Yoshi, is this something you'd like to take on?
Assignee | ||
Comment 3•8 years ago
|
||
(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•8 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.
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Assignee | ||
Comment 8•8 years ago
|
||
Assignee | ||
Comment 9•8 years ago
|
||
Assignee | ||
Comment 10•8 years ago
|
||
There are still some failures on try, I am fixing them https://treeherder.mozilla.org/#/jobs?repo=try&revision=da760e808229
Status: NEW → ASSIGNED
Reporter | ||
Comment 11•8 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•8 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•8 years ago
|
Attachment #8761091 -
Flags: feedback+
Reporter | ||
Updated•8 years ago
|
Attachment #8761092 -
Flags: feedback+
Reporter | ||
Updated•8 years ago
|
Attachment #8761093 -
Flags: feedback+
Reporter | ||
Comment 13•8 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.
Assignee | ||
Comment 14•8 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"); 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
Assignee | ||
Comment 15•8 years ago
|
||
(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•8 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.
Assignee | ||
Comment 17•8 years ago
|
||
Addressed comment 11
Attachment #8761089 -
Attachment is obsolete: true
Assignee | ||
Comment 18•8 years ago
|
||
addressed comment 12
Attachment #8761090 -
Attachment is obsolete: true
Assignee | ||
Comment 19•8 years ago
|
||
Assignee | ||
Comment 20•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=186b5dcd87fc
Assignee | ||
Updated•8 years ago
|
Attachment #8766160 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8766161 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8761091 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8761092 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8761093 -
Flags: review?(francois)
Assignee | ||
Updated•8 years ago
|
Attachment #8766162 -
Flags: review?(francois)
Reporter | ||
Updated•8 years ago
|
Attachment #8761091 -
Flags: review?(francois) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8761092 -
Flags: review?(francois) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8761093 -
Flags: review?(francois) → review+
Reporter | ||
Comment 21•8 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•8 years ago
|
Attachment #8766161 -
Flags: review?(francois) → review+
Reporter | ||
Updated•8 years ago
|
Attachment #8766162 -
Flags: review?(francois) → review+
Assignee | ||
Comment 22•8 years ago
|
||
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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b7a8fe507301
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in
before you can comment on or make changes to this bug.
Description
•