-
Notifications
You must be signed in to change notification settings - Fork 0
feat: add API middleware for CORS, security, rate limiting - PR-6 #154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| from functools import wraps | ||
| from flask import request, jsonify | ||
|
|
||
| def require_api_key(f): | ||
| @wraps(f) | ||
| def decorated_function(*args, **kwargs): | ||
| api_key = request.headers.get('X-API-Key') | ||
| if api_key != 'your-secret-key': # Replace with actual key or env var | ||
| return jsonify({'error': 'API key required'}), 401 | ||
| return f(*args, **kwargs) | ||
| return decorated_function | ||
|
Comment on lines
+4
to
+11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider rate-limiting failed auth attempts. If the rate limiter runs only after successful auth, attackers can spray invalid keys. Either reorder decorators at callsites (rate limit outermost) or add a lightweight per-IP failure counter here. 🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,21 @@ | ||||||||||||||||
| from collections import defaultdict | ||||||||||||||||
| from datetime import datetime, timedelta | ||||||||||||||||
| from flask import abort, current_app | ||||||||||||||||
|
|
||||||||||||||||
|
Comment on lines
+1
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing imports and unused import break runtime (F821) and linting.
Apply: from collections import defaultdict
from datetime import datetime, timedelta
-from flask import abort, current_app
+from flask import abort, request
+from functools import wraps📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||
| # Simple rate limiter using memory (use Redis for production) | ||||||||||||||||
| rate_limit = defaultdict(list) | ||||||||||||||||
|
|
||||||||||||||||
| def rate_limit(max_requests=100, window_minutes=1): | ||||||||||||||||
| def decorator(f): | ||||||||||||||||
|
Comment on lines
+5
to
+9
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Name collision: After defining the function, Apply: # Simple rate limiter using memory (use Redis for production)
-rate_limit = defaultdict(list)
+REQUEST_LOG = defaultdict(list)
def rate_limit(max_requests=100, window_minutes=1):
def decorator(f):
@wraps(f)
def decorated_function(*args, **kwargs):
- client_ip = request.remote_addr
+ # Be proxy-aware
+ forwarded_for = request.headers.get('X-Forwarded-For')
+ client_ip = forwarded_for.split(',')[0].strip() if forwarded_for else (request.remote_addr or 'unknown')
now = datetime.utcnow()
window_start = now - timedelta(minutes=window_minutes)
- rate_limit[client_ip] = [req_time for req_time in rate_limit[client_ip] if req_time > window_start]
- if len(rate_limit[client_ip]) >= max_requests:
+ REQUEST_LOG[client_ip] = [t for t in REQUEST_LOG[client_ip] if t > window_start]
+ if len(REQUEST_LOG[client_ip]) >= max_requests:
abort(429, description="Rate limit exceeded")
- rate_limit[client_ip].append(now)
+ REQUEST_LOG[client_ip].append(now)
return f(*args, **kwargs)Also applies to: 15-19 🧰 Tools🪛 Ruff (0.12.2)8-8: Redefinition of unused (F811) 🤖 Prompt for AI Agents |
||||||||||||||||
| @wraps(f) | ||||||||||||||||
| def decorated_function(*args, **kwargs): | ||||||||||||||||
| client_ip = request.remote_addr | ||||||||||||||||
| now = datetime.utcnow() | ||||||||||||||||
| window_start = now - timedelta(minutes=window_minutes) | ||||||||||||||||
| rate_limit[client_ip] = [req_time for req_time in rate_limit[client_ip] if req_time > window_start] | ||||||||||||||||
| if len(rate_limit[client_ip]) >= max_requests: | ||||||||||||||||
| abort(429, description="Rate limit exceeded") | ||||||||||||||||
| rate_limit[client_ip].append(now) | ||||||||||||||||
| return f(*args, **kwargs) | ||||||||||||||||
| return decorated_function | ||||||||||||||||
| return decorator | ||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,45 @@ | ||||||||||||||||||||||||
| from fastapi import Depends, HTTPException, status | ||||||||||||||||||||||||
| from fastapi.security import HTTPBearer, HTTPAuthorizationCredentials | ||||||||||||||||||||||||
| from jose import JWTError, jwt | ||||||||||||||||||||||||
| from passlib.context import CryptContext | ||||||||||||||||||||||||
| from datetime import datetime, timedelta | ||||||||||||||||||||||||
| from typing import Optional | ||||||||||||||||||||||||
|
|
||||||||||||||||||||||||
| # Security settings | ||||||||||||||||||||||||
| SECRET_KEY = "your-secret-key" # Should be loaded from config | ||||||||||||||||||||||||
|
Comment on lines
+7
to
+9
|
||||||||||||||||||||||||
| # Security settings | |
| SECRET_KEY = "your-secret-key" # Should be loaded from config | |
| import os | |
| # Security settings | |
| SECRET_KEY = os.environ.get("SECRET_KEY") | |
| if not SECRET_KEY: | |
| raise RuntimeError("SECRET_KEY environment variable not set") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚨 issue (security): Hardcoded secret key should be loaded securely from configuration.
Use environment variables or a secure config system to prevent exposure of sensitive keys.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoding secrets like SECRET_KEY is a critical security vulnerability. The comment indicates it should be loaded from config, but the implementation uses a hardcoded value. This secret must be loaded from a secure source, such as environment variables or a secret management service, to prevent unauthorized access and token forgery in production environments. You should import os and use os.getenv('SECRET_KEY'). The application should fail to start if the key is not set in production.
| SECRET_KEY = "your-secret-key" # Should be loaded from config | |
| SECRET_KEY = os.getenv("SECRET_KEY") # Should be loaded from config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Verify dependencies are declared.
Requires fastapi, python-jose, passlib[bcrypt]. Ensure they’re in requirements/pyproject.
🏁 Script executed:
#!/bin/bash
# Check for required deps
rg -nP '^(fastapi|python-jose|passlib(\[bcrypt\])?)' requirements* pyproject.toml setup.cfg setup.py || trueLength of output: 152
🏁 Script executed:
#!/bin/bash
# Locate dependency declaration files and verify required packages
dep_files=$(find . -maxdepth 2 -type f \( -name 'pyproject.toml' -o -name 'requirements.txt' \))
echo "Found files:"
echo "$dep_files"
for f in $dep_files; do
echo "---- Checking $f ----"
grep -nE 'fastapi|python-jose|passlib(\[bcrypt\])?' "$f" || echo "No required deps in $f"
doneLength of output: 265
Declare missing dependencies
python-jose and passlib[bcrypt] aren’t listed in your dependency files—add them to pyproject.toml (under [tool.poetry.dependencies]) or dependencies/requirements.txt alongside fastapi to ensure the authentication code will install correctly.
🤖 Prompt for AI Agents
In src/security/auth.py around lines 13 to 15, the code uses CryptContext and
likely jose for JWT handling but the required packages are not declared; add
"passlib[bcrypt]" and "python-jose" to your project's dependencies (either under
[tool.poetry.dependencies] in pyproject.toml or in dependencies/requirements.txt
alongside fastapi), pin appropriate versions or use compatible ranges, and then
run your package manager (poetry install or pip install -r requirements.txt) to
ensure the authentication code can import and run.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ACCESS_TOKEN_EXPIRE_MINUTES constant is defined as 30, but the default expiration time for the token is hardcoded to 15 minutes. This is inconsistent and can be misleading. The default expiration should use the defined constant to ensure consistency.
| expire = datetime.utcnow() + timedelta(minutes=15) | |
| expire = datetime.utcnow() + timedelta(minutes=ACCESS_TOKEN_EXPIRE_MINUTES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): We've found these issues:
- Add single value to dictionary directly rather than using update() (
simplify-dictionary-update) - Inline variable that is immediately returned (
inline-immediately-returned-variable)
| to_encode.update({"exp": expire}) | |
| encoded_jwt = jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM) | |
| return encoded_jwt | |
| to_encode["exp"] = expire | |
| return jwt.encode(to_encode, SECRET_KEY, algorithm=ALGORITHM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (code-quality): Explicitly raise from a previous error (raise-from-previous-error)
| except JWTError: | |
| raise credentials_exception | |
| except JWTError as e: | |
| raise credentials_exception from e |
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,31 @@ | ||||
| from collections import defaultdict | ||||
| from datetime import datetime, timedelta | ||||
|
||||
| from datetime import datetime, timedelta |
Copilot
AI
Sep 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The window cleanup logic is duplicated between is_allowed and get_remaining_requests methods. Consider extracting this into a private helper method to reduce code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This RateLimiter implementation has two issues:
- It is not thread-safe. In a concurrent environment, multiple requests for the same
identifiercan cause a race condition, potentially allowing more requests thanmax_requests. - Code is duplicated. The logic to clean up old request timestamps is repeated in
is_allowedandget_remaining_requests.
Both issues can be solved by introducing a lock (from the threading module) for thread safety and refactoring the cleanup logic into a private method. You will need to add import threading at the top of the file.
class RateLimiter:
def __init__(self, max_requests: int = 100, window_seconds: int = 3600):
self.max_requests = max_requests
self.window_seconds = window_seconds
self.requests = defaultdict(list)
self._lock = threading.Lock()
def _cleanup_requests(self, identifier: str):
"""Removes timestamps outside the current window. Must be called within a lock."""
now = time.time()
window_start = now - self.window_seconds
self.requests[identifier] = [
timestamp for timestamp in self.requests[identifier]
if timestamp > window_start
]
def is_allowed(self, identifier: str) -> bool:
with self._lock:
self._cleanup_requests(identifier)
if len(self.requests[identifier]) < self.max_requests:
self.requests[identifier].append(time.time())
return True
return False
def get_remaining_requests(self, identifier: str) -> int:
with self._lock:
self._cleanup_requests(identifier)
return max(0, self.max_requests - len(self.requests[identifier]))| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,20 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| from flask import Flask, jsonify | ||||||||||||||||||||||||||||||||||||||||||||
| from flask_cors import CORS | ||||||||||||||||||||||||||||||||||||||||||||
| from auth import require_api_key | ||||||||||||||||||||||||||||||||||||||||||||
| from rate_limiter import rate_limit | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+4
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainVerify import style based on packaging layout.
Run: Expected:
🏁 Script executed: #!/bin/bash
set -euo pipefail
# Is src a package?
fd -a '^__init__\.py$' src || true
# Where are direct imports used?
rg -nP 'from (auth|rate_limiter) import 'Length of output: 637 Use package-relative imports in - from auth import require_api_key
- from rate_limiter import rate_limit
+ from .auth import require_api_key
+ from .rate_limiter import rate_limitApply the same convention across all modules under 📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| app = Flask(__name__) | ||||||||||||||||||||||||||||||||||||||||||||
| CORS(app) # Enable CORS for all routes | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion CORS: restrict origins via env; default can remain permissive for dev. Avoid blanket “allow-all” in prod. Load allowed origins from Apply: -from flask import Flask, jsonify
+import os
+from flask import Flask, jsonify
from flask_cors import CORS
from auth import require_api_key
from rate_limiter import rate_limit
app = Flask(__name__)
-CORS(app) # Enable CORS for all routes
+# Restrict in prod via CORS_ORIGINS="https://app.example.com,https://admin.example.com"
+origins_env = os.getenv('CORS_ORIGINS', '').strip()
+if origins_env:
+ origins = [o.strip() for o in origins_env.split(',') if o.strip()]
+ CORS(app, resources={r"/api/*": {"origins": origins}})
+else:
+ CORS(app) # permissive default for dev📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| @app.route('/api/health') | ||||||||||||||||||||||||||||||||||||||||||||
| def health(): | ||||||||||||||||||||||||||||||||||||||||||||
| return jsonify({'status': 'healthy'}) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| @app.route('/api/protected', methods=['POST']) | ||||||||||||||||||||||||||||||||||||||||||||
| @require_api_key | ||||||||||||||||||||||||||||||||||||||||||||
| @rate_limit(max_requests=10, window_minutes=1) | ||||||||||||||||||||||||||||||||||||||||||||
| def protected(): | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Decorator order: rate-limit should wrap auth to throttle invalid-key hammering. Current order runs auth first. Swap them so the limiter executes before auth. Apply: @app.route('/api/protected', methods=['POST'])
-@require_api_key
-@rate_limit(max_requests=10, window_minutes=1)
+@rate_limit(max_requests=10, window_minutes=1)
+@require_api_key
def protected():
return jsonify({'message': 'Protected endpoint'})📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| return jsonify({'message': 'Protected endpoint'}) | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if __name__ == '__main__': | ||||||||||||||||||||||||||||||||||||||||||||
| app.run(host='0.0.0.0', port=5000) | ||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded API key and non-constant-time compare — use env var + compare_digest.
Current code ships a plaintext secret and uses direct string equality. Replace with an env-driven secret and constant‑time comparison; also return a proper 401 with WWW-Authenticate.
Apply:
📝 Committable suggestion
🤖 Prompt for AI Agents