Fix/critical stability, security, and logic fixes#7
Fix/critical stability, security, and logic fixes#7Kalebtes2031 wants to merge 22 commits intoJavaScript-Mastery-Pro:mainfrom
Conversation
…nauthorized access
…board student count
…event silent failures
… negative timestamps
…o prevent 500 errors
📝 WalkthroughWalkthroughThe pull request enforces resource ownership across API endpoints by adding Changes
Poem
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/api/grades/route.ts (1)
76-86:⚠️ Potential issue | 🟠 MajorAdd ObjectId validation for
studentIdand enforce defaultmaxMarksbefore upsert.The POST handler lacks two critical validations that the GET handler includes:
Missing ObjectId validation: GET validates
studentIdwithmongoose.Types.ObjectId.isValid()(line 47), but POST accepts any non-empty string, allowing invalid IDs to reach the database.Unenforced default
maxMarks: When the client omitsmaxMarks, the API usesmax = 100only forcalcGrade(), but the database receives...data(which has nomaxMarksfield). The Zod refine and Mongoose pre-hook only validatemarks <= maxMarksifmaxMarksexists, somarks: 150is stored withoutmaxMarks, silently exceeding the intended 100-point scale.Add ObjectId validation and explicit
maxMarksenforcement before the upsert:Proposed fix
const parsed = GradeSchema.safeParse(body) if (!parsed.success) return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }) const data = parsed.data + if (!mongoose.Types.ObjectId.isValid(data.studentId)) { + return NextResponse.json({ error: 'Invalid studentId' }, { status: 400 }) + } + const max = data.maxMarks ?? 100 + if (data.marks > max) { + return NextResponse.json( + { error: 'marks must be less than or equal to maxMarks' }, + { status: 400 }, + ) + } const term = data.term ?? 'Term 1' const grade = await Grade.findOneAndUpdate( { teacherId: userId, studentId: data.studentId, subject: data.subject, term }, - { $set: { ...data, term, teacherId: userId, grade: calcGrade(data.marks, max) } }, + { $set: { ...data, maxMarks: max, term, teacherId: userId, grade: calcGrade(data.marks, max) } }, { upsert: true, new: true } )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/grades/route.ts` around lines 76 - 86, Validate parsed.data.studentId using mongoose.Types.ObjectId.isValid() and return a 400 NextResponse if invalid; compute const max = data.maxMarks ?? 100 and then assign data.maxMarks = max before calling Grade.findOneAndUpdate so the stored document includes maxMarks; ensure calcGrade(data.marks, max) is used to set the grade and keep the upsert call to Grade.findOneAndUpdate({ teacherId: userId, studentId: data.studentId, subject: data.subject, term }, { $set: { ...data, term, teacherId: userId, grade: calcGrade(data.marks, max) } }, { upsert: true, new: true }) so validation and pre-hooks receive the enforced maxMarks and a valid ObjectId for studentId.models/Grade.ts (1)
44-53:⚠️ Potential issue | 🟠 MajorPartial updates bypass the
marks ≤ maxMarksvalidation.Lines 49–52 and 66–69 require both
marksandmaxMarksto be present in the same update. A partial update like{ marks: 150 }passes validation becausemaxMarksisundefined, allowing the invariant to be broken against the existing document. The same applies in reverse for{ maxMarks: 80 }when existing marks exceed it.Fetch the existing document within the middleware and merge it with the incoming update before validating:
Proposed fix
+async function validateMarksUpdate( + query: Record<string, unknown>, + update: Record<string, unknown>, + modelRef: mongoose.Model<IGrade>, +) { + const set = update.$set && typeof update.$set === "object" + ? update.$set as Record<string, unknown> + : {} + const source = { ...update, ...set } + + if (source.marks === undefined && source.maxMarks === undefined) return + + const existing = await modelRef.findOne(query).select("marks maxMarks").lean() + const marks = typeof source.marks === "number" ? source.marks : existing?.marks + const maxMarks = typeof source.maxMarks === "number" ? source.maxMarks : existing?.maxMarks + + if (typeof marks === "number" && typeof maxMarks === "number" && marks > maxMarks) { + throw new Error("marks must be less than or equal to maxMarks") + } +} + -GradeSchema.pre("findOneAndUpdate", function () { +GradeSchema.pre("findOneAndUpdate", async function () { const update = this.getUpdate() as Record<string, unknown>; if (update && typeof update === "object") { - // Support both direct updates and $set operator - const source = (update.$set && typeof update.$set === "object" ? update.$set : update) as Record<string, unknown>; - const marks = source.marks; - const maxMarks = source.maxMarks; - if ( - marks !== undefined && typeof marks === "number" && - maxMarks !== undefined && typeof maxMarks === "number" && - marks > maxMarks - ) { - throw new Error("marks must be less than or equal to maxMarks"); - } + await validateMarksUpdate(this.getQuery() as Record<string, unknown>, update, this.model as mongoose.Model<IGrade>) } }); -GradeSchema.pre("updateOne", function () { +GradeSchema.pre("updateOne", async function () { const update = this.getUpdate() as Record<string, unknown>; if (update && typeof update === "object") { - // Support both direct updates and $set operator - const source = (update.$set && typeof update.$set === "object" ? update.$set : update) as Record<string, unknown>; - const marks = source.marks; - const maxMarks = source.maxMarks; - if ( - marks !== undefined && typeof marks === "number" && - maxMarks !== undefined && typeof maxMarks === "number" && - marks > maxMarks - ) { - throw new Error("marks must be less than or equal to maxMarks"); - } + await validateMarksUpdate(this.getQuery() as Record<string, unknown>, update, this.model as mongoose.Model<IGrade>) } });Also applies to lines 61–70.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/Grade.ts` around lines 44 - 53, The validation currently reads the incoming update only (via source) so partial updates like {marks} or {maxMarks} skip the invariant; change the middleware to load the existing Grade document (by using this.getQuery() / this.findOne() inside the pre hook that handles updates), merge the persisted values with the incoming source (accounting for both direct fields and update.$set) to produce resolved marks and maxMarks, then run the existing comparison (marks !== undefined && maxMarks !== undefined && marks > maxMarks) against the merged values; adjust the same logic used around the source/marks/maxMarks variables so both full and partial updates validate correctly.app/api/attendance/route.ts (1)
163-166:⚠️ Potential issue | 🟡 MinorCatch-block logging inconsistent with GET handler.
The GET handler (lines 92-95) logs
err instanceof Error ? err.message : err, but this POST catch logs the fullerr, which can include the stack trace. For consistency with the PR's "sanitize catch blocks" goal, normalize this too. The response body is already safe; this is about log hygiene only.🔧 Proposed change
} catch (err) { - console.error(err) + console.error( + 'POST /api/attendance error:', + err instanceof Error ? err.message : err, + ) return NextResponse.json({ error: 'Internal server error' }, { status: 500 }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/attendance/route.ts` around lines 163 - 166, In the POST handler's catch block inside the route.ts file (the export async function POST / POST request handler), replace the current console.error(err) with the same sanitized logging used in the GET handler — e.g., console.error(err instanceof Error ? err.message : err) — so logs only the error message (not full stack/object); keep the existing NextResponse.json({ error: 'Internal server error' }, { status: 500 }) response unchanged.
🧹 Nitpick comments (4)
app/dashboard/OverviewClient.tsx (1)
203-207: LGTM on the response-OK gate.Validating every
Responsebefore calling.json()is the right fix; non-OK HTML/error bodies will now surface a clear error instead of throwing opaque JSON parse failures downstream.One optional nit: the thrown
Errorincludesres.url, which for same-origin fetches is a relative-resolved absolute URL and typically harmless, but if any of these endpoints ever carry query params with user identifiers, that message flows intosetError(...)and gets rendered in the UI on line 395. Consider a more generic user-facing message and logging the detailed URL/status separately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/dashboard/OverviewClient.tsx` around lines 203 - 207, The current error thrown in the response-OK gate exposes res.url (e.g. in the loop over studentsRes, assignmentsRes, attendanceRes, gradesRes, announcementsRes) which can flow into setError(...) and be shown to users; change the thrown Error to a generic, user-safe message (e.g. "Failed to load data") and move the detailed diagnostic (res.url and res.status) into a separate log call (console.error or processLogger.error) before throwing, so setError gets a safe string while devs still see the URL/status for debugging.app/api/students/[id]/route.ts (2)
8-8: Dead code:ALLOWED_UPDATE_FIELDSis no longer referenced.Since the zod schema now governs the allowed update fields, this whitelist constant is unused and should be removed to avoid drift (note it also incorrectly listed
grade, which is not on theStudentmodel).🔧 Proposed cleanup
-const ALLOWED_UPDATE_FIELDS = ['name', 'email', 'grade', 'rollNo', 'class', 'phone', 'address', 'parentName', 'parentPhone'] - export async function PUT(req: NextRequest, ctx: { params: Promise<{ id: string }> }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/students/`[id]/route.ts at line 8, Remove the now-unused constant ALLOWED_UPDATE_FIELDS from the file because updates are validated by the zod schema and the whitelist is dead code (and incorrectly lists non-existent fields like grade); locate the declaration of ALLOWED_UPDATE_FIELDS in route.ts and delete it, then run a quick search in the module for any remaining references (none should exist) to ensure no usages remain and run tests/TS build to confirm no compile errors.
29-41: HoistStudentUpdateSchemato module scope.The schema is reconstructed on every PUT request. This matches the pattern used in
app/api/students/route.tsandapp/api/attendance/route.ts, whereStudentSchema,AttendanceSchema, andBulkSchemaare all defined at module level, and avoids per-request allocation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/students/`[id]/route.ts around lines 29 - 41, StudentUpdateSchema is being created inside the request handler causing per-request reconstruction; move the StudentUpdateSchema definition to module scope (top of the file) so it’s built once, then use StudentUpdateSchema.safeParse(body) in the handler and keep the existing parsed/error handling (parsed, parsed.success, NextResponse.json) unchanged; ensure the exported/available symbol is used by the PUT handler and update any imports/refs if needed.app/api/attendance/route.ts (1)
132-162: LGTM — bulk and singlestudentIdvalidation before upsert.Validation runs after Zod parse but before any DB access, and the bulk path short-circuits on the first invalid id with a helpful message. Filter/
$setshapes correctly scope byteacherId.Minor optional polish: the bulk loop could accumulate every invalid index/id and return them in a single response for better client UX on large payloads, but this is not required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/attendance/route.ts` around lines 132 - 162, No functional fix required—the current post-Zod validation using mongoose.Types.ObjectId.isValid for BulkSchema and AttendanceSchema before calling Attendance.bulkWrite or Attendance.findOneAndUpdate is correct; if you want the optional polish, in the bulk branch (where data is typed as z.infer<typeof BulkSchema>) replace the early-return-on-first-invalid loop with a collector that accumulates all invalid {index, studentId} pairs and, if any exist, return a single 400 JSON response listing them (rather than short-circuiting on the first invalid id) so the client receives all validation failures at once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/dashboard/OverviewClient.tsx`:
- Line 324: Update totalAssignments to use the paginated total field rather than
deriving from the possibly truncated array: replace the current expression that
uses assignments.assignments?.length with an expression that prefers
assignments.total (e.g., assignments.total ?? assignments.assignments?.length ??
0). Modify the totalAssignments assignment in OverviewClient (where
totalStudents was fixed) so it mirrors the totalStudents pattern and uses the
paginated response's total property.
---
Outside diff comments:
In `@app/api/attendance/route.ts`:
- Around line 163-166: In the POST handler's catch block inside the route.ts
file (the export async function POST / POST request handler), replace the
current console.error(err) with the same sanitized logging used in the GET
handler — e.g., console.error(err instanceof Error ? err.message : err) — so
logs only the error message (not full stack/object); keep the existing
NextResponse.json({ error: 'Internal server error' }, { status: 500 }) response
unchanged.
In `@app/api/grades/route.ts`:
- Around line 76-86: Validate parsed.data.studentId using
mongoose.Types.ObjectId.isValid() and return a 400 NextResponse if invalid;
compute const max = data.maxMarks ?? 100 and then assign data.maxMarks = max
before calling Grade.findOneAndUpdate so the stored document includes maxMarks;
ensure calcGrade(data.marks, max) is used to set the grade and keep the upsert
call to Grade.findOneAndUpdate({ teacherId: userId, studentId: data.studentId,
subject: data.subject, term }, { $set: { ...data, term, teacherId: userId,
grade: calcGrade(data.marks, max) } }, { upsert: true, new: true }) so
validation and pre-hooks receive the enforced maxMarks and a valid ObjectId for
studentId.
In `@models/Grade.ts`:
- Around line 44-53: The validation currently reads the incoming update only
(via source) so partial updates like {marks} or {maxMarks} skip the invariant;
change the middleware to load the existing Grade document (by using
this.getQuery() / this.findOne() inside the pre hook that handles updates),
merge the persisted values with the incoming source (accounting for both direct
fields and update.$set) to produce resolved marks and maxMarks, then run the
existing comparison (marks !== undefined && maxMarks !== undefined && marks >
maxMarks) against the merged values; adjust the same logic used around the
source/marks/maxMarks variables so both full and partial updates validate
correctly.
---
Nitpick comments:
In `@app/api/attendance/route.ts`:
- Around line 132-162: No functional fix required—the current post-Zod
validation using mongoose.Types.ObjectId.isValid for BulkSchema and
AttendanceSchema before calling Attendance.bulkWrite or
Attendance.findOneAndUpdate is correct; if you want the optional polish, in the
bulk branch (where data is typed as z.infer<typeof BulkSchema>) replace the
early-return-on-first-invalid loop with a collector that accumulates all invalid
{index, studentId} pairs and, if any exist, return a single 400 JSON response
listing them (rather than short-circuiting on the first invalid id) so the
client receives all validation failures at once.
In `@app/api/students/`[id]/route.ts:
- Line 8: Remove the now-unused constant ALLOWED_UPDATE_FIELDS from the file
because updates are validated by the zod schema and the whitelist is dead code
(and incorrectly lists non-existent fields like grade); locate the declaration
of ALLOWED_UPDATE_FIELDS in route.ts and delete it, then run a quick search in
the module for any remaining references (none should exist) to ensure no usages
remain and run tests/TS build to confirm no compile errors.
- Around line 29-41: StudentUpdateSchema is being created inside the request
handler causing per-request reconstruction; move the StudentUpdateSchema
definition to module scope (top of the file) so it’s built once, then use
StudentUpdateSchema.safeParse(body) in the handler and keep the existing
parsed/error handling (parsed, parsed.success, NextResponse.json) unchanged;
ensure the exported/available symbol is used by the PUT handler and update any
imports/refs if needed.
In `@app/dashboard/OverviewClient.tsx`:
- Around line 203-207: The current error thrown in the response-OK gate exposes
res.url (e.g. in the loop over studentsRes, assignmentsRes, attendanceRes,
gradesRes, announcementsRes) which can flow into setError(...) and be shown to
users; change the thrown Error to a generic, user-safe message (e.g. "Failed to
load data") and move the detailed diagnostic (res.url and res.status) into a
separate log call (console.error or processLogger.error) before throwing, so
setError gets a safe string while devs still see the URL/status for debugging.
🪄 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: aa6b3cf6-0c87-4b19-8404-86ac9dbb2e98
📒 Files selected for processing (13)
app/api/announcements/[id]/route.tsapp/api/assignments/[id]/route.tsapp/api/assignments/route.tsapp/api/attendance/route.tsapp/api/grades/[id]/route.tsapp/api/grades/route.tsapp/api/profile/route.tsapp/api/students/[id]/route.tsapp/api/students/route.tsapp/dashboard/OverviewClient.tsxapp/dashboard/announcements/AnnouncementsClient.tsxapp/dashboard/grades/GradesClient.tsxmodels/Grade.ts
|
|
||
| setStats({ | ||
| totalStudents: students.students?.length ?? 0, | ||
| totalStudents: students.total ?? students.students?.length ?? 0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the response shape of /api/assignments to see if a `total` field exists
fd -t f 'route.(ts|js)' app/api/assignments | xargs -I{} sh -c 'echo "=== {} ==="; cat {}'Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 5767
Apply the same totalAssignments fix as used for totalStudents.
The /api/assignments endpoint returns a paginated response shape { assignments, total, page, pages } (confirmed in app/api/assignments/route.ts). Using assignments.total instead of deriving totalAssignments from the (possibly truncated) assignments array will prevent under-reporting, matching the fix applied to totalStudents.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/dashboard/OverviewClient.tsx` at line 324, Update totalAssignments to use
the paginated total field rather than deriving from the possibly truncated
array: replace the current expression that uses assignments.assignments?.length
with an expression that prefers assignments.total (e.g., assignments.total ??
assignments.assignments?.length ?? 0). Modify the totalAssignments assignment in
OverviewClient (where totalStudents was fixed) so it mirrors the totalStudents
pattern and uses the paginated response's total property.
1 participant
Overview
This PR addresses multiple critical bugs identified during the BugHunt Blitz competition. The fixes focus on preventing runtime crashes, securing user data against IDOR vulnerabilities, and ensuring logic consistency across the EduDesk teacher dashboard.
Key Fixes
Runtime Stability & Error Handling
ObjectIdformat validation forstudentIdin Attendance and Grade APIs to prevent 500 errors when receiving malformed IDs.res.okchecks for all dashboard data fetches to prevent the UI from processing error responses as valid data.calcGradeandpctfunctions to preventInfinity/NaNrendering issues.awaitonGrade.findOneAndUpdateto ensure database operations complete before the response is sent.Security & Data Integrity
[id]) for Students, Assignments, Grades, and Announcements by strictly enforcingteacherIdownership filters.userId.Logic & Correctness
safeParseresults were ignored, allowing invalid student data to be persisted.totalcount instead of the length of pagination-capped arrays.$setoperator.timeAgologic to handle "Just now" cases and prevent negative time displays.Notes
Summary by CodeRabbit
Security
Bug Fixes
Improvements