Fix domain_is_in? for deeply nested subdomain

[cm-dyoshikawa]

@micke

Hi,
Thank you for reviewing and merging my previous PR.

I found that there are some email address patterns that cannot be detected when using domain_is_in? in disposable_domain?.

For example, when disposable_domain.com is included in the blocklist:

• foo@disposable_domain.com
• [email protected]_domain.com

can be detected, but:

• [email protected]_domain.com

with two or more nested subdomains cannot be detected.

Therefore, I modified domain_is_in? to detect deeply nested subdomains as well.
I've also added RSpec test cases for this.

Thanks.

[cm-dyoshikawa]

This has been automatically fixed by my code editor. Removing unnecessary spaces.

[phoet]

hmhmmm, i guess it has always been truethy? i mean domains typically contain a dot. 🤔

[phoet]

@micke was it on purpose, that in any case, the TLD so com from example.com was checked against the list? i guess we can speed up the lib significantly by getting rid of this.

[cm-dyoshikawa]

To me as well, the TLD check seems unnecessary.

Shall I remove the TLD check while we have this opportunity? I think it's possible.

[cm-dyoshikawa]

The implementation might look something like this:

      domain_hierarchy_level = 0
      while (i = address_domain.index('.')) && domain_hierarchy_level < @domain_hierarchy_max_depth
        address_domain = address_domain[(i + 1)..-1]
        # Skip checking TLD (if there's no more dot, it's considered as TLD)
        break unless address_domain.include?('.')
        return true if domain_list.include?(address_domain)
        domain_hierarchy_level += 1
      end

This implementation includes this discussion.

[phoet]

i have not tested this thoroughly, but maybe you get the basic idea here and can finish it off

    def domain_is_in?(domain_list, max_depth = 3)
      address_domain = address.domain.downcase
      return true if domain_list.include?(address_domain)

      tokens = address_domain.split('.')
      return false if tokens.length < 3

      limited_sub_domain_part = tokens.reverse.first(max_depth).reverse.join('.')

      domain_list.include?(limited_sub_domain_part)
    end

there is no iteration over parts, so this is faster and we get rid of the while.

[phoet]

i've seen so many dead systems because of a broken while that i would try to get rid of it in any library implementation.

[phoet]

maybe it would be good if this was actually a configurable option like max_depth = 3 on the domain so subdomains deeper that x would just be ignored and the rest would need to be configured individually in the disposable list?

[cm-dyoshikawa]

Thx, I understand your concern. I think it's possible.

How about an interface like this?

-   def initialize(address, dns_timeout = 5, dns_nameserver = nil)
+   def initialize(address, dns_timeout = 5, dns_nameserver = nil, domain_hierarchy_max_depth: 3)
      @parse_error = false
      @raw_address = address
      @dns_timeout = dns_timeout
      @dns_nameserver = dns_nameserver
+     @domain_hierarchy_max_depth = domain_hierarchy_max_depth

[cm-dyoshikawa]

The implementation might look something like this:

      domain_hierarchy_level = 0
      while i = address_domain.index('.') && domain_hierarchy_level < @domain_hierarchy_max_depth
        address_domain = address_domain[(i + 1)..-1]
        return true if domain_list.include?(address_domain)
        domain_hierarchy_level += 1
      end

[phoet]

👍 great you added tests for this.

[cm-dyoshikawa]

Thank you 🙌