security(auth): add rate limiting for brute-force protection#28
Open
DeryFerd wants to merge 8 commits into
Open
security(auth): add rate limiting for brute-force protection#28DeryFerd wants to merge 8 commits into
DeryFerd wants to merge 8 commits into
Conversation
- Add RateLimiter class with sliding window algorithm - Apply rate limiting to /login endpoint (5 attempts per 5 minutes) - Thread-safe implementation with proper locking - Comprehensive unit tests with 12 test cases - Support for custom identifier functions - JSON and HTML error responses
- Move config import inside rate_limit decorator - Prevents issues when tests reload config module - Fixes failing auth open redirect tests in CI
…event rate limit failures
4b21bee to
b68484f
Compare
…test interference
Owner
|
There is too many unrelated changes for this |
Contributor
Author
|
@anvie Thanks for the feedback. I just pushed commit 0040afd to fix the failing CI (rate limiter custom identifier path), and both checks are now passing on run 25902803580. You’re right that this PR currently includes unrelated files. I can immediately follow up by trimming this branch to only rate-limiter/auth/test changes (or split unrelated work into a separate PR), whichever you prefer. |
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.
Problem
The login endpoint currently has Cloudflare Turnstile CAPTCHA but no rate limiting. This means attackers can attempt multiple passwords per CAPTCHA solve, making brute-force attacks feasible against weak passwords. While CAPTCHA helps, it's not enough on its own—rate limiting provides defense-in-depth.
What Changed
Added a thread-safe rate limiter that tracks login attempts per IP address using a sliding window algorithm. The
/loginendpoint now blocks requests after 5 failed attempts within a 5-minute window.New files:
backend/rate_limiter.py— Core rate limiting implementation with configurable limitsunit_tests/test_rate_limiter.py— 12 test cases covering edge cases and thread safetyModified files:
routes/auth.py— Applied@rate_limitdecorator to login endpointThe rate limiter uses in-memory storage (good enough for single-instance deployments) and automatically cleans up expired timestamps. For multi-instance setups, this could be extended to use Redis or similar, but that's out of scope for now.
How It Works
When a user hits the login endpoint:
The implementation is thread-safe using locks, so concurrent requests from the same IP won't race and bypass the limit.
Testing
All 12 unit tests pass, covering:
Manually tested the login flow:
Compatibility
This is backward compatible—existing login behavior is unchanged for users who don't hit the limit. No database migrations needed since rate limit data is in-memory.
The rate limiter is generic enough to be reused on other endpoints if needed (API routes, password reset, etc.), though that's not part of this PR.