minor #19233 [HttpKernel] Move handling of conflicting origin IPs to catch block (magnusnordlander, nicolas-grekas)

This PR was merged into the 2.7 branch.

Discussion
----------

[HttpKernel] Move handling of conflicting origin IPs to catch block

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #19217
| License       | MIT
| Doc PR        | -

Commits
-------

db84101 [HttpKernel] Add listener that checks when request has both Forwarded and X-Forwarded-For
1f00b55 [HttpKernel] Move conflicting origin IPs handling to catch block

Author: fabpot

src/Symfony/Bundle/FrameworkBundle/Resources/config/web.xml

@@ -46,5 +46,9 @@
             <argument type="service" id="request_stack" />
             <tag name="kernel.event_subscriber" />
         </service>
+
+        <service id="validate_request_listener" class="Symfony\Component\HttpKernel\EventListener\ValidateRequestListener">
+            <tag name="kernel.event_subscriber" />
+        </service>
     </services>
 </container>

src/Symfony/Bundle/FrameworkBundle/composer.json

@@ -22,8 +22,8 @@
         "symfony/config": "~2.4",
         "symfony/event-dispatcher": "~2.5",
         "symfony/finder": "~2.0,>=2.0.5",
-        "symfony/http-foundation": "~2.4.9|~2.5,>=2.5.4",
-        "symfony/http-kernel": "~2.7",
+        "symfony/http-foundation": "~2.7",
+        "symfony/http-kernel": "~2.7.15|~2.8.8",
         "symfony/filesystem": "~2.3",
         "symfony/routing": "~2.6,>2.6.4",
         "symfony/security-core": "~2.6.13|~2.7.9|~2.8",

src/Symfony/Component/HttpKernel/EventListener/ValidateRequestListener.php

@@ -0,0 +1,55 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\EventListener;
+
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Validates that the headers and other information indicating the
+ * client IP address of a request are consistent.
+ *
+ * @author Magnus Nordlander <[email protected]>
+ */
+class ValidateRequestListener implements EventSubscriberInterface
+{
+    /**
+     * Performs the validation.
+     *
+     * @param GetResponseEvent $event
+     */
+    public function onKernelRequest(GetResponseEvent $event)
+    {
+        if (!$event->isMasterRequest()) {
+            return;
+        }
+        $request = $event->getRequest();
+
+        if ($request::getTrustedProxies()) {
+            // This will throw an exception if the headers are inconsistent.
+            $request->getClientIps();
+        }
+    }
+
+    /**
+     * {@inheritdoc}
+     */
+    public static function getSubscribedEvents()
+    {
+        return array(
+            KernelEvents::REQUEST => array(
+                array('onKernelRequest', 256),
+            ),
+        );
+    }
+}

src/Symfony/Component/HttpKernel/HttpKernel.php

@@ -63,6 +63,9 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ
         try {
             return $this->handleRaw($request, $type);
         } catch (\Exception $e) {
+            if ($e instanceof ConflictingHeadersException) {
+                $e = new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e);
+            }
             if (false === $catch) {
                 $this->finishRequest($request, $type);
 
@@ -115,13 +118,6 @@ public function terminateWithException(\Exception $exception)
      */
     private function handleRaw(Request $request, $type = self::MASTER_REQUEST)
     {
-        if (self::MASTER_REQUEST === $type && $request::getTrustedProxies()) {
-            try {
-                $request->getClientIps();
-            } catch (ConflictingHeadersException $e) {
-                throw new BadRequestHttpException('The request headers contain conflicting information regarding the origin of this request.', $e);
-            }
-        }
         $this->requestStack->push($request);
 
         // request

src/Symfony/Component/HttpKernel/Tests/EventListener/ValidateRequestListenerTest.php

@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\HttpKernel\Tests\EventListener;
+
+use Symfony\Component\EventDispatcher\EventDispatcher;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\EventListener\ValidateRequestListener;
+use Symfony\Component\HttpKernel\Event\GetResponseEvent;
+use Symfony\Component\HttpKernel\HttpKernelInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+class ValidateRequestListenerTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @expectedException Symfony\Component\HttpFoundation\Exception\ConflictingHeadersException
+     */
+    public function testListenerThrowsWhenMasterRequestHasInconsistentClientIps()
+    {
+        $dispatcher = new EventDispatcher();
+        $kernel = $this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface');
+
+        $request = new Request();
+        $request->setTrustedProxies(array('1.1.1.1'));
+        $request->server->set('REMOTE_ADDR', '1.1.1.1');
+        $request->headers->set('FORWARDED', '2.2.2.2');
+        $request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
+
+        $dispatcher->addListener(KernelEvents::REQUEST, array(new ValidateRequestListener(), 'onKernelRequest'));
+        $event = new GetResponseEvent($kernel, $request, HttpKernelInterface::MASTER_REQUEST);
+
+        $dispatcher->dispatch(KernelEvents::REQUEST, $event);
+    }
+}

src/Symfony/Component/HttpKernel/Tests/HttpKernelTest.php

@@ -276,26 +276,20 @@ public function testVerifyRequestStackPushPopDuringHandle()
      */
     public function testInconsistentClientIpsOnMasterRequests()
     {
-        $kernel = new HttpKernel(new EventDispatcher(), $this->getResolver());
-        $request = new Request();
-        $request->setTrustedProxies(array('1.1.1.1'));
-        $request->server->set('REMOTE_ADDR', '1.1.1.1');
-        $request->headers->set('FORWARDED', '2.2.2.2');
-        $request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addListener(KernelEvents::REQUEST, function ($event) {
+            $event->getRequest()->getClientIp();
+        });
 
-        $kernel->handle($request, $kernel::MASTER_REQUEST, false);
-    }
+        $kernel = new HttpKernel($dispatcher, $this->getResolver());
 
-    public function testInconsistentClientIpsOnSubRequests()
-    {
-        $kernel = new HttpKernel(new EventDispatcher(), $this->getResolver());
         $request = new Request();
         $request->setTrustedProxies(array('1.1.1.1'));
         $request->server->set('REMOTE_ADDR', '1.1.1.1');
         $request->headers->set('FORWARDED', '2.2.2.2');
         $request->headers->set('X_FORWARDED_FOR', '3.3.3.3');
 
-        $this->assertInstanceOf('Symfony\Component\HttpFoundation\Response', $kernel->handle($request, $kernel::SUB_REQUEST, false));
+        $kernel->handle($request, $kernel::MASTER_REQUEST, false);
     }
 
     protected function getResolver($controller = null)