Skip to content

Commit

Permalink
Fix regression caused by symfony#36833 and reported in symfony#37174.
Browse files Browse the repository at this point in the history
  • Loading branch information
mpdude committed Jun 10, 2020
1 parent f87c993 commit b0b8cf4
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 14 deletions.
41 changes: 27 additions & 14 deletions src/Symfony/Component/HttpKernel/HttpCache/Store.php
Expand Up @@ -152,8 +152,8 @@ public function lookup(Request $request)
}

$headers = $match[1];
if (file_exists($body = $this->getPath($headers['x-content-digest'][0]))) {
return $this->restoreResponse($headers, $body);
if (file_exists($path = $this->getPath($headers['x-content-digest'][0]))) {
return $this->restoreResponse($headers, $path);
}

// TODO the metaStore referenced an entity that doesn't exist in
Expand All @@ -177,15 +177,28 @@ public function write(Request $request, Response $response)
$key = $this->getCacheKey($request);
$storedEnv = $this->persistRequest($request);

$digest = $this->generateContentDigest($response);
$response->headers->set('X-Content-Digest', $digest);
if ($response->headers->has('X-Body-File')) {
// Assume the response came from disk, but at least perform some safeguard checks
if (!$response->headers->has('X-Content-Digest')) {
throw new \RuntimeException('A restored response must have the X-Content-Digest header');
}

if (!$this->save($digest, $response->getContent(), false)) {
throw new \RuntimeException('Unable to store the entity.');
}
$digest = $response->headers->get('X-Content-Digest');
if ($this->getPath($digest) !== $response->headers->get('X-Body-File')) {
throw new \RuntimeException('X-Body-File and X-Content-Digest do not match');
}
// Everything seems ok, omit writing content to disk
} else {
$digest = $this->generateContentDigest($response);
$response->headers->set('X-Content-Digest', $digest);

if (!$response->headers->has('Transfer-Encoding')) {
$response->headers->set('Content-Length', \strlen($response->getContent()));
if (!$this->save($digest, $response->getContent(), false)) {
throw new \RuntimeException('Unable to store the entity.');
}

if (!$response->headers->has('Transfer-Encoding')) {
$response->headers->set('Content-Length', \strlen($response->getContent()));
}
}

// read existing cache entries, remove non-varying, and add this one to the list
Expand Down Expand Up @@ -477,19 +490,19 @@ private function persistResponse(Response $response)
* Restores a Response from the HTTP headers and body.
*
* @param array $headers An array of HTTP headers for the Response
* @param string $body The Response body
* @param string $path Path to the Response body
*
* @return Response
*/
private function restoreResponse($headers, $body = null)
private function restoreResponse($headers, $path = null)
{
$status = $headers['X-Status'][0];
unset($headers['X-Status']);

if (null !== $body) {
$headers['X-Body-File'] = [$body];
if (null !== $path) {
$headers['X-Body-File'] = [$path];
}

return new Response($body, $status, $headers);
return new Response($path, $status, $headers);
}
}
33 changes: 33 additions & 0 deletions src/Symfony/Component/HttpKernel/Tests/HttpCache/StoreTest.php
Expand Up @@ -118,6 +118,39 @@ public function testWritesResponseEvenIfXContentDigestIsPresent()
$this->assertNotNull($response);
}

public function testWritingARestoredResponseDoesNotCorruptCache()
{
/*
* This covers the regression reported in https://github.com/symfony/symfony/issues/37174.
*
* A restored response does *not* load the body, but only keep the file path in a special X-Body-File
* header. For reasons (?), the file path was also used as the restored response body.
* It would be up to others (HttpCache...?) to hohor this header and actually load the response content
* from there.
*
* When a restored response was stored again, the Store itself would ignore the header. In the first
* step, this would compute a new Content Digest based on the file path in the restored response body;
* this is covered by "Checkpoint 1" below. But, since the X-Body-File header was left untouched (Checkpoint 2), downstream
* code (HttpCache...) would not immediately notice.
*
* Only upon performing the lookup for a second time, we'd get a Response where the (wrong) Content Digest
* is also reflected in the X-Body-File header, this time also producing wrong content when the downstream
* evaluates it.
*/
$this->store->write($this->request, $this->response);
$digest = $this->response->headers->get('X-Content-Digest');
$path = $this->getStorePath($digest);

$response = $this->store->lookup($this->request);
$this->store->write($this->request, $response);
$this->assertEquals($digest, $response->headers->get('X-Content-Digest')); // Checkpoint 1
$this->assertEquals($path, $response->headers->get('X-Body-File')); // Checkpoint 2

$response = $this->store->lookup($this->request);
$this->assertEquals($digest, $response->headers->get('X-Content-Digest'));
$this->assertEquals($path, $response->headers->get('X-Body-File'));
}

public function testFindsAStoredEntryWithLookup()
{
$this->storeSimpleEntry();
Expand Down

0 comments on commit b0b8cf4

Please sign in to comment.