From be03632d2cb1bc06e523c1c7b8d91281e9997544 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Thomas=20M=C3=BCller?= <323649642+oc-tmueller@users.noreply.github.com> Date: Thu, 3 Sep 2026 10:24:37 +0200 Subject: [PATCH] fix: treat the OAuth2 user id as opaque MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The token endpoint and the bearer auth module split the user id stored on an authorization code, access token or refresh token at the first colon and then resolved whatever user the remainder named. That is a leftover of the `login name:user id` format which only v0.5.0 - v0.5.2 ever wrote: e5b508c restored raw user id storage in 2022, so the parsers see a plain user id today, and a legacy pair value cannot reach them either - authorization codes expire after 10 minutes, access tokens after 1 hour, and refresh tokens were always stored with the already-split value. The parsing is therefore dead code, and a user id which legitimately contains a colon must not be reinterpreted. Treat the stored value as an opaque user id in all three places. The AuthModuleTest case which asserted the splitting behaviour is replaced by one asserting that the stored user id is used verbatim. Co-Authored-By: Claude Opus 5 Signed-off-by: Thomas Müller <323649642+oc-tmueller@users.noreply.github.com> --- lib/AuthModule.php | 6 +- lib/Controller/OAuthApiController.php | 10 --- tests/unit/AuthModuleTest.php | 25 +++---- .../Controller/OAuthApiControllerTest.php | 70 +++++++++++++++++++ 4 files changed, 82 insertions(+), 29 deletions(-) diff --git a/lib/AuthModule.php b/lib/AuthModule.php index 389f3bc9..f2ce9503 100644 --- a/lib/AuthModule.php +++ b/lib/AuthModule.php @@ -113,11 +113,7 @@ public function authToken($bearerToken): ?IUser { /** @var \OCP\IUserManager $userManager */ $userManager = $container->query('UserManager'); - $userId = $accessToken->getUserId(); - if (\strstr($userId, ':')) { - list(1 => $userId) = \explode(':', $userId, 2); - } - return $userManager->get($userId); + return $userManager->get($accessToken->getUserId()); } protected function tokenCanBeHandledByOpenIDConnect(): bool { diff --git a/lib/Controller/OAuthApiController.php b/lib/Controller/OAuthApiController.php index 13a8986c..c459f7b6 100644 --- a/lib/Controller/OAuthApiController.php +++ b/lib/Controller/OAuthApiController.php @@ -192,11 +192,6 @@ public function generateToken( $userId = $authorizationCode->getUserId(); - // strip off username if it exists - if (\strstr($userId, ':')) { - list(, $userId) = \explode(':', $userId, 2); - } - $this->authorizationCodeMapper->delete($authorizationCode); $userObj = $this->userManager->get($userId); @@ -235,11 +230,6 @@ public function generateToken( $userId = $refreshToken->getUserId(); - // strip off username if it exists - if (\strstr($userId, ':')) { - list(, $userId) = \explode(':', $userId, 2); - } - $userObj = $this->userManager->get($userId); if ($userObj === null || !$userObj->isEnabled()) { $this->logger->debug("the matching user is missing or disabled", ['app'=>__CLASS__]); diff --git a/tests/unit/AuthModuleTest.php b/tests/unit/AuthModuleTest.php index 3d35a2cc..f347b95b 100755 --- a/tests/unit/AuthModuleTest.php +++ b/tests/unit/AuthModuleTest.php @@ -37,9 +37,6 @@ class AuthModuleTest extends TestCase { /** @var String $userId */ private $userId = 'john'; - /** @var String $userIdConcat */ - private $userIdConcat = 'John Doe:john'; - /** @var ClientMapper $clientMapper */ private $clientMapper; @@ -118,18 +115,18 @@ public function testAuth() { $user = $this->authModule->auth($request); $this->assertNotNull($user); $this->assertEquals($this->userId, $user->getUID()); + } - // Valid request with ConcatUserID - $request = $this->getMockBuilder(IRequest::class)->getMock(); - $this->accessToken->setUserId($this->userIdConcat); - $this->accessToken = $this->accessTokenMapper->update($this->accessToken); - $request->expects($this->once()) - ->method('getHeader') - ->with($this->equalTo('Authorization')) - ->will($this->returnValue('Bearer ' . $this->accessToken->getToken())); - $user = $this->authModule->auth($request); - $this->assertNotNull($user); - $this->assertEquals($this->userId, $user->getUID()); + /** + * The user id stored on the token is opaque - a colon in it must not be + * treated as a "login name:user id" separator, otherwise the token + * authenticates a different account than the one it was issued for. + */ + public function testAuthTokenKeepsUserIdWithColon() { + $this->accessToken->setUserId('attacker:' . $this->userId); + $this->accessTokenMapper->update($this->accessToken); + + $this->assertNull($this->authModule->authToken($this->accessToken->getToken())); } /** diff --git a/tests/unit/Controller/OAuthApiControllerTest.php b/tests/unit/Controller/OAuthApiControllerTest.php index 5e36b202..ea2da0b7 100755 --- a/tests/unit/Controller/OAuthApiControllerTest.php +++ b/tests/unit/Controller/OAuthApiControllerTest.php @@ -625,6 +625,76 @@ public function testGenerateTokenWithAuthorizationCodeAndPKCES256() { $this->assertNotEmpty($json->token_type); $this->assertEquals('Bearer', $json->token_type); } + /** + * The stored user id is opaque - a colon in it must not be treated as a + * "login name:user id" separator, otherwise the token is issued for a + * different account than the one which authorized the code. + */ + public function testGenerateTokenWithAuthorizationCodeKeepsUserIdWithColon() { + $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; + $_SERVER['PHP_AUTH_PW'] = $this->clientSecret; + + $userIdWithColon = 'attacker:' . $this->userId; + $this->authorizationCode->setUserId($userIdWithColon); + $this->authorizationCodeMapper->update($this->authorizationCode); + + $requestedUserIds = []; + $this->mockUserManagerFor($userIdWithColon, $requestedUserIds); + + $result = $this->controller->generateToken( + 'authorization_code', + $this->authorizationCode->getCode(), + $this->redirectUri + ); + $this->assertEquals(200, $result->getStatus()); + $json = \json_decode($result->render()); + $this->assertEquals([$userIdWithColon], $requestedUserIds); + $this->assertEquals($userIdWithColon, $json->user_id); + $this->assertEquals($userIdWithColon, $this->accessTokenMapper->findByToken($json->access_token)->getUserId()); + $this->assertEquals($userIdWithColon, $this->refreshTokenMapper->findByToken($json->refresh_token)->getUserId()); + } + + /** + * @see testGenerateTokenWithAuthorizationCodeKeepsUserIdWithColon + */ + public function testGenerateTokenWithRefreshTokenKeepsUserIdWithColon() { + $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; + $_SERVER['PHP_AUTH_PW'] = $this->clientSecret; + + $userIdWithColon = 'attacker:' . $this->userId; + $this->refreshToken->setUserId($userIdWithColon); + $this->refreshTokenMapper->update($this->refreshToken); + + $requestedUserIds = []; + $this->mockUserManagerFor($userIdWithColon, $requestedUserIds); + + $result = $this->controller->generateToken('refresh_token', null, null, $this->refreshToken->getToken()); + $this->assertEquals(200, $result->getStatus()); + $json = \json_decode($result->render()); + $this->assertEquals([$userIdWithColon], $requestedUserIds); + $this->assertEquals($userIdWithColon, $json->user_id); + $this->assertEquals($userIdWithColon, $this->accessTokenMapper->findByToken($json->access_token)->getUserId()); + $this->assertEquals($userIdWithColon, $this->refreshTokenMapper->findByToken($json->refresh_token)->getUserId()); + } + + /** + * Lets the user manager resolve $userId only, and records every requested id. + * + * @param string $userId The only user id which resolves to an enabled user. + * @param array $requestedUserIds Collects the requested user ids. + * @return void + */ + private function mockUserManagerFor($userId, array &$requestedUserIds) { + $userMock = $this->createMock(IUser::class); + $userMock->method('isEnabled')->willReturn(true); + + $this->userManager->method('get') + ->willReturnCallback(function ($requestedUserId) use ($userId, $userMock, &$requestedUserIds) { + $requestedUserIds[] = $requestedUserId; + return $requestedUserId === $userId ? $userMock : null; + }); + } + public function testGenerateTokenWithAuthorizationCodeAndPKCESInvalidChallengeMethod() { $_SERVER['PHP_AUTH_USER'] = $this->clientIdentifier1; $_SERVER['PHP_AUTH_PW'] = $this->clientSecret;