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

HttpCache does not consider ESI resources in HEAD requests #24243

Closed
wants to merge 4 commits into
base: 2.7
from

Conversation

Projects
None yet
5 participants
@mpdude
Contributor

mpdude commented Sep 17, 2017

Q A
Branch? 2.7
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR

Due to this shortcut:

private function restoreResponseBody(Request $request, Response $response)
{
if ($request->isMethod('HEAD') || 304 === $response->getStatusCode()) {
$response->setContent(null);
$response->headers->remove('X-Body-Eval');
$response->headers->remove('X-Body-File');
return;
}

... the HttpCache never looks at the response body for HEAD requests. This makes it completely miss ESI-related tweaks like computing the correct TTL, removing validation headers or updating the Content-Length.

From RFC2616 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4):

The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request.

Although it says "SHOULD", I think it can be misleading at best when HEAD requests do, for example, return different (greater) s-maxage values than a corresponding GET request.

@@ -1133,7 +1133,7 @@ public function testEsiCacheSendsTheLowestTtl()
array(
'status' => 200,
'body' => 'Hello World!',
'headers' => array('Cache-Control' => 's-maxage=300'),
'headers' => array('Cache-Control' => 's-maxage=200'),

This comment has been minimized.

@mpdude

mpdude Sep 17, 2017

Contributor

Using a different maxage here so one can better tell the three responses apart.

@mpdude

mpdude Sep 17, 2017

Contributor

Using a different maxage here so one can better tell the three responses apart.

@@ -1147,8 +1147,33 @@ public function testEsiCacheSendsTheLowestTtl()
$this->request('GET', '/', array(), array(), true);
$this->assertEquals('Hello World! My name is Bobby.', $this->response->getContent());
// check for 100 or 99 as the test can be executed after a second change
$this->assertTrue(in_array($this->response->getTtl(), array(99, 100)));

This comment has been minimized.

@mpdude

mpdude Sep 17, 2017

Contributor

We probably don't need this workaround anymore since we've got clock mocking (agree @nicolas-grekas?)

@mpdude

mpdude Sep 17, 2017

Contributor

We probably don't need this workaround anymore since we've got clock mocking (agree @nicolas-grekas?)

This comment has been minimized.

@fabpot

fabpot Sep 27, 2017

Member

agreed :)

@fabpot

fabpot Sep 27, 2017

Member

agreed :)

@@ -1192,7 +1248,6 @@ public function testEsiRecalculateContentLengthHeader()
'body' => '<esi:include src="/foo" />',
'headers' => array(
'Content-Length' => 26,
'Cache-Control' => 's-maxage=300',

This comment has been minimized.

@mpdude

mpdude Sep 17, 2017

Contributor

Removed as it is not relevant for this test case

@mpdude

mpdude Sep 17, 2017

Contributor

Removed as it is not relevant for this test case

@@ -633,14 +633,6 @@ protected function store(Request $request, Response $response)
*/
private function restoreResponseBody(Request $request, Response $response)
{
if ($request->isMethod('HEAD') || 304 === $response->getStatusCode()) {

This comment has been minimized.

@mpdude

mpdude Sep 17, 2017

Contributor

The 304 === ... part was added a long long time ago in #1413, seemingly not covered by tests.

I think it can safely be removed because $response is the response taken from the cache, which cannot be a 304.

@mpdude

mpdude Sep 17, 2017

Contributor

The 304 === ... part was added a long long time ago in #1413, seemingly not covered by tests.

I think it can safely be removed because $response is the response taken from the cache, which cannot be a 304.

// Response does not include possibly dynamic content (ESI, SSI), so we need
// not handle the content for HEAD requests
if (!$request->isMethod('HEAD')) {
$response->setContent(file_get_contents($response->headers->get('X-Body-File')));

This comment has been minimized.

@mpdude

mpdude Sep 17, 2017

Contributor

This tries to keep part of the performance optimization removed above – if X-Body-File is intended to mark responses that do not contain dynamic content like ESI/SSI...?

@mpdude

mpdude Sep 17, 2017

Contributor

This tries to keep part of the performance optimization removed above – if X-Body-File is intended to mark responses that do not contain dynamic content like ESI/SSI...?

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Sep 18, 2017

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Sep 26, 2017

Contributor

@fabpot The license header in the src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php file is somewhat special. Anything I can do to make fabbot happy?

Contributor

mpdude commented Sep 26, 2017

@fabpot The license header in the src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php file is somewhat special. Anything I can do to make fabbot happy?

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 27, 2017

Member

@mpdude No, just keep it like it is.

Member

fabpot commented Sep 27, 2017

@mpdude No, just keep it like it is.

@fabpot

fabpot approved these changes Sep 27, 2017

with minor comments

@xabbuh

This comment has been minimized.

Show comment
Hide comment
@xabbuh

xabbuh Sep 27, 2017

Member

But doesn't this now always violate the "The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response." part?

Member

xabbuh commented Sep 27, 2017

But doesn't this now always violate the "The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response." part?

@mpdude

This comment has been minimized.

Show comment
Hide comment
@mpdude

mpdude Sep 27, 2017

Contributor

@xabbuh – not sure what you mean. That no body should be returned? There's code elsewhere (can figure it out if you like) that will remove the body for HEAD requests. Until then, we need it to determine Content-Length.

Does that answer your question?

Contributor

mpdude commented Sep 27, 2017

@xabbuh – not sure what you mean. That no body should be returned? There's code elsewhere (can figure it out if you like) that will remove the body for HEAD requests. Until then, we need it to determine Content-Length.

Does that answer your question?

@xabbuh

xabbuh approved these changes Sep 28, 2017

@mpdude Thanks, we indeed call Response::prepare() afterwards. Thus, this looks safe to me. 👍

@fabpot

This comment has been minimized.

Show comment
Hide comment
@fabpot

fabpot Sep 28, 2017

Member

Thank you @mpdude.

Member

fabpot commented Sep 28, 2017

Thank you @mpdude.

fabpot added a commit that referenced this pull request Sep 28, 2017

bug #24243 HttpCache does not consider ESI resources in HEAD requests…
… (mpdude)

This PR was squashed before being merged into the 2.7 branch (closes #24243).

Discussion
----------

HttpCache does not consider ESI resources in HEAD requests

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Due to this shortcut:
https://github.com/symfony/symfony/blob/3b42d8859ea98fdd17012e21f774f55d33cccc3f/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php#L634-L642
... the `HttpCache` never looks at the response body for `HEAD` requests. This makes it completely miss ESI-related tweaks like computing the correct TTL, removing validation headers or updating the `Content-Length`.

From RFC2616 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9.4):

> The HEAD method is identical to GET except that the server MUST NOT return a message-body in the response. The metainformation contained in the HTTP headers in response to a HEAD request SHOULD be identical to the information sent in response to a GET request.

Although it says "SHOULD", I think it can be misleading at best when HEAD requests do, for example, return different (greater) `s-maxage` values than a corresponding GET request.

Commits
-------

4dd0e53 HttpCache does not consider ESI resources in HEAD requests

@xabbuh xabbuh closed this Sep 28, 2017

@mpdude mpdude deleted the webfactory:esi_broken_in_head_requests branch Sep 28, 2017

This was referenced Oct 5, 2017

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