Skip to content

Commit 5c8f064

Browse files
nicolas-grekasnicolas-grekas
committed
Merge branch '6.1' into 6.2
* 6.1: [Security/Http] Check tokens before loading users from providers
2 parents a725660 + d77cdbb commit 5c8f064

File tree

8 files changed

+79
-86
lines changed

8 files changed

+79
-86
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: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -50,15 +50,16 @@ public function __construct(TokenProviderInterface $tokenProvider, #[\SensitiveP
5050

5151
public function createRememberMeCookie(UserInterface $user): void
5252
{
53-
$series = base64_encode(random_bytes(64));
54-
$tokenValue = $this->generateHash(base64_encode(random_bytes(64)));
53+
$series = random_bytes(66);
54+
$tokenValue = strtr(base64_encode(substr($series, 33)), '+/=', '-_~');
55+
$series = strtr(base64_encode(substr($series, 0, 33)), '+/=', '-_~');
5556
$token = new PersistentToken($user::class, $user->getUserIdentifier(), $series, $tokenValue, new \DateTime());
5657

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

61-
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
62+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
6263
{
6364
if (!str_contains($rememberMeDetails->getValue(), ':')) {
6465
throw new AuthenticationException('The cookie is incorrectly formatted.');
@@ -80,16 +81,26 @@ public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInte
8081
throw new AuthenticationException('The cookie has expired.');
8182
}
8283

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

95106
public function clearRememberMeCookie(): void
@@ -102,7 +113,7 @@ public function clearRememberMeCookie(): void
102113
}
103114

104115
$rememberMeDetails = RememberMeDetails::fromRawCookie($cookie);
105-
[$series, ] = explode(':', $rememberMeDetails->getValue());
116+
[$series] = explode(':', $rememberMeDetails->getValue());
106117
$this->tokenProvider->deleteTokenBySeries($series);
107118
}
108119

@@ -113,9 +124,4 @@ public function getTokenProvider(): TokenProviderInterface
113124
{
114125
return $this->tokenProvider;
115126
}
116-
117-
private function generateHash(string $tokenValue): string
118-
{
119-
return hash_hmac('sha256', $tokenValue, $this->secret);
120-
}
121127
}

‎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 & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,19 @@ public function createRememberMeCookie(UserInterface $user): void
5050
$this->createCookie($details);
5151
}
5252

53+
public function consumeRememberMeCookie(RememberMeDetails $rememberMeDetails): UserInterface
54+
{
55+
try {
56+
$this->signatureHasher->acceptSignatureHash($rememberMeDetails->getUserIdentifier(), $rememberMeDetails->getExpires(), $rememberMeDetails->getValue());
57+
} catch (InvalidSignatureException $e) {
58+
throw new AuthenticationException('The cookie\'s hash is invalid.', 0, $e);
59+
} catch (ExpiredSignatureException $e) {
60+
throw new AuthenticationException('The cookie has expired.', 0, $e);
61+
}
62+
63+
return parent::consumeRememberMeCookie($rememberMeDetails);
64+
}
65+
5366
public function processRememberMe(RememberMeDetails $rememberMeDetails, UserInterface $user): void
5467
{
5568
try {

‎Tests/LoginLink/LoginLinkHandlerTest.php‎

Lines changed: 16 additions & 24 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
)
@@ -127,7 +128,7 @@ public function testCreateLoginLinkWithLifetime()
127128
&& abs(time() + 1000 - $parameters['expires']) <= 1
128129
&& isset($parameters['hash'])
129130
// make sure hash is what we expect
130-
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], array_values($extraProperties));
131+
&& $parameters['hash'] === $this->createSignatureHash('weaverryan', $parameters['expires'], $extraProperties);
131132
}),
132133
UrlGeneratorInterface::ABSOLUTE_URL
133134
)
@@ -147,7 +148,7 @@ public function testCreateLoginLinkWithLifetime()
147148
public function testConsumeLoginLink()
148149
{
149150
$expires = time() + 500;
150-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
151+
$signature = $this->createSignatureHash('weaverryan', $expires);
151152
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
152153

153154
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
@@ -163,44 +164,37 @@ public function testConsumeLoginLink()
163164

164165
public function testConsumeLoginLinkWithExpired()
165166
{
166-
$this->expectException(ExpiredLoginLinkException::class);
167167
$expires = time() - 500;
168-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
168+
$signature = $this->createSignatureHash('weaverryan', $expires);
169169
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
170170

171-
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
172-
$this->userProvider->createUser($user);
173-
174171
$linker = $this->createLinker(['max_uses' => 3]);
172+
$this->expectException(ExpiredLoginLinkException::class);
175173
$linker->consumeLoginLink($request);
176174
}
177175

178176
public function testConsumeLoginLinkWithUserNotFound()
179177
{
180-
$this->expectException(InvalidLoginLinkException::class);
181-
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires=10000');
178+
$request = Request::create('/login/verify?user=weaverryan&hash=thehash&expires='.(time() + 500));
182179

183180
$linker = $this->createLinker();
181+
$this->expectException(InvalidLoginLinkException::class);
184182
$linker->consumeLoginLink($request);
185183
}
186184

187185
public function testConsumeLoginLinkWithDifferentSignature()
188186
{
189-
$this->expectException(InvalidLoginLinkException::class);
190187
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=fake_hash&expires=%d', time() + 500));
191188

192-
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
193-
$this->userProvider->createUser($user);
194-
195189
$linker = $this->createLinker();
190+
$this->expectException(InvalidLoginLinkException::class);
196191
$linker->consumeLoginLink($request);
197192
}
198193

199194
public function testConsumeLoginLinkExceedsMaxUsage()
200195
{
201-
$this->expectException(ExpiredLoginLinkException::class);
202196
$expires = time() + 500;
203-
$signature = $this->createSignatureHash('weaverryan', $expires, ['[email protected]', 'pwhash']);
197+
$signature = $this->createSignatureHash('weaverryan', $expires);
204198
$request = Request::create(sprintf('/login/verify?user=weaverryan&hash=%s&expires=%d', $signature, $expires));
205199

206200
$user = new TestLoginLinkHandlerUser('weaverryan', '[email protected]', 'pwhash');
@@ -211,6 +205,7 @@ public function testConsumeLoginLinkExceedsMaxUsage()
211205
$this->expiredLinkCache->save($item);
212206

213207
$linker = $this->createLinker(['max_uses' => 3]);
208+
$this->expectException(ExpiredLoginLinkException::class);
214209
$linker->consumeLoginLink($request);
215210
}
216211

@@ -238,15 +233,12 @@ public function testConsumeLoginLinkWithMissingExpiration()
238233
$linker->consumeLoginLink($request);
239234
}
240235

241-
private function createSignatureHash(string $username, int $expires, array $extraFields): string
236+
private function createSignatureHash(string $username, int $expires, array $extraFields = ['emailProperty' => '[email protected]', 'passwordProperty' => 'pwhash']): string
242237
{
243-
$fields = [base64_encode($username), $expires];
244-
foreach ($extraFields as $extraField) {
245-
$fields[] = base64_encode($extraField);
246-
}
238+
$hasher = new SignatureHasher($this->propertyAccessor, array_keys($extraFields), 's3cret');
239+
$user = new TestLoginLinkHandlerUser($username, $extraFields['emailProperty'] ?? '', $extraFields['passwordProperty'] ?? '', $extraFields['lastAuthenticatedAt'] ?? null);
247240

248-
// matches hash logic in the class
249-
return base64_encode(hash_hmac('sha256', implode(':', $fields), 's3cret'));
241+
return $hasher->computeSignatureHash($user, $expires);
250242
}
251243

252244
private function createLinker(array $options = [], array $extraProperties = ['emailProperty', 'passwordProperty']): LoginLinkHandler
@@ -330,7 +322,7 @@ public function loadUserByIdentifier(string $userIdentifier): TestLoginLinkHandl
330322

331323
public function refreshUser(UserInterface $user): TestLoginLinkHandlerUser
332324
{
333-
return $this->users[$username];
325+
return $this->users[$user->getUserIdentifier()];
334326
}
335327

336328
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
}

0 commit comments

Comments
 (0)