Skip to content

Fix/bughunt anhtu081204#4

Open
leanhtucc wants to merge 11 commits intoJavaScript-Mastery-Pro:mainfrom
leanhtucc:fix/bughunt-anhtu081204
Open

Fix/bughunt anhtu081204#4
leanhtucc wants to merge 11 commits intoJavaScript-Mastery-Pro:mainfrom
leanhtucc:fix/bughunt-anhtu081204

Conversation

@leanhtucc
Copy link
Copy Markdown

@leanhtucc leanhtucc commented Apr 18, 2026

Summary by CodeRabbit

Release Notes

  • Security & Data Protection

    • Strengthened access controls: All record modifications and deletions now scoped to authenticated users' own data.
  • Bug Fixes

    • Improved request validation to catch invalid inputs earlier.
    • Standardized grade calculations for consistency throughout the application.
    • Enhanced error handling with clearer error messages.
  • New Features

    • Extended grade management with flexible field updates.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 18, 2026

📝 Walkthrough

Walkthrough

Security hardening across API endpoints by adding owner-based authorization filters (teacherId: userId), enhancing request body validation, and centralizing grade calculation logic into a shared utility module. Mongoose validators strengthened with enhanced options and error handling standardized.

Changes

Cohort / File(s) Summary
API Authorization & Input Validation
app/api/announcements/[id]/route.ts, app/api/assignments/[id]/route.ts, app/api/students/[id]/route.ts
Added owner-scoping filters (teacherId: userId) to PUT/DELETE queries. Enhanced request validation: reject non-object/array payloads, detect unknown fields outside ALLOWED_FIELDS/ALLOWED_UPDATE_FIELDS, return 400 when no whitelisted fields provided. Enabled Mongoose validation with runValidators: true and context: 'query'.
API Assignments & Grades Routes
app/api/assignments/route.ts, app/api/grades/route.ts
Simplified error responses: removed conditional error.stack/error.message inclusion, now always return fixed { error: 'Internal server error' }. Updated grades/route.ts POST to use calculateLetterGrade() helper, enforce maxMarks default (100), and properly await database calls with enhanced Mongoose options.
Grade Field Updates
app/api/grades/[id]/route.ts
Expanded updatable fields (studentId, studentName, subject, term, marks, maxMarks) with per-field type/value validation. Added strict body shape validation, unknown-field rejection, and duplicate-key error handling (409 on code === 11000). Scoped by teacherId, computes missing values from existing document, rejects marks > maxMarks, and recalculates grade via calculateLetterGrade().
Student Creation
app/api/students/route.ts
Enforced Zod validation: capture safeParse() result and return 400 with flattened errors on failure. Create record using validated parsed.data merged with teacherId, not unvalidated body.
Profile Endpoint
app/api/profile/route.ts
Removed query-parameter-based user resolution; now always derive userId from auth() result. Updated exported function signature from GET(req: NextRequest) to GET().
Centralized Grading Logic
lib/grading.ts
New module exporting GRADE_POINTS (letter-to-numeric mapping), getGradePoint(grade), calculateCgpaFromGrades(grades) (returns string with 2 decimals or null), and calculateLetterGrade(marks, max) (percentage-based thresholds).
Dashboard Components
app/dashboard/OverviewClient.tsx, app/dashboard/grades/GradesClient.tsx, app/dashboard/students/StudentsClient.tsx
Removed locally defined GRADE_POINT maps and CGPA helpers, replaced with imports from @/lib/grading. Updated stat calculations and CGPA rendering to use getGradePoint() and calculateCgpaFromGrades(). Minor className formatting adjustments (no logic changes).
Grade Model Validation
models/Grade.ts
Replaced synchronous pre("findOneAndUpdate")/pre("updateOne") hooks with async validator. Extracts marks/maxMarks from top-level or $set subfields, queries current document for missing values, applies 100 fallback for maxMarks during upsert, then validates marks ≤ maxMarks.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~55 minutes

Poem

🐰 Whiskers twitching with delight,
Authorization locks pulled tight,
Grade points gathered, validated with care,
Shared logic hops from here to there,
Security strengthened, errors laid bare!

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 4.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Fix/bughunt anhtu081204' is vague and generic, using non-descriptive terms that do not convey meaningful information about the changeset's primary purpose or scope. Replace with a clear, specific title that describes the main security/validation improvements, such as 'Add request validation and ownership checks to API endpoints' or 'Strengthen authorization and input validation across API routes'.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
app/api/grades/route.ts (2)

59-64: ⚠️ Potential issue | 🟠 Major

Validate marks against the defaulted maximum at request time.

The Zod schema validation only checks marks <= maxMarks when maxMarks is explicitly provided. When maxMarks is omitted, the validation passes regardless of marks value. The code then defaults maxMarks to 100, but by then the damage is done—a request with marks: 150 passes validation and fails later at the database layer, returning a 500 error instead of a 400. Add validation at line 64 to catch this before the database write:

Proposed fix
    const data = parsed.data
    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'
🤖 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 59 - 64, After parsing with
GradeSchema, ensure marks are validated against the defaulted max by checking
parsed.data.marks <= max (where max is set from data.maxMarks ?? 100); if marks
exceed max, return a 400 NextResponse with a clear error message instead of
proceeding to DB. Update the route handler to perform this runtime check right
after const max = data.maxMarks ?? 100 and before any database writes
(referencing GradeSchema, parsed, data, and max).

8-13: ⚠️ Potential issue | 🟠 Major

Verify student ownership and derive studentName server-side before the upsert.

studentId only validates as non-empty string without checking ObjectId format or ownership by the authenticated teacher. A teacher could create grades for students belonging to other teachers. Additionally, studentName is accepted from the client and written directly to the grade record. Add explicit ObjectId validation, confirm the student is owned by userId, and fetch the student's name from the database to prevent unauthorized access and data inconsistency.

🤖 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 8 - 13, The GradeSchema currently
accepts a free-form studentId and studentName from the client; change validation
and upsert logic so you (1) validate studentId as a proper ObjectId (e.g., use
ObjectId.isValid) instead of just non-empty string, (2) remove/trust
client-supplied studentName and instead load the Student record from the
database (using the same DB/model code used elsewhere) and derive studentName
server-side, and (3) verify the fetched Student.ownerId or teacherId equals the
authenticated userId before performing the upsert in the grades upsert handler
(the code paths referencing GradeSchema, studentId, studentName and the upsert
function in app/api/grades/route.ts); if validation or ownership check fails,
return an authorization/validation error and do not write the grade.
🧹 Nitpick comments (1)
app/api/assignments/route.ts (1)

51-54: Minor: inconsistent error logging between GET and POST.

GET now logs the raw error object, while POST at lines 83–86 still gates on instanceof Error and logs only error.message. Worth picking one convention across both handlers (and across sibling routes) for predictable log output.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/api/assignments/route.ts` around lines 51 - 54, GET and POST error
handling in route.ts is inconsistent: the GET handler logs the raw error object
via console.error('GET /api/assignments error:', error) while the POST handler
uses an instanceof check and logs only error.message; unify them by changing the
POST handler to log the full error object (e.g., console.error('POST
/api/assignments error:', error)) instead of gating on error instanceof Error
and using error.message, or alternatively change both to use a shared logger;
update the POST branch that references error instanceof Error and error.message
to log the complete error object and preserve the same NextResponse.json({
error: 'Internal server error' }, { status: 500 }) behavior.
🤖 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/grades/`[id]/route.ts:
- Around line 120-122: The DELETE handler currently passes the raw id from
ctx.params into Grade.findOneAndDelete which allows invalid ObjectId strings to
reach Mongoose and throw a cast error; before calling connectDB() /
Grade.findOneAndDelete, validate the id using
mongoose.Types.ObjectId.isValid(id) (same approach as the PUT handler) and
return a 404 for invalid ids. Update the code that reads const { id } = await
ctx.params to perform the isValid check and short-circuit with the same 404
response used by the PUT route, then only call connectDB() and
Grade.findOneAndDelete({ _id: id, teacherId: userId }) when the id is valid.
- Around line 51-56: The studentId validation currently only checks shape;
extend it to verify ownership by querying the Student model: after confirming
sanitizedBody.studentId is a valid ObjectId, load Student.findById(studentId)
and ensure its teacherId matches the authenticated teacher's id (derive that
from the request/session used in this route), returning a 403 if not owned; if
owned, set/override sanitizedBody.studentName server-side from the Student
record to prevent spoofing. Apply the same ownership+studentName derivation for
the other update path referenced (lines ~93-101) so all incoming studentId
updates are validated and studentName is authoritative.

In `@lib/grading.ts`:
- Around line 22-31: calculateLetterGrade has an inconsistent A+ threshold and
no guard for invalid divisor: change the first condition from pct > 90 to pct >=
90 so A+ starts at 90, and add an early validation in calculateLetterGrade that
checks max > 0 (and that marks/max produce a finite number) — if validation
fails, either throw a clear Error (e.g., "invalid max") or return a defined
fallback, then compute pct and apply the existing >= thresholds.

In `@models/Grade.ts`:
- Around line 41-87: The validator only inspects direct fields and $set, so
extend pickNumberFromUpdate/validateMarksMaxMarksOnUpdate to defensively detect
other atomic operators that can affect marks/maxMarks (at least $inc, $mul,
$min, $max) and compute resulting numeric values when possible: when an operator
like $inc/$mul/$min/$max is present for "marks" or "maxMarks", if the operator
value is a finite number and the current value (fetched via
this.model.findOne(this.getQuery()).select("marks maxMarks").lean()) is numeric,
apply the operator to derive nextMarks/nextMaxMarks (e.g., next = current + inc,
current * mul, min(current, val), max(current, val)); if you cannot derive a
finite result, treat the next value as unknown so the existing checks will fetch
current/defaults and still validate. Finally, register
validateMarksMaxMarksOnUpdate on GradeSchema.pre for "updateMany" and
"replaceOne" in addition to the existing "findOneAndUpdate" and "updateOne".

---

Outside diff comments:
In `@app/api/grades/route.ts`:
- Around line 59-64: After parsing with GradeSchema, ensure marks are validated
against the defaulted max by checking parsed.data.marks <= max (where max is set
from data.maxMarks ?? 100); if marks exceed max, return a 400 NextResponse with
a clear error message instead of proceeding to DB. Update the route handler to
perform this runtime check right after const max = data.maxMarks ?? 100 and
before any database writes (referencing GradeSchema, parsed, data, and max).
- Around line 8-13: The GradeSchema currently accepts a free-form studentId and
studentName from the client; change validation and upsert logic so you (1)
validate studentId as a proper ObjectId (e.g., use ObjectId.isValid) instead of
just non-empty string, (2) remove/trust client-supplied studentName and instead
load the Student record from the database (using the same DB/model code used
elsewhere) and derive studentName server-side, and (3) verify the fetched
Student.ownerId or teacherId equals the authenticated userId before performing
the upsert in the grades upsert handler (the code paths referencing GradeSchema,
studentId, studentName and the upsert function in app/api/grades/route.ts); if
validation or ownership check fails, return an authorization/validation error
and do not write the grade.

---

Nitpick comments:
In `@app/api/assignments/route.ts`:
- Around line 51-54: GET and POST error handling in route.ts is inconsistent:
the GET handler logs the raw error object via console.error('GET
/api/assignments error:', error) while the POST handler uses an instanceof check
and logs only error.message; unify them by changing the POST handler to log the
full error object (e.g., console.error('POST /api/assignments error:', error))
instead of gating on error instanceof Error and using error.message, or
alternatively change both to use a shared logger; update the POST branch that
references error instanceof Error and error.message to log the complete error
object and preserve the same NextResponse.json({ error: 'Internal server error'
}, { status: 500 }) behavior.
🪄 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: d24ead4a-2930-44f7-94ae-62e8aae8bca6

📥 Commits

Reviewing files that changed from the base of the PR and between c71cbbf and 139f0a6.

📒 Files selected for processing (13)
  • app/api/announcements/[id]/route.ts
  • app/api/assignments/[id]/route.ts
  • app/api/assignments/route.ts
  • app/api/grades/[id]/route.ts
  • app/api/grades/route.ts
  • app/api/profile/route.ts
  • app/api/students/[id]/route.ts
  • app/api/students/route.ts
  • app/dashboard/OverviewClient.tsx
  • app/dashboard/grades/GradesClient.tsx
  • app/dashboard/students/StudentsClient.tsx
  • lib/grading.ts
  • models/Grade.ts

Comment on lines +51 to +56
if ('studentId' in sanitizedBody) {
const studentId = sanitizedBody.studentId
if (typeof studentId !== 'string' || !mongoose.Types.ObjectId.isValid(studentId)) {
return NextResponse.json({ error: 'studentId must be a valid id' }, { status: 400 })
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cat -n app/api/grades/[id]/route.ts

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 6322


🏁 Script executed:

find . -type f -name "*.ts" -path "*/models/*" | head -20

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 212


🏁 Script executed:

cat -n ./models/Student.ts

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 1302


🏁 Script executed:

cat -n ./models/Grade.ts

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 3784


Verify student ownership before accepting studentId updates.

The studentId field is shape-validated (lines 51–56) but lacks ownership verification. A teacher can update any grade to reference a student they don't own, breaking data isolation. Before accepting the studentId value, query the Student model to confirm it belongs to the authenticated teacher, and derive studentName server-side to prevent spoofed associations.

Suggested direction
 import mongoose from 'mongoose'
 import { connectDB } from '@/lib/mongodb'
 import { Grade } from '@/models/Grade'
+import { Student } from '@/models/Student'
 import { calculateLetterGrade } from '@/lib/grading'
     await connectDB()
+
+    if (typeof sanitizedBody.studentId === 'string') {
+      const student = await Student.findOne({ _id: sanitizedBody.studentId, teacherId: userId }).select('name').lean()
+      if (!student) {
+        return NextResponse.json({ error: 'Student not found' }, { status: 404 })
+      }
+      sanitizedBody.studentName = student.name
+    }
 
     const existingGrade = await Grade.findOne({ _id: id, teacherId: userId })

Also applies to: 93-101

🤖 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 51 - 56, The studentId validation
currently only checks shape; extend it to verify ownership by querying the
Student model: after confirming sanitizedBody.studentId is a valid ObjectId,
load Student.findById(studentId) and ensure its teacherId matches the
authenticated teacher's id (derive that from the request/session used in this
route), returning a 403 if not owned; if owned, set/override
sanitizedBody.studentName server-side from the Student record to prevent
spoofing. Apply the same ownership+studentName derivation for the other update
path referenced (lines ~93-101) so all incoming studentId updates are validated
and studentName is authoritative.

Comment on lines 120 to +122
const { id } = await ctx.params
await connectDB()
const deleted = await Grade.findOneAndDelete({ _id: id })
const deleted = await Grade.findOneAndDelete({ _id: id, teacherId: userId })
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the DELETE route handler
cat -n app/api/grades/[id]/route.ts | head -150

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 6322


Add ObjectId validation to DELETE endpoint before querying Mongoose.

The PUT endpoint validates the id using mongoose.Types.ObjectId.isValid() at line 18 and returns 404 for invalid IDs, but the DELETE endpoint at line 122 passes the raw id directly to Mongoose. Invalid ObjectIds will trigger a cast error caught by the catch block, returning 500 instead of 404.

Proposed fix
   try {
     const { id } = await ctx.params
+    if (!mongoose.Types.ObjectId.isValid(id)) {
+      return NextResponse.json({ error: 'Grade not found' }, { status: 404 })
+    }
+
     await connectDB()
     const deleted = await Grade.findOneAndDelete({ _id: id, 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
const { id } = await ctx.params
await connectDB()
const deleted = await Grade.findOneAndDelete({ _id: id })
const deleted = await Grade.findOneAndDelete({ _id: id, teacherId: userId })
const { id } = await ctx.params
if (!mongoose.Types.ObjectId.isValid(id)) {
return NextResponse.json({ error: 'Grade 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 120 - 122, The DELETE handler
currently passes the raw id from ctx.params into Grade.findOneAndDelete which
allows invalid ObjectId strings to reach Mongoose and throw a cast error; before
calling connectDB() / Grade.findOneAndDelete, validate the id using
mongoose.Types.ObjectId.isValid(id) (same approach as the PUT handler) and
return a 404 for invalid ids. Update the code that reads const { id } = await
ctx.params to perform the isValid check and short-circuit with the same 404
response used by the PUT route, then only call connectDB() and
Grade.findOneAndDelete({ _id: id, teacherId: userId }) when the id is valid.

Comment thread lib/grading.ts
Comment on lines +22 to +31
export function calculateLetterGrade(marks: number, max: number): string {
const pct = (marks / max) * 100
if (pct > 90) return 'A+'
if (pct >= 80) return 'A'
if (pct >= 70) return 'B+'
if (pct >= 60) return 'B'
if (pct >= 50) return 'C'
if (pct >= 40) return 'D'
return 'F'
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Likely boundary bug: pct > 90 is inconsistent with the other >= thresholds.

Every other branch uses >=, but the A+ branch uses strict >. That makes the A+ threshold functionally 91, not 90 — a student scoring exactly 90% receives an A instead of A+, while a student scoring exactly 80% still receives an A. If this asymmetry is intentional, a short comment would help; otherwise it should be aligned.

🔧 Proposed fix
 export function calculateLetterGrade(marks: number, max: number): string {
   const pct = (marks / max) * 100
-  if (pct > 90) return 'A+'
+  if (pct >= 90) return 'A+'
   if (pct >= 80) return 'A'
   if (pct >= 70) return 'B+'
   if (pct >= 60) return 'B'
   if (pct >= 50) return 'C'
   if (pct >= 40) return 'D'
   return 'F'
 }

Also consider guarding against max <= 0 (division produces NaN/Infinity → silently returns 'F'), since this helper is pure and can be called with arbitrary inputs outside the Mongoose schema's min: 1 constraint.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/grading.ts` around lines 22 - 31, calculateLetterGrade has an
inconsistent A+ threshold and no guard for invalid divisor: change the first
condition from pct > 90 to pct >= 90 so A+ starts at 90, and add an early
validation in calculateLetterGrade that checks max > 0 (and that marks/max
produce a finite number) — if validation fails, either throw a clear Error
(e.g., "invalid max") or return a defined fallback, then compute pct and apply
the existing >= thresholds.

Comment thread models/Grade.ts
Comment on lines +41 to +87
function pickNumberFromUpdate(update: Record<string, unknown>, field: "marks" | "maxMarks"): number | undefined {
const direct = update[field];
if (typeof direct === "number" && Number.isFinite(direct)) return direct;

const $set = update.$set;
if ($set && typeof $set === "object") {
const setValue = ($set as Record<string, unknown>)[field];
if (typeof setValue === "number" && Number.isFinite(setValue)) return setValue;
}
});

GradeSchema.pre("updateOne", function () {
const update = this.getUpdate() as Record<string, unknown>;
if (update && typeof update === "object") {
const marks = update.marks;
const maxMarks = update.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");
return undefined;
}

async function validateMarksMaxMarksOnUpdate(this: mongoose.Query<unknown, IGrade>) {
const update = this.getUpdate() as Record<string, unknown> | null;
if (!update || typeof update !== "object") return;

let nextMarks = pickNumberFromUpdate(update, "marks");
let nextMaxMarks = pickNumberFromUpdate(update, "maxMarks");

if (nextMarks === undefined && nextMaxMarks === undefined) return;

if (nextMarks === undefined || nextMaxMarks === undefined) {
const currentRaw = await this.model.findOne(this.getQuery()).select("marks maxMarks").lean();
if (currentRaw && typeof currentRaw === "object") {
const current = currentRaw as { marks?: unknown; maxMarks?: unknown };
if (nextMarks === undefined && typeof current.marks === "number") nextMarks = current.marks;
if (nextMaxMarks === undefined && typeof current.maxMarks === "number") nextMaxMarks = current.maxMarks;
}

// For upsert flows where maxMarks is omitted, schema default is 100.
if (nextMarks !== undefined && nextMaxMarks === undefined) {
nextMaxMarks = 100;
}
}
});

if (
nextMarks !== undefined &&
nextMaxMarks !== undefined &&
nextMarks > nextMaxMarks
) {
throw new Error("marks must be less than or equal to maxMarks");
}
}

GradeSchema.pre("findOneAndUpdate", validateMarksMaxMarksOnUpdate);
GradeSchema.pre("updateOne", validateMarksMaxMarksOnUpdate);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Check whether any caller updates Grade.marks/maxMarks using non-$set operators.
rg -nP --type=ts -C2 '\$(inc|mul|min|max)\b' | rg -nC2 -i 'grade|marks|maxMarks'
rg -nP --type=ts -C2 'Grade\.(updateMany|replaceOne|bulkWrite)\b'

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 66


🏁 Script executed:

# 1. Look for Grade model update patterns in the codebase
rg -nP --type=ts 'Grade\.(findOneAndUpdate|updateOne|updateMany|replaceOne|bulkWrite)' -C3

# 2. Search for all Grade.* method calls to understand usage
rg -nP --type=ts 'Grade\.(find|update|delete|insert)' | head -30

# 3. Check for test files related to Grade
fd -t f '(test|spec|\.test|\.spec)' -i | grep -i grade

# 4. Look at actual update calls (any .update or similar pattern)
rg -nP --type=ts '\.(findOneAndUpdate|updateOne|updateMany)\s*\(' | head -20

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 2220


🏁 Script executed:

# 5. Verify the Grade model definition and hooks registered
rg -nP --type=ts 'GradeSchema\.(pre|post)' -C2

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 689


🏁 Script executed:

# 6. Check if there are any direct MongoDB operator patterns used anywhere
rg -nP --type=ts '\{\s*\$' -A2 | head -40

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 1498


🏁 Script executed:

# Check the marks validation in the API handler
cat -n app/api/grades/\[id\]/route.ts | sed -n '85,100p'

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 862


🏁 Script executed:

# Check the Grade creation/update validation logic
cat -n app/api/grades/route.ts | sed -n '60,75p'

Repository: JavaScript-Mastery-Pro/discord-challenge

Length of output: 946


Add defensive checks for atomic operators in Grade validator and register hooks for additional update methods.

The validator currently only inspects direct fields and $set nested fields. While the codebase exclusively uses $set for Grade updates (verified across app/api/grades/route.ts and app/api/grades/[id]/route.ts), a direct MongoDB call using { $inc: { marks: 999 } } or { $mul: { marks: 10 } } would silently bypass the marks ≤ maxMarks check.

Additionally, the validator is registered only for findOneAndUpdate and updateOne, but not for updateMany or replaceOne. While these aren't currently used for Grade writes, registering them preempts accidental constraint violations if the codebase evolves.

Recommended fix:

 async function validateMarksMaxMarksOnUpdate(this: mongoose.Query<unknown, IGrade>) {
   const update = this.getUpdate() as Record<string, unknown> | null;
   if (!update || typeof update !== "object") return;

+  for (const op of ["$inc", "$mul", "$min", "$max"] as const) {
+    const opVal = (update as Record<string, unknown>)[op];
+    if (opVal && typeof opVal === "object") {
+      const keys = Object.keys(opVal as Record<string, unknown>);
+      if (keys.some((k) => k === "marks" || k === "maxMarks")) {
+        throw new Error(`Updating marks/maxMarks via ${op} is not supported`);
+      }
+    }
+  }
+
   let nextMarks = pickNumberFromUpdate(update, "marks");

Also register the validator for updateMany and replaceOne:

 GradeSchema.pre("findOneAndUpdate", validateMarksMaxMarksOnUpdate);
 GradeSchema.pre("updateOne", validateMarksMaxMarksOnUpdate);
+GradeSchema.pre("updateMany", validateMarksMaxMarksOnUpdate);
+GradeSchema.pre("replaceOne", validateMarksMaxMarksOnUpdate);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@models/Grade.ts` around lines 41 - 87, The validator only inspects direct
fields and $set, so extend pickNumberFromUpdate/validateMarksMaxMarksOnUpdate to
defensively detect other atomic operators that can affect marks/maxMarks (at
least $inc, $mul, $min, $max) and compute resulting numeric values when
possible: when an operator like $inc/$mul/$min/$max is present for "marks" or
"maxMarks", if the operator value is a finite number and the current value
(fetched via this.model.findOne(this.getQuery()).select("marks
maxMarks").lean()) is numeric, apply the operator to derive
nextMarks/nextMaxMarks (e.g., next = current + inc, current * mul, min(current,
val), max(current, val)); if you cannot derive a finite result, treat the next
value as unknown so the existing checks will fetch current/defaults and still
validate. Finally, register validateMarksMaxMarksOnUpdate on GradeSchema.pre for
"updateMany" and "replaceOne" in addition to the existing "findOneAndUpdate" and
"updateOne".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant