fix(imap): surface auth failures to the client and make the rate limiter self-healing - #13358
Open
alexdimarco wants to merge 3 commits into
Open
fix(imap): surface auth failures to the client and make the rate limiter self-healing#13358alexdimarco wants to merge 3 commits into
alexdimarco wants to merge 3 commits into
Conversation
alexdimarco
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
July 25, 2026 20:13
Contributor
|
Thanks for your pr. |
… 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 nextcloud#7279 Fixes nextcloud#12136 Assisted-by: Claude:claude-fable-5 Signed-off-by: Alex DiMarco <alex@dimarcotech.com>
…iter
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 <alex@dimarcotech.com>
alexdimarco
force-pushed
the
fix/surface-imap-auth-errors
branch
from
July 25, 2026 20:58
93e9190 to
47fa7cf
Compare
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 <alex@dimarcotech.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7279
Fixes #12136
Problem
When IMAP rejects an account's credentials mid-session (changed/expired password, or a transient failure of the mail server's auth backend), the resulting
Horde_Imap_Client_ExceptionwithLOGIN_AUTHENTICATIONFAILEDbubbles up toErrorMiddlewareas a generic HTTP 500 "Server error". The web client cannot distinguish this from any other failure, so:App.vuekeeps pollingmailboxes#syncevery 30 seconds forever,We hit exactly this in production on 2026-07-25: a mail-server backend blip rejected valid credentials a handful of times within an hour, and the account stayed dead for hours with no actionable feedback.
What this PR does
Commit 1 — surface the failure and stop the doomed polling:
ImapAuthenticationFailedException(aClientException, HTTP 424 Failed Dependency) carrying areasonofAUTHENTICATION_FAILEDorRATE_LIMITED. 424 was chosen over 401 deliberately: the Nextcloud session is fine, only the IMAP dependency failed, and 401 would trigger the session-expiry machinery.ErrorMiddlewarewalks the exception chain of any uncaught exception from a#[TrapError]method; a HordeLOGIN_AUTHENTICATIONFAILEDanywhere in the chain becomes the typed jsendfailpayload, logged once at info level instead of error. Covers sync, body and attachment endpoints with one hunk.getDkimgets the previously missing#[TrapError].ImapToDbSynchronizer::sync()'s unguarded$client->login()is wrapped in aServiceException;SyncJobdetects auth failures anywhere in the chain so cron logs them at info too.convert.js; on auth failure the account is flagged via the existingaccount.errorbanner inNavigation.vue, a persistent toast names the account, background sync skips flagged accounts, and IMAP-backed side lookups (DKIM/itineraries/smart replies) are skipped while flagged. Initial syncs fail loudly (individual and unified/priority fan-out) soinitializeCachecannot loop requesting envelopes that can never be cached;initializeCache()now returns its promise chain so failures reach the error UI, which shows an actionable message instead of "Not found".Commit 2 — make the rate limiter self-healing (leaky bucket):
Instead of hard-blocking for the rest of a 3-hour window, the limiter keeps allowing real attempts on a schedule: 3 unthrottled attempts, one immediate retry, then one per 30 seconds (×3), then one per 5 minutes. A successful login clears all throttle state. The frontend probe scheduler mirrors the cadence, so a transient server-side failure now heals within seconds-to-minutes of the server recovering — no manual cache surgery, no page reload — and the settings Save always recovers within one interval.
Trade-off, made deliberately: against a server that keeps rejecting (genuinely wrong credentials) the attempt budget rises from 3 per 3h to roughly one gentle attempt per 5 minutes with background sync paused — still far below abusive rates, while cutting self-recovery after transient failures from up to 3 hours to at most 5 minutes. Happy to discuss a slower tail if preferred.
Test plan
ErrorMiddlewaretranslation (raw/wrapped/doubly-wrapped/rate-limited), andHordeImapClientschedule pinning (threshold arithmetic, 30s/5min anchors, success reset, both counted rejection messages incl.Mail server denied authentication.); the integration rate-limit test is updated for the new semantics.Notes for reviewers
RATE_LIMITEDreason relies on the limiter's exact'Too many auth attempts'message (same coupling asCouldNotConnectException::getReason()); happy to move both onto a shared constant.CONNECTION_ERROR; a rate-limit-specific hint would be a nice follow-up.