Closed
Bug 1121826
Opened 10 years ago
Closed 10 years ago
Some characters([, ]) in URL are url-encoded unexpectedly in $_SERVER variable.
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla38
People
(Reporter: f.masamichi, Assigned: valentin)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
patch
|
mcmanus
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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.
Summary: Some charactors([, ]) are url encoded unexpectedly in $_SERVER variable. → Some charactors([, ]) in URL are url-encoded unexpectedly in $_SERVER variable.
Comment 1•10 years ago
|
||
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?
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(annevk)
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86_64 → All
Assignee | ||
Comment 2•10 years ago
|
||
That change was made in bug 473822.
:annevk would know which is the right thing to do here.
Flags: needinfo?(valentin.gosu)
Comment 3•10 years ago
|
||
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.
Blocks: url
Flags: needinfo?(annevk)
Updated•10 years ago
|
Flags: needinfo?(bzbarsky)
Updated•10 years ago
|
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
> 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
Updated•10 years ago
|
Summary: Some charactors([, ]) in URL are url-encoded unexpectedly in $_SERVER variable. → Some characters([, ]) in URL are url-encoded unexpectedly in $_SERVER variable.
Comment 5•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.)
Assignee | ||
Comment 8•10 years ago
|
||
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
Attachment #8549944 -
Flags: review?(mcmanus)
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Updated•10 years ago
|
Attachment #8549944 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 10•10 years ago
|
||
Comment 11•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Assignee | ||
Comment 12•10 years ago
|
||
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
Attachment #8549944 -
Flags: approval-mozilla-beta?
Attachment #8549944 -
Flags: approval-mozilla-aurora?
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8549944 -
Flags: approval-mozilla-beta?
Attachment #8549944 -
Flags: approval-mozilla-beta+
Attachment #8549944 -
Flags: approval-mozilla-aurora?
Attachment #8549944 -
Flags: approval-mozilla-aurora+
Updated•10 years ago
|
Comment 13•10 years ago
|
||
Comment 15•10 years ago
|
||
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?
Flags: needinfo?(sledru)
Comment 16•10 years ago
|
||
Actually, no. Sorry about the noise!
Flags: needinfo?(sledru)
Keywords: qawanted
You need to log in
before you can comment on or make changes to this bug.
Description
•