From 56c73539d4a07c7ce687508bae858dd7880782dc Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Sun, 7 Jun 2026 15:49:02 +0200 Subject: [PATCH 1/2] fix(token): prevent reload loop from refresh-token rotation race Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Service/TokenService.php | 160 ++++++++++++- tests/unit/Service/TokenServiceTest.php | 304 ++++++++++++++++++++++++ 2 files changed, 452 insertions(+), 12 deletions(-) create mode 100644 tests/unit/Service/TokenServiceTest.php diff --git a/lib/Service/TokenService.php b/lib/Service/TokenService.php index 9da79c6f..31ff1e39 100644 --- a/lib/Service/TokenService.php +++ b/lib/Service/TokenService.php @@ -28,6 +28,8 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\Http\Client\IClient; use OCP\IAppConfig; +use OCP\ICache; +use OCP\ICacheFactory; use OCP\IConfig; use OCP\IRequest; use OCP\ISession; @@ -49,8 +51,14 @@ class TokenService { private const SESSION_TOKEN_KEY = Application::APP_ID . '-user-token'; private const REFRESH_LOCK_KEY = Application::APP_ID . '-lock-key'; + // Coordination keys for cross-request refresh deduplication (suffixed with the session id) + private const REFRESH_RESULT_CACHE_PREFIX = 'refresh-result_'; + private const REFRESH_THROTTLE_CACHE_PREFIX = 'refresh-throttle_'; + // Minimum delay between two refresh attempts of the same session (seconds) + private const REFRESH_THROTTLE_TTL = 30; private IClient $client; + private ICache $cache; public function __construct( public HttpClientHelper $clientService, @@ -69,12 +77,18 @@ public function __construct( private ProviderMapper $providerMapper, private ILockingProvider $lockingProvider, private ITimeFactory $timeFactory, + ICacheFactory $cacheFactory, ) { + $this->cache = $cacheFactory->createDistributed('user_oidc'); } public function storeToken(array $tokenData): Token { $token = new Token($tokenData, $this->timeFactory); $this->session->set(self::SESSION_TOKEN_KEY, json_encode($token, JSON_THROW_ON_ERROR)); + // Mirror the token into the distributed cache so concurrent requests (which may hold a + // stale in-memory session snapshot) can reuse it instead of refreshing again with an + // already-rotated refresh token. See refresh() and getToken(). + $this->cacheRefreshedToken($token); $this->logger->debug('[TokenService] Store token in the session', ['session_id' => $this->session->getId()]); return $token; } @@ -102,8 +116,8 @@ public function getToken(bool $refreshIfExpired = true, bool $refreshIfExpiring if (!$token->isExpired()) { // proactively refresh when past half the token lifetime to keep the IdP session alive if ($refreshIfExpiring && $token->isExpiring() && $token->getRefreshToken() !== null && !$token->refreshIsExpired()) { - $this->logger->debug('[TokenService] getToken: token is expiring, proactively refreshing to keep IdP session alive, expires in ' . strval($token->getExpiresInFromNow())); - return $this->refresh($token); + $this->logger->debug('[TokenService] getToken: token is expiring, refreshing to keep the IdP session alive, expires in ' . strval($token->getExpiresInFromNow())); + return $this->refreshOrAdopt($token, requireFresh: true); } $this->logger->debug('[TokenService] getToken: token is still valid, it expires in ' . strval($token->getExpiresInFromNow()) . ' and refresh expires in ' . strval($token->getRefreshExpiresInFromNow())); return $token; @@ -113,13 +127,40 @@ public function getToken(bool $refreshIfExpired = true, bool $refreshIfExpiring // try to refresh the token if there is a refresh token and it is still valid if ($refreshIfExpired && $token->getRefreshToken() !== null && !$token->refreshIsExpired()) { $this->logger->debug('[TokenService] getToken: token is expired and refresh token is still valid, refresh expires in ' . strval($token->getRefreshExpiresInFromNow())); - return $this->refresh($token); + return $this->refreshOrAdopt($token, requireFresh: false); } $this->logger->debug('[TokenService] getToken: return a token that has not been refreshed'); return $token; } + /** + * Satisfy a refresh request while protecting the IdP from redundant/racing calls: + * - if a concurrent or recent request already refreshed (visible in the shared cache), + * adopt that token instead of presenting our (possibly already-rotated) refresh token; + * - else, if a refresh was attempted very recently, skip and return the current token + * (throttle) so a dead refresh token or slow IdP is not hammered on every request; + * - else perform the refresh. + * + * @param Token $token the token currently stored in the session (expiring or expired) + * @param bool $requireFresh when true (token still valid, proactive keep-alive) only adopt a + * cached token that is not itself expiring; when false (token already expired) adopt + * any still-valid cached token, since even an expiring one is an upgrade + */ + private function refreshOrAdopt(Token $token, bool $requireFresh): Token { + $cachedToken = $this->getCachedRefreshedToken(); + if ($cachedToken !== null && !$cachedToken->isExpired() && (!$requireFresh || !$cachedToken->isExpiring())) { + $this->logger->debug('[TokenService] getToken: adopting token already refreshed by another request'); + return $this->storeToken($cachedToken->jsonSerialize()); + } + if ($this->isRefreshThrottled()) { + $this->logger->debug('[TokenService] getToken: refresh throttled, returning current token, expires in ' . strval($token->getExpiresInFromNow())); + return $token; + } + $this->markRefreshAttempt(); + return $this->refresh($token); + } + /** * Check to make sure the login token is still valid * @@ -169,9 +210,14 @@ public function checkLoginToken(): void { $token = $this->getToken(refreshIfExpired: true, refreshIfExpiring: true); if ($token === null) { $this->logger->debug('[TokenService] checkLoginToken: token is null'); - // if we don't have a token but we had one once, - // it means the session (where we store the token) has died - // so we need to reauthenticate + // if we don't have a token but we had one once, the session (where we store the + // token) has died. Only force re-authentication on a real top-level navigation; on + // background/XHR requests keep the Nextcloud session intact, otherwise logging out + // here would make the web UI force-reload the page and lose unsaved work (#1449). + if (!RequestClassificationService::isTopLevelHtmlNavigation($this->request)) { + $this->logger->debug('[TokenService] checkLoginToken: token is null on a non-navigation request, keeping the session'); + return; + } $this->logger->debug('[TokenService] checkLoginToken: token is null and user had_token_once -> logout'); $this->userSession->logout(); return; @@ -186,8 +232,12 @@ public function checkLoginToken(): void { public function reauthenticate(int $providerId) { if (!RequestClassificationService::isTopLevelHtmlNavigation($this->request)) { - $this->userSession->logout(); - $this->logger->debug('[TokenService] reauthenticate skipped: request is not a top-level HTML navigation', [ + // Do NOT terminate the Nextcloud session on background/XHR requests. Calling logout() + // here kills the session mid-work; the web UI then detects the dead session and + // force-reloads the page, discarding any unsaved work (#1449). The Nextcloud session + // is independent of the OIDC access-token validity, so we keep it and re-authenticate + // on the next top-level navigation instead. + $this->logger->debug('[TokenService] reauthenticate skipped: not a top-level HTML navigation, keeping the session', [ 'provider_id' => $providerId, 'request_uri' => $this->request->getRequestUri(), ]); @@ -241,6 +291,19 @@ public function refresh(Token $token): Token { // * while we were waiting for the lock (another request held it) // * OR in another process between the moment this process checked // the token expiration and the moment it attempted to acquire the lock + // + // Check the distributed cache first: unlike $this->session, it is shared across + // processes and is not subject to per-request in-memory snapshots, so it reliably + // surfaces a token just refreshed by a concurrent request even when the session + // backend does not lock concurrent requests (e.g. Redis/memcached sessions). + $cachedToken = $this->getCachedRefreshedToken(); + if ($cachedToken !== null && !$cachedToken->isExpired() && !$cachedToken->isExpiring()) { + $this->logger->debug('[TokenService] Token already refreshed by another request (cache)'); + return $this->storeToken($cachedToken->jsonSerialize()); + } + + // Fallback double-check against the in-session token (covers setups without a + // distributed cache, where concurrent same-session requests are serialized anyway). $sessionData = $this->session->get(self::SESSION_TOKEN_KEY); if ($sessionData) { $currentToken = new Token(json_decode($sessionData, true, 512, JSON_THROW_ON_ERROR), $this->timeFactory); @@ -250,8 +313,18 @@ public function refresh(Token $token): Token { } } - // Token still expired, proceed with refresh - $oidcProvider = $this->providerMapper->getProvider($token->getProviderId()); + // Refresh using the freshest refresh token we know about. If the cache holds a newer + // (still valid) token than the one we were called with, use its refresh token to avoid + // presenting a rotated/invalidated one to the IdP. + $baseToken = $token; + if ($cachedToken !== null && !$cachedToken->isExpired() + && $cachedToken->getRefreshToken() !== null + && $cachedToken->getCreatedAt() >= $baseToken->getCreatedAt()) { + $baseToken = $cachedToken; + } + + // Token still expired/expiring, proceed with refresh + $oidcProvider = $this->providerMapper->getProvider($baseToken->getProviderId()); $discovery = $this->discoveryService->obtainDiscovery($oidcProvider); $clientSecret = $oidcProvider->getClientSecret(); @@ -270,14 +343,14 @@ public function refresh(Token $token): Token { 'client_id' => $oidcProvider->getClientId(), 'client_secret' => $clientSecret, 'grant_type' => 'refresh_token', - 'refresh_token' => $token->getRefreshToken(), + 'refresh_token' => $baseToken->getRefreshToken(), ] ); $bodyArray = json_decode(trim($body), true, 512, JSON_THROW_ON_ERROR); $this->logger->debug('[TokenService] ---- Refresh token success'); return $this->storeToken( - array_merge($bodyArray, ['provider_id' => $token->getProviderId()]) + array_merge($bodyArray, ['provider_id' => $baseToken->getProviderId()]) ); } catch (\Exception $e) { $this->logger->error('[TokenService] Failed to refresh token', ['exception' => $e]); @@ -294,6 +367,69 @@ public function refresh(Token $token): Token { } } + /** + * Store the freshly refreshed token in the distributed cache, encrypted, so concurrent + * requests can reuse it instead of refreshing again. The entry lives for the remaining + * lifetime of the token (it is only useful while still valid). + */ + private function cacheRefreshedToken(Token $token): void { + try { + $ttl = max($token->getExpiresInFromNow(), self::REFRESH_THROTTLE_TTL); + $this->cache->set( + self::REFRESH_RESULT_CACHE_PREFIX . $this->session->getId(), + $this->crypto->encrypt(json_encode($token, JSON_THROW_ON_ERROR)), + $ttl, + ); + } catch (\Throwable $e) { + // Caching is a best-effort optimization; never let it break token handling + $this->logger->debug('[TokenService] Failed to cache refreshed token', ['exception' => $e]); + } + } + + /** + * Read the token most recently refreshed for this session from the distributed cache. + */ + private function getCachedRefreshedToken(): ?Token { + try { + $cached = $this->cache->get(self::REFRESH_RESULT_CACHE_PREFIX . $this->session->getId()); + if (!is_string($cached) || $cached === '') { + return null; + } + $json = $this->crypto->decrypt($cached); + return new Token(json_decode($json, true, 512, JSON_THROW_ON_ERROR), $this->timeFactory); + } catch (\Throwable $e) { + $this->logger->debug('[TokenService] Failed to read cached refreshed token', ['exception' => $e]); + return null; + } + } + + /** + * Whether a refresh has been attempted recently for this session. + */ + private function isRefreshThrottled(): bool { + try { + // read directly instead of hasKey() (deprecated to avoid TOCTOU); the marker value is irrelevant + return $this->cache->get(self::REFRESH_THROTTLE_CACHE_PREFIX . $this->session->getId()) !== null; + } catch (\Throwable $e) { + return false; + } + } + + /** + * Record that a refresh has just been attempted for this session. + */ + private function markRefreshAttempt(): void { + try { + $this->cache->set( + self::REFRESH_THROTTLE_CACHE_PREFIX . $this->session->getId(), + '1', + self::REFRESH_THROTTLE_TTL, + ); + } catch (\Throwable $e) { + // best-effort throttle, ignore failures + } + } + public function decodeIdToken(Token $token): array { $provider = $this->providerMapper->getProvider($token->getProviderId()); $jwks = $this->discoveryService->obtainJWK($provider, $token->getIdToken()); diff --git a/tests/unit/Service/TokenServiceTest.php b/tests/unit/Service/TokenServiceTest.php new file mode 100644 index 00000000..da7dc9dd --- /dev/null +++ b/tests/unit/Service/TokenServiceTest.php @@ -0,0 +1,304 @@ +clientService = $this->createMock(HttpClientHelper::class); + $this->session = $this->createMock(ISession::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->tokenProvider = $this->createMock(IProvider::class); + $this->config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->crypto = $this->createMock(ICrypto::class); + $this->request = $this->createMock(IRequest::class); + $this->urlGenerator = $this->createMock(IURLGenerator::class); + $this->eventDispatcher = $this->createMock(IEventDispatcher::class); + $this->appManager = $this->createMock(IAppManager::class); + $this->discoveryService = $this->createMock(DiscoveryService::class); + $this->providerMapper = $this->createMock(ProviderMapper::class); + $this->lockingProvider = $this->createMock(ILockingProvider::class); + $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->cacheFactory = $this->createMock(ICacheFactory::class); + $this->cache = $this->createMock(ICache::class); + + $this->timeFactory->method('getTime')->willReturn(self::NOW); + $this->session->method('getId')->willReturn('sid'); + $this->cacheFactory->method('createDistributed')->willReturn($this->cache); + // transforming stub so tests can prove a value went through encryption before being cached + $this->crypto->method('encrypt')->willReturnCallback(fn (string $plaintext): string => 'enc:' . $plaintext); + + $this->tokenService = new TokenService( + $this->clientService, + $this->session, + $this->userSession, + $this->tokenProvider, + $this->config, + $this->appConfig, + $this->logger, + $this->crypto, + $this->request, + $this->urlGenerator, + $this->eventDispatcher, + $this->appManager, + $this->discoveryService, + $this->providerMapper, + $this->lockingProvider, + $this->timeFactory, + $this->cacheFactory, + ); + } + + /** + * Build a serialized token as it is stored in the session / cache. + */ + private function tokenJson( + string $accessToken, + int $createdAt, + int $expiresIn = 300, + ?string $refreshToken = 'refresh-token', + ?int $providerId = 1, + ?int $refreshExpiresIn = null, + ): string { + return json_encode([ + 'id_token' => null, + 'access_token' => $accessToken, + 'expires_in' => $expiresIn, + 'refresh_expires_in' => $refreshExpiresIn, + 'refresh_token' => $refreshToken, + 'created_at' => $createdAt, + 'provider_id' => $providerId, + ], JSON_THROW_ON_ERROR); + } + + /** + * Fix 1: when another request has already refreshed the token (visible via the shared + * cache), getToken() must adopt that result instead of refreshing again — so it never + * presents the already-rotated refresh token to the IdP. + */ + public function testGetTokenAdoptsCachedRefreshedTokenWithoutContactingIdp(): void { + // session holds a still-valid but expiring token (created at 800, half-life passed at 950, now 1000) + $this->session->method('get')->willReturn($this->tokenJson('old-access', 800)); + // the shared cache holds a fresh token refreshed by a concurrent request + $this->cache->method('get')->willReturn('encrypted'); + $this->crypto->method('decrypt')->willReturn($this->tokenJson('fresh-access', self::NOW)); + + // the IdP must NOT be contacted + $this->clientService->expects($this->never())->method('post'); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('fresh-access', $result->getAccessToken()); + } + + /** + * Fix 2: while a proactive refresh was attempted recently (throttle marker present) and no + * fresh token is cached, getToken() must return the current still-valid token unchanged + * instead of hammering the IdP on every request. + */ + public function testGetTokenThrottlesProactiveRefresh(): void { + $this->session->method('get')->willReturn($this->tokenJson('old-access', 800)); + $this->cache->method('get')->willReturnCallback(function (string $key) { + // no fresh token cached, but a proactive refresh was attempted recently + return $key === self::REFRESH_THROTTLE_CACHE_KEY ? '1' : null; + }); + + $this->clientService->expects($this->never())->method('post'); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('old-access', $result->getAccessToken()); + } + + /** + * When not throttled and nothing is cached, the proactive refresh must still happen: the IdP + * is contacted once, the throttle marker is recorded, and the new token is returned. + */ + public function testGetTokenProactivelyRefreshesAndRecordsAttempt(): void { + $this->session->method('get')->willReturn($this->tokenJson('old-access', 800)); + // nothing cached and not throttled (every cache read misses) + $this->cache->method('get')->willReturn(null); + + $setEntries = []; + $this->cache->method('set')->willReturnCallback(function ($key, $value, $ttl) use (&$setEntries): bool { + $setEntries[$key] = $value; + return true; + }); + + $provider = new Provider(); + $provider->setClientId('client-id'); + $provider->setClientSecret(''); + $this->providerMapper->method('getProvider')->willReturn($provider); + $this->discoveryService->method('obtainDiscovery')->willReturn(['token_endpoint' => 'https://idp.example/token']); + + $this->clientService->expects($this->once()) + ->method('post') + ->willReturn($this->tokenJson('refreshed-access', self::NOW, refreshToken: 'rotated-refresh-token')); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('refreshed-access', $result->getAccessToken()); + // a proactive refresh attempt must be recorded for throttling + Assert::assertArrayHasKey(self::REFRESH_THROTTLE_CACHE_KEY, $setEntries); + // the refreshed token must be encrypted, then mirrored into the shared cache + Assert::assertArrayHasKey(self::REFRESH_RESULT_CACHE_KEY, $setEntries); + $cachedBlob = $setEntries[self::REFRESH_RESULT_CACHE_KEY]; + Assert::assertStringStartsWith('enc:', $cachedBlob, 'the cached token must go through ICrypto::encrypt'); + $cachedToken = json_decode(substr($cachedBlob, 4), true, 512, JSON_THROW_ON_ERROR); + Assert::assertSame('refreshed-access', $cachedToken['access_token']); + Assert::assertSame('rotated-refresh-token', $cachedToken['refresh_token']); + } + + /** + * Fix 1 (concurrency core): a request that reaches refresh() after losing the race must, once + * it holds the lock, detect the token already refreshed by the winner via the shared cache and + * reuse it — never POSTing a rotated refresh token to the IdP. + */ + public function testRefreshReusesTokenAlreadyRefreshedByConcurrentRequest(): void { + $this->cache->method('get')->willReturn('encrypted'); + $this->crypto->method('decrypt')->willReturn($this->tokenJson('fresh-access', self::NOW)); + + $this->clientService->expects($this->never())->method('post'); + + $staleToken = new Token(json_decode($this->tokenJson('old-access', 800), true, 512, JSON_THROW_ON_ERROR), $this->timeFactory); + $result = $this->tokenService->refresh($staleToken); + + Assert::assertSame('fresh-access', $result->getAccessToken()); + } + + /** + * #1449: a background/XHR request whose token cannot be refreshed must NOT terminate the + * Nextcloud session — doing so makes the web UI detect a dead session and force-reload the + * page, losing unsaved work. Only a real top-level navigation may log out and redirect. + */ + public function testReauthenticateKeepsSessionOnBackgroundRequest(): void { + // not a top-level HTML navigation (e.g. a notifications / heartbeat poll) + $this->request->method('getMethod')->willReturn('POST'); + + $this->userSession->expects($this->never())->method('logout'); + + $this->tokenService->reauthenticate(1); + } + + /** + * Expired-token path: if another request already refreshed (visible in the shared cache), + * adopt it without contacting the IdP — recovering the session instead of bouncing the user. + */ + public function testGetTokenAdoptsCachedTokenWhenSessionTokenExpired(): void { + // session token fully expired (created at 600, expired at 900, now 1000) + $this->session->method('get')->willReturn($this->tokenJson('old-access', 600)); + $this->cache->method('get')->willReturn('encrypted'); + $this->crypto->method('decrypt')->willReturn($this->tokenJson('fresh-access', self::NOW)); + + $this->clientService->expects($this->never())->method('post'); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('fresh-access', $result->getAccessToken()); + } + + /** + * Expired-token path: a refresh attempted very recently is throttled, so a dead refresh token + * is not POSTed to the IdP on every background request. The (still-expired) token is returned + * and the caller keeps the session alive rather than bouncing the user. + */ + public function testGetTokenThrottlesRefreshOfExpiredToken(): void { + $this->session->method('get')->willReturn($this->tokenJson('old-access', 600)); + $this->cache->method('get')->willReturnCallback(function (string $key) { + // nothing cached to adopt, but a refresh was attempted recently + return $key === self::REFRESH_THROTTLE_CACHE_KEY ? '1' : null; + }); + + $this->clientService->expects($this->never())->method('post'); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('old-access', $result->getAccessToken()); + } +} From f97e9a4ee0e0cfaa828f341d59ec19ad76e9c4b1 Mon Sep 17 00:00:00 2001 From: Git'Fellow <12234510+solracsf@users.noreply.github.com> Date: Thu, 11 Jun 2026 08:37:01 +0200 Subject: [PATCH 2/2] fix(token): classify navigations via Fetch Metadata, not XHR-header absence Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com> --- lib/Controller/LoginController.php | 10 ++ lib/Service/RequestClassificationService.php | 35 ++++++- lib/Service/TokenService.php | 18 +++- .../RequestClassificationServiceTest.php | 95 ++++++++++++++++++- tests/unit/Service/TokenServiceTest.php | 27 ++++++ 5 files changed, 179 insertions(+), 6 deletions(-) diff --git a/lib/Controller/LoginController.php b/lib/Controller/LoginController.php index 6abda947..8d9ec1df 100644 --- a/lib/Controller/LoginController.php +++ b/lib/Controller/LoginController.php @@ -715,6 +715,16 @@ public function code(string $state = '', string $code = '', string $scope = '', ['provider_id' => $providerId], ); $this->tokenService->storeToken($tokenData); + if (($data['refresh_token'] ?? null) === null) { + $this->logger->warning( + 'store_login_token is enabled but the identity provider did not return a refresh token.' + . ' The login token cannot be renewed, so session keep-alive will not work and users will be' + . ' re-authenticated on the next page navigation after the token expires.' + . ' Depending on the provider, issuing refresh tokens may require requesting the offline_access' + . ' scope (e.g. Authentik, Microsoft Entra ID, Okta) or enabling refresh tokens for the client.', + ['provider_id' => $providerId] + ); + } } $this->config->setUserValue($user->getUID(), Application::APP_ID, 'had_token_once', '1'); diff --git a/lib/Service/RequestClassificationService.php b/lib/Service/RequestClassificationService.php index b9c39ac0..b9acebec 100644 --- a/lib/Service/RequestClassificationService.php +++ b/lib/Service/RequestClassificationService.php @@ -12,11 +12,44 @@ use OCP\IRequest; class RequestClassificationService { + /** + * Whether the request is a real top-level browser page navigation (typing a URL, clicking a + * link, reloading) as opposed to a background fetch()/XHR (notifications polling, heartbeats, + * dashboard widgets, …). + * + * This gates the disruptive logout+redirect to the OIDC login flow, so it MUST NOT + * misclassify a background request as a navigation: doing so terminates the session and makes + * the web UI reload the page, losing unsaved work (see #1449). Relying on the absence of + * X-Requested-With / OCS-apirequest is not enough — Nextcloud's background fetcher and various + * fetch()-based polls do not set those headers. + */ public static function isTopLevelHtmlNavigation(IRequest $request): bool { if (strtoupper($request->getMethod()) !== 'GET') { return false; } + // Speculative loads (prefetch/prerender) carry navigation-like Fetch Metadata but are not + // user-visible navigations; triggering the logout+redirect from one would invisibly + // terminate the session. Sec-Purpose is the standard marker, Purpose the legacy one. + if (stripos($request->getHeader('Sec-Purpose') . $request->getHeader('Purpose'), 'prefetch') !== false + || stripos($request->getHeader('Sec-Purpose'), 'prerender') !== false) { + return false; + } + + // Fetch Metadata (https://www.w3.org/TR/fetch-metadata/): modern browsers send these on + // every request. Only a real top-level navigation produces Sec-Fetch-Mode: navigate + // together with Sec-Fetch-Dest: document; background fetch()/XHR send + // Sec-Fetch-Mode: cors|same-origin|no-cors and Sec-Fetch-Dest: empty. + // When the client sends Fetch Metadata, trust it. + $secFetchMode = $request->getHeader('Sec-Fetch-Mode'); + $secFetchDest = $request->getHeader('Sec-Fetch-Dest'); + if ($secFetchMode !== '' || $secFetchDest !== '') { + return $secFetchMode === 'navigate' && $secFetchDest === 'document'; + } + + // Fallback for clients without Fetch Metadata (older browsers, some non-browser clients): + // exclude known API/XHR markers and require an HTML Accept header, since browser document + // navigations always send "Accept: text/html, …" while JSON polls do not. if ($request->getHeader('OCS-apirequest') !== '') { return false; } @@ -25,6 +58,6 @@ public static function isTopLevelHtmlNavigation(IRequest $request): bool { return false; } - return true; + return str_contains(strtolower($request->getHeader('Accept')), 'text/html'); } } diff --git a/lib/Service/TokenService.php b/lib/Service/TokenService.php index 31ff1e39..6465e7d0 100644 --- a/lib/Service/TokenService.php +++ b/lib/Service/TokenService.php @@ -142,10 +142,12 @@ public function getToken(bool $refreshIfExpired = true, bool $refreshIfExpiring * (throttle) so a dead refresh token or slow IdP is not hammered on every request; * - else perform the refresh. * + * When $requireFresh is true (token still valid, proactive keep-alive) only a cached token + * that is not itself expiring is adopted; when false (token already expired) any still-valid + * cached token is adopted, since even an expiring one is an upgrade. + * * @param Token $token the token currently stored in the session (expiring or expired) - * @param bool $requireFresh when true (token still valid, proactive keep-alive) only adopt a - * cached token that is not itself expiring; when false (token already expired) adopt - * any still-valid cached token, since even an expiring one is an upgrade + * @param bool $requireFresh whether only a still-fresh (non-expiring) cached token may be adopted */ private function refreshOrAdopt(Token $token, bool $requireFresh): Token { $cachedToken = $this->getCachedRefreshedToken(); @@ -348,6 +350,16 @@ public function refresh(Token $token): Token { ); $bodyArray = json_decode(trim($body), true, 512, JSON_THROW_ON_ERROR); + // RFC 6749 section 6: the provider MAY omit the refresh token in a refresh response, + // in which case the previous refresh token remains valid. Carry it over so the session + // does not silently lose the ability to refresh (rotating providers always send a new one). + if (($bodyArray['refresh_token'] ?? null) === null && $baseToken->getRefreshToken() !== null) { + $bodyArray['refresh_token'] = $baseToken->getRefreshToken(); + if (!isset($bodyArray['refresh_expires_in']) && $baseToken->getRefreshExpiresIn() !== null) { + // preserve the remaining validity of the carried-over refresh token + $bodyArray['refresh_expires_in'] = max(0, $baseToken->getRefreshExpiresInFromNow()); + } + } $this->logger->debug('[TokenService] ---- Refresh token success'); return $this->storeToken( array_merge($bodyArray, ['provider_id' => $baseToken->getProviderId()]) diff --git a/tests/unit/Service/RequestClassificationServiceTest.php b/tests/unit/Service/RequestClassificationServiceTest.php index 82df5f04..d93cd254 100644 --- a/tests/unit/Service/RequestClassificationServiceTest.php +++ b/tests/unit/Service/RequestClassificationServiceTest.php @@ -28,11 +28,96 @@ public function testIsTopLevelHtmlNavigation(string $method, array $headers, boo public static function topLevelHtmlNavigationProvider(): array { return [ - 'top level navigation' => [ + // Real top-level navigation as sent by a modern browser + 'navigation via fetch metadata' => [ 'GET', - [], + [ + 'Sec-Fetch-Mode' => 'navigate', + 'Sec-Fetch-Dest' => 'document', + 'Accept' => 'text/html,application/xhtml+xml', + ], + true, + ], + // Background fetch()/XHR from a modern browser (e.g. dashboard widget) + 'cors fetch via fetch metadata' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'cors', + 'Sec-Fetch-Dest' => 'empty', + ], + false, + ], + 'same-origin fetch via fetch metadata' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'same-origin', + 'Sec-Fetch-Dest' => 'empty', + ], + false, + ], + // The exact #1449 regression: notifications/logreader background polls that do NOT set + // OCS-apirequest / X-Requested-With but DO send fetch metadata must not look like a navigation + 'background poll without xhr markers' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'cors', + 'Sec-Fetch-Dest' => 'empty', + 'Accept' => 'application/json, text/plain, */*', + ], + false, + ], + // navigate mode but not a document destination (e.g. iframe/object) -> do not redirect + 'navigate mode non-document dest' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'navigate', + 'Sec-Fetch-Dest' => 'iframe', + ], + false, + ], + // Speculative loads look like navigations but must never trigger logout+redirect + 'prefetch with navigation metadata' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'navigate', + 'Sec-Fetch-Dest' => 'document', + 'Sec-Purpose' => 'prefetch', + ], + false, + ], + 'prerender with navigation metadata' => [ + 'GET', + [ + 'Sec-Fetch-Mode' => 'navigate', + 'Sec-Fetch-Dest' => 'document', + 'Sec-Purpose' => 'prefetch;prerender', + ], + false, + ], + 'legacy purpose prefetch' => [ + 'GET', + [ + 'Purpose' => 'prefetch', + 'Accept' => 'text/html,application/xhtml+xml', + ], + false, + ], + // Fallback path (no fetch metadata): legacy browser document navigation + 'legacy navigation via accept html' => [ + 'GET', + [ + 'Accept' => 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8', + ], true, ], + // Fallback path: legacy JSON poll without fetch metadata + 'legacy json poll' => [ + 'GET', + [ + 'Accept' => 'application/json, text/plain, */*', + ], + false, + ], 'xhr request' => [ 'GET', [ @@ -47,6 +132,12 @@ public static function topLevelHtmlNavigationProvider(): array { ], false, ], + // Header-less GET (curl, health check, ...) is not a browser navigation + 'bare get without headers' => [ + 'GET', + [], + false, + ], 'non get request' => [ 'POST', [], diff --git a/tests/unit/Service/TokenServiceTest.php b/tests/unit/Service/TokenServiceTest.php index da7dc9dd..8580148a 100644 --- a/tests/unit/Service/TokenServiceTest.php +++ b/tests/unit/Service/TokenServiceTest.php @@ -250,6 +250,33 @@ public function testRefreshReusesTokenAlreadyRefreshedByConcurrentRequest(): voi Assert::assertSame('fresh-access', $result->getAccessToken()); } + /** + * RFC 6749 section 6: a provider MAY omit the refresh token in a refresh response, in which + * case the previous refresh token remains valid. It must be carried over into the stored + * token, otherwise non-rotating providers lose refreshability after the first refresh. + */ + public function testRefreshKeepsPreviousRefreshTokenWhenResponseOmitsIt(): void { + $this->session->method('get')->willReturn($this->tokenJson('old-access', 800, refreshToken: 'original-refresh-token')); + $this->cache->method('get')->willReturn(null); + + $provider = new Provider(); + $provider->setClientId('client-id'); + $provider->setClientSecret(''); + $this->providerMapper->method('getProvider')->willReturn($provider); + $this->discoveryService->method('obtainDiscovery')->willReturn(['token_endpoint' => 'https://idp.example/token']); + + // the refresh response contains no refresh_token (non-rotating provider) + $this->clientService->expects($this->once()) + ->method('post') + ->willReturn($this->tokenJson('refreshed-access', self::NOW, refreshToken: null)); + + $result = $this->tokenService->getToken(refreshIfExpired: true, refreshIfExpiring: true); + + Assert::assertNotNull($result); + Assert::assertSame('refreshed-access', $result->getAccessToken()); + Assert::assertSame('original-refresh-token', $result->getRefreshToken(), 'the previous refresh token must be kept when the response omits one'); + } + /** * #1449: a background/XHR request whose token cannot be refreshed must NOT terminate the * Nextcloud session — doing so makes the web UI detect a dead session and force-reload the