Skip to content

Commit

Permalink
bug #24243 HttpCache does not consider ESI resources in HEAD requests…
Browse files Browse the repository at this point in the history 
… (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
  • Loading branch information
@fabpot
fabpot committed Sep 28, 2017
2 parents 9ed6dd4 + 4dd0e53 commit d178a9b
Show file tree
Hide file tree
Showing 2 changed files with 124 additions and 13 deletions.
14 changes: 5 additions & 9 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Expand Up @@ -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()) {
$response->setContent(null);
$response->headers->remove('X-Body-Eval');
$response->headers->remove('X-Body-File');

return;
}

if ($response->headers->has('X-Body-Eval')) {
ob_start();

Expand All @@ -656,7 +648,11 @@ private function restoreResponseBody(Request $request, Response $response)
$response->headers->set('Content-Length', strlen($response->getContent()));
}
} elseif ($response->headers->has('X-Body-File')) {
$response->setContent(file_get_contents($response->headers->get('X-Body-File')));
// 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')));
}
} else {
return;
}
Expand Down
123 changes: 119 additions & 4 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
View file
Expand Up @@ -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'),
),
array(
'status' => 200,
Expand All @@ -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->assertEquals(100, $this->response->getTtl());
}

public function testEsiCacheSendsTheLowestTtlForHeadRequests()
{
$responses = array(
array(
'status' => 200,
'body' => 'I am a long-lived master response, but I embed a short-lived resource: <esi:include src="/foo" />',
'headers' => array(
'Cache-Control' => 's-maxage=300',
'Surrogate-Control' => 'content="ESI/1.0"',
),
),
array(
'status' => 200,
'body' => 'I am a short-lived resource',
'headers' => array('Cache-Control' => 's-maxage=100'),
),
);

$this->setNextResponses($responses);

$this->request('HEAD', '/', array(), array(), true);

$this->assertEmpty($this->response->getContent());
$this->assertEquals(100, $this->response->getTtl());
}

public function testEsiCacheForceValidation()
Expand Down Expand Up @@ -1184,6 +1209,37 @@ public function testEsiCacheForceValidation()
$this->assertTrue($this->response->headers->hasCacheControlDirective('no-cache'));
}

public function testEsiCacheForceValidationForHeadRequests()
{
$responses = array(
array(
'status' => 200,
'body' => 'I am the master response and use expiration caching, but I embed another resource: <esi:include src="/foo" />',
'headers' => array(
'Cache-Control' => 's-maxage=300',
'Surrogate-Control' => 'content="ESI/1.0"',
),
),
array(
'status' => 200,
'body' => 'I am the embedded resource and use validation caching',
'headers' => array('ETag' => 'foobar'),
),
);

$this->setNextResponses($responses);

$this->request('HEAD', '/', array(), array(), true);

// The response has been assembled from expiration and validation based resources
// This can neither be cached nor revalidated, so it should be private/no cache
$this->assertEmpty($this->response->getContent());
$this->assertNull($this->response->getTtl());
$this->assertTrue($this->response->mustRevalidate());
$this->assertTrue($this->response->headers->hasCacheControlDirective('private'));
$this->assertTrue($this->response->headers->hasCacheControlDirective('no-cache'));
}

public function testEsiRecalculateContentLengthHeader()
{
$responses = array(
Expand All @@ -1192,7 +1248,6 @@ public function testEsiRecalculateContentLengthHeader()
'body' => '<esi:include src="/foo" />',
'headers' => array(
'Content-Length' => 26,
'Cache-Control' => 's-maxage=300',
'Surrogate-Control' => 'content="ESI/1.0"',
),
),
Expand All @@ -1210,6 +1265,37 @@ public function testEsiRecalculateContentLengthHeader()
$this->assertEquals(12, $this->response->headers->get('Content-Length'));
}

public function testEsiRecalculateContentLengthHeaderForHeadRequest()
{
$responses = array(
array(
'status' => 200,
'body' => '<esi:include src="/foo" />',
'headers' => array(
'Content-Length' => 26,
'Surrogate-Control' => 'content="ESI/1.0"',
),
),
array(
'status' => 200,
'body' => 'Hello World!',
'headers' => array(),
),
);

$this->setNextResponses($responses);

$this->request('HEAD', '/', array(), array(), true);

// https://www.w3.org/Protocols/rfc2616/rfc2616-sec14.html#sec14.13
// "The Content-Length entity-header field indicates the size of the entity-body,
// in decimal number of OCTETs, sent to the recipient or, in the case of the HEAD
// method, the size of the entity-body that would have been sent had the request
// been a GET."
$this->assertEmpty($this->response->getContent());
$this->assertEquals(12, $this->response->headers->get('Content-Length'));
}

public function testClientIpIsAlwaysLocalhostForForwardedRequests()
{
$this->setNextResponse();
Expand Down Expand Up @@ -1301,6 +1387,35 @@ public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponses()
$this->assertNull($this->response->getLastModified());
}

public function testEsiCacheRemoveValidationHeadersIfEmbeddedResponsesAndHeadRequest()
{
$time = \DateTime::createFromFormat('U', time());

$responses = array(
array(
'status' => 200,
'body' => '<esi:include src="/hey" />',
'headers' => array(
'Surrogate-Control' => 'content="ESI/1.0"',
'ETag' => 'hey',
'Last-Modified' => $time->format(DATE_RFC2822),
),
),
array(
'status' => 200,
'body' => 'Hey!',
'headers' => array(),
),
);

$this->setNextResponses($responses);

$this->request('HEAD', '/', array(), array(), true);
$this->assertEmpty($this->response->getContent());
$this->assertNull($this->response->getETag());
$this->assertNull($this->response->getLastModified());
}

public function testDoesNotCacheOptionsRequest()
{
$this->setNextResponse(200, array('Cache-Control' => 'public, s-maxage=60'), 'get');
Expand Down

0 comments on commit d178a9b

Please sign in to comment.