feat: add rate limiting for authentication endpoints #807
Conversation
Signed-off-by: Grant Ramsay <[email protected]>
Signed-off-by: Grant Ramsay <[email protected]>
Signed-off-by: Grant Ramsay <[email protected]>
…and configuration Signed-off-by: Grant Ramsay <[email protected]>
…rotection Signed-off-by: Grant Ramsay <[email protected]>
…ge notes - Create complete rate limiting documentation (docs/usage/rate-limiting.md) - Overview of rate limiting features and backends - Configuration guide for in-memory and Redis backends - Usage examples and best practices - Security considerations and troubleshooting - 541 lines of comprehensive documentation - Add rate limiting feature to README.md and docs/index.md - Describe conservative limits for auth endpoints - Mention both in-memory and Redis backend support - Note HTTP 429 responses with Retry-After header - Highlight Prometheus metrics integration - Add decorator ordering notes to both caching and rate limiting docs - Document correct order when using both decorators - Explain security rationale (rate limit before cache) - Provide code examples with both decorators - Cross-reference between documentation pages - Update mkdocs.yml navigation - Add "Rate Limiting and Security" before Metrics section - Position logically in Usage section All documentation follows existing style and format.
📝 WalkthroughWalkthroughAdds an opt-in rate-limiting subsystem using slowapi with in-memory or Redis backends, per-endpoint limits on authentication routes, a 429 Retry-After handler, Prometheus metric for violations, settings/configuration entries, documentation, and tests; no existing public signatures removed. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant App as FastAPI
participant Decorator as rate_limited
participant Storage as Redis/Memory
participant Auth as Auth Handler
participant Metrics as Prometheus
Client->>App: HTTP request (e.g., POST /register)
App->>Decorator: invoke rate_limited (key = get_remote_address)
Decorator->>Storage: read/update counter for key+endpoint
alt Within limit
Storage-->>Decorator: count < quota
Decorator-->>App: allow
App->>Auth: call endpoint handler (request injected)
Auth-->>App: response (200/201/...)
App-->>Client: return response
else Limit exceeded
Storage-->>Decorator: count >= quota
Decorator->>Metrics: increment_rate_limit_exceeded(endpoint, limit)
Decorator-->>App: raise RateLimitExceeded
App->>App: rate_limit_handler(parse_retry_after)
App-->>Client: 429 with Retry-After header and JSON detail
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
🔇 Additional comments (3)
Comment |
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Signed-off-by: Grant Ramsay <[email protected]>
…ancement plan - Mark critical item #3 as complete with implementation details - Update summary to show 4 of 5 critical issues closed - Add checkmarks to all 7 implemented rate limits - Mark rate limiting complete in Sprint 1 implementation order - Add comprehensive user-based rate limiting enhancement plan to future features - Document implementation approach, benefits, and technical considerations
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @app/rate_limit/decorators.py:
- Around line 61-63: The code sets client_ip = "unknown" and then assumes
request.client is non-None when doing request.client.host, which can raise
AttributeError; update the logic in the decorator where client_ip is determined
to safely handle a None request.client (e.g., check request is truthy and
request.client is not None and hasattr(request.client, "host") or use
getattr(request.client, "host", None)) and fall back to "unknown" if host is
missing; make this change around the client_ip assignment to avoid accessing
.host on None.
In @tests/integration/test_rate_limit_metrics.py:
- Around line 44-53: The assertion in the test uses a weak fallback ("or '#
HELP' in metrics_text") that always passes; update the check in
tests/integration/test_rate_limit_metrics.py to assert presence of
"rate_limit_exceeded_total" only (i.e., replace the current compound assert with
assert "rate_limit_exceeded_total" in metrics_text) or, if metric timing is
genuinely unreliable, mark the test with @pytest.mark.skip and add a short skip
reason; locate the assertion around variables metrics_response and metrics_text
to apply the change.
In @tests/unit/test_rate_limit_config.py:
- Around line 21-32: The period validation is inconsistent; update the check in
the loop over limits in tests/unit/test_rate_limit_config.py (variables: limits,
limit, count, period) to use a single pattern match instead of the current list
+ endswith logic—validate that period matches an optional numeric prefix
followed by one of the base units with optional plural (e.g. regex like:
optional digits + (second|minute|hour|day) with optional trailing "s"), and
replace the existing asserts with a regex match assertion so inputs like
"15minutes" or "2hours" are handled correctly.
🧹 Nitpick comments (6)
tests/unit/test_logging_helpers.py (1)
10-20: Suggest additional test cases for comprehensive coverage.The current tests only cover empty queries and queries without parameters. Consider adding tests for:
- Queries with sensitive keys that should be redacted (e.g.,
"token=secret123"→"token=REDACTED")- Queries with mixed sensitive and non-sensitive keys (e.g.,
"name=test&token=secret"→"name=test&token=REDACTED")- Queries with multiple sensitive keys
- Edge cases like queries with blank values
This would ensure the redaction logic is fully validated.
🧪 Proposed additional test cases
Add these test methods to the
TestRedactQueryclass:def test_redact_query_redacts_sensitive_keys(self) -> None: """Test redaction of sensitive query parameters.""" assert "token=REDACTED" in LoggingMiddleware._redact_query("token=secret123") assert "api_key=REDACTED" in LoggingMiddleware._redact_query("api_key=mykey") def test_redact_query_preserves_non_sensitive_keys(self) -> None: """Test non-sensitive parameters are preserved.""" result = LoggingMiddleware._redact_query("name=test&page=1") assert "name=test" in result assert "page=1" in result def test_redact_query_handles_mixed_keys(self) -> None: """Test redaction with both sensitive and non-sensitive parameters.""" result = LoggingMiddleware._redact_query("name=test&token=secret&page=1") assert "name=test" in result assert "token=REDACTED" in result assert "page=1" in resultpyproject.toml (1)
244-244: Consider advocating for slowapi to switch toinspect.iscoroutinefunctionto silence the deprecation warning.Slowapi (latest release 0.1.9 in Feb 2024) is not actively maintained with infrequent releases and sporadic development activity. The deprecation warning for
asyncio.iscoroutinefunctionis a known issue—slowapi has a PR to switch toinspect.iscoroutinefunction(the CPython-recommended replacement), as other projects like Starlette have already done. While filtering the warning is a valid interim workaround, consider opening an issue or reaching out to maintainers to prioritize this fix, since the slow release cycle means it may remain unresolved for some time.tests/unit/test_rate_limit_helpers.py (1)
6-41: Good test coverage for the parsing helper.The tests cover the main formats and edge cases well. Consider adding a test for empty string input to ensure robustness:
💡 Optional: Add edge case for empty string
def test_parse_empty_string(self) -> None: """Test empty string returns default.""" assert parse_retry_after("") == "3600"docs/usage/rate-limiting.md (1)
531-548: Dynamic rate limits example is incomplete.The example shows the decorator structure but doesn't actually apply rate limiting—the comment
# Apply limit dynamicallyhas no implementation. This could mislead users.Consider either completing the example with actual dynamic limit application (which would require custom slowapi integration) or adding a note that this is a conceptual pattern requiring additional implementation.
app/rate_limit/handlers.py (2)
10-50: Extract default retry value as a constant.The hardcoded value
"3600"appears four times (lines 25, 33, 48, 62). Consider extracting it as a module-level constant for better maintainability.♻️ Proposed refactor
Add at the top of the file after imports:
# Default retry-after period (1 hour in seconds) DEFAULT_RETRY_AFTER = "3600"Then replace the hardcoded values:
def parse_retry_after(limit_str: str) -> str: """Parse rate limit string and return Retry-After seconds. Args: limit_str: Rate limit string like "3 per 1 hour" or "5/15minutes" Returns: Retry-After value in seconds as string """ # Handle both "X per Y period" and "X/Y period" formats limit_str = limit_str.replace(" per ", "/") # Extract the time period parts = limit_str.split("/") if len(parts) < 2: # noqa: PLR2004 - return "3600" # Default to 1 hour + return DEFAULT_RETRY_AFTER period_str = parts[1].strip().lower() # Parse period to seconds # Extract number prefix if present (e.g., "15minutes" -> 15) match = re.match(r"(\d+)?\s*(\w+)", period_str) if not match: - return "3600" + return DEFAULT_RETRY_AFTER multiplier_str, unit = match.groups() multiplier = int(multiplier_str) if multiplier_str else 1 # Convert unit to seconds if unit.startswith("second"): seconds = 1 elif unit.startswith("minute"): seconds = 60 elif unit.startswith("hour"): seconds = 3600 elif unit.startswith("day"): seconds = 86400 else: - seconds = 3600 # Default to hour + seconds = 3600 # Default to hour (fallback within conversion) return str(multiplier * seconds)And in the handler:
async def rate_limit_handler( request: Request, # noqa: ARG001 - Required by FastAPI exc: RateLimitExceeded, ) -> JSONResponse: """Handle rate limit exceeded exceptions. Returns a 429 status with Retry-After header. """ # Calculate Retry-After from limit string - retry_after = "3600" # Default to 1 hour + retry_after = DEFAULT_RETRY_AFTER if exc.limit is not None: limit_str = str(exc.limit.limit) retry_after = parse_retry_after(limit_str)
38-49: Consider using a dict for unit-to-seconds mapping.The if-elif chain for unit conversion could be replaced with a dictionary lookup for better maintainability and readability.
♻️ Proposed refactor
+ # Unit to seconds mapping + UNIT_TO_SECONDS = { + "second": 1, + "minute": 60, + "hour": 3600, + "day": 86400, + } + multiplier_str, unit = match.groups() multiplier = int(multiplier_str) if multiplier_str else 1 - # Convert unit to seconds - if unit.startswith("second"): - seconds = 1 - elif unit.startswith("minute"): - seconds = 60 - elif unit.startswith("hour"): - seconds = 3600 - elif unit.startswith("day"): - seconds = 86400 - else: - seconds = 3600 # Default to hour + # Find matching unit (using startswith for flexibility) + seconds = next( + (v for k, v in UNIT_TO_SECONDS.items() if unit.startswith(k)), + 3600 # Default to hour if no match + ) return str(multiplier * seconds)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.env.exampleREADME.mdSECURITY-REVIEW.mdTODO.mdapp/config/settings.pyapp/main.pyapp/metrics/__init__.pyapp/metrics/custom.pyapp/rate_limit/__init__.pyapp/rate_limit/config.pyapp/rate_limit/decorators.pyapp/rate_limit/handlers.pyapp/resources/auth.pydocs/index.mddocs/usage/caching.mddocs/usage/rate-limiting.mdmkdocs.ymlpyproject.tomlrequirements-dev.txtrequirements.txttests/integration/test_rate_limit_metrics.pytests/integration/test_rate_limit_redis.pytests/integration/test_rate_limiting.pytests/unit/test_logging.pytests/unit/test_logging_helpers.pytests/unit/test_rate_limit_config.pytests/unit/test_rate_limit_helpers.pytests/unit/test_rate_limit_init.py
💤 Files with no reviewable changes (1)
- tests/unit/test_logging.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-01T19:57:45.447Z
Learnt from: seapagan
Repo: seapagan/fastapi-template PR: 803
File: requirements.txt:164-165
Timestamp: 2026-01-01T19:57:45.447Z
Learning: When using fastapi-cache2 (versions <= 0.2.2), ensure Redis is constrained to <5.0.0. Do not upgrade Redis to 5.x or 7.x as it is incompatible. In this repository, pin Redis to a compatible 4.x version (e.g., redis==4.6.0 or redis<5.0.0) in requirements.txt to maintain compatibility with fastapi-cache2.
Applied to files:
requirements.txt
🧬 Code graph analysis (11)
app/rate_limit/decorators.py (4)
app/config/log_config.py (1)
LogCategory(13-28)app/metrics/custom.py (1)
increment_rate_limit_exceeded(60-66)app/rate_limit/__init__.py (1)
get_limiter(19-66)app/logs.py (1)
warning(36-39)
app/metrics/custom.py (2)
app/config/settings.py (1)
get_settings(267-269)tests/integration/test_metrics_endpoint.py (1)
test_auth_failure_metrics(354-380)
tests/integration/test_rate_limit_redis.py (2)
app/config/settings.py (2)
Settings(30-263)rate_limit_storage_url(169-181)tests/unit/test_validation.py (1)
TestRedisUrlProperty(199-269)
tests/unit/test_rate_limit_init.py (2)
app/config/settings.py (1)
redis_url(151-166)app/rate_limit/__init__.py (1)
get_limiter(19-66)
app/main.py (1)
app/rate_limit/handlers.py (1)
rate_limit_handler(53-71)
tests/unit/test_rate_limit_config.py (1)
app/rate_limit/config.py (1)
RateLimits(6-33)
tests/unit/test_logging_helpers.py (1)
app/middleware/logging_middleware.py (2)
LoggingMiddleware(21-101)_redact_query(48-69)
tests/unit/test_rate_limit_helpers.py (1)
app/rate_limit/handlers.py (1)
parse_retry_after(10-50)
app/metrics/__init__.py (1)
app/metrics/custom.py (1)
increment_rate_limit_exceeded(60-66)
app/rate_limit/__init__.py (3)
app/config/settings.py (2)
get_settings(267-269)redis_url(151-166)app/config/log_config.py (1)
LogCategory(13-28)app/logs.py (1)
info(26-29)
app/resources/auth.py (3)
app/rate_limit/config.py (1)
RateLimits(6-33)app/rate_limit/decorators.py (1)
rate_limited(14-81)app/database/db.py (1)
get_database(78-81)
🪛 LanguageTool
docs/usage/caching.md
[uncategorized] ~217-~217: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...mited()` decorators together, place rate limiting first for security. Rate limits should ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
SECURITY-REVIEW.md
[grammar] ~518-~518: Use a hyphen to join words.
Context: ...ints. Next enhancement: add user/API-key based limiting for authenticated API o...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~652-~652: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...condition (#11) - ✅ app/rate_limit/ - Rate limiting implementation (#3) High Priority:...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
docs/usage/rate-limiting.md
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...rute force attacks, abuse, and denial of service attempts. Rate limiting is **dis...
(QB_NEW_EN_HYPHEN)
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...ntaining good user experience. The rate limiting system supports two backends: ...
(QB_NEW_EN_HYPHEN)
[grammar] ~76-~76: Use a hyphen to join words.
Context: ...ate limiting. ## How It Works ### Rate Limiting Behavior The rate limiting sys...
(QB_NEW_EN_HYPHEN)
[grammar] ~78-~78: Use a hyphen to join words.
Context: ...ks ### Rate Limiting Behavior The rate limiting system has three operating mode...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~95-~95: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: .... Automatically falls back to in-memory rate limiting 3. Application continues functioning no...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~149-~149: Use a hyphen to join words.
Context: ...s whether to use Redis or in-memory rate limiting backend. Only relevant when `RA...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~224-~224: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ached()` decorators together, place rate limiting first for security. Rate limits should ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~281-~281: Use a hyphen to join words.
Context: ...0/day"` - 1000 requests per day ## Rate Limiting Strategy ### IP-Based Limiting...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~430-~430: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...k) - When Disabled: Zero overhead - rate limiting code is not executed ### Redis Memor...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~529-~529: Use a hyphen to join words.
Context: ... ### Dynamic Rate Limits For user-tier based limits, consider creating a custom...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.14)
🔇 Additional comments (44)
pyproject.toml (1)
37-37: LGTM - slowapi dependency added.The addition of slowapi for rate limiting functionality is appropriate.
requirements-dev.txt (1)
184-185: LGTM - Dependencies correctly added.The slowapi (0.1.9) and limits (5.6.0) dependencies have been properly added to the auto-generated requirements file. The pinned versions ensure reproducible builds.
Also applies to: 399-400
README.md (1)
131-136: LGTM - Clear and comprehensive documentation.The rate limiting feature documentation is well-written and covers all essential aspects:
- Opt-in nature (disabled by default)
- Backend options (in-memory for single-instance, Redis for multi-instance)
- Conservative default limits with specific examples
- HTTP 429 responses with Retry-After header
- Prometheus metrics integration
This provides users with a clear understanding of the feature's capabilities and behavior.
app/config/settings.py (2)
141-144: LGTM - Rate limiting configuration follows established patterns.The
rate_limit_enabledsetting is appropriately:
- Disabled by default (opt-in)
- Documented with clear comments
- Consistent with other opt-in features (cache_enabled, metrics_enabled)
- Designed to work in conjunction with redis_enabled
168-181: LGTM - Storage URL property implements correct backend selection.The
rate_limit_storage_urlproperty correctly:
- Returns Redis URL when both rate_limit_enabled and redis_enabled are True
- Returns empty string for in-memory storage (when either flag is False)
- Includes comprehensive docstring explaining the behavior
- Follows the same pattern as the existing redis_url property
This design cleanly separates Redis-backed rate limiting from in-memory rate limiting.
.env.example (1)
153-160: LGTM! Clear and well-documented rate limiting configuration.The opt-in design with sensible defaults and clear documentation of Redis integration is excellent. The reuse of existing Redis configuration keeps setup simple.
app/metrics/__init__.py (1)
7-7: LGTM! Correct export of new metric function.The import and all entry properly expose the new rate limit metric function, following the established pattern for other metrics.
Also applies to: 18-18
requirements.txt (2)
171-172: Redis version correctly maintained for fastapi-cache2 compatibility.Good—redis==4.6.0 remains within the required constraint (redis<5.0.0) for fastapi-cache2<=0.2.2 compatibility. Based on learnings, this is correct.
53-54: No action needed—slowapi version is current and secure.The slowapi version 0.1.9 used in the project is the latest stable release (Feb 5, 2024) and has no known security vulnerabilities, security advisories, or CVEs recorded in major vulnerability databases (Safety DB, Snyk).
Likely an incorrect or invalid review comment.
app/metrics/custom.py (2)
32-38: LGTM! Rate limit counter follows established metrics pattern.The Counter definition is well-structured with appropriate labels (endpoint, limit) for granular tracking of rate limit violations.
59-66: LGTM! Increment function correctly implements metrics guard pattern.The function properly follows the existing pattern of checking metrics_enabled before incrementing, ensuring zero overhead when metrics are disabled.
app/rate_limit/config.py (1)
6-33: Rate limit string formats are intentionally supported and properly validated.The format validation in
tests/unit/test_rate_limit_config.py(lines 21-32) explicitly accepts"5/15minutes"format. The test validator accepts period names in["second", "minute", "hour", "day", "minutes", "hours"]OR any period ending with"minutes"(line 32), which intentionally supports custom time periods like"15minutes". The testtest_conservative_limits(line 37) confirmsLOGIN == "5/15minutes"is the expected value. The decorator passes all rate limit strings directly to slowapi'slimiter.limit()method, which handles these formats correctly. Mixed singular/plural period names are intentionally supported.Likely an incorrect or invalid review comment.
SECURITY-REVIEW.md (2)
54-76: LGTM! Thorough documentation of rate limiting implementation.The documentation accurately reflects the implementation details: slowapi integration, dual backend support (in-memory and Redis), conservative per-endpoint limits, HTTP 429 responses with Retry-After headers, and Prometheus metrics tracking. The checkmarks on each specific endpoint provide clear visibility into coverage.
517-547: Excellent future enhancement planning.The detailed feature idea for user-based rate limiting (in addition to the IP-based implementation) is well thought out. The implementation approach, code examples, and technical considerations provide a clear roadmap for when this enhancement is prioritized.
TODO.md (1)
7-8: LGTM!The TODO item accurately reflects the current implementation (IP-based rate limiting) and documents the next enhancement (user-based quotas).
mkdocs.yml (1)
82-82: LGTM!The navigation entry is correctly added and properly formatted.
docs/index.md (1)
80-85: LGTM!The feature description accurately documents the rate limiting implementation with all key details: opt-in nature, dual backend support, specific limits, and HTTP 429 responses with Retry-After headers.
docs/usage/caching.md (1)
215-231: Excellent security guidance on decorator order.This tip provides important security advice: rate limiting should be checked before caching to prevent users from bypassing rate limits via cached responses. The code example clearly demonstrates the correct pattern with rate_limited applied first.
app/main.py (2)
167-171: LGTM! Exception handler registration looks correct.The
cast("Any", rate_limit_handler)works around FastAPI's exception handler type expectations. The handler signature inapp/rate_limit/handlers.pycorrectly acceptsRequestandRateLimitExceededand returnsJSONResponse.
204-208: LGTM! Middleware and state setup is correctly unconditional.The comment clearly explains why
app.state.limiterandSlowAPIMiddlewareare always registered—the limiter itself handles the disabled state internally viaenabled=False. This avoids conditional middleware registration complexity.tests/integration/test_rate_limit_redis.py (1)
8-45: LGTM! Good coverage of the storage URL property logic.The tests correctly verify all three branches of
rate_limit_storage_url:
- Both flags enabled → Redis URL
- Rate limiting enabled, Redis disabled → empty string
- Rate limiting disabled → empty string
Direct
Settingsinstantiation is a clean approach for testing computed properties.tests/integration/test_rate_limit_metrics.py (1)
22-24: Monkeypatching cached settings may not affect already-initialized components.The
limitersingleton inapp/rate_limit/__init__.pyis created at import time. Patchingsettings.rate_limit_enabledafter import won't reinitialize the limiter. This may explain why the test uses a soft assertion.Consider whether the test needs to force limiter reinitialization, or if a dedicated test fixture that patches settings before app initialization would be more reliable.
app/rate_limit/decorators.py (1)
41-45: Limiter is resolved at decoration time, not request time.
get_limiter()is called when the decorator is applied (module import), which means the limiter configuration is locked in at startup. This is likely intentional for performance, but worth noting that runtime setting changes won't affect already-decorated endpoints.app/rate_limit/__init__.py (2)
19-66: LGTM! Clean singleton implementation with appropriate backend selection.The
get_limiter()function correctly:
- Uses singleton pattern to avoid re-initialization
- Selects Redis backend only when both
rate_limit_enabledandredis_enabledare true- Falls back to
memory://for in-memory storage- Logs initialization state under the AUTH category
- Respects the
enabledflag to allow zero-overhead when disabled
69-70: Module-level initialization locks configuration at import time.
limiter = get_limiter()executes at module import, so settings are read once at startup. This is appropriate for the use case but means configuration changes require a restart.docs/usage/rate-limiting.md (1)
1-560: Comprehensive and well-structured documentation.The documentation covers all aspects of the rate limiting feature: configuration options, usage patterns, security considerations, troubleshooting, and advanced usage. The examples are practical and the explanations are clear.
tests/unit/test_rate_limit_init.py (2)
12-34: LGTM! Good test isolation.The test properly resets the cached limiter to force re-initialization and verifies the expected log message. The use of
mocker.ANYfor the LogCategory parameter is acceptable here.
36-57: LGTM! Consistent test pattern.The test follows the same pattern as the Redis test, ensuring consistent coverage of both initialization paths.
app/rate_limit/handlers.py (1)
53-71: LGTM! Proper exception handling and response structure.The handler correctly returns a 429 response with appropriate headers and falls back gracefully when limit information is unavailable.
tests/unit/test_rate_limit_config.py (1)
34-42: LGTM! Exact value validation ensures security requirements.The test properly validates that the rate limits match the conservative values specified in SECURITY-REVIEW.md.
tests/integration/test_rate_limiting.py (6)
14-26: LGTM! Proper test isolation.The autouse fixture correctly resets rate limiter storage between tests to ensure test independence.
33-72: LGTM! Comprehensive rate limit testing.The test properly validates the 3/hour limit on registration and verifies both successful and rate-limited responses.
74-114: LGTM! Handles authentication failure scenarios.The test correctly accepts both authentication failures (400, 401) and success (200) as valid non-rate-limited responses, ensuring the rate limiter is tested independently of authentication logic.
116-144: LGTM! Tests forgot-password rate limiting.The test validates the 3/hour limit on password recovery to prevent email flooding.
146-185: LGTM! Validates Retry-After header presence.The test ensures rate-limited responses include the required Retry-After header using case-insensitive lookup.
187-226: LGTM! Verifies user-friendly error messages.The test confirms that rate limit errors include appropriate user-facing messaging.
app/resources/auth.py (8)
29-30: LGTM! Proper imports for rate limiting.The imports are correctly added for the rate limiting functionality.
58-64: LGTM! Rate limiting applied to registration.The
@rate_limiteddecorator is correctly applied with the REGISTER limit, and the Request parameter is properly added with ARG001 suppression since slowapi requires it but the endpoint doesn't use it directly.
90-95: LGTM! Rate limiting applied to login.The decorator and Request parameter are correctly added to protect against brute-force attacks.
114-119: LGTM! Rate limiting applied to token refresh.The decorator properly protects the refresh endpoint from token harvesting attempts.
130-135: LGTM! Rate limiting applied to verification.The decorator correctly limits email verification attempts to prevent abuse.
152-158: LGTM! Rate limiting applied to forgot-password.The decorator properly prevents email flooding through the password recovery endpoint.
179-184: LGTM! Rate limiting applied to password reset form.The decorator is correctly applied. Note that this endpoint already had a Request parameter (line 181), so no ARG001 suppression is added since the Request is used in the endpoint logic.
252-256: LGTM! Rate limiting applied to password reset POST.The decorator protects the critical password reset operation. The Request parameter (line 254) was already present and is used throughout the endpoint, so no ARG001 suppression is needed.
- Add null check for request.client to prevent AttributeError - Remove weak assertion fallback that always passed in metrics test - Fix period validation regex to properly handle compound periods
- Extract DEFAULT_RETRY_AFTER constant to eliminate hardcoded values - Refactor unit conversion from if-elif chain to dictionary lookup - Add comprehensive query redaction tests for all edge cases - Add empty string edge case test for retry-after parsing - Clarify dynamic rate limits documentation with implementation notes
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unit/test_rate_limit_config.py (1)
11-30: Solid format validation with correct regex for compound periods.The regex pattern correctly handles both simple periods ("hour") and compound periods ("15minutes"), which aligns with the commit message about strengthening period validation.
💡 Optional: Add semantic validation for count > 0
While not critical for config tests, you could optionally add a check that the parsed count is greater than zero to catch semantically invalid values like "0/hour":
for limit in limits: assert "/" in limit count, period = limit.split("/") assert count.isdigit() + assert int(count) > 0, f"Count must be positive: {limit}" # Period: [number]unit (second/minute/hour/day + optional 's') assert re.match( r"^\d*\s*(seconds?|minutes?|hours?|days?)$", period ), f"Invalid period format: {period}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
app/rate_limit/decorators.pyapp/rate_limit/handlers.pydocs/usage/rate-limiting.mdtests/integration/test_rate_limit_metrics.pytests/unit/test_logging_helpers.pytests/unit/test_rate_limit_config.pytests/unit/test_rate_limit_helpers.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/unit/test_rate_limit_helpers.py
- tests/integration/test_rate_limit_metrics.py
- tests/unit/test_logging_helpers.py
- app/rate_limit/decorators.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_rate_limit_config.py (1)
app/rate_limit/config.py (1)
RateLimits(6-33)
🪛 LanguageTool
docs/usage/rate-limiting.md
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...rute force attacks, abuse, and denial of service attempts. Rate limiting is **dis...
(QB_NEW_EN_HYPHEN)
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...ntaining good user experience. The rate limiting system supports two backends: ...
(QB_NEW_EN_HYPHEN)
[grammar] ~76-~76: Use a hyphen to join words.
Context: ...ate limiting. ## How It Works ### Rate Limiting Behavior The rate limiting sys...
(QB_NEW_EN_HYPHEN)
[grammar] ~78-~78: Use a hyphen to join words.
Context: ...ks ### Rate Limiting Behavior The rate limiting system has three operating mode...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~95-~95: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: .... Automatically falls back to in-memory rate limiting 3. Application continues functioning no...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~149-~149: Use a hyphen to join words.
Context: ...s whether to use Redis or in-memory rate limiting backend. Only relevant when `RA...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~224-~224: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ached()` decorators together, place rate limiting first for security. Rate limits should ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~281-~281: Use a hyphen to join words.
Context: ...0/day"` - 1000 requests per day ## Rate Limiting Strategy ### IP-Based Limiting...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~431-~431: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...k) - When Disabled: Zero overhead - rate limiting code is not executed ### Redis Memor...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~536-~536: Use a hyphen to join words.
Context: ...omplex dynamic scenarios. For user-tier based limits, you could structure a cust...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
- GitHub Check: test (3.11)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
🔇 Additional comments (6)
tests/unit/test_rate_limit_config.py (2)
1-10: LGTM! Clean test structure.The imports and test class setup are well-organized with appropriate dependencies for validating rate limit configuration.
32-40: Excellent! Enforces security requirements.These exact-value assertions ensure rate limits remain aligned with the security review requirements and prevent unintended configuration drift. The
noqa: S105comments appropriately suppress false positives from bandit.app/rate_limit/handlers.py (3)
1-19: LGTM! Clean module structure.The imports, constants, and unit mapping are well-organized. The 1-hour default is a reasonable fallback.
21-56: LGTM! Robust parsing with appropriate fallbacks.The function correctly handles multiple format variations and gracefully falls back to the default on invalid input. The use of
startswithat line 51 provides flexibility for plural forms (e.g., "minute" matches "minutes").
58-76: LGTM! Proper exception handling with security-conscious response.The handler correctly extracts the limit, parses it for Retry-After, and returns a generic error message (avoiding information leakage about specific limits).
docs/usage/rate-limiting.md (1)
1-575: LGTM! Comprehensive and technically accurate documentation.The documentation is well-structured, provides clear examples, and accurately describes the rate limiting implementation. Key strengths:
- Clear instructions for both in-memory and Redis setups
- Code examples are correct and include necessary parameters (e.g.,
request: Request)- Security section is balanced, listing both protections and limitations
- Honest about conceptual examples (lines 530-534) that require additional implementation
- Comprehensive troubleshooting guidance
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/test_rate_limit_config.py (2)
11-31: Consider adding a completeness check to catch missing constants.The format validation logic is sound and correctly handles slowapi's format (e.g., "15minutes", "hour"). However, if a new rate limit constant is added to
RateLimitsbut forgotten here, the test will still pass.♻️ Optional: Add a completeness test
You could add a test method that uses introspection to ensure all
RateLimitsconstants are covered:def test_all_rate_limits_tested(self) -> None: """Ensure all RateLimits constants are tested.""" from typing import get_type_hints # Get all ClassVar attributes from RateLimits rate_limit_attrs = [ attr for attr in dir(RateLimits) if not attr.startswith('_') and isinstance(getattr(RateLimits, attr), str) ] tested_limits = [ RateLimits.REGISTER, RateLimits.LOGIN, RateLimits.FORGOT_PASSWORD, RateLimits.VERIFY, RateLimits.REFRESH, RateLimits.RESET_PASSWORD_GET, RateLimits.RESET_PASSWORD_POST, ] assert len(rate_limit_attrs) == len(tested_limits), ( f"Mismatch: {len(rate_limit_attrs)} constants defined but {len(tested_limits)} tested" )
33-41: LGTM! All assertions correctly validate the documented limits.The exact value assertions match the constants in
app/rate_limit/config.pyand align with SECURITY-REVIEW.md requirements.Minor note: The
# noqa: S105comments (lines 37, 40-41) suppress hardcoded password warnings, but these are just string literals—not actual secrets. They're harmless but possibly unnecessary.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unit/test_rate_limit_config.py
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unit/test_rate_limit_config.py (1)
app/rate_limit/config.py (1)
RateLimits(6-33)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
- GitHub Check: test (3.14)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
tests/unit/test_rate_limit_config.py (1)
1-10: LGTM! Clean test file structure.The imports and test class setup follow best practices with clear documentation.
Replace hardcoded list with dynamic discovery of RateLimits constants. This ensures new rate limit constants are automatically tested without manual updates to the test file.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/usage/rate-limiting.md (2)
7-7: Address hyphenation in compound adjectives and nouns.Several compound adjectives and nouns require hyphens for proper English grammar when modifying following nouns or used in specific contexts. This improves readability and professionalism.
📝 Proposed grammar fixes
- denial of service attempts + denial-of-service attempts - The rate limiting system supports two backends: + The rate-limiting system supports two backends: - ### Rate Limiting Behavior + ### Rate-Limiting Behavior - The rate limiting system has three operating modes: + The rate-limiting system has three operating modes: - Automatically falls back to in-memory rate limiting + Automatically falls back to in-memory rate-limiting - Controls whether to use Redis or in-memory rate limiting backend + Controls whether to use Redis or in-memory rate-limiting backend - place rate limiting first for security + place rate-limiting first for security - ## Rate Limiting Strategy + ## Rate-Limiting Strategy - **When Disabled**: Zero overhead - rate limiting code is not + **When Disabled**: Zero overhead - rate-limiting code is not - For user-tier based limits, you could structure a custom + For user-tier-based limits, you could structure a customAlso applies to: 12-12, 76-78, 95-95, 149-149, 224-224, 281-281, 431-431, 536-536
282-292: Clarify IP-based limiting caveats for documentation clarity.The IP-based limiting explanation (lines 285-291) correctly identifies shared network issues, but consider adding a note about X-Forwarded-For headers for deployments behind trusted proxies. While line 499 mentions this briefly, it could be more prominent in this core section for users diagnosing why shared-network users hit limits quickly.
Consider expanding line 289 with:
Note: If your application is behind a reverse proxy (nginx, CloudFlare, AWS ALB), configure slowapi to respect X-Forwarded-For headers to track individual clients correctly rather than all users' shared proxy IP.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/usage/rate-limiting.md
🧰 Additional context used
🪛 LanguageTool
docs/usage/rate-limiting.md
[grammar] ~7-~7: Use a hyphen to join words.
Context: ...rute force attacks, abuse, and denial of service attempts. Rate limiting is **dis...
(QB_NEW_EN_HYPHEN)
[grammar] ~12-~12: Use a hyphen to join words.
Context: ...ntaining good user experience. The rate limiting system supports two backends: ...
(QB_NEW_EN_HYPHEN)
[grammar] ~76-~76: Use a hyphen to join words.
Context: ...ate limiting. ## How It Works ### Rate Limiting Behavior The rate limiting sys...
(QB_NEW_EN_HYPHEN)
[grammar] ~78-~78: Use a hyphen to join words.
Context: ...ks ### Rate Limiting Behavior The rate limiting system has three operating mode...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~95-~95: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: .... Automatically falls back to in-memory rate limiting 3. Application continues functioning no...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~149-~149: Use a hyphen to join words.
Context: ...s whether to use Redis or in-memory rate limiting backend. Only relevant when `RA...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~224-~224: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...ached()` decorators together, place rate limiting first for security. Rate limits should ...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~281-~281: Use a hyphen to join words.
Context: ...0/day"` - 1000 requests per day ## Rate Limiting Strategy ### IP-Based Limiting...
(QB_NEW_EN_HYPHEN)
[uncategorized] ~431-~431: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...k) - When Disabled: Zero overhead - rate limiting code is not executed ### Redis Memor...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~536-~536: Use a hyphen to join words.
Context: ...omplex dynamic scenarios. For user-tier based limits, you could structure a cust...
(QB_NEW_EN_HYPHEN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
- GitHub Check: test (3.13)
- GitHub Check: test (3.14)
🔇 Additional comments (2)
docs/usage/rate-limiting.md (2)
189-192: Clarify rate limiting is not automatically enabled in tests.The documentation states tests "automatically enable rate limiting," but the actual test implementation shows rate limiting is disabled by default (
rate_limit_enabled=False) and must be explicitly enabled per test usingmonkeypatch.setattr(reset_rate_limiter, "enabled", True). The in-memory storage backend claim is correct—when enabled, tests usememory://storage and do not require Redis. Update the note to reflect that rate limiting requires manual per-test enabling rather than automatic enablement.
196-220: Document that the decorator gracefully degrades without therequestparameter rather than failing with clear error messages.The implementation applies a fallback mechanism when the
request: Requestparameter is missing—it attempts to extract the request from kwargs or args, and defaults the client IP to"unknown"if not found. This means violations of the "MUST include arequest: Requestparameter" requirement won't be caught with validation errors or helpful error messages. While all production endpoints correctly include the parameter, developers won't receive immediate feedback if they omit it, leading to silent degradation of rate limiting accuracy (requests would be grouped under an"unknown"IP instead of the actual client IP). Consider adding runtime validation in the decorator to raise a clear error if the request parameter is missing or cannot be resolved, ensuring users catch configuration mistakes early rather than discovering subtle rate-limiting issues later.
Add RATE_LIMIT_ENABLED=false to pytest env to prevent user's .env from affecting tests. Rate limiting integration tests explicitly enable it via monkeypatch when needed.
Summary
Implements comprehensive rate limiting for all authentication endpoints to protect against brute force attacks, spam registration, and abuse. Addresses security review item #3 from SECURITY-REVIEW.md.
Key Features
Retry-Afterheader indicating reset timeImplementation Details
New Modules
app/rate_limit/config.py- Conservative rate limit definitionsapp/rate_limit/__init__.py- Limiter initialization with backend selectionapp/rate_limit/decorators.py- @rate_limited() decorator with logging/metricsapp/rate_limit/handlers.py- Exception handler and Retry-After calculationModified Files
app/main.py- Register rate limit exception handler and middlewareapp/resources/auth.py- Apply rate limiting to all 7 auth endpointsapp/config/settings.py- Add RATE_LIMIT_ENABLED configurationapp/metrics/custom.py- Add rate_limit_exceeded_total metric.env.example- Document RATE_LIMIT_ENABLED settingArchitecture
get_remote_addressDocumentation
docs/usage/rate-limiting.md(541 lines)Breaking Changes
None. Rate limiting is disabled by default and completely opt-in.
Configuration
Testing the PR
Enable rate limiting:
Start the server:
Test rate limiting on login endpoint:
Check for
Retry-Afterheader in 429 responseView metrics (if enabled):
curl http://localhost:8000/metrics | grep rate_limitMigration Notes
No migration needed. Feature is opt-in and backward compatible.
Summary by CodeRabbit
New Features
Configuration
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.