Skip to content

Commit 9fe0cbe

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '5.4' into 6.0
* 5.4: [Security/Http] Check tokens before loading users from providers
2 parents 029447a + 4f5f6c6 commit 9fe0cbe

8 files changed

+80
-93
lines changed

‎LoginLink/LoginLinkHandler.php

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,6 @@ public function consumeLoginLink(Request $request): UserInterface
8383
{
8484
$userIdentifier = $request->get('user');
8585

86-
try {
87-
$user = $this->userProvider->loadUserByIdentifier($userIdentifier);
88-
} catch (UserNotFoundException $exception) {
89-
throw new InvalidLoginLinkException('User not found.', 0, $exception);
90-
}
91-
9286
if (!$hash = $request->get('hash')) {
9387
throw new InvalidLoginLinkException('Missing "hash" parameter.');
9488
}
@@ -97,7 +91,13 @@ public function consumeLoginLink(Request $request): UserInterface
9791
}
9892

9993
try {
94+
$this->signatureHasher->acceptSignatureHash($userIdentifier, $expires, $hash);
95+
96+
$user = $this->userProvider->loadUserByIdentifier($userIdentifier);
97+
10098
$this->signatureHasher->verifySignatureHash($user, $expires, $hash);
99+
} catch (UserNotFoundException $e) {
100+
throw new InvalidLoginLinkException('User not found.', 0, $e);
101101
} catch (ExpiredSignatureException $e) {
102102
throw new ExpiredLoginLinkException(ucfirst(str_ireplace('signature', 'login link', $e->getMessage())), 0, $e);
103103
} catch (InvalidSignatureException $e) {

‎RememberMe/PersistentRememberMeHandler.php

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -53,18 +53,16 @@ public function __construct(TokenProviderInterface $tokenProvider, string $secre
5353
*/
5454
public function createRememberMeCookie(UserInterface $user): void
5555
{
56-
$series = base64_encode(random_bytes(64));
57-
$tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
56+
$series = random_bytes(66);
57+
$tokenValue = strtr(base64_encode(substr($series, 33)), '+/=', '-_~');
58+
$series = strtr(base64_encode(substr($series, 0, 33)), '+/=', '-_~');
5859
$token = new PersistentToken(\get_class($user), $user->getUserIdentifier(), $series, $tokenValue, new \DateTime());
5960

6061
$this->tokenProvider->createNewToken($token);
6162
$this->createCookie(RememberMeDetails::fromPersistentToken($token, time() + $this->options['lifetime']));
6263
}
6364

64-
/**
65-
* {@inheritdoc}
66-
*/
67-
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
65+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
6866
{
6967
if (!str_contains($rememberMeDetails->getValue(), ':')) {
7068
throw new AuthenticationException('The cookie is incorrectly formatted.');
@@ -86,18 +84,28 @@ public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInte
8684
throw new AuthenticationException('The cookie has expired.');
8785
}
8886

87+
return parent::consumeRememberMeCookie($rememberMeDetails->withValue($persistentToken->getLastUsed()->getTimestamp().':'.$rememberMeDetails->getValue().':'.$persistentToken->getClass()));
88+
}
89+
90+
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
91+
{
92+
[$lastUsed, $series, $tokenValue, $class] = explode(':', $rememberMeDetails->getValue(), 4);
93+
$persistentToken = new PersistentToken($class, $rememberMeDetails->getUserIdentifier(), $series, $tokenValue, new \DateTime('@'.$lastUsed));
94+
8995
// if a token was regenerated less than a minute ago, there is no need to regenerate it
9096
// if multiple concurrent requests reauthenticate a user we do not want to update the token several times
91-
if ($persistentToken->getLastUsed()->getTimestamp() + 60 < time()) {
92-
$tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
93-
$tokenLastUsed = new \DateTime();
94-
if ($this->tokenVerifier) {
95-
$this->tokenVerifier->updateExistingToken($persistentToken, $tokenValue, $tokenLastUsed);
96-
}
97-
$this->tokenProvider->updateToken($series, $tokenValue, $tokenLastUsed);
98-
99-
$this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue));
97+
if ($persistentToken->getLastUsed()->getTimestamp() + 60 >= time()) {
98+
return;
99+
}
100+
101+
$tokenValue = strtr(base64_encode(random_bytes(33)), '+/=', '-_~');
102+
$tokenLastUsed = new \DateTime();
103+
if ($this->tokenVerifier) {
104+
$this->tokenVerifier->updateExistingToken($persistentToken, $tokenValue, $tokenLastUsed);
100105
}
106+
$this->tokenProvider->updateToken($series, $tokenValue, $tokenLastUsed);
107+
108+
$this->createCookie($rememberMeDetails->withValue($series.':'.$tokenValue));
101109
}
102110

103111
/**
@@ -113,7 +121,7 @@ public function clearRememberMeCookie(): void
113121
}
114122

115123
$rememberMeDetails = RememberMeDetails::fromRawCookie($cookie);
116-
[$series, ] = explode(':', $rememberMeDetails->getValue());
124+
[$series] = explode(':', $rememberMeDetails->getValue());
117125
$this->tokenProvider->deleteTokenBySeries($series);
118126
}
119127

@@ -124,9 +132,4 @@ public function getTokenProvider(): TokenProviderInterface
124132
{
125133
return $this->tokenProvider;
126134
}
127-
128-
private function generateHash(string $tokenValue): string
129-
{
130-
return hash_hmac('sha256', $tokenValue, $this->secret);
131-
}
132135
}

‎RememberMe/RememberMeDetails.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,14 @@ public function __construct(string $userFqcn, string $userIdentifier, int $expir
3636

3737
public static function fromRawCookie(string $rawCookie): self
3838
{
39-
$cookieParts = explode(self::COOKIE_DELIMITER, base64_decode($rawCookie), 4);
39+
$cookieParts = explode(self::COOKIE_DELIMITER, $rawCookie, 4);
4040
if (4 !== \count($cookieParts)) {
4141
throw new AuthenticationException('The cookie contains invalid data.');
4242
}
43-
if (false === $cookieParts[1] = base64_decode($cookieParts[1], true)) {
43+
if (false === $cookieParts[1] = base64_decode(strtr($cookieParts[1], '-_~', '+/='), true)) {
4444
throw new AuthenticationException('The user identifier contains a character from outside the base64 alphabet.');
4545
}
46+
$cookieParts[0] = strtr($cookieParts[0], '.', '\\');
4647

4748
return new static(...$cookieParts);
4849
}
@@ -83,6 +84,6 @@ public function getValue(): string
8384
public function toString(): string
8485
{
8586
// $userIdentifier is encoded because it might contain COOKIE_DELIMITER, we assume other values don't
86-
return base64_encode(implode(self::COOKIE_DELIMITER, [$this->userFqcn, base64_encode($this->userIdentifier), $this->expires, $this->value]));
87+
return implode(self::COOKIE_DELIMITER, [strtr($this->userFqcn, '\\', '.'), strtr(base64_encode($this->userIdentifier), '+/=', '-_~'), $this->expires, $this->value]);
8788
}
8889
}

‎RememberMe/SignatureRememberMeHandler.php

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,19 @@ public function createRememberMeCookie(UserInterface $user): void
5353
$this->createCookie($details);
5454
}
5555

56-
/**
57-
* {@inheritdoc}
58-
*/
56+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
57+
{
58+
try {
59+
$this->signatureHasher->acceptSignatureHash($rememberMeDetails->getUserIdentifier(), $rememberMeDetails->getExpires(), $rememberMeDetails->getValue());
60+
} catch (InvalidSignatureException $e) {
61+
throw new AuthenticationException('The cookie\'s hash is invalid.', 0, $e);
62+
} catch (ExpiredSignatureException $e) {
63+
throw new AuthenticationException('The cookie has expired.', 0, $e);
64+
}
65+
66+
return parent::consumeRememberMeCookie($rememberMeDetails);
67+
}
68+
5969
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
6070
{
6171
try {

‎Tests/LoginLink/LoginLinkHandlerTest.php

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,7 @@ protected function setUp(): void
5353

5454
/**
5555
* @group time-sensitive
56+
*
5657
* @dataProvider provideCreateLoginLinkData
5758
*/
5859
public function testCreateLoginLink($user, array $extraProperties, Request $request = null)
@@ -68,7 +69,7 @@ public function testCreateLoginLink($user, array $extraProperties, Request $requ
6869
// allow a small expiration offset to avoid time-sensitivity
6970
&& abs(time() + 600 - $parameters['expires']) <= 1
7071
// make sure hash is what we expect
71-
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], array_values($extraProperties));
72+
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], $extraProperties);
7273
}),
7374
UrlGeneratorInterface::ABSOLUTE_URL
7475
)
@@ -115,7 +116,7 @@ public function provideCreateLoginLinkData()
115116
public function testConsumeLoginLink()
116117
{
117118
$expires = time() + 500;
118-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
119+
$signature = $this->createSignatureHash('weaverryan', $expires);
119120
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
120121

121122
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
@@ -131,44 +132,37 @@ public function testConsumeLoginLink()
131132

132133
public function testConsumeLoginLinkWithExpired()
133134
{
134-
$this->expectException(ExpiredLoginLinkException::class);
135135
$expires = time() - 500;
136-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
136+
$signature = $this->createSignatureHash('weaverryan', $expires);
137137
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
138138

139-
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
140-
$this->userProvider->createUser($user);
141-
142139
$linker = $this->createLinker(['max_uses' => 3]);
140+
$this->expectException(ExpiredLoginLinkException::class);
143141
$linker->consumeLoginLink($request);
144142
}
145143

146144
public function testConsumeLoginLinkWithUserNotFound()
147145
{
148-
$this->expectException(InvalidLoginLinkException::class);
149-
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires=10000');
146+
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires='.(time() + 500));
150147

151148
$linker = $this->createLinker();
149+
$this->expectException(InvalidLoginLinkException::class);
152150
$linker->consumeLoginLink($request);
153151
}
154152

155153
public function testConsumeLoginLinkWithDifferentSignature()
156154
{
157-
$this->expectException(InvalidLoginLinkException::class);
158155
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=fake_hash&expires=%d', time() + 500));
159156

160-
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
161-
$this->userProvider->createUser($user);
162-
163157
$linker = $this->createLinker();
158+
$this->expectException(InvalidLoginLinkException::class);
164159
$linker->consumeLoginLink($request);
165160
}
166161

167162
public function testConsumeLoginLinkExceedsMaxUsage()
168163
{
169-
$this->expectException(ExpiredLoginLinkException::class);
170164
$expires = time() + 500;
171-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
165+
$signature = $this->createSignatureHash('weaverryan', $expires);
172166
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
173167

174168
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
@@ -179,6 +173,7 @@ public function testConsumeLoginLinkExceedsMaxUsage()
179173
$this->expiredLinkCache->save($item);
180174

181175
$linker = $this->createLinker(['max_uses' => 3]);
176+
$this->expectException(ExpiredLoginLinkException::class);
182177
$linker->consumeLoginLink($request);
183178
}
184179

@@ -206,15 +201,12 @@ public function testConsumeLoginLinkWithMissingExpiration()
206201
$linker->consumeLoginLink($request);
207202
}
208203

209-
private function createSignatureHash(string $username, int $expires, array $extraFields): string
204+
private function createSignatureHash(string $username, int $expires, array $extraFields = ['emailProperty' => '[email protected]', 'passwordProperty' => 'pwhash']): string
210205
{
211-
$fields = [base64_encode($username), $expires];
212-
foreach ($extraFields as $extraField) {
213-
$fields[] = base64_encode($extraField);
214-
}
206+
$hasher = new SignatureHasher($this->propertyAccessor, array_keys($extraFields), 's3cret');
207+
$user = new TestLoginLinkHandlerUser($username, $extraFields['emailProperty'] ?? '', $extraFields['passwordProperty'] ?? '', $extraFields['lastAuthenticatedAt'] ?? null);
215208

216-
// matches hash logic in the class
217-
return base64_encode(hash_hmac('sha256', implode(':', $fields), 's3cret'));
209+
return $hasher->computeSignatureHash($user, $expires);
218210
}
219211

220212
private function createLinker(array $options = [], array $extraProperties = ['emailProperty', 'passwordProperty']): LoginLinkHandler
@@ -298,7 +290,7 @@ public function loadUserByIdentifier(string $userIdentifier): TestLoginLinkHandl
298290

299291
public function refreshUser(UserInterface $user): TestLoginLinkHandlerUser
300292
{
301-
return $this->users[$username];
293+
return $this->users[$user->getUserIdentifier()];
302294
}
303295

304296
public function supportsClass(string $class): bool

‎Tests/RememberMe/PersistentRememberMeHandlerTest.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -93,8 +93,8 @@ public function testConsumeRememberMeCookieValid()
9393

9494
/** @var Cookie $cookie */
9595
$cookie = $this->request->attributes->get(ResponseListener::COOKIE_ATTR_NAME);
96-
$rememberParts = explode(':', base64_decode($rememberMeDetails->toString()), 4);
97-
$cookieParts = explode(':', base64_decode($cookie->getValue()), 4);
96+
$rememberParts = explode(':', $rememberMeDetails->toString(), 4);
97+
$cookieParts = explode(':', $cookie->getValue(), 4);
9898

9999
$this->assertSame($rememberParts[0], $cookieParts[0]); // class
100100
$this->assertSame($rememberParts[1], $cookieParts[1]); // identifier

‎Tests/RememberMe/SignatureRememberMeHandlerTest.php

Lines changed: 15 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,11 @@
1212
namespace Symfony\Component\Security\Http\Tests\RememberMe;
1313

1414
use PHPUnit\Framework\TestCase;
15-
use Symfony\Bridge\PhpUnit\ClockMock;
1615
use Symfony\Component\HttpFoundation\Cookie;
1716
use Symfony\Component\HttpFoundation\Request;
1817
use Symfony\Component\HttpFoundation\RequestStack;
18+
use Symfony\Component\PropertyAccess\PropertyAccess;
1919
use Symfony\Component\Security\Core\Exception\AuthenticationException;
20-
use Symfony\Component\Security\Core\Signature\Exception\ExpiredSignatureException;
21-
use Symfony\Component\Security\Core\Signature\Exception\InvalidSignatureException;
2220
use Symfony\Component\Security\Core\Signature\SignatureHasher;
2321
use Symfony\Component\Security\Core\User\InMemoryUser;
2422
use Symfony\Component\Security\Core\User\InMemoryUserProvider;
@@ -36,10 +34,8 @@ class SignatureRememberMeHandlerTest extends TestCase
3634

3735
protected function setUp(): void
3836
{
39-
$this->signatureHasher = $this->createMock(SignatureHasher::class);
37+
$this->signatureHasher = new SignatureHasher(PropertyAccess::createPropertyAccessor(), [], 's3cret');
4038
$this->userProvider = new InMemoryUserProvider();
41-
$user = new InMemoryUser('wouter', null);
42-
$this->userProvider->createUser($user);
4339
$this->requestStack = new RequestStack();
4440
$this->request = Request::create('/login');
4541
$this->requestStack->push($this->request);
@@ -51,18 +47,17 @@ protected function setUp(): void
5147
*/
5248
public function testCreateRememberMeCookie()
5349
{
54-
ClockMock::register(SignatureRememberMeHandler::class);
55-
5650
$user = new InMemoryUser('wouter', null);
57-
$this->signatureHasher->expects($this->once())->method('computeSignatureHash')->with($user, $expire = time() + 31536000)->willReturn('abc');
51+
$signature = $this->signatureHasher->computeSignatureHash($user, $expire = time() + 31536000);
52+
$this->userProvider->createUser(new InMemoryUser('wouter', null));
5853

5954
$this->handler->createRememberMeCookie($user);
6055

6156
$this->assertTrue($this->request->attributes->has(ResponseListener::COOKIE_ATTR_NAME));
6257

6358
/** @var Cookie $cookie */
6459
$cookie = $this->request->attributes->get(ResponseListener::COOKIE_ATTR_NAME);
65-
$this->assertEquals(base64_encode(InMemoryUser::class.':d291dGVy:'.$expire.':abc'), $cookie->getValue());
60+
$this->assertEquals(strtr(InMemoryUser::class, '\\', '.').':d291dGVy:'.$expire.':'.$signature, $cookie->getValue());
6661
}
6762

6863
public function testClearRememberMeCookie()
@@ -76,50 +71,36 @@ public function testClearRememberMeCookie()
7671
$this->assertNull($cookie->getValue());
7772
}
7873

79-
/**
80-
* @group time-sensitive
81-
*/
8274
public function testConsumeRememberMeCookieValid()
8375
{
84-
$this->signatureHasher->expects($this->once())->method('verifySignatureHash')->with($user = new InMemoryUser('wouter', null), 360, 'signature');
85-
$this->signatureHasher->expects($this->any())
86-
->method('computeSignatureHash')
87-
->with($user, $expire = time() + 31536000)
88-
->willReturn('newsignature');
76+
$user = new InMemoryUser('wouter', null);
77+
$signature = $this->signatureHasher->computeSignatureHash($user, $expire = time() + 3600);
78+
$this->userProvider->createUser(new InMemoryUser('wouter', null));
8979

90-
$rememberMeDetails = new RememberMeDetails(InMemoryUser::class, 'wouter', 360, 'signature');
80+
$rememberMeDetails = new RememberMeDetails(InMemoryUser::class, 'wouter', $expire, $signature);
9181
$this->handler->consumeRememberMeCookie($rememberMeDetails);
9282

9383
$this->assertTrue($this->request->attributes->has(ResponseListener::COOKIE_ATTR_NAME));
9484

9585
/** @var Cookie $cookie */
9686
$cookie = $this->request->attributes->get(ResponseListener::COOKIE_ATTR_NAME);
97-
$this->assertEquals((new RememberMeDetails(InMemoryUser::class, 'wouter', $expire, 'newsignature'))->toString(), $cookie->getValue());
87+
$this->assertNotEquals((new RememberMeDetails(InMemoryUser::class, 'wouter', $expire, $signature))->toString(), $cookie->getValue());
9888
}
9989

10090
public function testConsumeRememberMeCookieInvalidHash()
10191
{
10292
$this->expectException(AuthenticationException::class);
10393
$this->expectExceptionMessage('The cookie\'s hash is invalid.');
104-
105-
$this->signatureHasher->expects($this->any())
106-
->method('verifySignatureHash')
107-
->with(new InMemoryUser('wouter', null), 360, 'badsignature')
108-
->will($this->throwException(new InvalidSignatureException()));
109-
110-
$this->handler->consumeRememberMeCookie(new RememberMeDetails(InMemoryUser::class, 'wouter', 360, 'badsignature'));
94+
$this->handler->consumeRememberMeCookie(new RememberMeDetails(InMemoryUser::class, 'wouter', time() + 600, 'badsignature'));
11195
}
11296

11397
public function testConsumeRememberMeCookieExpired()
11498
{
99+
$user = new InMemoryUser('wouter', null);
100+
$signature = $this->signatureHasher->computeSignatureHash($user, 360);
101+
115102
$this->expectException(AuthenticationException::class);
116103
$this->expectExceptionMessage('The cookie has expired.');
117-
118-
$this->signatureHasher->expects($this->any())
119-
->method('verifySignatureHash')
120-
->with(new InMemoryUser('wouter', null), 360, 'signature')
121-
->will($this->throwException(new ExpiredSignatureException()));
122-
123-
$this->handler->consumeRememberMeCookie(new RememberMeDetails(InMemoryUser::class, 'wouter', 360, 'signature'));
104+
$this->handler->consumeRememberMeCookie(new RememberMeDetails(InMemoryUser::class, 'wouter', 360, $signature));
124105
}
125106
}

‎composer.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
],
1818
"require": {
1919
"php": ">=8.0.2",
20-
"symfony/security-core": "^5.4|^6.0",
20+
"symfony/security-core": "^5.4.19|~6.0.19|~6.1.11|^6.2.5",
2121
"symfony/http-foundation": "^5.4|^6.0",
2222
"symfony/http-kernel": "^5.4|^6.0",
2323
"symfony/polyfill-mbstring": "~1.0",

0 commit comments

Comments
 (0)