Merge pull request #274 from bastnic/feature/hash-sanity-check

check sanity of user context hash, if not, prevent setting any cache
FriendsOfSymfony · Feb 24, 2016 · f12b74b · f12b74b
2 parents 26e44bd + 9543119
commit f12b74b
Show file tree

Hide file tree

Showing 3 changed files with 95 additions and 1 deletion.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,6 +1,19 @@
 Changelog
 =========
 
+
+1.3.7
+-----
+
+* Add a sanity check on UserContextHash to avoid invalid content being cached
+  (example: anonymous cache set for authenticated user). This scenario occures
+  when the UserContextHash is cached by Varnish via 
+  `fos_http_cache.user_context.hash_cache_ttl` > 0 and the session is lost via 
+  garbage collector. The data given is the anonymous one despite having a hash 
+  for authenticated, all authenticated users will then have the anonymous version.
+  Same problem could occurs with users having is role changed or anything else
+  that can modify the hash.
+
 1.3.2
 -----
 

diff --git a/EventListener/UserContextSubscriber.php b/EventListener/UserContextSubscriber.php
@@ -46,6 +46,11 @@ class UserContextSubscriber implements EventSubscriberInterface
      */
     private $userIdentifierHeaders;
 
+    /**
+     * @var string
+     */
+    private $hash;
+
     /**
      * @var string
      */
@@ -88,6 +93,10 @@ public function onKernelRequest(GetResponseEvent $event)
         }
 
         if (!$this->requestMatcher->matches($event->getRequest())) {
+            if ($event->getRequest()->headers->has($this->hashHeader)) {
+                $this->hash = $this->hashGenerator->generateHash();
+            }
+
             return;
         }
 
@@ -124,9 +133,19 @@ public function onKernelResponse(FilterResponseEvent $event)
         }
 
         $response = $event->getResponse();
+        $request = $event->getRequest();
+
         $vary = $response->getVary();
 
-        if ($event->getRequest()->headers->has($this->hashHeader)) {
+        if ($request->headers->has($this->hashHeader)) {
+            // hash has changed, session has most certainly changed, prevent setting incorrect cache
+            if (!is_null($this->hash) && $this->hash !== $request->headers->get($this->hashHeader)) {
+                $response->setClientTtl(0);
+                $response->headers->addCacheControlDirective('no-cache');
+
+                return;
+            }
+
             if (!in_array($this->hashHeader, $vary)) {
                 $vary[] = $this->hashHeader;
             }

diff --git a/Tests/Unit/EventListener/UserContextSubscriberTest.php b/Tests/Unit/EventListener/UserContextSubscriberTest.php
@@ -104,6 +104,7 @@ public function testOnKernelRequestNotMatched()
 
         $requestMatcher = $this->getRequestMatcher($request, false);
         $hashGenerator = \Mockery::mock('\FOS\HttpCache\UserContext\HashGenerator');
+        $hashGenerator->shouldReceive('generateHash')->andReturn('hash');
 
         $userContextSubscriber = new UserContextSubscriber($requestMatcher, $hashGenerator, array('X-SessionId'), 'X-Hash');
         $event = $this->getKernelRequestEvent($request);
@@ -168,6 +169,67 @@ public function testOnKernelResponseNotCached()
         $this->assertEquals('X-SessionId', $event->getResponse()->headers->get('Vary'));
     }
 
+    /**
+     * If there is no hash in the request, vary on the user identifier.
+     */
+    public function testFullRequestHashOk()
+    {
+        $request = new Request();
+        $request->setMethod('GET');
+        $request->headers->set('X-Hash', 'hash');
+
+        $requestMatcher = $this->getRequestMatcher($request, false);
+        $hashGenerator = \Mockery::mock('\FOS\HttpCache\UserContext\HashGenerator');
+        $hashGenerator->shouldReceive('generateHash')->andReturn('hash');
+
+        // onKernelRequest
+        $userContextSubscriber = new UserContextSubscriber($requestMatcher, $hashGenerator, array('X-SessionId'), 'X-Hash');
+        $event = $this->getKernelRequestEvent($request);
+
+        $userContextSubscriber->onKernelRequest($event);
+
+        $response = $event->getResponse();
+
+        $this->assertNull($response);
+
+        // onKernelResponse
+        $event = $this->getKernelResponseEvent($request);
+        $userContextSubscriber->onKernelResponse($event);
+
+        $this->assertContains('X-Hash', $event->getResponse()->headers->get('Vary'));
+    }
+
+    /**
+     * If there is no hash in the requests but session changed, prevent setting bad cache
+     */
+    public function testFullRequestHashChanged()
+    {
+        $request = new Request();
+        $request->setMethod('GET');
+        $request->headers->set('X-Hash', 'hash');
+
+        $requestMatcher = $this->getRequestMatcher($request, false);
+        $hashGenerator = \Mockery::mock('\FOS\HttpCache\UserContext\HashGenerator');
+        $hashGenerator->shouldReceive('generateHash')->andReturn('hash-changed');
+
+        // onKernelRequest
+        $userContextSubscriber = new UserContextSubscriber($requestMatcher, $hashGenerator, array('X-SessionId'), 'X-Hash');
+        $event = $this->getKernelRequestEvent($request);
+
+        $userContextSubscriber->onKernelRequest($event);
+
+        $response = $event->getResponse();
+
+        $this->assertNull($response);
+
+        // onKernelResponse
+        $event = $this->getKernelResponseEvent($request);
+        $userContextSubscriber->onKernelResponse($event);
+
+        $this->assertFalse($event->getResponse()->headers->has('Vary'));
+        $this->assertEquals('max-age=0, no-cache, private', $event->getResponse()->headers->get('Cache-Control'));
+    }
+
     protected function getKernelRequestEvent(Request $request, $type = HttpKernelInterface::MASTER_REQUEST)
     {
         return new GetResponseEvent(