Fix: improve backend validation, security and data integrity across APIs#18
Fix: improve backend validation, security and data integrity across APIs#18BhathiyaVicum wants to merge 11 commits intoJavaScript-Mastery-Pro:mainfrom
Conversation
📝 WalkthroughWalkthroughThis pull request adds authorization scoping to resource mutations (grades, students), introduces request validation with proper error responses, improves field defaults and fallback logic, and strengthens data validation in models. Eight files across API routes and data models are updated with small, focused changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
models/Grade.ts (1)
41-54:⚠️ Potential issue | 🟡 MinorValidation gap: partial updates that change only
marksare not checked against storedmaxMarks.The hook only throws when both
marksandmaxMarksare present in the update payload. If a caller updates onlymarks(as PUT/api/grades/[id]can via the whitelist), the new value is not compared against the existingmaxMarksin the document, somarks > maxMarkscan be persisted silently.Consider fetching the current document in the hook (
await this.model.findOne(this.getQuery())) when only one of the two fields is being updated, or enforcing both fields together at the API/Zod layer forPUT.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/Grade.ts` around lines 41 - 54, The pre('findOneAndUpdate') hook on GradeSchema currently only validates when both marks and maxMarks are provided; modify it to handle partial updates by, when either marks or maxMarks is missing from this.getUpdate(), fetching the existing document via await this.model.findOne(this.getQuery()) and using its stored maxMarks/marks to perform the marks <= maxMarks check before allowing the update (keep the existing logic that checks update.$set and update fields and still throw Error("marks must be less than or equal to maxMarks") when the combined values violate the constraint). Ensure you use GradeSchema.pre("findOneAndUpdate"), this.getUpdate(), this.getQuery(), and this.model.findOne() to locate and compare stored values.app/api/grades/[id]/route.ts (1)
55-67:⚠️ Potential issue | 🟡 MinorDELETE handler is missing ObjectId validation.
Unlike
PUT(lines 17-19), theDELETEpath passesiddirectly tofindOneAndDeletewithoutmongoose.Types.ObjectId.isValid(id). An invalid id will trigger a MongooseCastErrorthat falls through to the 500 handler instead of returning a clean 400/404.🔧 Proposed fix
try { const { id } = await ctx.params + if (!mongoose.Types.ObjectId.isValid(id)) { + return NextResponse.json({ error: 'Not found' }, { status: 404 }) + } await connectDB()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/grades/`[id]/route.ts around lines 55 - 67, The DELETE handler currently passes id from ctx.params directly into Grade.findOneAndDelete which can throw a Mongoose CastError for malformed ids; add the same ObjectId validation used in the PUT path by checking mongoose.Types.ObjectId.isValid(id) (or equivalent) after extracting const { id } = await ctx.params and before calling connectDB/Grade.findOneAndDelete, and if invalid return a NextResponse.json error with a 400 status (or 404 per project convention) so malformed ids don't bubble as 500 errors.app/api/grades/route.ts (1)
55-56:⚠️ Potential issue | 🟠 MajorDon't leak stack traces in error responses.
Both catch blocks return
error.stackin the JSON response body on 500s. Stack traces expose internal paths, dependency versions, and can aid attackers in fingerprinting the runtime. Log the stack server-side but return a generic message to clients.🔧 Proposed fix
- return NextResponse.json({ error: error instanceof Error ? error.stack : 'Internal server error' }, { status: 500 }) + return NextResponse.json({ error: 'Internal server error' }, { status: 500 })Also applies to: 90-91
🤖 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 55 - 56, The catch blocks that call console.error('GET /api/grades error:', ...) and return NextResponse.json({ error: error instanceof Error ? error.stack : 'Internal server error' }, { status: 500 }) leak stack traces to clients; change them to log the full error/stack server-side (keep console.error with error and error.stack) but return a generic message to the client (e.g. { error: 'Internal server error' }) via NextResponse.json and remove any use of error.stack in the response; update all similar catch sites (both occurrences around the GET handler and lines ~90-91) so only server logs include stack details while client responses are generic.app/api/students/[id]/route.ts (1)
21-34:⚠️ Potential issue | 🟡 MinorGuard against non-object JSON bodies before
incheck.
await req.json()may resolve tonull, a primitive, or an array.key in bodywill throw aTypeErrorifbodyisnull/primitive, bypassing the clean 400 response and surfacing as a 500.🔧 Proposed fix
let body try { body = await req.json() } catch { return NextResponse.json({ error: 'Bad Request' }, { status: 400 }) } + if (!body || typeof body !== 'object' || Array.isArray(body)) { + return NextResponse.json({ error: 'Bad Request' }, { status: 400 }) + } // Sanitize: only allow whitelisted fields const sanitizedBody: Record<string, unknown> = {} for (const key of ALLOWED_UPDATE_FIELDS) { if (key in body) { sanitizedBody[key] = body[key] } }Also consider rejecting empty
sanitizedBodywith a 400 to avoid no-op updates.🤖 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 21 - 34, Check that the parsed payload from await req.json() is a plain non-null object before using the `in` operator: after parsing `body` from `req.json()` (in this handler), validate `typeof body === 'object' && body !== null && !Array.isArray(body)` and return NextResponse.json({ error: 'Bad Request' }, { status: 400 }) if that fails; then build `sanitizedBody` by iterating over ALLOWED_UPDATE_FIELDS as before, and if `sanitizedBody` is empty reject with a 400 to avoid no-op updates.
🧹 Nitpick comments (2)
app/api/profile/route.ts (1)
25-25: Fallback chain looks good; consider normalizing at the schema layer too.The trimmed
fullName→ trimmedfirstName→"Unknown User"fallback correctly prevents the Teacher validation error for empty/whitespace names on this code path. However, permodels/Teacher.ts(lines 23–44), thenamefield isrequired: truewithout any trim/min-length validation or default, so other creators of Teacher documents can still bypass this normalization and write empty or whitespace-only names. Consider adding a schema-level safeguard (e.g.,trim: trueplus aminlength/validator, or a pre-save hook) so the invariant is enforced centrally rather than only at this call site.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/api/profile/route.ts` at line 25, The route-level fallback trims clerkUser.fullName/firstName which prevents empty names here but the invariant must be enforced in the Teacher model; update the Teacher Mongoose schema (models/Teacher.ts) for the name field to include trim: true and a minlength or custom validator that rejects empty/whitespace-only strings (or add a pre('save') hook on the Teacher schema that normalizes and validates doc.name), so all writes (create/update) cannot persist blank names regardless of call site.models/Grade.ts (1)
41-54: Good fix, butupdateOnehook now inconsistent.The
findOneAndUpdatehook correctly handles both$set-wrapped and root-level shapes. However, theupdateOnehook just below (lines 56-69) still only readsupdate.marks/update.maxMarksat the root, so$set-shapedupdateOnecalls will bypass validation. Recommend applying the same normalization there for symmetry.🔧 Proposed fix
GradeSchema.pre("updateOne", function () { - const update = this.getUpdate() as Record<string, unknown>; + const update = this.getUpdate() as any; if (update && typeof update === "object") { - const marks = update.marks; - const maxMarks = update.maxMarks; + const marks = update?.$set?.marks ?? update?.marks; + const maxMarks = update?.$set?.maxMarks ?? update?.maxMarks;Also, prefer a narrower type over
anyforgetUpdate()(e.g.,UpdateQuery<IGrade>from mongoose) to retain type safety.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/Grade.ts` around lines 41 - 54, The updateOne pre-hook currently only checks root-level update.marks/maxMarks and misses $set-wrapped updates; modify the GradeSchema.pre("updateOne", ...) handler to normalize the update the same way as the findOneAndUpdate hook (read marks via update?.$set?.marks ?? update?.marks and maxMarks via update?.$set?.maxMarks ?? update?.maxMarks), validate that both are numbers and that marks <= maxMarks, and throw the same Error("marks must be less than or equal to maxMarks") on violation; also replace the getUpdate() any type with a narrower mongoose type (e.g., UpdateQuery<IGrade>) for type safety.
🤖 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/api/announcements/route.ts`:
- Line 10: The API route's zod schema sets audience:
z.string().optional().default('all') which conflicts with the Mongoose model
(models/Announcement.ts) and client (AnnouncementsClient.tsx) that expect "All"
(capital A); update the default in the API validation in route.ts from 'all' to
'All' (and optionally ensure any created/normalized value in the handler uses
the audience value from the parsed input so it persists "All") so stored records
match the model and the client's a.audience !== "All" checks.
In `@app/api/grades/route.ts`:
- Around line 46-48: The current logic silently ignores an invalid studentId
(the mongoose.Types.ObjectId.isValid check) and thus returns all grades;
instead, in the handler where query and studentId are processed (references:
studentId, mongoose.Types.ObjectId.isValid, and query.studentId = new
mongoose.Types.ObjectId(studentId)), validate studentId and if it is provided
but invalid respond with a 400 Bad Request (or return an empty result) with a
clear error message; do this before mutating query so invalid input does not
fall through to returning all teacher grades.
In `@models/Attendance.ts`:
- Around line 20-28: The Mongoose 'class' field validator is redundant because
trim: true runs first and the validator uses v.trim(), and the upstream Zod
schema (the z.string().min(1) used for the attendance input) currently allows
whitespace-only strings which will be trimmed to empty and then fail at the
model layer; update the Zod validation to z.string().trim().min(1) so
whitespace-only input is rejected at the API layer, and simplify the Attendance
model 'class' field by removing the redundant validator (or change its check to
v.length > 0 if you prefer an explicit guard) so both layers are consistent.
---
Outside diff comments:
In `@app/api/grades/`[id]/route.ts:
- Around line 55-67: The DELETE handler currently passes id from ctx.params
directly into Grade.findOneAndDelete which can throw a Mongoose CastError for
malformed ids; add the same ObjectId validation used in the PUT path by checking
mongoose.Types.ObjectId.isValid(id) (or equivalent) after extracting const { id
} = await ctx.params and before calling connectDB/Grade.findOneAndDelete, and if
invalid return a NextResponse.json error with a 400 status (or 404 per project
convention) so malformed ids don't bubble as 500 errors.
In `@app/api/grades/route.ts`:
- Around line 55-56: The catch blocks that call console.error('GET /api/grades
error:', ...) and return NextResponse.json({ error: error instanceof Error ?
error.stack : 'Internal server error' }, { status: 500 }) leak stack traces to
clients; change them to log the full error/stack server-side (keep console.error
with error and error.stack) but return a generic message to the client (e.g. {
error: 'Internal server error' }) via NextResponse.json and remove any use of
error.stack in the response; update all similar catch sites (both occurrences
around the GET handler and lines ~90-91) so only server logs include stack
details while client responses are generic.
In `@app/api/students/`[id]/route.ts:
- Around line 21-34: Check that the parsed payload from await req.json() is a
plain non-null object before using the `in` operator: after parsing `body` from
`req.json()` (in this handler), validate `typeof body === 'object' && body !==
null && !Array.isArray(body)` and return NextResponse.json({ error: 'Bad
Request' }, { status: 400 }) if that fails; then build `sanitizedBody` by
iterating over ALLOWED_UPDATE_FIELDS as before, and if `sanitizedBody` is empty
reject with a 400 to avoid no-op updates.
In `@models/Grade.ts`:
- Around line 41-54: The pre('findOneAndUpdate') hook on GradeSchema currently
only validates when both marks and maxMarks are provided; modify it to handle
partial updates by, when either marks or maxMarks is missing from
this.getUpdate(), fetching the existing document via await
this.model.findOne(this.getQuery()) and using its stored maxMarks/marks to
perform the marks <= maxMarks check before allowing the update (keep the
existing logic that checks update.$set and update fields and still throw
Error("marks must be less than or equal to maxMarks") when the combined values
violate the constraint). Ensure you use GradeSchema.pre("findOneAndUpdate"),
this.getUpdate(), this.getQuery(), and this.model.findOne() to locate and
compare stored values.
---
Nitpick comments:
In `@app/api/profile/route.ts`:
- Line 25: The route-level fallback trims clerkUser.fullName/firstName which
prevents empty names here but the invariant must be enforced in the Teacher
model; update the Teacher Mongoose schema (models/Teacher.ts) for the name field
to include trim: true and a minlength or custom validator that rejects
empty/whitespace-only strings (or add a pre('save') hook on the Teacher schema
that normalizes and validates doc.name), so all writes (create/update) cannot
persist blank names regardless of call site.
In `@models/Grade.ts`:
- Around line 41-54: The updateOne pre-hook currently only checks root-level
update.marks/maxMarks and misses $set-wrapped updates; modify the
GradeSchema.pre("updateOne", ...) handler to normalize the update the same way
as the findOneAndUpdate hook (read marks via update?.$set?.marks ??
update?.marks and maxMarks via update?.$set?.maxMarks ?? update?.maxMarks),
validate that both are numbers and that marks <= maxMarks, and throw the same
Error("marks must be less than or equal to maxMarks") on violation; also replace
the getUpdate() any type with a narrower mongoose type (e.g.,
UpdateQuery<IGrade>) for type safety.
🪄 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: d93a204f-a5ca-4160-b576-b6f3ba02d391
📒 Files selected for processing (8)
app/api/announcements/route.tsapp/api/grades/[id]/route.tsapp/api/grades/route.tsapp/api/profile/route.tsapp/api/students/[id]/route.tsapp/api/students/route.tsmodels/Attendance.tsmodels/Grade.ts
| title: z.string().min(1), | ||
| content: z.string().min(1), | ||
| audience: z.string().optional(), | ||
| audience: z.string().optional().default('all'), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C2 "audience" -g '!**/node_modules/**'Repository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 4396
Fix audience default casing inconsistency ('all' → 'All').
The Mongoose schema in models/Announcement.ts defaults audience to 'All' (capital A), and the client UI in AnnouncementsClient.tsx defaults to 'All' and explicitly checks a.audience !== "All" to determine whether to display an audience badge. The API route defaults to lowercase 'all', creating a casing mismatch that causes records created via this endpoint to have 'all' while the client expects 'All', breaking the badge display logic and data consistency.
Fix
- audience: z.string().optional().default('all'),
+ audience: z.string().optional().default('All'),📝 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.
| audience: z.string().optional().default('all'), | |
| audience: z.string().optional().default('All'), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/announcements/route.ts` at line 10, The API route's zod schema sets
audience: z.string().optional().default('all') which conflicts with the Mongoose
model (models/Announcement.ts) and client (AnnouncementsClient.tsx) that expect
"All" (capital A); update the default in the API validation in route.ts from
'all' to 'All' (and optionally ensure any created/normalized value in the
handler uses the audience value from the parsed input so it persists "All") so
stored records match the model and the client's a.audience !== "All" checks.
| if (studentId && mongoose.Types.ObjectId.isValid(studentId)) { | ||
| query.studentId = new mongoose.Types.ObjectId(studentId) | ||
| } |
There was a problem hiding this comment.
Invalid studentId is silently ignored — returns all teacher grades.
When studentId is provided but not a valid ObjectId, the filter is silently dropped and the response returns every grade for the teacher. That's likely unexpected for callers and can mask client bugs. Prefer returning 400 (or an empty result) on invalid input.
🔧 Proposed fix
- if (studentId && mongoose.Types.ObjectId.isValid(studentId)) {
- query.studentId = new mongoose.Types.ObjectId(studentId)
+ if (studentId) {
+ if (!mongoose.Types.ObjectId.isValid(studentId)) {
+ return NextResponse.json({ error: 'Invalid studentId' }, { status: 400 })
+ }
+ query.studentId = new mongoose.Types.ObjectId(studentId)
}📝 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 (studentId && mongoose.Types.ObjectId.isValid(studentId)) { | |
| query.studentId = new mongoose.Types.ObjectId(studentId) | |
| } | |
| if (studentId) { | |
| if (!mongoose.Types.ObjectId.isValid(studentId)) { | |
| return NextResponse.json({ error: 'Invalid studentId' }, { status: 400 }) | |
| } | |
| query.studentId = new mongoose.Types.ObjectId(studentId) | |
| } |
🤖 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 46 - 48, The current logic silently
ignores an invalid studentId (the mongoose.Types.ObjectId.isValid check) and
thus returns all grades; instead, in the handler where query and studentId are
processed (references: studentId, mongoose.Types.ObjectId.isValid, and
query.studentId = new mongoose.Types.ObjectId(studentId)), validate studentId
and if it is provided but invalid respond with a 400 Bad Request (or return an
empty result) with a clear error message; do this before mutating query so
invalid input does not fall through to returning all teacher grades.
| class: { | ||
| type: String, | ||
| required: true, | ||
| trim: true, | ||
| validate: { | ||
| validator: (v: string) => v.trim().length > 0, | ||
| message: 'Class cannot be empty', | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the Zod schema for `class` in the attendance API route still uses min(1) without trim.
rg -nP -C2 '\bclass\s*:\s*z\.string\(\)' --type=tsRepository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 1057
Validator is redundant after trim: true; also ensure upstream Zod schema matches.
Since trim: true runs before validators, the input is already trimmed by the time the validator executes, so v.trim().length > 0 is equivalent to v.length > 0. More importantly, the Zod schema in app/api/attendance/route.ts (line 10) uses z.string().min(1) for class, which accepts whitespace-only strings like " ". Those will pass API validation but be trimmed to "" by Mongoose and fail here with a validation error instead of being rejected cleanly at the API layer. Align the Zod schema (e.g., .trim().min(1)) so the API rejects whitespace-only input before it reaches the model.
Suggested fixes
class: {
type: String,
required: true,
trim: true,
validate: {
- validator: (v: string) => v.trim().length > 0,
+ validator: (v: string) => v.length > 0,
message: 'Class cannot be empty',
},
},And in app/api/attendance/route.ts:
- class: z.string().min(1),
+ class: z.string().trim().min(1),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@models/Attendance.ts` around lines 20 - 28, The Mongoose 'class' field
validator is redundant because trim: true runs first and the validator uses
v.trim(), and the upstream Zod schema (the z.string().min(1) used for the
attendance input) currently allows whitespace-only strings which will be trimmed
to empty and then fail at the model layer; update the Zod validation to
z.string().trim().min(1) so whitespace-only input is rejected at the API layer,
and simplify the Attendance model 'class' field by removing the redundant
validator (or change its check to v.length > 0 if you prefer an explicit guard)
so both layers are consistent.
1 participant
PR Details
Email - bhathiyav2004@gmail.com
No. of Issues Fixed - 10
Summary
This PR improves backend validation, security, and data integrity across multiple modules including grades, attendance, students, announcements, and profile systems.
Changes Included
Grades
Attendance
Students
Announcements
Profile
Improvements