bug #46317 [Security/Http] Ignore invalid URLs found in failure/success paths (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[Security/Http] Ignore invalid URLs found in failure/success paths

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #43567
| License       | MIT
| Doc PR        | -

Commits
-------

389df989b9 [Security/Http] Ignore invalid URLs found in failure/success paths

Author: fabpot

Http/Authentication/DefaultAuthenticationFailureHandler.php

@@ -70,31 +70,34 @@ public function setOptions(array $options)
      */
     public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
     {
-        if ($failureUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['failure_path_parameter'])) {
-            $this->options['failure_path'] = $failureUrl;
-        }
+        $options = $this->options;
+        $failureUrl = ParameterBagUtils::getRequestParameterValue($request, $options['failure_path_parameter']);
 
-        if (null === $this->options['failure_path']) {
-            $this->options['failure_path'] = $this->options['login_path'];
+        if (\is_string($failureUrl) && str_starts_with($failureUrl, '/')) {
+            $options['failure_path'] = $failureUrl;
+        } elseif ($this->logger && $failureUrl) {
+            $this->logger->debug(sprintf('Ignoring query parameter "%s": not a valid URL.', $options['failure_path_parameter']));
         }
 
-        if ($this->options['failure_forward']) {
+        $options['failure_path'] ?? $options['failure_path'] = $options['login_path'];
+
+        if ($options['failure_forward']) {
             if (null !== $this->logger) {
-                $this->logger->debug('Authentication failure, forward triggered.', ['failure_path' => $this->options['failure_path']]);
+                $this->logger->debug('Authentication failure, forward triggered.', ['failure_path' => $options['failure_path']]);
             }
 
-            $subRequest = $this->httpUtils->createRequest($request, $this->options['failure_path']);
+            $subRequest = $this->httpUtils->createRequest($request, $options['failure_path']);
             $subRequest->attributes->set(Security::AUTHENTICATION_ERROR, $exception);
 
             return $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
         }
 
         if (null !== $this->logger) {
-            $this->logger->debug('Authentication failure, redirect triggered.', ['failure_path' => $this->options['failure_path']]);
+            $this->logger->debug('Authentication failure, redirect triggered.', ['failure_path' => $options['failure_path']]);
         }
 
         $request->getSession()->set(Security::AUTHENTICATION_ERROR, $exception);
 
-        return $this->httpUtils->createRedirectResponse($request, $this->options['failure_path']);
+        return $this->httpUtils->createRedirectResponse($request, $options['failure_path']);
     }
 }

Http/Authentication/DefaultAuthenticationSuccessHandler.php

@@ -11,6 +11,7 @@
 
 namespace Symfony\Component\Security\Http\Authentication;
 
+use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Http\HttpUtils;
@@ -29,6 +30,7 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle
     use TargetPathTrait;
 
     protected $httpUtils;
+    protected $logger;
     protected $options;
     protected $providerKey;
     protected $defaultOptions = [
@@ -42,9 +44,10 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle
     /**
      * @param array $options Options for processing a successful authentication attempt
      */
-    public function __construct(HttpUtils $httpUtils, array $options = [])
+    public function __construct(HttpUtils $httpUtils, array $options = [], LoggerInterface $logger = null)
     {
         $this->httpUtils = $httpUtils;
+        $this->logger = $logger;
         $this->setOptions($options);
     }
 
@@ -102,10 +105,16 @@ protected function determineTargetUrl(Request $request)
             return $this->options['default_target_path'];
         }
 
-        if ($targetUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['target_path_parameter'])) {
+        $targetUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['target_path_parameter']);
+
+        if (\is_string($targetUrl) && str_starts_with($targetUrl, '/')) {
             return $targetUrl;
         }
 
+        if ($this->logger && $targetUrl) {
+            $this->logger->debug(sprintf('Ignoring query parameter "%s": not a valid URL.', $this->options['target_path_parameter']));
+        }
+
         if (null !== $this->providerKey && $targetUrl = $this->getTargetPath($request->getSession(), $this->providerKey)) {
             $this->removeTargetPath($request->getSession(), $this->providerKey);
 

Http/Tests/Authentication/DefaultAuthenticationFailureHandlerTest.php

@@ -187,6 +187,26 @@ public function testFailurePathParameterCanBeOverwritten()
         $handler->onAuthenticationFailure($this->request, $this->exception);
     }
 
+    public function testFailurePathFromRequestWithInvalidUrl()
+    {
+        $options = ['failure_path_parameter' => '_my_failure_path'];
+
+        $this->request->expects($this->once())
+            ->method('get')->with('_my_failure_path')
+            ->willReturn('some_route_name');
+
+        $this->logger->expects($this->exactly(2))
+            ->method('debug')
+            ->withConsecutive(
+                ['Ignoring query parameter "_my_failure_path": not a valid URL.'],
+                ['Authentication failure, redirect triggered.', ['failure_path' => '/login']]
+            );
+
+        $handler = new DefaultAuthenticationFailureHandler($this->httpKernel, $this->httpUtils, $options, $this->logger);
+
+        $handler->onAuthenticationFailure($this->request, $this->exception);
+    }
+
     private function getRequest()
     {
         $request = $this->createMock(Request::class);

Http/Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php

@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Http\Tests\Authentication;
 
 use PHPUnit\Framework\TestCase;
+use Psr\Log\LoggerInterface;
 use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Session\SessionInterface;
 use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
@@ -113,4 +114,25 @@ public function getRequestRedirections()
             ],
         ];
     }
+
+    public function testTargetPathFromRequestWithInvalidUrl()
+    {
+        $httpUtils = $this->createMock(HttpUtils::class);
+        $options = ['target_path_parameter' => '_my_target_path'];
+        $token = $this->createMock(TokenInterface::class);
+
+        $request = $this->createMock(Request::class);
+        $request->expects($this->once())
+            ->method('get')->with('_my_target_path')
+            ->willReturn('some_route_name');
+
+        $logger = $this->createMock(LoggerInterface::class);
+        $logger->expects($this->once())
+            ->method('debug')
+            ->with('Ignoring query parameter "_my_target_path": not a valid URL.');
+
+        $handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options, $logger);
+
+        $handler->onAuthenticationSuccess($request, $token);
+    }
 }