-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[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
Conversation
** 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
You can close, the other PR: your patch will come up to master when we will merge branches. |
@markuspoerschke @andygrunwald @timglabisch FYI, the other PRs were closed, this one will be continued |
Looks good to me. 👍 ping @symfony/deciders |
No progress on this? @fabpot |
@@ -1299,7 +1314,7 @@ public function getBaseUrlData() | |||
{ | |||
return array( | |||
array( | |||
'/foo%20bar', | |||
'/foo%20bar/', |
There was a problem hiding this comment.
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?
@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:
|
still no progress on this after weeks? |
👍 |
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
Note: This caused a bug in the unit tests of Drupal 8, see https://qa.drupal.org/pifr/test/966618 ... |
@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 ;-) |
Well, it used to work, but yeah it was a bug in Heidelberg our setup code
|
…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
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
in Request::prepareBaseUrl() there are checks to find the baseUrl:
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:
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)
[HttpFoundation] [Request]
[HttpFoundation] [Tests] [RequestTest]
This is the symfony 2.3 branch fix for the issue related to #13038 and #13040
Happy christmas!