Fixed pathinfo calculation for requests starting with a question mark. #21968

syzygymsu · 2017-03-10T20:25:35Z

Q	A
Branch?	2.7
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#21967
License	MIT
Doc PR

With improper strpos result check calculated pathinfo for requests starting with '?' equals to request itself.
Correct pathinfo for those requests should be '/'.

jakzal · 2017-03-10T23:56:08Z

See #21967 (comment)

syzygymsu · 2017-03-11T02:20:35Z

Well, #21967 could be questionable. But here are more arguments:

prepareBaseUrl() already performs exactly the same operation and it works for requests starting with '?':

        $truncatedRequestUri = $requestUri;
        if (false !== $pos = strpos($requestUri, '?')) {
            $truncatedRequestUri = substr($requestUri, 0, $pos);
        }

preparePathInfo() can work with empty path returning '/'. Why it should work differently for empty path + query?
String containing '?' does not look like a valid pathinfo anyway.
Whole class becomes inconsistent because query is duplicated in pathinfo and qs:

echo Request::createFromGlobals()->getUri(); // prints: http://localhost?a=b?a=b

jakzal · 2017-03-11T09:18:41Z

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

+        $server['REQUEST_URI'] = '?a=b';
+        $request->initialize(array(), array(), array(), array(), array(), $server);
+
+        $this->assertEquals('/', $request->getPathInfo());


Please create a new test method for this case.

And also tests the query string to be extracted correctly.

jakzal · 2017-03-11T09:20:26Z

Give than web servers treat such requests as valid ones I'm 👍 (once my comment is addressed).

xabbuh · 2017-03-13T10:39:53Z

👍

Tobion · 2017-03-13T17:19:56Z

What about other invalid request uris like ab without a leading slash? This is currently not adding the leading slash which can result in bugs further down. So that should be changed as well if we start to accept invalid requests.

jakzal · 2017-03-13T19:28:26Z

src/Symfony/Component/Security/Http/Tests/HttpUtilsTest.php

@@ -144,7 +144,7 @@ public function testCheckRequestPath()
        // Plus must not decoded to space
        $this->assertTrue($utils->checkRequestPath($this->getRequest('/foo+bar'), '/foo+bar'));
        // Checking unicode
-        $this->assertTrue($utils->checkRequestPath($this->getRequest(urlencode('/вход')), '/вход'));
+        $this->assertTrue($utils->checkRequestPath($this->getRequest('/'.urlencode('вход')), '/вход'));


You shouldn't be changing the existing tests.

I think this test only meant to check unicode and not an encoded slash. Should I just leave it failing?

Also it's an interesting situation here: urlencoded slash could be allowed in requests. Then it is not a path separator but part of path segment.
But HttpUtils::checkRequestPath implementation cannot distinguish between /x/ and /x%2F.

jakzal · 2017-03-13T19:30:45Z

src/Symfony/Component/HttpFoundation/Tests/RequestTest.php

+    public function testNonstandardRequests($requestUri, $queryString, $expectedPathInfo, $expectedUri, $expectedBasePath = '', $expectedBaseUrl = null) {
+        if(is_null($expectedBaseUrl)) {
+            $expectedBaseUrl = $expectedBasePath;
+        }


I'd prefer to explicitly specify this in the data set, rather than introducing conditional logic in the test. I'd also remove defaults from the method signature so that all data rows in the data provider look consistent.

fabpot · 2017-03-22T20:27:35Z

Thank you @syzygymsu.

…estion mark. (syzygymsu) This PR was squashed before being merged into the 2.7 branch (closes #21968). Discussion ---------- Fixed pathinfo calculation for requests starting with a question mark. | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #21967 | License | MIT | Doc PR | With improper `strpos` result check calculated pathinfo for requests starting with '?' equals to request itself. Correct pathinfo for those requests should be '/'. Commits ------- 43297b4 Fixed pathinfo calculation for requests starting with a question mark.

- fix bad conflict resolving issue - port #21968 to 3.3+

…h a question mark. (syzygymsu) This PR was merged into the 3.3 branch. Discussion ---------- [3.3] Fixed pathinfo calculation for requests starting with a question mark. | Q | A | ------------- | --- | Branch? | 3.3 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24487 | License | MIT | Doc PR | no Fix of bad merge conflict resolving as mentioned in #24487. Port #21968 to 3.3+ Commits ------- c17a922 Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port #21968 to 3.3+

…. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+

* 3.3: (22 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void declare argument type [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks streamed response should return $this $isClientIpsVali is not used content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+ Fixed unsetting from loosely equal keys OrderedHashMap add DOMElement as return type in Crawler::getIterator to support foreach support in ide Fixed mistake in exception expectation [Debug] Fix same vendor detection in class loader ...

* 3.3: (22 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void declare argument type [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks streamed response should return $this $isClientIpsVali is not used content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port #21968 to 3.3+ Fixed unsetting from loosely equal keys OrderedHashMap add DOMElement as return type in Crawler::getIterator to support foreach support in ide Fixed mistake in exception expectation [Debug] Fix same vendor detection in class loader ...

* 3.3: (22 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void declare argument type [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks streamed response should return $this $isClientIpsVali is not used content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+ Fixed unsetting from loosely equal keys OrderedHashMap add DOMElement as return type in Crawler::getIterator to support foreach support in ide Fixed mistake in exception expectation [Debug] Fix same vendor detection in class loader ...

* 3.4: (26 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void [FrameworkBundle][Serializer] Move DateIntervalNormalizer definition to xml declare argument type Improving annotation loader message [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks Update UPGRADE-4.0.md streamed response should return $this $isClientIpsVali is not used [WebServerBundle] Prevent commands from being registered by convention content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+ ...

* 3.4: (26 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void [FrameworkBundle][Serializer] Move DateIntervalNormalizer definition to xml declare argument type Improving annotation loader message [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks Update UPGRADE-4.0.md streamed response should return $this $isClientIpsVali is not used [WebServerBundle] Prevent commands from being registered by convention content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port #21968 to 3.3+ ...

* 3.4: (26 commits) [Routing] Fix resource miss [Security] Fixed auth provider authenticate() cannot return void [FrameworkBundle][Serializer] Move DateIntervalNormalizer definition to xml declare argument type Improving annotation loader message [FrameworkBundle][Serializer] Move normalizer/encoders definitions to xml file & remove unnecessary checks Update UPGRADE-4.0.md streamed response should return $this $isClientIpsVali is not used [WebServerBundle] Prevent commands from being registered by convention content can be a resource Adding the Form default theme files to be warmed up in Twig's cache Remove BC Break label from `NullDumper` class Username and password in basic auth are allowed to contain '.' Remove obsolete PHPDoc from UriSigner [Serializer] YamlEncoder: throw if the Yaml component isn't installed [Serializer] ObjectNormalizer: throw if PropertyAccess isn't installed [PropertyInfo] Add support for the iterable type pdo session fix Fixed pathinfo calculation for requests starting with a question mark. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+ ...

Fixed pathinfo calculation for requests starting with a question mark.

40ce65c

carsonbot added Status: Needs Review Bug labels Mar 10, 2017

jakzal reviewed Mar 11, 2017

View reviewed changes

jakzal mentioned this pull request Mar 13, 2017

[HttpFoundation] fix path info when request path is not sent #21984

Closed

xabbuh mentioned this pull request Mar 13, 2017

Incorrect routing for requests without leading slash #21967

Closed

syzygymsu added 2 commits March 13, 2017 22:16

Support more non-standard requests.

4032f47

Fixed code style

839d339

jakzal reviewed Mar 13, 2017

View reviewed changes

nicolas-grekas added this to the 2.7 milestone Mar 14, 2017

fabpot closed this Mar 22, 2017

This was referenced Apr 4, 2017

Release v2.7.26 #22263

Merged

Release v2.8.19 #22284

Merged

Release v3.2.7 #22293

Merged

aivus mentioned this pull request Oct 16, 2017

[3.3] Fixed pathinfo calculation for requests starting with a question mark. #24573

Merged

fabpot pushed a commit that referenced this pull request Oct 16, 2017

Fixed pathinfo calculation for requests starting with a question mark.

c17a922

- fix bad conflict resolving issue - port #21968 to 3.3+

symfony-splitter pushed a commit to symfony/http-foundation that referenced this pull request Oct 16, 2017

Fixed pathinfo calculation for requests starting with a question mark…

b54ae1f

…. - fix bad conflict resolving issue - port symfony/symfony#21968 to 3.3+

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed pathinfo calculation for requests starting with a question mark. #21968

Fixed pathinfo calculation for requests starting with a question mark. #21968

syzygymsu commented Mar 10, 2017

jakzal commented Mar 10, 2017

syzygymsu commented Mar 11, 2017

jakzal Mar 11, 2017

Tobion Mar 13, 2017

jakzal commented Mar 11, 2017

xabbuh commented Mar 13, 2017

Tobion commented Mar 13, 2017

jakzal Mar 13, 2017

syzygymsu Mar 13, 2017

jakzal Mar 13, 2017 •

edited

fabpot commented Mar 22, 2017

Fixed pathinfo calculation for requests starting with a question mark. #21968

Fixed pathinfo calculation for requests starting with a question mark. #21968

Conversation

syzygymsu commented Mar 10, 2017

jakzal commented Mar 10, 2017

syzygymsu commented Mar 11, 2017

jakzal Mar 11, 2017

Choose a reason for hiding this comment

Tobion Mar 13, 2017

Choose a reason for hiding this comment

jakzal commented Mar 11, 2017

xabbuh commented Mar 13, 2017

Tobion commented Mar 13, 2017

jakzal Mar 13, 2017

Choose a reason for hiding this comment

syzygymsu Mar 13, 2017

Choose a reason for hiding this comment

jakzal Mar 13, 2017 • edited

Choose a reason for hiding this comment

fabpot commented Mar 22, 2017

jakzal Mar 13, 2017 •

edited