Feature/rate limiter #134
Feature/rate limiter #134
Conversation
|
Removed NGINX rate-limiter logic; kept only SlowAPI rate limiting in this commit |
There was a problem hiding this comment.
Pull request overview
This PR implements a comprehensive rate limiting solution for FastAPI endpoints using slowapi, providing layered protection alongside Nginx-level rate limiting to defend against repeated API attacks.
Key Changes:
- Added
slowapi==0.1.9dependency for application-level rate limiting - Created
lenny/core/ratelimit.pymodule with configurable rate limits (general: 100/min, strict: 20/min, lenient: 300/min) - Applied rate limit decorators to 9 endpoints with endpoint-specific limits, while keeping OPDS and
/itemsendpoints unrestricted
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| requirements.txt | Added slowapi==0.1.9 dependency for rate limiting functionality |
| lenny/core/ratelimit.py | New module implementing rate limit configuration with environment variable support and FastAPI integration |
| lenny/app.py | Integrated rate limiter initialization into the FastAPI application startup |
| lenny/routes/api.py | Added Request parameters to endpoints and applied rate limit decorators to 9 endpoints (general limits on read/borrow/return operations, strict limits on upload/authenticate) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def _parse_rate_limit(env_var: str, default: int) -> int: | ||
| """ | ||
| Parse rate limit from environment variable. | ||
| Supports both integer format (100) and string format ('100/minute'). | ||
| Returns the integer count of requests. | ||
| """ | ||
| value = os.environ.get(env_var, str(default)) | ||
| if '/' in str(value): | ||
| return int(value.split('/')[0]) | ||
| return int(value) |
There was a problem hiding this comment.
The _parse_rate_limit function lacks a return type annotation. Consider adding -> int to the function signature for consistency with type hints and improved code clarity.
| # Create the limiter instance | ||
| limiter = Limiter( | ||
| key_func=get_remote_address, | ||
| default_limits=[RATE_LIMIT_GENERAL], |
There was a problem hiding this comment.
[nitpick] The limiter is configured with default_limits=[RATE_LIMIT_GENERAL], which applies the general rate limit (100/minute) to all endpoints by default. However, based on the PR description, OPDS endpoints and /items should have no rate limiting. Consider whether this default should be removed (set to []) to explicitly require rate limits on each endpoint, preventing unintended rate limiting on endpoints that should be unrestricted.
| default_limits=[RATE_LIMIT_GENERAL], | |
| default_limits=[], |
| ) | ||
| from lenny.core.readium import ReadiumAPI | ||
| from lenny.core.models import Item | ||
| from lenny.core.ratelimit import limiter, RATE_LIMIT_GENERAL, RATE_LIMIT_LENIENT, RATE_LIMIT_STRICT |
There was a problem hiding this comment.
RATE_LIMIT_LENIENT is imported but never used in this file. Consider removing it from the import statement to keep the code clean and avoid confusion.
| from lenny.core.ratelimit import limiter, RATE_LIMIT_GENERAL, RATE_LIMIT_LENIENT, RATE_LIMIT_STRICT | |
| from lenny.core.ratelimit import limiter, RATE_LIMIT_GENERAL, RATE_LIMIT_STRICT |
|
|
||
| # Rate limit window in seconds - must be compatible between nginx and slowapi | ||
| # Using 60 seconds (1 minute) as a standard window | ||
| RATE_LIMIT_WINDOW = int(os.environ.get('RATE_LIMIT_WINDOW', 60)) |
There was a problem hiding this comment.
RATE_LIMIT_WINDOW is defined but never used anywhere in this module or imported elsewhere. Consider removing it or documenting its intended use case if it's meant for future Nginx configuration compatibility.
| RATE_LIMIT_WINDOW = int(os.environ.get('RATE_LIMIT_WINDOW', 60)) |
2 participants
Closes #118
Rate Limiter for FastAPI Endpoints
Implements comprehensive rate limiting using Nginx
limit_reqandslowapito protect the API from overloaded repeated attacks.Key Features
/items: No rate limiting (as requested)/upload&/authenticate: Strict (20/min)Changes
slowapi==0.1.9dependencylenny/core/ratelimit.pyconfiguration moduleTesting Results
OPDS endpoints: 20/20 requests passed (no rate limiting)
/items endpoint: 20/20 requests passed (no rate limiting)
General endpoints: 11 requests passed, then 503 (rate limited)
Strict endpoints: 8 requests passed, then 503 (rate limited)
Verification
PTAL @ronibhakta1