fix(security): resolve IDOR and missing authentication on ticket endpoints - #2502
Merged
riteshbonthalakoti merged 2 commits intoJun 9, 2026
Conversation
|
@codeboost-tr is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds cookie/header Supabase auth helpers and a FastAPI dependency get_current_user, then secures ticket endpoints (GET /tickets, POST /tickets/save, GET /tickets/{ticket_id}) to require authentication, derive company_id from profiles, enforce tenant isolation, and prevent user_id spoofing. ChangesAuthentication and Tenant Isolation for Ticket Endpoints
Sequence DiagramsequenceDiagram
participant Client
participant FastAPI_Route
participant get_current_user
participant Supabase_Auth
participant profiles_Table
participant tickets_Table
Client->>FastAPI_Route: request (cookie or Authorization: Bearer ...)
FastAPI_Route->>get_current_user: token extraction & validation
get_current_user->>Supabase_Auth: validate session / user
Supabase_Auth-->>get_current_user: user payload
get_current_user->>profiles_Table: fetch profile (company_id)
profiles_Table-->>get_current_user: company_id
FastAPI_Route->>tickets_Table: query/insert using company_id from profile
tickets_Table-->>FastAPI_Route: return tenant-scoped data or insert result
FastAPI_Route-->>Client: 200 / 403 / 401 depending on auth and tenant checks
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related Issues
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/main.py (1)
678-697:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistent user ID references in tenant validation and logging.
Three issues in this block:
Line 682: Logs a hash of
request_body.user_id(attacker-supplied) instead ofauthenticated_user_id. An attacker could send an arbitraryuser_idin the request body, and the log would record their target rather than their actual identity.Line 688: The condition
elif request_body.user_id:checks the original request body value, but at this pointfinal_data["user_id"]has already been set toauthenticated_user_id. Since authentication is enforced, there's always an authenticated user, so this condition should simply beelse:to catch users without tenant assignment.Line 696: Same issue as line 682 — uses
request_body.user_idfor logging instead ofauthenticated_user_id.🔧 Proposed fix
if final_data.get("company_id"): # User provided company_id: verify it matches their profile. if profile_company_id and final_data["company_id"] != profile_company_id: - user_hash = hashlib.sha256(str(request_body.user_id).encode()).hexdigest()[:8] + user_hash = hashlib.sha256(str(authenticated_user_id).encode()).hexdigest()[:8] logger.warning(f"Tenant mismatch: user {user_hash} attempted {final_data['company_id']}, assigned to {profile_company_id}") raise HTTPException(status_code=403, detail="User not authorized for this tenant") elif profile_company_id: # Backfill company_id from profile. final_data["company_id"] = profile_company_id - elif request_body.user_id: + else: # User has no tenant assignment. raise HTTPException(status_code=400, detail="User has no tenant assignment") # Backfill company name if missing. if not final_data.get("company") and profile.get("company"): final_data["company"] = profile["company"] - user_hash = hashlib.sha256(str(request_body.user_id).encode()).hexdigest()[:8] + user_hash = hashlib.sha256(str(authenticated_user_id).encode()).hexdigest()[:8] logger.info(f"Tenant linkage: user_hash={user_hash}, company_id={final_data.get('company_id')}")🤖 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 678 - 697, The tenant-validation block uses attacker-supplied request_body.user_id for decisions/logging and checks request_body.user_id in a branch; change these to use the authenticated user id already set in final_data["user_id"] (or the variable authenticated_user_id if present): replace request_body.user_id with final_data["user_id"] when computing user_hash for logger.warning and logger.info, and change the branch `elif request_body.user_id:` to just `else:` so the "no tenant assignment" path relies on the authenticated id/backfill logic in final_data; keep the existing hashing logic (hashlib.sha256(...).hexdigest()[:8]) for logs.
🤖 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.
Inline comments:
In `@backend/main.py`:
- Around line 320-341: The get_current_user function may return a user dict
missing required fields (downstream expects user["id"]); after resolving the
Supabase result into a plain dict (in get_current_user) validate required fields
(at minimum "id", optionally "email" if used elsewhere) and if any are missing
raise HTTPException(status_code=401, detail="Invalid session: missing user id")
(or an appropriate 401 message). Locate the conversion logic in get_current_user
(the branch that uses user.model_dump(), user.dict(), or dict(user)), perform
the validation immediately after obtaining the dict, and only return the user
dict if the required keys are present. Ensure the exception is raised before
returning so downstream code never receives a malformed user object.
- Around line 752-764: The profile lookup uses .single() which raises when no
row exists; change the lookup to avoid the exception (use .maybe_single() or
catch the exception) so a missing profile yields profile_res.data as None and
triggers the intended 403 path; update the call in the block that builds
profile_res (the supabase.table("profiles").select("company_id").eq("id",
user[\"id\"]).single().execute() sequence) to use maybe_single() or wrap the
execute in try/except, then keep the existing profile = profile_res.data or {}
and user_company_id = profile.get("company_id") logic so a missing profile
results in raising HTTPException(status_code=403, detail="User has no company
assignment").
- Around line 615-627: The profile lookup using
supabase.table("profiles").select(...).single() (see the profile_res/profile
variables) can raise when zero or multiple rows exist; wrap that fetch in a
try/except similar to the POST /tickets/save endpoint: catch the exception from
.single(), handle it by setting profile = {} (or None) and then raise
HTTPException(status_code=403, detail="User has no company assignment") when
company_id is missing; optionally log the caught exception for diagnostics
before returning the 403.
---
Outside diff comments:
In `@backend/main.py`:
- Around line 678-697: The tenant-validation block uses attacker-supplied
request_body.user_id for decisions/logging and checks request_body.user_id in a
branch; change these to use the authenticated user id already set in
final_data["user_id"] (or the variable authenticated_user_id if present):
replace request_body.user_id with final_data["user_id"] when computing user_hash
for logger.warning and logger.info, and change the branch `elif
request_body.user_id:` to just `else:` so the "no tenant assignment" path relies
on the authenticated id/backfill logic in final_data; keep the existing hashing
logic (hashlib.sha256(...).hexdigest()[:8]) for logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…oints
This commit adds Depends(get_current_user) to GET /tickets, GET /tickets/{ticket_id}, and POST /tickets/save. It enforces tenant isolation by fetching the authenticated user's company_id from their profile and explicitly validating ownership before returning or mutating data. Also fixes a missing Response import.
codeboost-tr
force-pushed
the
fix/security-idor-tickets
branch
from
June 9, 2026 07:06
bf6d958 to
c44a385
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/main.py (2)
741-743:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve
HTTPExceptionstatus codes here.Lines 666, 684, and 689 intentionally raise
HTTPException, but this outerexcept Exceptioncatches them all and turns them into a 500. That collapses tenant mismatch and profile/assignment failures into the wrong response code.Suggested fix
- except Exception as e: + except HTTPException: + raise + except Exception as e: traceback.print_exc() raise HTTPException(status_code=500, detail=str(e))This contradicts the PR’s stated 403/tenant-isolation behavior.
🤖 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 741 - 743, The outer except block is converting all exceptions (including FastAPI HTTPException raised earlier like the tenant/403 cases) into a 500; update the except handler to detect and re-raise existing HTTPException instances instead of wrapping them: inside the except Exception as e block (the try/except surrounding the request handling logic) check if isinstance(e, HTTPException) and if so raise e, otherwise continue to traceback.print_exc() and raise a new HTTPException(status_code=500, detail=str(e)); ensure HTTPException is imported/used consistently so tenant mismatch and profile/assignment failures keep their original status codes.
695-696:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLog the authenticated user, not the spoofable request field.
After Line 675 you correctly override
user_id, but Line 695 still hashesrequest_body.user_id. A spoofed payload will therefore leave misleading tenant-linkage logs even though the write is fixed.Suggested fix
- user_hash = hashlib.sha256(str(request_body.user_id).encode()).hexdigest()[:8] + user_hash = hashlib.sha256(str(authenticated_user_id).encode()).hexdigest()[:8]🤖 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 695 - 696, The tenant-linkage log is using the spoofable field request_body.user_id to compute user_hash; replace that with the authenticated/overridden user_id variable (the one set after Line 675) when computing user_hash so logs reflect the true authenticated user; update the computation that produces user_hash and the logger.info call (referencing user_hash and final_data.get('company_id')) to use user_id instead of request_body.user_id.
♻️ Duplicate comments (1)
backend/main.py (1)
333-340:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNormalize the Supabase user before checking required fields.
Line 334 validates membership on the raw
result.userobject, but Lines 336-340 already assume that object may needmodel_dump()/dict()conversion. With model-backed Supabase responses, valid sessions can still be rejected here before the normalization path runs.Suggested fix
- user = getattr(result, "user", None) or (result.get("user") if isinstance(result, dict) else None) - if not user or "id" not in user or "email" not in user: - raise HTTPException(status_code=401, detail="Invalid session: Missing user data") - if hasattr(user, "model_dump"): - return user.model_dump() - if hasattr(user, "dict"): - return user.dict() - return dict(user) + raw_user = getattr(result, "user", None) or (result.get("user") if isinstance(result, dict) else None) + if not raw_user: + raise HTTPException(status_code=401, detail="Invalid session: Missing user data") + if hasattr(raw_user, "model_dump"): + user = raw_user.model_dump() + elif hasattr(raw_user, "dict"): + user = raw_user.dict() + else: + user = dict(raw_user) + if not user.get("id") or not user.get("email"): + raise HTTPException(status_code=401, detail="Invalid session: Missing user data") + return user🤖 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 333 - 340, Normalize the Supabase `user` value before validating required fields: if `result` can be an object or dict, first extract `user = getattr(result, "user", None) or (result.get("user") if isinstance(result, dict) else None)`, then if that `user` has `model_dump()` or `dict()` convert it to a plain dict (call `user.model_dump()` or `user.dict()` accordingly) and reassign `user` to that dict, and only afterwards check for `"id"` and `"email"` and raise the `HTTPException` if missing; update the code paths around the `user` variable (the normalization and the subsequent membership checks) so validation runs against the normalized dict form.
🤖 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.
Outside diff comments:
In `@backend/main.py`:
- Around line 741-743: The outer except block is converting all exceptions
(including FastAPI HTTPException raised earlier like the tenant/403 cases) into
a 500; update the except handler to detect and re-raise existing HTTPException
instances instead of wrapping them: inside the except Exception as e block (the
try/except surrounding the request handling logic) check if isinstance(e,
HTTPException) and if so raise e, otherwise continue to traceback.print_exc()
and raise a new HTTPException(status_code=500, detail=str(e)); ensure
HTTPException is imported/used consistently so tenant mismatch and
profile/assignment failures keep their original status codes.
- Around line 695-696: The tenant-linkage log is using the spoofable field
request_body.user_id to compute user_hash; replace that with the
authenticated/overridden user_id variable (the one set after Line 675) when
computing user_hash so logs reflect the true authenticated user; update the
computation that produces user_hash and the logger.info call (referencing
user_hash and final_data.get('company_id')) to use user_id instead of
request_body.user_id.
---
Duplicate comments:
In `@backend/main.py`:
- Around line 333-340: Normalize the Supabase `user` value before validating
required fields: if `result` can be an object or dict, first extract `user =
getattr(result, "user", None) or (result.get("user") if isinstance(result, dict)
else None)`, then if that `user` has `model_dump()` or `dict()` convert it to a
plain dict (call `user.model_dump()` or `user.dict()` accordingly) and reassign
`user` to that dict, and only afterwards check for `"id"` and `"email"` and
raise the `HTTPException` if missing; update the code paths around the `user`
variable (the normalization and the subsequent membership checks) so validation
runs against the normalized dict form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ed8dac9-c296-4226-9a05-4e8fe47bc106
📒 Files selected for processing (2)
backend/main.pypr_body_helpdesk.txt
✅ Files skipped from review due to trivial changes (1)
- pr_body_helpdesk.txt
riteshbonthalakoti
added
gssoc
Owner
|
Hi @codeboost-tr! Absolute pleasure to have you building with us. I've successfully merged your PR! 🚀 Please make sure to sign up under the company Ritesh PVT Limited when testing your features! Let's keep building! 🔥 |
riteshbonthalakoti
merged commit Jun 9, 2026
cbba135
into
riteshbonthalakoti:gssoc
9 of 10 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔒 SECURITY FIX: IDOR and Missing Authentication on Ticket Endpoints
This PR addresses the critical security vulnerability reported in Issue #1669, where the
/ticketsendpoints lacked authentication and tenant isolation, potentially leading to cross-tenant data breaches.Changes Implemented in
backend/main.py:get_current_userand related authentication helpers above the API routes to ensure they can be used as dependencies (Depends) without raisingNameError.Responseimport fromfastapiwhich was preventing the application from starting when calling auth endpoints.GET /tickets:Depends(get_current_user)to enforce authentication.company_idfrom theprofilestable.company_id(Tenant Isolation).GET /tickets/{ticket_id}:Depends(get_current_user).company_idmatches the authenticated user'scompany_id. Raises403 Forbiddenif there is a mismatch, preventing IDOR.POST /tickets/save:Depends(get_current_user)to enforce authentication.request_body.user_idwith the authenticated user's ID (user["id"]) to prevent attackers from assigning tickets to arbitrary users.Verification:
TestClientto verify that unauthenticated requests now return401 Unauthorized.GET /tickets/{ticket_id}returns403 Forbidden.POST /tickets/savestrictly uses the authenticated user's ID.Closes #1669
Summary by CodeRabbit