Fix bug hunt issues: authorization, validation, and schema mismatches#5
Open
pranjalkuhikar wants to merge 8 commits intoJavaScript-Mastery-Pro:mainfrom
Open
Conversation
- Add teacherId filter to all PUT/DELETE operations to prevent cross-tenant data access - Fix missing await on Zod validation and findOneAndUpdate calls - Remove userId query parameter from profile API to fix IDOR vulnerability - Correct field names in ALLOWED_UPDATE_FIELDS arrays to match schemas - Set default value for maxMarks in grade schema to prevent null reference - Add comprehensive bug fix report documenting all issues
Update PUT and DELETE endpoints to include teacherId in query filters, ensuring teachers can only modify or delete their own announcements. Also remove unused 'body' and 'expiresAt' fields from ALLOWED_FIELDS and improve code formatting consistency.
Modify PUT and DELETE endpoints to ensure users can only modify or delete their own assignments by adding teacherId to query filters. This prevents unauthorized access to other teachers' assignments. Also remove 'dueDate' from allowed update fields as it's no longer used, and improve code formatting for consistency.
The POST route was missing the `await` keyword before `Grade.findOneAndUpdate`, causing a promise to be returned instead of the result. This could lead to incorrect API responses. Also, set a default value for `maxMarks` in the Zod schema to prevent potential undefined errors.
Ensure teachers can only modify or delete their own grades by adding teacherId to the query filters in PUT and DELETE handlers.
- Remove query parameter support from GET endpoint to only return current user's profile - Add more detailed validation error messages for PUT endpoint - Standardize code formatting with consistent semicolons and quotes
Previously, the API would attempt to create a student record even if the request body failed schema validation. This could lead to storing invalid or malformed data. Now, the result of `safeParse` is checked, and a 400 error with the validation details is returned if parsing fails. The validated data (`parsed.data`) is then used for creating the student record, ensuring data integrity.
Add teacherId filter to Student.findOneAndUpdate and Student.findOneAndDelete queries to ensure teachers can only modify their own students. This prevents unauthorized access and maintains data isolation between teachers. Also reorder ALLOWED_UPDATE_FIELDS for consistency and improve error response formatting.
📝 WalkthroughWalkthroughThis pull request introduces security and validation hardening across multiple API endpoints. Changes include enforcing Zod validation with explicit error returns, fixing IDOR vulnerabilities, adding multi-tenant ownership filters to restrict unauthorized data access, correcting async/await handling, and updating field allowlists. A comprehensive Bug-Fix-Report.md documents all ten discrete issues and their fixes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API Handler
participant Auth
participant Database
Client->>API Handler: PUT /api/assignments/[id]<br/>(with body)
API Handler->>Auth: Extract userId from session
Auth-->>API Handler: userId (or unauthorized)
alt Unauthorized
API Handler-->>Client: 401 Unauthorized
else Authorized
API Handler->>API Handler: Validate body with Zod
alt Validation fails
API Handler-->>Client: 400 Bad Request<br/>(with flattened errors)
else Validation succeeds
API Handler->>Database: findOneAndUpdate<br/>({ _id, teacherId: userId })
Database-->>API Handler: Updated doc (or null)
alt Document found and owned
API Handler-->>Client: 200 OK (updated resource)
else Not found or ownership mismatch
API Handler-->>Client: 404 Not Found
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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: 10
🤖 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/`[id]/route.ts:
- Around line 27-43: After parsing JSON with req.json(), validate that the
parsed body is a non-null object before using "key in body" to sanitize; if the
parsed body is null or not an object, return a 400 NextResponse.json error.
Update the logic around body, the sanitization loop that fills sanitizedBody,
and any early return paths so they check typeof body === "object" && body !==
null (or equivalent) prior to iterating ALLOWED_FIELDS.
In `@app/api/assignments/`[id]/route.ts:
- Around line 36-58: Guard against null/non-object request bodies before using
the `in` operator: validate `body` (from `req.json()`) is a plain object (e.g.,
typeof body === "object" && body !== null) and return a 400 error if not; then
build `sanitizedBody` from `ALLOWED_UPDATE_FIELDS` as before. Also enable
Mongoose validators on update by passing `runValidators: true` (and optionally
`context: "query"`) to `Assignment.findOneAndUpdate` so schema validators for
`status`, `kanbanStatus`, `deadline`, `maxMarks`, etc. are enforced.
In `@app/api/grades/`[id]/route.ts:
- Around line 71-77: The handler currently passes the DELETE id straight into
Grade.findOneAndDelete which can cause a 500 when an invalid ObjectId is
supplied; before calling connectDB() / Grade.findOneAndDelete, validate the id
from ctx.params (use mongoose.isValidObjectId or Types.ObjectId.isValid) and
return a 400/controlled client error if invalid. Update the route handler (where
ctx.params and Grade.findOneAndDelete are used) to check the id validity and
short-circuit with a client error response when it fails validation.
- Around line 35-48: The current partial-update flow builds sanitizedBody from
ALLOWED_UPDATE_FIELDS and calls Grade.findOneAndUpdate directly, which allows
bypassing model checks (e.g., marks <= maxMarks) and skips recomputing the
derived grade; instead, fetch the existing Grade by id/teacherId after
connectDB, merge the existing document with sanitizedBody to produce the full
candidate payload, call calcGrade(candidate) (move/reuse calcGrade from
app/api/grades/route.ts into a shared helper), validate that marks <= maxMarks
on the merged payload, set the computed grade field on the merged object, and
then persist the merged object via findOneAndUpdate (or replace) so the derived
grade and validations are enforced.
In `@app/api/grades/route.ts`:
- Around line 54-57: The code currently returns error.stack to clients in the
catch blocks (see the NextResponse.json call using error instanceof Error ?
error.stack) — change both catch handlers to log the full error/stack on the
server (e.g., using console.error or your logger) and return a generic JSON 500
response (e.g., { error: "Internal server error" }) via NextResponse.json; keep
references to the same error variable and NextResponse.json call sites so you
only replace the response body while preserving server-side logging of the
stack.
- Around line 79-106: Validate and authorize studentId before the
Grade.findOneAndUpdate upsert: import mongoose and Student, verify
mongoose.isValidObjectId(data.studentId) and return a 400 if invalid, then load
the Student via Student.findById(data.studentId) and ensure the student exists
and its owner/teacher (e.g., student.teacherId) matches the current userId; if
not, return 403/404. Only after these checks proceed to call
Grade.findOneAndUpdate (use the validated ObjectId when building the query) and
compute grade with calcGrade as before.
In `@app/api/profile/route.ts`:
- Around line 49-59: After parsing JSON with req.json(), guard against
non-object values (e.g., null or primitives) before destructuring into { name,
department, subjects, phone, bio, academicHistory }; if body is null or typeof
body !== 'object' return NextResponse.json({ error: "Invalid JSON in request
body" }, { status: 400 }) so the route returns a 400 validation error instead of
throwing a runtime exception during destructuring.
In `@app/api/students/`[id]/route.ts:
- Around line 34-47: The loop that builds sanitizedBody assumes body is a
non-null object, but req.json() can be null (or non-object) which makes `key in
body` throw; update the handler around the ALLOWED_UPDATE_FIELDS loop (where
body and sanitizedBody are used) to first verify body is an object (e.g., typeof
body === "object" && body !== null) and if not either return a 400 Bad Request
or treat body as an empty object, then proceed to iterate ALLOWED_UPDATE_FIELDS
and copy allowed keys into sanitizedBody; ensure all references to body in that
block are protected by this guard.
In `@Bug-Fix-Report.md`:
- Around line 28-39: The snippet validates request payload with
StudentSchema.safeParse into parsed but then persists the unvalidated body;
change the Student.create call to use the validated parsed.data instead of body
(i.e., replace usage of body in the object spread with parsed.data) so the route
persists only the validated fields while still adding teacherId; update
occurrences around StudentSchema.safeParse, parsed, and Student.create
accordingly.
- Around line 158-169: The fixed-code example's ALLOWED_FIELDS array still
contains "expiresAt", which contradicts Bug `#8` and the allowlist in the
announcements route; update the example by removing the "expiresAt" entry from
the ALLOWED_FIELDS constant so it matches the actual allowlist used in
app/api/announcements/[id]/route.ts (i.e., only include "title", "content",
"audience", "category", and "pinned").
🪄 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: 65f6dd86-5aec-47a4-9c98-3d524793bc63
📒 Files selected for processing (8)
Bug-Fix-Report.mdapp/api/announcements/[id]/route.tsapp/api/assignments/[id]/route.tsapp/api/grades/[id]/route.tsapp/api/grades/route.tsapp/api/profile/route.tsapp/api/students/[id]/route.tsapp/api/students/route.ts
Comment on lines
+27
to
43
| let body; | ||
| try { | ||
| body = await req.json() | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: 'Invalid JSON request body' }, { status: 400 }) | ||
| return NextResponse.json( | ||
| { error: "Invalid JSON request body" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Sanitize: only allow whitelisted fields | ||
| const sanitizedBody: Record<string, unknown> = {} | ||
| const sanitizedBody: Record<string, unknown> = {}; | ||
| for (const key of ALLOWED_FIELDS) { | ||
| if (key in body) { | ||
| sanitizedBody[key] = body[key] | ||
| sanitizedBody[key] = body[key]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Reject non-object JSON before sanitizing.
req.json() can parse null, and key in body will then throw, producing a 500 for an invalid request body.
🛡️ Proposed fix
try {
body = await req.json();
} catch {
return NextResponse.json(
{ error: "Invalid JSON request body" },
{ status: 400 },
);
}
+
+ if (!body || typeof body !== "object" || Array.isArray(body)) {
+ return NextResponse.json(
+ { error: "Request body must be an object" },
+ { status: 400 },
+ );
+ }
// Sanitize: only allow whitelisted fields📝 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.
Suggested change
| let body; | |
| try { | |
| body = await req.json() | |
| body = await req.json(); | |
| } catch { | |
| return NextResponse.json({ error: 'Invalid JSON request body' }, { status: 400 }) | |
| return NextResponse.json( | |
| { error: "Invalid JSON request body" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // Sanitize: only allow whitelisted fields | |
| const sanitizedBody: Record<string, unknown> = {} | |
| const sanitizedBody: Record<string, unknown> = {}; | |
| for (const key of ALLOWED_FIELDS) { | |
| if (key in body) { | |
| sanitizedBody[key] = body[key] | |
| sanitizedBody[key] = body[key]; | |
| } | |
| } | |
| let body; | |
| try { | |
| body = await req.json(); | |
| } catch { | |
| return NextResponse.json( | |
| { error: "Invalid JSON request body" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if (!body || typeof body !== "object" || Array.isArray(body)) { | |
| return NextResponse.json( | |
| { error: "Request body must be an object" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| // Sanitize: only allow whitelisted fields | |
| const sanitizedBody: Record<string, unknown> = {}; | |
| for (const key of ALLOWED_FIELDS) { | |
| if (key in body) { | |
| sanitizedBody[key] = body[key]; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/announcements/`[id]/route.ts around lines 27 - 43, After parsing JSON
with req.json(), validate that the parsed body is a non-null object before using
"key in body" to sanitize; if the parsed body is null or not an object, return a
400 NextResponse.json error. Update the logic around body, the sanitization loop
that fills sanitizedBody, and any early return paths so they check typeof body
=== "object" && body !== null (or equivalent) prior to iterating ALLOWED_FIELDS.
Comment on lines
+36
to
+58
| let body; | ||
| try { | ||
| body = await req.json() | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 }) | ||
| return NextResponse.json( | ||
| { error: "Invalid JSON in request body" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
|
|
||
| // Sanitize: only allow whitelisted fields | ||
| const sanitizedBody: Record<string, unknown> = {} | ||
| const sanitizedBody: Record<string, unknown> = {}; | ||
| for (const key of ALLOWED_UPDATE_FIELDS) { | ||
| if (key in body) { | ||
| sanitizedBody[key] = body[key] | ||
| sanitizedBody[key] = body[key]; | ||
| } | ||
| } | ||
|
|
||
| const assignment = await Assignment.findOneAndUpdate( | ||
| { _id: id }, | ||
| { _id: id, teacherId: userId }, | ||
| sanitizedBody, | ||
| { new: true } | ||
| ) | ||
| if (!assignment) return NextResponse.json({ error: 'Not found' }, { status: 404 }) | ||
| return NextResponse.json(assignment) | ||
| { new: true }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Mongoose findOneAndUpdate run schema validators by default, or must runValidators: true be passed?
💡 Result:
Mongoose findOneAndUpdate does not run schema validators by default. You must pass { runValidators: true } in the options to enable update validators.
Citations:
- 1: https://mongoosejs.com/docs/validation.html
- 2: https://mongoosejs.com/docs/8.x/docs/validation.html
🏁 Script executed:
#!/bin/bash
node <<'EOF'
// Test 1: Does 'in' operator throw with null?
console.log("Test 1: 'in' operator with null");
try {
const body = null;
const key = "test";
const result = key in body;
console.log(" Result:", result);
} catch (e) {
console.log(" ✓ THROWS:", e.message);
}
// Test 2: Does 'in' operator work with object?
console.log("\nTest 2: 'in' operator with object");
try {
const body = { test: 1 };
const key = "test";
const result = key in body;
console.log(" Result:", result);
} catch (e) {
console.log(" Error:", e.message);
}
// Test 3: Does 'in' operator throw with array?
console.log("\nTest 3: 'in' operator with array");
try {
const body = [1, 2, 3];
const key = "0";
const result = key in body;
console.log(" Result:", result);
} catch (e) {
console.log(" Error:", e.message);
}
EOFRepository: JavaScript-Mastery-Pro/discord-challenge
Length of output: 280
🌐 Web query:
Mongoose runValidators context: "query" update validation option explained
💡 Result:
In Mongoose, when using update validators with the runValidators: true option in operations like updateOne, findOneAndUpdate, etc., the context: "query" option controls the value of this within custom validator functions. By default, during update validation (without context: "query"), this in validators refers to an undefined or null value (or the global object in non-strict mode), because the full document may not be loaded in memory. Setting context: "query" changes this to the Query object. This allows validators to access other parts of the update using query methods like this.get('name') or this.getUpdate.$set.name, enabling cross-field validation based on the update data. Example from older docs (v5.x) and community usage: toySchema.path('color').validate(function(value) { if (this.getUpdate.$set.name.toLowerCase.indexOf('red') !== -1) { return value === 'red'; } return true; }, 'Must be red if name contains "red"'); Toy.updateOne({}, { color: 'blue', name: 'Red Power Ranger' }, { runValidators: true, context: 'query' }); This fails validation because name contains 'red' but color is not 'red'. In recent Mongoose versions (v6+ to v9+ as of 2026), the official docs state that this refers to the Query by default during update validation: "When running with update validators, this is the Query". The explicit context: "query" option appears deprecated or unnecessary, as the behavior is now default. However, Stack Overflow answers and some plugins still recommend it for compatibility. Usage example: Model.findOneAndUpdate( { _id: id }, { $set: { field: value } }, { runValidators: true, context: 'query' } // For cross-field validation access ); Note caveats: Update validators only run on specific operators ($set, $unset, etc.) and only on updated paths. Sources confirm this behavior across versions 5-9.
Citations:
- 1: https://stackoverflow.com/questions/38117150/what-does-the-contextquery-option-do-when-using-mongoose
- 2: https://mongoosejs.com/docs/validation.html
- 3: https://mongoosejs.com/docs/5.x/docs/validation.html
- 4: https://stackoverflow.com/questions/31101984/mongoose-findoneandupdate-and-runvalidators-not-working
Add request body validation and enable Mongoose update validators.
Sending JSON null as the request body will cause a TypeError when the code executes key in body, since the in operator cannot be used on null. Additionally, Mongoose's findOneAndUpdate does not run schema validators by default—you must explicitly pass runValidators: true to validate fields like status, kanbanStatus, deadline, or maxMarks.
🛡️ Proposed fix
try {
body = await req.json();
} catch {
return NextResponse.json(
{ error: "Invalid JSON in request body" },
{ status: 400 },
);
}
+
+ if (!body || typeof body !== "object" || Array.isArray(body)) {
+ return NextResponse.json(
+ { error: "Request body must be an object" },
+ { 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];
}
}
const assignment = await Assignment.findOneAndUpdate(
{ _id: id, teacherId: userId },
sanitizedBody,
- { new: true },
+ { new: true, runValidators: true },
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/assignments/`[id]/route.ts around lines 36 - 58, Guard against
null/non-object request bodies before using the `in` operator: validate `body`
(from `req.json()`) is a plain object (e.g., typeof body === "object" && body
!== null) and return a 400 error if not; then build `sanitizedBody` from
`ALLOWED_UPDATE_FIELDS` as before. Also enable Mongoose validators on update by
passing `runValidators: true` (and optionally `context: "query"`) to
`Assignment.findOneAndUpdate` so schema validators for `status`, `kanbanStatus`,
`deadline`, `maxMarks`, etc. are enforced.
Comment on lines
35
to
+48
| // Sanitize: only allow whitelisted fields | ||
| const sanitizedBody: Record<string, unknown> = {} | ||
| const sanitizedBody: Record<string, unknown> = {}; | ||
| for (const key of ALLOWED_UPDATE_FIELDS) { | ||
| if (key in body) { | ||
| sanitizedBody[key] = body[key] | ||
| sanitizedBody[key] = body[key]; | ||
| } | ||
| } | ||
|
|
||
| await connectDB() | ||
| await connectDB(); | ||
| const grade = await Grade.findOneAndUpdate( | ||
| { _id: id }, | ||
| { _id: id, teacherId: userId }, | ||
| sanitizedBody, | ||
| { new: true } | ||
| ) | ||
| if (!grade) return NextResponse.json({ error: 'Not found' }, { status: 404 }) | ||
| return NextResponse.json(grade) | ||
| { new: true }, | ||
| ); |
There was a problem hiding this comment.
Revalidate partial grade updates before persisting.
A PUT with only marks can bypass the model hook’s marks <= maxMarks check because maxMarks is absent from the update object, and changing marks/maxMarks does not recompute the stored grade.
🛡️ Suggested direction
// 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];
}
}
await connectDB();
+ const existing = await Grade.findOne({ _id: id, teacherId: userId });
+ if (!existing)
+ return NextResponse.json({ error: "Not found" }, { status: 404 });
+
+ const nextMarks =
+ typeof sanitizedBody.marks === "number" ? sanitizedBody.marks : existing.marks;
+ const nextMaxMarks =
+ typeof sanitizedBody.maxMarks === "number" ? sanitizedBody.maxMarks : existing.maxMarks;
+
+ if (nextMarks > nextMaxMarks) {
+ return NextResponse.json(
+ { error: "marks must be less than or equal to maxMarks" },
+ { status: 400 },
+ );
+ }
+
+ sanitizedBody.grade = calcGrade(nextMarks, nextMaxMarks);
+
const grade = await Grade.findOneAndUpdate(This also requires moving/reusing calcGrade from app/api/grades/route.ts.
🤖 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 35 - 48, The current
partial-update flow builds sanitizedBody from ALLOWED_UPDATE_FIELDS and calls
Grade.findOneAndUpdate directly, which allows bypassing model checks (e.g.,
marks <= maxMarks) and skips recomputing the derived grade; instead, fetch the
existing Grade by id/teacherId after connectDB, merge the existing document with
sanitizedBody to produce the full candidate payload, call calcGrade(candidate)
(move/reuse calcGrade from app/api/grades/route.ts into a shared helper),
validate that marks <= maxMarks on the merged payload, set the computed grade
field on the merged object, and then persist the merged object via
findOneAndUpdate (or replace) so the derived grade and validations are enforced.
Comment on lines
71
to
+77
| try { | ||
| const { id } = await ctx.params | ||
| await connectDB() | ||
| const deleted = await Grade.findOneAndDelete({ _id: id }) | ||
|
|
||
| const { id } = await ctx.params; | ||
| await connectDB(); | ||
| const deleted = await Grade.findOneAndDelete({ | ||
| _id: id, | ||
| teacherId: userId, | ||
| }); |
There was a problem hiding this comment.
Validate the DELETE id before querying.
Unlike PUT, DELETE sends invalid ObjectIds to Mongoose, which can surface as a 500 instead of a controlled client error.
🛡️ Proposed fix
try {
const { id } = await ctx.params;
+ if (!mongoose.Types.ObjectId.isValid(id)) {
+ return NextResponse.json({ error: "Not found" }, { status: 404 });
+ }
+
await connectDB();
const deleted = await Grade.findOneAndDelete({📝 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.
Suggested change
| try { | |
| const { id } = await ctx.params | |
| await connectDB() | |
| const deleted = await Grade.findOneAndDelete({ _id: id }) | |
| const { id } = await ctx.params; | |
| await connectDB(); | |
| const deleted = await Grade.findOneAndDelete({ | |
| _id: id, | |
| teacherId: userId, | |
| }); | |
| try { | |
| const { id } = await ctx.params; | |
| if (!mongoose.Types.ObjectId.isValid(id)) { | |
| return NextResponse.json({ error: "Not found" }, { status: 404 }); | |
| } | |
| await connectDB(); | |
| const deleted = await Grade.findOneAndDelete({ | |
| _id: id, | |
| teacherId: userId, | |
| }); |
🤖 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 71 - 77, The handler currently
passes the DELETE id straight into Grade.findOneAndDelete which can cause a 500
when an invalid ObjectId is supplied; before calling connectDB() /
Grade.findOneAndDelete, validate the id from ctx.params (use
mongoose.isValidObjectId or Types.ObjectId.isValid) and return a 400/controlled
client error if invalid. Update the route handler (where ctx.params and
Grade.findOneAndDelete are used) to check the id validity and short-circuit with
a client error response when it fails validation.
Comment on lines
+54
to
+57
| return NextResponse.json( | ||
| { error: error instanceof Error ? error.stack : "Internal server error" }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
Do not return stack traces in API responses.
Both catch blocks expose error.stack to clients. Keep stack traces in logs only and return a generic 500 payload.
🔒 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 },
+ );- 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: 112-115
🤖 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 54 - 57, The code currently returns
error.stack to clients in the catch blocks (see the NextResponse.json call using
error instanceof Error ? error.stack) — change both catch handlers to log the
full error/stack on the server (e.g., using console.error or your logger) and
return a generic JSON 500 response (e.g., { error: "Internal server error" })
via NextResponse.json; keep references to the same error variable and
NextResponse.json call sites so you only replace the response body while
preserving server-side logging of the stack.
Comment on lines
+79
to
+106
| const parsed = GradeSchema.safeParse(body); | ||
| if (!parsed.success) | ||
| return NextResponse.json( | ||
| { error: parsed.error.flatten() }, | ||
| { status: 400 }, | ||
| ); | ||
|
|
||
| const data = parsed.data; | ||
| const max = data.maxMarks; | ||
| 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), | ||
| }, | ||
| }, | ||
| { upsert: true, new: true }, | ||
| ); |
There was a problem hiding this comment.
Validate and authorize studentId before upserting grades.
studentId is only checked as a non-empty string, then used directly in the upsert. A malformed ID can fall through to a 500, and a known student ID from another teacher can create a cross-tenant grade reference under the current teacher.
🛡️ Proposed fix
+ if (!mongoose.Types.ObjectId.isValid(data.studentId)) {
+ return NextResponse.json({ error: "Invalid studentId" }, { status: 400 });
+ }
+
+ const studentExists = await Student.exists({
+ _id: data.studentId,
+ teacherId: userId,
+ });
+ if (!studentExists) {
+ return NextResponse.json({ error: "Student not found" }, { status: 404 });
+ }
+
const grade = await Grade.findOneAndUpdate(Also add the required imports:
import mongoose from "mongoose";
import { Student } from "@/models/Student";🤖 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 79 - 106, Validate and authorize
studentId before the Grade.findOneAndUpdate upsert: import mongoose and Student,
verify mongoose.isValidObjectId(data.studentId) and return a 400 if invalid,
then load the Student via Student.findById(data.studentId) and ensure the
student exists and its owner/teacher (e.g., student.teacherId) matches the
current userId; if not, return 403/404. Only after these checks proceed to call
Grade.findOneAndUpdate (use the validated ObjectId when building the query) and
compute grade with calcGrade as before.
Comment on lines
+49
to
+59
| let body; | ||
| try { | ||
| body = await req.json() | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 }) | ||
| return NextResponse.json( | ||
| { error: "Invalid JSON in request body" }, | ||
| { status: 400 }, | ||
| ); | ||
| } | ||
| const { name, department, subjects, phone, bio, academicHistory } = body | ||
|
|
||
| const { name, department, subjects, phone, bio, academicHistory } = body; |
There was a problem hiding this comment.
Reject non-object JSON before destructuring.
null is valid JSON, but const { ... } = body throws for it and returns a 500 instead of a validation 400.
🛡️ Proposed fix
try {
body = await req.json();
} catch {
return NextResponse.json(
{ error: "Invalid JSON in request body" },
{ status: 400 },
);
}
+
+ if (!body || typeof body !== "object" || Array.isArray(body)) {
+ return NextResponse.json(
+ { error: "Request body must be an object" },
+ { status: 400 },
+ );
+ }
const { name, department, subjects, phone, bio, academicHistory } = body;📝 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.
Suggested change
| let body; | |
| try { | |
| body = await req.json() | |
| body = await req.json(); | |
| } catch { | |
| return NextResponse.json({ error: 'Invalid JSON in request body' }, { status: 400 }) | |
| return NextResponse.json( | |
| { error: "Invalid JSON in request body" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| const { name, department, subjects, phone, bio, academicHistory } = body | |
| const { name, department, subjects, phone, bio, academicHistory } = body; | |
| let body; | |
| try { | |
| body = await req.json(); | |
| } catch { | |
| return NextResponse.json( | |
| { error: "Invalid JSON in request body" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| if (!body || typeof body !== "object" || Array.isArray(body)) { | |
| return NextResponse.json( | |
| { error: "Request body must be an object" }, | |
| { status: 400 }, | |
| ); | |
| } | |
| const { name, department, subjects, phone, bio, academicHistory } = body; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/profile/route.ts` around lines 49 - 59, After parsing JSON with
req.json(), guard against non-object values (e.g., null or primitives) before
destructuring into { name, department, subjects, phone, bio, academicHistory };
if body is null or typeof body !== 'object' return NextResponse.json({ error:
"Invalid JSON in request body" }, { status: 400 }) so the route returns a 400
validation error instead of throwing a runtime exception during destructuring.
Comment on lines
+34
to
47
| let body; | ||
| try { | ||
| body = await req.json() | ||
| body = await req.json(); | ||
| } catch { | ||
| return NextResponse.json({ error: 'Bad Request' }, { status: 400 }) | ||
| return NextResponse.json({ error: "Bad Request" }, { status: 400 }); | ||
| } | ||
|
|
||
| // Sanitize: only allow whitelisted fields | ||
| const sanitizedBody: Record<string, unknown> = {} | ||
| const sanitizedBody: Record<string, unknown> = {}; | ||
| for (const key of ALLOWED_UPDATE_FIELDS) { | ||
| if (key in body) { | ||
| sanitizedBody[key] = body[key] | ||
| sanitizedBody[key] = body[key]; | ||
| } | ||
| } |
There was a problem hiding this comment.
Guard the allowlist loop against non-object bodies.
For JSON like null, key in body throws and turns a bad request into a 500.
🛡️ Proposed fix
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> = {};📝 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.
Suggested change
| let body; | |
| try { | |
| body = await req.json() | |
| body = await req.json(); | |
| } catch { | |
| return NextResponse.json({ error: 'Bad Request' }, { status: 400 }) | |
| return NextResponse.json({ error: "Bad Request" }, { status: 400 }); | |
| } | |
| // Sanitize: only allow whitelisted fields | |
| const sanitizedBody: Record<string, unknown> = {} | |
| const sanitizedBody: Record<string, unknown> = {}; | |
| for (const key of ALLOWED_UPDATE_FIELDS) { | |
| if (key in body) { | |
| sanitizedBody[key] = body[key] | |
| sanitizedBody[key] = body[key]; | |
| } | |
| } | |
| 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]; | |
| } | |
| } |
🤖 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 34 - 47, The loop that builds
sanitizedBody assumes body is a non-null object, but req.json() can be null (or
non-object) which makes `key in body` throw; update the handler around the
ALLOWED_UPDATE_FIELDS loop (where body and sanitizedBody are used) to first
verify body is an object (e.g., typeof body === "object" && body !== null) and
if not either return a 400 Bad Request or treat body as an empty object, then
proceed to iterate ALLOWED_UPDATE_FIELDS and copy allowed keys into
sanitizedBody; ensure all references to body in that block are protected by this
guard.
Comment on lines
+28
to
+39
| **Fixed Code:** | ||
|
|
||
| ```typescript | ||
| const parsed = StudentSchema.safeParse(body); | ||
| if (!parsed.success) | ||
| return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }); | ||
|
|
||
| const student = await Student.create({ | ||
| ...(body as Record<string, unknown>), | ||
| teacherId: userId, | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
Update the fixed snippet to use parsed.data.
The actual route correctly creates the student from validated output, but this report still shows raw body being persisted.
📝 Proposed fix
const student = await Student.create({
- ...(body as Record<string, unknown>),
+ ...parsed.data,
teacherId: userId,
});📝 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.
Suggested change
| **Fixed Code:** | |
| ```typescript | |
| const parsed = StudentSchema.safeParse(body); | |
| if (!parsed.success) | |
| return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }); | |
| const student = await Student.create({ | |
| ...(body as Record<string, unknown>), | |
| teacherId: userId, | |
| }); | |
| ``` | |
| const parsed = StudentSchema.safeParse(body); | |
| if (!parsed.success) | |
| return NextResponse.json({ error: parsed.error.flatten() }, { status: 400 }); | |
| const student = await Student.create({ | |
| ...parsed.data, | |
| teacherId: userId, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Bug-Fix-Report.md` around lines 28 - 39, The snippet validates request
payload with StudentSchema.safeParse into parsed but then persists the
unvalidated body; change the Student.create call to use the validated
parsed.data instead of body (i.e., replace usage of body in the object spread
with parsed.data) so the route persists only the validated fields while still
adding teacherId; update occurrences around StudentSchema.safeParse, parsed, and
Student.create accordingly.
Comment on lines
+158
to
+169
| **Fixed Code:** | ||
|
|
||
| ```typescript | ||
| const ALLOWED_FIELDS = [ | ||
| "title", | ||
| "content", | ||
| "audience", | ||
| "category", | ||
| "pinned", | ||
| "expiresAt", | ||
| ]; | ||
| ``` |
There was a problem hiding this comment.
Remove expiresAt from this fixed-code example.
This snippet contradicts Bug #8 and the actual app/api/announcements/[id]/route.ts allowlist, which no longer includes expiresAt.
📝 Proposed fix
const ALLOWED_FIELDS = [
"title",
"content",
"audience",
"category",
"pinned",
- "expiresAt",
];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Bug-Fix-Report.md` around lines 158 - 169, The fixed-code example's
ALLOWED_FIELDS array still contains "expiresAt", which contradicts Bug `#8` and
the allowlist in the announcements route; update the example by removing the
"expiresAt" entry from the ALLOWED_FIELDS constant so it matches the actual
allowlist used in app/api/announcements/[id]/route.ts (i.e., only include
"title", "content", "audience", "category", and "pinned").
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
1 participant
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
Fixed multiple bugs in the project including security, validation, and schema mismatch issues.
Changes Made
teacherIdfiltering to PUT and DELETE routes to enforce proper authorizationuserIdquery parameter support fromGET /api/profilePOST /api/studentsawaitinPOST /api/gradesmaxMarksin grades validationBug-Fix-Report.mdfile documenting the identified issues and fixesBugs Fixed
maxMarksdueDate,grade,body, andexpiresAtTesting
{ _id: id, teacherId: userId }400on invalid payloadmaxMarkscorrectlySummary by CodeRabbit
Release Notes
Bug Fixes
Documentation