427957 - Mixed direction IDN doesn't work (right-to-left and left-to-right characters)

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[Michael Schonfeld]

Works in Safari & FF2, but not in FF3. Thus, Regression from FF2.

Trying to load http://www.מיץפטל.com/ produces an "Address Not Found" page.

Expected result:
Should load the page (forward URL to http://www.xn--debbkf5af.com/).

Comment 1

[Mike Connor [:mconnor]]

-> Core::Internationalization

Comment 2

[Simon Montagu :smontagu]

The testcases at http://idn.icann.org/#The_example.test_names still work, including Hebrew and Arabic script, so the problem is more specific.

Regression is between 2007-12-03-02 and 2007-12-04-02, which suggests bug 402008.

Comment 3

[Simon Montagu :smontagu]

The problem seems to be that we are validating the domain name as a whole according to RFC 3454, not the individual labels as RFC 3490 specifies.

The bidi restrictions in RFC 3454 prohibit mixing right-to-left and left-to-right characters in the same string, so מיץפטל is valid, but מיץפטל.com is not.

Comment 4

[Behnam Esfahbod [:zwnj]]

Same problem with http://ثبت‌دامنه.ایران.ir/

Comment 5

[Simon Montagu :smontagu]

Attachment: Patch

Comment 6

[[e-mail]]

Comment on attachment 314935 [details] [diff] [review]
Patch

nice catch - although i'm curious why bug 402008 regressed this, since it just refactored code and didn't really change behavior. anyway, thanks for patching :)

>Index: netwerk/dns/src/nsIDNService.cpp
>===================================================================
>+  PRUint32 len = 0, offset = 0;
>+  nsresult rv;
>+  nsAString::const_iterator start, end;
>+  inUTF16.BeginReading(start);
>+  inUTF16.EndReading(end);
>+  outUTF16.Truncate();

fwiw that .Truncate() isn't necessary, the string was just declared above.

Comment 7

[Simon Montagu :smontagu]

(In reply to comment #6)
> i'm curious why bug 402008 regressed this, since it just
> refactored code and didn't really change behavior.

I suppose it exposed it rather than causing it, but fwiw the crucial difference is the change from:

-    if (gShowPunycode || NS_FAILED(gIDN->Normalize(host, temp)))
-        return gIDN->ConvertUTF8toACE(host, result);

to:

+    rv = Normalize(input, _retval);
+    if (NS_FAILED(rv)) return rv;

...since Normalize() had this bug and ConvertUTF8ToACE() didn't.

Comment 8

[Simon Montagu :smontagu]

and I guess that explains bug 355156, too :)

Comment 9

[Damon Sicore (:damons)]

Per blocking request, can you please give a few sentences explaining the impact
of this bug?  Sounds pretty bad, but we're asking ourselves whether or not
individual bugs would stop ship at this point.

Comment 10

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

Comment on attachment 314935 [details] [diff] [review]
Patch

+    if (*start++ == (PRUnichar)'.') {

PRUnichar('.')

+      outUTF16.Append((PRUnichar)'.');

here too

Comment 11

[Simon Montagu :smontagu]

(In reply to comment #9)
> Per blocking request, can you please give a few sentences explaining the impact
> of this bug?

It's a standards compliance issue (RFC 3490), and a regression, and it has the effect of wiping whole countries off the IDN map. I don't know how many bidirectional IDNs are actually out there, Behnam may have some data about that from his work at nic.ir, but the number is only going to grow. I really don't think we can ship with this bug.

And we do have a patch in hand with reviews and unit tests :)

Comment 12

[Behnam Esfahbod [:zwnj]]

Well, at nic.ir, we have more than 3'000 domains under .ایران.ir (.IRAN.ir) [1] which all are affected by this bug.

Beside that, GoDaddy has recently started selling IDN domains under .com and .net, which the domain in all Arabic, Persia, Urdu, and Hebrew languages would be unusable because of this bug. [2]

Also this bug do affect all IDN sub-domains under every LTR TLD, which might cause some Arabic/Hebrew dictionaries/encyclopedias stop working.

1: http://www.nic.ir/Statistics
2: Example: http://www.ایران.com/ (IRAN.com)

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

(In reply to comment #11)
> It's a standards compliance issue (RFC 3490), and a regression, and it has the
> effect of wiping whole countries off the IDN map. I don't know how many

Convincing!

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 314935 [details] [diff] [review]
Patch

a1.9=beltzner

Comment 15

[Simon Montagu :smontagu]

Checked in.