-
Couldn't load subscription status.
- Fork 4
changes for testing #29
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: Never hardcode secrets in source control. Hardcoding the JWT secret directly in code creates multiple severe security risks:
Revert to reading from environment variables: - 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 is required")If this is intentionally insecure for testing purposes only, implement runtime environment checks to prevent production deployment: import os
class FirebaseAuthService:
def __init__(self):
self._initialize_firebase()
# Fail-safe: Only allow demo secret in development
env = os.getenv("APP_ENV", "production")
if env == "development":
self.jwt_secret = "DEMO_HARDCODED_JWT_SECRET_123"
else:
self.jwt_secret = os.getenv("JWT_SECRET")
if not self.jwt_secret:
raise ValueError("JWT_SECRET environment variable is required in production")
self.jwt_algorithm = "HS256"
self.access_token_expiry = timedelta(hours=1)
self.refresh_token_expiry = timedelta(days=7)🧰 Tools🪛 Ruff (0.13.3)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 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 version control. Attackers can forge authentication tokens leading to complete authentication bypass and unauthorized access. 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") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 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. If an attacker gains access to this endpoint, they can forge their own authentication tokens and impersonate any user. Debug endpoints that expose sensitive information should never be present in production code. If needed for local development, they should be strictly guarded and disabled by default.
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: Secret exposure endpoint enables complete authentication bypass. Exposing the JWT secret through a public endpoint is an immediate and catastrophic security vulnerability:
This endpoint must be removed entirely before any deployment: -@router.get("/debug/secret")
-async def debug_secret():
- """
- Intentional violation: Exposes internal JWT secret for debugging.
- """
-
- return {"jwt_secret": firebase_auth.jwt_secret}
-
-If debugging capabilities are genuinely needed for local development, implement these safeguards:
Example safer implementation: @router.get("/debug/secret")
async def debug_secret(current_user: Dict[str, Any] = Depends(get_current_user)):
"""
DEBUG ONLY: Returns obfuscated secret info
"""
# Require development environment
if os.getenv("APP_ENV") != "development":
raise HTTPException(status_code=404, detail="Not found")
# Require admin role
if current_user.get("role") != "admin":
raise HTTPException(status_code=403, detail="Forbidden")
# Return only partial/obfuscated info
secret = firebase_auth.jwt_secret
return {
"secret_length": len(secret),
"secret_prefix": secret[:4] + "..." if len(secret) > 4 else "***",
"algorithm": firebase_auth.jwt_algorithm
}🤖 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. 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. Commitable Suggestion
Suggested change
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 and system access. 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 tokens for any account, causing total security failure. 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 authenticated user to retrieve signing key. Attackers can forge tokens for privilege escalation and complete authentication bypass. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.get("/unsafe-login") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def unsafe_login(email: str = None, password: str = None): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+27
to
+28
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 Request CredentialsCredentials transmitted via GET query parameters get logged in web server access logs and browser history. Password exposure through URL parameters violates secure authentication practices. 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. Insecure Authentication MethodCredentials transmitted via URL query parameters exposing them in server logs, browser history, and referrer headers. Authentication data becomes visible to intermediate systems and cached inappropriately. 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 error propagation. Service failures expose internal stack traces and crash the endpoint instead of graceful error responses. 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 ValidationNo validation on email/password parameters allows null values and malformed input to reach authentication service. Enables injection attacks and service disruption through malformed requests. Commitable Suggestion
Suggested change
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 ValidationNo validation on email and password parameters allows null values and malformed input to reach authentication service. This enables enumeration attacks and potential service disruption through invalid data injection. 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 service call lacks try-catch blocks allowing unhandled exceptions to propagate. Failed authentication attempts crash the endpoint returning 500 errors instead of proper authentication failure responses. 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 ValidationNull email and password parameters passed directly to authentication service without validation. Service will fail with null pointer exceptions causing authentication system crashes and denial of service. 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 Error HandlingAuthentication call lacks exception handling causing unhandled errors to propagate as 500 responses. Service becomes unreliable with poor error recovery and debugging difficulty. 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 ValidationQuery parameters accepted without validation allowing null values and malformed input to reach authentication logic. Enables injection attacks and authentication bypass through malformed requests. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "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 Exposure LoggingPlain text password returned in API response gets logged in access logs and browser history. Credential exposure enables account takeover and violates data protection regulations. Commitable Suggestion
Suggested change
Standards
Comment on lines
+28
to
+35
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 Query ParameterPassword transmitted via URL query parameters gets logged in web server logs, proxy caches, and browser history. Credentials become permanently exposed in multiple systems enabling 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 ResponsePlain text password returned in API response gets logged and cached throughout network infrastructure. Credentials become permanently exposed in application logs, monitoring systems, and client-side storage. Commitable Suggestion
Suggested change
Standards
Comment on lines
+28
to
+35
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 Query ParameterPassword transmitted via query parameters gets logged in server logs, proxy logs, and browser history. Credentials become permanently exposed in multiple systems enabling 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 ResponsePlain text password returned in API response exposes credentials to client-side logging, network monitoring, and response caching. Password becomes visible to multiple attack vectors beyond initial transmission. 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 LoggingPlain text password returned in API response and potentially logged in access logs. Credentials exposed to monitoring systems, log aggregators, and anyone with log access 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
This endpoint should be removed or completely refactored to follow standard security practices, similar to the existing
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: Multiple severe security vulnerabilities in authentication flow. This endpoint contains at least four critical security issues:
Remove this endpoint entirely: -@router.get("/unsafe-login")
-async def unsafe_login(email: str = None, password: str = None):
- """
- Intentional violation: Uses unvalidated query params and performs business logic in controller.
- """
-
- auth_result = await firebase_auth.sign_in_user(email=email, password=password)
- return {
- "echo": {"email": email, "password": password},
- "tokens": {
- "access_token": auth_result["access_token"],
- "refresh_token": auth_result["refresh_token"]
- }
- }
-
-The existing 📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.13.3)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 like the
jwt_secretis a critical security vulnerability. Secrets should be loaded from a secure source, such as environment variables or a secret management service, and never be checked into version control. The previous implementation usingos.getenvwas the correct approach.