1121826 - Some characters([, ]) in URL are url-encoded unexpectedly in $_SERVER variable.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Masamichi]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/39.0.2171.95 Safari/537.36

Steps to reproduce:

1. Prepare a script like this.
//request_check.php
<?php
exit(var_dump($_SERVER['REQUEST_URI']));
?>

2. Access the script with query parameter. 
http://localhost/request_check.php?hoge[]=fuga&hoge[]=piyo 

3. You will get a string like this.
IE8
string(51) "/otsukano/request_check.php?hoge[]=fuga&hoge[]=piyo" 

Chrome
string(51) "/otsukano/request_check.php?hoge[]=fuga&hoge[]=piyo"

FireFox35
string(59) "/otsukano/request_check.php?hoge%5B%5D=fuga&hoge%5B%5D=piyo"


Actual results:

The value of $_SERVER['REQUEST_URI']) is url-encoded unexpectedly
([] => %5B%5D).



Expected results:

The value should not be url-encoded.

Comment 1

[:Gijs (he/him)]

I tried to find the relevant spec bits for this. Reading https://url.spec.whatwg.org/#url-writing , it seems it says that a query string must consist of URL units, which are either percent-encoded bytes or in the set of url code points: https://url.spec.whatwg.org/#url-code-points which don't include [ or ] , leading me to think that they should therefore be encoded. However, (a) my success rate when trying to read specs is not great, and (b) the cross-browser inconsistency is not a good sign here. Anne/bz/valentin, you probably know more than me, can (one of) you comment?

Comment 2

[Valentin Gosu [:valentin] (he/him)]

That change was made in bug 473822.
:annevk would know which is the right thing to do here.

Comment 3

[Anne (:annevk)]

Per https://url.spec.whatwg.org/#query-state it appears these are not to be escaped when found in a URL. It seems they are non-conforming to appear as such (they are not URL code points), but at the moment they do not get escaped by the parser and therefore what I suggested in bug 473822 is wrong.

Testing other browsers using http://intertwingly.net/projects/pegurl/liveview.html shows that none of them escape either "[" or "]", but that does not explain the behavior people are talking about in the other bug.

Perhaps the only problem was that we removed percent-encoding rather than leaving it in place?

I'm really sorry about not having done due diligence the last time.

Comment 4

[Sam Ruby]

> my success rate when trying to read specs is not great

Nor is mine :-)

One thing I'm much better at is looking at actual test results:

https://url.spec.whatwg.org/interop/test-results/

None of these tests include square brackets outside of the authority/hostname field.  So lets add some.  If people have suggestions for a few tests, I can add the tests, gather results, and we can discuss the way forward.

Meahwhile, the following may be helpful:

http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=3368

Comment 5

[Julian Reschke]

The relevant spec is RFC 3986. "pchar" does not allow "[" nor "]", thus Firefox is correct in percent-encoding these characters.

A recipient that trips over percent-encoding like this is broken and should be fixed.

Comment 6

[Anthony Roy]

(In reply to Anne (:annevk) from comment #3)
> Testing other browsers using
> http://intertwingly.net/projects/pegurl/liveview.html shows that none of
> them escape either "[" or "]", but that does not explain the behavior people
> are talking about in the other bug.

Just to clarify about bug 473822, the behavior that was causing issues for my team was Firefox decoding correctly percent-encoded square brackets in the URL bar, and making that "sticky". Typing in "?foo%5Bb%5D=bar" correctly sent that to the server, but in the URL bar it displayed "?foo[b]=bar". Now if you copied/pasted, bookmarked, or even clicked in to the URL bar and pressed enter, the next request to the server used the naked square brackets. It was this behind-the-scenes changing of what the user had entered (or been directed to) that was causing issues.

Comment 7

[Anne (:annevk)]

Thanks Anthony, I agree that we should stop doing that. (I suppose that for display purposes we still might want to show [ and ], but we should not give an altered URL when you start to manipulate or copy it. Same as with Unicode <> ASCII.)

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: backout cc192030c28f - brackets shouldn't be automatically escaped in the Query

I reverted the change in nsEscape.cpp and changed the unit test.
This should probably be uplifted so users don't get stuck with the bug for too long

Comment 10

[Valentin Gosu [:valentin] (he/him)]

Try is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7bb9b8fe50b


https://hg.mozilla.org/integration/mozilla-inbound/rev/9124cbf3e75f

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9124cbf3e75f

Comment 12

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8549944 [details] [diff] [review]
backout cc192030c28f - brackets shouldn't be automatically escaped in the Query

Approval Request Comment

[Feature/regressing bug #]:
Bug 473822

[User impact if declined]:
Some websites may not work for users on those specific releases.

[Describe test coverage new/current, TBPL]:
I have changed a test to make sure this behaviour does not regress in the future.

[Risks and why]:
Very low risk. It reverts back to a behaviour Firefox has had for a long time.

[String/UUID change made/needed]:
None

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac9e91778eb6
https://hg.mozilla.org/releases/mozilla-beta/rev/12bda229bf83

Comment 15

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This seems to have a dedicated straight forward test. Sylvestre, do you think it needs some additional manual testing outside of what's covered automatically?

Comment 16

[Sylvestre Ledru [:Sylvestre]]

Actually, no. Sorry about the noise!