Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 54c9054

Browse filesBrowse files
fabpotfabpot
committed
bug #37182 [HttpKernel] Fix regression where Store does not return response body correctly (mpdude)
This PR was squashed before being merged into the 3.4 branch. Discussion ---------- [HttpKernel] Fix regression where Store does not return response body correctly | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | Fix #37174 | License | MIT | Doc PR | Since #36833, the `Store` no longer uses or trusts the `X-Content-Digest` header present on a response, since that may come (in the case of using `CachingHttpClient`) from upstream HTTP sources. Instead, the `X-Content-Digest` is re-computed every time a response is written by the `Store`. Additionally, the `Store` is implemented in a way that when restoring responses, it does _not_ actually load the response body, but just keeps the file path to the content on disk in another internal header called `X-Body-File`. It is up to others (`HttpCache`, for example) to actually load the content from there. For reasons I could not determine, the file path is also set as the response body. When the `HttpCache` performs revalidations, it may happen that it wants the `Store` to persist a previously restored response. In that case, the `Store` fails to honor its own `X-Body-File` header. Instead, it would compute (since #36833) the `X-Content-Digest`, which now is a hash of the cache file path. So, we end up with a response that still carries `X-Body-File` for the original, correct response. Since the `HttpCache` honors this value, we don't immediately notice that. But inside the `Store`, the request is now associated with the _new_ (bogus) content entry. It takes another round of looking up the content in the `Store` to now get a response where the `X-Body-File` _also_ points to the wrong content entry. Although I feel a bit uncomfortable with trusting headers that seemingly need to be evaluated in different classes and may come from elsewhere, my suggestion is to skip the write inside `Store` if `X-Body-File` and `X-Content-Digest` are both present and consistent with each other. Additionally, a `file_exists` check could be added to provide additional assertions, at the cost of accessing the filesystem. Commits ------- 176e769 [HttpKernel] Fix regression where Store does not return response body correctly
2 parents f87c993 + 176e769 commit 54c9054
Copy full SHA for 54c9054

File tree

Expand file treeCollapse file tree

2 files changed

+60
-14
lines changed
Filter options
Expand file treeCollapse file tree

2 files changed

+60
-14
lines changed

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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/HttpCache/Store.php
+27-14Lines 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

Copy file name to clipboardExpand all lines: src/Symfony/Component/HttpKernel/Tests/HttpCache/StoreTest.php
+33Lines 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)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.