Fix surrogate not using original request #27309
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c025335 to
eb13665
Compare
eb13665 to
5a7aefd
Compare
|
AppVeyor failures look unrelated and the license issue with fabbot.io is something I think I shouldn't fix? |
| // We must duplicate here to get a separate instance because the application will modify the request during | ||
| // the application flow (we know it always does because we do ourselves by setting REMOTE_ADDR to 127.0.0.1 | ||
| // and adding the X-Forwarded-For header, see HttpCache::forward()). | ||
| $this->request = $request->duplicate(); |
There was a problem hiding this comment.
why duplicate and not clone? (clone is used elsewhere in the class)
There was a problem hiding this comment.
also: should we then use the duplicated/cloned request instead of $request directly?
There was a problem hiding this comment.
duplicate resets everything. I think that can't harm, but it might actually not be necessary (as the request really should be "fresh").
should we then use the duplicated/cloned request instead of $request directly?
I don't think so. The duplicate is created to store it for further reference, not to use it for the current request. If the clone would be used, the original problem ($request modification over multiple surrogate request) would re-appear.
There was a problem hiding this comment.
Done in dcb3833.
…tierderuette) This PR was submitted for the 3.4 branch but it was merged into the 2.8 branch instead (closes symfony#27630). Discussion ---------- [Validator][Form] Remove BOM in some xlf files | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | | Fixed tickets | #... | License | MIT | Doc PR | symfony/symfony-docs#... I removed first blank space from some xml files It caused this error during cache:clear [ERROR 4] Start tag expected, '<' not found (in n/a - line 1, column 1) Commits ------- 0bc53d6 [Validator] Remove BOM in some xlf files
This PR was submitted for the master branch but it was squashed and merged into the 2.8 branch instead (closes symfony#27508). Discussion ---------- [Finder] Update RealIteratorTestCase | Q | A | ------------- | --- | Branch? | 2.8 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony#27480 | License | MIT | Doc PR | n/a Makes the entire test directory empty instead of trying to delete particular files and directories. The old method failed when trying to remove a directory which was not empty. Commits ------- 7d0ebd4 [Finder] Update RealIteratorTestCase
dcb3833 to
ab86f43
Compare
|
Thank you @Toflar. |
nicolas-grekas
added a commit
that referenced
this pull request
Jun 19, 2018
This PR was submitted for the 3.4 branch but it was squashed and merged into the 2.8 branch instead (closes #27309). Discussion ---------- Fix surrogate not using original request | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | Warning: This might need some close attention. It took me hours to wrap my head around that issue :-) So the problem is that `HttpCache::forward()` (or essentially any part in your application) can modify the `$request` that is passed to the `HttpCache::handle()` and the surrogate can never access the original request using `HttpCache::getRequest()` anymore. Example: * Main request (GET `/foobar`) * It's not in the cache, so `HttpCache::forward()` modifies `REMOTE_ADDR` to `127.0.0.1` and adds the `X-Forwarded-For` header. * The request is sent to the application and any e.g. `kernel.request` listener might modify the `$request` further. * Now the `/foobar` route returns `text/html` that contains some `<esi src="=/fragment_path"` tag. * `HttpCache` has an instance of `SurrogateInterface` so (in our case `Esi`) will be asked to `process()` and then later on `handle()` the `/fragment_path` request. For that, `Esi` (or in fact `AbstractSurrogate` uses the following line to create a subrequest and pass it on to the application again: ```php $subRequest = Request::create($uri, Request::METHOD_GET, array(), $cache->getRequest()->cookies->all(), array(), $cache->getRequest()->server->all()); ``` What you can see here, is that it uses `$cache->getRequest()`. And here follows the problem: We did not duplicate (clone) the original request so essentially `$cache->getRequest()` is a reference to the current request that `HttpKernel::forward()` modified and probably any other part of the application did so too. So for example the original `REMOTE_ADDR` (client IP) got lost. What we should do instead is duplicate the original request so the surrogates can actually behave like a real reverse proxy such as Varnish would by keeping all the original request attributes. Commits ------- ab86f43 Fix surrogate not using original request
This was referenced Jun 25, 2018
Merged
Merged
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Warning: This might need some close attention. It took me hours to wrap my head around that issue :-)
So the problem is that
HttpCache::forward()(or essentially any part in your application) can modify the$requestthat is passed to theHttpCache::handle()and the surrogate can never access the original request usingHttpCache::getRequest()anymore.Example:
/foobar)HttpCache::forward()modifiesREMOTE_ADDRto127.0.0.1and adds theX-Forwarded-Forheader.kernel.requestlistener might modify the$requestfurther./foobarroute returnstext/htmlthat contains some<esi src="=/fragment_path"tag.HttpCachehas an instance ofSurrogateInterfaceso (in our caseEsi) will be asked toprocess()and then later onhandle()the/fragment_pathrequest. For that,Esi(or in factAbstractSurrogateuses the following line to create a subrequest and pass it on to the application again:What you can see here, is that it uses
$cache->getRequest(). And here follows the problem:We did not duplicate (clone) the original request so essentially
$cache->getRequest()is a reference to the current request thatHttpKernel::forward()modified and probably any other part of the application did so too. So for example the originalREMOTE_ADDR(client IP) got lost.What we should do instead is duplicate the original request so the surrogates can actually behave like a real reverse proxy such as Varnish would by keeping all the original request attributes.