Closed Bug 282270 Opened 16 years ago Closed 16 years ago
Display IDN urls as punycode by default (pref controlled)
Until we have a better solution, firstname.lastname@example.org wish network.enableIDN to be turned off for Firefox 1.0.1 and Mozilla 1.8 beta. IDN should end up disabled for anyone who updates to these releases, although if they subsequently turn it on themselves (either using about:config or the XPI in bug 282269) then it should stay that way. Gerv
Blocking the upcoming releases. *Not* blocking 1.1 because we're hoping to have the IDN issues resolved enough to turn it back on then, but in any case if this is checked in for 1.8b it'll be that way for 1.1
What about Mozilla 1.7.6? Looks like dveditz marked blocking1.7.6+, but you only mentioned 1.0.1 and 1.8b.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.8beta2
Why do you not make a messagebox or other sign, if the browser is on a xn-- Domaine ? Then is the visit possible and not blocked the Idea of IDE
And if enable IDN with about:config, no user make this. When, make in the Preferences a Checkbox to enable. If meaningfully when is the standard ON
This does include 1.7.6. To be clear: we need code which disables IDN, but remembers the original state, so we can later have code which restores it, when and if we have a longer-term solution to the IDN problem. But we need a way of people manually re-enabling it if they wish to. This is not just a case of setting the default in prefs.js, because that will apply only to new profiles. So, I suggest the following implementation strategy: - Change the pref which the IDN subsystem checks to be "network.enableIDN2". - Default that pref to "false" in prefs.js - Make the XPI in bug 282269 toggle that pref, not another one. This preserves the original value of the network.enableIDN pref by the simple expident of not touching it at all. When we have a better solution, we just change the IDN system back to checking the network.enableIDN pref, and the network.enableIDN2 pref gets abandoned. Gerv
What is the point of making things so complicated? Let's just change the default setting for network.enableIDN to false. If people switch that to true in their own profiles, then it won't matter if a future version of Firefox also has the default value switched to true. As a side note: I think we should make the pref system preserve user choices in prefs.js even if those user choices match the default choices, and that's something we could fix for Firefox 1.1 as well. But, I don't think that that "bug" is a concern here.
Darin: this patch protects none of our 25,000,000+ current users! It only changes the default value for new profiles. It also means that, when and if we want to re-enable IDN support, we won't know what to do. In a particular profile, is IDN off because we switched it off (in which case, we should switch it back on), or because the user switched it off (in which case, we shouldn't)? With this proposal, there's no way to tell. In the future, we can't go around re-enabling IDN for people who've turned it off explicitly. We need to respect their choice, and so we need to distinguish between the two cases. Gerv
> Darin: this patch protects none of our 25,000,000+ current users! It only > changes the default value for new profiles. I don't follow. The users preferences are overlaid on top of the default set. So, assuming that users have not changed the default value of the pref to false (i.e., that they did not hand edit all.js), their profiles would not have the value set to true. So, changing the default value to false, would affect all existing profiles as desired. > It also means that, when and if we want to re-enable IDN support, we won't > know what to do. I believe that we should enable IDN when we have proper anti-spoofing measures in place. Yes, this means changing the default value of the preference back to true. I understand your concern about giving people a way to say that they don't want IDN when they install Firefox 1.x (x > 0), but that seems like a different problem. Perhaps Firefox 1.x should introduce the user to IDN when first encountered on the web or something like that so that they can make a decision at that time about whether or not they want IDN in Firefox 1.x. Introducing additional prefs at this time seems like unnecessary complication. If any users have set network.enableIDN to false now it is because they don't want IDN now. Their choice will be preserved when they install Firefox 1.0.1, and like I said we should not enable IDN in Firefox 1.x without explaining what that means to the user and letting them choose at that time.
> The users preferences are overlaid on top of the default set. Ah, sorry - that's the bit of info I was missing. I thought all.js was the initial defaults, and got copied into the profile, and was then overwritten/modified as users changed prefs. So your solution is fine, then :-) Gerv
Comment on attachment 174455 [details] [diff] [review] v1 patch >-pref("network.enableIDN", true); >+pref("network.enableIDN", false); Just to make sure everything is clear on this default pref business (the comments show some confusion) changing this default means *everyone* will have IDN turned off by the next release. Our pref system "optimizes" by deleting user settings that match the default. This causes problems, for example, sharing profiles between Netscape and Mozilla Suite: any pref that has different product defaults will end up removed from your profile so you will get the differing product defaults rather than your preferred setting. Or switching back and forther between older and newer versions (such as testing the trunk, using the last stable) when we change a default value. In this IDN case setting this to false will set everyone to false. Anyone who currently has explicitly chosen false will have their setting "optimized" away, anyone who had the default true will get the new default false. When we turn this pref back on in a future release it will be turned back on for everyone, even the people who had explicitly turned it off prior to our upcoming releases. This should be OK: these people turned it off because of the spoofing problem and we will presumably have a solution to that before turning it back on. Some number of them may wish it to remain off and they may be surprised to find that it is not. Worse, they may trust a spoof because they remember killing the feature so it "can't" be an IDN domain. We could avoid this by changing the pref name as Gerv suggests (need changes in nsStandardURL.cpp, nsDNSService2.cpp, and nsHttpHandler.cpp -- this constant should have been in a shared header), but this simpler solution is the better one for most people. r/sr=dveditz a=dveditz on behalf of drivers, we discussed this and this is what we want.
Attachment #174455 - Flags: superreview?(dveditz)
Attachment #174455 - Flags: superreview+
Attachment #174455 - Flags: review?(dveditz)
Attachment #174455 - Flags: review+
Attachment #174455 - Flags: approval1.8b+
Attachment #174455 - Flags: approval1.7.6+
Attachment #174455 - Flags: approval-aviary1.0.1+
do note that in case anyone has this pref in user.js, then that setting will not be affected.
I heard some reservations if just turning off the pref will stop links to IDNs of working and if it will stop us sending IDN names to proxies which resolve them. Does that help in those cases? If it doesn't, I fear it's a less-than-half solution where we want a full solution :(
This patch disabled IDN in a slightly different way. It makes it so that we convert any unicode hostname to punycode, so that HREFs using IDN will continue to function. It also eliminates the need to worry about IDN aware proxies. To avoid the spoofing problems, this patch makes it so that we never convert from punycode back to unicode. A preference is introduced that allows users to enable full support for IDN.
Comment on attachment 174547 [details] [diff] [review] v2 patch >+#define NS_NET_PREF_IDNTESTBED "network.IDN_testbed" >+#define NS_NET_PREF_IDNPREFIX "network.IDN_prefix" Were these preferences previously broken or working? (That depends on initialization order, right? i.e., whether the IDN service was created before the profile was selected?) I'm not crazy about the idea of making them work if they were broken before. >+ PRBool mShowPunyCode; Punycode is one word, so the C shouldn't be capitalized. Feel free to roll in the fix to bug 282520 if you want. I'll look in more detail later.
Comment on attachment 174547 [details] [diff] [review] v2 patch A few more thoughts: While this has the advantatage of not breaking links, bookmarks, or international *input* in the URL bar, does it break cookies? If so, is there an easy fix? The previous patch should be backed out when this is landed (if that patch has landed).
> While this has the advantatage of not breaking links, bookmarks, or > international *input* in the URL bar, does it break cookies? If so, is there > an easy fix? The cookie database should be storing ASCII valued hostnames only, so this should not impact it. That doesn't mean that it doesn't, so I'll need to test that carefully before proceeding with this patch. > The previous patch should be backed out when this is landed (if that patch has > landed). The previous patch did not land.
The prefs were broken in exactly the same way that they were broken for network.enableIDN. See bug 261934. I'm fixing the same bug with these preferences.
Why not simply display all non ISO-8859-1 (or whatever a user considers safe) characters in RED wherever the URL is shown. That way it would be clearly visible when some domain name contains cyrillic characters that look like latin characters or other tricks like that.
haferfrost: this bug report is about getting a simple stopgap measure in place ASAP. bug 279099 is about creating a better auti-spoofing solution for IDN.
Comment on attachment 174547 [details] [diff] [review] v2 patch So I guess my one other comment is that I'm a little uncomfortable with changing ConvertACEtoUTF8 to do something other than what it says. Why was that needed?
Attachment #174547 - Flags: review?(dbaron) → review+
<darin> dbaron: ConvertACEtoUTF8 needs to be hacked because we don't want to ever perform that conversion. doing so means that we might show UTF8 instead of ACE. <darin> ASCII is a subset of UTF-8 so we aren't violating the interface types <darin> we are just violating the interface contract <dbaron> ok <darin> this might impact global history <darin> but that is something i'm willing to accept * dbaron marks r+
Comment on attachment 174455 [details] [diff] [review] v1 patch marking patch obsolete. we don't want this one.
Attachment #174455 - Attachment is obsolete: true
Comment on attachment 174547 [details] [diff] [review] v2 patch Do you want to add an observer for the new pref? or is it temporary so we don't care? I would think that even in the future, some paranoid group will want to use that pref. sr=dveditz a=dveditz for branches seek a= from #drivers for 1.8b1, it may be too late
revised version of the v2 patch that was checked in on the trunk for 1.8b1. i'll land this same patch on the 1.0.1 and 1.7 branches shortly.
Changing summary to reflect bug morph.
Summary: Turn off network.enableIDN on branch and trunk → Display IDN urls as punycode by default (pref controlled)
Whiteboard: need checkin
tested on 2005021714-trunk seamonkey Mac build: both the network.enableIDN and network.IDN_show_punycode are now true by default. and making a change to either of these prefs (via about:config) persists between sessions (as should be expected).
see bug 279099 for the root problem.
test site: http://www.shmoo.com/testing_punycode/ when network.IDN_show_punycode is true, mouse over the two pseudo paypal links and look at the statusbar: it should display www.xn--pypal-4ve.com but when you turn that pref to false, mousing over those links will (deceptively) show www.paypal.com vrfy'd fixed using 2005021714-trunk seamonkey Mac --tracy or jay, could you quickly check this on windows and linux?
verified on Windows also
Status: RESOLVED → VERIFIED
darin: why not remove the IDN_testbed support since Verisign is no longer doing that? This is really a short-term hack that I believe does not need to stay in the codebase.
> darin: why not remove the IDN_testbed support since Verisign is no longer doing > that? This is really a short-term hack that I believe does not need to stay in > the codebase. Wil, yeah.. i had the same thought. for the stable mozilla branches, i decided that'd it be better not to remove those. but, i don't object to cleaning it up for the trunk. i'll file a bug.
see bug 282821
This fix is responsible for bug 283992.
This is assigned to SA14163, marked as "Vendor Patch" in Mozilla 1.7.6 and Thunderbird 1.0.2. It was handled as patched earlier in Firefox 1.0.1 too, naturally: http://secunia.com/advisories/14163/
Will Thunderbird eventually show punycode in its status bar? I'm currently using TB 220.127.116.11 and this does not work (even if I enable both network.IDN_show_punycode and network.enableIDN). At the risk of telling you something that you already know, I think that there currently exists a security risk in Thunderbird. When I hover my mouse over an IDN link in an HTML email, the status bar shows some of the unicode characters of the link as ASCII, and it hides other unicode characters, so the user would be fooled into clicking on the link. For example, Thunderbird shows this link: http://www.theshmo%d0%begroup.com/ as http://www.theshmoogroup.com
Here's another example. This one uses the &#...; format instead of the %number %number format. If this is in the HTML of the email: <a href='http://www.theshmoоgroup.com/'>Click here to enter TSG</a> Then Thunderbird again shows this in the status bar: http://theshmoogroup.com/ If I click on the link in the email, Firefox is launched and it tries to take me to www.theshmo%d0%begroup.com. Well, that's true if I save the email with UTF-8 encoding. If I use the "current character encoding" instead of UTF-8, a question mark appears in the status bar: http://www.shmo?group.com/. In this case, clicking on the link does nothing. Firefox is not launched.
You need to log in before you can comment on or make changes to this bug.