Skip to content

HttpCache does not consider ESI resources in HEAD requests #24243

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

HttpCache does not consider ESI resources in HEAD requests #24243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bcf00f1
Add tests to cover ESI tag handling during HEAD requests
mpdude Sep 17, 2017
8614c35
For ESI/SSI responses, evaluate body also during HEAD requests
mpdude Sep 17, 2017
6c3602d
Fix LICENSE header (although I did not touch it)
mpdude Sep 17, 2017
3beccc6
Fix license header
mpdude Sep 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 5 additions & 9 deletions src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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->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')));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...?

}
} else {
return;
}
Expand Down
123 changes: 119 additions & 4 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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'),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

),
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)));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed :)

$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',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as it is not relevant for this test case

'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