Skip to content

Commit

Permalink
bug #51350 [Security] Prevent creating session in stateless firewalls…
Browse files Browse the repository at this point in the history
… (Seb33300)

This PR was squashed before being merged into the 6.3 branch.

Discussion
----------

[Security] Prevent creating session in stateless firewalls

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix symfony/symfony#51319
| License       | MIT
| Doc PR        |
<!--
Replace this notice by a short README for your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Please check related issue for details.

Same as symfony/symfony#51320 with `@chalasr` suggestion: symfony/symfony#51320 (comment)

Commits
-------

4efd50e34c [Security] Prevent creating session in stateless firewalls
  • Loading branch information
chalasr committed Aug 25, 2023
2 parents 16f232d + e2af19c commit 0afb37c
Show file tree
Hide file tree
Showing 4 changed files with 35 additions and 2 deletions.
4 changes: 3 additions & 1 deletion Authentication/DefaultAuthenticationFailureHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,9 @@ public function onAuthenticationFailure(Request $request, AuthenticationExceptio

$this->logger?->debug('Authentication failure, redirect triggered.', ['failure_path' => $options['failure_path']]);

$request->getSession()->set(SecurityRequestAttributes::AUTHENTICATION_ERROR, $exception);
if (!$request->attributes->getBoolean('_stateless')) {
$request->getSession()->set(SecurityRequestAttributes::AUTHENTICATION_ERROR, $exception);
}

return $this->httpUtils->createRedirectResponse($request, $options['failure_path']);
}
Expand Down
2 changes: 1 addition & 1 deletion Authentication/DefaultAuthenticationSuccessHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ protected function determineTargetUrl(Request $request): string
}

$firewallName = $this->getFirewallName();
if (null !== $firewallName && $targetUrl = $this->getTargetPath($request->getSession(), $firewallName)) {
if (null !== $firewallName && !$request->attributes->getBoolean('_stateless') && $targetUrl = $this->getTargetPath($request->getSession(), $firewallName)) {
$this->removeTargetPath($request->getSession(), $firewallName);

return $targetUrl;
Expand Down
12 changes: 12 additions & 0 deletions Tests/Authentication/DefaultAuthenticationFailureHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ protected function setUp(): void

$this->session = $this->createMock(SessionInterface::class);
$this->request = $this->createMock(Request::class);
$this->request->attributes = new ParameterBag(['_stateless' => false]);
$this->request->expects($this->any())->method('getSession')->willReturn($this->session);
$this->exception = $this->getMockBuilder(AuthenticationException::class)->onlyMethods(['getMessage'])->getMock();
}
Expand Down Expand Up @@ -89,6 +90,17 @@ public function testExceptionIsPersistedInSession()
$handler->onAuthenticationFailure($this->request, $this->exception);
}

public function testExceptionIsNotPersistedInSessionOnStatelessRequest()
{
$this->request->attributes = new ParameterBag(['_stateless' => true]);

$this->session->expects($this->never())
->method('set')->with(SecurityRequestAttributes::AUTHENTICATION_ERROR, $this->exception);

$handler = new DefaultAuthenticationFailureHandler($this->httpKernel, $this->httpUtils, [], $this->logger);
$handler->onAuthenticationFailure($this->request, $this->exception);
}

public function testExceptionIsPassedInRequestOnForward()
{
$options = ['failure_forward' => true];
Expand Down
19 changes: 19 additions & 0 deletions Tests/Authentication/DefaultAuthenticationSuccessHandlerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,25 @@ public function testRequestRedirectionsWithTargetPathInSessions()
$this->assertSame('http://localhost/admin/dashboard', $handler->onAuthenticationSuccess($requestWithSession, $token)->getTargetUrl());
}

public function testStatelessRequestRedirections()
{
$session = $this->createMock(SessionInterface::class);
$session->expects($this->never())->method('get')->with('_security.admin.target_path');
$session->expects($this->never())->method('remove')->with('_security.admin.target_path');
$statelessRequest = Request::create('/');
$statelessRequest->setSession($session);
$statelessRequest->attributes->set('_stateless', true);

$urlGenerator = $this->createMock(UrlGeneratorInterface::class);
$urlGenerator->expects($this->any())->method('generate')->willReturn('http://localhost/login');
$httpUtils = new HttpUtils($urlGenerator);
$token = $this->createMock(TokenInterface::class);
$handler = new DefaultAuthenticationSuccessHandler($httpUtils);
$handler->setFirewallName('admin');

$this->assertSame('http://localhost/', $handler->onAuthenticationSuccess($statelessRequest, $token)->getTargetUrl());
}

public static function getRequestRedirections()
{
return [
Expand Down

0 comments on commit 0afb37c

Please sign in to comment.