364129 - nsIEffectiveTLDService::getEffectiveTLDLength returns a misleading length in case of IP addresses

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Dão Gottwald [:dao]]

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 72.14.221.104, 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

Comment 1

[Dave Camp (:dcamp)]

Attachment: Return 0 for a dotted decimal hostname

This patch adapts IsNumericHostname from the places code, and returns 0 if a dotted decimal hostname is passed in.

Comment 2

[Wladimir Palant]

Shouldn't this consider IPv6 addresses? Maybe use PR_StringToNetAddr instead of IsNumericHostname?

Comment 3

[Wladimir Palant]

Another objection: why should it return 0 in this case? That would effectively allow the site 1.2.3.4 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".

Comment 4

[Wladimir Palant]

Attachment: More general patch and testcase

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 5

[Christian :Biesinger (don't email me, ping me on IRC)]

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?

Comment 6

[Wladimir Palant]

Attachment: Patch with trailing dot accounted for

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.

Comment 7

[[e-mail]]

using an nsCAutoString here is far superior to malloc'ing. like this:

nsCAutoString hostname(aHostName);
if (hostname.Last() == '.')
  hostname.Truncate(hostname.Length() - 1);

Comment 8

[[e-mail]]

(you can also just use hostname.Trim('.') to remove any and all leading and trailing dots.)

Comment 9

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 10

[benc]

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?

Comment 11

[Wladimir Palant]

Can you provide a concrete use case?

Comment 12

[[e-mail]]

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.

Comment 13

[[e-mail]]

-> me

Comment 14

[Damon Sicore (:damons)]

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.

Comment 15

[[e-mail]]

fixed per bug 368989.