Do not sanity check hash of anonymous requests (#289)

Author: dbu

DependencyInjection/FOSHttpCacheExtension.php

@@ -192,6 +192,9 @@ private function loadUserContext(ContainerBuilder $container, XmlFileLoader $loa
             ->replaceArgument(3, $config['user_hash_header'])
             ->replaceArgument(4, $config['hash_cache_ttl']);
 
+        $container->getDefinition($this->getAlias().'.user_context.anonymous_request_matcher')
+            ->replaceArgument(0, $config['user_identifier_headers']);
+
         if ($config['logout_handler']['enabled']) {
             $container->getDefinition($this->getAlias().'.user_context.logout_handler')
                 ->replaceArgument(1, $config['user_identifier_headers'])

EventListener/UserContextSubscriber.php

@@ -13,6 +13,7 @@
 
 use FOS\HttpCache\UserContext\HashGenerator;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\RequestMatcherInterface;
 use Symfony\Component\HttpKernel\Event\FilterResponseEvent;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
@@ -61,12 +62,22 @@ class UserContextSubscriber implements EventSubscriberInterface
      */
     private $ttl;
 
+    /**
+     * Used to exclude anonymous requests (no authentication nor session) from user hash sanity check.
+     * It prevents issues when the hash generator that is used returns a customized value for anonymous users,
+     * that differs from the documented, hardcoded one.
+     *
+     * @var RequestMatcherInterface
+     */
+    private $anonymousRequestMatcher;
+
     public function __construct(
         RequestMatcherInterface $requestMatcher,
         HashGenerator $hashGenerator,
         array $userIdentifierHeaders = array('Cookie', 'Authorization'),
         $hashHeader = 'X-User-Context-Hash',
-        $ttl = 0
+        $ttl = 0,
+        RequestMatcherInterface $anonymousRequestMatcher = null
     ) {
         if (!count($userIdentifierHeaders)) {
             throw new \InvalidArgumentException('The user context must vary on some request headers');
@@ -76,6 +87,7 @@ public function __construct(
         $this->userIdentifierHeaders = $userIdentifierHeaders;
         $this->hashHeader = $hashHeader;
         $this->ttl = $ttl;
+        $this->anonymousRequestMatcher = $anonymousRequestMatcher;
     }
 
     /**
@@ -93,7 +105,7 @@ public function onKernelRequest(GetResponseEvent $event)
         }
 
         if (!$this->requestMatcher->matches($event->getRequest())) {
-            if ($event->getRequest()->headers->has($this->hashHeader)) {
+            if ($event->getRequest()->headers->has($this->hashHeader) && !$this->isAnonymous($event->getRequest())) {
                 $this->hash = $this->hashGenerator->generateHash();
             }
 
@@ -120,6 +132,20 @@ public function onKernelRequest(GetResponseEvent $event)
         $event->setResponse($response);
     }
 
+    /**
+     * Tests if $request is an anonymous request or not.
+     *
+     * For backward compatibility reasons, true will be returned if no anonymous request matcher was provided.
+     *
+     * @param Request $request
+     *
+     * @return bool
+     */
+    private function isAnonymous(Request $request)
+    {
+        return $this->anonymousRequestMatcher ? $this->anonymousRequestMatcher->matches($request) : false;
+    }
+
     /**
      * Add the context hash header to the headers to vary on if the header was
      * present in the request.

Resources/config/user_context.xml

@@ -27,6 +27,7 @@
             <argument />
             <argument />
             <argument />
+            <argument type="service" id="fos_http_cache.user_context.anonymous_request_matcher" />
             <tag name="kernel.event_subscriber" />
         </service>
 
@@ -39,5 +40,9 @@
             <argument />
             <argument />
         </service>
+
+        <service id="fos_http_cache.user_context.anonymous_request_matcher" class="FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher">
+            <argument type="collection" />
+        </service>
     </services>
 </container>

Tests/Unit/EventListener/UserContextSubscriberTest.php

@@ -198,6 +198,39 @@ public function testFullRequestHashOk()
         $this->assertContains('X-Hash', $event->getResponse()->headers->get('Vary'));
     }
 
+    /**
+     * If the request is an anonymous one, no hash should be generated/validated.
+     */
+    public function testFullAnonymousRequestHashNotGenerated()
+    {
+        $request = new Request();
+        $request->setMethod('GET');
+        $request->headers->set('X-Hash', 'anonymous-hash');
+
+        $requestMatcher = $this->getRequestMatcher($request, false);
+        $hashGenerator = \Mockery::mock('\FOS\HttpCache\UserContext\HashGenerator');
+        $hashGenerator->shouldReceive('generateHash')->never();
+
+        $anonymousRequestMatcher = \Mockery::mock('\Symfony\Component\HttpFoundation\RequestMatcherInterface');
+        $anonymousRequestMatcher->shouldReceive('matches')->andReturn(true);
+
+        // onKernelRequest
+        $userContextSubscriber = new UserContextSubscriber($requestMatcher, $hashGenerator, array('X-SessionId'), 'X-Hash', 0, $anonymousRequestMatcher);
+        $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.
      */

Tests/Unit/UserContext/AnonymousRequestMatcherTest.php

@@ -0,0 +1,69 @@
+<?php
+
+/*
+ * This file is part of the FOSHttpCacheBundle package.
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\HttpCacheBundle\Tests\Unit\UserContext;
+
+use FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher;
+use PHPUnit_Framework_TestCase;
+use Symfony\Component\HttpFoundation\Request;
+
+class AnonymousRequestMatcherTest extends PHPUnit_Framework_TestCase
+{
+    public function testMatchAnonymousRequest()
+    {
+        $request = new Request();
+
+        $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);
+
+        $this->assertTrue($requestMatcher->matches($request));
+    }
+
+    public function testNoMatchIfCookie()
+    {
+        $request = new Request();
+        $request->headers->set('Cookie', 'PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42=25f6d9c5a843e3c948cd26902385a527');
+        $request->cookies->set('PHPSESSID7e476fc9f29f69d2ad6f11dbcd663b42', '25f6d9c5a843e3c948cd26902385a527');
+
+        $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);
+
+        $this->assertFalse($requestMatcher->matches($request));
+    }
+
+    public function testNoMatchIfEmptyCookieHeader()
+    {
+        $request = new Request();
+        $request->headers->set('Cookie', '');
+
+        $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);
+
+        $this->assertTrue($requestMatcher->matches($request));
+    }
+
+    public function testNoMatchIfAuthenticationHeader()
+    {
+        $request = new Request();
+        $request->headers->set('Authorization', 'foo: bar');
+
+        $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);
+
+        $this->assertFalse($requestMatcher->matches($request));
+    }
+
+    public function testMatchEmptyCookieHeaderHeader()
+    {
+        $request = new Request();
+        $request->headers->set('Cookie', '');
+
+        $requestMatcher = new AnonymousRequestMatcher(['Cookie', 'Authorization']);
+
+        $this->assertTrue($requestMatcher->matches($request));
+    }
+}

UserContext/AnonymousRequestMatcher.php

@@ -0,0 +1,47 @@
+<?php
+
+/*
+ * This file is part of the FOSHttpCacheBundle package.
+ *
+ * (c) FriendsOfSymfony <http://friendsofsymfony.github.com/>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace FOS\HttpCacheBundle\UserContext;
+
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\RequestMatcherInterface;
+
+/**
+ * Matches anonymous requests using a list of identification headers.
+ */
+class AnonymousRequestMatcher implements RequestMatcherInterface
+{
+    private $userIdentifierHeaders;
+
+    /**
+     * @param array $userIdentifierHeaders List of request headers that authenticate a non-anonymous request
+     */
+    public function __construct(array $userIdentifierHeaders)
+    {
+        $this->userIdentifierHeaders = $userIdentifierHeaders;
+    }
+
+    public function matches(Request $request)
+    {
+        foreach ($this->userIdentifierHeaders as $header) {
+            if ($request->headers->has($header)) {
+                if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) {
+                    // ignore empty cookie header
+                    continue;
+                }
+
+                return false;
+            }
+        }
+
+        return true;
+    }
+}

composer.json

@@ -29,7 +29,7 @@
         "mockery/mockery": "0.9.*",
         "monolog/monolog": "*",
         "sensio/framework-extra-bundle": "^2.3||^3.0",
-        "symfony/symfony": "^2.3||^3.0",
+        "symfony/symfony": "^2.3.4||^3.0",
         "symfony/phpunit-bridge": "^2.7||^3.0",
         "symfony/expression-language": "^2.4||^3.0",
         "symfony/monolog-bundle": "^2.3||^3.0",