Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1251)

Issue 29785581: Issue 6091 - Add checks for overly generic antiadblock filters (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 2 months ago by Jon Sonesen
Modified:
5 months, 3 weeks ago
Reviewers:
Thomas Greiner, kzar
Base URL:
https://hg.adblockplus.org/adblockplusui/
Visibility:
Public.

Description

Issue 6091 - Add check to antiadblock notifications for tld and public suffix filters

Patch Set 1 #

Total comments: 13

Patch Set 2 : Address PS1 comments #

Total comments: 8

Patch Set 3 : Address PS2 comments #

Patch Set 4 : Actually moved all logic to start of iteration #

Total comments: 7

Patch Set 5 : Address PS5 comments #

Total comments: 2

Patch Set 6 : Address PS6 Comments #

Total comments: 9
Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -1 line) Patch
M lib/antiadblockInit.js View 1 2 3 4 5 2 chunks +8 lines, -1 line 9 comments Download

Messages

Total messages: 26
Jon Sonesen
1 year, 2 months ago (2018-05-18 20:11:09 UTC) #1
Jon Sonesen
On 2018/05/18 20:11:09, Jon Sonesen wrote: I used a gist to replicate the state of ...
1 year, 2 months ago (2018-05-18 20:15:24 UTC) #2
Jon Sonesen
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) Is hardly ...
1 year, 2 months ago (2018-05-18 20:16:18 UTC) #3
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) On 2018/05/18 ...
1 year, 1 month ago (2018-05-22 12:43:10 UTC) #4
kzar
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* global publicSuffixes */ Nit: Seems like we generally ...
1 year, 1 month ago (2018-05-22 13:24:03 UTC) #5
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode60 lib/antiadblockInit.js:60: if (!(publicSuffixes.hasOwnProperty(domain))) On 2018/05/22 13:24:02, kzar wrote: > Well ...
1 year, 1 month ago (2018-05-22 17:48:59 UTC) #6
Jon Sonesen
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode56 lib/antiadblockInit.js:56: if (domain && included && urlFilters.indexOf(urlFilter) == -1) On ...
1 year, 1 month ago (2018-05-22 18:26:12 UTC) #7
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode56 lib/antiadblockInit.js:56: if (domain && included && urlFilters.indexOf(urlFilter) == -1) On ...
1 year, 1 month ago (2018-05-22 18:43:29 UTC) #8
Jon Sonesen
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* global publicSuffixes */ On 2018/05/22 13:24:02, kzar wrote: ...
1 year, 1 month ago (2018-05-22 21:25:19 UTC) #9
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) Effectively, this logic ...
1 year, 1 month ago (2018-05-23 13:45:55 UTC) #10
kzar
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 13:45:54, ...
1 year, 1 month ago (2018-05-23 14:41:45 UTC) #11
Jon Sonesen
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 13:45:54, ...
1 year, 1 month ago (2018-05-23 23:12:52 UTC) #12
Jon Sonesen
I realized it may be better to start with this check at the start of ...
1 year, 1 month ago (2018-05-24 00:00:10 UTC) #13
kzar
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/24 00:00:09, ...
1 year, 1 month ago (2018-05-24 11:28:58 UTC) #14
Jon Sonesen
On 2018/05/24 11:28:58, kzar wrote: > https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js > File lib/antiadblockInit.js (right): > > https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js#newcode58 > ...
1 year, 1 month ago (2018-05-24 14:07:37 UTC) #15
kzar
https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit.js#newcode55 lib/antiadblockInit.js:55: let domainIsTLD = !domain.includes(".") || domain.endsWith("."); This logic doesn't ...
1 year, 1 month ago (2018-05-25 13:51:57 UTC) #16
Jon Sonesen
Hey, I have some confusion about what you guys want me to do here. https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit.js ...
1 year, 1 month ago (2018-05-25 21:04:40 UTC) #17
Jon Sonesen
Sorry, I guess the ordering of the different comments on PS2 sort of confused me ...
1 year, 1 month ago (2018-05-25 22:06:23 UTC) #18
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.match(/\.[^$]/) && !publicSuffixes.hasOwnProperty(domain)) Detail: We're only interested in ...
1 year, 1 month ago (2018-05-28 13:09:37 UTC) #19
Jon Sonesen
https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js#newcode58 lib/antiadblockInit.js:58: if (domain.match(/\.[^$]/) && !publicSuffixes.hasOwnProperty(domain)) On 2018/05/28 13:09:37, Thomas Greiner ...
1 year, 1 month ago (2018-05-29 20:31:43 UTC) #20
Thomas Greiner
LGTM https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* globals publicSuffixes */ Detail: This whitespace appears ...
1 year, 1 month ago (2018-05-30 10:18:04 UTC) #21
kzar
https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* globals publicSuffixes */ On 2018/05/30 10:18:03, Thomas Greiner ...
1 year, 1 month ago (2018-05-30 10:57:48 UTC) #22
Thomas Greiner
https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* globals publicSuffixes */ On 2018/05/30 10:57:48, kzar wrote: ...
1 year, 1 month ago (2018-05-30 12:30:10 UTC) #23
kzar
https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js#newcode18 lib/antiadblockInit.js:18: /* globals publicSuffixes */ On 2018/05/30 12:30:09, Thomas Greiner ...
1 year, 1 month ago (2018-05-30 12:59:03 UTC) #24
kzar
On 2018/05/30 12:59:03, kzar wrote: > No: > > "com", "com.", "co.uk", "co.uk.", "foo.no-ip.co.uk", "foo.no-ip.co.uk." ...
1 year, 1 month ago (2018-05-30 13:00:16 UTC) #25
Jon Sonesen
1 year, 1 month ago (2018-05-30 15:46:54 UTC) #26
Thanks for the comments, I will add tests and fix the whitespace/regex and call
order.

https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit.js
File lib/antiadblockInit.js (right):

https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit...
lib/antiadblockInit.js:18: /* globals publicSuffixes */
On 2018/05/30 12:59:02, kzar wrote:
> On 2018/05/30 12:30:09, Thomas Greiner wrote:
> > On 2018/05/30 10:57:48, kzar wrote:
> > > The newline? If so it seems consistent with how we did it elsewhere.
> > 
> > No, the whitespace character to the left of the comment.
> 
> Oh yea, I see what you mean now.

Acknowledged.

https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit...
lib/antiadblockInit.js:58: if (/\.[^$]/.test(domain) &&
!publicSuffixes.hasOwnProperty(domain))
On 2018/05/30 12:59:02, kzar wrote:
> On 2018/05/30 12:30:09, Thomas Greiner wrote:
> > This regex will indeed fail for multi-part TLDs.
> 
> Well my concern was that "co.uk" is in the publicSuffixes Object but not
> "co.uk.". But perhaps the regex was wrong too!
> 
> > - !domain.endsWith(".") && domain.includes(".")
> 
> Perhaps you'd be better using .indexOf and then checking the index isn't
either
> -1 nor the length of the array-1?
> 
> Either way please check your code against these Jon:
> 
> Yes:
> 
> "foo.com", "foo.com.", "foo.co.uk", "foo.co.uk.", "foo.no-ip.co.uk",
> "foo.no-ip.co.uk."
> 
> No:
> 
> "com", "com.", "co.uk", "co.uk.", "foo.no-ip.co.uk", "foo.no-ip.co.uk."
> 
> In fact this would probably be a good place to add some unit tests.

Unit tests it is then!
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5