Skip to content

fix(imap): surface auth failures to the client and make the rate limiter self-healing - #13358

Open
alexdimarco wants to merge 3 commits into
nextcloud:mainfrom
alexdimarco:fix/surface-imap-auth-errors
Open

fix(imap): surface auth failures to the client and make the rate limiter self-healing#13358
alexdimarco wants to merge 3 commits into
nextcloud:mainfrom
alexdimarco:fix/surface-imap-auth-errors

Conversation

@alexdimarco

Copy link
Copy Markdown

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_Exception with LOGIN_AUTHENTICATIONFAILED bubbles up to ErrorMiddleware as a generic HTTP 500 "Server error". The web client cannot distinguish this from any other failure, so:

  • the background sync loop in App.vue keeps polling mailboxes#sync every 30 seconds forever,
  • the user gets no hint that their credentials are wrong (bodies and attachments just stop loading),
  • the server log fills with error-level entries — we measured 149 entries/hour from a single affected account, and
  • the client-side auth rate limiter compounds it: after 3 failures it hard-blocks the account for the rest of a 3-hour window, and the account-settings Save cannot recover with an unchanged password because its own connectivity test is rate-limited by the same cache key (the lockout concern described in Invalid passwords or IMAP login errors do not trigger a comprehensive error message for existing accounts #7279).

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:

  • New ImapAuthenticationFailedException (a ClientException, HTTP 424 Failed Dependency) carrying a reason of AUTHENTICATION_FAILED or RATE_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.
  • ErrorMiddleware walks the exception chain of any uncaught exception from a #[TrapError] method; a Horde LOGIN_AUTHENTICATIONFAILED anywhere in the chain becomes the typed jsend fail payload, logged once at info level instead of error. Covers sync, body and attachment endpoints with one hunk. getDkim gets the previously missing #[TrapError].
  • ImapToDbSynchronizer::sync()'s unguarded $client->login() is wrapped in a ServiceException; SyncJob detects auth failures anywhere in the chain so cron logs them at info too.
  • Frontend: the new type maps in convert.js; on auth failure the account is flagged via the existing account.error banner in Navigation.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) so initializeCache cannot 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

  • New unit tests: ErrorMiddleware translation (raw/wrapped/doubly-wrapped/rate-limited), and HordeImapClient schedule 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.
  • Full unit suite green against a Nextcloud 34 server checkout.
  • Validated on a real production outage (Nextcloud 34 / backported to 5.10.9): banner + toast appear, polling stops (network tab quiet apart from probes), a single info line per request in the log, and automatic recovery once the server accepted logins again.

Notes for reviewers

  • The RATE_LIMITED reason relies on the limiter's exact 'Too many auth attempts' message (same coupling as CouldNotConnectException::getReason()); happy to move both onto a shared constant.
  • A throttled settings-Save currently surfaces as generic CONNECTION_ERROR; a rate-limit-specific hint would be a nice follow-up.

@kesselb

kesselb commented Jul 25, 2026

Copy link
Copy Markdown
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
alexdimarco force-pushed the fix/surface-imap-auth-errors branch from 93e9190 to 47fa7cf Compare July 25, 2026 20:58
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants