fix: add authentication to POST /tickets and PATCH /tickets/{ticket_id} endpoints (Fixes #1018)#1032
Conversation
…d} endpoints (Fixes ritesh-1918#1018) - Add Depends(get_current_user) to POST /tickets endpoint - Add Depends(get_current_user) to PATCH /tickets/{ticket_id} endpoint - Prevent unauthenticated access to ticket creation and updates - Prevent identity spoofing by requiring authenticated user
|
@zeroknowledge0x is attempting to deploy a commit to the ritesh Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 authenticated user dependencies and tenant isolation to ticket endpoints, secures and rate-limits /ai/log_correction, includes authenticated user_id in correction entries, replaces unsafe file I/O with FileLock + executor-based persistence, and adds tests for the new behaviors. ChangesAuthentication and Tenant Isolation for Tickets & Correction Logging
Sequence Diagram(s)sequenceDiagram
participant Client
participant RateLimiter
participant API as /ai/log_correction
participant Auth as get_current_user
participant Executor
participant FileLock
participant File as corrections_log.json
Client->>RateLimiter: POST /ai/log_correction (10/minute)
RateLimiter->>API: allow
API->>Auth: Depends(get_current_user)
Auth-->>API: user (user_id)
API->>Executor: submit read-modify-write task (entry with user_id)
Executor->>FileLock: acquire
FileLock->>File: read JSON, append, trim to cap, write JSON
FileLock-->>Executor: release
Executor-->>API: task complete
API-->>Client: 200 (saved / no_change)
sequenceDiagram
participant Client
participant Endpoint as Ticket Endpoint
participant Auth as get_current_user
participant Supabase
participant Profiles as profiles table
Client->>Endpoint: Request (with auth)
Endpoint->>Auth: Depends(get_current_user)
Auth-->>Endpoint: user_id, company_id
alt GET /tickets
Endpoint->>Supabase: query tickets filtered by company_id
Supabase-->>Endpoint: tickets
else POST /tickets/save
Endpoint->>Profiles: lookup profile by user_id
Profiles-->>Endpoint: company_id
Endpoint->>Supabase: upsert ticket with auth user_id and company_id
else GET /tickets/{ticket_id}
Endpoint->>Supabase: fetch ticket
alt company_id matches
Supabase-->>Endpoint: ticket
else
Endpoint-->>Client: 403 Forbidden
end
end
Endpoint-->>Client: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
…itesh-1918#1031) - Add aiofiles and filelock imports - Use thread pool executor for non-blocking file I/O - Add FileLock to prevent race conditions during read-modify-write - Add entry cap enforcement (CORRECTIONS_LOG_MAX) - Add rate limiting (10/minute) - Add authentication (Depends(get_current_user)) - Log user_id with correction entry
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
backend/main.py (3)
706-716:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
owner_idis taken from the request body, not the authenticated user.Authentication now blocks unauthenticated callers, but the new ticket's
owner_idstill comes from the client-providedTicketRecord, so an authenticated user can create a ticket under an arbitrary owner — the identity-spoofing concern from#1018. Bind ownership to the verified identity:🛡️ Proposed fix
async def create_ticket(ticket: TicketRecord, user: dict = Depends(get_current_user)): """Save a new ticket into the system.""" + ticket.owner_id = user.get("id") # Check for duplicates before adding existing = next((t for t in TICKETS_DB if t.ticket_id == ticket.ticket_id), None)🤖 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 706 - 716, The create_ticket endpoint accepts a TicketRecord whose owner_id is currently trusted from the request body—replace that trust by binding ownership to the authenticated user: in create_ticket (and before any duplicate-check or append to TICKETS_DB), overwrite ticket.owner_id with the verified identity from the user dependency (e.g., use user["id"] or user.get("id")), then perform the duplicate lookup/append using the now-authenticated owner; this ensures TICKETS_DB and the duplicate check use the authenticated owner rather than a client-provided owner_id.
619-637:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the authenticated
auth_user_idconsistently instead ofrequest_body.user_id.
final_data["user_id"]is correctly overwritten with the authenticated id at Line 595, but the tenant branch and audit logging still read the client-suppliedrequest_body.user_id(Lines 622, 628, 636). Sinceuser_idis a required field onTicketSaveRequest, theelif request_body.user_id:at Line 628 is effectively always true when reached, and the hashed audit values reflect the spoofable input rather than the verified identity. Replace these withauth_user_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 619 - 637, The tenant-check and audit logging are still using the client-supplied request_body.user_id instead of the authenticated auth_user_id; update the tenant branch to use auth_user_id for verification and hashing (replace request_body.user_id with auth_user_id when computing user_hash and in the elif that checks for a missing tenant), and update the logger.info call to hash/log auth_user_id; keep existing behavior of backfilling final_data["user_id"] from the authenticated id but ensure all tenant logic and audit messages reference auth_user_id, and continue to use profile_company_id and profile as before.
719-731:⚠️ Potential issue | 🟠 Major | ⚡ Quick winNo ownership/tenant check on update.
Any authenticated user can patch any ticket in
TICKETS_DBby id, including overwriting arbitrary fields via the unconstrainedupdatesdict. Authentication alone does not enforce authorization here. Verify that the caller owns (or is permitted to modify) the ticket before applyingupdates, and consider restricting which fieldsupdatesmay set.🤖 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 719 - 731, The update_ticket endpoint currently applies any incoming updates to any ticket in TICKETS_DB without authorization; modify update_ticket to first locate the ticket, verify the caller from get_current_user (e.g., user['id'] or user['user_id']) is the owner/authorized editor of that ticket (compare against a ticket ownership field such as ticket.owner_id or ticket.user_id) and raise HTTPException(status_code=403) if not permitted, and then whitelist/validate update keys (create an ALLOWED_UPDATE_FIELDS set and only apply those keys) before merging into ticket.dict() and constructing TicketRecord to prevent arbitrary field overwrites.
🧹 Nitpick comments (1)
backend/main.py (1)
548-550: ⚡ Quick winErrors are returned with HTTP 200.
Both the JSON-parse failure (Line 487) and the save failure here return
{"status": "error"}with the default 200 status, so callers cannot distinguish success from failure via status code. Consider raisingHTTPException(e.g. 400 for invalid body, 500 for save failure) or returning an explicit error status code.🤖 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 548 - 550, The error handling currently returns {"status":"error"} with HTTP 200; change these handlers to raise HTTPException with appropriate status codes so clients can detect failures: in the JSON-parse failure path (the except around parsing the request body) raise HTTPException(status_code=400, detail="Invalid JSON: <error>") and in the save failure except block (the except Exception as e where logger.error("[CORRECTION ERROR] Could not save: {e}") is called) raise HTTPException(status_code=500, detail=str(e)); keep the logger.error calls but replace the returned dicts with raising HTTPException to surface proper HTTP status codes to callers.
🤖 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 696-701: The tenant check currently skips when the caller lacks
user_company_id; change it to fail-closed by rejecting requests when the
caller's company cannot be resolved and still enforce company equality when
ticket has a company. Replace the condition around
user_company_id/ticket_company_id so that if user_company_id is falsy you raise
HTTPException(403), and also raise if ticket_company_id is truthy and
user_company_id != ticket_company_id; update the block that reads
user_metadata/user_company_id/ticket_company_id to implement: if not
user_company_id or (ticket_company_id and user_company_id != ticket_company_id):
raise HTTPException(status_code=403, detail="Access denied: ticket belongs to a
different company or caller has no company_id")
- Around line 486-489: The function log_correction (and other places) reference
a missing logger, causing NameError; add a module-level logger by importing
logging if not present and creating a module logger (e.g., logger =
logging.getLogger(__name__)) near the top of the file after imports so
log_correction, any other functions (lines cited) and module-level logs use this
shared logger; you can keep save_ticket's local logger if intentionally
different, but ensure all references to logger inside log_correction use the
newly defined module-level logger.
- Around line 479-481: The endpoints (e.g., the POST route function
log_correction using the Depends annotation) reference get_current_user before
it is defined which causes an import-time NameError; fix by relocating the async
def get_current_user(...) definition to earlier in the module so it appears
above the first endpoint that uses Depends(get_current_user) (also ensure other
endpoints at the noted locations that use get_current_user are after the moved
definition), or alternatively import get_current_user from a top-level auth
module and replace the local reference so Depends resolves at import time.
In `@backend/tests/test_log_correction.py`:
- Around line 14-17: The supabase patch is only active during import so requests
hit the real get_current_user/supabase; modify the test setup to apply the patch
for the duration of the tests (e.g., start a patcher at module/fixture scope and
stop it after tests or use a pytest fixture that monkeypatches main.supabase) so
that main.supabase remains mocked while TestClient(app) is used and during
test_log_correction_saves_entry execution; ensure the patch references
"main.supabase" and remains active until after all requests complete.
- Around line 31-50: The tests currently use `@patch`("main.get_current_user")
which doesn't affect FastAPI's injected dependency; replace those patches in
TestLogCorrection (including test_log_correction_requires_auth and
test_log_correction_saves_entry) by setting
app.dependency_overrides[get_current_user] = mock_get_current_user before making
client requests and clearing the override after each test (e.g., pop or clear
dependency_overrides) so FastAPI uses mock_get_current_user; keep any
CORRECTIONS_LOG_PATH patching (or replace with a tmp path fixture) for
test_log_correction_saves_entry but remove the `@patch` decorator on
get_current_user and update imports to reference app and get_current_user from
main.
In `@backend/tests/test_tickets_auth.py`:
- Around line 42-155: Add tests to cover POST /tickets and PATCH
/tickets/{ticket_id} authentication: create four test functions named
test_create_ticket_requires_auth, test_create_ticket_with_auth,
test_update_ticket_requires_auth, and test_update_ticket_with_auth following the
existing Test* patterns; for the "requires_auth" tests call
client.post("/tickets", ...) and client.patch("/tickets/ticket-123", ...)
without an Authorization header and assert 401, and for the "with_auth" tests
patch main.get_current_user and main.supabase, mock
supabase.table(...).insert().execute() (for POST) and .update().eq().execute()
or .select().single().execute() (for PATCH) to return MagicMock data and call
the endpoints with headers={"Authorization": "Bearer test-token"}, then assert
200 and validate returned ticket id or updated fields to ensure authenticated
success paths are exercised.
- Around line 91-120: The test test_save_ticket_uses_auth_user_id currently
posts a spoofed user_id but never asserts outcome; update it to assert the API
used the authenticated user id from mock_auth (e.g., "test-user-id-123") and not
the spoofed "spoofed-user-id" by: 1) asserting response.status_code is success
and response.json() contains the expected user id or ticket owner, 2) inspecting
the insert call on mock_supabase
(mock_supabase.table.return_value.insert.assert_called_once() and examine the
call args) to assert the payload passed to insert includes "user_id":
"test-user-id-123" and does not include "spoofed-user-id", and 3) optionally
assert mock_supabase.table.return_value.insert.call_args or the returned
MOCK_TICKET.data shows the authenticated id—target symbols:
test_save_ticket_uses_auth_user_id, client.post("/tickets/save"),
mock_supabase.table.return_value.insert, and MOCK_TICKET.
---
Outside diff comments:
In `@backend/main.py`:
- Around line 706-716: The create_ticket endpoint accepts a TicketRecord whose
owner_id is currently trusted from the request body—replace that trust by
binding ownership to the authenticated user: in create_ticket (and before any
duplicate-check or append to TICKETS_DB), overwrite ticket.owner_id with the
verified identity from the user dependency (e.g., use user["id"] or
user.get("id")), then perform the duplicate lookup/append using the
now-authenticated owner; this ensures TICKETS_DB and the duplicate check use the
authenticated owner rather than a client-provided owner_id.
- Around line 619-637: The tenant-check and audit logging are still using the
client-supplied request_body.user_id instead of the authenticated auth_user_id;
update the tenant branch to use auth_user_id for verification and hashing
(replace request_body.user_id with auth_user_id when computing user_hash and in
the elif that checks for a missing tenant), and update the logger.info call to
hash/log auth_user_id; keep existing behavior of backfilling
final_data["user_id"] from the authenticated id but ensure all tenant logic and
audit messages reference auth_user_id, and continue to use profile_company_id
and profile as before.
- Around line 719-731: The update_ticket endpoint currently applies any incoming
updates to any ticket in TICKETS_DB without authorization; modify update_ticket
to first locate the ticket, verify the caller from get_current_user (e.g.,
user['id'] or user['user_id']) is the owner/authorized editor of that ticket
(compare against a ticket ownership field such as ticket.owner_id or
ticket.user_id) and raise HTTPException(status_code=403) if not permitted, and
then whitelist/validate update keys (create an ALLOWED_UPDATE_FIELDS set and
only apply those keys) before merging into ticket.dict() and constructing
TicketRecord to prevent arbitrary field overwrites.
---
Nitpick comments:
In `@backend/main.py`:
- Around line 548-550: The error handling currently returns {"status":"error"}
with HTTP 200; change these handlers to raise HTTPException with appropriate
status codes so clients can detect failures: in the JSON-parse failure path (the
except around parsing the request body) raise HTTPException(status_code=400,
detail="Invalid JSON: <error>") and in the save failure except block (the except
Exception as e where logger.error("[CORRECTION ERROR] Could not save: {e}") is
called) raise HTTPException(status_code=500, detail=str(e)); keep the
logger.error calls but replace the returned dicts with raising HTTPException to
surface proper HTTP status codes to callers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 928131eb-e5f1-48fd-a4d6-0a0156e22c24
📒 Files selected for processing (3)
backend/main.pybackend/tests/test_log_correction.pybackend/tests/test_tickets_auth.py
Move logger = logging.getLogger(__name__) to module level so it's available in all functions including log_correction. Remove duplicate function-level logger definition.
…assertions Address CodeRabbit review: - Add assertions to user_id spoofing test - Add test_create_ticket_requires_auth - Add test_update_ticket_requires_auth - Add test_create_ticket_with_auth - Add test_update_ticket_with_auth
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 (5)
backend/main.py (5)
720-730:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
PATCH /tickets/{ticket_id}authenticates but never authorizes.Once a caller is logged in, this handler will update any in-memory ticket whose
ticket_idthey know. Without anowner_idor tenant check, this is still an IDOR on writes. Return 403 unless the caller owns the ticket, or has an explicit elevated role.🤖 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 720 - 730, The update_ticket handler currently authenticates via get_current_user but performs no authorization; before applying updates in update_ticket (and before converting ticket.dict() to TicketRecord), check that the current user (from user param returned by get_current_user) is either the ticket owner (compare user['id'] to ticket.owner_id or ticket.user_id as used in the TicketRecord) or has an elevated role (e.g., user.get('role') in ('admin','support')); if not, raise an HTTPException with status_code=403 to deny the operation; keep this check adjacent to the loop that finds the ticket in TICKETS_DB so unauthorized updates are prevented early.
683-685:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't wrap intentional
HTTPExceptions as 500s.This catches the 400/403/404/503 branches raised above and turns them into a generic 500, so the endpoint won't return the auth/tenant statuses this PR adds. Re-raise
HTTPExceptionunchanged before the fallback.Suggested fix
- except Exception as e: + except HTTPException: + raise + except Exception as e: traceback.print_exc() raise HTTPException(status_code=500, detail=str(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 683 - 685, The catch-all except in backend.main (the except Exception as e block) is converting already-intended HTTPException responses into generic 500s; change the handler so that if the caught exception is an instance of fastapi.HTTPException (or HTTPException imported in this module) you re-raise it unchanged, otherwise handle/log and raise the 500 HTTPException as currently done. Locate the except Exception as e block and add an isinstance(e, HTTPException) check to re-raise the original HTTPException before the fallback.
618-631:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject
company_idoverrides when the authenticated profile has no tenant.If
profile_company_idis missing and the request body suppliescompany_id, this branch accepts the caller's value and inserts the ticket anyway. That still allows cross-tenant writes for accounts without a resolved tenant. Fail closed unless the authenticated profile yields a tenant, then require an exact match.Suggested fix
profile_company_id = profile.get("company_id") - 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] - 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: + if not profile_company_id: + raise HTTPException(status_code=400, detail="User has no tenant assignment") + if final_data.get("company_id") and final_data["company_id"] != profile_company_id: + user_hash = hashlib.sha256(str(auth_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") + if not final_data.get("company_id"): # Backfill company_id from profile. final_data["company_id"] = profile_company_id - elif request_body.user_id: - # User has no tenant assignment. - raise HTTPException(status_code=400, detail="User has no tenant assignment")🤖 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 618 - 631, The current logic allows a caller-supplied final_data["company_id"] when profile_company_id is missing; change validation in the block handling final_data.get("company_id") so that if profile_company_id is falsy you reject the request (raise HTTPException 403) instead of accepting the provided company_id, and if profile_company_id exists enforce equality as before (keep the existing tenant-mismatch logging using user_hash built from request_body.user_id). Update the branch around profile_company_id / final_data to: if final_data.get("company_id"): if not profile_company_id -> raise 403; elif final_data["company_id"] != profile_company_id -> log and raise 403; elif profile_company_id -> backfill final_data["company_id"] = profile_company_id; otherwise raise 400 for missing tenant.
567-575:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFail closed when the caller has no
company_id.
user_metadata.get("company_id") or company_idlets a user with no tenant metadata either omit the filter and list every ticket, or pass anycompany_idand read another tenant's data. That reopens the exfiltration path this PR is meant to close.Suggested fix
- user_company_id = user_metadata.get("company_id") or company_id + user_company_id = user_metadata.get("company_id") + if not user_company_id: + raise HTTPException(status_code=403, detail="Access denied: caller has no company_id") + if company_id and company_id != user_company_id: + raise HTTPException(status_code=403, detail="Access denied: company_id mismatch") query = supabase.table("tickets").select("*").order("created_at", desc=True) - if user_company_id: - query = query.eq("company_id", user_company_id) + query = query.eq("company_id", user_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 567 - 575, The code currently sets user_company_id = user_metadata.get("company_id") or company_id which lets callers without tenant metadata bypass or override the tenant filter; change this to fail closed by requiring an explicit tenant on the caller: read user_company_id only from user_metadata (user_metadata.get("company_id")); if it's missing or does not match the requested company_id, return an authorization error (HTTP 403) rather than omitting the filter; then apply query.eq("company_id", user_company_id) unconditionally when constructing the supabase tickets query (use the existing variables user_metadata, user_company_id, company_id, and query to locate the change).
707-716:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
POST /ticketsstill trusts a spoofableowner_id.The route now requires auth, but it persists
ticket.owner_idfrom the body untouched. Any authenticated caller can create a ticket on behalf of another user. Overrideowner_idfromuser["id"]or reject mismatches before appending toTICKETS_DB.🤖 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 707 - 716, The create_ticket route currently trusts the owner_id from the request body; update the create_ticket function to enforce the authenticated user by either (a) overriding ticket.owner_id with user["id"] before duplicate checking and persisting to TICKETS_DB, or (b) validating that ticket.owner_id == user["id"] and returning a 403/400 if it differs. Ensure you reference the get_current_user dependency and use TicketRecord/TICKETS_DB for the duplicate check and append, and keep logging consistent after setting the authoritative owner_id.
🤖 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 720-730: The update_ticket handler currently authenticates via
get_current_user but performs no authorization; before applying updates in
update_ticket (and before converting ticket.dict() to TicketRecord), check that
the current user (from user param returned by get_current_user) is either the
ticket owner (compare user['id'] to ticket.owner_id or ticket.user_id as used in
the TicketRecord) or has an elevated role (e.g., user.get('role') in
('admin','support')); if not, raise an HTTPException with status_code=403 to
deny the operation; keep this check adjacent to the loop that finds the ticket
in TICKETS_DB so unauthorized updates are prevented early.
- Around line 683-685: The catch-all except in backend.main (the except
Exception as e block) is converting already-intended HTTPException responses
into generic 500s; change the handler so that if the caught exception is an
instance of fastapi.HTTPException (or HTTPException imported in this module) you
re-raise it unchanged, otherwise handle/log and raise the 500 HTTPException as
currently done. Locate the except Exception as e block and add an isinstance(e,
HTTPException) check to re-raise the original HTTPException before the fallback.
- Around line 618-631: The current logic allows a caller-supplied
final_data["company_id"] when profile_company_id is missing; change validation
in the block handling final_data.get("company_id") so that if profile_company_id
is falsy you reject the request (raise HTTPException 403) instead of accepting
the provided company_id, and if profile_company_id exists enforce equality as
before (keep the existing tenant-mismatch logging using user_hash built from
request_body.user_id). Update the branch around profile_company_id / final_data
to: if final_data.get("company_id"): if not profile_company_id -> raise 403;
elif final_data["company_id"] != profile_company_id -> log and raise 403; elif
profile_company_id -> backfill final_data["company_id"] = profile_company_id;
otherwise raise 400 for missing tenant.
- Around line 567-575: The code currently sets user_company_id =
user_metadata.get("company_id") or company_id which lets callers without tenant
metadata bypass or override the tenant filter; change this to fail closed by
requiring an explicit tenant on the caller: read user_company_id only from
user_metadata (user_metadata.get("company_id")); if it's missing or does not
match the requested company_id, return an authorization error (HTTP 403) rather
than omitting the filter; then apply query.eq("company_id", user_company_id)
unconditionally when constructing the supabase tickets query (use the existing
variables user_metadata, user_company_id, company_id, and query to locate the
change).
- Around line 707-716: The create_ticket route currently trusts the owner_id
from the request body; update the create_ticket function to enforce the
authenticated user by either (a) overriding ticket.owner_id with user["id"]
before duplicate checking and persisting to TICKETS_DB, or (b) validating that
ticket.owner_id == user["id"] and returning a 403/400 if it differs. Ensure you
reference the get_current_user dependency and use TicketRecord/TICKETS_DB for
the duplicate check and append, and keep logging consistent after setting the
authoritative owner_id.
- Fail closed on tenant isolation: reject callers with no company_id - Bind owner_id to authenticated user in create_ticket (prevent spoofing) - Add authorization check in update_ticket (owner or admin/support role) - Whitelist update fields to prevent arbitrary overwrites - Re-raise HTTPException before catch-all to preserve 400/403/404 statuses - Use auth_user_id consistently in audit logging instead of request_body.user_id - Fail closed when profile_company_id is missing in save_ticket
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 (1)
backend/tests/test_tickets_auth.py (1)
45-62:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftFix auth mocking in tests:
@patch("main.get_current_user")won’t override FastAPIDepends(get_current_user)In
backend/tests/test_tickets_auth.py, the tests decorated with@patch("main.get_current_user", ...)won’t reliably affect the dependency used by the/ticketsroute. FastAPI expects auth dependency overrides viaapp.dependency_overrides, so thewith_authtests are likely exercising the realget_current_userinstead ofmock_get_current_user, meaning the intended identity/tenant behavior isn’t actually being controlled.Suggested approach
# in a fixture or test setup main.app.dependency_overrides[main.get_current_user] = mock_get_current_user # cleanup main.app.dependency_overrides.pop(main.get_current_user, None)Apply this to every occurrence of
@patch("main.get_current_user", ...)in the file and keep@patch("main.supabase")for the DB layer.🤖 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/tests/test_tickets_auth.py` around lines 45 - 62, The tests currently patch main.get_current_user which does not override FastAPI's dependency; instead set main.app.dependency_overrides[main.get_current_user] = mock_get_current_user in the test setup (or fixture) before calling client.get and remove it after (pop main.get_current_user, None) in teardown, while keeping the `@patch`("main.supabase") usage for DB mocking; update every test that uses `@patch`("main.get_current_user") to use app.dependency_overrides with mock_get_current_user so the /tickets route uses the mocked get_current_user.
♻️ Duplicate comments (1)
backend/tests/test_tickets_auth.py (1)
119-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSpoofing assertion is effectively optional — the test can pass without verifying the security fix.
assert response.status_code in (200, 201, 400, 422)accepts validation failures (400/422), and theuser_idcheck on Lines 121-125 only runsif ... insert.called. If the request is rejected (or insert is never reached), the core assertion is skipped and the test still passes — leaving the anti-spoofing objective unverified.Pin the success status and make the insert assertion unconditional:
🔒 Proposed tightening
- assert response.status_code in (200, 201, 400, 422) - # Verify the insert was called with the authenticated user_id, not the spoofed one - if mock_supabase.table.return_value.insert.called: - insert_call_args = mock_supabase.table.return_value.insert.call_args - saved_data = insert_call_args[0][0] if insert_call_args[0] else insert_call_args[1] - assert saved_data.get("user_id") == "test-user-id-123", \ - f"Expected authenticated user_id, got {saved_data.get('user_id')}" + assert response.status_code in (200, 201) + mock_supabase.table.return_value.insert.assert_called_once() + saved_data = mock_supabase.table.return_value.insert.call_args.args[0] + assert saved_data["user_id"] == MOCK_USER["id"] + assert saved_data["user_id"] != "spoofed-user-id"Note: this only becomes meaningful once auth is mocked via
dependency_overrides(see comment on Lines 45-62).🤖 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/tests/test_tickets_auth.py` around lines 119 - 125, The test currently allows 400/422 responses and only checks the spoofing protection if mock_supabase.table.return_value.insert.called, which lets the test pass without verifying anti-spoofing; change the response.status_code assertion to expect success (e.g., assert response.status_code in (200, 201)) and remove the conditional guard so you always assert mock_supabase.table.return_value.insert was called and that saved_data.get("user_id") == "test-user-id-123" (use the existing insert_call_args/saved_data extraction logic), ensuring the insert and user_id check run unconditionally (and ensure auth is properly mocked via dependency_overrides as noted).
🧹 Nitpick comments (1)
backend/tests/test_tickets_auth.py (1)
176-203: ⚡ Quick winAuthenticated create/update tests assert only a status range and don't verify the persisted payload.
assert response.status_code in (200, 201)/(200, 204)confirms little beyond "not an error", and neither test checks that the insert/update was actually invoked with the expected, authenticated-bound data. Combined with the dependency-override gap (Lines 45-62), these currently pass without exercising the auth-protected write paths.Consider asserting the mocked Supabase call and returned ticket:
💚 Strengthen assertions
def test_create_ticket_with_auth(self, mock_supabase, mock_auth): """Test that POST /tickets works with authentication.""" mock_supabase.table.return_value.insert.return_value.execute.return_value = MagicMock(data=[MOCK_TICKET]) response = client.post("/tickets", json={ ... }, headers={"Authorization": "Bearer test-token"}) - assert response.status_code in (200, 201) + assert response.status_code in (200, 201) + mock_supabase.table.return_value.insert.assert_called_once()🤖 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/tests/test_tickets_auth.py` around lines 176 - 203, Update the two tests (test_create_ticket_with_auth and test_update_ticket_with_auth) to assert that the Supabase write was called with the expected payload and that the response body contains the persisted ticket: for test_create_ticket_with_auth verify mock_supabase.table().insert was called_with the ticket dict including the authenticated user/company binding and that response.json() matches/contains MOCK_TICKET; for test_update_ticket_with_auth verify mock_supabase.table().update().eq was called_with the update dict and correct ticket id, and that response.json() or response.content reflects MOCK_TICKET (or check status and empty body for 204 accordingly). Also ensure the get_current_user patch (used as mock_get_current_user) is applied so the auth-bound fields are present when asserting the payload.
🤖 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/tests/test_tickets_auth.py`:
- Around line 45-62: The tests currently patch main.get_current_user which does
not override FastAPI's dependency; instead set
main.app.dependency_overrides[main.get_current_user] = mock_get_current_user in
the test setup (or fixture) before calling client.get and remove it after (pop
main.get_current_user, None) in teardown, while keeping the
`@patch`("main.supabase") usage for DB mocking; update every test that uses
`@patch`("main.get_current_user") to use app.dependency_overrides with
mock_get_current_user so the /tickets route uses the mocked get_current_user.
---
Duplicate comments:
In `@backend/tests/test_tickets_auth.py`:
- Around line 119-125: The test currently allows 400/422 responses and only
checks the spoofing protection if
mock_supabase.table.return_value.insert.called, which lets the test pass without
verifying anti-spoofing; change the response.status_code assertion to expect
success (e.g., assert response.status_code in (200, 201)) and remove the
conditional guard so you always assert mock_supabase.table.return_value.insert
was called and that saved_data.get("user_id") == "test-user-id-123" (use the
existing insert_call_args/saved_data extraction logic), ensuring the insert and
user_id check run unconditionally (and ensure auth is properly mocked via
dependency_overrides as noted).
---
Nitpick comments:
In `@backend/tests/test_tickets_auth.py`:
- Around line 176-203: Update the two tests (test_create_ticket_with_auth and
test_update_ticket_with_auth) to assert that the Supabase write was called with
the expected payload and that the response body contains the persisted ticket:
for test_create_ticket_with_auth verify mock_supabase.table().insert was
called_with the ticket dict including the authenticated user/company binding and
that response.json() matches/contains MOCK_TICKET; for
test_update_ticket_with_auth verify mock_supabase.table().update().eq was
called_with the update dict and correct ticket id, and that response.json() or
response.content reflects MOCK_TICKET (or check status and empty body for 204
accordingly). Also ensure the get_current_user patch (used as
mock_get_current_user) is applied so the auth-bound fields are present when
asserting the payload.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a4d4c1e-1f59-44bc-bd55-7f3c5e9d1a0d
📒 Files selected for processing (2)
backend/main.pybackend/tests/test_tickets_auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/main.py
…dency_overrides in tests - Tenant isolation: reject when caller has no company_id (fail-closed) - Tests: replace @patch('main.get_current_user') with app.dependency_overrides - Tests: unconditional insert assertions in spoofing test - Tests: add missing POST /tickets and PATCH /tickets/{ticket_id} auth tests
- Move extract_token() and get_current_user() before line 483 (first Depends() usage) - Use string literal 'access_token' instead of ACCESS_COOKIE constant (defined later) - Fix supabase mock scope in test_log_correction.py to persist during test execution Addresses CodeRabbit review: forward reference would cause NameError at import time
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 (1)
backend/tests/test_log_correction.py (1)
59-86:⚠️ Potential issue | 🟠 MajorFix CORRECTIONS_LOG_PATH mocking so
open()reads/writestemp_path
backend/main.py’s_write_logusesopen(CORRECTIONS_LOG_PATH, ...)directly (notstr(CORRECTIONS_LOG_PATH)), so mocking only__str__inbackend/tests/test_log_correction.pyisn’t sufficient—open()relies onos.fspath()/__fspath__. PatchCORRECTIONS_LOG_PATHwith a realPath(temp_path)or setmock_path.__fspath__.return_value = str(temp_path)(plus the existingexists/statstubs).- The executor timing concern is already addressed:
log_correctionawaitsrun_in_executor(..., _write_log)before returning.🤖 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/tests/test_log_correction.py` around lines 59 - 86, The test's patch of CORRECTIONS_LOG_PATH doesn't let open() resolve the patched path because open() uses os.fspath()/__fspath__; update test_log_correction_saves_entry to make the patched CORRECTIONS_LOG_PATH fspath-able by either assigning a real Path(temp_path) to the patch or setting mock_path.__fspath__.return_value = str(temp_path) (in addition to the existing mock_path.exists and mock_path.stat stubs) so that backend.main._write_log's open(CORRECTIONS_LOG_PATH, ...) reads/writes the temporary file and the assertions on logs[0]["user_id"] succeed.
🤖 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/tests/test_log_correction.py`:
- Around line 59-86: The test's patch of CORRECTIONS_LOG_PATH doesn't let open()
resolve the patched path because open() uses os.fspath()/__fspath__; update
test_log_correction_saves_entry to make the patched CORRECTIONS_LOG_PATH
fspath-able by either assigning a real Path(temp_path) to the patch or setting
mock_path.__fspath__.return_value = str(temp_path) (in addition to the existing
mock_path.exists and mock_path.stat stubs) so that backend.main._write_log's
open(CORRECTIONS_LOG_PATH, ...) reads/writes the temporary file and the
assertions on logs[0]["user_id"] succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ecdb54be-504d-4056-98f6-0de713301f25
📒 Files selected for processing (3)
backend/main.pybackend/tests/test_log_correction.pybackend/tests/test_tickets_auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/main.py
CodeRabbit flagged that patching CORRECTIONS_LOG_PATH with a MagicMock breaks open() calls because os.fspath() can't resolve the mock. Fix: import the actual CORRECTIONS_LOG_PATH and patch with a real Path(temp_path) inside a context manager so open() works correctly.
|
All CodeRabbit review comments have been addressed in the latest commits:
|
ritesh-1918
added
gssoc
|
Hi @zeroknowledge0x! 🙌 Thank you so much for your excellent contribution: "fix: add authentication to POST /tickets and PATCH /tickets/{ticket_id} endpoints (Fixes #1018)"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
|
Hi @zeroknowledge0x! 🙌 Thank you so much for your excellent contribution: "fix: add authentication to POST /tickets and PATCH /tickets/{ticket_id} endpoints (Fixes #1018)"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
ritesh-1918
added
level:intermediate
|
Hi @zeroknowledge0x! 🙌 Thank you so much for your excellent contribution: "fix: add authentication to POST /tickets and PATCH /tickets/{ticket_id} endpoints (Fixes #1018)"! We really appreciate the high-quality code and effort you have put into the platform. Just a quick, friendly heads-up as we prepare our manual merging and verification queues—please make sure to complete all the mandatory community steps listed below. Once those manual steps are verified, we'll get your PR officially merged into the Let's build something amazing together! 🚀🔥 🌟 Project Support & Developer Network (Show Some Love!)As we prepare our manual verification and merging queues, please take a quick moment to ensure you have completed all four community steps:
Note: Having all four steps completed manually is required before your PR points are officially cleared. |
ritesh-1918
added
level:beginner
- Fail closed on tenant isolation: reject callers with no company_id - Bind owner_id to authenticated user in create_ticket (prevent spoofing) - Add authorization check in update_ticket (owner or admin/support role) - Whitelist update fields to prevent arbitrary overwrites - Re-raise HTTPException before catch-all to preserve 400/403/404 statuses - Use auth_user_id consistently in audit logging instead of request_body.user_id - Fail closed when profile_company_id is missing in save_ticket
2 participants
Fixes #1018, #1031
Summary
Add authentication, tenant isolation, and concurrency fixes to ticket and correction endpoints.
Changes
Ticket Endpoints (Fixes #1018)
Depends(get_current_user)+ tenant isolation (filter by company_id)Log Correction Endpoint (Fixes #1031)
Depends(get_current_user)— only authenticated users can log corrections@limiter.limit("10/minute")to prevent abuserun_in_executor— prevents blocking the event loopFileLockprevents race conditions during read-modify-write cycleCORRECTIONS_LOG_MAXenv var (default 10000) bounds log sizeTesting
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests