From 310b488ad9df2b3bbefd520262c9579b86885094 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 7 Apr 2026 12:26:54 +0200 Subject: [PATCH] fix: Reduce the mixups between apptokens and session ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/User/Session.php | 51 +++++++++++++++++++++++++++------- tests/lib/User/SessionTest.php | 29 +++++++++++-------- 2 files changed, 58 insertions(+), 22 deletions(-) diff --git a/lib/private/User/Session.php b/lib/private/User/Session.php index e7bfcf56407..68ae35da01f 100644 --- a/lib/private/User/Session.php +++ b/lib/private/User/Session.php @@ -389,8 +389,15 @@ class Session implements IUserSession, Emitter { } try { - $isTokenPassword = $this->isTokenPassword($password); - } catch (ExpiredTokenException $e) { + $dbToken = $this->getTokenFromPassword($password); + $isTokenPassword = $dbToken !== null; + if (($dbToken instanceof PublicKeyToken) + && ($dbToken->getType() !== IToken::PERMANENT_TOKEN) + ) { + // Refuse session tokens here, only app tokens are handled + return false; + } + } catch (ExpiredTokenException) { // Just return on an expired token no need to check further or record a failed login return false; } @@ -411,7 +418,6 @@ class Session implements IUserSession, Emitter { } if ($isTokenPassword) { - $dbToken = $this->tokenProvider->getToken($password); $userFromToken = $this->manager->get($dbToken->getUID()); $isValidEmailLogin = $userFromToken->getEMailAddress() === $user && $this->validateTokenLoginName($userFromToken->getEMailAddress(), $dbToken); @@ -501,6 +507,24 @@ class Session implements IUserSession, Emitter { } } + /** + * Check if the given 'password' is actually a device token + * + * @throws ExpiredTokenException + */ + private function getTokenFromPassword(string $password): ?\OCP\Authentication\Token\IToken { + try { + return $this->tokenProvider->getToken($password); + } catch (ExpiredTokenException $e) { + throw $e; + } catch (InvalidTokenException $ex) { + $this->logger->debug('Token is not valid: ' . $ex->getMessage(), [ + 'exception' => $ex, + ]); + return null; + } + } + protected function prepareUserLogin($firstTimeLogin, $refreshCsrfToken = true) { if ($refreshCsrfToken) { // TODO: mock/inject/use non-static @@ -807,6 +831,7 @@ class Session implements IUserSession, Emitter { */ public function tryTokenLogin(IRequest $request) { $authHeader = $request->getHeader('Authorization'); + $tokenFromCookie = false; if (str_starts_with($authHeader, 'Bearer ')) { $token = substr($authHeader, 7); } elseif ($request->getCookie($this->config->getSystemValueString('instanceid')) !== null) { @@ -814,6 +839,7 @@ class Session implements IUserSession, Emitter { // session and the request has a session cookie try { $token = $this->session->getId(); + $tokenFromCookie = true; } catch (SessionNotAvailableException $ex) { return false; } @@ -821,6 +847,18 @@ class Session implements IUserSession, Emitter { return false; } + try { + $dbToken = $this->tokenProvider->getToken($token); + } catch (InvalidTokenException $e) { + // Can't really happen but better safe than sorry + return false; + } + + if ($dbToken instanceof PublicKeyToken && $dbToken->getType() === IToken::TEMPORARY_TOKEN && !$tokenFromCookie) { + // Session token but from Bearer header, not allowed + return false; + } + if (!$this->loginWithToken($token)) { return false; } @@ -828,13 +866,6 @@ class Session implements IUserSession, Emitter { return false; } - try { - $dbToken = $this->tokenProvider->getToken($token); - } catch (InvalidTokenException $e) { - // Can't really happen but better save than sorry - return true; - } - // Set the session variable so we know this is an app password if ($dbToken instanceof PublicKeyToken && $dbToken->getType() === IToken::PERMANENT_TOKEN) { $this->session->set('app_password', $token); diff --git a/tests/lib/User/SessionTest.php b/tests/lib/User/SessionTest.php index 50c449559a0..3b92c7f78f3 100644 --- a/tests/lib/User/SessionTest.php +++ b/tests/lib/User/SessionTest.php @@ -481,16 +481,18 @@ class SessionTest extends \Test\TestCase { $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); + $token = $this->createMock(IToken::class); /** @var Session $userSession */ $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher]) - ->onlyMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); - $userSession->expects($this->once()) - ->method('isTokenPassword') - ->willReturn(true); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('I-AM-AN-APP-PASSWORD') + ->willReturn($token); $userSession->expects($this->once()) ->method('login') ->with('john', 'I-AM-AN-APP-PASSWORD') @@ -1230,16 +1232,18 @@ class SessionTest extends \Test\TestCase { $manager = $this->createMock(Manager::class); $session = $this->createMock(ISession::class); $request = $this->createMock(IRequest::class); + $token = $this->createMock(IToken::class); /** @var Session $userSession */ $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher]) - ->onlyMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); - $userSession->expects($this->once()) - ->method('isTokenPassword') - ->willReturn(true); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('I-AM-AN-PASSWORD') + ->willReturn($token); $userSession->expects($this->once()) ->method('login') ->with('john', 'I-AM-AN-PASSWORD') @@ -1280,12 +1284,13 @@ class SessionTest extends \Test\TestCase { /** @var Session $userSession */ $userSession = $this->getMockBuilder(Session::class) ->setConstructorArgs([$manager, $session, $this->timeFactory, $this->tokenProvider, $this->config, $this->random, $this->lockdownManager, $this->logger, $this->dispatcher]) - ->onlyMethods(['isTokenPassword', 'login', 'supportsCookies', 'createSessionToken', 'getUser']) + ->onlyMethods(['login', 'supportsCookies', 'createSessionToken', 'getUser']) ->getMock(); - $userSession->expects($this->once()) - ->method('isTokenPassword') - ->willReturn(false); + $this->tokenProvider->expects($this->once()) + ->method('getToken') + ->with('I-AM-AN-PASSWORD') + ->willThrowException(new InvalidTokenException()); $userSession->expects($this->once()) ->method('login') ->with('john@foo.bar', 'I-AM-AN-PASSWORD')