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

RESOLVED FIXED in Firefox 36

Status

()

Core
Networking
--
major
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Masamichi, Assigned: valentin)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

35 Branch
mozilla38
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox36 fixed, firefox37 fixed, firefox38 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

4 years ago
Summary: Some charactors([, ]) are url encoded unexpectedly in $_SERVER variable. → Some charactors([, ]) in URL are url-encoded unexpectedly in $_SERVER variable.
(Reporter)

Updated

4 years ago
Severity: normal → major

Comment 1

4 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?
Component: Untriaged → Untriaged
Flags: needinfo?(valentin.gosu)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(annevk)
OS: Windows 7 → All
Product: Firefox → Core
Hardware: x86_64 → All
(Assignee)

Comment 2

4 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

4 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: 906714
Flags: needinfo?(annevk)

Updated

4 years ago
Flags: needinfo?(bzbarsky)

Updated

4 years ago
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true

Updated

4 years ago
Blocks: 473822

Comment 4

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

4 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

4 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

4 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

4 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

4 years ago
Created attachment 8549944 [details] [diff] [review]
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
Attachment #8549944 - Flags: review?(mcmanus)
(Assignee)

Updated

4 years ago
Assignee: nobody → valentin.gosu
Status: NEW → ASSIGNED
Attachment #8549944 - Flags: review?(mcmanus) → review+

Updated

4 years ago
Duplicate of this bug: 1122714
https://hg.mozilla.org/mozilla-central/rev/9124cbf3e75f
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
(Assignee)

Comment 12

4 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?
status-firefox36: --- → affected
status-firefox37: --- → fixed
status-firefox38: --- → fixed
Attachment #8549944 - Flags: approval-mozilla-beta?
Attachment #8549944 - Flags: approval-mozilla-beta+
Attachment #8549944 - Flags: approval-mozilla-aurora?
Attachment #8549944 - Flags: approval-mozilla-aurora+
Keywords: qawanted
status-firefox37: fixed → affected
https://hg.mozilla.org/releases/mozilla-aurora/rev/ac9e91778eb6
https://hg.mozilla.org/releases/mozilla-beta/rev/12bda229bf83
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Flags: in-testsuite+

Updated

4 years ago
Duplicate of this bug: 1124209
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)
Actually, no. Sorry about the noise!
Flags: needinfo?(sledru)
Keywords: qawanted

Updated

4 years ago
Depends on: 1124600
Duplicate of this bug: 1133707
Duplicate of this bug: 1128434
You need to log in before you can comment on or make changes to this bug.