304905 - UnEscapeURIForUI should leave %HH in hostname escaped

Status: NEW

(Core :: Internationalization, defect)

Description

[Jesse Ruderman]

UnEscapeURIForUI should leave %HH in the hostname escaped to prevent spoofing. 
See https://bugzilla.mozilla.org/show_bug.cgi?id=246804#c14.  It should probably
only unescape the path&filename portion of the URI.

Comment 1

[Jesse Ruderman]

This is necessary to prevent spoofing, especially when Thunderbird is used with
a browser that doesn't have a fix for bug 304904 on Windows or Mac.

Comment 2

[Jungshik Shin]

I'm afraid that's not trivial. I tried to do different things on different
portions of a URL in another bug (to better support IDNs) and ended up creating
an infinite loop (for javascript URLs, iirc)...Maybe, we can exclude javascript
urls ...
 
I'm taking it anyway.

Comment 3

[Benjamin Smedberg]

If we get a fix for bug 304904 we're not going to block on this, since it seems
very regression-prne.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Doesn't this significantly reduce usability when valid escaped hostnames are actually used?

Comment 5

[Jesse Ruderman]

I thought the % character was always invalid in hostnames.  Didn't bug 304904 created a blacklist for characters in hostnames that includes "%"?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> I thought the % character was always invalid in hostnames.

Depends on what you mean by "hostname".  See bug 304904 comment 9 and bug 304904 comment 10.  I guess for now we don't handle that right, so this _might_ be OK as a (branch-only?) quick-fix, but we should really fix our code to handle it right.  Note also bug 304904 comment 13, which says much the same thing.

Also note that the actual patch for bug 304904 first unescapes the hostname, _then_ checks for invalid characters.  See bug 304904 comment 24 and bug 304904 comment 55 and the patch that was checked in.  So yes, '%' is invalid in a hostname _after_ unescaping has occurred.  Don't put %25 in your hostname.

Comment 7

[Daniel Veditz [:dveditz]]

sort of related to the IDN/dns-spoofing bugs Neil Harris is working on. The UI presentation should match our internal interpretation. 

Comment 8

[Simon Montagu :smontagu]

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

Comment 9

[Jungshik Shin]

Perhaps, we should escape it and then convert to punycode if unescaped hostname turned out to be unsafe according to the criteria we use to determine whether to use punycode in the address bar. 

Masayuki, you have a patch changing this part of the code, don't you? Why don't you take this? 

Comment 10

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Yes. I was working on bug 320807 that is separating the URI for each parts, and they are decoded on each parts. In the patch, the pre path (host, username and password) is not decoded for security.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Note that I'll restart the work for bug 320807 after a2 or b1. Because we need very many works for text rendering on thebes.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note bug 309671.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months, so the assignee is being reset.