Skip to content

Commit 107a0e5

Browse files
Merge branch '4.4' into 5.4
* 4.4: [Security/Http] Remove CSRF tokens from storage on successful login [HttpKernel] Remove private headers before storing responses with HttpCache
2 parents b8c7604 + c75c569 commit 107a0e5

File tree

8 files changed

+100
-16
lines changed

8 files changed

+100
-16
lines changed

src/Symfony/Bundle/SecurityBundle/Resources/config/security.php

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,10 @@
103103
->set('security.authentication.trust_resolver', AuthenticationTrustResolver::class)
104104

105105
->set('security.authentication.session_strategy', SessionAuthenticationStrategy::class)
106-
->args([param('security.authentication.session_strategy.strategy')])
106+
->args([
107+
param('security.authentication.session_strategy.strategy'),
108+
service('security.csrf.token_storage')->ignoreOnInvalid(),
109+
])
107110
->alias(SessionAuthenticationStrategyInterface::class, 'security.authentication.session_strategy')
108111

109112
->set('security.authentication.session_strategy_noop', SessionAuthenticationStrategy::class)

src/Symfony/Bundle/SecurityBundle/Tests/Functional/CsrfFormLoginTest.php

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,12 @@
1111

1212
namespace Symfony\Bundle\SecurityBundle\Tests\Functional;
1313

14+
use Symfony\Bundle\FrameworkBundle\KernelBrowser;
15+
use Symfony\Component\EventDispatcher\EventDispatcherInterface;
16+
use Symfony\Component\HttpFoundation\Response;
17+
use Symfony\Component\HttpKernel\Event\RequestEvent;
18+
use Symfony\Component\HttpKernel\KernelEvents;
19+
1420
class CsrfFormLoginTest extends AbstractWebTestCase
1521
{
1622
/**
@@ -20,6 +26,10 @@ public function testFormLoginAndLogoutWithCsrfTokens($options)
2026
{
2127
$client = $this->createClient($options);
2228

29+
$this->callInRequestContext($client, function () {
30+
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
31+
});
32+
2333
$form = $client->request('GET', '/login')->selectButton('login')->form();
2434
$form['user_login[username]'] = 'johannes';
2535
$form['user_login[password]'] = 'test';
@@ -40,6 +50,10 @@ public function testFormLoginAndLogoutWithCsrfTokens($options)
4050
$client->click($logoutLinks[0]);
4151

4252
$this->assertRedirect($client->getResponse(), '/');
53+
54+
$this->callInRequestContext($client, function () {
55+
$this->assertFalse(static::getContainer()->get('security.csrf.token_storage')->hasToken('foo'));
56+
});
4357
}
4458

4559
/**
@@ -49,6 +63,10 @@ public function testFormLoginWithInvalidCsrfToken($options)
4963
{
5064
$client = $this->createClient($options);
5165

66+
$this->callInRequestContext($client, function () {
67+
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
68+
});
69+
5270
$form = $client->request('GET', '/login')->selectButton('login')->form();
5371
$form['user_login[_token]'] = '';
5472
$client->submit($form);
@@ -57,6 +75,10 @@ public function testFormLoginWithInvalidCsrfToken($options)
5775

5876
$text = $client->followRedirect()->text(null, true);
5977
$this->assertStringContainsString('Invalid CSRF token.', $text);
78+
79+
$this->callInRequestContext($client, function () {
80+
$this->assertTrue(static::getContainer()->get('security.csrf.token_storage')->hasToken('foo'));
81+
});
6082
}
6183

6284
/**
@@ -202,4 +224,22 @@ public function provideLegacyClientOptions()
202224
yield [['test_case' => 'CsrfFormLogin', 'root_config' => 'legacy_config.yml', 'enable_authenticator_manager' => false]];
203225
yield [['test_case' => 'CsrfFormLogin', 'root_config' => 'legacy_routes_as_path.yml', 'enable_authenticator_manager' => false]];
204226
}
227+
228+
private function callInRequestContext(KernelBrowser $client, callable $callable): void
229+
{
230+
/** @var EventDispatcherInterface $eventDispatcher */
231+
$eventDispatcher = static::getContainer()->get(EventDispatcherInterface::class);
232+
$wrappedCallable = function (RequestEvent $event) use (&$callable) {
233+
$callable();
234+
$event->setResponse(new Response(''));
235+
$event->stopPropagation();
236+
};
237+
238+
$eventDispatcher->addListener(KernelEvents::REQUEST, $wrappedCallable);
239+
try {
240+
$client->request('GET', '/'.uniqid('', true));
241+
} finally {
242+
$eventDispatcher->removeListener(KernelEvents::REQUEST, $wrappedCallable);
243+
}
244+
}
205245
}

src/Symfony/Bundle/SecurityBundle/Tests/Functional/LogoutTest.php

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,14 @@ public function testCsrfTokensAreClearedOnLogout()
2424
{
2525
$client = $this->createClient(['enable_authenticator_manager' => true, 'test_case' => 'LogoutWithoutSessionInvalidation', 'root_config' => 'config.yml']);
2626
$client->disableReboot();
27-
$this->callInRequestContext($client, function () {
28-
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
29-
});
3027

3128
$client->request('POST', '/login', [
3229
'_username' => 'johannes',
3330
'_password' => 'test',
3431
]);
3532

3633
$this->callInRequestContext($client, function () {
37-
$this->assertTrue(static::getContainer()->get('security.csrf.token_storage')->hasToken('foo'));
38-
$this->assertSame('bar', static::getContainer()->get('security.csrf.token_storage')->getToken('foo'));
34+
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
3935
});
4036

4137
$client->request('GET', '/logout');
@@ -52,18 +48,14 @@ public function testLegacyCsrfTokensAreClearedOnLogout()
5248
{
5349
$client = $this->createClient(['enable_authenticator_manager' => false, 'test_case' => 'LogoutWithoutSessionInvalidation', 'root_config' => 'config.yml']);
5450
$client->disableReboot();
55-
$this->callInRequestContext($client, function () {
56-
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
57-
});
5851

5952
$client->request('POST', '/login', [
6053
'_username' => 'johannes',
6154
'_password' => 'test',
6255
]);
6356

6457
$this->callInRequestContext($client, function () {
65-
$this->assertTrue(static::getContainer()->get('security.csrf.token_storage')->hasToken('foo'));
66-
$this->assertSame('bar', static::getContainer()->get('security.csrf.token_storage')->getToken('foo'));
58+
static::getContainer()->get('security.csrf.token_storage')->setToken('foo', 'bar');
6759
});
6860

6961
$client->request('GET', '/logout');

src/Symfony/Bundle/SecurityBundle/composer.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@
2929
"symfony/security-core": "^5.4|^6.0",
3030
"symfony/security-csrf": "^4.4|^5.0|^6.0",
3131
"symfony/security-guard": "^5.3",
32-
"symfony/security-http": "^5.4|^6.0"
32+
"symfony/security-http": "^5.4.20|~6.0.20|~6.1.12|^6.2.6"
3333
},
3434
"require-dev": {
3535
"doctrine/annotations": "^1.10.4|^2",

src/Symfony/Component/HttpKernel/HttpCache/Store.php

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,17 +29,28 @@ class Store implements StoreInterface
2929
private $keyCache;
3030
/** @var array<string, resource> */
3131
private $locks = [];
32+
private $options;
3233

3334
/**
35+
* Constructor.
36+
*
37+
* The available options are:
38+
*
39+
* * private_headers Set of response headers that should not be stored
40+
* when a response is cached. (default: Set-Cookie)
41+
*
3442
* @throws \RuntimeException
3543
*/
36-
public function __construct(string $root)
44+
public function __construct(string $root, array $options = [])
3745
{
3846
$this->root = $root;
3947
if (!is_dir($this->root) && !@mkdir($this->root, 0777, true) && !is_dir($this->root)) {
4048
throw new \RuntimeException(sprintf('Unable to create the store directory (%s).', $this->root));
4149
}
4250
$this->keyCache = new \SplObjectStorage();
51+
$this->options = array_merge([
52+
'private_headers' => ['Set-Cookie'],
53+
], $options);
4354
}
4455

4556
/**
@@ -216,6 +227,10 @@ public function write(Request $request, Response $response)
216227
$headers = $this->persistResponse($response);
217228
unset($headers['age']);
218229

230+
foreach ($this->options['private_headers'] as $h) {
231+
unset($headers[strtolower($h)]);
232+
}
233+
219234
array_unshift($entries, [$storedEnv, $headers]);
220235

221236
if (!$this->save($key, serialize($entries))) {

src/Symfony/Component/HttpKernel/Tests/HttpCache/StoreTest.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,10 @@
1212
namespace Symfony\Component\HttpKernel\Tests\HttpCache;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\HttpFoundation\Cookie;
1516
use Symfony\Component\HttpFoundation\Request;
1617
use Symfony\Component\HttpFoundation\Response;
18+
use Symfony\Component\HttpKernel\HttpCache\HttpCache;
1719
use Symfony\Component\HttpKernel\HttpCache\Store;
1820

1921
class StoreTest extends TestCase
@@ -317,6 +319,17 @@ public function testPurgeHttpAndHttps()
317319
$this->assertEmpty($this->getStoreMetadata($requestHttps));
318320
}
319321

322+
public function testDoesNotStorePrivateHeaders()
323+
{
324+
$request = Request::create('https://example.com/foo');
325+
$response = new Response('foo');
326+
$response->headers->setCookie(Cookie::fromString('foo=bar'));
327+
328+
$this->store->write($request, $response);
329+
$this->assertArrayNotHasKey('set-cookie', $this->getStoreMetadata($request)[0][1]);
330+
$this->assertNotEmpty($response->headers->getCookies());
331+
}
332+
320333
protected function storeSimpleEntry($path = null, $headers = [])
321334
{
322335
if (null === $path) {

src/Symfony/Component/Security/Http/Session/SessionAuthenticationStrategy.php

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Component\HttpFoundation\Request;
1515
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
16+
use Symfony\Component\Security\Csrf\TokenStorage\ClearableTokenStorageInterface;
1617

1718
/**
1819
* The default session strategy implementation.
@@ -31,10 +32,15 @@ class SessionAuthenticationStrategy implements SessionAuthenticationStrategyInte
3132
public const INVALIDATE = 'invalidate';
3233

3334
private $strategy;
35+
private $csrfTokenStorage = null;
3436

35-
public function __construct(string $strategy)
37+
public function __construct(string $strategy, ClearableTokenStorageInterface $csrfTokenStorage = null)
3638
{
3739
$this->strategy = $strategy;
40+
41+
if (self::MIGRATE === $strategy) {
42+
$this->csrfTokenStorage = $csrfTokenStorage;
43+
}
3844
}
3945

4046
/**
@@ -47,10 +53,12 @@ public function onAuthentication(Request $request, TokenInterface $token)
4753
return;
4854

4955
case self::MIGRATE:
50-
// Note: this logic is duplicated in several authentication listeners
51-
// until Symfony 5.0 due to a security fix with BC compat
5256
$request->getSession()->migrate(true);
5357

58+
if ($this->csrfTokenStorage) {
59+
$this->csrfTokenStorage->clear();
60+
}
61+
5462
return;
5563

5664
case self::INVALIDATE:

src/Symfony/Component/Security/Http/Tests/Session/SessionAuthenticationStrategyTest.php

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Component\HttpFoundation\Request;
1616
use Symfony\Component\HttpFoundation\Session\SessionInterface;
1717
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
18+
use Symfony\Component\Security\Csrf\TokenStorage\ClearableTokenStorageInterface;
1819
use Symfony\Component\Security\Http\Session\SessionAuthenticationStrategy;
1920

2021
class SessionAuthenticationStrategyTest extends TestCase
@@ -57,6 +58,18 @@ public function testSessionIsInvalidated()
5758
$strategy->onAuthentication($this->getRequest($session), $this->createMock(TokenInterface::class));
5859
}
5960

61+
public function testCsrfTokensAreCleared()
62+
{
63+
$session = $this->createMock(SessionInterface::class);
64+
$session->expects($this->once())->method('migrate')->with($this->equalTo(true));
65+
66+
$csrfStorage = $this->createMock(ClearableTokenStorageInterface::class);
67+
$csrfStorage->expects($this->once())->method('clear');
68+
69+
$strategy = new SessionAuthenticationStrategy(SessionAuthenticationStrategy::MIGRATE, $csrfStorage);
70+
$strategy->onAuthentication($this->getRequest($session), $this->createMock(TokenInterface::class));
71+
}
72+
6073
private function getRequest($session = null)
6174
{
6275
$request = $this->createMock(Request::class);

0 commit comments

Comments
 (0)