Skip to content
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

Jump to bottom

ESI subrequests fail when using caching directives (Symfony 2.2) #7091

Closed
mweimerskirch opened this issue Feb 16, 2013 · 3 comments
Closed

ESI subrequests fail when using caching directives (Symfony 2.2) #7091

mweimerskirch opened this issue Feb 16, 2013 · 3 comments

Comments

@mweimerskirch
Copy link
Contributor

I noticed ESI subrequests fail in Symfony 2.2 when using an "smaxage" header. After a debugging session I noticed that this was due to "AccessDeniedHttpException" being thrown because the subrequests were given the original client IP (83.xx.xx.xx) instead of the local one (127.0.0.1). Important: This only happens when using an "smaxage" header to cache the response of the subrequest. Without the smaxage header there is no problem. And it only happens with subrequest, not when using an smaxage header on the main response.

I use ESI tags ({{ render_esi(controller('XXXBundle:Default:yyy', {})) }}) and add an "smaxage" header to cache the response of the subrequest. The first request is fine (when there is no cache yet) but on the second request (after the cache has been created) the ESI part fails to include and the log shows an "AccessDeniedHttpException".

The problem can not be reproduced locally because obviously the original client IP (127.0.0.1) always equals the "trusted" proxy IP (127.0.0.1).
@mweimerskirch
Copy link
Contributor Author

After investigating a bit further I fount that $subRequest->server->set('REMOTE_ADDR', '127.0.0.1'); is called in the "fetch()" method, i.e. only when the cache is missed. It should be called earlier so the IP address is also overridden for retrieving cached entries.

@mweimerskirch mweimerskirch mentioned this issue Feb 16, 2013
Closed
fabpot pushed a commit that referenced this issue Feb 17, 2013
@mweimerskirch @fabpot
Fix REMOTE_ADDR for cached subrequests
0eff68f 
Fixes #7091
fabpot added a commit that referenced this issue Feb 17, 2013
@fabpot
merged branch mweimerskirch/patch-12 (PR #7092)
8fc69c3 
This PR was submitted for the 2.2 branch but it was merged into the 2.1 branch instead (closes #7092).

Commits
-------

187645f Fix REMOTE_ADDR for cached subrequests

Discussion
----------

[HttpKernel/HttpCache] Fix "REMOTE_ADDR" for cached subrequests

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | none that I know of
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | 7091
| License       | MIT

I moved the code that modifies the REMOTE_ADDR variable further up the chain so that cached subrequests also receive the local IP address. Before, only new subrequests received the local IP address and cached ones received the original IP, which made "validateRequest" in FragmentListener fail.

Please review. I'm not sure about side-effects of this patch, including possible security issues.

Fixes #7091

---------------------------------------------------------------------------

by bamarni at 2013-02-16T11:49:27Z

@fabpot rejected setting this on the master request, so you should do it on the ```forward()``` method instead.

---------------------------------------------------------------------------

by mweimerskirch at 2013-02-16T12:13:46Z

@bamarni @fabpot done
@fabpot fabpot closed this as completed Feb 17, 2013
@ChristianRiesen
Copy link

@mweimerskirch Sent you an email. I think this fix introduces a new bug...

@ChristianRiesen
Copy link

False alarm, works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@fabpot @mweimerskirch @ChristianRiesen