Skip to content

Commit ecb18a4

Browse files
julien-ncjulien-nc
committed
fix(oauth2): prevent redeeming an unused authorization code via the refresh token grant
The authorization code expiration and redemption state checks only applied to the authorization_code grant, so an expired but still unredeemed authorization code could be exchanged for an access token through the refresh token grant, bypassing the intended 10-minute expiration until the expired code record was cleaned up. Require a row to have already delivered a token (token_count > 0) when rotating through the refresh token grant, enforced atomically in rotateToken() which already serializes concurrent redemptions through the hashed_code check. Assisted-by: OpenCode:GLM-5.3-Flash Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
1 parent 7a1d440 commit ecb18a4

4 files changed

Lines changed: 80 additions & 9 deletions

File tree

‎apps/oauth2/lib/Controller/OauthApiController.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ public function getToken(
227227
$code,
228228
$newCode,
229229
$newEncryptedToken,
230-
$grant_type === 'authorization_code',
230+
$grant_type,
231231
);
232232

233233
if ($updatedRows !== 1) {

‎apps/oauth2/lib/Db/AccessTokenMapper.php‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -66,12 +66,14 @@ public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void
6666
}
6767

6868
/**
69-
* Rotate an access token only if it still matches the caller's previously-read state.
69+
* Rotate an access token only if it still matches the caller's previously-read state
70+
* and its token count matches the requested grant type.
7071
*
71-
* @param bool $expectAuthorizationCodeState Require the token to still be unused
72+
* @param 'authorization_code'|'refresh_token' $grantType Require the row to still be unused for 'authorization_code'
73+
* or to have already delivered one token for 'refresh_token'
7274
* @return int Number of updated rows
7375
*/
74-
public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int {
76+
public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, string $grantType): int {
7577
$qb = $this->connection->getQueryBuilder();
7678
$qb
7779
->update($this->getTableName())
@@ -81,8 +83,12 @@ public function rotateToken(int $id, string $oldCode, string $newCode, string $e
8183
->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT)))
8284
->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode))));
8385

84-
if ($expectAuthorizationCodeState) {
86+
if ($grantType === 'authorization_code') {
8587
$qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
88+
} else {
89+
// an unused (possibly expired) authorization code
90+
// must not be redeemed through the refresh token grant
91+
$qb->andWhere($qb->expr()->gt('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT)));
8692
}
8793

8894
return $qb->executeStatement();

‎apps/oauth2/tests/Controller/OauthApiControllerTest.php‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -425,7 +425,7 @@ public function testRefreshTokenValidAppToken(): void {
425425
'validrefresh',
426426
'random128',
427427
'newEncryptedToken',
428-
false,
428+
'refresh_token',
429429
)->willReturn(1);
430430

431431
$expected = new JSONResponse([
@@ -531,7 +531,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void {
531531
'validrefresh',
532532
'random128',
533533
'newEncryptedToken',
534-
false,
534+
'refresh_token',
535535
)->willReturn(1);
536536

537537
$expected = new JSONResponse([
@@ -640,7 +640,7 @@ public function testRefreshTokenExpiredAppToken(): void {
640640
'validrefresh',
641641
'random128',
642642
'newEncryptedToken',
643-
false,
643+
'refresh_token',
644644
)->willReturn(1);
645645

646646
$expected = new JSONResponse([
@@ -739,7 +739,7 @@ public function testRefreshTokenRedeemedConcurrently(): void {
739739
'validrefresh',
740740
'random128',
741741
'newEncryptedToken',
742-
false,
742+
'refresh_token',
743743
)->willReturn(0);
744744

745745
$this->throttler->expects($this->never())

‎apps/oauth2/tests/Db/AccessTokenMapperTest.php‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,4 +54,69 @@ public function testDeleteByClientId(): void {
5454
$this->accessTokenMapper->deleteByClientId(1234);
5555
$this->accessTokenMapper->getByCode('MyAwesomeToken');
5656
}
57+
58+
private function insertToken(int $clientId, int $tokenCount): AccessToken {
59+
$token = new AccessToken();
60+
$token->clientId = $clientId;
61+
$token->tokenId = time();
62+
$token->encryptedToken = 'MyEncryptedToken';
63+
$token->hashedCode = hash('sha512', 'MyAwesomeToken');
64+
$token->tokenCount = $tokenCount;
65+
$this->accessTokenMapper->insert($token);
66+
return $token;
67+
}
68+
69+
public function testRotateTokenInAuthorizationCodeState(): void {
70+
$this->accessTokenMapper->deleteByClientId(1235);
71+
$token = $this->insertToken(1235, 0);
72+
73+
$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code');
74+
$this->assertEquals(1, $rotated);
75+
76+
$row = $this->accessTokenMapper->getByCode('rotatedcode');
77+
$this->assertEquals('OtherEncryptedToken', $row->encryptedToken);
78+
$this->assertEquals(1, $row->tokenCount);
79+
80+
$this->accessTokenMapper->deleteByClientId(1235);
81+
}
82+
83+
public function testRotateTokenWithRefreshTokenGrantOnUnusedToken(): void {
84+
$this->accessTokenMapper->deleteByClientId(1235);
85+
$token = $this->insertToken(1235, 0);
86+
87+
// an unused authorization code is not a valid refresh token
88+
$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token');
89+
$this->assertEquals(0, $rotated);
90+
91+
$row = $this->accessTokenMapper->getByCode('MyAwesomeToken');
92+
$this->assertEquals('MyEncryptedToken', $row->encryptedToken);
93+
$this->assertEquals(0, $row->tokenCount);
94+
95+
$this->accessTokenMapper->deleteByClientId(1235);
96+
}
97+
98+
public function testRotateTokenInRefreshTokenState(): void {
99+
$this->accessTokenMapper->deleteByClientId(1235);
100+
$token = $this->insertToken(1235, 1);
101+
102+
$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token');
103+
$this->assertEquals(1, $rotated);
104+
105+
$row = $this->accessTokenMapper->getByCode('rotatedcode');
106+
$this->assertEquals('OtherEncryptedToken', $row->encryptedToken);
107+
$this->assertEquals(2, $row->tokenCount);
108+
109+
$this->accessTokenMapper->deleteByClientId(1235);
110+
}
111+
112+
public function testRotateTokenWithAuthorizationCodeGrantOnUsedToken(): void {
113+
$this->accessTokenMapper->deleteByClientId(1235);
114+
$token = $this->insertToken(1235, 1);
115+
116+
// an already used token can't be processed through the authorization code grant
117+
$rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code');
118+
$this->assertEquals(0, $rotated);
119+
120+
$this->accessTokenMapper->deleteByClientId(1235);
121+
}
57122
}

0 commit comments

Comments
 (0)