Closed Bug 364129 Opened 17 years ago Closed 16 years ago
IEffective TLDService::get Effective TLDLength returns a misleading length in case of IP addresses
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061215 Minefield/3.0a1 Build Identifier: A return value of 3 doesn't really make sense for 188.8.131.52, does it? There's also no rule in nsIEffectiveTLDService saying that only non-IP hosts should be passed. I would expect that getEffectiveTLDLength returns 0, -1 or throws an exception -- however, it shouldn't return an otherwise valid length of a nonexisting effective TLD. Reproducible: Always
Summary: nsEffectiveTLDService::getEffectiveTLDLength shouldn't return a misleading length in case of IP addresses → nsEffectiveTLDService::getEffectiveTLDLength returns a misleading length in case of IP addresses
Summary: nsEffectiveTLDService::getEffectiveTLDLength returns a misleading length in case of IP addresses → nsIEffectiveTLDService::getEffectiveTLDLength returns a misleading length in case of IP addresses
Status: UNCONFIRMED → NEW
Ever confirmed: true
This patch adapts IsNumericHostname from the places code, and returns 0 if a dotted decimal hostname is passed in.
Shouldn't this consider IPv6 addresses? Maybe use PR_StringToNetAddr instead of IsNumericHostname?
Another objection: why should it return 0 in this case? That would effectively allow the site 184.108.40.206 to set document.domain to "4" - see bug 368700, this shouldn't be possible. We would also have the same problem with cookie domains once they start using EffectiveTLDService. The correct return value for dotted decimal hostnames should be the length of the hostname - the whole thing is one "TLD".
Dave, I hope you don't mind that I take this bug. I agree with Dao, this bug needs to be fixed soon if we want to use EffectiveTLDService somewhere. This patch now uses PR_StringToNetAddr() to recognize IPv4 and IPv6 addresses as hostnames. It will return the entire length of the hostname in this case. Unfortunately, this will remain only a partial fix until bug 380543 is fixed. IPv4 addresses with a trailing dot aren't recognized (at the very least on Windows). That's what was already fixed for regular host names in bug 368702.
Comment on attachment 264634 [details] [diff] [review] More general patch and testcase please do document this in the IDL file. why use ToNewCString instead of PromiseFlatCString?
Attachment #264634 - Flags: review?(cbiesinger) → review+
From discussion in bug 380543 it seems that we better remove the trailing dot from IP addresses ourselves, just to be sure. Unfortunately, PromiseFlatCString cannot be used then. biesi, please check again whether you agree with these changes.
Flags: blocking1.9? → blocking1.9+
Target Milestone: --- → mozilla1.9alpha6
using an nsCAutoString here is far superior to malloc'ing. like this: nsCAutoString hostname(aHostName); if (hostname.Last() == '.') hostname.Truncate(hostname.Length() - 1);
(you can also just use hostname.Trim('.') to remove any and all leading and trailing dots.)
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
I'd like to make a plea for not making this service too heavily cookie oriented. There might be other contexts where what you want from an IPv4 address is the in-addr.arpa domain or handling by octet or handling by subnetmask. The idea that the whole IPv4 address is the effective TLD for an address might make sense for cookies, but possibly not anywhere else. Wouldn't you want to make the handling of data something like: - a function that identifies a "hostname" as an FQDN -or- IPv4 -or- IPv6 value. Cookies would use this before calling effectiveTLD. - a set of functions that a module could use to do various sub-processing of the specific data types?
Can you provide a concrete use case?
Comment on attachment 266246 [details] [diff] [review] Patch with trailing dot accounted for i've rolled the fix here into patch v3 in bug 368989, since i was in there anyway and this is related. obsoleting this patch for now.
Assignee: trev.moz → dwitte
Status: ASSIGNED → NEW
If bug 268989 does indeed fix this issue, please make sure it gets landed ASAP (already commented in 368989) and resolve this issue. Trying to get these beta blockers knocked out as soon as humanly possible. Thanks again for the help.
fixed per bug 368989.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.