⚔️ Vanguard: [Advanced Security Enhancement/Fix] IDOR in Teacher Notes#224
⚔️ Vanguard: [Advanced Security Enhancement/Fix] IDOR in Teacher Notes#224ldsgroups225 wants to merge 1 commit intomasterfrom
Conversation
* Fixed missing tenant-isolation in updateStudentNote and deleteStudentNote queries by enforcing a schoolId check in the where clause * Fixed missing session validation in updateStudentNote and deleteStudentNote server functions by adding getTeacherContext() to ensure an active session * Passed down retrieved context schoolId and teacherId to backend queries
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis change addresses a multi-tenant isolation vulnerability (IDOR/BOLA) in student note operations. Changes
Sequence DiagramsequenceDiagram
participant Client
participant ServerFn as Server Function<br/>(student-notes.ts)
participant AuthCtx as getTeacherContext()
participant DataLayer as Data Query<br/>(teacher-notes.ts)
participant DB as Database
Client->>ServerFn: updateStudentNote(noteId, ...)
ServerFn->>AuthCtx: getTeacherContext()
AuthCtx-->>ServerFn: { teacherId, schoolId } or null
alt No Auth Context
ServerFn-->>Client: { success: false, error: 'UNAUTHORIZED' }
else Authorized
ServerFn->>DataLayer: updateStudentNote({ schoolId, noteId, teacherId, ... })
DataLayer->>DB: UPDATE conductRecords<br/>WHERE id = noteId<br/>AND schoolId = schoolId
DB-->>DataLayer: row updated
DataLayer-->>ServerFn: { success: true }
ServerFn-->>Client: { success: true }
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/teacher/src/teacher/functions/student-notes.ts (2)
147-167:⚠️ Potential issue | 🟡 MinorSecurity fix is correct, but backend errors are not handled.
The
getTeacherContext()check andschoolIdpropagation correctly address the IDOR vulnerability. However,updateNote()can throw"Failed to update note"when no row is returned. This exception will propagate to the client unhandled.As per coding guidelines: "Do not throw raw exceptions in server functions; handle errors appropriately before returning results."
🛡️ Proposed fix to wrap the call in try-catch
const { updateStudentNote: updateNote } = await import('@repo/data-ops/queries/teacher-notes') - const note = await updateNote({ - schoolId: context.schoolId, - noteId: data.noteId, - teacherId: context.teacherId, - title: data.title, - content: data.content, - priority: data.priority, - }) - - return { - success: true, - note, + try { + const note = await updateNote({ + schoolId: context.schoolId, + noteId: data.noteId, + teacherId: context.teacherId, + title: data.title, + content: data.content, + priority: data.priority, + }) + return { success: true, note } + } catch { + return { success: false, error: 'UPDATE_FAILED' } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/teacher/src/teacher/functions/student-notes.ts` around lines 147 - 167, Wrap the call to updateNote (imported as updateStudentNote/updateNote) inside a try-catch after the getTeacherContext() check so backend exceptions are not propagated to the client; catch any thrown error from updateNote and return a controlled failure response (e.g. { success: false, error: 'UPDATE_FAILED' } or include a sanitized message) instead of letting the raw exception bubble up, and log the original error server-side for diagnostics. Ensure this change occurs in the same server function that calls getTeacherContext() and returns the note.
173-189:⚠️ Potential issue | 🟡 MinorSame error handling concern as
updateStudentNote.The security fix is correctly implemented. Database errors from
deleteNote()should be caught and returned as a failure response rather than propagating as raw exceptions.🛡️ Proposed fix
const { deleteStudentNote: deleteNote } = await import('@repo/data-ops/queries/teacher-notes') - await deleteNote({ - schoolId: context.schoolId, - noteId: data.noteId, - teacherId: context.teacherId, - }) - - return { - success: true, + try { + await deleteNote({ + schoolId: context.schoolId, + noteId: data.noteId, + teacherId: context.teacherId, + }) + return { success: true } + } catch { + return { success: false, error: 'DELETE_FAILED' } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/teacher/src/teacher/functions/student-notes.ts` around lines 173 - 189, The deleteStudentNote handler currently calls deleteNote(...) (imported from `@repo/data-ops/queries/teacher-notes`) without catching database errors; mirror the updateStudentNote fix by wrapping the deleteNote call in a try/catch after getTeacherContext() succeeds and return a failure response instead of letting exceptions bubble up — e.g., catch any error from deleteNote and return { success: false, error: error.message || 'INTERNAL_ERROR' } (or a standardized error code) while preserving the existing UNAUTHORIZED check using getTeacherContext().packages/data-ops/src/queries/teacher-notes.ts (1)
130-161:⚠️ Potential issue | 🟠 MajorResultAsync pattern is mandatory for data-ops queries — refactor to return
ResultAsync<T, DatabaseError>instead of throwing.The
schoolIdconstraint in the WHERE clause correctly prevents cross-tenant modifications. However, this function violates the mandatory pattern forpackages/data-ops/src/queries/**/*.ts: it must returnResultAsync<T, DatabaseError>from@praha/byethrowand never throw raw exceptions. All other query files in this directory follow this pattern. Replace thethrow new Error(...)with Result monad error handling and attach.mapErr(tapLogErr(logger, context))for consistent error logging.Additionally,
teacherIdis accepted as a parameter but never used in the query logic. If the intent is to restrict updates to only the teacher who created the note, add an authorization check to the WHERE clause; otherwise, remove the unused parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/data-ops/src/queries/teacher-notes.ts` around lines 130 - 161, The updateStudentNote function currently throws on failure and ignores the teacherId param; refactor it to return ResultAsync<{id,title,content,type,priority,createdAt}, DatabaseError> (from `@praha/byethrow`) instead of throwing: convert the DB call block (getDb().update(conductRecords).set(...).where(...).returning()) into a ResultAsync, map the successful row to the expected return shape, and replace the throw new Error('Failed to update note') with a ResultAsync failure using the DatabaseError type; also attach .mapErr(tapLogErr(logger, {context: 'updateStudentNote', noteId: params.noteId})) to the result for consistent logging. While refactoring either remove the unused teacherId parameter from updateStudentNote or enforce authorization by adding eq(conductRecords.teacherId, params.teacherId) to the WHERE clause so the tenant+teacher constraint is applied.
🧹 Nitpick comments (1)
apps/teacher/src/teacher/functions/student-notes.ts (1)
107-125: Consider: Read operations lack tenant scoping.While this PR correctly fixes the write operations,
getStudentNotes,getBehaviorSummary, andgetNotesTrenddo not callgetTeacherContext()or filter byschoolId. A teacher knowing a student ID from another school could potentially read their notes.This is out of scope for this security fix but worth tracking separately. Based on learnings: "All database queries involving school-scoped data must include tenant filtering with
where(eq(table.schoolId, ctx.schoolId))."Do you want me to open an issue to track adding tenant scoping to the read operations?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/teacher/src/teacher/functions/student-notes.ts` around lines 107 - 125, getStudentNotes (and similarly getBehaviorSummary and getNotesTrend) perform read operations without tenant scoping; update each handler to call getTeacherContext() to obtain ctx and ensure all DB queries filter by the teacher's schoolId (e.g., add where(eq(table.schoolId, ctx.schoolId)) in the query builder) so reads are limited to the teacher's school; locate these handlers by the exported function names getStudentNotes, getBehaviorSummary, getNotesTrend and add the context fetch and schoolId where-clause to the imported query calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/teacher/src/teacher/functions/student-notes.ts`:
- Around line 147-167: Wrap the call to updateNote (imported as
updateStudentNote/updateNote) inside a try-catch after the getTeacherContext()
check so backend exceptions are not propagated to the client; catch any thrown
error from updateNote and return a controlled failure response (e.g. { success:
false, error: 'UPDATE_FAILED' } or include a sanitized message) instead of
letting the raw exception bubble up, and log the original error server-side for
diagnostics. Ensure this change occurs in the same server function that calls
getTeacherContext() and returns the note.
- Around line 173-189: The deleteStudentNote handler currently calls
deleteNote(...) (imported from `@repo/data-ops/queries/teacher-notes`) without
catching database errors; mirror the updateStudentNote fix by wrapping the
deleteNote call in a try/catch after getTeacherContext() succeeds and return a
failure response instead of letting exceptions bubble up — e.g., catch any error
from deleteNote and return { success: false, error: error.message ||
'INTERNAL_ERROR' } (or a standardized error code) while preserving the existing
UNAUTHORIZED check using getTeacherContext().
In `@packages/data-ops/src/queries/teacher-notes.ts`:
- Around line 130-161: The updateStudentNote function currently throws on
failure and ignores the teacherId param; refactor it to return
ResultAsync<{id,title,content,type,priority,createdAt}, DatabaseError> (from
`@praha/byethrow`) instead of throwing: convert the DB call block
(getDb().update(conductRecords).set(...).where(...).returning()) into a
ResultAsync, map the successful row to the expected return shape, and replace
the throw new Error('Failed to update note') with a ResultAsync failure using
the DatabaseError type; also attach .mapErr(tapLogErr(logger, {context:
'updateStudentNote', noteId: params.noteId})) to the result for consistent
logging. While refactoring either remove the unused teacherId parameter from
updateStudentNote or enforce authorization by adding
eq(conductRecords.teacherId, params.teacherId) to the WHERE clause so the
tenant+teacher constraint is applied.
---
Nitpick comments:
In `@apps/teacher/src/teacher/functions/student-notes.ts`:
- Around line 107-125: getStudentNotes (and similarly getBehaviorSummary and
getNotesTrend) perform read operations without tenant scoping; update each
handler to call getTeacherContext() to obtain ctx and ensure all DB queries
filter by the teacher's schoolId (e.g., add where(eq(table.schoolId,
ctx.schoolId)) in the query builder) so reads are limited to the teacher's
school; locate these handlers by the exported function names getStudentNotes,
getBehaviorSummary, getNotesTrend and add the context fetch and schoolId
where-clause to the imported query calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8b8de68b-7df3-4c9f-842e-370b99b7267f
📒 Files selected for processing (3)
.jules/vanguard.mdapps/teacher/src/teacher/functions/student-notes.tspackages/data-ops/src/queries/teacher-notes.ts
1 participant
Vulnerability Path: The
updateStudentNoteanddeleteStudentNoteserver functions inapps/teacher/src/teacher/functions/student-notes.tslacked agetTeacherContext()call to enforce an active session and retrieve the user'sschoolId. Consequently, these functions passed an emptyteacherIdand omitted passing aschoolIdto the backend data functions. The backend queries inpackages/data-ops/src/queries/teacher-notes.tswere executingUPDATEandDELETEstatements on theconductRecordstable solely bynoteId, opening a direct IDOR/BOLA vulnerability where a user in Tenant A could modify or delete notes belonging to Tenant B by guessing thenoteId.Action Taken: Added
getTeacherContext()to theupdateStudentNoteanddeleteStudentNoteserver functions to explicitly retrieve the active session context and explicitly pass theschoolIdandteacherIdarguments. Updated the backendupdateStudentNoteanddeleteStudentNotequery parameters to mandateschoolIdand applied a strictand(eq(conductRecords.id, params.noteId), eq(conductRecords.schoolId, params.schoolId))condition to the DrizzleWHEREclauses, effectively eliminating the IDOR/BOLA vulnerability by strictly enforcing tenant isolation.PR created automatically by Jules for task 790790760363773327 started by @ldsgroups225
Summary by CodeRabbit