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

Fix surrogate not using original request #27309

Merged
merged 6 commits into from Jun 19, 2018

Conversation

Projects
None yet
7 participants
@Toflar
Contributor

Toflar commented May 18, 2018

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:
$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.

@Toflar Toflar changed the base branch from master to 3.4 May 18, 2018

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 18, 2018

@Toflar

This comment has been minimized.

Contributor

Toflar commented May 22, 2018

AppVeyor failures look unrelated and the license issue with fabbot.io is something I think I shouldn't fix?

@nicolas-grekas

(for 2.8!)

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 2.8 Jun 1, 2018

// 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();

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 1, 2018

Member

why duplicate and not clone? (clone is used elsewhere in the class)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 1, 2018

Member

also: should we then use the duplicated/cloned request instead of $request directly?

This comment has been minimized.

@aschempp

aschempp Jun 1, 2018

Contributor

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.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 19, 2018

Member

let's use clone then :)

This comment has been minimized.

@Toflar

Toflar Jun 19, 2018

Contributor

Done in dcb3833.

@xabbuh xabbuh added the HttpKernel label Jun 2, 2018

gautierderuette and others added some commits Jun 18, 2018

bug #27630 [Validator][Form] Remove BOM in some xlf files (gautierder…
…uette)

This PR was submitted for the 3.4 branch but it was merged into the 2.8 branch instead (closes #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
minor #27508 [Finder] Update RealIteratorTestCase (flip111)
This PR was submitted for the master branch but it was squashed and merged into the 2.8 branch instead (closes #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 | #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

@nicolas-grekas nicolas-grekas changed the base branch from 3.4 to 2.8 Jun 19, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 19, 2018

Thank you @Toflar.

@nicolas-grekas nicolas-grekas merged commit ab86f43 into symfony:2.8 Jun 19, 2018

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

nicolas-grekas added a commit that referenced this pull request Jun 19, 2018

bug #27309 Fix surrogate not using original request (Toflar)
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

@Toflar Toflar deleted the Toflar:fix-surrogate-not-using-original-request branch Jun 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment