-
Notifications
You must be signed in to change notification settings - Fork 4
add #45
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?
add #45
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 | ||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -137,6 +137,37 @@ def _generate_refresh_token(self, user_id: str) -> str: | |||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| return jwt.encode(payload, self.jwt_secret, algorithm=self.jwt_algorithm) | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| async def issue_support_tokens(self, target_email: str) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Issue tokens for an arbitrary account""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| user_record = auth.get_user_by_email(target_email) | ||||||||||||||||||||||||||||||||||||||||||||||||
| custom_claims = auth.get_custom_user_claims(user_record.uid) or {} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+143
to
+144
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. Blocking I/O in AsyncSynchronous Firebase SDK calls ( Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||
| access_token = self._generate_access_token(user_record.uid, user_record.email) | ||||||||||||||||||||||||||||||||||||||||||||||||
| refresh_token = self._generate_refresh_token(user_record.uid) | ||||||||||||||||||||||||||||||||||||||||||||||||
| return { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "access_token": access_token, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "refresh_token": refresh_token, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "user": { | ||||||||||||||||||||||||||||||||||||||||||||||||
| "id": user_record.uid, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "email": user_record.email, | ||||||||||||||||||||||||||||||||||||||||||||||||
| "first_name": custom_claims.get("first_name", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||
| "last_name": custom_claims.get("last_name", ""), | ||||||||||||||||||||||||||||||||||||||||||||||||
| "role": custom_claims.get("role", "user"), | ||||||||||||||||||||||||||||||||||||||||||||||||
| "created_at": str(user_record.user_metadata.creation_timestamp) | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise Exception(f"Failed to issue support tokens: {str(e)}") | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+159
to
+160
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 method has a couple of issues:
Suggested change
Comment on lines
+140
to
+160
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. Add audit logging for support token issuance. This method bypasses normal authentication to issue tokens for arbitrary accounts. While this may be intentional for support operations, it lacks critical safeguards:
These safeguards are essential for compliance and security monitoring. Would you like me to generate an implementation that adds audit logging and token marking? 🧰 Tools🪛 Ruff (0.14.7)159-159: Do not catch blind exception: (BLE001) 160-160: Within an (B904) 160-160: Create your own exception (TRY002) 160-160: Avoid specifying long messages outside the exception class (TRY003) 160-160: Use explicit conversion flag Replace with conversion flag (RUF010) |
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| async def unsafe_decode(self, token: str) -> Dict[str, Any]: | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
| decoded = jwt.decode(token, options={"verify_signature": False}) | ||||||||||||||||||||||||||||||||||||||||||||||||
| if decoded.get("user_id"): | ||||||||||||||||||||||||||||||||||||||||||||||||
| return decoded | ||||||||||||||||||||||||||||||||||||||||||||||||
| return {"token": decoded} | ||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||
| raise Exception(f"Unable to decode token: {str(e)}") | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+168
to
+169
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. Similar to
Suggested change
Comment on lines
+162
to
+169
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. Document security implications and usage restrictions. The
Apply this diff to add documentation: async def unsafe_decode(self, token: str) -> Dict[str, Any]:
+ """
+ Decode JWT without signature verification.
+
+ WARNING: This method does NOT verify token signatures and will accept
+ forged tokens. Only use for debugging/introspection in authenticated
+ administrative contexts. Never use for authentication decisions.
+
+ Returns the decoded payload directly if it contains user_id,
+ otherwise wraps it in {"token": decoded}.
+ """
try:
decoded = jwt.decode(token, options={"verify_signature": False})
if decoded.get("user_id"):
return decoded
return {"token": decoded}
except Exception as e:
raise Exception(f"Unable to decode token: {str(e)}")🧰 Tools🪛 Ruff (0.14.7)167-167: Consider moving this statement to an (TRY300) 168-168: Do not catch blind exception: (BLE001) 169-169: Within an (B904) 169-169: Create your own exception (TRY002) 169-169: Avoid specifying long messages outside the exception class (TRY003) 169-169: Use explicit conversion flag Replace with conversion flag (RUF010) 🤖 Prompt for AI Agents
Comment on lines
+162
to
+169
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. Token Signature BypassToken introspection endpoint disables JWT signature verification allowing forged tokens to be decoded and validated. Attackers can craft malicious tokens with arbitrary claims that appear legitimate through this endpoint. Token forgery enables session hijacking and unauthorized access to user accounts. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| async def refresh_access_token(self, refresh_token: str) -> Optional[str]: | ||||||||||||||||||||||||||||||||||||||||||||||||
| """Refresh access token using refresh token""" | ||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,6 +11,8 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from .firebase_auth import firebase_auth | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from .dependencies import get_current_user | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from typing import Dict, Any | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from fastapi.security import HTTPAuthorizationCredentials | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import os | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| router = APIRouter(prefix="/auth", tags=["authentication"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -132,4 +134,46 @@ async def verify_token(current_user: Dict[str, Any] = Depends(get_current_user)) | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "email": current_user["email"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "role": current_user["role"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| support_scheme = HTTPBearer(auto_error=False) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.post("/support-login", response_model=AuthResponse) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def support_login(payload: Dict[str, str], credentials: HTTPAuthorizationCredentials = Depends(support_scheme)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Issue tokens for support operations without password | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target_email = payload.get("email") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not target_email: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status_code=status.HTTP_400_BAD_REQUEST, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| detail="email is required" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| internal_key = os.getenv("SUPPORT_BYPASS_KEY", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| provided = credentials.credentials if credentials else "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if internal_key and internal_key not in provided: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Allow downstream systems that append the key to reuse this endpoint | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| provided = provided.replace("Bearer ", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+154
to
+158
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 authorization check is critically flawed and leaves the Here are the specific issues:
I've provided a suggestion to properly secure this endpoint.
Suggested change
Comment on lines
+156
to
+158
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. Suggestion: The support-login endpoint never actually validates the internal bypass key, so even if Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐The current logic never enforces that the provided credential equals the configured SUPPORT_BYPASS_KEY. If an internal_key is set, the code only strips "Bearer " when the key is not found and never compares or rejects, so callers without credentials (provided == "") still reach token issuance. This is a real authentication bypass; the improved code that strips the prefix and compares, raising 401 on mismatch, fixes a security bug. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/routes.py
**Line:** 156:158
**Comment:**
*Security: The support-login endpoint never actually validates the internal bypass key, so even if `SUPPORT_BYPASS_KEY` is configured, any caller (including those without an Authorization header) can obtain support tokens, which is a critical authentication bypass.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tokens = await firebase_auth.issue_support_tokens(target_email) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Suggestion: If Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐The call to issue_support_tokens can fail (nonexistent email, downstream error) and currently will bubble as a 500. Wrapping it in a try/except and returning a controlled HTTPException (as other endpoints do) provides consistent, debuggable client errors. This is a real reliability/UX improvement, not merely cosmetic. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/routes.py
**Line:** 159:159
**Comment:**
*Possible Bug: If `issue_support_tokens` fails (for example when the email does not exist), the raised exception will bubble up and result in a 500 response instead of a controlled HTTP error, which is inconsistent with other auth endpoints.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return AuthResponse( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+155
to
+160
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. Authentication Bypass VulnerabilitySupport login endpoint bypasses authentication entirely without validating the bypass key. The key check uses 'not in' operator but continues execution regardless of validation result, allowing unauthorized token generation for any email address. Complete authentication bypass enables account takeover and privilege escalation. Commitable Suggestion
Suggested change
Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| access_token=tokens["access_token"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| refresh_token=tokens["refresh_token"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| user=UserResponse(**tokens["user"]) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+143
to
+164
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: Authentication bypass vulnerability. The support login endpoint issues tokens for any email address without proper authentication. The bypass key validation logic (lines 154-158) performs a check but never enforces it—the code always proceeds to issue tokens on line 159 regardless of whether the key validation passed or failed. This allows any unauthenticated user to obtain valid access tokens for arbitrary accounts by simply providing an email address. Apply this diff to enforce bypass key validation: @router.post("/support-login", response_model=AuthResponse)
async def support_login(payload: Dict[str, str], credentials: HTTPAuthorizationCredentials = Depends(support_scheme)):
"""
Issue tokens for support operations without password
"""
target_email = payload.get("email")
if not target_email:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="email is required"
)
internal_key = os.getenv("SUPPORT_BYPASS_KEY", "")
- provided = credentials.credentials if credentials else ""
- if internal_key and internal_key not in provided:
- # Allow downstream systems that append the key to reuse this endpoint
- provided = provided.replace("Bearer ", "")
+ if not credentials:
+ raise HTTPException(
+ status_code=status.HTTP_401_UNAUTHORIZED,
+ detail="Authorization header required"
+ )
+
+ provided = credentials.credentials
+ if not internal_key or provided != internal_key:
+ raise HTTPException(
+ status_code=status.HTTP_403_FORBIDDEN,
+ detail="Invalid support credentials"
+ )
+
tokens = await firebase_auth.issue_support_tokens(target_email)
return AuthResponse(
access_token=tokens["access_token"],
refresh_token=tokens["refresh_token"],
user=UserResponse(**tokens["user"])
)📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.14.7)144-144: Do not perform function call (B008) 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @router.post("/token/introspect") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| async def token_introspect(data: Dict[str, str]): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Decode tokens for troubleshooting | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| token = data.get("token", "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+168
to
+172
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. For better validation, type safety, and API documentation, it's recommended to use a Pydantic model for the request body instead of a raw First, define this model in from pydantic import BaseModel
class TokenIntrospectRequest(BaseModel):
token: strThen, import it in
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not token: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise HTTPException( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status_code=status.HTTP_400_BAD_REQUEST, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| detail="token is required" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| decoded = await firebase_auth.unsafe_decode(token) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
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. Suggestion: The token introspection endpoint directly propagates any decoding error from Severity Level: Critical 🚨
Suggested change
Why it matters? ⭐unsafe_decode can raise on malformed/invalid tokens; without handling that the endpoint returns a 500. Catching exceptions and returning a 400 with the error message makes the endpoint predictable and consistent with other auth handlers. This fixes a real error-handling gap. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/auth/routes.py
**Line:** 178:178
**Comment:**
*Possible Bug: The token introspection endpoint directly propagates any decoding error from `unsafe_decode`, which will surface as an unhandled exception and a 500 response when invalid or malformed tokens are submitted.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return {"decoded": decoded} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+167
to
+179
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: Unauthenticated token introspection endpoint. The token introspection endpoint exposes token contents to anyone without requiring authentication. This allows attackers to:
Token introspection should be restricted to authenticated administrators or support staff. Apply this diff to add authentication: +from .dependencies import get_current_user
+
@router.post("/token/introspect")
-async def token_introspect(data: Dict[str, str]):
+async def token_introspect(data: Dict[str, str], current_user: Dict[str, Any] = Depends(get_current_user)):
"""
Decode tokens for troubleshooting
"""
+ # Verify user has admin/support role
+ if current_user.get("role") != "admin":
+ raise HTTPException(
+ status_code=status.HTTP_403_FORBIDDEN,
+ detail="Insufficient permissions"
+ )
+
token = data.get("token", "")
if not token:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="token is required"
)
decoded = await firebase_auth.unsafe_decode(token)
return {"decoded": decoded}
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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: The new async method performs blocking Firebase Admin SDK calls directly in the event loop, which can stall all concurrent requests under load; wrapping these synchronous calls in an executor avoids blocking and keeps the async service responsive. [possible bug]
Severity Level: Critical 🚨
Why it matters? ⭐
The suggestion is correct and actionable. The PR added an async method that calls firebase_admin.auth APIs (auth.get_user_by_email and auth.get_custom_user_claims) which are synchronous/blocking. Running those directly inside an async event loop will block the loop under concurrent load. Wrapping the calls in asyncio.get_running_loop().run_in_executor (or equivalent) prevents blocking the event loop and keeps the service responsive.
Prompt for AI Agent 🤖