[ArtyomSavchenko]

No description provided.

[claude]

Review summary

Limiting OTP attempts is a good idea, and the design holds up: the attempt is counted before the code is checked, OtpLocked is defined once with entries in all 14 locales, and the Postgres migration (ADD COLUMN IF NOT EXISTS ... DEFAULT 0) uses upstream's key. $inc is already supported by both the Mongo and Postgres collections.

Two issues to address before merge (details in the inline comments):

1. Anyone can lock out any user (high): the window lock also blocks sendOtp, so someone who knows only an email can keep that user from receiving codes, one hour at a time, indefinitely. Combined with the existing password lockout, the user has no way left to sign in. The per-code cap alone already stops brute force.
2. Each guess is counted several times: verifyOtpAttempt increments every active OTP, so with 2 or more active codes (after a resend) each guess counts 2 or more times toward the window limit. Real users get locked out after a handful of typos.

Minor: OTP_MAX_ATTEMPTS and the other limits are read from process.env when the module loads, while the nearby OTP settings use getMetadata(accountPlugin.metadata.*). Consider following the same pattern so these limits can be configured and tested the same way.

🤖 Generated with Claude Code

[MichaelUray]

Thanks for tackling OTP brute force. I reviewed the two commits up to db9929d. The double counting from the earlier bot comment is fixed, $inc is atomic on Mongo and Postgres, existing rows are handled and all 14 locales have the new message. A few things still need to change before this is safe to merge, though.

Blocking

1. The window lock lets anyone lock a user out of OTP login, and the correct code is rejected too (server/account/src/utils.ts:642, :767). I know the sendOtp part is planned for a separate PR, but with this PR merged as is, the lockout already exists: someone who knows only the email address requests a code and sends ~16 wrong guesses, after which sendOtp refuses new codes and verifyOtpAttempt rejects even the correct code for the whole window. Repeating that once an hour keeps the user out indefinitely, and the password lock tells the user to unlock via OTP. Removing the check only from sendOtp would not be enough, because verifyOtpAttempt checks the window before comparing the code. Suggestion: ship only the per-code cap in this PR (window limit off by default or removed) and add a throttle that cannot be triggered against a victim (e.g. keyed by client IP plus social id, or exponential backoff) in the follow-up.

2. Guesses that overlap code creation are not counted (utils.ts:757-763). When the first query finds no active OTP, verification still runs the second query and accepts a matching code found there. sendOtp inserts the code only after the email is sent (utils.ts:668-669), so concurrent verifications can do their first read before the insert and their second read after it: they skip the increment but are still checked against the new code. In an in-memory reproduction of this interleaving with the production function, 19 guesses returned invalid, a 20th matching guess returned valid, and the stored attempt count stayed at 0. Suggestion: return invalid immediately when the first query finds no active code, and only accept a code whose budget this request actually consumed (conditional increment or transaction), with a deterministic test that inserts a code between the two reads.

3. After a resend, older codes are outside the per-code cap (utils.ts:757-772). The increment now targets the newest active code, but the exhaustion check uses the code that matched. With a code lifetime longer than the resend delay (e.g. the in-code fallbacks of 60 s / 30 s), five wrong guesses on code A, then a resend creating B, then the correct A on the sixth try succeeds (counters A=5, B=1), and further wrong guesses only charge B. With the window limit disabled, which OtpLimits explicitly supports, nothing bounds guesses against A. Suggestion: invalidate previous codes on resend, or accept and charge only the newest active code.

Should fix

4. The tests cannot show that attempts accumulate. mockDb.otp.update is a no-op jest.fn(), so the second read never sees the increment. The "last allowed attempt" test starts at 5 stored attempts and expects success, but with a working increment production returns locked (6 attempts). utils.test.ts:1052 also pins the behaviour from point 1 (correct code rejected under the window lock). A stateful collection stub that applies $inc, plus tests for overlapping codes and the interleaving from point 2, would cover this.
5. The migration comment refers to an upstream key that does not exist (server/account/src/collections/postgres/migrations.ts:890). There is no "v33" for this on upstream/develop; the open upstream PR hcengineering/platform#11143 adds the same column under account_db_v28_add_otp_attempts with different counting. The SQL is idempotent, so the database is fine, but the comment is misleading and the next upstream sync will need to reconcile the two.

Nits

• An env var set to an empty string becomes NaN, which disables the limits and makes the expired-code cleanup throw on every run; parse with a fallback.
• The window is measured from code creation, not from the attempts.
• The lock message gives no time or way to recover.
• Every failed guess logs the user's email at warn level.
• The three new env vars are not documented.