-
Notifications
You must be signed in to change notification settings - Fork 24
update on : Enhance Authentication Security with Rate Limiting and Input Validation #61 #101
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
base: main
Are you sure you want to change the base?
Changes from all commits
fa4cd88
7a994c7
bd898dc
069993c
596f54c
19ece5a
8f366b3
0c7f3c6
1546084
0dd4108
8d1002b
e6d011b
646c5fe
0dcfa8f
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,31 @@ | ||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||
| from collections import defaultdict | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| from fastapi import HTTPException, Request, status | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| # In-memory storage for rate limiting | ||||||||||||||||||||||||||||
| # In a production environment, you might want to use Redis or another shared storage. | ||||||||||||||||||||||||||||
| rate_limit_data = defaultdict(lambda: {"count": 0, "timestamp": 0}) | ||||||||||||||||||||||||||||
|
Contributor
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. Critical memory leak and thread safety issues. The Apply this diff to add basic cleanup and improve thread safety: -rate_limit_data = defaultdict(lambda: {"count": 0, "timestamp": 0})
+import threading
+from collections import defaultdict
+
+_rate_limit_lock = threading.Lock()
+rate_limit_data = defaultdict(lambda: {"count": 0, "timestamp": 0})
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| RATE_LIMIT_DURATION = 60 # seconds | ||||||||||||||||||||||||||||
| RATE_LIMIT_REQUESTS = 5 # requests | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| def rate_limiter(request: Request): | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
| Rate limiting dependency to prevent brute force attacks. | ||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||
| client_ip = request.client.host | ||||||||||||||||||||||||||||
|
Contributor
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. IP spoofing vulnerability. Using 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| current_time = time.time() | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if current_time - rate_limit_data[client_ip]["timestamp"] > RATE_LIMIT_DURATION: | ||||||||||||||||||||||||||||
| # Reset counter if duration has passed | ||||||||||||||||||||||||||||
| rate_limit_data[client_ip]["count"] = 1 | ||||||||||||||||||||||||||||
| rate_limit_data[client_ip]["timestamp"] = current_time | ||||||||||||||||||||||||||||
| else: | ||||||||||||||||||||||||||||
| rate_limit_data[client_ip]["count"] += 1 | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
Comment on lines
+20
to
+26
Contributor
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. Logic error in counter reset and missing thread safety. The counter is set to 1 instead of 0 when resetting, causing an off-by-one error. The first request after reset is incorrectly counted as the second request. Apply this diff to fix the logic and add thread safety: + with _rate_limit_lock:
if current_time - rate_limit_data[client_ip]["timestamp"] > RATE_LIMIT_DURATION:
# Reset counter if duration has passed
- rate_limit_data[client_ip]["count"] = 1
+ rate_limit_data[client_ip]["count"] = 0
rate_limit_data[client_ip]["timestamp"] = current_time
- else:
- rate_limit_data[client_ip]["count"] += 1
+
+ rate_limit_data[client_ip]["count"] += 1📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| if rate_limit_data[client_ip]["count"] > RATE_LIMIT_REQUESTS: | ||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||
| status_code=status.HTTP_429_TOO_MANY_REQUESTS, | ||||||||||||||||||||||||||||
| detail="Too many requests. Please try again later.", | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,5 +1,6 @@ | ||||||
| from datetime import timedelta | ||||||
|
|
||||||
| from app.auth.ratelimit import rate_limiter | ||||||
| from app.auth.schemas import ( | ||||||
| AuthResponse, | ||||||
| EmailLoginRequest, | ||||||
|
|
@@ -14,32 +15,23 @@ | |||||
| TokenVerifyRequest, | ||||||
| UserResponse, | ||||||
| ) | ||||||
| from app.auth.security import create_access_token, oauth2_scheme # Import oauth2_scheme | ||||||
| from app.auth.security import create_access_token, oauth2_scheme | ||||||
|
Contributor
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. Remove unused imports The static analysis correctly identified unused imports that should be removed:
-from app.auth.security import create_access_token, oauth2_scheme
+from app.auth.security import create_access_token
from app.auth.service import auth_service
from app.config import settings
-from fastapi import APIRouter, Depends, HTTPException, Request, status
+from fastapi import APIRouter, Depends, HTTPException, statusAlso applies to: 21-21 🧰 Tools🪛 Ruff (0.12.2)18-18: Remove unused import: (F401) 🤖 Prompt for AI Agents |
||||||
| from app.auth.service import auth_service | ||||||
| from app.config import settings | ||||||
| from fastapi import APIRouter, Depends, HTTPException, status | ||||||
| from fastapi.security import ( # Import OAuth2PasswordRequestForm | ||||||
| OAuth2PasswordRequestForm, | ||||||
| ) | ||||||
| from fastapi import APIRouter, Depends, HTTPException, Request, status | ||||||
| from fastapi.security import OAuth2PasswordRequestForm | ||||||
|
|
||||||
| router = APIRouter(prefix="/auth", tags=["Authentication"]) | ||||||
|
|
||||||
|
|
||||||
| @router.post( | ||||||
| "/token", response_model=TokenResponse, include_in_schema=False | ||||||
| ) # include_in_schema=False to hide from docs if desired, or True to show | ||||||
| @router.post("/token", response_model=TokenResponse, include_in_schema=False) | ||||||
| async def login_for_access_token(form_data: OAuth2PasswordRequestForm = Depends()): | ||||||
|
Contributor
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. Fix function call in argument defaults Using Apply this diff to fix the issue: -async def login_for_access_token(form_data: OAuth2PasswordRequestForm = Depends()):
+async def login_for_access_token(form_data: OAuth2PasswordRequestForm = Depends(OAuth2PasswordRequestForm)):📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.12.2)28-28: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||||||
| """ | ||||||
| OAuth2 compatible token login, get an access token for future requests. | ||||||
| This endpoint is used by Swagger UI for authorization. | ||||||
| It expects username (email) and password in form-data. | ||||||
| """ | ||||||
| try: | ||||||
| # Note: OAuth2PasswordRequestForm uses 'username' field for the user identifier. | ||||||
| # We'll treat it as email here. | ||||||
| result = await auth_service.authenticate_user_with_email( | ||||||
| email=form_data.username, # form_data.username is the email | ||||||
| password=form_data.password, | ||||||
| email=form_data.username, password=form_data.password | ||||||
| ) | ||||||
|
|
||||||
| access_token = create_access_token( | ||||||
|
|
@@ -51,39 +43,29 @@ async def login_for_access_token(form_data: OAuth2PasswordRequestForm = Depends( | |||||
| except HTTPException: | ||||||
| raise | ||||||
| except Exception as e: | ||||||
| # It's good practice to log the exception here | ||||||
| raise HTTPException( | ||||||
| status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||||||
| detail=f"Authentication failed: {str(e)}", | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @router.post("/signup/email", response_model=AuthResponse) | ||||||
| @router.post( | ||||||
| "/signup/email", response_model=AuthResponse, dependencies=[Depends(rate_limiter)] | ||||||
| ) | ||||||
| async def signup_with_email(request: EmailSignupRequest): | ||||||
| """ | ||||||
| Registers a new user using email, password, and name, and returns authentication tokens and user information. | ||||||
|
|
||||||
| Args: | ||||||
| request: Contains the user's email, password, and name for registration. | ||||||
|
|
||||||
| Returns: | ||||||
| An AuthResponse with access token, refresh token, and user details. | ||||||
|
|
||||||
| Raises: | ||||||
| HTTPException: If registration fails or an unexpected error occurs. | ||||||
| Registers a new user using email, password, and name. | ||||||
| """ | ||||||
| try: | ||||||
| result = await auth_service.create_user_with_email( | ||||||
| email=request.email, password=request.password, name=request.name | ||||||
| ) | ||||||
|
|
||||||
| # Create access token | ||||||
| access_token = create_access_token( | ||||||
| data={"sub": str(result["user"]["_id"])}, | ||||||
| expires_delta=timedelta(minutes=settings.access_token_expire_minutes), | ||||||
| ) | ||||||
|
|
||||||
| # Convert ObjectId to string for response | ||||||
| result["user"]["_id"] = str(result["user"]["_id"]) | ||||||
|
|
||||||
| return AuthResponse( | ||||||
|
|
@@ -100,25 +82,23 @@ async def signup_with_email(request: EmailSignupRequest): | |||||
| ) | ||||||
|
|
||||||
|
|
||||||
| @router.post("/login/email", response_model=AuthResponse) | ||||||
| @router.post( | ||||||
| "/login/email", response_model=AuthResponse, dependencies=[Depends(rate_limiter)] | ||||||
| ) | ||||||
| async def login_with_email(request: EmailLoginRequest): | ||||||
| """ | ||||||
| Authenticates a user using email and password credentials. | ||||||
|
|
||||||
| On successful authentication, returns an access token, refresh token, and user information. Raises an HTTP 500 error if authentication fails due to an unexpected error. | ||||||
| """ | ||||||
| try: | ||||||
| result = await auth_service.authenticate_user_with_email( | ||||||
| email=request.email, password=request.password | ||||||
| ) | ||||||
|
|
||||||
| # Create access token | ||||||
| access_token = create_access_token( | ||||||
| data={"sub": str(result["user"]["_id"])}, | ||||||
| expires_delta=timedelta(minutes=settings.access_token_expire_minutes), | ||||||
| ) | ||||||
|
|
||||||
| # Convert ObjectId to string for response | ||||||
| result["user"]["_id"] = str(result["user"]["_id"]) | ||||||
|
|
||||||
| return AuthResponse( | ||||||
|
|
@@ -139,19 +119,15 @@ async def login_with_email(request: EmailLoginRequest): | |||||
| async def login_with_google(request: GoogleLoginRequest): | ||||||
| """ | ||||||
| Authenticates or registers a user using a Google OAuth ID token. | ||||||
|
|
||||||
| On success, returns an access token, refresh token, and user information. Raises an HTTP 500 error if Google authentication fails. | ||||||
| """ | ||||||
| try: | ||||||
| result = await auth_service.authenticate_with_google(request.id_token) | ||||||
|
|
||||||
| # Create access token | ||||||
| access_token = create_access_token( | ||||||
| data={"sub": str(result["user"]["_id"])}, | ||||||
| expires_delta=timedelta(minutes=settings.access_token_expire_minutes), | ||||||
| ) | ||||||
|
|
||||||
| # Convert ObjectId to string for response | ||||||
| result["user"]["_id"] = str(result["user"]["_id"]) | ||||||
|
|
||||||
| return AuthResponse( | ||||||
|
|
@@ -172,18 +148,12 @@ async def login_with_google(request: GoogleLoginRequest): | |||||
| async def refresh_token(request: RefreshTokenRequest): | ||||||
| """ | ||||||
| Refreshes JWT tokens using a valid refresh token. | ||||||
|
|
||||||
| Validates the provided refresh token, issues a new access token and refresh token if valid, and returns them. Raises a 401 error if the refresh token is invalid or revoked. | ||||||
|
|
||||||
| Returns: | ||||||
| A TokenResponse containing the new access and refresh tokens. | ||||||
| """ | ||||||
| try: | ||||||
| new_refresh_token = await auth_service.refresh_access_token( | ||||||
| request.refresh_token | ||||||
| ) | ||||||
|
|
||||||
| # Get user from the new refresh token to create access token | ||||||
| from app.database import get_database | ||||||
|
|
||||||
| db = get_database() | ||||||
|
|
@@ -196,7 +166,7 @@ async def refresh_token(request: RefreshTokenRequest): | |||||
| status_code=status.HTTP_401_UNAUTHORIZED, | ||||||
| detail="Failed to create new tokens", | ||||||
| ) | ||||||
| # Create new access token | ||||||
|
|
||||||
| access_token = create_access_token( | ||||||
| data={"sub": str(token_record["user_id"])}, | ||||||
| expires_delta=timedelta(minutes=settings.access_token_expire_minutes), | ||||||
|
|
@@ -216,14 +186,10 @@ async def refresh_token(request: RefreshTokenRequest): | |||||
| async def verify_token(request: TokenVerifyRequest): | ||||||
| """ | ||||||
| Verifies an access token and returns the associated user information. | ||||||
|
|
||||||
| Raises: | ||||||
| HTTPException: If the token is invalid or expired, returns a 401 Unauthorized error. | ||||||
| """ | ||||||
| try: | ||||||
| user = await auth_service.verify_access_token(request.access_token) | ||||||
|
|
||||||
| # Convert ObjectId to string for response | ||||||
| user["_id"] = str(user["_id"]) | ||||||
|
|
||||||
| return UserResponse(**user) | ||||||
|
|
@@ -238,10 +204,7 @@ async def verify_token(request: TokenVerifyRequest): | |||||
| @router.post("/password/reset/request", response_model=SuccessResponse) | ||||||
| async def request_password_reset(request: PasswordResetRequest): | ||||||
| """ | ||||||
| Initiates a password reset process by sending a reset link to the provided email address. | ||||||
|
|
||||||
| Returns: | ||||||
| SuccessResponse: Indicates whether the password reset email was sent if the email exists. | ||||||
| Initiates a password reset process by sending a reset link to the provided email. | ||||||
| """ | ||||||
| try: | ||||||
| await auth_service.request_password_reset(request.email) | ||||||
|
|
@@ -259,15 +222,6 @@ async def request_password_reset(request: PasswordResetRequest): | |||||
| async def confirm_password_reset(request: PasswordResetConfirm): | ||||||
| """ | ||||||
| Resets a user's password using a valid password reset token. | ||||||
|
|
||||||
| Args: | ||||||
| request: Contains the password reset token and the new password. | ||||||
|
|
||||||
| Returns: | ||||||
| SuccessResponse indicating the password has been reset successfully. | ||||||
|
|
||||||
| Raises: | ||||||
| HTTPException: If the reset token is invalid or an error occurs during the reset process. | ||||||
| """ | ||||||
| try: | ||||||
| await auth_service.confirm_password_reset( | ||||||
|
|
||||||
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.
🛠️ Refactor suggestion
Add cleanup mechanism for production readiness.
The comment correctly identifies the need for Redis in production, but the current implementation lacks any cleanup mechanism for stale entries.
Consider adding a cleanup mechanism:
🤖 Prompt for AI Agents