Add daily cap for email verifications#188
Conversation
| ErrMaxCodeAttempts = NewExposedError(13010, "maximum code verification attempts exceeded") | ||
| ErrInvalidCode = NewExposedError(13011, "invalid verification code") | ||
| ErrRegistrationVerificationPending = NewExposedError(13012, "registration verification already pending for this email") | ||
| ErrDailyVerificationLimitReached = NewExposedError(13013, "daily verification limit reached for email") |
There was a problem hiding this comment.
cc @szilardszaloki New error code that we should also add to the client.
There was a problem hiding this comment.
@DJAndries Would you mind letting me know which endpoints can return this error condition? I know /v2/verify/init can. Perhaps /v2/accounts/password/init as well?
There was a problem hiding this comment.
Yes, both /v2/verify/init and /v2/accounts/password/init can return this error
There was a problem hiding this comment.
Pull request overview
This PR aims to mitigate repeated brute-force email verification attempts by adding a per-email daily limit to verification initialization, and by retaining verification rows longer so the daily window can be enforced.
Changes:
- Add a new exposed error for “daily verification limit reached”.
- Enforce a per-email daily cap in
Datastore.CreateVerification()by counting verifications created since start-of-day. - Extend the cron cleanup window for
verificationsand add controller/test coverage for the new error path.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| util/error.go | Adds a new exposed error code for daily verification limiting. |
| migrations/11_cron.up.sql | Extends verification-row retention to support daily counting. |
| datastore/verifications.go | Adds daily-count logic to block further verification creation for an email. |
| controllers/verification.go | Maps the new daily-cap error to a 400 response in /v2/verify/init. |
| controllers/verification_test.go | Adds a test asserting daily-cap behavior on repeated /v2/verify/init calls. |
Comments suppressed due to low confidence (1)
controllers/verification.go:155
ErrDailyVerificationLimitReachedis now surfaced here as a 400, butInitializeVerificationis also called fromcontrollers/accounts.go(e.g.SetupPasswordInitwhen creating a registration verification). That path currently doesn’t treat this error as a client error, so it will respond 500 instead of 400 when the daily cap is hit. Consider adding the same error handling there (and any other call sites) to keep responses consistent.
if err != nil {
if errors.Is(err, util.ErrTooManyVerifications) ||
errors.Is(err, util.ErrDailyVerificationLimitReached) ||
errors.Is(err, util.ErrIntentNotAllowed) ||
errors.Is(err, util.ErrEmailDomainNotSupported) ||
errors.Is(err, util.ErrAccountExists) ||
errors.Is(err, util.ErrRegistrationVerificationPending) ||
errors.Is(err, util.ErrAccountDoesNotExist) {
util.RenderErrorResponse(w, r, http.StatusBadRequest, err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fmarier
left a comment
There was a problem hiding this comment.
Let's change the 5 for a 10 and then merge: https://github.com/brave/accounts/pull/188/files#r3277717457
Resolves #183