From 845457f0012cafdb55fc73d7664487818c6ed5cd Mon Sep 17 00:00:00 2001 From: Alex DiMarco Date: Fri, 24 Jul 2026 00:07:45 -0400 Subject: [PATCH 1/3] fix(imap): surface authentication failures to the client and stop the sync retry loop When IMAP rejects an account's credentials the Horde exception used to bubble up to ErrorMiddleware as a generic HTTP 500 that the web client cannot distinguish from any other failure. The client therefore kept polling mailbox sync every 30 seconds forever, flooding the server log at error level (including the client-side auth rate limiter's 'Too many auth attempts'), while the user got no hint that their credentials were wrong. Backend: - Add ImapAuthenticationFailedException (ClientException, HTTP 424) carrying an AUTHENTICATION_FAILED/RATE_LIMITED reason - Translate any Horde LOGIN_AUTHENTICATIONFAILED anywhere in an uncaught exception chain centrally in ErrorMiddleware: return the typed jsend fail payload and log once at info instead of error - Add the missing #[TrapError] to MessagesController::getDkim so its failures take the same path instead of the global exception handler - Wrap the unguarded $client->login() in ImapToDbSynchronizer::sync in a ServiceException; detect auth failures anywhere in the chain in SyncJob so cron logs them at info level too Frontend: - Map the new exception type in convert.js and attach the structured payload to converted errors - On auth failure flag the account (reusing the existing account.error banner in Navigation.vue), show a persistent toast naming the account, and skip flagged accounts in the background sync loops; IMAP-backed background lookups (DKIM, itineraries, smart replies) are skipped too while the account cannot authenticate - Probe a flagged account on a schedule and clear the flag on the first successful sync so transient failures heal automatically - Fail initial syncs loudly (individual mailboxes and unified/priority fan-out) so Mailbox.vue's initializeCache cannot loop requesting envelopes that can never be cached; return the promise chain from initializeCache so failures reach the error UI, and show an actionable message instead of 'Not found' in the thread view Fixes #7279 Fixes #12136 Assisted-by: Claude:claude-fable-5 Signed-off-by: Alex DiMarco --- lib/BackgroundJob/SyncJob.php | 7 +- lib/Controller/MessagesController.php | 1 + .../ImapAuthenticationFailedException.php | 65 +++++++++++++ lib/Http/Middleware/ErrorMiddleware.php | 18 ++++ lib/Service/Sync/ImapToDbSynchronizer.php | 10 +- src/components/Mailbox.vue | 2 +- src/components/ThreadEnvelope.vue | 18 +++- src/errors/ImapAuthenticationFailedError.js | 16 ++++ src/errors/convert.js | 7 +- src/store/mainStore/actions.js | 95 ++++++++++++++++++- .../Http/Middleware/ErrorMiddlewareTest.php | 51 ++++++++++ 11 files changed, 273 insertions(+), 17 deletions(-) create mode 100644 lib/Exception/ImapAuthenticationFailedException.php create mode 100644 src/errors/ImapAuthenticationFailedError.js diff --git a/lib/BackgroundJob/SyncJob.php b/lib/BackgroundJob/SyncJob.php index e3c1676271..260b6cf66f 100644 --- a/lib/BackgroundJob/SyncJob.php +++ b/lib/BackgroundJob/SyncJob.php @@ -8,10 +8,9 @@ namespace OCA\Mail\BackgroundJob; -use Horde_Imap_Client_Exception; use OCA\Mail\AppInfo\Application; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\IncompleteSyncException; -use OCA\Mail\Exception\ServiceException; use OCA\Mail\IMAP\MailboxSync; use OCA\Mail\Service\AccountService; use OCA\Mail\Service\Sync\ImapToDbSynchronizer; @@ -123,9 +122,7 @@ protected function run($argument) { 'exception' => $e, ]); } catch (Throwable $e) { - if ($e instanceof ServiceException - && $e->getPrevious() instanceof Horde_Imap_Client_Exception - && $e->getPrevious()->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED) { + if (ImapAuthenticationFailedException::findHordeAuthFailure($e) !== null) { $this->logger->info('Cron mail sync authentication failed for account {accountId}', [ 'accountId' => $accountId, 'exception' => $e, diff --git a/lib/Controller/MessagesController.php b/lib/Controller/MessagesController.php index 022964f703..60384e978b 100755 --- a/lib/Controller/MessagesController.php +++ b/lib/Controller/MessagesController.php @@ -297,6 +297,7 @@ public function getItineraries(int $id): JSONResponse { * @param int $id * @return JSONResponse */ + #[TrapError] public function getDkim(int $id): JSONResponse { if ($this->userId === null) { return new JSONResponse([], Http::STATUS_UNAUTHORIZED); diff --git a/lib/Exception/ImapAuthenticationFailedException.php b/lib/Exception/ImapAuthenticationFailedException.php new file mode 100644 index 0000000000..af7c699fdf --- /dev/null +++ b/lib/Exception/ImapAuthenticationFailedException.php @@ -0,0 +1,65 @@ +reason = $reason; + } + + public static function fromHorde(Horde_Imap_Client_Exception $e): self { + $reason = $e->getMessage() === 'Too many auth attempts' + ? self::REASON_RATE_LIMITED + : self::REASON_AUTHENTICATION_FAILED; + return new self('IMAP authentication failed', (int)$e->getCode(), $e, $reason); + } + + /** + * Find an IMAP authentication failure anywhere in an exception chain + */ + public static function findHordeAuthFailure(?Throwable $e): ?Horde_Imap_Client_Exception { + while ($e !== null) { + if ($e instanceof Horde_Imap_Client_Exception + && $e->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED) { + return $e; + } + $e = $e->getPrevious(); + } + return null; + } + + public function getReason(): string { + return $this->reason; + } + + #[\Override] + public function getHttpCode(): int { + return Http::STATUS_FAILED_DEPENDENCY; + } +} diff --git a/lib/Http/Middleware/ErrorMiddleware.php b/lib/Http/Middleware/ErrorMiddleware.php index 8e0999759d..6e507effed 100644 --- a/lib/Http/Middleware/ErrorMiddleware.php +++ b/lib/Http/Middleware/ErrorMiddleware.php @@ -12,6 +12,7 @@ use Exception; use Horde_Imap_Client_Exception; use OCA\Mail\Exception\ClientException; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\NotImplemented; use OCA\Mail\Exception\ServiceException; use OCA\Mail\Http\JsonResponse; @@ -69,6 +70,23 @@ public function afterException($controller, $methodName, Exception $exception) { return JSONResponse::fail([], Http::STATUS_NOT_IMPLEMENTED); } + $authFailure = ImapAuthenticationFailedException::findHordeAuthFailure($exception); + if ($authFailure !== null) { + $wrapped = ImapAuthenticationFailedException::fromHorde($authFailure); + // The client is informed and can react, no need to flood the log + $this->logger->info('IMAP authentication failed: ' . $authFailure->getMessage(), [ + 'exception' => $exception, + ]); + return JsonResponse::fail( + [ + 'message' => $wrapped->getMessage(), + 'type' => get_class($wrapped), + 'reason' => $wrapped->getReason(), + ], + $wrapped->getHttpCode() + ); + } + $temporary = $this->isTemporaryException($exception); if ($temporary) { $this->logger->warning($exception->getMessage(), [ diff --git a/lib/Service/Sync/ImapToDbSynchronizer.php b/lib/Service/Sync/ImapToDbSynchronizer.php index 2cf929bc43..1065c8be43 100644 --- a/lib/Service/Sync/ImapToDbSynchronizer.php +++ b/lib/Service/Sync/ImapToDbSynchronizer.php @@ -186,7 +186,15 @@ public function sync(Account $account, return $rebuildThreads; } - $client->login(); // Need to login before fetching capabilities. + try { + $client->login(); // Need to login before fetching capabilities. + } catch (Horde_Imap_Client_Exception $e) { + throw new ServiceException( + 'Sync failed for account ' . $account->getId() . ' mailbox ' . $mailbox->getId() . ': ' . $e->getMessage(), + 0, + $e, + ); + } // There is no partial sync when using QRESYNC. As per RFC the client will always pull // all changes. This is a cheap operation when using QRESYNC as the server keeps track diff --git a/src/components/Mailbox.vue b/src/components/Mailbox.vue index 77dd09c082..edab2383f3 100644 --- a/src/components/Mailbox.vue +++ b/src/components/Mailbox.vue @@ -233,7 +233,7 @@ export default { this.error = false logger.debug(`syncing folder ${this.mailbox.databaseId} (${this.query}) during cache initalization`) - this.sync(true) + return this.sync(true) .then(() => { this.loadingCacheInitialization = false diff --git a/src/components/ThreadEnvelope.vue b/src/components/ThreadEnvelope.vue index 7484e623aa..82f87de93c 100644 --- a/src/components/ThreadEnvelope.vue +++ b/src/components/ThreadEnvelope.vue @@ -872,22 +872,32 @@ export default { this.handleThreadScrolling() }) } catch (error) { - this.error = error + const account = this.mainStore.getAccount(this.envelope.accountId) + if (account?.error) { + // The account cannot authenticate against IMAP: show an + // actionable message instead of a generic 'Not found' + this.error = new Error(t('mail', 'Authentication failed for account {email}. Please update your credentials in the account settings.', { email: account.emailAddress })) + } else { + this.error = error + } this.loading = Loading.Done logger.error('Could not fetch message', { error }) } + // Skip IMAP-backed background lookups while the account cannot authenticate + const accountHasAuthError = !!this.mainStore.getAccount(this.envelope.accountId)?.error + // Fetch itineraries if they haven't been included in the message data - if (this.message && !this.message.itineraries) { + if (this.message && !this.message.itineraries && !accountHasAuthError) { this.fetchItineraries() } // Fetch dkim - if (this.message && this.message.dkimValid === undefined) { + if (this.message && this.message.dkimValid === undefined && !accountHasAuthError) { this.fetchDkim() } // Fetch smart replies - if (this.enabledFreePrompt && this.message && !['trash', 'junk'].includes(this.mailbox.specialRole) && !this.showFollowUpHeader) { + if (this.enabledFreePrompt && this.message && !accountHasAuthError && !['trash', 'junk'].includes(this.mailbox.specialRole) && !this.showFollowUpHeader) { try { this.smartReplies = await smartReply(this.envelope.databaseId) } catch (error) { diff --git a/src/errors/ImapAuthenticationFailedError.js b/src/errors/ImapAuthenticationFailedError.js new file mode 100644 index 0000000000..61d1d71364 --- /dev/null +++ b/src/errors/ImapAuthenticationFailedError.js @@ -0,0 +1,16 @@ +/** + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ + +export default class ImapAuthenticationFailedError extends Error { + constructor(message) { + super(message) + this.name = ImapAuthenticationFailedError.getName() + this.message = message + } + + static getName() { + return 'ImapAuthenticationFailedError' + } +} diff --git a/src/errors/convert.js b/src/errors/convert.js index e52921417c..fceb010c81 100644 --- a/src/errors/convert.js +++ b/src/errors/convert.js @@ -4,6 +4,7 @@ */ import CouldNotConnectError from './CouldNotConnectError.js' +import ImapAuthenticationFailedError from './ImapAuthenticationFailedError.js' import MailboxLockedError from './MailboxLockedError.js' import MailboxNotCachedError from './MailboxNotCachedError.js' import ManageSieveError from './ManageSieveError.js' @@ -19,6 +20,7 @@ const map = { 'OCA\\Mail\\Exception\\SentMailboxNotSetException': NoSentMailboxConfiguredError, 'OCA\\Mail\\Exception\\TrashMailboxNotSetException': NoTrashMailboxConfiguredError, 'OCA\\Mail\\Exception\\CouldNotConnectException': CouldNotConnectError, + 'OCA\\Mail\\Exception\\ImapAuthenticationFailedException': ImapAuthenticationFailedError, 'OCA\\Mail\\Exception\\ManyRecipientsException': ManyRecipientsError, 'Horde\\ManageSieve\\Exception': ManageSieveError, } @@ -44,5 +46,8 @@ export function convertAxiosError(axiosError) { return axiosError } - return new map[response.data.data.type](response.data.data.message) + const error = new map[response.data.data.type](response.data.data.message) + // Attach the structured payload so handlers can inspect extra fields (e.g. reason) + error.data = response.data.data + return error } diff --git a/src/store/mainStore/actions.js b/src/store/mainStore/actions.js index 825551d174..f1eafcb158 100644 --- a/src/store/mainStore/actions.js +++ b/src/store/mainStore/actions.js @@ -3,7 +3,7 @@ * SPDX-License-Identifier: AGPL-3.0-or-later */ -import { showError, showWarning, TOAST_DEFAULT_TIMEOUT } from '@nextcloud/dialogs' +import { showError, showWarning, TOAST_DEFAULT_TIMEOUT, TOAST_PERMANENT_TIMEOUT } from '@nextcloud/dialogs' import { translate as t } from '@nextcloud/l10n' import DOMPurify from 'dompurify' import escapeRegExp from 'lodash/fp/escapeRegExp.js' @@ -32,6 +32,7 @@ import { where, } from 'ramda' import Vue from 'vue' +import ImapAuthenticationFailedError from '../../errors/ImapAuthenticationFailedError.js' import MailboxLockedError from '../../errors/MailboxLockedError.js' import { matchError } from '../../errors/match.js' import SyncIncompleteError from '../../errors/SyncIncompleteError.js' @@ -142,6 +143,37 @@ import useOutboxStore from '../outboxStore.js' const sliceToPage = slice(0, PAGE_SIZE) +// Accounts flagged with an authentication error are excluded from background +// sync, but a single probe request is let through periodically so transient +// failures (e.g. an OAuth token refresh hiccup) heal without a page reload. +const AUTH_ERROR_RETRY_INTERVAL = 5 * 60 * 1000 +const AUTH_ERROR_PROBE_WINDOW = 30 * 1000 +const authErrorProbes = new Map() + +/** + * @param {object} account the account to check + * @return {boolean} whether syncing the account should be skipped + */ +function shouldSkipAuthErroredAccount(account) { + if (!account.error) { + return false + } + + const now = Date.now() + const lastProbe = authErrorProbes.get(account.id) ?? 0 + if (now - lastProbe < AUTH_ERROR_PROBE_WINDOW) { + // A probe was just allowed — let the related requests through + return false + } + if (now - lastProbe < AUTH_ERROR_RETRY_INTERVAL) { + return true + } + + // Open a new probe window + authErrorProbes.set(account.id, now) + return false +} + const findIndividualMailboxes = curry((getMailboxes, specialRole) => pipe( filter(complement(prop('isUnified'))), map(prop('id')), @@ -266,6 +298,7 @@ export default function mainStoreActions() { const account = await updateAccount(config) logger.debug('account updated', { account }) this.editAccountMutation({ ...account, error: false }) + authErrorProbes.delete(account.id) await this.syncMailboxesForAccount(this.accountsUnmapped[account.id]) return account }) @@ -900,9 +933,19 @@ export default function mainStoreActions() { const mailbox = this.getMailbox(mailboxId) - // Skip superfluous requests if using passwordless authentication. They will fail anyway. + // Skip superfluous requests if using passwordless authentication or if the + // account has an authentication error. They will fail anyway. const passwordIsUnavailable = this.getPreference('password-is-unavailable', false) - const isDisabled = (account) => passwordIsUnavailable && !!account.provisioningId + const isDisabled = (account) => (passwordIsUnavailable && !!account.provisioningId) || shouldSkipAuthErroredAccount(account) + + if (init && (mailbox.isUnified || mailbox.isPriorityInbox) + && this.getAccounts.some((account) => !account.isUnified && !!account.error)) { + // An initial sync must fail loudly when an account is skipped for an + // authentication error. Filtering it out silently would resolve the + // fan-out below successfully without ever caching the account's + // mailboxes, sending Mailbox.vue's initializeCache into a request loop. + throw new ImapAuthenticationFailedError('An account has an authentication error') + } if (mailbox.isUnified) { return Promise.all(this.getAccounts @@ -930,6 +973,19 @@ export default function mainStoreActions() { })) } + const account = this.getAccount(mailbox.accountId) + if (account && !account.isUnified && isDisabled(account)) { + logger.debug(`Skipping sync of mailbox ${mailboxId}: account ${mailbox.accountId} is disabled or has an authentication error`) + if (init) { + // An initial sync must fail loudly. Resolving would make + // Mailbox.vue poll for envelopes that can never be cached. + throw account.error + ? new ImapAuthenticationFailedError('Account has an authentication error') + : new Error('Account sync is disabled') + } + return [] + } + const ids = this.getEnvelopes(mailboxId, query).map((env) => env.databaseId) const lastTimestamp = this.getPreference('sort-order') === 'newest' ? null : this.getEnvelopes(mailboxId, query)[0]?.dateInt logger.debug(`mailbox sync of ${mailboxId} (${query}) has ${ids.length} known IDs. ${lastTimestamp} is the last known message timestamp`, { mailbox }) @@ -937,6 +993,14 @@ export default function mainStoreActions() { .then((syncData) => { logger.debug(`mailbox ${mailboxId} (${query}) synchronized, ${syncData.newMessages.length} new, ${syncData.changedMessages.length} changed and ${syncData.vanishedMessages.length} vanished messages`) + const syncedAccount = this.getAccount(mailbox.accountId) + if (syncedAccount && syncedAccount.error) { + // A probe request went through: the account works again + logger.info(`Account ${mailbox.accountId} can connect again, resuming sync`) + this.editAccountMutation({ ...syncedAccount, error: false }) + authErrorProbes.delete(mailbox.accountId) + } + const unifiedMailbox = this.getUnifiedMailbox(mailbox.specialRole) this.addEnvelopesMutation({ @@ -980,6 +1044,26 @@ export default function mainStoreActions() { init, }) }, + [ImapAuthenticationFailedError.getName()]: (error) => { + const failedAccount = this.getAccount(mailbox.accountId) + if (failedAccount && !failedAccount.error) { + logger.warn(`Authentication failed for account ${mailbox.accountId} (reason: ${error.data?.reason ?? 'unknown'}), pausing sync for this account`, { error }) + this.editAccountMutation({ ...failedAccount, error: true }) + // The failed request counts as the first probe + authErrorProbes.set(mailbox.accountId, Date.now()) + showError( + t('mail', 'Authentication failed for account {email}. Please update your credentials in the account settings.', { email: failedAccount.emailAddress }), + { timeout: TOAST_PERMANENT_TIMEOUT }, + ) + } + + if (init) { + // Mirror MailboxLockedError: an initial sync must fail + // loudly or Mailbox.vue keeps polling for envelopes + // that can never be cached. + throw error + } + }, [MailboxLockedError.getName()]: (error) => { if (init) { logger.info('Sync failed because the mailbox is locked, stopping here because this is an initial sync', { error }) @@ -1002,9 +1086,10 @@ export default function mainStoreActions() { }) }, async syncInboxes() { - // Skip superfluous requests if using passwordless authentication. They will fail anyway. + // Skip superfluous requests if using passwordless authentication or if the + // account has an authentication error. They will fail anyway. const passwordIsUnavailable = this.getPreference('password-is-unavailable', false) - const isDisabled = (account) => passwordIsUnavailable && !!account.provisioningId + const isDisabled = (account) => (passwordIsUnavailable && !!account.provisioningId) || shouldSkipAuthErroredAccount(account) return handleHttpAuthErrors(async () => { const results = await Promise.all(this.getAccounts diff --git a/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php b/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php index a0902fa1ab..6a52ff6e0d 100644 --- a/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php +++ b/tests/Unit/Http/Middleware/ErrorMiddlewareTest.php @@ -12,6 +12,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Exception; use Horde_Imap_Client_Exception; +use OCA\Mail\Exception\ImapAuthenticationFailedException; use OCA\Mail\Exception\NotImplemented; use OCA\Mail\Exception\ServiceException; use OCA\Mail\Http\Middleware\ErrorMiddleware; @@ -129,6 +130,56 @@ public function foo() { $this->assertInstanceOf(JSONResponse::class, $response); } + public function imapAuthFailureData(): array { + return [ + 'raw horde exception' => [ + new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + 'rate limiter engaged' => [ + new Horde_Imap_Client_Exception('Too many auth attempts', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED), + ImapAuthenticationFailedException::REASON_RATE_LIMITED, + ], + 'wrapped in service exception' => [ + new ServiceException('Could not load message', 0, new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED)), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + 'doubly wrapped' => [ + new ServiceException('Sync failed', 0, new ServiceException('login failed', 0, new Horde_Imap_Client_Exception('Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED))), + ImapAuthenticationFailedException::REASON_AUTHENTICATION_FAILED, + ], + ]; + } + + /** + * @dataProvider imapAuthFailureData + */ + public function testTranslatesImapAuthenticationFailures(Throwable $ex, string $expectedReason): void { + $request = $this->createStub(IRequest::class); + $controller = new class($request) extends Controller { + public function __construct(IRequest $request) { + parent::__construct('myapp', $request); + } + + #[TrapError] + public function foo() { + } + }; + $this->logger->expects($this->once()) + ->method('info'); + $this->logger->expects($this->never()) + ->method('error'); + + $response = $this->middleware->afterException($controller, 'foo', $ex); + + $this->assertInstanceOf(JSONResponse::class, $response); + $this->assertSame(Http::STATUS_FAILED_DEPENDENCY, $response->getStatus()); + $data = $response->getData(); + $this->assertSame('fail', $data['status']); + $this->assertSame(ImapAuthenticationFailedException::class, $data['data']['type']); + $this->assertSame($expectedReason, $data['data']['reason']); + } + public function temporaryExceptionsData(): array { return [ [new ServiceException('not temporary'), false], From 47fa7cf79b75332ab8a065911ce66659d6b92a73 Mon Sep 17 00:00:00 2001 From: Alex DiMarco Date: Sat, 25 Jul 2026 15:41:29 -0400 Subject: [PATCH 2/3] fix(imap): replace the hard auth lockout with a leaky-bucket rate limiter A transient IMAP-server auth failure (e.g. a backend blip rejecting valid credentials - observed in production on 2026-07-25) used to arm the rate limiter into a hard block for the rest of the 3h window - and the account-settings save could never clear it with an unchanged password because its connection test is rate-limited by the same key. The limiter now keeps allowing real attempts on a schedule instead of hard-blocking: 3 unthrottled attempts, then one immediate retry, then one per 30 seconds for the next 3 attempts, then one per 5 minutes. A successful login clears all throttle state, so recovery from a transient outage completes within one interval. The frontend probe scheduler mirrors the same cadence. Both rejection variants counted by the limiter ('Authentication failed.' and 'Mail server denied authentication.') are covered by unit tests. Trade-off, made deliberately: against a server that keeps rejecting (genuinely wrong credentials) the attempt budget rises from 3 per 3h window to roughly one gentle attempt per 5 minutes with background sync paused - still far below any abusive rate, while cutting self-recovery after transient server-side failures from up to 3 hours to at most 5 minutes (30 seconds while the fast retries last). Assisted-by: Claude:claude-fable-5 Signed-off-by: Alex DiMarco --- lib/IMAP/HordeImapClient.php | 54 ++++++-- src/store/mainStore/actions.js | 33 +++-- .../IMAP/IMAPClientFactoryTest.php | 6 +- tests/Unit/IMAP/HordeImapClientTest.php | 129 ++++++++++++++++-- 4 files changed, 187 insertions(+), 35 deletions(-) diff --git a/lib/IMAP/HordeImapClient.php b/lib/IMAP/HordeImapClient.php index b9701becce..6756e5de96 100644 --- a/lib/IMAP/HordeImapClient.php +++ b/lib/IMAP/HordeImapClient.php @@ -15,7 +15,6 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\IMemcache; use OCP\IMemcacheTTL; -use function floor; /** * "Decorator" around Horde's IMAP client to add auth error rate limiting. @@ -62,7 +61,14 @@ public function login() { } } - private const RATE_LIMIT_WINDOW = 3 * 60 * 60; + /** Failed attempts are forgotten after this many seconds */ + private const FAILURE_TTL = 3 * 60 * 60; + /** Unthrottled attempts before the leaky bucket engages */ + private const THROTTLE_THRESHOLD = 3; + /** Throttled attempts allowed at the fast interval before slowing down */ + private const FAST_RETRIES = 3; + private const FAST_RETRY_INTERVAL = 30; + private const SLOW_RETRY_INTERVAL = 5 * 60; protected function imapLogin() { $result = parent::_login(); @@ -76,27 +82,45 @@ protected function _login() { return $this->imapLogin(); } - $now = $this->timeFactory->getTime(); - $window = floor($now / self::RATE_LIMIT_WINDOW); - $cacheKey = $this->hash . (string)$window; + $failureKey = $this->hash . '_failures'; + $attemptKey = $this->hash . '_last_attempt'; - $counter = $this->rateLimiterCache->get($cacheKey); - if ($counter !== null && $counter >= 3) { - // Enough errors. Let's fail without involving IMAP - throw new Horde_Imap_Client_Exception( - 'Too many auth attempts', - Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED - ); + $failures = (int)$this->rateLimiterCache->get($failureKey); + if ($failures >= self::THROTTLE_THRESHOLD) { + // Leaky bucket instead of a hard block: keep allowing one real + // attempt per interval so transient server-side failures heal on + // their own — a few quick retries first, then a slow trickle. + $interval = $failures < self::THROTTLE_THRESHOLD + self::FAST_RETRIES + ? self::FAST_RETRY_INTERVAL + : self::SLOW_RETRY_INTERVAL; + $now = $this->timeFactory->getTime(); + $lastAttempt = (int)$this->rateLimiterCache->get($attemptKey); + if ($now - $lastAttempt < $interval) { + // Enough errors. Let's fail without involving IMAP + throw new Horde_Imap_Client_Exception( + 'Too many auth attempts', + Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED + ); + } + // Not atomic: a parallel burst may pass together, but the next + // interval only opens once the timestamp is stored + $this->rateLimiterCache->set($attemptKey, $now, self::FAILURE_TTL); } try { - return $this->imapLogin(); + $result = $this->imapLogin(); + if ($failures > 0) { + // The credentials work (again): reset the throttle state + $this->rateLimiterCache->remove($failureKey); + $this->rateLimiterCache->remove($attemptKey); + } + return $result; } catch (Horde_Imap_Client_Exception $e) { if ($e->getCode() === Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED && in_array($e->getMessage(), ['Authentication failed.', 'Mail server denied authentication.'], true)) { - $this->rateLimiterCache->inc($cacheKey); + $this->rateLimiterCache->inc($failureKey); if ($this->rateLimiterCache instanceof IMemcacheTTL) { - $this->rateLimiterCache->setTTL($cacheKey, self::RATE_LIMIT_WINDOW); + $this->rateLimiterCache->setTTL($failureKey, self::FAILURE_TTL); } } throw $e; diff --git a/src/store/mainStore/actions.js b/src/store/mainStore/actions.js index f1eafcb158..9a03e8d53f 100644 --- a/src/store/mainStore/actions.js +++ b/src/store/mainStore/actions.js @@ -144,10 +144,13 @@ import useOutboxStore from '../outboxStore.js' const sliceToPage = slice(0, PAGE_SIZE) // Accounts flagged with an authentication error are excluded from background -// sync, but a single probe request is let through periodically so transient -// failures (e.g. an OAuth token refresh hiccup) heal without a page reload. -const AUTH_ERROR_RETRY_INTERVAL = 5 * 60 * 1000 -const AUTH_ERROR_PROBE_WINDOW = 30 * 1000 +// sync, but probe requests are let through so transient failures (e.g. a mail +// server hiccup or an OAuth token refresh problem) heal without a page reload. +// Mirrors the backend rate limiter: a few quick retries, then a slow trickle. +const AUTH_ERROR_FAST_RETRIES = 3 +const AUTH_ERROR_FAST_RETRY_INTERVAL = 30 * 1000 +const AUTH_ERROR_SLOW_RETRY_INTERVAL = 5 * 60 * 1000 +const AUTH_ERROR_PROBE_WINDOW = 15 * 1000 const authErrorProbes = new Map() /** @@ -160,17 +163,26 @@ function shouldSkipAuthErroredAccount(account) { } const now = Date.now() - const lastProbe = authErrorProbes.get(account.id) ?? 0 - if (now - lastProbe < AUTH_ERROR_PROBE_WINDOW) { + const probe = authErrorProbes.get(account.id) + if (!probe) { + // Unknown state (e.g. flagged at page load): probe right away + authErrorProbes.set(account.id, { openedAt: now, count: 1 }) + return false + } + if (now - probe.openedAt < AUTH_ERROR_PROBE_WINDOW) { // A probe was just allowed — let the related requests through return false } - if (now - lastProbe < AUTH_ERROR_RETRY_INTERVAL) { + + const interval = probe.count < AUTH_ERROR_FAST_RETRIES + ? AUTH_ERROR_FAST_RETRY_INTERVAL + : AUTH_ERROR_SLOW_RETRY_INTERVAL + if (now - probe.openedAt < interval) { return true } // Open a new probe window - authErrorProbes.set(account.id, now) + authErrorProbes.set(account.id, { openedAt: now, count: probe.count + 1 }) return false } @@ -1049,8 +1061,9 @@ export default function mainStoreActions() { if (failedAccount && !failedAccount.error) { logger.warn(`Authentication failed for account ${mailbox.accountId} (reason: ${error.data?.reason ?? 'unknown'}), pausing sync for this account`, { error }) this.editAccountMutation({ ...failedAccount, error: true }) - // The failed request counts as the first probe - authErrorProbes.set(mailbox.accountId, Date.now()) + // The failed request counts as attempt zero; the first + // probe is due one fast interval later + authErrorProbes.set(mailbox.accountId, { openedAt: Date.now(), count: 0 }) showError( t('mail', 'Authentication failed for account {email}. Please update your credentials in the account settings.', { email: failedAccount.emailAddress }), { timeout: TOAST_PERMANENT_TIMEOUT }, diff --git a/tests/Integration/IMAP/IMAPClientFactoryTest.php b/tests/Integration/IMAP/IMAPClientFactoryTest.php index 0d60515db8..443565ab95 100644 --- a/tests/Integration/IMAP/IMAPClientFactoryTest.php +++ b/tests/Integration/IMAP/IMAPClientFactoryTest.php @@ -139,7 +139,9 @@ public function testRateLimiting(): void { $client = $imapClientFactory->getClient($account); self::assertInstanceOf(HordeImapClient::class, $client); - foreach ([1, 2, 3] as $attempts) { + // Three free attempts arm the throttle, the fourth is the immediately + // allowed leaky-bucket retry - all four reach IMAP and fail there + foreach ([1, 2, 3, 4] as $attempts) { try { $client->login(); $this->fail("Login #$attempts should cause an exception"); @@ -151,6 +153,8 @@ public function testRateLimiting(): void { // 🔥 This is fine 🔥 } } + // The fifth attempt within the same 30 second interval is throttled + // locally without contacting IMAP $this->expectException(Horde_Imap_Client_Exception::class); $this->expectExceptionCode(Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED); $this->expectExceptionMessage('Too many auth attempts'); diff --git a/tests/Unit/IMAP/HordeImapClientTest.php b/tests/Unit/IMAP/HordeImapClientTest.php index fe7668f721..d05583be61 100644 --- a/tests/Unit/IMAP/HordeImapClientTest.php +++ b/tests/Unit/IMAP/HordeImapClientTest.php @@ -11,6 +11,7 @@ use ChristophWurst\Nextcloud\Testing\TestCase; use Horde_Imap_Client_Exception; +use OC\Memcache\ArrayCache; use OCA\Mail\IMAP\HordeImapClient; use OCP\AppFramework\Utility\ITimeFactory; use OCP\IMemcache; @@ -24,41 +25,151 @@ interface IMemcacheWithTTL extends IMemcache, IMemcacheTTL { * Testable subclass that stubs out the real IMAP connection. */ class TestableHordeImapClient extends HordeImapClient { + /** @var list<'ok'|'fail'> */ + public array $outcomes = []; + public int $realAttempts = 0; + public function __construct() { // Skip Horde constructor — we only test rate limiter logic. } + public function doLogin() { + return $this->_login(); + } + protected function imapLogin() { + $this->realAttempts++; + $outcome = array_shift($this->outcomes) ?? 'fail'; + if ($outcome === 'ok') { + return true; + } throw new Horde_Imap_Client_Exception( - 'Authentication failed.', + $outcome === 'denied' ? 'Mail server denied authentication.' : 'Authentication failed.', Horde_Imap_Client_Exception::LOGIN_AUTHENTICATIONFAILED, ); } } class HordeImapClientTest extends TestCase { - private IMemcacheWithTTL|MockObject $cache; + private ArrayCache $cache; private ITimeFactory|MockObject $timeFactory; private TestableHordeImapClient $client; + private int $now = 100000; protected function setUp(): void { parent::setUp(); - $this->cache = $this->createMock(IMemcacheWithTTL::class); + $this->cache = new ArrayCache('horde_test'); $this->timeFactory = $this->createMock(ITimeFactory::class); + $this->timeFactory->method('getTime') + ->willReturnCallback(fn () => $this->now); $this->client = new TestableHordeImapClient(); $this->client->enableRateLimiter($this->cache, 'testhash', $this->timeFactory); } + private function loginExpectingAuthFailure(): void { + try { + $this->client->doLogin(); + $this->fail('login should have failed with an authentication error'); + } catch (Horde_Imap_Client_Exception $e) { + $this->assertSame('Authentication failed.', $e->getMessage()); + } + } + + private function loginExpectingThrottle(): void { + try { + $this->client->doLogin(); + $this->fail('login should have been throttled'); + } catch (Horde_Imap_Client_Exception $e) { + $this->assertSame('Too many auth attempts', $e->getMessage()); + } + } + + public function testFirstThreeFailuresAreNotThrottled(): void { + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + $this->assertSame(3, $this->client->realAttempts); + } + + public function testFastRetriesEveryThirtySecondsThenSlowTrickle(): void { + // Arm the throttle with three failures + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + // One real attempt is allowed right away (failure #4)… + $this->loginExpectingAuthFailure(); + $this->assertSame(4, $this->client->realAttempts); + + // …but not a second one within the same 30 seconds + $this->now += 10; + $this->loginExpectingThrottle(); + $this->assertSame(4, $this->client->realAttempts); + + // Failures #5 and #6 at 30 second spacing + $this->now += 20; + $this->loginExpectingAuthFailure(); + $this->now += 30; + $this->loginExpectingAuthFailure(); + $this->assertSame(6, $this->client->realAttempts); + + // From now on only one attempt per 5 minutes + $this->now += 30; + $this->loginExpectingThrottle(); + $this->now += 240; + $this->loginExpectingThrottle(); + $this->assertSame(6, $this->client->realAttempts); + + // 300 seconds after the last real attempt the next one is allowed + $this->now += 30; + $this->loginExpectingAuthFailure(); + $this->assertSame(7, $this->client->realAttempts); + } + + public function testSuccessfulLoginResetsTheThrottle(): void { + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + $this->loginExpectingAuthFailure(); + + // The immediate throttled attempt succeeds (server recovered) + $this->client->outcomes = ['ok', 'ok', 'ok']; + $this->assertTrue($this->client->doLogin()); + + // State is cleared: subsequent logins are not throttled at all + $this->assertTrue($this->client->doLogin()); + $this->assertTrue($this->client->doLogin()); + $this->assertSame(6, $this->client->realAttempts); + } + + public function testDeniedAuthenticationAlsoCounts(): void { + /** @var IMemcacheWithTTL|MockObject $cache */ + $cache = $this->createMock(IMemcacheWithTTL::class); + $cache->method('get')->willReturn(null); + $cache->expects($this->once())->method('inc')->with('testhash_failures'); + + $client = new TestableHordeImapClient(); + $client->outcomes = ['denied']; + $client->enableRateLimiter($cache, 'testhash', $this->timeFactory); + + $this->expectException(Horde_Imap_Client_Exception::class); + $this->expectExceptionMessage('Mail server denied authentication.'); + $client->doLogin(); + } + public function testSetsTtlOnAuthFailure(): void { - $this->timeFactory->method('getTime')->willReturn(100000); - $expectedKey = 'testhash9'; - $this->cache->method('get')->with($expectedKey)->willReturn(null); - $this->cache->expects($this->once())->method('inc')->with($expectedKey); - $this->cache->expects($this->once())->method('setTTL')->with($expectedKey, 3 * 60 * 60); + /** @var IMemcacheWithTTL|MockObject $cache */ + $cache = $this->createMock(IMemcacheWithTTL::class); + $cache->method('get')->willReturn(null); + $cache->expects($this->once())->method('inc')->with('testhash_failures'); + $cache->expects($this->once())->method('setTTL')->with('testhash_failures', 3 * 60 * 60); + + $client = new TestableHordeImapClient(); + $client->enableRateLimiter($cache, 'testhash', $this->timeFactory); $this->expectException(Horde_Imap_Client_Exception::class); - $this->client->login(); + $client->doLogin(); } } From 211f181ed18e65412f917bdc79d2c1c7b769f8f3 Mon Sep 17 00:00:00 2001 From: Alex DiMarco Date: Sun, 26 Jul 2026 21:52:37 -0400 Subject: [PATCH 3/3] fix(imap): drop the redundant int cast on the Horde exception code Throwable::getCode() is already an int here, and psalm rejects the redundant cast (RedundantCast). Assisted-by: Claude:claude-opus-5 Signed-off-by: Alex DiMarco --- lib/Exception/ImapAuthenticationFailedException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Exception/ImapAuthenticationFailedException.php b/lib/Exception/ImapAuthenticationFailedException.php index af7c699fdf..2c0a7e8e4d 100644 --- a/lib/Exception/ImapAuthenticationFailedException.php +++ b/lib/Exception/ImapAuthenticationFailedException.php @@ -37,7 +37,7 @@ public static function fromHorde(Horde_Imap_Client_Exception $e): self { $reason = $e->getMessage() === 'Too many auth attempts' ? self::REASON_RATE_LIMITED : self::REASON_AUTHENTICATION_FAILED; - return new self('IMAP authentication failed', (int)$e->getCode(), $e, $reason); + return new self('IMAP authentication failed', $e->getCode(), $e, $reason); } /**