Repository navigation
Count only failed MFA attempts toward the rate limit - #192
Conversation
The limit of five attempts per kind of factor in five minutes counted every attempt, successful and refused ones included. Every run, exec and deploy approval needs its own MFA proof, so a person running a few commands, or approving deploys for a few apps, went past five in five minutes, and each refused retry kept the window full. Their commands failed after a good PIN and touch, with "Security key verification failed" (or "websocket: bad handshake" for an exec). Only attempts that failed or are still being checked count now. A refused attempt is still recorded, and is answered with 429 "Too many failed MFA attempts" instead of a verification failure.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Attempt accounting and error propagation internal/gateway/db/mfa_attempts.go, internal/gateway/db/mfa_attempts_test.go, internal/gateway/auth/mfa/attempts.go, internal/gateway/auth/mfa/security_test.go, docs/src/content/docs/concepts/mfa-security.mdx |
The reservation count includes failed and unfinished attempts, but excludes successful and rate-limited attempts. Tests cover attempt status and factor type. Auth-layer errors wrap the limit error, and the documentation describes the limit. |
Rate-limit responses internal/gateway/middleware/mfa.go, internal/gateway/proxy/mfa_verification.go, internal/gateway/handlers/deploy_approval_requests_mfa_test.go |
MFA middleware and request verification return HTTP 429 for attempt-limit errors. A deploy approval test checks the response and confirms the approval remains pending. |
Version updates
| Layer / File(s) | Summary |
|---|---|
Release identifiers convox.yml, web/package.json |
The gateway and admin image tags and the web package version change to 0.1.25. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant verifyByType
participant ReserveMFAAttempt
participant handleVerifyError
verifyByType->>ReserveMFAAttempt: reserve MFA attempt
ReserveMFAAttempt-->>verifyByType: return attempt-limit error
verifyByType->>handleVerifyError: pass verification error
handleVerifyError-->>verifyByType: return HTTP 429 and mfa_rate_limited
Possibly related PRs
- DocSpring/rack-gateway#182: Updates MFA requirements for
runandexec, which this PR’s documentation references.
Suggested reviewers: claude
Merge Risk: 
501f8
-
No actionable merge-blocking risk remains after the reviewed MFA rate-limit changes and normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (3 skipped: 3… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title "Count only failed MFA attempts toward the rate limit" matches the main change. The PR changes the MFA rate limit so that only failed or unfinished attempts count. Successful attempts and re… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Title check
-
Explanation
The title "Count only failed MFA attempts toward the rate limit" matches the main change. The PR changes the MFA rate limit so that only failed or unfinished attempts count. Successful attempts and refused (rate_limited) attempts no longer count. The changes in internal/gateway/db/mfa_attempts.go, the new tests, and the middleware and proxy error handling all support this. The title is specific and short. It does not mention the version bump or the new 429 handling, which are secondary.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 7 files. (3 skipped: 3 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
A rabbit checks the counters twice
Failed and waiting proofs take space
Success slips past the counting gate
Too many knocks bring 429
The gateway sets its tags anew
And carrots wait for code review
Comment @coderabbitai help to get the list of available commands.
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| JavaScript |  |
Oct 26, 2026 1:33a.m. | Review ↗ |
| Go | |
Oct 26, 2026 1:33a.m. | Review ↗ |
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The MFA rate limit (five attempts per kind of factor in five minutes) counted every attempt: successful ones, and the refused ones too. Every
run,execand deploy approval needs its own MFA proof, so a person running a few commands, or approving deploys for a few apps, went past five in five minutes. After a good PIN and touch they got "Security key verification failed" (401mfa_verification_failed), or "websocket: bad handshake" for an exec. Each refused retry added another attempt, so retrying kept them locked out.Fix
ReserveMFAAttemptcounts only attempts that failed or are still being checked. Successful attempts don't count, and neither do refused (rate_limited) ones. Concurrent attempts are still counted one at a time under the user's row lock, so the limit still caps guesses at a code.mfa_rate_limited) and on the rack proxy, instead of a verification failure.Testing
TestReserveMFAAttemptCountsOnlyFailedAndUnfinishedAttempts: ten successes, then the limit is reached by unfinished and failed attempts only, and refused attempts don't count. It fails against the old query.TestApproveDeployApprovalRequest_RateLimitedMFA: a rate-limited approval answers 429mfa_rate_limitedand the request stays pending.Includes the bump to v0.1.25.
Summary by CodeRabbit
Bug Fixes
Documentation