Issue 29785581: Issue 6091 - Add checks for overly generic antiadblock filters (Closed)
|
Created:
May 18, 2018, 8:11 p.m. by Jon Sonesen Modified:
Jan. 29, 2019, 3:52 p.m. Base URL:
https://hg.adblockplus.org/adblockplusui/ Visibility:
Public. |
DescriptionIssue 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
MessagesTotal messages: 26
On 2018/05/18 20:11:09, Jon Sonesen wrote: I used a gist to replicate the state of the filterlist that caused the issue: https://gist.githubusercontent.com/VoyagerPunk/b4f49f6ab600ce72607a7bf2763f86...
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) Is hardly an exhaustive list of TLDs but there are also quite a few so not sure how many we should cover.
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) On 2018/05/18 20:16:18, Jon Sonesen wrote: > Is hardly an exhaustive list of TLDs but there are also quite a few so not sure > how many we should cover. All public suffixes (including TLDs) are listed in https://publicsuffix.org/list/public_suffix_list.dat and https://publicsuffix.org/list/effective_tld_names.dat but, for some reason, it seems like "com" and others are not included in the lib/publicSuffixList.js file we generate when building the extension (see https://hg.adblockplus.org/buildtools/file/168dac24ad9e/publicSuffixListUpdat...). Instead, we're filtering out lines which don't contain a dot and consider any domain that doesn't contain a dot as a TLD (see https://hg.adblockplus.org/adblockpluschrome/file/939bf6cdd435/lib/tldjs.js#l30). Be careful, however, when checking for that because of FQDNs which end with a dot but it looks like we're not checking for those there either. https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:60: if (!(publicSuffixes.hasOwnProperty(domain))) Ideally, this logic would be part of `Filter.prototype.isGeneric()` because I'd consider filters which apply to a large subset of domains (e.g. all "com" domains) also as generic. @Dave What's your take on that?
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:18: /* global publicSuffixes */ Nit: Seems like we generally write "globals" instead of "global" in our code so far. https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:56: if (domain && included && urlFilters.indexOf(urlFilter) == -1) Seems kinda inefficient how we're doing urlFilters.indexOf each time. Perhaps we should make urlFilters a Set, but then convert it to an Array when assigning it to notification.urlFilters? https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) On 2018/05/22 12:43:10, Thomas Greiner wrote: > ... it seems like "com" and others are not included in the lib/publicSuffixList.js... > Instead, we're filtering out lines which don't contain a dot and consider any > domain that doesn't contain a dot as a TLD... careful... of FQDNs which end with a > dot... Yes, I expected we would first check that the domain had at least one dot before checking it wasn't present in the public suffix list. We definitely don't these hard coded domains here. I didn't consider the case of a trailing dot however. I did a test to see if they can even show up and found they can. So Thomas is right, be careful to handle those. (I found we don't handle trailing dots consistently, we strip them for blocking filters but not element hiding filters, so I've opened #6690 to discuss that.) https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:60: if (!(publicSuffixes.hasOwnProperty(domain))) On 2018/05/22 12:43:10, Thomas Greiner wrote: > Ideally, this logic would be part of `Filter.prototype.isGeneric()` because I'd > consider filters which apply to a large subset of domains (e.g. all "com" > domains) also as generic. > > @Dave What's your take on that? Well you're right that a filter for the domain "com" is a very generic filter and therefore what you suggest makes sense. Unfortunately in the context of the core code a filter being generic or specific means something else, which means we can't do it in practice (without overhauling getSelectorsForDomain and other core code).
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:60: if (!(publicSuffixes.hasOwnProperty(domain))) On 2018/05/22 13:24:02, kzar wrote: > Well you're right that a filter for the domain "com" is a very generic filter > and therefore what you suggest makes sense. Unfortunately in the context of the > core code a filter being generic or specific means something else, which means > we can't do it in practice (without overhauling getSelectorsForDomain and other > core code). That's quite a pity. Nevermind then.
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:56: if (domain && included && urlFilters.indexOf(urlFilter) == -1) On 2018/05/22 13:24:02, kzar wrote: > Seems kinda inefficient how we're doing urlFilters.indexOf each time. Perhaps we > should make urlFilters a Set, but then convert it to an Array when assigning it > to notification.urlFilters? I agree here, and was thinking about this. Was not sure if it counted as an unrelated change though. @Thomas whats your opinion here? https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:58: if (["COM", "INFO", "ORG", "NET"].indexOf(domain) == -1) On 2018/05/22 13:24:02, kzar wrote: > On 2018/05/22 12:43:10, Thomas Greiner wrote: > > ... it seems like "com" and others are not included in the > lib/publicSuffixList.js... > > Instead, we're filtering out lines which don't contain a dot and consider any > > domain that doesn't contain a dot as a TLD... careful... of FQDNs which end > with a > > dot... > > Yes, I expected we would first check that the domain had at least one dot before > checking it wasn't present in the public suffix list. We definitely don't these > hard coded domains here. > > I didn't consider the case of a trailing dot however. I did a test to see if > they can even show up and found they can. So Thomas is right, be careful to > handle those. (I found we don't handle trailing dots consistently, we strip them > for blocking filters but not element hiding filters, so I've opened #6690 to > discuss that.) Okay, thanks for that. Was not sure how we handled generic filters in other places and this was just a "works for now" implementation as I wasn't 100% sure how to proceed. Thank you guys for clearing it up. https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:60: if (!(publicSuffixes.hasOwnProperty(domain))) On 2018/05/22 17:48:59, Thomas Greiner wrote: > On 2018/05/22 13:24:02, kzar wrote: > > Well you're right that a filter for the domain "com" is a very generic filter > > and therefore what you suggest makes sense. Unfortunately in the context of > the > > core code a filter being generic or specific means something else, which means > > we can't do it in practice (without overhauling getSelectorsForDomain and > other > > core code). > > That's quite a pity. Nevermind then. Acknowledged.
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:56: if (domain && included && urlFilters.indexOf(urlFilter) == -1) On 2018/05/22 18:26:11, Jon Sonesen wrote: > I agree here, and was thinking about this. Was not sure if it counted as an > unrelated change though. @Thomas whats your opinion here? I don't know whether it'd be faster and, if so, by how much but it should be easy to find out using jsPerf or other means. The results may also depend on the amount of domains we have to process so note that the antiadblockfilters.txt list currently contains less than 400 filters with less than 300 domains.
https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29785582/lib/antiadblockInit... lib/antiadblockInit.js:18: /* global publicSuffixes */ On 2018/05/22 13:24:02, kzar wrote: > Nit: Seems like we generally write "globals" instead of "global" in our code so > far. Done.
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) Effectively, this logic leads to the following results: example.com true or false == true example.com. true or true == true com false or false == false com. false or true == true However, we wanted to treat "com" and "com." the same here so shouldn't it instead be `domain.indexOf(".") != -1 || !domain.endsWith(".")`? https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) Suggestion: Double negation usually makes code quite hard to read (i.e. `!= -1` here means "does not not exist") so rewriting it to the following could help making it a bit easier to understand: let firstDot = domain.indexOf("."); if (firstDot == -1 || firstDot == domain.length - 1) continue; Alternatively, you could use the regular expression `/\.[^$]/` to check whether the domain contains a dot that's not at the end of the string.
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 13:45:54, Thomas Greiner wrote: > Suggestion: Double negation usually makes code quite hard to read (i.e. `!= -1` > here means "does not not exist") so rewriting it to the following could help > making it a bit easier to understand: > > let firstDot = domain.indexOf("."); > if (firstDot == -1 || firstDot == domain.length - 1) > continue; > > Alternatively, you could use the regular expression `/\.[^$]/` to check whether > the domain contains a dot that's not at the end of the string. Also please can you use `.includes(".")` instead of `.indexOf(".") != -1` ?
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 13:45:54, Thomas Greiner wrote: > Effectively, this logic leads to the following results: > > http://example.com true or false == true > http://example.com. true or true == true > com false or false == false > com. false or true == true > > However, we wanted to treat "com" and "com." the same here so shouldn't it > instead be `domain.indexOf(".") != -1 || !domain.endsWith(".")`? Hey, My bad I think I misunderstood the last comments reading comprehension faaiil. Thanks, will do. https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 14:41:45, kzar wrote: > On 2018/05/23 13:45:54, Thomas Greiner wrote: > > Suggestion: Double negation usually makes code quite hard to read (i.e. `!= > -1` > > here means "does not not exist") so rewriting it to the following could help > > making it a bit easier to understand: > > > > let firstDot = domain.indexOf("."); > > if (firstDot == -1 || firstDot == domain.length - 1) > > continue; > > > > Alternatively, you could use the regular expression `/\.[^$]/` to check > whether > > the domain contains a dot that's not at the end of the string. > > Also please can you use `.includes(".")` instead of `.indexOf(".") != -1` ? For sure https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 13:45:54, Thomas Greiner wrote: > Suggestion: Double negation usually makes code quite hard to read (i.e. `!= -1` > here means "does not not exist") so rewriting it to the following could help > making it a bit easier to understand: > > let firstDot = domain.indexOf("."); > if (firstDot == -1 || firstDot == domain.length - 1) > continue; > > Alternatively, you could use the regular expression `/\.[^$]/` to check whether > the domain contains a dot that's not at the end of the string. Acknowledged.
I realized it may be better to start with this check at the start of the iteration in order to skip over some logic, but if this is harder to read or is not needed I am happy to move it back. https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/23 14:41:45, kzar wrote: > On 2018/05/23 13:45:54, Thomas Greiner wrote: > > Suggestion: Double negation usually makes code quite hard to read (i.e. `!= > -1` > > here means "does not not exist") so rewriting it to the following could help > > making it a bit easier to understand: > > > > let firstDot = domain.indexOf("."); > > if (firstDot == -1 || firstDot == domain.length - 1) > > continue; > > > > Alternatively, you could use the regular expression `/\.[^$]/` to check > whether > > the domain contains a dot that's not at the end of the string. > > Also please can you use `.includes(".")` instead of `.indexOf(".") != -1` ? Actually, on second thought would it be better to use `.startsWith(".")` here?
https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29787560/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || domain.endsWith(".")) On 2018/05/24 00:00:09, Jon Sonesen wrote: > Actually, on second thought would it be better to use `.startsWith(".")` here? I'm not sure I understand the question. Why do you consider using startsWith?
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... > lib/antiadblockInit.js:58: if (domain.indexOf(".") != -1 || > domain.endsWith(".")) > On 2018/05/24 00:00:09, Jon Sonesen wrote: > > Actually, on second thought would it be better to use `.startsWith(".")` here? > > I'm not sure I understand the question. Why do you consider using startsWith? That was a mistake, I write the draft they for it about it. Sorry!
https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:55: let domainIsTLD = !domain.includes(".") || domain.endsWith("."); This logic doesn't look right to me, what about the example of "dave.example.com.", that's definitely not a TLD. https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:55: let domainIsTLD = !domain.includes(".") || domain.endsWith("."); This variable seems kind of pointless, when we only use it once and immediately after we declare it. https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:56: if (domainIsTLD || publicSuffixes.hasOwnProperty(domain)) If the domain ends in "." it won't be found in publicSuffixes, for example "googlecode.com.". https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:59: if (domain && included && urlFilters.indexOf(urlFilter) == -1) It seems a shame we're doing these cheaper checks `domain && included` after some more expensive ones like `domain.endsWith(".")`.
Hey, I have some confusion about what you guys want me to do here. https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:55: let domainIsTLD = !domain.includes(".") || domain.endsWith("."); On 2018/05/25 13:51:56, kzar wrote: > This logic doesn't look right to me, what about the example of > "dave.example.com.", that's definitely not a TLD. Hm, but there is the case where `com.` and `com` should be treated equally, meaning it is considered a TLD. However in all other cases it isn't considered a TLD? I am confused about what you guys want here. If it doesn't have a dot it's generic but if it ends with a dot it isn't and yet "com" and "com." are treated the same? This is seems contradictory can you please clarify? https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:56: if (domainIsTLD || publicSuffixes.hasOwnProperty(domain)) On 2018/05/25 13:51:57, kzar wrote: > If the domain ends in "." it won't be found in publicSuffixes, for example > "googlecode.com.". Acknowledged. https://codereview.adblockplus.org/29785581/diff/29788572/lib/antiadblockInit... lib/antiadblockInit.js:59: if (domain && included && urlFilters.indexOf(urlFilter) == -1) On 2018/05/25 13:51:57, kzar wrote: > It seems a shame we're doing these cheaper checks `domain && included` after > some more expensive ones like `domain.endsWith(".")`. Acknowledged.
Sorry, I guess the ordering of the different comments on PS2 sort of confused me but as Thomas suggested the regex seems to work well, and then can just check if it's in the publicSuffixList.
https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.match(/\.[^$]/) && !publicSuffixes.hasOwnProperty(domain)) Detail: We're only interested in whether the string matches so please use `/\.[^$]/.test(domain)` instead. See also https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit.js File lib/antiadblockInit.js (right): https://codereview.adblockplus.org/29785581/diff/29790624/lib/antiadblockInit... lib/antiadblockInit.js:58: if (domain.match(/\.[^$]/) && !publicSuffixes.hasOwnProperty(domain)) On 2018/05/28 13:09:37, Thomas Greiner wrote: > Detail: We're only interested in whether the string matches so please use > `/\.[^$]/.test(domain)` instead. > > See also > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Acknowledged.
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... lib/antiadblockInit.js:18: /* globals publicSuffixes */ Detail: This whitespace appears to have been added by accident.
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 10:18:03, Thomas Greiner wrote: > Detail: This whitespace appears to have been added by accident. The newline? If so it seems consistent with how we did it elsewhere. (More or less anyway. Apparently it varies a little if we write "globals" or "global", if we put that above or below "use strict"; and if we add the newline or not.) https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit... lib/antiadblockInit.js:58: if (/\.[^$]/.test(domain) && !publicSuffixes.hasOwnProperty(domain)) Please combine the two if statements. Also please do the publicSuffixes lookup before the regexp test, I think it's cheaper. Also what about a case like "co.uk."?
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 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. https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit... lib/antiadblockInit.js:58: if (/\.[^$]/.test(domain) && !publicSuffixes.hasOwnProperty(domain)) On 2018/05/30 10:57:48, kzar wrote: > Also what about a case like "co.uk."? Good call. This regex will indeed fail for multi-part TLDs. Here are two suggestions I could think of for tackling this: - /\..*[^.]$/.test(domain) - !domain.endsWith(".") && domain.includes(".")
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: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. https://codereview.adblockplus.org/29785581/diff/29793585/lib/antiadblockInit... lib/antiadblockInit.js:58: if (/\.[^$]/.test(domain) && !publicSuffixes.hasOwnProperty(domain)) 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.
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." Argh, sorry that should have been "no-ip.co.uk" and "no-ip.co.uk."!
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! 
|
