search

danesparza / domainname-parser

:department_store: .NET domain name parsing library (uses publicsuffix.org)
MIT License
35 stars 19 forks source link

Parsing russian.cntv.cn domain #4

Closed StanislavPrusac closed 8 years ago

StanislavPrusac commented 9 years ago

If you have "russian.cntv.cn" for parsing in line: tldIndex = domainString.IndexOf("." + MatchingRule.Name, StringComparison.InvariantCultureIgnoreCase);

https://github.com/danesparza/domainname-parser/blob/master/DomainName.Library/DomainName.cs#L145

you will get: tempSudomainAndDomain = "russian" TLD = "cntv.cn"

Is this error?

danesparza commented 9 years ago

Looks like .cn exists in publicsuffix.org -- and I don't think that the behavior you described is expected. So yeah -- I think it's a bug.

alexwarren commented 9 years ago

The bug occurs with any domain name that matches the start of a TLD.

For example with www.comscore.com, domainname-parser thinks the TLD is comscore.com, because it looks for the first index of .com in the string.

It looks like the fix would be to update line 145 of DomainName.cs to use LastIndexOf instead of IndexOf?

danesparza commented 9 years ago

Yep. I think changing this code would probably be the right approach. We should probably add this edge case to the unit tests.

@alexwarren :star: Great catch!

danesparza commented 9 years ago

Wow -- it's been a while since I looked at those unit tests, too. They should be refactored to use Arrange / Act / Assert.