412458 - Percent encodings in url bar inconsistent

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect, P1)

Description

[Mycroft Project (CC)]

From bug 397815...

Visit google.co.uk (etc)
Paste http://www2 and search.
Location bar reads:
http://www.google.co.uk/search?hl=en&q=http%253A%252F%252Fwww2&btnG=Google+Search&meta=

%253A should surely be %3A?
%252F should surely be %2F?

Place cursor in URL bar and hit enter.
Now searching for http%3A%2F%2Fwww2

Comment 1

[Simon Bünzli]

So, decodeURI isn't as thorough as decodeURIComponent (which still might not decode the last escaped character)...

OTOH we really might want to decode a URL's path only (i.e. ignore host, query and hash) so that this issue and similar data: issues (cf. this bug's URL) don't require too funky work-arounds.

Yet another alternative: what about trying to decode only ranges of known save characters (space, umlauts et al. and different scripts such as Arabic)? Then we wouldn't have to re-escape anything at all.

Comment 2

[Mycroft Project (CC)]

Well personally I don't want to see any user-friendly decoding here...

Now when I load 'xxx?a=blah blah blah'
I see the same in the URL bar but
REQUEST_URI is 'xxx?a=blah%20blah%20blah'

and if I go for 'xxx?a=blah blah%blah'
I see 'xxx?a=blah%20blah%blah' in the URL bar but
REQUEST_URI is 'xxx?a=blah%20blah%blah'

finally if I go for 'xxx?a=blah blah%25blah'
I see 'xxx?a=blah blah%25blah' in the URL bar but
REQUEST_URI is 'xxx?a=blah%20blah%25blah'

I would expect to see blah%20blah%20blah,blah%20blah%25blah and blah%20blah%2525blah respectively (I think... I'm thoroughly knotted know)

Comment 3

[Mycroft Project (CC)]

(In reply to comment #2)
>
> (I think... I'm thoroughly knotted know)
> 

'now' even... :)

Comment 4

[Dão Gottwald [:dao]]

I've just looked at the innards of decodeURI, and the fix for this should be pretty trivial (even though not pretty).

Comment 5

[[:Aleksej]]

If this is the same bug the Checkins link in bug 412425 suffered from, the severity should be major, IMO. I have also seen the bug with Wikipedia.

Comment 6

[Dão Gottwald [:dao]]

Attachment: patch

Comment 7

[Dão Gottwald [:dao]]

(In reply to comment #4)
> I've just looked at the innards of decodeURI

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla%2Fjs%2Fsrc%2Fjsstr.c&rev=3.183&cvsroot=%2Fcvsroot&mark=4556,4557#4556

Comment 8

[Dão Gottwald [:dao]]

Attachment: patch v2

This time, without breaking addresses like data:text/plain,Colon%253A.

Comment 9

[Simon Bünzli]

Dao: While fixing the %-related fall-out from bug 366797, couldn't you include the fix for bug 410726 in this one as well? Would save one of us some unbitrotting...

Comment 10

[Dão Gottwald [:dao]]

Well, I think this should block beta 3, but currently it isn't, so I don't want to add anything that could lengthen the review process.

Comment 12

[Duncan Loveday]

FWIW I just got the regression for this

OK in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011105 Minefield/3.0b3pre downloaded from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008-01-11-05-trunk/

Bug in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008011205 Minefield/3.0b3pre downloaded from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2008-01-12-05-trunk/

....but then you must know this already.

Comment 13

[Dão Gottwald [:dao]]

Comment on attachment 297505 [details] [diff] [review]
patch v2

Waiting for bug 410726 to land. There's also still a bug in this patch.

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v3

updated to trunk, added the 'i' modifier

Comment 15

[Dão Gottwald [:dao]]

Attachment: patch v3.1

... and without capturing the subpattern in the first reg. exp., because we don't need it.

Comment 16

[Reed Loden [:reed]]

Comment on attachment 298080 [details] [diff] [review]
patch v3.1

>+      if (!/%25(?:3B|2F|3F|3A|40|26|3D|2B|24|2C|23)/i.test(value))
...
>+                    .replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]/ig,

Did you mean ?: instead of ?! there (see above |if| test)?

Comment 17

[Dão Gottwald [:dao]]

(In reply to comment #16)
> (From update of attachment 298080 [details] [diff] [review])
> >+      if (!/%25(?:3B|2F|3F|3A|40|26|3D|2B|24|2C|23)/i.test(value))
> ...
> >+                    .replace(/%(?!3B|2F|3F|3A|40|26|3D|2B|24|2C|23)|[\r\n\t]/ig,
> 
> Did you mean ?: instead of ?! there (see above |if| test)?

No, ?! is a negative lookahead assertion. It matches % not followed by 3B|2F|....

Comment 18

[Reed Loden [:reed]]

(In reply to comment #17)
> No, ?! is a negative lookahead assertion. It matches % not followed by
> 3B|2F|....

Ah, yes, duh. My bad. :)

Comment 20

[Reed Loden [:reed]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.935; previous revision: 1.934
done

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre ID:2008012504

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3pre) Gecko/2008012504 Minefield/3.0b3pre