Percent encodings in url bar inconsistent

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Address Bar
P1
major
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: Mycroft Project (CC), Assigned: dao)

Tracking

({dogfood, regression})

Trunk
Firefox 3 beta3
dogfood, regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

10 years ago
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

Updated

10 years ago
Flags: blocking-firefox3?

Comment 1

10 years ago
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.
(Reporter)

Comment 2

10 years ago
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)
(Reporter)

Comment 3

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

'now' even... :)
(Assignee)

Comment 4

10 years ago
I've just looked at the innards of decodeURI, and the fix for this should be pretty trivial (even though not pretty).
Assignee: nobody → dao
Blocks: 397815
OS: Windows XP → All
Hardware: PC → All

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
Created attachment 297212 [details] [diff] [review]
patch
Attachment #297212 - Flags: review?(gavin.sharp)
(Assignee)

Comment 7

10 years ago
(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
Status: NEW → ASSIGNED
(Assignee)

Updated

10 years ago
Severity: normal → major
(Assignee)

Comment 8

10 years ago
Created attachment 297505 [details] [diff] [review]
patch v2

This time, without breaking addresses like data:text/plain,Colon%253A.
Attachment #297212 - Attachment is obsolete: true
Attachment #297505 - Flags: review?(gavin.sharp)
Attachment #297212 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
Priority: -- → P1
Target Milestone: --- → Firefox 3 M11

Comment 9

10 years ago
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...
(Assignee)

Comment 10

10 years ago
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.

Updated

10 years ago
Duplicate of this bug: 413158

Comment 12

10 years ago
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.
(Assignee)

Comment 13

10 years ago
Comment on attachment 297505 [details] [diff] [review]
patch v2

Waiting for bug 410726 to land. There's also still a bug in this patch.
Attachment #297505 - Attachment is obsolete: true
Attachment #297505 - Flags: review?(gavin.sharp)
(Assignee)

Comment 14

10 years ago
Created attachment 298079 [details] [diff] [review]
patch v3

updated to trunk, added the 'i' modifier
Attachment #298079 - Flags: review?(gavin.sharp)
Keywords: dogfood
(Assignee)

Comment 15

10 years ago
Created attachment 298080 [details] [diff] [review]
patch v3.1

... and without capturing the subpattern in the first reg. exp., because we don't need it.
Attachment #298079 - Attachment is obsolete: true
Attachment #298080 - Flags: review?(gavin.sharp)
Attachment #298079 - Flags: review?(gavin.sharp)
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)?
(Assignee)

Comment 17

10 years ago
(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|....
(In reply to comment #17)
> No, ?! is a negative lookahead assertion. It matches % not followed by
> 3B|2F|....

Ah, yes, duh. My bad. :)

Updated

10 years ago
Whiteboard: [has patch][needs review gavin]
Duplicate of this bug: 413857
Flags: blocking-firefox3? → blocking-firefox3+
Attachment #298080 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][needs review gavin]
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
Status: RESOLVED → VERIFIED
Blocks: 425480
(Assignee)

Updated

10 years ago
No longer blocks: 425480
(Assignee)

Updated

10 years ago
Blocks: 425480
You need to log in before you can comment on or make changes to this bug.