fix(pairing): reset the failed-approval counter on a successful approval - #62145
Conversation
|
Verified the premise on current main: Nice detail: the reset only writes the file when the counter is non-zero, so the happy path adds no I/O. Ran |
|
Thanks for the focused regression fix. The premise remains live on current The new regression test covers the relevant interleaving case rather than duplicating the existing active-lockout test at Automated hermes-sweeper review. |
approve_code()'s success path never cleared _failures:{platform}. The
counter is incremented on every non-matching code, persisted in
_rate_limits.json, and only ever reset to 0 when it reaches
MAX_FAILED_ATTEMPTS (firing the lockout). So it counts failures over the
gateway's entire lifetime, not consecutive ones.
An owner who mistypes a pairing code on a handful of separate occasions
— each time immediately retyping it correctly and successfully pairing —
accumulates those isolated typos. A later single fresh typo then hits
MAX_FAILED_ATTEMPTS and locks the whole platform out for an hour, at
which point _is_locked_out gates approve_code and even the *correct*
code is rejected.
Reset the counter on a successful approval, matching standard
brute-force-guard semantics (the counter tracks consecutive failures).
This does not weaken protection: an attacker cannot produce a success
without a valid code, and 5 consecutive wrong attempts still lock out.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
a06742e to
893eb9d
Compare
|
Rebased onto current main — the branch was still based on a July 12 checkout, so its CI runs had aged out and no checks were being reported. Fresh runs are in flight now. Premise re-verified on today's main: |
|
Correct call — the counter tracks consecutive failures, so a success has to clear it. Cherry-picked into #74427 with authorship preserved; the reset moved into the shared |
Problem
PairingStore.approve_code()'s success path never clears_failures:{platform}. The counter is incremented on every non-matching code (_record_failed_attempt), persisted in_rate_limits.json, and only ever reset to0when it reachesMAX_FAILED_ATTEMPTS(which fires the 1-hour lockout). So it counts failures over the gateway's entire lifetime, not consecutive ones — and the sibling guard_is_rate_limitedin the same class correctly uses a time window, so this counter is the odd one out.Concrete failure (normal operation)
Pairing codes are 8 random chars, so mistyping one is common. An owner who pairs users over time and fat-fingers the code on 4 separate occasions — each time immediately retyping it correctly and successfully pairing — accumulates
_failures:telegram = 4(never reset, survives restarts).A later single fresh typo →
_record_failed_attempt→fails = 5→ lockout for 1 hour. The owner now types the correct code, but_is_locked_outgatesapprove_code(the #10195 fix) and returnsNone— the valid pairing is rejected and all approval on that platform is dead for an hour, from five non-consecutive typos.Fix
Reset the counter on a successful approval — standard brute-force-guard semantics, where the counter tracks consecutive failures. Added
_reset_failed_attempts()(mirrors_record_failed_attempt) and call it from theapprove_codesuccess path.This does not weaken protection: an attacker cannot produce a success without a valid code, and 5 consecutive wrong attempts still lock out exactly as before. It only stops isolated, interleaved-with-success typos from accumulating.
Tests
Added
test_successful_approval_resets_failure_countertoTestLockout:MAX_FAILED_ATTEMPTS - 1wrong attempts, then a valid approval, asserts_failuresis cleared and a subsequent single typo does not trip the lockout. Fails on current code (counter stays at 4), passes with the fix.python -m pytest tests/gateway/test_pairing.py— 52 passed, 1 skipped.Dedup
#21325(merged) and#10248(closed) added/duplicated the lockout check inapprove_code; neither resets the failure counter on success. This is a distinct defect.🤖 Generated with Claude Code