[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13039

rk3rn3r · 2014-12-19T16:10:15Z

Hi everyone!

We at trivago had an issue with the Request object. It seems that all versions of symfony 2.x and 3.x are affected from this (possible) bug (don't checked 1.x).
Here is the problem:

some old legacy pages are deployed in the Document Root, let's say /var/www/www.test.com/ .
one or more new applications based on symfony are deployed to /var/release/new_app1/ , /var/release/new_app2/ , ... .
in /var/www/www.test.com/ there is a symlink "app" to /var/release/new_app1/web, like:
/var/www/www.test.com/app --> /var/release/new_app1/web/

there is a "SEO"/human-readable rewrite rule for Document Root (if called path/file not exist): (.*) --> app/app.php

the problem comes, when the user calls a uri starting with "app" or whatever the rewrite rule / symlink points to:

the user calls "http://www.test.com/apparthotel-1234"
results in $_SERVER parameters like this

'DOCUMENT_ROOT' =>'/var/www/www.test.com',
'SCRIPT_FILENAME' => '/var/www/www.test.com/app/app.php',
'SCRIPT_NAME' => '/app/app.php',
'PHP_SELF' => '/app/app.php/apparthotel-1234'

in Request::prepareBaseUrl() there are checks to find the baseUrl:

        if ($baseUrl && false !== $prefix = $this->getUrlencodedPrefix($requestUri, $baseUrl)) {
            // full $baseUrl matches
            return $prefix;
        }

        if ($baseUrl && false !== $prefix = $this->getUrlencodedPrefix($requestUri, dirname($baseUrl))) {
            // directory portion of $baseUrl matches
            return rtrim($prefix, '/');
        }

first it is checked if (in our case) "/app/app.php" is in the request uri (/apparthotel-1234).
it's not.

then it takes the dirname (of /app/app.php) which is /app and checks if it is in the request uri (/apparthotel-1234), and YES, it is! and "/app" is returned, but this is wrong, it should be empty (because it comes from a rewrite rule from root: /)!

later in preparePathInfo(), if there is a baseUrl, then the baseUrl is removed from the request uri:
/apparthotel-1234 ---> /arthotel-1234

The cause is, the second baseUrl check, checks if the path of the application is already in the uri, like when the request was "http://www.test.com/app/apparthotel-1234" and hit a rewrite rule like (.*) --> app.php in there, but because it matches a directory it must match "dirname($baseUrl) . '/'".

I also needed to fix one unit test of the getBaseUrl test:
the request uri recently was "/foo%20bar".
but from the $_SERVER infos "foo bar" is a directory, see:

'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/app.php',
'SCRIPT_NAME' => '/foo bar/app.php',
'PHP_SELF' => '/foo bar/app.php',

webservers will redirect a request "http://www.test.com/foo%20bar" to "http://www.test.com/foo%20bar/" when "foo bar" is a directory. checked this for apache 2.x and nginx 1.4.x.

this fix is for symfony master (3.0.x, see #13039).
I also prepared a merge request for actual 2.7 branch, it will also follow in some minutes. (see #13040)

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	this, #13040, #13038, #7329
License	MIT

[HttpFoundation] [Request]

added missing slash to baseUrl-path part check to remove the path, only when it's also a path in the uri
[HttpFoundation] [Tests] [RequestTest]
fixed and added unittests

This is the symfony 2.3 branch fix for the issue related to #13038 and #13040

Happy christmas!

** added missing slash to baseUrl-path part check to remove the path, only when it's also a path in the uri * fixed and added unittests

nicolas-grekas · 2014-12-19T17:20:29Z

You can close, the other PR: your patch will come up to master when we will merge branches.

rk3rn3r · 2014-12-20T09:33:48Z

since I saw recent changes on these lines of code from @Seldaek and @fabpot , what do you think?

rk3rn3r · 2014-12-20T09:39:46Z

@Tobion in #13038 wrote this is

Highly related to #7329

rk3rn3r · 2014-12-20T09:43:48Z

@markuspoerschke @andygrunwald @timglabisch FYI, the other PRs were closed, this one will be continued

fabpot · 2014-12-21T08:25:32Z

Looks good to me. 👍

ping @symfony/deciders

rk3rn3r · 2014-12-29T08:35:30Z

No progress on this? @fabpot

jakzal · 2014-12-29T10:47:23Z

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

@@ -1299,7 +1314,7 @@ public function getBaseUrlData()
    {
        return array(
            array(
-                '/foo%20bar',
+                '/foo%20bar/',


Just double checking. Could anyone rely on the slash not being there and what are the implications?

timglabisch · 2014-12-29T15:04:36Z

@rk3rn3r i played a bit with this issue, it's hard to reproduce. i think you're right, the test was broken, you just fixed it. here are some testcases i played with:

        array(
            '/foo%20bar',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/app.php',
                'SCRIPT_NAME'     => '/foo bar/app.php',
                'PHP_SELF'        => '/foo bar/app.php',
            ),
            '',
            '/foo%20bar',
        ),
        array(
            '/foo%20bar/',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/app.php',
                'SCRIPT_NAME'     => '/foo bar/app.php',
                'PHP_SELF'        => '/foo bar/app.php',
            ),
            '/foo%20bar',
            '/',
        ),
        array(
            '/foo%20bar',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo baz/app.php',
                'SCRIPT_NAME'     => '/foo baz/app.php',
                'PHP_SELF'        => '/foo baz/app.php',
            ),
            '',
            '/foo%20bar',
        ),
        array(
            '/foo',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/foo',
                'SCRIPT_NAME'     => '/foo',
                'PHP_SELF'        => '/foo',
            ),
            '/foo',
            '/',
        ),
        array(
            '/foo bar/foo/foo bar',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/foo/foo bar',
                'SCRIPT_NAME'     => '/foo bar',
                'PHP_SELF'        => '/foo bar',
            ),
            '/foo bar',
            '/foo/foo bar',
        ),
        array(
            '/foo bar',
            array(
                'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo/foo bar/foo/foo bar',
                'SCRIPT_NAME'     => '/foo',
                'PHP_SELF'        => '/foo',
            ),
            '/foo bar',
            '/',
        ),

rk3rn3r · 2015-01-15T09:30:02Z

still no progress on this after weeks?

stof · 2015-01-18T13:23:49Z

👍

fabpot · 2015-01-18T13:31:31Z

Thank you @rk3rn3r.

…g path_info (rk3rn3r) This PR was squashed before being merged into the 2.3 branch (closes #13039). Discussion ---------- [HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info Hi everyone! We at trivago had an issue with the Request object. It seems that all versions of symfony 2.x and 3.x are affected from this (possible) bug (don't checked 1.x). Here is the problem: some old legacy pages are deployed in the Document Root, let's say /var/www/www.test.com/ . one or more new applications based on symfony are deployed to /var/release/new_app1/ , /var/release/new_app2/ , ... . in /var/www/www.test.com/ there is a symlink "app" to /var/release/new_app1/web, like: /var/www/www.test.com/app --> /var/release/new_app1/web/ there is a "SEO"/human-readable rewrite rule for Document Root (if called path/file not exist): (.*) --> app/app.php the problem comes, when the user calls a uri starting with "app" or whatever the rewrite rule / symlink points to: the user calls "http://www.test.com/apparthotel-1234" results in $_SERVER parameters like this ``` 'DOCUMENT_ROOT' =>'/var/www/www.test.com', 'SCRIPT_FILENAME' => '/var/www/www.test.com/app/app.php', 'SCRIPT_NAME' => '/app/app.php', 'PHP_SELF' => '/app/app.php/apparthotel-1234' ``` in Request::prepareBaseUrl() there are checks to find the baseUrl: ``` if ($baseUrl && false !== $prefix = $this->getUrlencodedPrefix($requestUri, $baseUrl)) { // full $baseUrl matches return $prefix; } if ($baseUrl && false !== $prefix = $this->getUrlencodedPrefix($requestUri, dirname($baseUrl))) { // directory portion of $baseUrl matches return rtrim($prefix, '/'); } ``` first it is checked if (in our case) "/app/app.php" is in the request uri (/apparthotel-1234). it's not. then it takes the dirname (of /app/app.php) which is /app and checks if it is in the request uri (/apparthotel-1234), and YES, it is! and "/app" is returned, but this is wrong, it should be empty (because it comes from a rewrite rule from root: /)! later in preparePathInfo(), if there is a baseUrl, then the baseUrl is removed from the request uri: /apparthotel-1234 ---> /arthotel-1234 The cause is, the second baseUrl check, checks if the path of the application is already in the uri, like when the request was "http://www.test.com/app/apparthotel-1234" and hit a rewrite rule like (.*) --> app.php in there, but because it matches a directory it must match "dirname($baseUrl) . '/'". I also needed to fix one unit test of the getBaseUrl test: the request uri recently was "/foo%20bar". but from the $_SERVER infos "foo bar" is a directory, see: ``` 'SCRIPT_FILENAME' => '/home/John Doe/public_html/foo bar/app.php', 'SCRIPT_NAME' => '/foo bar/app.php', 'PHP_SELF' => '/foo bar/app.php', ``` webservers will redirect a request "http://www.test.com/foo%20bar" to "http://www.test.com/foo%20bar/" when "foo bar" is a directory. checked this for apache 2.x and nginx 1.4.x. this fix is for symfony master (3.0.x, see #13039). I also prepared a merge request for actual 2.7 branch, it will also follow in some minutes. (see #13040) | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | this, #13040, #13038, #7329 | License | MIT [HttpFoundation] [Request] * added missing slash to baseUrl-path part check to remove the path, only when it's also a path in the uri [HttpFoundation] [Tests] [RequestTest] * fixed and added unittests This is the symfony 2.3 branch fix for the issue related to #13038 and #13040 Happy christmas! Commits ------- 3a3ecd3 [HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info

dawehner · 2015-02-04T07:59:27Z

Note: This caused a bug in the unit tests of Drupal 8, see https://qa.drupal.org/pifr/test/966618 ...
It assumed that you can use Request::create('/subdir') instead of Request::create('/subdir/')

rk3rn3r · 2015-02-04T09:30:44Z

@dawehner , but this is bug of the unit test then? because a subdirectory, that is requested by /subdir will be redirected to /subdir/. The assumption is wrong then ;-)

dawehner · 2015-02-04T09:52:48Z

Well, it used to work, but yeah it was a bug in Heidelberg our setup code
On Feb 4, 2015 10:31 AM, "René Kerner" notifications@github.com wrote:

@dawehner https://github.com/dawehner , but this is bug of the unit
test then? because a subdirectory, that is requested by /subdir will be
redirected to /subdir/. The assumption is wrong then ;-)

—
Reply to this email directly or view it on GitHub
#13039 (comment).

…ined in pathInfo (danez) This PR was squashed before being merged into the 2.3 branch (closes #14335). Discussion ---------- [HttpFoundation] Fix baseUrl when script filename is contained in pathInfo | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #13617 | License | MIT | Doc PR | When the script filename is just /index.php, dirname() returns '/' for it. In Request::prepareBaseUrl() we append '/' to it (as introduced in #13039), which is wrong in this scenario as the resulting string is '//'. When we rtrim('/') the output of dirname() then '/' would be constructed in this case, and in all other cases it makes no difference as dirname() already trims the right forward slash if there are path segments. The test-cases should clarify the exact scenario. Commits ------- f24a6dd [HttpFoundation] Fix baseUrl when script filename is contained in pathInfo

* fixed baseUrl parsing

5e18bfd

** added missing slash to baseUrl-path part check to remove the path, only when it's also a path in the uri * fixed and added unittests

This was referenced Dec 19, 2014

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13040

Closed

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13038

Closed

* fixed styleguide

eedf817

rk3rn3r mentioned this pull request Dec 20, 2014

[HttpFoundation] Problem with Request::getBaseUrl and url-rewrites #7329

Closed

jakzal reviewed Dec 29, 2014
View reviewed changes

fabpot added the HttpFoundation label Jan 2, 2015

fabpot closed this Jan 18, 2015

Tobion mentioned this pull request Feb 7, 2015

prepareBaseUrl() Request bug #13617

Closed

danez mentioned this pull request Apr 13, 2015

[HttpFoundation] Fix baseUrl when script filename is contained in pathInfo #14335

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13039

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13039

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13039

[HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info #13039

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!