bug #37182 [HttpKernel] Fix regression where Store does not return re…

…sponse 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

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

@@ -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
@@ -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
@@ -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);
     }
 }

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

@@ -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();