8000 [HttpKernel] Fix regression where Store does not return response body correctly by mpdude · Pull Request #37182 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpKernel] Fix regression where Store does not return response body correctly #37182

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

Merged
merged 1 commit into from
Jun 11, 2020
Merged

[HttpKernel] Fix regression where Store does not return response body correctly #37182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 27 additions & 14 deletions src/Symfony/Component/HttpKernel/HttpCache/Store.php
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Copy link
Contributor

Choose a reason for hiding this comment

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

string|null

*
* @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
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Copy link
Member

Choose a reason for hiding this comment

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

honor

* 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