8000 [HttpKernel] Fix regression where Store does not return response body… · symfony/symfony@176e769 · GitHub
[go: up one dir, main page]
Skip to content

Commit 176e769

Browse files
mpdudefabpot
mpdude
authored and
fabpot
committed
[HttpKernel] Fix regression where Store does not return response body correctly
1 parent f87c993 commit 176e769

File tree

2 files changed

+60
-14
lines changed

2 files changed

+60
-14
lines changed

‎src/Symfony/Component/HttpKernel/HttpCache/Store.php

Lines changed: 27 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,8 @@ public function lookup(Request $request)
152152
}
153153

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

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

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

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

187-
if (!$response->headers->has('Transfer-Encoding')) {
188-
$response->headers->set('Content-Length', \strlen($response->getContent()));
195+
if (!$this->save($digest, $response->getContent(), false)) {
196+
throw new \RuntimeException('Unable to store the entity.');
197+
}
198+
199+
if (!$response->headers->has('Transfer-Encoding')) {
200+
$response->headers->set('Content-Length', \strlen($response->getContent()));
201+
}
189202
}
190203

191204
// read existing cache entries, remove non-varying, and add this one to the list
@@ -477,19 +490,19 @@ private function persistResponse(Response $response)
477490
* Restores a Response from the HTTP headers and body.
478491
*
479492
* @param array $headers An array of HTTP headers for the Response
480-
* @param string $body The Response body
493+
* @param string $path Path to the Response body
481494
*
482495
* @return Response
483496
*/
484-
private function restoreResponse($headers, $body = null)
497+
private function restoreResponse($headers, $path = null)
485498
{
486499
$status = $headers['X-Status'][0];
487500
unset($headers['X-Status']);
488501

489-
if (null !== $body) {
490-
$headers['X-Body-File'] = [$body];
502+
if (null !== $path) {
503+
$headers['X-Body-File'] = [$path];
491504
}
492505

493-
return new Response($body, $status, $headers);
506+
return new Response($path, $status, $headers);
494507
}
495508
}

‎src/Symfony/Component/HttpKernel/Tests/HttpCache/StoreTest.php

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,39 @@ public function testWritesResponseEvenIfXContentDigestIsPresent()
118118
$this->assertNotNull($response);
119119
}
120120

121+
public function testWritingARestoredResponseDoesNotCorruptCache()
122+
{
123+
/*
124+
* This covers the regression reported in https://github.com/symfony/symfony/issues/37174.
125+
*
126+
* A restored response does *not* load the body, but only keep the file path in a special X-Body-File
127+
* header. For reasons (?), the file path was also used as the restored response body.
128+
* It would be up to others (HttpCache...?) to hohor this header and actually load the response content
129+
* from there.
130+
*
131+
* When a restored response was stored again, the Store itself would ignore the header. In the first
132+
* step, this would compute a new Content Digest based on the file path in the restored response body;
133+
* this is covered by "Checkpoint 1" below. But, since the X-Body-File header was left untouched (Checkpoint 2), downstream
134+
* code (HttpCache...) would not immediately notice.
135+
*
136+
* Only upon performing the lookup for a second time, we'd get a Response where the (wrong) Content Digest
137+
* is also reflected in the X-Body-File header, this time also producing wrong content when the downstream
138+
* evaluates it.
139+
*/
140+
$this->store->write($this->request, $this->response);
141+
$digest = $this->response->headers->get('X-Content-Digest');
142+
$path = $this->getStorePath($digest);
143+
144+
$response = $this->store->lookup($this->request);
145+
$this->store->write($this->request, $response);
146+
$this->assertEquals($digest, $response->headers->get('X-Content-Digest')); // Checkpoint 1
147+
$this->assertEquals($path, $response->headers->get('X-Body-File')); // Checkpoint 2
148+
149+
$response = $this->store->lookup($this->request);
150+
$this->assertEquals($digest, $response->headers->get('X-Content-Digest'));
151+
$this->assertEquals($path, $response->headers->get('X-Body-File'));
152+
}
153+
121154
public function testFindsAStoredEntryWithLookup()
122155
{
123156
$this->storeSimpleEntry();

0 commit comments

Comments
 (0)
0