Skip to content

Distinguish OAuth account-link rate-limited retries from genuine sends - #3567

Merged
Freika merged 1 commit into
masterfrom
detail/bug-fix/distinguish-oauth-account-link-rate-limited-retrie-a7050c
Sep 10, 2026
Merged

Distinguish OAuth account-link rate-limited retries from genuine sends#3567
Freika merged 1 commit into
masterfrom
detail/bug-fix/distinguish-oauth-account-link-rate-limited-retrie-a7050c

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Bug

OAuth account-link verification emails suppressed by the per-account rate-limit cache were reported to users as if they had been sent.

Auth::FindOrCreateOauthUser#send_verification_email rate-limits via Rails.cache.write(..., unless_exist: true), which returns false when the key already exists. That false sentinel was computed and then discarded — send_verification_email returned nil on rate-limit, handle_email_collision raised LinkVerificationSent unconditionally, and callers couldn't tell a suppressed send from a genuine one:

  • The web Auth::AccountLinksController#email_fallback flashed the affirmative "We sent a confirmation link to X" notice even when no mailer job was enqueued.
  • Api::V1::Auth::{Google,Apple}Controller returned HTTP 202 error: 'verification_sent' for both genuine sends and rate-limited retries.

So every rate-limited retry misled the user. A narrow but real subset of web-flow users (email-link path, 15min < t < 1hour after the original send, original link unclicked past its TTL, original email inaccessible, password forgotten) were fully stranded with no accurate recovery signal.

Fix

Propagate the rate-limit state to callers and branch the user-facing message on it.

  • Auth::FindOrCreateOauthUser::LinkVerificationSent now carries a rate_limited: attribute (default false).
  • send_verification_email returns :sent or :rate_limited; handle_email_collision uses that sentinel to set rate_limited on the exception.
  • Auth::AccountLinksController#email_fallback flashes the existing confirmation_link_sent notice only when a job was actually enqueued; otherwise it flashes a new account_link_rate_limited alert.
  • Api::V1::Auth::{Google,Apple}Controller branch on e.rate_limited: genuine send → HTTP 202 error: 'verification_sent'; rate-limited retry → HTTP 429 error: 'verification_rate_limited' with a Retry-After: 3600 header.
  • Added the three new I18n keys to all supported locales (en, fr, de, es, pl, ca, zh).

This does not regress the enumeration posture: account existence is already disclosed on the first collision response, and the rate-limit state is self-known to the requester (their own prior request wrote the cache key). A distinct verification_rate_limited response leaks nothing new — unlike the Devise paranoid pattern this isn't an anonymous probe.

Testing

  • Service, web, and API request specs for the rate-limit branch pass, including new assertions that LinkVerificationSent.rate_limited is false on genuine sends and :raise_only flows and true on rate-limited retries, that the web email_fallback flashes the notice vs. the distinct alert correctly, and that the API controllers return 429 with
    Retry-After: "3600" and no second mailer on the rate-limited retry.
  • Unaffected paths verified: unverified email → 403, account pending deletion → 409, identity match, new-user, Apple-omits-email, the web from_omniauth raise-only flow, the password-challenge #confirm path (still links despite the rate-limit), and the expired-token Auth::VerifyAccountLinkToken rejection. Swagger contracts for the Google/Apple auth endpoints still generate.
  • Locale YAML parses for all locales, and the three new keys resolve with no "translation missing" for en, fr, de, es, pl, ca, zh.
  • rails zeitwerk:check and rubocop on the changed files are clean.

End-to-end smoke verification (not versioned — the Google/Apple id_token verifiers require real Google/Apple-issued tokens, so the verifiers were stubbed in-process while a real Puma server handled the HTTP requests):

  • API: booted Rails, started Puma on 127.0.0.1:33333, and issued real Net::HTTP::Post requests. First Google/Apple collision within the hour → HTTP 202 error: 'verification_sent', no Retry-After. Second collision → HTTP 429 error: 'verification_rate_limited', Retry-After: "3600", existing user's provider/uid unchanged.
  • Web: an ActionDispatch::IntegrationTest drove the full Rack stack via OmniAuth test-mode and the real /users/auth/openid_connect/callback route. Genuine email_fallback → notice set, alert nil, 1 mailer enqueued; rate-limited retry → notice nil, distinct alert set, 0 mailers; #confirm linked the identity even with the rate-limit key pre-seeded; clicking the originally-emailed link after its 15-min TTL showed "Link invalid or expired." and the password challenge still linked afterward.

Targeted at dev per the repo's contributing guide. No issue number was provided for reference.


Automatic Fixes PRs can be configured here.

@detail-app
detail-app Bot requested a review from Freika September 7, 2026 10:02
@Freika
Freika merged commit e462cc6 into master Sep 10, 2026
19 checks passed
@Freika
Freika deleted the detail/bug-fix/distinguish-oauth-account-link-rate-limited-retrie-a7050c branch September 10, 2026 20:30
pull Bot pushed a commit to AmirulAndalib/dawarich that referenced this pull request Sep 13, 2026
Follow-up to Freika#3567, which merged before these review fixes landed.

The rate-limit cache held a boolean, so a retry fifty-five minutes into the
window was still told to wait a full hour. It now stores the send time and the
exception carries the remaining seconds, clamped to the window.

The web fallback and the API controllers share one helper for acquiring the
limit, so a send from either path is accounted the same way, and the new 429 is
described in the rswag specs so it reaches the generated contract that the
mobile client reads.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant