From 338761245100e09dafdce47f7646ef7d4fc14f64 Mon Sep 17 00:00:00 2001
From: Fabien Potencier <fabien.potencier@gmail.com>
Date: Wed, 19 Jul 2017 07:07:01 +0200
Subject: [PATCH 1/4] [Security] refactored tests

---
 ...efaultAuthenticationSuccessHandlerTest.php | 80 +++++++------------
 1 file changed, 31 insertions(+), 49 deletions(-)

diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
index 2c133014cc..70836170c7 100644
--- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
+++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
@@ -12,31 +12,28 @@
 namespace Symfony\Component\Security\Http\Tests\Authentication;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler;
 
 class DefaultAuthenticationSuccessHandlerTest extends TestCase
 {
     private $httpUtils = null;
-
-    private $request = null;
-
     private $token = null;
 
     protected function setUp()
     {
         $this->httpUtils = $this->getMockBuilder('Symfony\Component\Security\Http\HttpUtils')->getMock();
-        $this->request = $this->getMockBuilder('Symfony\Component\HttpFoundation\Request')->getMock();
-        $this->request->headers = $this->getMockBuilder('Symfony\Component\HttpFoundation\HeaderBag')->getMock();
         $this->token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
     }
 
     public function testRequestIsRedirected()
     {
-        $response = $this->expectRedirectResponse('/');
+        $request = Request::create('/');
+        $response = $this->expectRedirectResponse($request, '/');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
@@ -48,24 +45,22 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
             'default_target_path' => '/dashboard',
         );
 
-        $response = $this->expectRedirectResponse('/dashboard');
+        $request = Request::create('/');
+        $response = $this->expectRedirectResponse($request, '/dashboard');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
 
     public function testTargetPathIsPassedWithRequest()
     {
-        $this->request->expects($this->once())
-            ->method('get')->with('_target_path')
-            ->will($this->returnValue('/dashboard'));
-
-        $response = $this->expectRedirectResponse('/dashboard');
+        $request = Request::create('/?_target_path=/dashboard');
+        $response = $this->expectRedirectResponse($request, '/dashboard');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
@@ -73,15 +68,11 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
     public function testTargetPathParameterIsCustomised()
     {
         $options = array('target_path_parameter' => '_my_target_path');
-
-        $this->request->expects($this->once())
-            ->method('get')->with('_my_target_path')
-            ->will($this->returnValue('/dashboard'));
-
-        $response = $this->expectRedirectResponse('/dashboard');
+        $request = Request::create('/?_my_target_path=/dashboard');
+        $response = $this->expectRedirectResponse($request, '/dashboard');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
@@ -95,16 +86,14 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
         $session->expects($this->once())
             ->method('remove')->with('_security.admin.target_path');
 
-        $this->request->expects($this->any())
-            ->method('getSession')
-            ->will($this->returnValue($session));
-
-        $response = $this->expectRedirectResponse('/admin/dashboard');
+        $request = Request::create('/?_my_target_path=/dashboard');
+        $request->setSession($session);
+        $response = $this->expectRedirectResponse($request, '/admin/dashboard');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
         $handler->setProviderKey('admin');
 
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
@@ -112,15 +101,12 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
     public function testTargetPathIsPassedAsReferer()
     {
         $options = array('use_referer' => true);
-
-        $this->request->headers->expects($this->once())
-            ->method('get')->with('Referer')
-            ->will($this->returnValue('/dashboard'));
-
-        $response = $this->expectRedirectResponse('/dashboard');
+        $request = Request::create('/');
+        $request->headers->set('Referer', '/dashboard');
+        $response = $this->expectRedirectResponse($request, '/dashboard');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
@@ -128,41 +114,37 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
     public function testRefererHasToBeDifferentThatLoginUrl()
     {
         $options = array('use_referer' => true);
-
-        $this->request->headers->expects($this->any())
-            ->method('get')->with('Referer')
-            ->will($this->returnValue('/login'));
-
+        $request = Request::create('/');
+        $request->headers->set('Referer', '/login');
         $this->httpUtils->expects($this->once())
-            ->method('generateUri')->with($this->request, '/login')
+            ->method('generateUri')->with($request, '/login')
             ->will($this->returnValue('/login'));
 
-        $response = $this->expectRedirectResponse('/');
+        $response = $this->expectRedirectResponse($request, '/');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
 
     public function testRefererTargetPathIsIgnoredByDefault()
     {
-        $this->request->headers->expects($this->never())->method('get');
-
-        $response = $this->expectRedirectResponse('/');
+        $request = Request::create('/');
+        $response = $this->expectRedirectResponse($request, '/');
 
         $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($this->request, $this->token);
+        $result = $handler->onAuthenticationSuccess($request, $this->token);
 
         $this->assertSame($response, $result);
     }
 
-    private function expectRedirectResponse($path)
+    private function expectRedirectResponse(Request $request, $path)
     {
         $response = new Response();
         $this->httpUtils->expects($this->once())
             ->method('createRedirectResponse')
-            ->with($this->request, $path)
+            ->with($request, $path)
             ->will($this->returnValue($response));
 
         return $response;

From b1f1ae26b47e6aaf5282584d880724fed66a86df Mon Sep 17 00:00:00 2001
From: Fabien Potencier <fabien.potencier@gmail.com>
Date: Wed, 19 Jul 2017 07:42:44 +0200
Subject: [PATCH 2/4] [Security] simplified tests

---
 ...efaultAuthenticationSuccessHandlerTest.php | 193 +++++++-----------
 1 file changed, 69 insertions(+), 124 deletions(-)

diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
index 70836170c7..8d39e63cd5 100644
--- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
+++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
@@ -15,138 +15,83 @@ use PHPUnit\Framework\TestCase;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler;
+use Symfony\Component\Security\Http\HttpUtils;
 
 class DefaultAuthenticationSuccessHandlerTest extends TestCase
 {
-    private $httpUtils = null;
-    private $token = null;
-
-    protected function setUp()
+    /**
+     * @dataProvider getRequestRedirections
+     */
+    public function testRequestRedirections(Request $request, $options, $redirectedUrl)
     {
-        $this->httpUtils = $this->getMockBuilder('Symfony\Component\Security\Http\HttpUtils')->getMock();
-        $this->token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
+        $httpUtils = new HttpUtils();
+        $token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
+        $handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options);
+        if ($request->hasSession()) {
+            $handler->setProviderKey('admin');
+        }
+        $this->assertSame('http://localhost'.$redirectedUrl, $handler->onAuthenticationSuccess($request, $token)->getTargetUrl());
     }
 
-    public function testRequestIsRedirected()
-    {
-        $request = Request::create('/');
-        $response = $this->expectRedirectResponse($request, '/');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testDefaultTargetPathCanBeForced()
-    {
-        $options = array(
-            'always_use_default_target_path' => true,
-            'default_target_path' => '/dashboard',
-        );
-
-        $request = Request::create('/');
-        $response = $this->expectRedirectResponse($request, '/dashboard');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testTargetPathIsPassedWithRequest()
-    {
-        $request = Request::create('/?_target_path=/dashboard');
-        $response = $this->expectRedirectResponse($request, '/dashboard');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testTargetPathParameterIsCustomised()
-    {
-        $options = array('target_path_parameter' => '_my_target_path');
-        $request = Request::create('/?_my_target_path=/dashboard');
-        $response = $this->expectRedirectResponse($request, '/dashboard');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testTargetPathIsTakenFromTheSession()
+    public function getRequestRedirections()
     {
         $session = $this->getMockBuilder('Symfony\Component\HttpFoundation\Session\SessionInterface')->getMock();
-        $session->expects($this->once())
-            ->method('get')->with('_security.admin.target_path')
-            ->will($this->returnValue('/admin/dashboard'));
-        $session->expects($this->once())
-            ->method('remove')->with('_security.admin.target_path');
+        $session->expects($this->once())->method('get')->with('_security.admin.target_path')->will($this->returnValue('/admin/dashboard'));
+        $session->expects($this->once())->method('remove')->with('_security.admin.target_path');
+        $requestWithSession = Request::create('/');
+        $requestWithSession->setSession($session);
 
-        $request = Request::create('/?_my_target_path=/dashboard');
-        $request->setSession($session);
-        $response = $this->expectRedirectResponse($request, '/admin/dashboard');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $handler->setProviderKey('admin');
-
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testTargetPathIsPassedAsReferer()
-    {
-        $options = array('use_referer' => true);
-        $request = Request::create('/');
-        $request->headers->set('Referer', '/dashboard');
-        $response = $this->expectRedirectResponse($request, '/dashboard');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testRefererHasToBeDifferentThatLoginUrl()
-    {
-        $options = array('use_referer' => true);
-        $request = Request::create('/');
-        $request->headers->set('Referer', '/login');
-        $this->httpUtils->expects($this->once())
-            ->method('generateUri')->with($request, '/login')
-            ->will($this->returnValue('/login'));
-
-        $response = $this->expectRedirectResponse($request, '/');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, $options);
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    public function testRefererTargetPathIsIgnoredByDefault()
-    {
-        $request = Request::create('/');
-        $response = $this->expectRedirectResponse($request, '/');
-
-        $handler = new DefaultAuthenticationSuccessHandler($this->httpUtils, array());
-        $result = $handler->onAuthenticationSuccess($request, $this->token);
-
-        $this->assertSame($response, $result);
-    }
-
-    private function expectRedirectResponse(Request $request, $path)
-    {
-        $response = new Response();
-        $this->httpUtils->expects($this->once())
-            ->method('createRedirectResponse')
-            ->with($request, $path)
-            ->will($this->returnValue($response));
-
-        return $response;
+        return array(
+            'default' => array(
+                Request::create('/'),
+                array(),
+                '/',
+            ),
+            'forced target path' => array(
+                Request::create('/'),
+                array('always_use_default_target_path' => true, 'default_target_path' => '/dashboard'),
+                '/dashboard',
+            ),
+            'target path as query string' => array(
+                Request::create('/?_target_path=/dashboard'),
+                array(),
+                '/dashboard',
+            ),
+            'target path name as query string is customized' => array(
+                Request::create('/?_my_target_path=/dashboard'),
+                array('target_path_parameter' => '_my_target_path'),
+                '/dashboard',
+            ),
+            'target path name as query string is customized and nested' => array(
+                Request::create('/?_target_path[value]=/dashboard'),
+                array('target_path_parameter' => '_target_path[value]'),
+                '/dashboard',
+            ),
+            'target path in session' => array(
+                $requestWithSession,
+                array(),
+                '/admin/dashboard',
+            ),
+            'target path as referer' => array(
+                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/dashboard')),
+                array('use_referer' => true),
+                '/dashboard',
+            ),
+            'target path as referer is ignored if not configured' => array(
+                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/dashboard')),
+                array(),
+                '/',
+            ),
+            'target path should be different than login URL' => array(
+                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/login')),
+                array('use_referer' => true, 'login_path' => '/login'),
+                '/',
+            ),
+            'target path should be different than login URL (query string does not matter)' => array(
+                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
+                array('use_referer' => true, 'login_path' => '/login'),
+                '/',
+            ),
+        );
     }
 }

From 9c7a1406cb81f4d1ccd7e3521347e2d847b7467c Mon Sep 17 00:00:00 2001
From: Fabien Potencier <fabien.potencier@gmail.com>
Date: Wed, 19 Jul 2017 07:57:00 +0200
Subject: [PATCH 3/4] [Security] fixed default target path when referer
 contains a query string

---
 .../DefaultAuthenticationSuccessHandler.php            | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php
index b6a7df5062..7da6e35572 100644
--- a/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php
+++ b/src/Symfony/Component/Security/Http/Authentication/DefaultAuthenticationSuccessHandler.php
@@ -118,8 +118,14 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle
             return $targetUrl;
         }
 
-        if ($this->options['use_referer'] && ($targetUrl = $request->headers->get('Referer')) && $targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) {
-            return $targetUrl;
+        if ($this->options['use_referer']) {
+            $targetUrl = $request->headers->get('Referer');
+            if (false !== $pos = strpos($targetUrl, '?')) {
+                $targetUrl = substr($targetUrl, 0, $pos);
+            }
+            if ($targetUrl !== $this->httpUtils->generateUri($request, $this->options['login_path'])) {
+                return $targetUrl;
+            }
         }
 
         return $this->options['default_target_path'];

From 022ac0be096a303aa4de7be64b84be9ffee3ecd9 Mon Sep 17 00:00:00 2001
From: Fabien Potencier <fabien.potencier@gmail.com>
Date: Wed, 19 Jul 2017 08:04:16 +0200
Subject: [PATCH 4/4] [Security] added more tests

---
 ...DefaultAuthenticationSuccessHandlerTest.php | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
index 8d39e63cd5..b42f840358 100644
--- a/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
+++ b/src/Symfony/Component/Security/Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
@@ -13,7 +13,6 @@ namespace Symfony\Component\Security\Http\Tests\Authentication;
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\HttpFoundation\Request;
-use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler;
 use Symfony\Component\Security\Http\HttpUtils;
 
@@ -24,7 +23,9 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
      */
     public function testRequestRedirections(Request $request, $options, $redirectedUrl)
     {
-        $httpUtils = new HttpUtils();
+        $urlGenerator = $this->getMockBuilder('Symfony\Component\Routing\Generator\UrlGeneratorInterface')->getMock();
+        $urlGenerator->expects($this->any())->method('generate')->will($this->returnValue('http://localhost/login'));
+        $httpUtils = new HttpUtils($urlGenerator);
         $token = $this->getMockBuilder('Symfony\Component\Security\Core\Authentication\Token\TokenInterface')->getMock();
         $handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options);
         if ($request->hasSession()) {
@@ -73,25 +74,30 @@ class DefaultAuthenticationSuccessHandlerTest extends TestCase
                 '/admin/dashboard',
             ),
             'target path as referer' => array(
-                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/dashboard')),
+                Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')),
                 array('use_referer' => true),
                 '/dashboard',
             ),
             'target path as referer is ignored if not configured' => array(
-                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/dashboard')),
+                Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/dashboard')),
                 array(),
                 '/',
             ),
             'target path should be different than login URL' => array(
-                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/login')),
+                Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login')),
                 array('use_referer' => true, 'login_path' => '/login'),
                 '/',
             ),
             'target path should be different than login URL (query string does not matter)' => array(
-                Request::create('/', 'GET', array(),  array(),  array(),  array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
+                Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
                 array('use_referer' => true, 'login_path' => '/login'),
                 '/',
             ),
+            'target path should be different than login URL (login_path as a route)' => array(
+                Request::create('/', 'GET', array(), array(), array(), array('HTTP_REFERER' => 'http://localhost/login?t=1&p=2')),
+                array('use_referer' => true, 'login_path' => 'login_route'),
+                '/',
+            ),
         );
     }
 }