fix: attempts in flight count against the sign-in limit - #942
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The brute-force limiter (
TrackService::checkTracking()) counted a source's recent attempts, the caller then ran the whole attempt, and a failure was recorded only at the end. On a sign-in, that attempt includes a bcrypt verify of about 277ms. Attempts sent together therefore all counted the same rows and all passed, so the ten-attempt limit let through as many guesses per window as there were workers to run them. The guard and the change it guards were separate statements with the slow part between them.Change
checkTracking()records the attempt before counting and leaves its own row out of the count. Of any two concurrent attempts, the one that counts second sees the first.TrackService::release()withdraws that row once the attempt is decided. A failure has recorded its own row by then, so the counts come out exactly as if the attempts had arrived one at a time. A successful attempt costs nothing, which matters for an office signing in from behind one address.checkTracking()releases in afinally:Login::doLogin()Api::setup()TrackRepository::deleteByIdBatch()is added for the release.Tests
TrackTest):BruteForceTrackingTest, real DB): two requests from one address, interleaved with nine attempts already standing. The second is refused while the first is in flight, and once both are released a third request is back under the limit.ApiTestandRefusalsTestassert that each door releases on both success and refusal.Mutation-verified:
TrackTestordering/count/release tests and the integration interleaving test.finallyreleases fails the door tests.