8000 [HttpFoundation] [Request] fix baseUrl parsing to fix wrong path_info by rk3rn3r · Pull Request #13039 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

rk3rn3r
Copy link
Contributor
@rk3rn3r rk3rn3r commented Dec 19, 2014

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
Copy link
Member

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

@rk3rn3r
Copy link
Contributor Author
rk3rn3r commented Dec 20, 2014

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

@rk3rn3r
Copy link
Contributor Author
rk3rn3r commented Dec 20, 2014

@Tobion in #13038 wrote this is

Highly related to #7329

@rk3rn3r
Copy link
Contributor Author
rk3rn3r commented Dec 20, 2014

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

@fabpot
Copy link
Member
fabpot commented Dec 21, 2014

Looks good to me. 👍

ping @symfony/deciders

@rk3rn3r
Copy link
Contributor Author
rk3rn3r commented Dec 29, 2014

No progress on this? @fabpot

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@timglabisch
Copy link

@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
Copy link
Contributor Author
rk3rn3r commented Jan 15, 2015

still no progress on this after weeks?

@stof
Copy link
Member
stof commented Jan 18, 2015

👍

@fabpot
Copy link
Member
fabpot commented Jan 18, 2015

Thank you @rk3rn3r.

fabpot added a commit that referenced this pull request Jan 18, 2015
…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
@fabpot fabpot closed this Jan 18, 2015
@dawehner
Copy link
Contributor
dawehner commented Feb 4, 2015

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
Copy link
Contributor Author
rk3rn3r commented Feb 4, 2015

@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
Copy link
Contributor
dawehner commented Feb 4, 2015

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).

fabpot added a commit that referenced this pull request May 20, 2015
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants
0