Enforce Row Level Security (RLS) by Scoping Supabase Clients with User JWTs - #2189
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 migrates the backend from a global, service-role-key Supabase client to per-user, RLS-enforced clients; adds comprehensive database-level Row Level Security policies; hardens frontend configuration validation; and offloads blocking AI inference calls to async worker threads. The changes directly address the security bypass documented in issue ChangesSupabase RLS Security Refactor and Backend Robustness
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
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 |
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.
Actionable comments posted: 5
🧹 Nitpick comments (3)
Frontend/src/services/api.js (1)
24-32: ⚡ Quick win"Safe helper" comment is inconsistent with throwing behavior.
The comment at line 24 describes
setStorageas a "Safe helper," but the function now throws an error on line 30 when localStorage quota is exceeded. This breaking throw can propagate from line 14 insidegetStorage(which is also documented as "Safe" at line 9), causinggetStorageto fail during default-data initialization.While throwing on quota exceeded is a reasonable design choice for stricter error handling (aligning with the PR's goal of removing silent fallbacks), the "Safe helper" documentation is now misleading.
📝 Proposed fix to update the misleading comment
-// Safe helper to set data and handle QuotaExceeded +// Helper to set data in localStorage; throws if quota is exceeded const setStorage = (key, data) => {Also consider updating the
getStoragecomment at line 9 to clarify that it may throw if default-data initialization fails:-// Safe helper to get data from storage or default +// Helper to get data from storage or initialize with default; may throw on storage quota errors const getStorage = (key, defaultData) => {🤖 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 `@Frontend/src/services/api.js` around lines 24 - 32, Update the misleading "Safe helper" comments: change the comment for setStorage to indicate it may throw an error when localStorage quota is exceeded (so callers should handle or propagate that error), and update the getStorage comment to note that it can throw if default-data initialization triggers setStorage's quota error; reference the setStorage and getStorage functions so readers know the documented behavior matches the implementation.backend/main.py (1)
38-51: ⚖️ Poor tradeoffService-role client remains in use, limiting RLS enforcement scope.
The global
supabaseclient (line 48) usingSUPABASE_SERVICE_ROLE_KEYstill bypasses RLS for several operations in this file (get_system_settings,get_current_user, auth endpoints) and in other modules per the provided context snippets:
backend/services/notification_routing.py(lines 52-90)backend/services/auto_close_service.py(lines 77-136)This is acceptable for privileged backend-only operations, but consider updating the comment on line 38 to clarify when service-role vs user-scoped clients should be used, and ensure a follow-up tracks migrating other services where user-context is available.
🤖 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 38 - 51, The comment and code create a global service-role Supabase client (supabase via create_client using SUPABASE_SERVICE_ROLE_KEY) which bypasses RLS; update the inline comment to explicitly state that this global client is only for privileged, backend-only operations and that any endpoints or functions that act on behalf of a user (e.g., get_system_settings, get_current_user and services referenced like notification_routing.py and auto_close_service.py) should instead create/request a user-scoped client using the user's access token; add a TODO to track migrating those services where user-context is available and ensure code paths that should respect RLS do not use the global supabase client.supabase/migrations/20260607_enable_rls.sql (1)
91-94: PreferTO authenticatedoverauth.role()for thesystem_settingsSELECT RLS policySupabase’s RLS guidance recommends targeting Postgres roles via the policy
TOclause (e.g.,TO authenticated) rather than usingauth.role()inUSING. Whileauth.role()typically reflects the JWTroleclaim (so your currentauth.role() = 'authenticated'should gate correctly), switching the policy toTO authenticatedavoids relying onauth.role()as the primary routing mechanism.
supabase/migrations/20260607_enable_rls.sql (lines 91-94)🤖 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 `@supabase/migrations/20260607_enable_rls.sql` around lines 91 - 94, The SELECT RLS policy on system_settings ("Authenticated users can view system settings") should use the TO clause rather than relying on auth.role() in USING; update the CREATE POLICY statement for system_settings to target the Postgres role by adding TO authenticated and remove or replace the current USING (auth.role() = 'authenticated') check so the policy uses role-level routing instead of the auth.role() expression.
🤖 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/.env.example`:
- Around line 1-3: The example .env is missing SUPABASE_SERVICE_ROLE_KEY which
causes the supabase client initialization (the global supabase used in backend
main.py when initializing admin operations) to fail; update the
backend/.env.example to include
SUPABASE_SERVICE_ROLE_KEY=<your-service-role-key> alongside SUPABASE_URL and
SUPABASE_ANON_KEY so developers have the admin key required for functions like
get_system_settings and get_current_user and for auth-related endpoints to avoid
the “[ERROR] SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY not set” failure during
startup.
In `@backend/main.py`:
- Around line 54-72: In get_user_supabase, ensure the extracted token from the
Authorization header is non-empty (reject cases like "Bearer " or empty string)
before calling create_client; if token is empty, raise HTTPException 401 with a
clear "Missing or invalid token" message. Also stop returning internal exception
text to clients: catch exceptions around create_client, log the full exception
internally (e.g., to your logger) and raise a generic HTTPException 500 with a
non-sensitive message like "Failed to initialize user client" instead of
including str(e). Ensure these checks apply to the Authorization parsing and the
create_client call so downstream errors are avoided.
- Around line 742-753: The update_ticket endpoint accepts an arbitrary updates
dict which can overwrite sensitive columns; fix by validating and filtering
fields before calling user_client.table("tickets").update: define a whitelist of
allowed updatable fields (e.g., status, viewed_at, assigned_to, priority) and
explicitly remove/ignore any keys like id, user_id, company_id, created_at,
created_by, or other sensitive columns, return a 400 if updates becomes empty or
contains disallowed keys, and then pass the sanitized updates to the existing
update call in update_ticket.
- Around line 727-739: The create_ticket endpoint inserts an unvalidated dict
and lacks the auth/profile/tenant checks implemented in save_ticket; update
create_ticket to either be deprecated or to replicate save_ticket's behavior by
(1) requiring and validating a typed TicketSaveRequest payload instead of raw
dict, (2) authenticating the caller via user_client.auth.get_user(token) (or
reuse get_user_supabase flow) to prevent user_id spoofing, (3) resolving
tenant_id from the user's profile (profiles table) and enforcing it on the
inserted record, (4) validating required fields and schema before calling
user_client.table("tickets").insert(...). Ensure responses and error handling
match save_ticket (HTTPException for missing auth or failed insert) and remove
direct print statements in favor of logging.
In `@Frontend/src/config.js`:
- Around line 7-9: The current check that logs and returns an empty string when
VITE_BACKEND_URL is missing (the envUrl check in Frontend/src/config.js) allows
the app to boot into a broken state; change this to fail-fast by throwing an
Error (or otherwise stopping initialization) when envUrl is falsy so the app
fails immediately with a clear message referencing VITE_BACKEND_URL; update the
envUrl/null-handling branch (the if (!envUrl) block) to throw a descriptive
Error (or call a build-time validation helper using import.meta.env) instead of
returning '' so downstream calls (e.g., axios requests) can't silently use
relative URLs.
---
Nitpick comments:
In `@backend/main.py`:
- Around line 38-51: The comment and code create a global service-role Supabase
client (supabase via create_client using SUPABASE_SERVICE_ROLE_KEY) which
bypasses RLS; update the inline comment to explicitly state that this global
client is only for privileged, backend-only operations and that any endpoints or
functions that act on behalf of a user (e.g., get_system_settings,
get_current_user and services referenced like notification_routing.py and
auto_close_service.py) should instead create/request a user-scoped client using
the user's access token; add a TODO to track migrating those services where
user-context is available and ensure code paths that should respect RLS do not
use the global supabase client.
In `@Frontend/src/services/api.js`:
- Around line 24-32: Update the misleading "Safe helper" comments: change the
comment for setStorage to indicate it may throw an error when localStorage quota
is exceeded (so callers should handle or propagate that error), and update the
getStorage comment to note that it can throw if default-data initialization
triggers setStorage's quota error; reference the setStorage and getStorage
functions so readers know the documented behavior matches the implementation.
In `@supabase/migrations/20260607_enable_rls.sql`:
- Around line 91-94: The SELECT RLS policy on system_settings ("Authenticated
users can view system settings") should use the TO clause rather than relying on
auth.role() in USING; update the CREATE POLICY statement for system_settings to
target the Postgres role by adding TO authenticated and remove or replace the
current USING (auth.role() = 'authenticated') check so the policy uses
role-level routing instead of the auth.role() expression.
🪄 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: c7b6568e-5b21-4e0b-aa0a-183618dc8442
📒 Files selected for processing (5)
Frontend/src/config.jsFrontend/src/services/api.jsbackend/.env.examplebackend/main.pysupabase/migrations/20260607_enable_rls.sql
| # Supabase Configuration | ||
| SUPABASE_URL=https://your-project.supabase.co | ||
| SUPABASE_SERVICE_KEY=your-service-key | ||
| SUPABASE_ANON_KEY=your-anon-key |
There was a problem hiding this comment.
Missing SUPABASE_SERVICE_ROLE_KEY in example file.
The example file removes SUPABASE_SERVICE_KEY but backend/main.py (lines 42-48) still initializes the global supabase client using SUPABASE_SERVICE_ROLE_KEY for admin operations (e.g., get_system_settings, get_current_user, auth endpoints). The example should document both keys:
# Supabase Configuration
SUPABASE_URL=https://your-project.supabase.co
SUPABASE_ANON_KEY=your-anon-key
+SUPABASE_SERVICE_ROLE_KEY=your-service-role-keyWithout this, developers will see the error on line 45: [ERROR] SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY not set.
🧰 Tools
🪛 dotenv-linter (4.0.0)
[warning] 3-3: [UnorderedKey] The SUPABASE_ANON_KEY key should go before the SUPABASE_URL key
(UnorderedKey)
🤖 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/.env.example` around lines 1 - 3, The example .env is missing
SUPABASE_SERVICE_ROLE_KEY which causes the supabase client initialization (the
global supabase used in backend main.py when initializing admin operations) to
fail; update the backend/.env.example to include
SUPABASE_SERVICE_ROLE_KEY=<your-service-role-key> alongside SUPABASE_URL and
SUPABASE_ANON_KEY so developers have the admin key required for functions like
get_system_settings and get_current_user and for auth-related endpoints to avoid
the “[ERROR] SUPABASE_URL or SUPABASE_SERVICE_ROLE_KEY not set” failure during
startup.
| def get_user_supabase(request: Request): | ||
| """Dependency to create a scoped Supabase client using the user's JWT token.""" | ||
| if not url or not anon_key: | ||
| raise HTTPException(status_code=500, detail="Supabase Anon Key or URL missing in backend/.env") | ||
|
|
||
| auth_header = request.headers.get("Authorization") | ||
| if not auth_header or not auth_header.startswith("Bearer "): | ||
| raise HTTPException(status_code=401, detail="Missing or invalid Authorization header") | ||
|
|
||
| token = auth_header.split(" ")[1] | ||
|
|
||
| try: | ||
| user_client = create_client(url, anon_key, options={ | ||
| "global": {"headers": {"Authorization": f"Bearer {token}"}} | ||
| }) | ||
| return user_client | ||
| except Exception as e: | ||
| raise HTTPException(status_code=500, detail=f"Failed to initialize user client: {str(e)}") | ||
|
|
There was a problem hiding this comment.
Validate token is non-empty and avoid leaking internal error details.
Two issues in the dependency:
-
Empty token accepted: If header is
"Bearer "(trailing space, no token),split(" ")[1]returns"", which passes tocreate_clientand may cause confusing downstream errors. -
Information leakage: Line 71 exposes internal exception details (
str(e)) which may reveal sensitive configuration or stack info to clients.
🛡️ Proposed fix
token = auth_header.split(" ")[1]
+ if not token:
+ raise HTTPException(status_code=401, detail="Empty Bearer token")
try:
user_client = create_client(url, anon_key, options={
"global": {"headers": {"Authorization": f"Bearer {token}"}}
})
return user_client
except Exception as e:
- raise HTTPException(status_code=500, detail=f"Failed to initialize user client: {str(e)}")
+ raise HTTPException(status_code=500, detail="Failed to initialize user client") from e🧰 Tools
🪛 Ruff (0.15.15)
[warning] 70-70: Do not catch blind exception: Exception
(BLE001)
[warning] 71-71: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
[warning] 71-71: Use explicit conversion flag
Replace with conversion flag
(RUF010)
🤖 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 54 - 72, In get_user_supabase, ensure the
extracted token from the Authorization header is non-empty (reject cases like
"Bearer " or empty string) before calling create_client; if token is empty,
raise HTTPException 401 with a clear "Missing or invalid token" message. Also
stop returning internal exception text to clients: catch exceptions around
create_client, log the full exception internally (e.g., to your logger) and
raise a generic HTTPException 500 with a non-sensitive message like "Failed to
initialize user client" instead of including str(e). Ensure these checks apply
to the Authorization parsing and the create_client call so downstream errors are
avoided.
| @app.post("/tickets") | ||
| @limiter.limit("30/minute") | ||
| async def create_ticket(ticket: dict, request: Request, user_client = Depends(get_user_supabase)): | ||
| """Save a new ticket into the system (persisted to Supabase).""" | ||
| if not user_client: | ||
| raise HTTPException(status_code=500, detail="Database connection not initialized") | ||
|
|
||
| res = user_client.table("tickets").insert(ticket).execute() | ||
| if not res.data: | ||
| raise HTTPException(status_code=400, detail="Failed to create ticket") | ||
|
|
||
| print(f"[DB] Ticket created: {res.data[0].get('id')}") | ||
| return res.data[0] |
There was a problem hiding this comment.
Endpoint lacks input validation and auth checks present in /tickets/save.
This endpoint accepts an unvalidated dict and inserts it directly, unlike save_ticket (lines 598-618) which:
- Validates the user via
user_client.auth.get_user(token) - Prevents user_id spoofing
- Resolves tenant linkage from profiles
- Uses a typed
TicketSaveRequestmodel
While the RLS INSERT policy (auth.uid() = user_id) provides some protection, this endpoint:
- Allows arbitrary fields that may not match the schema
- Doesn't enforce tenant linkage or profile validation
- Could fail silently if
user_idis missing from the dict
Consider deprecating this endpoint in favor of /tickets/save, or applying the same validation:
-async def create_ticket(ticket: dict, request: Request, user_client = Depends(get_user_supabase)):
+async def create_ticket(ticket: TicketSaveRequest, request: Request, user_client = Depends(get_user_supabase)):🧰 Tools
🪛 Ruff (0.15.15)
[warning] 729-729: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 727 - 739, The create_ticket endpoint inserts
an unvalidated dict and lacks the auth/profile/tenant checks implemented in
save_ticket; update create_ticket to either be deprecated or to replicate
save_ticket's behavior by (1) requiring and validating a typed TicketSaveRequest
payload instead of raw dict, (2) authenticating the caller via
user_client.auth.get_user(token) (or reuse get_user_supabase flow) to prevent
user_id spoofing, (3) resolving tenant_id from the user's profile (profiles
table) and enforcing it on the inserted record, (4) validating required fields
and schema before calling user_client.table("tickets").insert(...). Ensure
responses and error handling match save_ticket (HTTPException for missing auth
or failed insert) and remove direct print statements in favor of logging.
| @app.patch("/tickets/{ticket_id}") | ||
| @limiter.limit("60/minute") | ||
| async def update_ticket(ticket_id: str, updates: dict, request: Request, user_client = Depends(get_user_supabase)): | ||
| """Partially update a ticket's fields (e.g., status, viewed_at) via Supabase.""" | ||
| if not user_client: | ||
| raise HTTPException(status_code=500, detail="Database connection not initialized") | ||
|
|
||
| raise HTTPException(status_code=404, detail="Ticket not found") | ||
| res = user_client.table("tickets").update(updates).eq("id", ticket_id).execute() | ||
| if not res.data: | ||
| raise HTTPException(status_code=404, detail="Ticket not found") | ||
|
|
||
| return res.data[0] |
There was a problem hiding this comment.
Arbitrary updates dict allows overwriting sensitive fields.
The endpoint accepts any fields in the updates dict, potentially allowing clients to modify sensitive columns like user_id, company_id, or created_at. While RLS prevents updating other users' tickets, a malicious user could corrupt their own ticket data.
Consider validating allowed update fields:
🛡️ Proposed fix
+ALLOWED_UPDATE_FIELDS = {"status", "priority", "assigned_team", "last_user_viewed_at", "updated_at"}
+
`@app.patch`("/tickets/{ticket_id}")
`@limiter.limit`("60/minute")
async def update_ticket(ticket_id: str, updates: dict, request: Request, user_client = Depends(get_user_supabase)):
"""Partially update a ticket's fields (e.g., status, viewed_at) via Supabase."""
if not user_client:
raise HTTPException(status_code=500, detail="Database connection not initialized")
+ # Filter to allowed fields only
+ safe_updates = {k: v for k, v in updates.items() if k in ALLOWED_UPDATE_FIELDS}
+ if not safe_updates:
+ raise HTTPException(status_code=400, detail="No valid update fields provided")
+
- res = user_client.table("tickets").update(updates).eq("id", ticket_id).execute()
+ res = user_client.table("tickets").update(safe_updates).eq("id", ticket_id).execute()🧰 Tools
🪛 Ruff (0.15.15)
[warning] 744-744: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🤖 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 742 - 753, The update_ticket endpoint accepts
an arbitrary updates dict which can overwrite sensitive columns; fix by
validating and filtering fields before calling
user_client.table("tickets").update: define a whitelist of allowed updatable
fields (e.g., status, viewed_at, assigned_to, priority) and explicitly
remove/ignore any keys like id, user_id, company_id, created_at, created_by, or
other sensitive columns, return a 400 if updates becomes empty or contains
disallowed keys, and then pass the sanitized updates to the existing update call
in update_ticket.
| if (!envUrl) { | ||
| console.error("CRITICAL: VITE_BACKEND_URL environment variable is missing. The frontend may not be able to communicate with the backend."); | ||
| return ''; |
There was a problem hiding this comment.
Returning empty string allows the app to start in a broken state.
When VITE_BACKEND_URL is missing, returning an empty string permits the application to initialize but causes all API calls to fail with confusing relative-URL errors. For example, from the relevant snippet in TicketTracking.jsx, the request becomes axios.post("/tickets/save", ...) instead of a fully-qualified backend URL, which will likely route to the wrong endpoint or fail entirely.
Consider throwing an error here to fail-fast during initialization, providing immediate feedback that the configuration is incomplete. Alternatively, leverage Vite's build-time environment variable validation to catch this before runtime.
🛡️ Proposed fix to fail-fast on missing configuration
const getBackendUrl = () => {
const envUrl = import.meta.env.VITE_BACKEND_URL;
if (!envUrl) {
console.error("CRITICAL: VITE_BACKEND_URL environment variable is missing. The frontend may not be able to communicate with the backend.");
- return '';
+ throw new Error("CRITICAL: VITE_BACKEND_URL environment variable is required. Please configure it in your .env file.");
}
return envUrl.trim().replace(/\/$/, '');
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!envUrl) { | |
| console.error("CRITICAL: VITE_BACKEND_URL environment variable is missing. The frontend may not be able to communicate with the backend."); | |
| return ''; | |
| const getBackendUrl = () => { | |
| const envUrl = import.meta.env.VITE_BACKEND_URL; | |
| if (!envUrl) { | |
| console.error("CRITICAL: VITE_BACKEND_URL environment variable is missing. The frontend may not be able to communicate with the backend."); | |
| throw new Error("CRITICAL: VITE_BACKEND_URL environment variable is required. Please configure it in your .env file."); | |
| } | |
| return envUrl.trim().replace(/\/$/, ''); | |
| }; |
🤖 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 `@Frontend/src/config.js` around lines 7 - 9, The current check that logs and
returns an empty string when VITE_BACKEND_URL is missing (the envUrl check in
Frontend/src/config.js) allows the app to boot into a broken state; change this
to fail-fast by throwing an Error (or otherwise stopping initialization) when
envUrl is falsy so the app fails immediately with a clear message referencing
VITE_BACKEND_URL; update the envUrl/null-handling branch (the if (!envUrl)
block) to throw a descriptive Error (or call a build-time validation helper
using import.meta.env) instead of returning '' so downstream calls (e.g., axios
requests) can't silently use relative URLs.
|
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! 🚀💻 |
3833249
into
riteshbonthalakoti:gssoc
Description
This PR resolves a critical security vulnerability where the backend bypassed Row Level Security (RLS) by connecting to Supabase using the global
SUPABASE_SERVICE_ROLE_KEY.We have refactored the database connection architecture to use a scoped Supabase client dependency.
Changes Made:
SUPABASE_ANON_KEYfrom environment variables.get_user_supabase(request: Request)that dynamically extracts theBearertoken from the incoming request.supabaseobject with the dynamically injecteduser_clientacross all core ticket endpoints (get_tickets,save_ticket,create_ticket,update_ticket,get_ticket_by_id).This guarantees that every database operation performed by a user automatically adheres to the strict RLS policies configured in the database layer, eliminating manual application-level tenant isolation risks.
Resolved Issue
Resolves #2188
Verification
401 Unauthorized.user_clientsuccessfully scopes data according to the RLS policy of the authenticated user.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Chores