Fix Insecure Tenant Auth (ID Spoofing) - #2039
Conversation
|
@ArshVermaGit is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR hardens the ChangesSecure Ticket Creation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ArshVermaGit
left a comment
There was a problem hiding this comment.
Hi @ritesh-1918 ! The issue has been resolved. Please review the PR and merge it under GSSoC. Thanks!
There was a problem hiding this comment.
🧹 Nitpick comments (2)
backend/main.py (2)
573-575: ⚡ Quick winPreserve exception context with
raise ... from e.The static analysis tool correctly identifies that re-raising without exception chaining loses the original error context, making debugging harder. Using
from epreserves the traceback.♻️ Suggested fix
except Exception as e: logger.error(f"Authentication failed: {e}") - raise HTTPException(status_code=401, detail="Authentication failed") + raise HTTPException(status_code=401, detail="Authentication failed") from e🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.py` around lines 573 - 575, The except block currently logs the error via logger.error and then re-raises a new HTTPException, which drops the original traceback; update the handler so the new exception is raised with exception chaining (use raise HTTPException(status_code=401, detail="Authentication failed") from e) to preserve the original context and traceback for debugging; locate the except Exception as e block that calls logger.error(f"Authentication failed: {e}") and modify the raise to include "from e".Source: Linters/SAST tools
578-580: ⚡ Quick winUse hashed user IDs in spoofing attempt logs for consistency.
The rest of this file logs user IDs as SHA-256 hashes (e.g., lines 605, 614, 628), but this spoofing log exposes raw UUIDs. While UUIDs are not directly PII, maintaining consistent logging patterns improves privacy hygiene and reduces correlation risk in log aggregation.
♻️ Suggested fix
+ user_hash = hashlib.sha256(str(trusted_user_id).encode()).hexdigest()[:8] + attempted_hash = hashlib.sha256(str(request_body.user_id).encode()).hexdigest()[:8] if request_body.user_id and request_body.user_id != trusted_user_id: - logger.warning(f"ID Spoofing Attempt: Token user {trusted_user_id} tried to act as {request_body.user_id}") + logger.warning(f"ID Spoofing Attempt: Token user {user_hash} tried to act as {attempted_hash}") raise HTTPException(status_code=403, detail="User ID mismatch. Spoofing detected.")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/main.py` around lines 578 - 580, The spoofing-warning currently logs raw UUIDs; change the log to use a SHA-256 hash of both request_body.user_id and trusted_user_id before emitting the message. Update the code around the if-check that references request_body.user_id and trusted_user_id and replace the logger.warning call to compute hex digests (e.g., hashlib.sha256(<id>.encode()).hexdigest()) for both IDs and include those hashed values in the warning, leaving the HTTPException behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/main.py`:
- Around line 573-575: The except block currently logs the error via
logger.error and then re-raises a new HTTPException, which drops the original
traceback; update the handler so the new exception is raised with exception
chaining (use raise HTTPException(status_code=401, detail="Authentication
failed") from e) to preserve the original context and traceback for debugging;
locate the except Exception as e block that calls logger.error(f"Authentication
failed: {e}") and modify the raise to include "from e".
- Around line 578-580: The spoofing-warning currently logs raw UUIDs; change the
log to use a SHA-256 hash of both request_body.user_id and trusted_user_id
before emitting the message. Update the code around the if-check that references
request_body.user_id and trusted_user_id and replace the logger.warning call to
compute hex digests (e.g., hashlib.sha256(<id>.encode()).hexdigest()) for both
IDs and include those hashed values in the warning, leaving the HTTPException
behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65ab2c20-2dcb-431f-bf18-f51c3de4c677
📒 Files selected for processing (2)
backend/.env.examplebackend/main.py
|
Hi @ArshVermaGit! Thanks for the contribution. I have triaged your PR and set it to merge into the
Welcome to the HELPDESK.AI developer family! 🚀💻 |
fdb999f
into
riteshbonthalakoti:gssoc
Description
This PR resolves a critical ID spoofing vulnerability in the
/tickets/saveendpoint by enforcing strict JWT validation on the backend.Changes Made:
save_ticketFastAPI endpoint to accept theRequestobject.AuthorizationBearer token from the request headers.supabase.auth.get_user(token)to cryptographically verify the JWT and retrieve the trusted user object.user_idis provided in the JSON body and it does not match the token's trusteduser_id, the request is immediately rejected with a 403 Forbidden error and logged as a spoofing attempt.request_body.user_idwith the trusted token ID to guarantee data integrity before insertion.By moving authentication to the backend token verification step, we guarantee that users can only ever submit tickets on behalf of their mathematically proven identity.
Resolved Issue
Resolves #2038
Summary by CodeRabbit
Bug Fixes
Chores