Redirect to FE preview page after login and ensure integrity of target links

core-bundle/src/Controller/BackendController.php

@@ -56,13 +56,16 @@ public function loginAction(Request $request): Response
         $this->initializeContaoFramework();
 
         if ($this->isGranted('IS_AUTHENTICATED_FULLY')) {
-            $queryString = '';
+            if ($request->query->has('redirect')) {
+                $uriSigner = $this->get('uri_signer');
 
-            if ($request->query->has('referer')) {
-                $queryString = '?'.base64_decode($request->query->get('referer'), true);
+                // We cannot use $request->getUri() here as we want to work with the original URI (no query string reordering)
+                if ($uriSigner->check($request->getSchemeAndHttpHost().$request->getBaseUrl().$request->getPathInfo().(null !== ($qs = $request->server->get('QUERY_STRING')) ? '?'.$qs : ''))) {
+                    return new RedirectResponse($request->query->get('redirect'));
+                }
             }
 
-            return new RedirectResponse($this->generateUrl('contao_backend').$queryString);
+            return new RedirectResponse($this->generateUrl('contao_backend'));
         }
 
         $controller = new BackendIndex();
@@ -214,6 +217,7 @@ public static function getSubscribedServices(): array
         $services = parent::getSubscribedServices();
 
         $services['contao.picker.builder'] = PickerBuilderInterface::class;
+        $services['uri_signer'] = 'uri_signer'; // FIXME: adjust this once https://github.com/symfony/symfony/pull/35298 has been merged
 
         return $services;
     }

core-bundle/src/Controller/Base64EncodedRedirectController.php

@@ -0,0 +1,53 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CoreBundle\Controller;
+
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
+use Symfony\Component\HttpKernel\UriSigner;
+use Symfony\Component\Routing\Annotation\Route;
+
+/**
+ * @internal
+ */
+class Base64EncodedRedirectController
+{
+    /**
+     * @var UriSigner
+     */
+    private $uriSigner;
+
+    public function __construct(UriSigner $uriSigner)
+    {
+        $this->uriSigner = $uriSigner;
+    }
+
+    /**
+     * @Route("/_contao/base64_redirect", name="contao_base64_redirect")
+     */
+    public function renderAction(Request $request): Response
+    {
+        if (!$request->query->has('redirect')) {
+            throw new BadRequestHttpException();
+        }
+
+        // We cannot use $request->getUri() here as we want to work with the original URI (no query string reordering)
+        if (!$this->uriSigner->check($request->getSchemeAndHttpHost().$request->getBaseUrl().$request->getPathInfo().(null !== ($qs = $request->server->get('QUERY_STRING')) ? '?'.$qs : ''))) {
+            throw new BadRequestHttpException();
+        }
+
+        return new RedirectResponse(base64_decode($request->query->get('redirect'), true));
+    }
+}

core-bundle/src/DependencyInjection/Compiler/MakeServicesPublicPass.php

@@ -30,6 +30,7 @@ class MakeServicesPublicPass implements CompilerPassInterface
         'security.authentication.trust_resolver',
         'security.firewall.map',
         'security.logout_url_generator',
+        'uri_signer',
     ];
 
     private const ALIASES = [

core-bundle/src/Resources/config/services.yml

@@ -92,6 +92,12 @@ services:
         tags:
             - controller.service_arguments
 
+    Contao\CoreBundle\Controller\Base64EncodedRedirectController:
+        arguments:
+            - '@uri_signer'
+        tags:
+            - controller.service_arguments
+
     # Backwards compatibility
     contao.controller.backend_csv_import:
         alias: Contao\CoreBundle\Controller\BackendCsvImportController
@@ -607,6 +613,7 @@ services:
         arguments:
             - '@security.http_utils'
             - '@router'
+            - '@uri_signer'
 
     contao.security.expiring_token_based_remember_me_services:
         class: Contao\CoreBundle\Security\Authentication\RememberMe\ExpiringTokenBasedRememberMeServices

core-bundle/src/Resources/contao/controllers/BackendIndex.php

@@ -14,6 +14,7 @@
 use Scheb\TwoFactorBundle\Security\Authentication\Exception\InvalidTwoFactorCodeException;
 use Scheb\TwoFactorBundle\Security\Authentication\Token\TwoFactorToken;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\UriSigner;
 use Symfony\Component\Routing\Router;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
@@ -66,18 +67,25 @@ public function run()
 			Message::addError($GLOBALS['TL_LANG']['ERR']['invalidLogin']);
 		}
 
-		$queryString = '';
-		$arrParams = array();
+		$router = $container->get('router');
+		$targetPath = $router->generate('contao_backend', array(), Router::ABSOLUTE_URL);
+		$failurePath = $router->generate('contao_backend_login', array(), Router::ABSOLUTE_URL);
+		$request = $container->get('request_stack')->getCurrentRequest();
 
-		if ($referer = Input::get('referer', true))
+		if ($request && $request->query->has('redirect'))
 		{
-			// Decode the referer and urlencode insert tags
-			$queryString = '?' . str_replace(array('{', '}'), array('%7B', '%7D'), base64_decode($referer, true));
-			$arrParams['referer'] = $referer;
+			/** @var UriSigner $uriSigner */
+			$uriSigner = $container->get('uri_signer');
+
+			// We cannot use $request->getUri() here as we want to work with the original URI (no query string reordering)
+			if ($uriSigner->check($request->getSchemeAndHttpHost() . $request->getBaseUrl() . $request->getPathInfo() . (null !== ($qs = $request->server->get('QUERY_STRING')) ? '?' . $qs : '')))
+			{
+				$redirectParams = array('redirect' => base64_encode($request->query->get('redirect')));
+				$targetPath = $uriSigner->sign($router->generate('contao_base64_redirect', $redirectParams, Router::ABSOLUTE_URL));
+				$failurePath = $uriSigner->sign($router->generate('contao_base64_redirect', array('redirect' => $router->generate('contao_backend_login', $redirectParams, Router::ABSOLUTE_URL)), Router::ABSOLUTE_URL));
+			}
 		}
 
-		$router = $container->get('router');
-
 		$objTemplate = new BackendTemplate('be_login');
 		$objTemplate->action = ampersand(Environment::get('request'));
 		$objTemplate->headline = $GLOBALS['TL_LANG']['MSC']['loginBT'];
@@ -110,8 +118,8 @@ public function run()
 		$objTemplate->feLink = $GLOBALS['TL_LANG']['MSC']['feLink'];
 		$objTemplate->default = $GLOBALS['TL_LANG']['MSC']['default'];
 		$objTemplate->jsDisabled = $GLOBALS['TL_LANG']['MSC']['jsDisabled'];
-		$objTemplate->targetPath = StringUtil::specialchars($router->generate('contao_backend', array(), Router::ABSOLUTE_URL) . $queryString);
-		$objTemplate->failurePath = StringUtil::specialchars($router->generate('contao_backend_login', $arrParams, Router::ABSOLUTE_URL));
+		$objTemplate->targetPath = StringUtil::specialchars($targetPath);
+		$objTemplate->failurePath = StringUtil::specialchars($failurePath);
 
 		return $objTemplate->getResponse();
 	}

core-bundle/src/Resources/contao/modules/ModuleLogin.php

@@ -13,6 +13,7 @@
 use Contao\CoreBundle\Security\Exception\LockedException;
 use Patchwork\Utf8;
 use Scheb\TwoFactorBundle\Security\Authentication\Exception\InvalidTwoFactorCodeException;
+use Symfony\Component\HttpKernel\UriSigner;
 use Symfony\Component\Routing\Router;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
 
@@ -59,14 +60,25 @@ public function generate()
 			return $objTemplate->parse();
 		}
 
+		$container = System::getContainer();
+		$request = $container->get('request_stack')->getCurrentRequest();
+
+		// If the form was submitted and the credentials were wrong, take the target path from the submitted data
+		// as otherwise it would take the current page
 		if ($_POST)
 		{
 			$this->targetPath = (string) Input::post('_target_path');
 		}
-		elseif ($this->redirectBack && ($referer = Input::get('referer', true)))
+		elseif ($this->redirectBack && $request && $request->query->has('redirect'))
 		{
-			// Decode the referer and urlencode insert tags
-			$this->targetPath = Environment::get('base') . str_replace(array('{', '}'), array('%7B', '%7D'), base64_decode($referer, true));
+			/** @var UriSigner $uriSigner */
+			$uriSigner = $container->get('uri_signer');
+
+			// We cannot use $request->getUri() here as we want to work with the original URI (no query string reordering)
+			if ($uriSigner->check($request->getSchemeAndHttpHost() . $request->getBaseUrl() . $request->getPathInfo() . (null !== ($qs = $request->server->get('QUERY_STRING')) ? '?' . $qs : '')))
+			{
+				$this->targetPath = base64_decode($request->query->get('redirect'));
+			}
 		}
 
 		return parent::generate();
@@ -85,6 +97,9 @@ protected function compile()
 		/** @var Router $router */
 		$router = $container->get('router');
 
+		/** @var UriSigner $uriSigner */
+		$uriSigner = $container->get('uri_signer');
+
 		/** @var AuthenticationException|null $exception */
 		$exception = $container->get('security.authentication_utils')->getLastAuthenticationError();
 		$authorizationChecker = $container->get('security.authorization_checker');
@@ -172,6 +187,12 @@ protected function compile()
 			$strRedirect = $objTarget->getAbsoluteUrl();
 		}
 
+		$request = $container->get('request_stack')->getCurrentRequest();
+
+		// Ensure we do not output any possible dangerous data (redirect back to previous URL allows for URL param injection)
+		$targetPath = $uriSigner->sign($router->generate('contao_base64_redirect', array('redirect' => base64_encode($strRedirect)), Router::ABSOLUTE_URL));
+		$failurePath = $uriSigner->sign($router->generate('contao_base64_redirect', array('redirect' => base64_encode($request->getUri())), Router::ABSOLUTE_URL));
+
 		$this->Template->username = $GLOBALS['TL_LANG']['MSC']['username'];
 		$this->Template->password = $GLOBALS['TL_LANG']['MSC']['password'][0];
 		$this->Template->action = $router->generate('contao_frontend_login');
@@ -181,8 +202,8 @@ protected function compile()
 		$this->Template->autologin = $this->autologin;
 		$this->Template->autoLabel = $GLOBALS['TL_LANG']['MSC']['autologin'];
 		$this->Template->forceTargetPath = (int) $blnRedirectBack;
-		$this->Template->targetPath = StringUtil::specialchars($strRedirect);
-		$this->Template->failurePath = StringUtil::specialchars(Environment::get('base') . Environment::get('request'));
+		$this->Template->targetPath = StringUtil::specialchars($targetPath);
+		$this->Template->failurePath = StringUtil::specialchars($failurePath);
 	}
 }
 

core-bundle/src/Resources/contao/pages/PageError401.php

@@ -13,6 +13,7 @@
 use Contao\CoreBundle\Exception\ForwardPageNotFoundException;
 use Contao\CoreBundle\Exception\InsufficientAuthenticationException;
 use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpKernel\UriSigner;
 
 /**
  * Provide methods to handle an error 401 page.
@@ -121,9 +122,12 @@ protected function prepare($objRootPage=null)
 			}
 
 			// Add the referer so the login module can redirect back
-			$referer = base64_encode(Environment::get('request'));
+			$url = $objNextPage->getAbsoluteUrl() . '?redirect=' . base64_encode(Environment::get('base') . Environment::get('request'));
 
-			$this->redirect($objNextPage->getFrontendUrl() . '?referer=' . $referer, (($obj401->redirect == 'temporary') ? 302 : 301));
+			/** @var UriSigner $uriSigner */
+			$uriSigner = System::getContainer()->get('uri_signer');
+
+			$this->redirect($uriSigner->sign($url), (($obj401->redirect == 'temporary') ? 302 : 301));
 		}
 
 		return $obj401;

core-bundle/src/Security/Authentication/AuthenticationEntryPoint.php

@@ -13,6 +13,7 @@
 namespace Contao\CoreBundle\Security\Authentication;
 
 use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\UriSigner;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
 use Symfony\Component\Routing\RouterInterface;
 use Symfony\Component\Security\Core\Exception\AuthenticationException;
@@ -31,13 +32,19 @@ class AuthenticationEntryPoint implements AuthenticationEntryPointInterface
      */
     private $router;
 
+    /**
+     * @var UriSigner
+     */
+    private $uriSigner;
+
     /**
      * @internal Do not inherit from this class; decorate the "contao.security.entry_point" service instead
      */
-    public function __construct(HttpUtils $httpUtils, RouterInterface $router)
+    public function __construct(HttpUtils $httpUtils, RouterInterface $router, UriSigner $uriSigner)
     {
         $this->httpUtils = $httpUtils;
         $this->router = $router;
+        $this->uriSigner = $uriSigner;
     }
 
     /**
@@ -51,10 +58,10 @@ public function start(Request $request, AuthenticationException $authException =
 
         $url = $this->router->generate(
             'contao_backend_login',
-            ['referer' => base64_encode($request->getQueryString())],
+            ['redirect' => $request->getUri()],
             UrlGeneratorInterface::ABSOLUTE_URL
         );
 
-        return $this->httpUtils->createRedirectResponse($request, $url);
+        return $this->httpUtils->createRedirectResponse($request, $this->uriSigner->sign($url));
     }
 }

core-bundle/tests/Controller/Base64EncodedRedirectControllerTest.php

@@ -0,0 +1,60 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Contao.
+ *
+ * (c) Leo Feyer
+ *
+ * @license LGPL-3.0-or-later
+ */
+
+namespace Contao\CoreBundle\Tests\Controller;
+
+use Contao\CoreBundle\Controller\Base64EncodedRedirectController;
+use Contao\CoreBundle\Tests\TestCase;
+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpKernel\Exception\BadRequestHttpException;
+use Symfony\Component\HttpKernel\UriSigner;
+
+class Base64EncodedRedirectControllerTest extends TestCase
+{
+    public function testReturnsBadRequestIfNoRedirectQueryParameterWasProvided(): void
+    {
+        $request = Request::create('https://contao.org/_contao/base64_redirect?foobar=nonsense');
+        $controller = new Base64EncodedRedirectController(new UriSigner('secret'));
+
+        $this->expectException(BadRequestHttpException::class);
+
+        $controller->renderAction($request);
+    }
+
+    public function testReturnsBadRequestIfUrlSigningWasIncorrect(): void
+    {
+        $redirect = base64_encode('https://contao.org/preview.php/about-contao.html');
+        $request = Request::create('https://contao.org/_contao/base64_redirect?_hash=nonsense&redirect='.$redirect);
+        $controller = new Base64EncodedRedirectController(new UriSigner('secret'));
+
+        $this->expectException(BadRequestHttpException::class);
+
+        $controller->renderAction($request);
+    }
+
+    public function testRedirectsToCorrectUrlSigningMatches(): void
+    {
+        $redirectUrl = 'https://contao.org/preview.php/about-contao.html';
+
+        $uriSigner = new UriSigner('secret');
+        $signedUri = $uriSigner->sign('https://contao.org/_contao/base64_redirect?redirect='.base64_encode($redirectUrl));
+
+        $controller = new Base64EncodedRedirectController($uriSigner);
+
+        /** @var RedirectResponse $response */
+        $response = $controller->renderAction(Request::create($signedUri));
+
+        $this->assertInstanceOf(RedirectResponse::class, $response);
+        $this->assertSame($redirectUrl, $response->getTargetUrl());
+    }
+}