283992 - keyword search for exactly one word with non-ASCII characters (umlauts and accentuated characters) doesn't work due to punycode-conversion

Status: RESOLVED FIXED

(Firefox :: Address Bar, defect)

Description

[Timo Baumann]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.7.6) Gecko/20050223 Firefox/1.0.1

searching for single words from the location bar does not work when they contain
umlauts (äöüÄÖÜ) or accentuated characters (I only tried a few but none of them
worked) because they are converted to punycode.
There is no problem in searching for words containing these characters if there
is more than one word that is searched for (e.g. type "à bientôt" in the
location bar) or if the keyword-search is evoked directly by typing
"keyword:Fahrvergnügen".

It seems that whenever the content typed into the location bar might be a domain
(that is doesn't contain any space characters) it is punycode-converted looked
up in the DNS. When the DNS-lookup for this fails, keyword search is invoked.
The problem is, that keyword-search should always search for what the user has
entered into the location bar, not only when more than one word was entered.
Punycode-conversion is wrong, because there are no security-concerns that users
are spoofed into doing "unsafe" searches :o)

Reproducible: Always

Steps to Reproduce:
1. type "Fahrvergnügen" into the location bar and hit enter



Actual Results:  
The location bar displays "keyword:xn--fahrvergngen-llb" and the page configured
to show up for this keyword-search is loaded (for me this is google). The
results are useless, because I didn't want to search for xn--fahrvergngen-llb
and what I looked for is not displayed.

Expected Results:  
The keyword-search should show results for "Fahrvergnügen" and the location bar
should display "keyword:Fahrvergnügen".

This bug is a regression from Firefox 1.0.

Comment 1

[Timo Baumann]

the character-encoding of my bug-report is UTF-8. So you might need to change
the way the page is displayed if you see funny characters but no umlauts or
accentuated characters...

Comment 2

[OstGote!]

Related to the fixing of Bug 282270 I guess.

Comment 3

[Jungshik Shin]

Yes, this has been bugging a lot of non-English speakers. An easy thing first. A
work-around is to keep the new punycode option 'true' (as is shipped) and turn
off 'browser.fixup.alternate.enabled'). 

What's going on is:

alternate fix-up kicks in if a single word is typed in the address bar AND the
word is purely ASCII. Otherwise, it's passed along to google 'I feel lucky' (or
whatever search address is set for browser...search.defaulturl..)

With 'punycode' option on, even non-ASCII words are turned into ASCII word when
this decision point is reached. Either we have to revert to the original string
at this branch point or  we have to check if it's IDN in punycode instead of
just using |IsASCII()|  

This should block aviary 1.0.2, but I can't even ask for it.

Comment 4

[Jungshik Shin]

*** Bug 284012 has been marked as a duplicate of this bug. ***

Comment 5

[Jungshik Shin]

(In reply to comment #3)

> this decision point is reached. Either we have to revert to the original string
> at this branch point or  we have to check if it's IDN in punycode instead of
> just using |IsASCII()|  

We have to revert to the original string before passing it along to google.

Comment 6

[Jungshik Shin]

My analysis was wrong. alternate-fixup is tried AFTER the keyword server look-up
so that turning off the former doesn't work around the problem (I was using 1.0
while thinking it's 1.0.1)

Comment 7

[Jungshik Shin]

In bug 282270, ConvertACEtoUTF8 was changed to depend on
network.IDN_show_punycode . There's no way for callers to override it. One way
to solve this problem is add a method to the interface (nsIIDNService which is
not yet frozen), ForceConvertACEtoUTF8 which doesn't take into account the pref.  

Darin, what do you think? This is not pretty....

An alternative is to add a property (|showPunycode|) to the interface, in which
case each caller has to check the pref. and set the property accordingly before
calling ConvertACEtoUTF8.  Or, perhaps we can just add a second parameter to
ConvertACEtoUTF8. I don't particularly like these two, either...

Comment 8

[Jungshik Shin]

An easy work-around is to prepend a single-word keyword with '?' like '?xyzabc'. 

Comment 9

[Gervase Markham [:gerv]]

Oh dear :-(

Any fix will hopefully only be necessary for the 1.0.x branch; we should have a
better solution to the whole problem for 1.1.

Gerv

Comment 10

[Jungshik Shin]

Attachment: patch for av 1.0.1 branch

This is not pretty, but takes the least effort(?)...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176341 [details] [diff] [review]
patch for av 1.0.1 branch

I'd be happier with not changing interfaces on this api-stable branch... But
it's darin's call on whether it's better to add a new "temporary" interface.

That said, no need to do the IsAscii check in ConvertACEtoUTF8() --
ForceConvertACEtoUTF8 will do it for you.

Comment 12

[Reuven Gonen]

I'm having this problem with version 1.0.1, but it seems to be working with a
trung build from:

Mozilla/5.0 (Windows; U; Windows NT 5.1; he-IL; rv:1.8b2) Gecko/20050302
Firefox/1.0+

can you check this with a trunk build and see if the problem still exists?

Comment 13

[Jungshik Shin]

No, it doesn't work with my trunk build with the source pulled half an hour ago. 
I have a patch for the trunk as well. They're almost identical to attachment
176341 [review]. I also hope we won't have to land it in the trunk. 

Comment 14

[Darin Fisher]

Comment on attachment 176341 [details] [diff] [review]
patch for av 1.0.1 branch

I think we should consider moving that IDN preference out of nsIDNService and
into nsStandardURL.  It seems like our low-level API should not be dependent on
a pref setting like this.  The only reason that I wrote the code the way I did
was because I wanted to ensure for Firefox 1.0.1 that we were absolutely
consistent in our handling of IDN.  Now, with this new API, we are back to a
situation where ACE may in some cases be auto-converted to UTF-8, and thus we
will need to carefully audit all uses.	So, there is less reason to put the IDN
spoofing fix in nsIDNService itself.

Comment 15

[Darin Fisher]

Worst case you can get the pref service and toggle the preference to make
ConvertACEToUTF8 work temporarily.  That requires no API changes, just some pref
juggling.

Comment 16

[Jungshik Shin]

Attachment: an alternative (with pref. juggling)

This just fixes up webshell as darin suggested. It's for trunk but should be
applicable to branch with a little change (it'll be rejected, but should be
easy enough to apply by hand)

Comment 17

[Gervase Markham [:gerv]]

For the trunk, surely we should be doing the correct fix rather than fixing up
the quick fix?

I think it's pretty clear that, whatever the exact outcome of the current
discussions about IDN, we're going to need a TLD whitelist or blacklist, at
least in the short term. Untrusted TLDs should display as punycode, as now. Does
it not make sense to set about implementing this mechanism rather than trying to
fix up the working of the temporary pref?

Gerv

Comment 18

[Darin Fisher]

I think for this bug we should be looking to get the original input from the
user instead of trying to deduce it from the value of nsIURI::host.  How do we
know that the user didn't type "xn--foopy"?  Maybe they really wanted to do a
search on it.  So, it seems like this fixup code should somehow retain access to
the originally entered text.

Comment 19

[Darin Fisher]

Comment on attachment 176447 [details] [diff] [review]
an alternative (with pref. juggling)

in order for this patch to be viable, you'd need to read the existing value of
the pref, set the pref to false, and then restore the old value of the pref. 
otherwise, you might trample over the user's preference setting.  maybe they
don't want it to be true.

Comment 20

[Darin Fisher]

again, maybe the other thing we could do for the 1.0.2 release is to move the
pref from nsIDNService to nsStandardURL.  that change might be better, but it
means double checking every caller of ConvertACEToUTF8, which currently is only
nsStandardURL :-)

Comment 21

[Jungshik Shin]

(In reply to comment #19)
> (From update of attachment 176447 [details] [diff] [review] [edit])
> in order for this patch to be viable, you'd need to read the existing value of
> the pref, set the pref to false, and then restore the old value of the pref. 

That's exactly what I'm doing. I don't explicltly restore the old value if the
existing value was false, but the effect is the same. 

> otherwise, you might trample over the user's preference setting.  

> maybe they  don't want it to be true.

 Of course, they don't so that I set it back to true ONLY when the existing
value I read was true.


Or, am I missing something here?

Comment 22

[Darin Fisher]

Comment on attachment 176447 [details] [diff] [review]
an alternative (with pref. juggling)

so sorry!  i read over the patch way too fast last time.  r=darin

Comment 23

[Jungshik Shin]

Comment on attachment 176447 [details] [diff] [review]
an alternative (with pref. juggling)

thanks for r. 
asking for sr.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 176447 [details] [diff] [review]
an alternative (with pref. juggling)

sr=bzbarsky for branch, and for trunk if we get desperate for this next
release.....

Comment 25

[Darin Fisher]

Comment on attachment 176341 [details] [diff] [review]
patch for av 1.0.1 branch

minusing this patch.  i'd prefer moving the pref handling into nsStandardURL
over this patch.

Comment 26

[Jungshik Shin]

Attachment: patch for branch

the same as attachment 176447 [details] [diff] [review], but this is for branch (the code around the
patched area is slightly different so that attachment 176447 [details] [diff] [review] doesn't get
applied to the branch)

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 178710 [details] [diff] [review]
patch for branch

not appropriate for the 1.0.3 security release

Comment 28

[Adam Hauner]

dveditz: ... but this bug is very visible regression from previous security
release. Could this fact change your decision?

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It's too late for 1.0.3, but we should consider it for 1.0.4 and 1.7.8.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Given that this hasn't landed on the trunk yet, it's not safe enough for a
security release.  But we should consider it for the next one if it's had enough
trunk testing.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, actually, it looks like it has landed on the trunk, on March 16.  I really
shouldn't have to do CVS archaeology to figure that out when looking at an
approval request.  Marking fixed on that basis, and adding approval requests for
the next branch releases (I think we should take it given that it's had almost 3
weeks of trunk testing).

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(Though I have to say, I actually prefer something more like jshin's original
patch for the trunk -- making the methods on nsIIDNService not do what they say
they do really is a rather bad idea.)

Comment 33

[Jungshik Shin]

I'm sorry that I forgot to mark this as fixed (I perhaps kept this open because
we need a better solution for the trunk. Nonetheless, I should have noted that
the patch had been landed). I was about to ask for reconsideration with details
including the fact that it had been in the trunk, but you guys beat me to that.
(I should have given those details when I asked for a1.0.3). Anyway, this has
little security implication. If this is a security risk, google is a security
risk, too :-) 

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, the criteria for the Aviary 1.0.x dot releases are essentially:
 * security bugs
 * regressions from previous security fixes in the Aviary 1.0.x series

This is a pretty major bug in the second category.

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 178710 [details] [diff] [review]
patch for branch

Change of plans for 1.0.3, so we're going to take this.  I plan to land this on
the aviary and mozilla1.7 branches unless somebody tells me it shouldn't go on
mozilla1.7 as well.

Comment 36

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Checked in to AVIARY_1_0_1_20050124_BRANCH and MOZILLA_1_7_BRANCH.

Comment 37

[Chase Phillips]

Clearing blocking-aviary1.1 flag.