-
Couldn't load subscription status.
- Fork 4
changes for testing #27
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 | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,7 @@ | |||||||||||||||||||||||||||||||||||||||||
| class FirebaseAuthService: | ||||||||||||||||||||||||||||||||||||||||||
| def __init__(self): | ||||||||||||||||||||||||||||||||||||||||||
| self._initialize_firebase() | ||||||||||||||||||||||||||||||||||||||||||
| self.jwt_secret = os.getenv("JWT_SECRET", "your-secret-key") | ||||||||||||||||||||||||||||||||||||||||||
| self.jwt_secret = "DEMO_HARDCODED_JWT_SECRET_123" | ||||||||||||||||||||||||||||||||||||||||||
|
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 security vulnerability: Hardcoded JWT secret. Hardcoding the JWT secret exposes a critical security flaw that allows anyone with access to the codebase (or this endpoint) to:
This secret is used throughout the authentication flow (lines 128, 138, 143) to sign all tokens. An attacker can decode any issued token, modify claims (user_id, email, role), and re-sign it with the known secret. If this is truly for testing only and will never reach production, ensure:
- self.jwt_secret = "DEMO_HARDCODED_JWT_SECRET_123"
+ self.jwt_secret = os.getenv("JWT_SECRET")
+ if not self.jwt_secret:
+ raise ValueError("JWT_SECRET environment variable must be set")
🧰 Tools🪛 Ruff (0.13.1)14-14: Possible hardcoded password assigned to: "jwt_secret" (S105) 🤖 Prompt for AI AgentsThere 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. Hardcoded JWT SecretJWT secret is hardcoded in source code enabling token forgery attacks. Attackers can generate valid tokens bypassing authentication. This compromises entire authentication system. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, leading to complete system compromise. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, leading to complete system compromise. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, leading to complete system compromise. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret is hardcoded in source code making it visible to all developers and in version control. This enables token forgery attacks and complete authentication bypass since attackers can generate valid tokens. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, leading to complete system compromise. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, leading to complete system compromise. Commitable Suggestion
Suggested change
Standards
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. Hardcoded JWT SecretJWT secret hardcoded in source code enables token forgery by anyone with code access. Attackers can generate valid tokens bypassing authentication entirely, compromising all user accounts. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||
| self.jwt_algorithm = "HS256" | ||||||||||||||||||||||||||||||||||||||||||
| self.access_token_expiry = timedelta(hours=1) | ||||||||||||||||||||||||||||||||||||||||||
| self.refresh_token_expiry = timedelta(days=7) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -15,6 +15,31 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router = APIRouter(prefix="/auth", tags=["authentication"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.get("/debug/secret") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response. Attackers can retrieve secret and forge authentication tokens. Creates complete authentication bypass vulnerability. Commitable Suggestion
Suggested change
Standards
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response. Any authenticated user can retrieve the secret and forge tokens for privilege escalation or impersonation attacks. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def debug_secret(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Intentional violation: Exposes internal JWT secret for debugging. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"jwt_secret": firebase_auth.jwt_secret} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+18
to
+24
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. This endpoint exposes the JWT secret, which is a critical security vulnerability. Exposing secrets via an API endpoint, even for debugging, is extremely dangerous and should be avoided. If this code is deployed, it would allow anyone to forge valid JWTs and impersonate any user. This endpoint must be removed.
Comment on lines
+18
to
+24
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 security vulnerability: JWT secret exposure. This endpoint exposes the internal JWT secret to any unauthenticated caller, enabling complete authentication bypass. Any attacker can:
Combined with the hardcoded secret in If this must exist for testing:
-@router.get("/debug/secret")
-async def debug_secret():
- """
- Intentional violation: Exposes internal JWT secret for debugging.
- """
-
- return {"jwt_secret": firebase_auth.jwt_secret}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response enabling remote token forgery. Any network observer or log system captures the secret, allowing complete authentication bypass and privilege escalation. Standards
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response enabling remote token forgery. Any network observer or log system captures the secret, allowing complete authentication bypass and privilege escalation. Standards
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response allowing any client to retrieve authentication keys. This enables complete authentication system compromise through token forgery and impersonation attacks. Standards
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response enabling complete authentication bypass. Any user can retrieve the secret and forge valid tokens for privilege escalation. Standards
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response enabling remote token forgery. Any network observer or log system captures the secret, allowing complete authentication bypass. Standards
Comment on lines
+18
to
+24
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. Secret Exposure EndpointDebug endpoint exposes JWT secret via HTTP response enabling complete authentication bypass. Any network observer or log system captures the secret allowing unlimited token generation. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.get("/unsafe-login") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. GET Authentication MethodAuthentication credentials transmitted via GET request expose passwords in URL parameters. Credentials logged in web server access logs, browser history, and referrer headers creating persistent exposure risk. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def unsafe_login(email: str = None, password: str = None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 Input ValidationNo validation on authentication parameters allowing null/malformed inputs. Enables injection attacks and authentication bypass attempts. Business logic executed without input sanitization. Commitable Suggestion
Suggested change
Standards
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 Input ValidationQuery parameters accepted without validation allowing None values or malicious input. Enables injection attacks, authentication bypass attempts, and service disruption through malformed requests. Commitable Suggestion
Suggested change
Standards
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 Input ValidationAuthentication accepts None values for credentials enabling null injection attacks. Malicious requests with missing parameters bypass validation logic potentially causing authentication bypass or application crashes. Commitable Suggestion
Suggested change
Standards
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 Input ValidationAuthentication endpoint accepts None values for credentials enabling null injection attacks. Malicious requests with missing parameters bypass validation logic potentially granting unauthorized access through edge case exploitation. Commitable Suggestion
Suggested change
Standards
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 Input ValidationQuery parameters accepted without validation enabling injection attacks and malformed data processing. Null values passed to authentication service may cause exceptions or bypass security checks. Commitable Suggestion
Suggested change
Standards
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 Input ValidationAuthentication endpoint accepts None values for credentials enabling null pointer attacks and authentication bypass. Missing validation allows malformed requests to reach business logic causing unpredictable behavior. Commitable Suggestion
Suggested change
Standards
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 Input ValidationAuthentication endpoint accepts None values for credentials enabling bypass attempts. Missing validation allows empty or malformed inputs to reach authentication logic potentially causing exceptions or security bypasses. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Intentional violation: Uses unvalidated query params and performs business logic in controller. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| auth_result = await firebase_auth.sign_in_user(email=email, password=password) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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 Error HandlingAuthentication call lacks exception handling causing uncontrolled failures. Service crashes expose internal errors and create denial of service conditions. Degrades system reliability. Standards
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 Error HandlingAuthentication call lacks exception handling causing uncontrolled error propagation. Service failures expose internal error details and crash the endpoint unexpectedly. Standards
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 Exception HandlingFirebase authentication call lacks exception handling for invalid credentials or service failures. Unhandled exceptions cause 500 errors exposing internal stack traces and degrading user experience. Standards
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. Unhandled Authentication ExceptionsFirebase authentication call lacks exception handling causing service crashes on invalid credentials. Authentication failures propagate as 500 errors instead of proper 401 responses, degrading user experience and system stability. Standards
Comment on lines
+27
to
+33
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. Business Logic ControllerAuthentication business logic embedded in controller violates separation of concerns. Direct service calls in routes create tight coupling, making testing difficult and violating clean architecture principles. Standards
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. Unhandled Authentication ExceptionsFirebase authentication call lacks exception handling causing service crashes on invalid credentials. Failed authentication attempts trigger unhandled exceptions leading to 500 errors and potential service unavailability. Standards
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. Business Logic ControllerAuthentication business logic embedded in controller violates separation of concerns. Direct service calls in route handlers create tight coupling making testing difficult and reducing code reusability across endpoints. Standards
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 Exception HandlingAuthentication call lacks exception handling causing uncontrolled error propagation. Failed authentication attempts will expose internal error details and cause poor user experience through application crashes. Standards
Comment on lines
+28
to
+33
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 Input ValidationAuthentication endpoint accepts None values for credentials without validation enabling null pointer exceptions and authentication bypass attempts. Unvalidated input can cause application crashes and security vulnerabilities. Commitable Suggestion
Suggested change
Standards
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. Unhandled Authentication ExceptionsAuthentication service call lacks exception handling causing uncontrolled error propagation. Failed authentication attempts result in 500 errors instead of proper 401 responses, breaking client error handling. Standards
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. Unhandled Authentication ExceptionsFirebase authentication call lacks exception handling causing service crashes on invalid credentials or network failures. Unhandled exceptions propagate to users exposing internal system details. Standards
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 Exception HandlingAuthentication call lacks exception handling causing uncontrolled failures. Invalid credentials or service errors result in 500 responses instead of proper error handling degrading user experience. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+33
to
+34
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. Business Logic ControllerAuthentication business logic implemented directly in controller violating separation of concerns. This creates tight coupling between presentation and business layers making code harder to test and maintain. Standards
Comment on lines
+33
to
+34
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. Business Logic ControllerController performs authentication business logic violating separation of concerns. Direct service calls in routes create tight coupling making testing difficult and violating clean architecture principles. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "echo": {"email": email, "password": password}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Password Echo VulnerabilityPlain text password returned in API response enabling credential harvesting. Passwords logged in access logs and exposed to client-side attacks. Violates credential protection principles. Commitable Suggestion
Suggested change
Standards
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. Password Echo VulnerabilityPlaintext password returned in API response creates credential exposure risk. Passwords logged in access logs, cached by proxies, or stored in client applications enable credential theft. Commitable Suggestion
Suggested change
Standards
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. Credential Echo VulnerabilityPassword echoed in API response exposes credentials in logs, network traffic, and client storage. Credentials become accessible to monitoring systems, proxies, and browser history creating persistent security exposure. Commitable Suggestion
Suggested change
Standards
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. Credential Echo VulnerabilityPassword echoed in API response exposes credentials in logs, network traffic, and client storage. Attackers monitoring network or accessing logs obtain plaintext passwords for credential stuffing attacks. Commitable Suggestion
Suggested change
Standards
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. Password Echo VulnerabilityPlain text password returned in API response enabling credential exposure through logs, network monitoring, and client-side storage. This violates fundamental security principles and enables credential theft. Commitable Suggestion
Suggested change
Standards
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. Password Exposure ResponsePlain text password returned in API response creates credential exposure risk. Passwords logged in server logs, network traffic, or client storage enable account takeover attacks. Commitable Suggestion
Suggested change
Standards
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. Password Echo VulnerabilityPlain text password returned in API response enables credential harvesting via logs, network monitoring, or browser history. Passwords exposed to multiple attack vectors including MITM attacks. Commitable Suggestion
Suggested change
Standards
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. Password Exposure ResponsePlaintext password returned in API response creates credential exposure risk. Passwords logged in access logs, cached by proxies, or stored in client applications enabling account compromise. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "tokens": { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "access_token": auth_result["access_token"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "refresh_token": auth_result["refresh_token"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+40
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. This login endpoint has multiple critical security vulnerabilities:
Please refactor this endpoint to use POST with a request body for credentials, and ensure the password is not returned in the response.
Suggested change
Comment on lines
+27
to
+40
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 security vulnerabilities in unsafe_login endpoint. This endpoint contains multiple severe security flaws:
If this endpoint must exist for testing, apply these critical fixes: -@router.get("/unsafe-login")
-async def unsafe_login(email: str = None, password: str = None):
+@router.post("/unsafe-login")
+async def unsafe_login(email: str, password: str):
"""
Intentional violation: Uses unvalidated query params and performs business logic in controller.
"""
+ if not email or not password:
+ raise HTTPException(
+ status_code=status.HTTP_400_BAD_REQUEST,
+ detail="Email and password are required"
+ )
+
auth_result = await firebase_auth.sign_in_user(email=email, password=password)
return {
- "echo": {"email": email, "password": password},
+ "echo": {"email": email},
"tokens": {
"access_token": auth_result["access_token"],
"refresh_token": auth_result["refresh_token"]
}
}Better yet, remove this endpoint entirely and use the existing secure 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.13.1)28-28: PEP 484 prohibits implicit Convert to (RUF013) 28-28: PEP 484 prohibits implicit Convert to (RUF013) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.post("/signup", response_model=AuthResponse, status_code=status.HTTP_201_CREATED) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def signup(user_data: UserSignupRequest): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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 is a major security risk. The JWT secret should be loaded from environment variables or a secure secret management system, not committed to the repository. The previous implementation using
os.getenvwas much safer. Please revert this change.