400552 - setting document.domain inconsistent in face of IDN whitelist

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Daniel Veditz [:dveditz]]

The rules for setting document.domain are enforced by string matching against the current domain, which is originally set based on the URI as normalized by the IDN rules.

Code that works at "www.müller.de" may or may not work depending on whether the current browser supports IDN or not, and would require different code from "www.müller.com" (which will be "www.xn--mller-kva.com" in Firefox). If we add or remove a TLD from the IDN whitelist the site would also have to adjust their code.

A site author could check document.domain first to see whether it's an IDN domain or not, and given differing support in different browser perhaps they have to do that anyway.

http://www.müller.de
--------------------
 document.domain = "müller.de";       // works
 document.domain = "xn--mller-kva.de" // throws

http://www.xn--mller-kva.com
----------------------------
 document.domain = "müller.com"        // throws
 document.domain = "xn--mller-kva.com" //works

Setting document.location works fine for all four versions.

Comment 1

[[e-mail]]

have patch in hand for this.

Comment 2

[[e-mail]]

Attachment: v1

this moves the domain checks down to after the new URI is constructed, at which point we can get the host of both URI's and compare normalized strings safely. as far as i can tell, this shouldn't affect any other behavior. it also saves on a bit of string conversion (though it will make the failure case take longer, since we now have to construct a URI first).

dveditz pointed out the whatwg spec for document.domain at http://www.whatwg.org/specs/web-apps/current-work/#domain, which doesn't make mention of normalization concerns. so, we shouldn't be breaking the spec by doing this. i'd note though, as an unrelated thing, that the spec states that setting the domain to null if it's already null should succeed (if i'm reading it right) - whereas currently we error out unconditionally if the new domain is null.

Comment 3

[Boris Zbarsky [:bzbarsky]]

We should probably raise the normalization issue with the whatwg, though, just so they do cover it.  

Comment 4

[Daniel Veditz [:dveditz]]

the whatwg spec also allows our previous behavior of letting the domain be plain "com" (or other TLD). There ought to be at least a security note as in the global storage case saying that some UA's don't allow it for security reasons.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 286234 [details] [diff] [review]
v1

r+sr=jst

Comment 6

[[e-mail]]

Comment on attachment 286234 [details] [diff] [review]
v1

requesting approval1.9.

Comment 7

[Johnny Stenback (:jst)]

I added this to http://wiki.mozilla.org/Firefox3/Beta2CheckinQueue#Non-Blockers to get this landed...

Comment 8

[[e-mail]]

checked in.

Comment 9

[Jeff Walden [:Waldo]]

I assume you'll want to add some new domains to the list at the following URL to test this:

http://mxr.mozilla.org/mozilla/source/testing/mochitest/runtests.pl.in#103

Ideally if you can do everything as (sub)+domains of example.com or something that'd be best, but if you need anything different it's probably fine so long as I at least know about it beforehand.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 286234 [details] [diff] [review]
v1

>Index: content/html/document/src/nsHTMLDocument.cpp

>+    // try to get the base domain; if this works, we're ok.
>+    // if we're dealing with an IP address, getting the base domain
>+    // will fail, as required.
>+    nsCAutoString baseDomain;
>+    ok = NS_SUCCEEDED(tldService->GetBaseDomain(newURI, 0, baseDomain));

This is wrong; |document.domain = "0.0.1"| for 127.0.0.1 means newURI has a host of 0.0.1, which is not an IP address and which does not cause this to fail.  Should have tested!  I filed bug 414812 to fix this issue.