fix: resolve IDOR vulnerability exposing private user data (#1853) - #1866
Open
atul-upadhyay-7 wants to merge 3 commits into
Open
fix: resolve IDOR vulnerability exposing private user data (#1853)#1866atul-upadhyay-7 wants to merge 3 commits into
atul-upadhyay-7 wants to merge 3 commits into
Conversation
- Enforce strict algorithm validation (reject 'none' algorithm, only accept HS256) - Add timing-safe signature comparison to prevent timing attacks - Validate JWT claims: exp, iat, iss, aud - Add SUPABASE_JWT_SECRET to env config and .env.example - Apply timing-safe comparison to cron and webhook secret verification - Defense-in-depth against crafted JWTs with elevated roles
…05#1853) - Add requireOwnershipOrAdmin middleware for server-side resource ownership validation - Add UUID format validation on all user ID parameters to prevent injection - Add secure /api/users/:userId/profile GET endpoint with field-level access control (public fields for other users, private fields for profile owner/admins only) - Add secure /api/users/:userId/profile PUT endpoint with ownership enforcement - Strengthen notification endpoint authorization with strict UUID validation and explicit IDOR blocking with audit logging - Create SQL migration for profiles RLS hardening with documented security boundary - Update Profile.tsx and EditProfile.tsx to use server-side endpoints instead of direct Supabase client calls, ensuring all profile access is authorized server-side Fixes durdana3105#1853
|
@atul-upadhyay-7 is attempting to deploy a commit to the durdana3105's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
📝 WalkthroughWalkthroughThe PR hardens local JWT verification, adds ownership-or-admin authorization, introduces protected profile endpoints, migrates profile pages to those endpoints, validates notification targets, and updates profile SELECT policies. ChangesSecurity and profile access hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProfilePage
participant SupabaseAuth
participant UsersAPI
participant RequireAuth
participant OwnershipCheck
participant SupabaseAdmin
ProfilePage->>SupabaseAuth: Get access token
ProfilePage->>UsersAPI: GET or PUT /api/users/:userId/profile
UsersAPI->>RequireAuth: Verify Bearer JWT
RequireAuth->>OwnershipCheck: Check owner or admin for PUT
OwnershipCheck->>SupabaseAdmin: Read or update profile
SupabaseAdmin-->>UsersAPI: Profile response
UsersAPI-->>ProfilePage: JSON result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (6)
backend/controllers/notificationController.js (1)
64-76: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake webhook privilege explicit rather than inferring it from a missing user.
req.user === undefinedis treated as permission to target any user. The current route establishes webhook authentication, but a future route mount without that middleware would silently bypass ownership checks. Set an explicit trusted-auth marker inverifyNotificationAuth, and reject requests that have neither that marker nor an authenticated 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/controllers/notificationController.js` around lines 64 - 76, Update verifyNotificationAuth to set an explicit trusted webhook-authentication marker on the request after successful webhook validation. In the notification authorization block, allow cross-user targeting only for admins or requests carrying that marker; reject requests with neither an authenticated req.user nor the trusted marker, while preserving self-targeting for standard authenticated users.backend/config.js (2)
17-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
SITE_URLlost its URL-format validation.Per the change summary this went from
z.string().url()to a plain optional string, while sibling URL vars (FRONTEND_URL,CLIENT_URL,PASSWORD_RESET_BASE_URL) keep.url(). If the relaxation was to allow a non-absolute value, it's worth a comment; otherwise restore.url()for consistency.Proposed change
- SITE_URL: z.string().optional(), + SITE_URL: z.string().url().optional(),🤖 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/config.js` at line 17, Update the SITE_URL schema definition to restore URL-format validation with the same optional URL pattern used by FRONTEND_URL, CLIENT_URL, and PASSWORD_RESET_BASE_URL. Only retain a plain optional string if the non-absolute-value requirement is intentional and document that exception.
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMisleading validation message on an optional field.
.min(1, "SUPABASE_JWT_SECRET is required for JWT verification").optional()never surfaces that message when the variable is absent — only when it is present-but-empty. If local JWT verification is meant to be mandatory in production, enforce it with asuperRefineonNODE_ENVinstead of relying on the message text.🤖 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/config.js` at line 10, Update the SUPABASE_JWT_SECRET validation and NODE_ENV configuration schema so JWT verification is required when running in production, using superRefine to add the required-field issue conditionally. Keep the secret optional outside production and replace the misleading min-length message with wording appropriate for a present-but-empty value.src/pages/EditProfile.tsx (1)
114-117: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe server's error message is parsed then thrown away.
Lines 114-117 carefully extract
errorData.error, but the catch at Line 123 always shows the generic "Error updating profile". Surface the thrown message so actionable server responses (e.g. "Name must be 100 characters or fewer") reach the 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 `@src/pages/EditProfile.tsx` around lines 114 - 117, Update the error handling in the profile submission flow around the response check and its catch block so the caught Error message is displayed to the user instead of always using the generic “Error updating profile” text. Preserve the server-derived message created by the non-OK response branch, including actionable validation details.src/pages/Profile.tsx (1)
94-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe profile fetch/save logic is now duplicated verbatim with
src/pages/EditProfile.tsx.Both pages independently do
getSession()→ buildAuthorizationheaders →fetch(${API_BASE_URL}/api/users/${id}/profile)→ hand-rollres.okerror parsing. Extracting a smallsrc/lib/profileApi.tswithgetProfile(userId)/updateProfile(userId, payload)would keep the auth header and error-shape contract in one place as the backend endpoint evolves.🤖 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 `@src/pages/Profile.tsx` around lines 94 - 127, The profile request flow in handleSave is duplicated with EditProfile.tsx. Extract shared getProfile(userId) and updateProfile(userId, payload) helpers into src/lib/profileApi.ts, centralizing session retrieval, Authorization headers, profile endpoint fetches, and res.ok error parsing; update both pages to use these helpers while preserving their existing payload and UI behavior.backend/routes/users.js (1)
152-170: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAllowlisted fields are copied through without type validation.
Only
name/bioget a length check, and even that is skipped for non-strings (updates.name && typeof updates.name === "string"), so{ name: { $x: 1 } }or a 10k-elementskillsarray is forwarded straight to Postgres — producing a 500 at best, oversized rows at worst. Consider a small per-field schema (type + max length + max array size), e.g. with zod, which is already a dependency in this backend.🤖 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/routes/users.js` around lines 152 - 170, Add schema-based validation for the allowlisted fields collected in updates, using the existing zod dependency and defining each field’s expected type plus maximum string lengths and array sizes. Validate updates before the database operation, reject malformed values such as object-valued name or oversized skills with a 400 response, and remove reliance on truthiness-based checks that skip invalid non-string values.
🤖 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/middlewares/requireAuth.js`:
- Around line 302-312: Case-insensitive UUID validation is followed by
case-sensitive ownership checks, incorrectly rejecting uppercase UUIDs. In
backend/middlewares/requireAuth.js lines 302-312, normalize both req.user.id and
targetUserId before the ownership comparison; apply the same normalization to
both sides of the ownership check in backend/routes/users.js lines 106-112 so
owners are recognized consistently.
- Line 153: Update the requireAdminRole route middleware chain before
requireOwnershipOrAdmin so it performs the profile lookup and populates
req.roles from the application role source, including profiles.is_admin and
active-role status; do not rely on requireAuth’s req.user.role, which only
contains the Supabase RLS role. Ensure ownership and admin account holders
receive the same bypass behavior as other profile-authenticated requests.
- Around line 60-70: Update the JWT validation logic in the requireAuth
middleware: require a numeric payload.exp and reject tokens when it is missing
or expired, and replace the literal payload.iss === "supabase" expectation with
validation that accepts the hosted Supabase issuer URL format while still
rejecting invalid issuers. Preserve the existing iat validation.
In `@backend/routes/users.js`:
- Around line 172-184: Update the profile update flow after the existing
Supabase error check to detect when the returned profile from maybeSingle() is
null, and return a 404 response instead of success. Preserve the current 500
handling for Supabase errors and the successful response for non-null profiles
in the profile update handler.
In `@src/pages/EditProfile.tsx`:
- Line 29: Update the profile-loading effect around the no-session guard and its
catch path so both failures set the profile loading sentinel to an explicit
empty/error state before navigating to the login page. Add the required navigate
dependency to the effect dependency list, while preserving the existing
successful profile-loading flow.
In `@supabase/migrations/20260727000000_idor_fix_private_profile_access.sql`:
- Around line 21-24: Update the migration header comment to accurately describe
the SELECT policy and column-level protection, removing the claim that it only
documents the policy or otherwise overstates database-layer privacy. Preserve
the explanation that public row access remains intentional for
PublicPortfolio.tsx and peer discovery, while clarifying the actual scope of the
migration.
- Around line 30-45: Replace the blanket SELECT policies on public.profiles that
use USING (true) with column-restricted access for authenticated and anonymous
users. Revoke broad table SELECT permission and grant only the public profile
columns, or expose them through a public-only view, keeping the selected columns
synchronized with PUBLIC_PROFILE_FIELDS and excluding private fields such as
email, last_active, availability, and learning_goals.
---
Nitpick comments:
In `@backend/config.js`:
- Line 17: Update the SITE_URL schema definition to restore URL-format
validation with the same optional URL pattern used by FRONTEND_URL, CLIENT_URL,
and PASSWORD_RESET_BASE_URL. Only retain a plain optional string if the
non-absolute-value requirement is intentional and document that exception.
- Line 10: Update the SUPABASE_JWT_SECRET validation and NODE_ENV configuration
schema so JWT verification is required when running in production, using
superRefine to add the required-field issue conditionally. Keep the secret
optional outside production and replace the misleading min-length message with
wording appropriate for a present-but-empty value.
In `@backend/controllers/notificationController.js`:
- Around line 64-76: Update verifyNotificationAuth to set an explicit trusted
webhook-authentication marker on the request after successful webhook
validation. In the notification authorization block, allow cross-user targeting
only for admins or requests carrying that marker; reject requests with neither
an authenticated req.user nor the trusted marker, while preserving
self-targeting for standard authenticated users.
In `@backend/routes/users.js`:
- Around line 152-170: Add schema-based validation for the allowlisted fields
collected in updates, using the existing zod dependency and defining each
field’s expected type plus maximum string lengths and array sizes. Validate
updates before the database operation, reject malformed values such as
object-valued name or oversized skills with a 400 response, and remove reliance
on truthiness-based checks that skip invalid non-string values.
In `@src/pages/EditProfile.tsx`:
- Around line 114-117: Update the error handling in the profile submission flow
around the response check and its catch block so the caught Error message is
displayed to the user instead of always using the generic “Error updating
profile” text. Preserve the server-derived message created by the non-OK
response branch, including actionable validation details.
In `@src/pages/Profile.tsx`:
- Around line 94-127: The profile request flow in handleSave is duplicated with
EditProfile.tsx. Extract shared getProfile(userId) and updateProfile(userId,
payload) helpers into src/lib/profileApi.ts, centralizing session retrieval,
Authorization headers, profile endpoint fetches, and res.ok error parsing;
update both pages to use these helpers while preserving their existing payload
and UI behavior.
🪄 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 Plus
Run ID: 9b718fcf-2443-4cd1-a356-5b983dbba5b6
📒 Files selected for processing (7)
backend/config.jsbackend/controllers/notificationController.jsbackend/middlewares/requireAuth.jsbackend/routes/users.jssrc/pages/EditProfile.tsxsrc/pages/Profile.tsxsupabase/migrations/20260727000000_idor_fix_private_profile_access.sql
Owner
|
please resolve merge conflicts |
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.
Summary
This PR fixes the Insecure Direct Object Reference (IDOR) vulnerability described in #1853. The fix ensures that the server validates user ownership before returning or allowing modification of private user data.
Problem
The API endpoints for fetching user match data and private profile details relied on user IDs provided in request payloads or URL parameters without adequate server-side authorization checks. Any authenticated user could potentially modify the ID parameter to access private data belonging to other users.
Changes
Backend
requireOwnershipOrAdminmiddleware (backend/middlewares/requireAuth.js): Reusable middleware that validates resource ownership by comparing the authenticated user's JWT ID against the target resource's owner ID. Includes strict UUID v4 format validation and admin bypass.backend/routes/users.js):GET /api/users/:userId/profile— Returns public profile fields to all authenticated users, but private fields (email, last_active, etc.) only to the profile owner or admins.PUT /api/users/:userId/profile— Profile update with ownership enforcement viarequireOwnershipOrAdminmiddleware.backend/controllers/notificationController.js): Added strict UUID v4 validation onuser_idparameter, explicit IDOR blocking with audit logging.Database
supabase/migrations/20260727000000_idor_fix_private_profile_access.sql): Documents the profiles table RLS security boundary.Frontend
/api/users/:id/profileendpoints instead of direct Supabase client calls.Testing
Fixes #1853
Summary by CodeRabbit
New Features
Bug Fixes