Skip to content

Commit 0af230c

Browse files
fabpotfabpot
committed
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
2 parents f5b6fa2 + 7877d48 commit 0af230c

File tree

4 files changed

+66
-12
lines changed

4 files changed

+66
-12
lines changed

‎Authentication/DefaultAuthenticationFailureHandler.php‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -70,31 +70,34 @@ public function setOptions(array $options)
7070
*/
7171
public function onAuthenticationFailure(Request $request, AuthenticationException $exception)
7272
{
73-
if ($failureUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['failure_path_parameter'])) {
74-
$this->options['failure_path'] = $failureUrl;
75-
}
73+
$options = $this->options;
74+
$failureUrl = ParameterBagUtils::getRequestParameterValue($request, $options['failure_path_parameter']);
7675

77-
if (null === $this->options['failure_path']) {
78-
$this->options['failure_path'] = $this->options['login_path'];
76+
if (\is_string($failureUrl) && str_starts_with($failureUrl, '/')) {
77+
$options['failure_path'] = $failureUrl;
78+
} elseif ($this->logger && $failureUrl) {
79+
$this->logger->debug(sprintf('Ignoring query parameter "%s": not a valid URL.', $options['failure_path_parameter']));
7980
}
8081

81-
if ($this->options['failure_forward']) {
82+
$options['failure_path'] ?? $options['failure_path'] = $options['login_path'];
83+
84+
if ($options['failure_forward']) {
8285
if (null !== $this->logger) {
83-
$this->logger->debug('Authentication failure, forward triggered.', ['failure_path' => $this->options['failure_path']]);
86+
$this->logger->debug('Authentication failure, forward triggered.', ['failure_path' => $options['failure_path']]);
8487
}
8588

86-
$subRequest = $this->httpUtils->createRequest($request, $this->options['failure_path']);
89+
$subRequest = $this->httpUtils->createRequest($request, $options['failure_path']);
8790
$subRequest->attributes->set(Security::AUTHENTICATION_ERROR, $exception);
8891

8992
return $this->httpKernel->handle($subRequest, HttpKernelInterface::SUB_REQUEST);
9093
}
9194

9295
if (null !== $this->logger) {
93-
$this->logger->debug('Authentication failure, redirect triggered.', ['failure_path' => $this->options['failure_path']]);
96+
$this->logger->debug('Authentication failure, redirect triggered.', ['failure_path' => $options['failure_path']]);
9497
}
9598

9699
$request->getSession()->set(Security::AUTHENTICATION_ERROR, $exception);
97100

98-
return $this->httpUtils->createRedirectResponse($request, $this->options['failure_path']);
101+
return $this->httpUtils->createRedirectResponse($request, $options['failure_path']);
99102
}
100103
}

‎Authentication/DefaultAuthenticationSuccessHandler.php‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
namespace Symfony\Component\Security\Http\Authentication;
1313

14+
use Psr\Log\LoggerInterface;
1415
use Symfony\Component\HttpFoundation\Request;
1516
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
1617
use Symfony\Component\Security\Http\HttpUtils;
@@ -29,6 +30,7 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle
2930
use TargetPathTrait;
3031

3132
protected $httpUtils;
33+
protected $logger;
3234
protected $options;
3335
protected $providerKey;
3436
protected $defaultOptions = [
@@ -42,9 +44,10 @@ class DefaultAuthenticationSuccessHandler implements AuthenticationSuccessHandle
4244
/**
4345
* @param array $options Options for processing a successful authentication attempt
4446
*/
45-
public function __construct(HttpUtils $httpUtils, array $options = [])
47+
public function __construct(HttpUtils $httpUtils, array $options = [], LoggerInterface $logger = null)
4648
{
4749
$this->httpUtils = $httpUtils;
50+
$this->logger = $logger;
4851
$this->setOptions($options);
4952
}
5053

@@ -102,10 +105,16 @@ protected function determineTargetUrl(Request $request)
102105
return $this->options['default_target_path'];
103106
}
104107

105-
if ($targetUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['target_path_parameter'])) {
108+
$targetUrl = ParameterBagUtils::getRequestParameterValue($request, $this->options['target_path_parameter']);
109+
110+
if (\is_string($targetUrl) && str_starts_with($targetUrl, '/')) {
106111
return $targetUrl;
107112
}
108113

114+
if ($this->logger && $targetUrl) {
115+
$this->logger->debug(sprintf('Ignoring query parameter "%s": not a valid URL.', $this->options['target_path_parameter']));
116+
}
117+
109118
if (null !== $this->providerKey && $targetUrl = $this->getTargetPath($request->getSession(), $this->providerKey)) {
110119
$this->removeTargetPath($request->getSession(), $this->providerKey);
111120

‎Tests/Authentication/DefaultAuthenticationFailureHandlerTest.php‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,26 @@ public function testFailurePathParameterCanBeOverwritten()
187187
$handler->onAuthenticationFailure($this->request, $this->exception);
188188
}
189189

190+
public function testFailurePathFromRequestWithInvalidUrl()
191+
{
192+
$options = ['failure_path_parameter' => '_my_failure_path'];
193+
194+
$this->request->expects($this->once())
195+
->method('get')->with('_my_failure_path')
196+
->willReturn('some_route_name');
197+
198+
$this->logger->expects($this->exactly(2))
199+
->method('debug')
200+
->withConsecutive(
201+
['Ignoring query parameter "_my_failure_path": not a valid URL.'],
202+
['Authentication failure, redirect triggered.', ['failure_path' => '/login']]
203+
);
204+
205+
$handler = new DefaultAuthenticationFailureHandler($this->httpKernel, $this->httpUtils, $options, $this->logger);
206+
207+
$handler->onAuthenticationFailure($this->request, $this->exception);
208+
}
209+
190210
private function getRequest()
191211
{
192212
$request = $this->createMock(Request::class);

‎Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\Security\Http\Tests\Authentication;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Psr\Log\LoggerInterface;
1516
use Symfony\Component\HttpFoundation\Request;
1617
use Symfony\Component\HttpFoundation\Session\SessionInterface;
1718
use Symfony\Component\Routing\Generator\UrlGeneratorInterface;
@@ -113,4 +114,25 @@ public function getRequestRedirections()
113114
],
114115
];
115116
}
117+
118+
public function testTargetPathFromRequestWithInvalidUrl()
119+
{
120+
$httpUtils = $this->createMock(HttpUtils::class);
121+
$options = ['target_path_parameter' => '_my_target_path'];
122+
$token = $this->createMock(TokenInterface::class);
123+
124+
$request = $this->createMock(Request::class);
125+
$request->expects($this->once())
126+
->method('get')->with('_my_target_path')
127+
->willReturn('some_route_name');
128+
129+
$logger = $this->createMock(LoggerInterface::class);
130+
$logger->expects($this->once())
131+
->method('debug')
132+
->with('Ignoring query parameter "_my_target_path": not a valid URL.');
133+
134+
$handler = new DefaultAuthenticationSuccessHandler($httpUtils, $options, $logger);
135+
136+
$handler->onAuthenticationSuccess($request, $token);
137+
}
116138
}

0 commit comments

Comments
 (0)