8000 [Security/Http] Ignore invalid URLs found in failure/success paths · symfony/symfony@389df98 · GitHub
[go: up one dir, main page]
Skip to content
Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 389df98

Browse files
nicolas-grekasnicolas-grekas
committed
[Security/Http] Ignore invalid URLs found in failure/success paths
1 parent ce4a1a9 commit 389df98

File tree

5 files changed

+67
-12
lines changed

5 files changed

+67
-12
lines changed

‎src/Symfony/Bundle/SecurityBundle/Resources/config/security_listeners.xml

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,6 +88,7 @@
8888
<service id="security.authentication.success_handler" class="Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler" abstract="true">
8989
<argument type="service" id="security.http_utils" />
9090
<argument type="collection" /> <!-- Options -->
91+
<argument type="service" id="logger" on-invalid="null" />
9192
</service>
9293

9394
<service id="security.authentication.custom_failure_handler" class="Symfony\Component\Security\Http\Authentication\CustomAuthenticationFailureHandler" abstract="true">

‎src/Symfony/Component/Security/Http/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
}

‎src/Symfony/Component/Security/Http/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_ F438 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

‎src/Symfony/Component/Security/Http/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);

‎src/Symfony/Component/Security/Http/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)
0