Fix surrogate not using original request

symfony · nicolas-grekas · Jun 19, 2018 · Jun 18, 2018 · Jun 19, 2018 · Jun 19, 2018
commit ab86f43d7877a906d22713d7fae793f2076ce3f6
diff --git a/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php b/src/Symfony/Component/HttpKernel/HttpCache/HttpCache.php
@@ -185,7 +185,11 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
        // FIXME: catch exceptions and implement a 500 error page here? -> in Varnish, there is a built-in error page mechanism
        if (HttpKernelInterface::MASTER_REQUEST === $type) {
            $this->traces = array();
-            $this->request = $request;
+            // Keep a clone of the original request for surrogates so they can access it.
+            // We must clone here to get a separate instance because the application will modify the request during
+            // the application flow (we know it always does because we do ourselves by setting REMOTE_ADDR to 127.0.0.1
+            // and adding the X-Forwarded-For header, see HttpCache::forward()).
+            $this->request = clone $request;
            if (null !== $this->surrogate) {
                $this->surrogateCacheStrategy = $this->surrogate->createCacheStrategy();
            }

diff --git a/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php b/src/Symfony/Component/HttpKernel/Tests/HttpCache/HttpCacheTest.php
@@ -11,9 +11,12 @@

 namespace Symfony\Component\HttpKernel\Tests\HttpCache;

+use Symfony\Component\HttpKernel\HttpCache\Esi;
 use Symfony\Component\HttpKernel\HttpCache\HttpCache;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\HttpCache\Store;
+use Symfony\Component\HttpKernel\HttpCache\StoreInterface;
 use Symfony\Component\HttpKernel\HttpKernelInterface;

 /**
@@ -1432,6 +1435,42 @@ public function testDoesNotCacheOptionsRequest()
        $this->assertHttpKernelIsNotCalled();
        $this->assertSame('get', $this->response->getContent());
    }
+
+    public function testUsesOriginalRequestForSurrogate()
+    {
+        $kernel = $this->getMockBuilder(HttpKernelInterface::class)->getMock();
+        $store = $this->getMockBuilder(StoreInterface::class)->getMock();
+
+        $kernel
+            ->expects($this->exactly(2))
+            ->method('handle')
+            ->willReturnCallback(function (Request $request) {
+                $this->assertSame('127.0.0.1', $request->server->get('REMOTE_ADDR'));
+
+                return new Response();
+            });
+
+        $cache = new HttpCache($kernel,
+            $store,
+            new Esi()
+        );
+
+        $request = Request::create('/');
+        $request->server->set('REMOTE_ADDR', '10.0.0.1');
+
+        // Main request
+        $cache->handle($request, HttpKernelInterface::MASTER_REQUEST);
+
+        // Main request was now modified by HttpCache
+        // The surrogate will ask for the request using $this->cache->getRequest()
+        // which MUST return the original request so the surrogate
+        // can actually behave like a reverse proxy like e.g. Varnish would.
+        $this->assertSame('10.0.0.1', $cache->getRequest()->getClientIp());
+        $this->assertSame('10.0.0.1', $cache->getRequest()->server->get('REMOTE_ADDR'));
+
+        // Surrogate request
+        $cache->handle($request, HttpKernelInterface::SUB_REQUEST);
+    }
 }

 class TestKernel implements HttpKernelInterface