Fix: Critical Security Vulnerability (IDOR) in evaluate-assessments Edge Function#174
Open
Varadraj75 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Open
Fix: Critical Security Vulnerability (IDOR) in evaluate-assessments Edge Function#174Varadraj75 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Varadraj75 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Conversation
- Create per-request Supabase client with user's JWT (enforces RLS) - Validate user identity via supabase.auth.getUser() - Use authenticated user.id instead of trusting patient_id from request body - Add error handling for failed DB inserts Fixes AOSSIE-Org#173
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds JWT authentication to the evaluate-assessments edge function, validates and extracts the authenticated user's ID, enforces request payload validation, verifies assessment and answer IDs, computes and persists assessment results, and centralizes error handling and standardized JSON responses. Changes
Sequence DiagramsequenceDiagram
participant Client
participant EdgeFunction as Edge Function
participant SupabaseAuth as Supabase Auth
participant SupabaseDB as Supabase DB
Client->>EdgeFunction: POST /evaluate-assessments (Authorization header + JSON)
EdgeFunction->>EdgeFunction: Validate Authorization header (401 if missing)
EdgeFunction->>SupabaseAuth: supabase.auth.getUser()
SupabaseAuth-->>EdgeFunction: user or error (401 on invalid)
EdgeFunction->>SupabaseDB: SELECT assessment by assessment_id
SupabaseDB-->>EdgeFunction: assessment or null (404 if not found)
EdgeFunction->>EdgeFunction: Validate payload, questions, answers (400 on invalid)
EdgeFunction->>EdgeFunction: Compute total score and isAutistic
EdgeFunction->>SupabaseDB: INSERT assessment_results (scoped by auth/RLS)
SupabaseDB-->>EdgeFunction: insert result or error (map errors to 400/403/500)
EdgeFunction->>Client: 200 + result JSON or appropriate error status
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Patches a critical IDOR vulnerability in the evaluate-assessments Supabase Edge Function by enforcing JWT-based authentication and binding writes to the authenticated user, preventing forged assessment submissions for arbitrary patient IDs.
Changes:
- Require
Authorizationheader and validate JWT viasupabase.auth.getUser(). - Create a per-request Supabase client using the request’s JWT to ensure RLS is applied.
- Stop trusting
patient_idfrom request body; useuser.idand add insert error handling.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
supabase/functions/evaluate-assessments/index.ts (1)
160-164: Add structured error logging in the catch path.Returning 500 without emitting context makes production debugging and incident triage harder. Log a sanitized error payload before responding.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/evaluate-assessments/index.ts` around lines 160 - 164, In the catch (error) block of this function add structured, sanitized error logging before returning the 500 response: capture a minimal payload (e.g., { message: String(error?.message), stack: String(error?.stack)?.split("\n")[0], requestId: ctx?.requestId || null }) and emit it via the existing logger (e.g., processLogger.error or console.error) so the log includes context but no sensitive data, then return the JSON 500 response as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/functions/evaluate-assessments/index.ts`:
- Around line 69-81: The current branch checks error before data and can return
500 even when the record is simply missing; invert and refine the checks in
evaluate-assessments/index.ts so you first detect missing results (e.g., if
(!data || data.length === 0) return new Response(JSON.stringify({ error:
"Assessment not found" }), { status: 404 });) and only afterwards handle genuine
query errors (if (error) return new Response(JSON.stringify({ error: "Internal
Server Error" }), { status: 500 });). Update the Response calls around the error
and data variables so not-found always yields 404 and real errors yield 500.
- Around line 109-125: The loop that currently uses continue to skip invalid or
unmatched answers (inside the for loop over answered_questions.questions where
matched_question and answer are resolved) should instead validate inputs: detect
duplicate question_id entries by tracking seen IDs (e.g., a Set of
question.question_id) and immediately return a 400 error if a duplicate is
found; likewise, if matched_question is not found or if answer is not found for
a given question.answer_id, return a 400 error indicating invalid question_id or
answer_id rather than silently continuing; update the handler that processes
answered_questions (the evaluate-assessments request handler) to perform these
checks before scoring so tampered submissions are rejected.
- Around line 59-89: Validate and type-check the incoming request body before
using it: wrap await req.json() in a try/catch to return a 400 on malformed
JSON, then verify that assessment_id is present and that questions is an array
whose elements have the expected shape (each contains question_id and answer_id)
before constructing answered_questions; if the shape is invalid return a 400
with a clear error message. Update the logic around req.json(), the extracted
assessment_id/questions, and the construction of AssessmentEvaluationRequestDTO
(and the use of AssessmentEvaluationQuestionDTO in questions.map) to perform
these checks and short-circuit with 400 for bad input.
---
Nitpick comments:
In `@supabase/functions/evaluate-assessments/index.ts`:
- Around line 160-164: In the catch (error) block of this function add
structured, sanitized error logging before returning the 500 response: capture a
minimal payload (e.g., { message: String(error?.message), stack:
String(error?.stack)?.split("\n")[0], requestId: ctx?.requestId || null }) and
emit it via the existing logger (e.g., processLogger.error or console.error) so
the log includes context but no sensitive data, then return the JSON 500
response as before.
- Add Content-Type: application/json header to all error responses - Validate and type-check request payload before processing - Map insert errors to proper status codes (403/400/500) - Check not-found before generic error to ensure correct 404 responses - Reject invalid or duplicate question_ids with 400 status - Add structured error logging in catch block for debugging
There was a problem hiding this comment.
♻️ Duplicate comments (1)
supabase/functions/evaluate-assessments/index.ts (1)
71-88:⚠️ Potential issue | 🟠 MajorValidate each
questions[]item before mapping to avoid 500s on malformed payloads.Top-level payload checks are good, but items inside
questionsare not shape-validated. Inputs likenullor objects missingquestion_id/answer_idcan throw during mapping and fall into generic 500 instead of returning 400.Suggested fix
+ const rawQuestions = (body as { questions?: unknown }).questions; + const hasValidQuestionShape = + Array.isArray(rawQuestions) && + rawQuestions.every( + (q): q is AssessmentEvaluationQuestionDTO => + !!q && + typeof q === "object" && + typeof (q as { question_id?: unknown }).question_id === "string" && + typeof (q as { answer_id?: unknown }).answer_id === "string" + ); + if ( !body || typeof body !== "object" || typeof (body as { assessment_id?: unknown }).assessment_id !== "string" || - !Array.isArray((body as { questions?: unknown }).questions) + !hasValidQuestionShape ) { return new Response( JSON.stringify({ - error: "Invalid payload: assessment_id and questions are required", + error: + "Invalid payload: assessment_id must be string and each question must include string question_id and answer_id", }), { status: 400, headers: JSON_HEADERS } ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/evaluate-assessments/index.ts` around lines 71 - 88, The payload validation currently checks only top-level body but not items inside questions; update the code after extracting const { assessment_id, questions } to iterate over questions and validate each item (ensure each item is a non-null object and has string question_id and answer_id, or otherwise matches AssessmentEvaluationQuestionDTO), and if any item fails return a 400 Response with a clear error; implement this check before any mapping/processing so functions that map questions won't throw on malformed entries (use the existing JSON_HEADERS and the same error Response pattern).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@supabase/functions/evaluate-assessments/index.ts`:
- Around line 71-88: The payload validation currently checks only top-level body
but not items inside questions; update the code after extracting const {
assessment_id, questions } to iterate over questions and validate each item
(ensure each item is a non-null object and has string question_id and answer_id,
or otherwise matches AssessmentEvaluationQuestionDTO), and if any item fails
return a 400 Response with a clear error; implement this check before any
mapping/processing so functions that map questions won't throw on malformed
entries (use the existing JSON_HEADERS and the same error Response pattern).
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
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.
Closes #173
📝 Description
This PR patches a critical IDOR (Insecure Direct Object Reference) security vulnerability in the
evaluate-assessmentsSupabase Edge Function. The function previously allowed any unauthenticated user to forge autism assessment results for arbitrary patient IDs by blindly trusting thepatient_idfrom the request body and using a global Supabase client initialized with the anonymous key — completely bypassing JWT authentication and Row Level Security (RLS).🔧 Changes Made
SUPABASE_ANON_KEY, which bypassed RLS for all requests.Authorizationheader now return401 Unauthorized.supabase.auth.getUser()to validate the authenticated user's identity before processing.patient_idfrom request body withuser.idextracted from the verified JWT — users can now only submit assessment results for themselves.File Modified:
supabase/functions/evaluate-assessments/index.ts📷 Screenshots or Visual Changes (if applicable)
N/A — This is a backend security fix with no UI changes.
🤝 Collaboration
Collaborated with: N/A
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes