From 8b2640287c34d8bd642b0851a0474bb0d242fa59 Mon Sep 17 00:00:00 2001 From: Julien Veyssier Date: Thu, 29 Oct 2026 15:14:45 +0100 Subject: [PATCH] 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 --- .../lib/Controller/OauthApiController.php | 2 +- apps/oauth2/lib/Db/AccessTokenMapper.php | 14 ++-- .../Controller/OauthApiControllerTest.php | 8 +-- .../oauth2/tests/Db/AccessTokenMapperTest.php | 66 +++++++++++++++++++ 4 files changed, 81 insertions(+), 9 deletions(-) diff --git a/apps/oauth2/lib/Controller/OauthApiController.php b/apps/oauth2/lib/Controller/OauthApiController.php index 8dc05851cf801..4d9beffc606c1 100644 --- a/apps/oauth2/lib/Controller/OauthApiController.php +++ b/apps/oauth2/lib/Controller/OauthApiController.php @@ -227,7 +227,7 @@ public function getToken( $code, $newCode, $newEncryptedToken, - $grant_type === 'authorization_code', + $grant_type, ); if ($updatedRows !== 1) { diff --git a/apps/oauth2/lib/Db/AccessTokenMapper.php b/apps/oauth2/lib/Db/AccessTokenMapper.php index a5e407b3ed1ee..ed5ae3e51a5c5 100644 --- a/apps/oauth2/lib/Db/AccessTokenMapper.php +++ b/apps/oauth2/lib/Db/AccessTokenMapper.php @@ -66,12 +66,14 @@ public function cleanupExpiredAuthorizationCode(ITimeFactory $timeFactory): void } /** - * Rotate an access token only if it still matches the caller's previously-read state. + * Rotate an access token only if it still matches the caller's previously-read state + * and its token count matches the requested grant type. * - * @param bool $expectAuthorizationCodeState Require the token to still be unused + * @param 'authorization_code'|'refresh_token' $grantType Require the row to still be unused for 'authorization_code' + * or to have already delivered one token for 'refresh_token' * @return int Number of updated rows */ - public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, bool $expectAuthorizationCodeState): int { + public function rotateToken(int $id, string $oldCode, string $newCode, string $encryptedToken, string $grantType): int { $qb = $this->connection->getQueryBuilder(); $qb ->update($this->getTableName()) @@ -81,8 +83,12 @@ public function rotateToken(int $id, string $oldCode, string $newCode, string $e ->where($qb->expr()->eq('id', $qb->createNamedParameter($id, IQueryBuilder::PARAM_INT))) ->andWhere($qb->expr()->eq('hashed_code', $qb->createNamedParameter(hash('sha512', $oldCode)))); - if ($expectAuthorizationCodeState) { + if ($grantType === 'authorization_code') { $qb->andWhere($qb->expr()->eq('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); + } else { + // an unused (possibly expired) authorization code + // must not be redeemed through the refresh token grant + $qb->andWhere($qb->expr()->gt('token_count', $qb->createNamedParameter(0, IQueryBuilder::PARAM_INT))); } return $qb->executeStatement(); diff --git a/apps/oauth2/tests/Controller/OauthApiControllerTest.php b/apps/oauth2/tests/Controller/OauthApiControllerTest.php index 6efa02fe62b2e..c8d6696084a03 100644 --- a/apps/oauth2/tests/Controller/OauthApiControllerTest.php +++ b/apps/oauth2/tests/Controller/OauthApiControllerTest.php @@ -425,7 +425,7 @@ public function testRefreshTokenValidAppToken(): void { 'validrefresh', 'random128', 'newEncryptedToken', - false, + 'refresh_token', )->willReturn(1); $expected = new JSONResponse([ @@ -531,7 +531,7 @@ public function testRefreshTokenValidAppTokenBasicAuth(): void { 'validrefresh', 'random128', 'newEncryptedToken', - false, + 'refresh_token', )->willReturn(1); $expected = new JSONResponse([ @@ -640,7 +640,7 @@ public function testRefreshTokenExpiredAppToken(): void { 'validrefresh', 'random128', 'newEncryptedToken', - false, + 'refresh_token', )->willReturn(1); $expected = new JSONResponse([ @@ -739,7 +739,7 @@ public function testRefreshTokenRedeemedConcurrently(): void { 'validrefresh', 'random128', 'newEncryptedToken', - false, + 'refresh_token', )->willReturn(0); $this->throttler->expects($this->never()) diff --git a/apps/oauth2/tests/Db/AccessTokenMapperTest.php b/apps/oauth2/tests/Db/AccessTokenMapperTest.php index 8eecc597ba598..cafadeb0708ec 100644 --- a/apps/oauth2/tests/Db/AccessTokenMapperTest.php +++ b/apps/oauth2/tests/Db/AccessTokenMapperTest.php @@ -54,4 +54,70 @@ public function testDeleteByClientId(): void { $this->accessTokenMapper->deleteByClientId(1234); $this->accessTokenMapper->getByCode('MyAwesomeToken'); } + + private function insertToken(int $clientId, int $tokenCount): AccessToken { + $token = new AccessToken(); + $token->clientId = $clientId; + $token->tokenId = time(); + $token->encryptedToken = 'MyEncryptedToken'; + $token->hashedCode = hash('sha512', 'MyAwesomeToken'); + $token->tokenCount = $tokenCount; + + $this->accessTokenMapper->insert($token); + return $token; + } + + public function testRotateTokenInAuthorizationCodeState(): void { + $this->accessTokenMapper->deleteByClientId(1235); + $token = $this->insertToken(1235, 0); + + $rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code'); + $this->assertEquals(1, $rotated); + + $row = $this->accessTokenMapper->getByCode('rotatedcode'); + $this->assertEquals('OtherEncryptedToken', $row->encryptedToken); + $this->assertEquals(1, $row->tokenCount); + + $this->accessTokenMapper->deleteByClientId(1235); + } + + public function testRotateTokenWithRefreshTokenGrantOnUnusedToken(): void { + $this->accessTokenMapper->deleteByClientId(1235); + $token = $this->insertToken(1235, 0); + + // an unused authorization code is not a valid refresh token + $rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token'); + $this->assertEquals(0, $rotated); + + $row = $this->accessTokenMapper->getByCode('MyAwesomeToken'); + $this->assertEquals('MyEncryptedToken', $row->encryptedToken); + $this->assertEquals(0, $row->tokenCount); + + $this->accessTokenMapper->deleteByClientId(1235); + } + + public function testRotateTokenInRefreshTokenState(): void { + $this->accessTokenMapper->deleteByClientId(1235); + $token = $this->insertToken(1235, 1); + + $rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'refresh_token'); + $this->assertEquals(1, $rotated); + + $row = $this->accessTokenMapper->getByCode('rotatedcode'); + $this->assertEquals('OtherEncryptedToken', $row->encryptedToken); + $this->assertEquals(2, $row->tokenCount); + + $this->accessTokenMapper->deleteByClientId(1235); + } + + public function testRotateTokenWithAuthorizationCodeGrantOnUsedToken(): void { + $this->accessTokenMapper->deleteByClientId(1235); + $token = $this->insertToken(1235, 1); + + // an already used token can't be processed through the authorization code grant + $rotated = $this->accessTokenMapper->rotateToken($token->id, 'MyAwesomeToken', 'rotatedcode', 'OtherEncryptedToken', 'authorization_code'); + $this->assertEquals(0, $rotated); + + $this->accessTokenMapper->deleteByClientId(1235); + } }