Skip to content

Feature/create batch#223

Open
codewkaushik404 wants to merge 10 commits intoOpenLake:mainfrom
codewkaushik404:feature/create-batch
Open

Feature/create batch#223
codewkaushik404 wants to merge 10 commits intoOpenLake:mainfrom
codewkaushik404:feature/create-batch

Conversation

@codewkaushik404
Copy link

@codewkaushik404 codewkaushik404 commented Jan 30, 2026


name: " Pull Request"
about: Propose and submit changes to the project for review
title: "PR: Implement createBatch endpoint ( /api/certificate-batch/ )"
labels: ""
assignees: harshitap1305, sakshi1755

Related Issue


Changes Introduced

  • Added: isAuthenticated middleware that verifies JWT from the Authorization header and attaches the verified user to req.user.
  • Added: createBatch controller (implemented from scratch) to create certificate batches for a club with server-side approver resolution.
  • Updated: Controllers to use Zod validation utilities (validateBatchSchema, zodObjectId) and return proper 4xx/5xx responses on validation/auth errors.
  • Added: DB lookups and checks to ensure only authorized CLUB_COORDINATORs can initiate a batch and approvers (GENSEC & President) are resolved server-side.
  • Added: User ID validation (zodObjectId + DB existence checks) for the users array before batch creation.

Why This Change?

  • Problem: No JWT-based guard for protected endpoints and no robust server-side implementation for creating certificate batches.
  • Solution: Implemented JWT verification middleware and a complete createBatch flow that:
    • Validates input
    • Enforces role and position/unit ownership
    • Verifies organization hierarchy (Club → Council)
    • Resolves approvers (GENSEC & President) server-side (tamper-proof)
    • Validates recipients
    • Creates a CertificateBatch document only when all checks pass
  • Impact: Secure and reliable certificate batch creation that prevents tampered client-side data and unauthorized access.

Screenshots

image image

Testing

  • Ran unit tests and all passed (npm test in the relevant directory).
  • Manually tested the following scenarios:
  • Authorized CLUB_COORDINATOR creates valid batch → 200 + created batch returned
  • Non-coordinator or coordinator for a different club → 403 Forbidden
  • Invalid/malformed user IDs → 400 with details of invalid entries
  • Missing approvers or broken org structure → 500 / clear error message
  • Tested on different browsers (Chrome, Firefox) for UI changes.
  • Verified there are no new console warnings or errors.

Documentation Updates

  • Updated the README.md with new instructions.
  • Added clear code comments where logic is complex.
  • N/A

Checklist

  • I have created a new branch for this PR (git checkout -b feature/my-amazing-feature).
  • I have starred the repository.
  • My code follows the project's coding style and conventions.
  • My commit messages are clear and follow the project's guidelines.
  • I have performed a self-review of my own code.
  • I have added tests that prove my fix is effective or that my feature works.
  • All new and existing tests passed locally with my changes.
  • This PR introduces no breaking changes (or they are clearly documented).

Deployment Notes

  • Requires a database migration/schema update.
  • Requires new environment variables to be set.
  • N/A

Summary by CodeRabbit

Release Notes

  • New Features

    • Added JWT-based token authentication for API access
    • Introduced certificate batch creation and management functionality
    • Enhanced profile photo handling with automatic image management
  • Bug Fixes

    • Improved password-based authentication with secure hashing
    • Strengthened input validation for forms and data submissions
  • Chores

    • Updated authentication infrastructure and dependencies
    • Refined database configuration and middleware setup

✏️ Tip: You can customize this high-level summary in your review settings.

@vercel
Copy link

vercel bot commented Jan 30, 2026

@codewkaushik404 is attempting to deploy a commit to the openlake's projects Team on Vercel.

A member of the Team first needs to authorize it.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 30, 2026

Walkthrough

The pull request transitions from local Passport authentication to JWT-based authentication, removes legacy passport-local dependencies, rewrites the user schema with automatic timestamps and conditional password validation, introduces certificate batch creation functionality with validation, and updates all route imports to use named exports for authentication middleware.

Changes

Cohort / File(s) Summary
Authentication & JWT Middleware
backend/middlewares/isAuthenticated.js, backend/routes/auth.js
Introduced jwtIsAuthenticated middleware for Bearer token validation; replaced passport-local login with bcrypt password verification and JWT signing; refactored registration with bcrypt hashing; updated middleware exports to named exports.
User Schema & Database
backend/models/schema.js, backend/config/db.js
Restructured user schema to nested mongoose.Schema with automatic timestamps, removed passportLocalMongoose and mongoose-findorcreate plugins, added dynamic password requirement (conditional on "local" strategy), and removed explicit Mongoose connection options.
Certificate Management
backend/models/certificateSchema.js, backend/controllers/certificateController.js, backend/routes/certificateRoutes.js, backend/utils/batchValidate.js
Created new CertificateBatch and Certificate schemas with indexes and partialFilterExpressions; implemented createBatch controller with validation, approver resolution, and user verification; wired new /api/certificate-batches route with JWT authentication; added Zod-based batch validation schema.
Authentication Validation
backend/utils/authValidate.js
Introduced Zod-based validation schemas for login and registration endpoints with email pattern matching and password constraints.
Passport Configuration
backend/config/passportConfig.js
Removed LocalStrategy import and registration; updated User model import path from local to ../models/schema.
Route Import Updates
backend/routes/achievements.js, backend/routes/announcements.js, backend/routes/dashboard.js, backend/routes/events.js, backend/routes/feedbackRoutes.js, backend/routes/onboarding.js, backend/routes/orgUnit.js, backend/routes/positionRoutes.js, backend/routes/skillsRoutes.js
Updated isAuthenticated imports from default to named exports across all route modules; minor formatting and spacing adjustments.
Profile Route Updates
backend/routes/profile.js, backend/routes/analytics.js
Refactored photo update handling with old image deletion and streamUpload integration; reformatted import statements to double quotes; updated updateStudentProfile with explicit field-level conditional handling and timestamp management.
Main Application Setup
backend/index.js, backend/package.json
Added DB connection initialization, cookie-parser middleware, and native express.json() in place of body-parser; wired new route modules (events, skills, achievements, positions, orgUnit, certificates); updated passport config path; replaced body-parser with bcrypt; added cookie-parser and zod dependencies; removed mongoose plugins and passport-local dependencies.

Sequence Diagrams

sequenceDiagram
    participant Client
    participant AuthRoute as Auth Route
    participant AuthValidate as Auth Validator
    participant User as User Model
    participant Bcrypt
    participant JWT as JWT Sign
    participant Cookie as Cookie

    Client->>AuthRoute: POST /login {username, password}
    AuthRoute->>AuthValidate: Validate input
    AuthValidate-->>AuthRoute: ✓ Valid
    AuthRoute->>User: Find user by username
    User-->>AuthRoute: User found
    AuthRoute->>Bcrypt: Compare password hash
    Bcrypt-->>AuthRoute: ✓ Match
    AuthRoute->>JWT: Sign token {user_id, role}
    JWT-->>AuthRoute: Signed JWT (1hr expiry)
    AuthRoute->>Cookie: Set httpOnly cookie
    AuthRoute-->>Client: {success: true, token}
Loading
sequenceDiagram
    participant Client
    participant CertRoute as Certificate Route
    participant JWTAuth as JWT Middleware
    participant CreateBatch as createBatch Controller
    participant BatchValidate as Batch Validator
    participant User as User Model
    participant OrgUnit as Org Unit Model
    participant Approver as Approver Lookup
    participant CertBatch as Certificate Batch Model

    Client->>CertRoute: POST /api/certificate-batches {Authorization header}
    CertRoute->>JWTAuth: Validate Bearer token
    JWTAuth-->>CertRoute: ✓ Decoded user
    CreateBatch->>User: Verify user exists & is CLUB_COORDINATOR
    User-->>CreateBatch: ✓ User + position
    CreateBatch->>BatchValidate: Validate batch payload
    BatchValidate-->>CreateBatch: ✓ Valid data
    CreateBatch->>OrgUnit: Verify target unit is Club
    OrgUnit-->>CreateBatch: ✓ Club + Council
    CreateBatch->>Approver: Resolve approvers from Council
    Approver-->>CreateBatch: General Secretary & President
    CreateBatch->>User: Verify all batch users exist
    User-->>CreateBatch: ✓ User IDs valid
    CreateBatch->>CertBatch: Create CertificateBatch record
    CertBatch-->>CreateBatch: ✓ Batch created
    CreateBatch-->>Client: {success: true, batch details}
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hopping through the changes we go,
JWT tokens begin to flow,
Certificates batch up, schemas renew,
Old passport guards fade from view,
Bcrypt hashing keeps secrets safe and sound,
Authentication's new ground we have found!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Title check ❓ Inconclusive The title 'Feature/create batch' is vague and does not clearly convey the specific change; it lacks descriptive detail about what the feature does or which endpoint was created. Use a more specific and descriptive title such as 'Add JWT authentication and createBatch endpoint for certificate management' to better summarize the main changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Description check ✅ Passed The PR description comprehensively covers all required template sections including related issues, detailed changes introduced, problem/solution/impact analysis, test scenarios, and deployment notes, aligning well with the template structure.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

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

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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
Contributor

@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: 11

Caution

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

⚠️ Outside diff range comments (4)
backend/config/db.js (1)

7-13: ⚠️ Potential issue | 🟠 Major

Fail fast when MONGODB_URI is missing or connect fails.

Right now the server can keep running without a DB connection, which will surface as confusing runtime errors later. Guard the env var and rethrow to halt startup on failure.

🔧 Proposed fix
 const connectDB = async () => {
   try {
     const ConnectDB = process.env.MONGODB_URI;
+    if (!ConnectDB) {
+      throw new Error("MONGODB_URI is not set");
+    }
     //Removing the options as they are no longer needed from mongoose6+
     await mongoose.connect(ConnectDB);
     console.log("MongoDB Connected");
   } catch (error) {
     console.error("MongoDB Connection Error:", error);
+    throw error;
   }
 };
backend/routes/profile.js (3)

9-31: ⚠️ Potential issue | 🟠 Major

Enforce ownership/role checks for ID_No-based updates.

ID_No comes from the request body, so any authenticated user could update someone else’s photo unless you verify ownership or privileged roles. This is a horizontal privilege escalation risk.
Consider using req.user as the source of truth (or allow overrides only for explicit roles).

🔒 Suggested guard (adjust role/field names to your auth model)
-      const { ID_No } = req.body;
+      const { ID_No } = req.body;
       if (!ID_No) {
         return res.status(400).json({ error: "ID_No is required" });
       }
+      if (req.user?.user_id !== ID_No) {
+        return res.status(403).json({ error: "Forbidden" });
+      }

106-111: ⚠️ Potential issue | 🟠 Major

Remove debug logs that include user data.

userId and updatedDetails can contain PII (email, phone, DOB, etc.). Logging these in production is a compliance/privacy risk.

🧹 Suggested removal
-    console.log("Received userId:", userId);
-    console.log("Received updatedDetails:", updatedDetails);

219-220: ⚠️ Potential issue | 🟡 Minor

Remove manual updated_at assignment; it's redundant with Mongoose timestamps.

The schema uses { timestamps: true }, which automatically manages the updatedAt field (camelCase). Manually setting updated_at (snake_case) is ignored under strict mode (the default) and doesn't serve any purpose.

🧽 Suggested cleanup
-    // Update the updated_at timestamp
-    user.updated_at = new Date();
🤖 Fix all issues with AI agents
In `@backend/controllers/certificateController.js`:
- Around line 14-16: There's a typo in the error JSON returned when a user isn't
found: the key is "messge" instead of "message"; update the response in the
controller (the block that checks `if (!user) { return
res.status(404).json({...}) }`) to use `message: "Invalid data (User not
found)"` so the client receives the correct key (look for the `user` variable
and the `res.status(404).json` call in certificateController.js).
- Around line 18-22: Update the role guard to require an explicit
CLUB_COORDINATOR role instead of allowing missing roles: replace the current
conditional that only checks user.role !== "CLUB_COORDINATOR" with a strict
check that returns 403 when user.role is falsy or not exactly "CLUB_COORDINATOR"
(i.e., if (!user.role || user.role !== "CLUB_COORDINATOR") return
res.status(403).json(...)); locate and change the check around the user.role
validation in certificateController.js where the current guard is implemented.
- Around line 39-116: The code dereferences and logs lookup results before null
checks (e.g., positionHolder, position, gensecObj, presidentHolder,
presidentObj), which can throw if a record is missing; move all console.log
calls until after the corresponding existence checks and add explicit guards for
presidentHolder and presidentObj (returning the proper error responses like the
existing patterns) after PositionHolder.findOne, Position.findById, User.findOne
(gensecObj), Position.findOne (presidentPosition), PositionHolder.findOne
(presidentHolder) and User.findById (presidentId) to ensure no property is
accessed on null/undefined.
- Around line 121-126: The check after calling zodObjectId.safeParse in the
users.map Promise (used to build userChecks) is incorrect because safeParse
always returns an object; change the conditional to check validation.success
(e.g., if (!validation.success)) and return the invalid response when success is
false; update any later uses that rely on validation.data to use validation.data
only when validation.success is true (inside the branch where the ID is valid).

In `@backend/middlewares/isAuthenticated.js`:
- Around line 13-28: In jwtIsAuthenticated, ensure process.env.JWT_SECRET_TOKEN
is present and return res.status(500).json({ message: "Server misconfiguration:
JWT secret missing" }) if not; when calling jwt.verify(token,
process.env.JWT_SECRET_TOKEN) pass an explicit algorithms allow-list (e.g.
algorithms: ['HS256']) to prevent algorithm downgrade attacks, assign the
verified payload to req.user as before, and keep the existing catch to return
401 for invalid/expired tokens.

In `@backend/models/certificateSchema.js`:
- Around line 62-66: The certificateId field is marked unique which causes
duplicate-key errors for multiple non-approved docs; remove the unique: true
from the certificateId field in the certificateSchema and instead add a partial
unique index on certificateId that only applies when status === "Approved"
(ensure the index also requires certificateId exists). Locate the schema
definition (certificateId in certificateSchema / CertificateSchema) and replace
the field-level unique constraint with a schema-level index using
partialFilterExpression for status:"Approved" (and certificateId existence) so
only approved documents enforce uniqueness.
- Around line 6-9: The ref name for the unit_id schema field is misspelled as
"Oraganizational_Unit"; update the unit_id field's ref to the correctly
registered model name "Organizational_Unit" so mongoose.populate() resolves
properly (locate the unit_id definition in the certificate schema and replace
the ref string).

In `@backend/routes/auth.js`:
- Around line 45-51: Replace incorrect res.json(401).json(...) calls with
res.status(401).json(...) in the auth route handler(s) where authentication
fails (the checks using bcrypt.compare and the earlier invalid-credentials
check), and make the same change for the other occurrence around the second
failure block (the one referenced at lines 89-92); update the two places that
call res.json(401).json to use res.status(401).json to set the HTTP status
correctly and avoid double-sending headers.

In `@backend/routes/feedbackRoutes.js`:
- Around line 189-212: The route currently trusts req.body.resolved_by which
allows spoofing; instead ignore the body field and derive the resolver from the
authenticated user (req.user). In the feedback resolution handler (where
feedbackId, actions_taken, resolved_by are read and Feedback.findById is
called), remove usage of req.body.resolved_by and set feedback.resolved_by =
req.user.id or req.user.email (whichever is the canonical identifier in your
auth model) after verifying req.user exists; keep validation for actions_taken
and the rest of the update logic, and ensure the API does not accept or persist
a client-supplied resolved_by.

In `@backend/routes/profile.js`:
- Around line 80-90: The code in the router.delete("/photo-delete",
isAuthenticated, async (req, res) => { ... }) handler calls user.findOne(...)
but `user` is undefined; update the handler to use the correct User model
reference (e.g., `User.findOne(...)`) and ensure the model is imported/required
at the top (or rename the local variable to avoid shadowing, e.g., `const
foundUser = await User.findOne({ user_id: ID_No })`), then update subsequent
references to use that variable (`foundUser`) instead of the undefined `user`.

In `@backend/utils/batchValidate.js`:
- Around line 3-10: The zodObjectId regex is too permissive (allows non-hex
chars); update the zodObjectId used by validateBatchSchema to enforce hex-only
ObjectIds by replacing the pattern with a hex-only regex (e.g.,
/^[0-9a-fA-F]{24}$/) so zodObjectId = zod.string().regex(...); ensure the same
updated zodObjectId is used in validateBatchSchema for unit_id and users array
elements.
🧹 Nitpick comments (1)
backend/package.json (1)

33-58: Verified: bcrypt, cookie-parser, and zod versions are secure and current.

All three packages check out—bcrypt 6.0.0 has no known advisories (prior CVEs fixed in 5.0.0+), cookie-parser 1.4.7 has no direct vulnerabilities, and zod 4.3.6 is the current latest stable release.

However, I notice express-rate-limiter@1.3.1 is also in the dependencies. This package is effectively unmaintained (~10 years old) and, while no CVEs are recorded, poses a supply chain risk. If it's unused, remove it; if actively used, consider migrating to express-rate-limit (the maintained alternative, already in your dependencies at 7.5.1) which has recent security hardening.

Comment on lines +14 to +16
if (!user) {
return res.status(404).json({ messge: "Invalid data (User not found)" });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Typo in error response key.

messge should be message.

💡 Suggested fix
-    return res.status(404).json({ messge: "Invalid data (User not found)" });
+    return res.status(404).json({ message: "Invalid data (User not found)" });
📝 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
if (!user) {
return res.status(404).json({ messge: "Invalid data (User not found)" });
}
if (!user) {
return res.status(404).json({ message: "Invalid data (User not found)" });
}
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 14 - 16, There's a
typo in the error JSON returned when a user isn't found: the key is "messge"
instead of "message"; update the response in the controller (the block that
checks `if (!user) { return res.status(404).json({...}) }`) to use `message:
"Invalid data (User not found)"` so the client receives the correct key (look
for the `user` variable and the `res.status(404).json` call in
certificateController.js).

Comment on lines +18 to +22
if (user.role && user.role !== "CLUB_COORDINATOR") {
return res
.status(403)
.json({ message: "Not authorized to perform the task" });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Require explicit CLUB_COORDINATOR role.

The current guard allows users with a missing role to proceed. Enforce a strict role check to avoid accidental privilege bypass.

💡 Suggested fix
-  if (user.role && user.role !== "CLUB_COORDINATOR") {
+  if (user.role !== "CLUB_COORDINATOR") {
     return res
       .status(403)
       .json({ message: "Not authorized to perform the task" });
   }
📝 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
if (user.role && user.role !== "CLUB_COORDINATOR") {
return res
.status(403)
.json({ message: "Not authorized to perform the task" });
}
if (user.role !== "CLUB_COORDINATOR") {
return res
.status(403)
.json({ message: "Not authorized to perform the task" });
}
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 18 - 22, Update
the role guard to require an explicit CLUB_COORDINATOR role instead of allowing
missing roles: replace the current conditional that only checks user.role !==
"CLUB_COORDINATOR" with a strict check that returns 403 when user.role is falsy
or not exactly "CLUB_COORDINATOR" (i.e., if (!user.role || user.role !==
"CLUB_COORDINATOR") return res.status(403).json(...)); locate and change the
check around the user.role validation in certificateController.js where the
current guard is implemented.

Comment on lines +39 to +116
console.log(id);
// Get coordinator's position and unit
const positionHolder = await PositionHolder.findOne({ user_id: id });
console.log(positionHolder._id);
if (!positionHolder) {
return res
.status(403)
.json({ message: "You are not part of any position in a unit" });
}

const position = await Position.findById(positionHolder.position_id);
console.log(position._id);
if (!position) {
return res.status(403).json({ message: "Your position is invalid" });
}

const userUnitId = position.unit_id.toString();
if (userUnitId !== unit_id) {
return res
.status(403)
.json({
message:
"You are not authorized to initiate batches outside of your club",
});
}

//const clubId = unit_id;
// Ensure unit_id is a Club
const unitObj = await OrganizationalUnit.findById(unit_id);
if (!unitObj || unitObj.type !== "Club") {
return res
.status(403)
.json({ message: "Invalid Data: unit is not a Club" });
}
console.log(unitObj._id);

// Get council (parent unit) and ensure it's a Council
if (!unitObj.parent_unit_id) {
return res
.status(403)
.json({ message: "Invalid Data: club does not belong to a council" });
}
console.log(unitObj.parent_unit_id);

const councilObj = await OrganizationalUnit.findById(unitObj.parent_unit_id);
if (!councilObj || councilObj.type !== "Council") {
return res.status(403).json({ message: "Invalid Data: council not found" });
}

//const councilId = councilObj._id.toString();
const presidentOrgUnitId = councilObj.parent_unit_id;
const category = councilObj.category.toUpperCase();

// Resolve General Secretary and President for the council (server-side, tamper-proof)
const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
console.log(gensecObj._id);

const presidentPosition = await Position.findOne({
unit_id: presidentOrgUnitId,
title: /president/i,
});
if (!presidentPosition) {
return res
.status(500)
.json({ message: "President position not found for council" });
}
console.log(presidentPosition._id);

const presidentHolder = await PositionHolder.findOne({
position_id: presidentPosition._id,
});
const presidentId = presidentHolder.user_id.toString();
console.log(presidentId);
const presidentObj = await User.findById(presidentId);

console.log(presidentObj._id);
if (!gensecObj || !presidentObj) {
return res.status(500).json({ message: "Approvers not found" });
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Guard nulls before dereferencing lookup results.

Several lookups are logged/dereferenced before null checks (positionHolder, position, gensecObj, presidentHolder/Obj). A missing record will throw and bypass the intended error response. Move logs after checks and add a guard for presidentHolder.

💡 Suggested fix
-  console.log(id);
-  const positionHolder = await PositionHolder.findOne({ user_id: id });
-  console.log(positionHolder._id);
-  if (!positionHolder) {
+  const positionHolder = await PositionHolder.findOne({ user_id: id });
+  if (!positionHolder) {
     return res
       .status(403)
       .json({ message: "You are not part of any position in a unit" });
   }
+  console.log(positionHolder._id);

-  const position = await Position.findById(positionHolder.position_id);
-  console.log(position._id);
-  if (!position) {
+  const position = await Position.findById(positionHolder.position_id);
+  if (!position) {
     return res.status(403).json({ message: "Your position is invalid" });
   }
+  console.log(position._id);

-  const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
-  console.log(gensecObj._id);
+  const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
+  if (!gensecObj) {
+    return res.status(500).json({ message: "Approvers not found" });
+  }
+  console.log(gensecObj._id);

   const presidentPosition = await Position.findOne({
     unit_id: presidentOrgUnitId,
     title: /president/i,
   });
   if (!presidentPosition) {
     return res
       .status(500)
       .json({ message: "President position not found for council" });
   }
   console.log(presidentPosition._id);

   const presidentHolder = await PositionHolder.findOne({
     position_id: presidentPosition._id,
   });
+  if (!presidentHolder) {
+    return res
+      .status(500)
+      .json({ message: "President position holder not found" });
+  }
   const presidentId = presidentHolder.user_id.toString();
   console.log(presidentId);
   const presidentObj = await User.findById(presidentId);

-  console.log(presidentObj._id);
-  if (!gensecObj || !presidentObj) {
+  if (!presidentObj) {
     return res.status(500).json({ message: "Approvers not found" });
   }
+  console.log(presidentObj._id);
📝 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
console.log(id);
// Get coordinator's position and unit
const positionHolder = await PositionHolder.findOne({ user_id: id });
console.log(positionHolder._id);
if (!positionHolder) {
return res
.status(403)
.json({ message: "You are not part of any position in a unit" });
}
const position = await Position.findById(positionHolder.position_id);
console.log(position._id);
if (!position) {
return res.status(403).json({ message: "Your position is invalid" });
}
const userUnitId = position.unit_id.toString();
if (userUnitId !== unit_id) {
return res
.status(403)
.json({
message:
"You are not authorized to initiate batches outside of your club",
});
}
//const clubId = unit_id;
// Ensure unit_id is a Club
const unitObj = await OrganizationalUnit.findById(unit_id);
if (!unitObj || unitObj.type !== "Club") {
return res
.status(403)
.json({ message: "Invalid Data: unit is not a Club" });
}
console.log(unitObj._id);
// Get council (parent unit) and ensure it's a Council
if (!unitObj.parent_unit_id) {
return res
.status(403)
.json({ message: "Invalid Data: club does not belong to a council" });
}
console.log(unitObj.parent_unit_id);
const councilObj = await OrganizationalUnit.findById(unitObj.parent_unit_id);
if (!councilObj || councilObj.type !== "Council") {
return res.status(403).json({ message: "Invalid Data: council not found" });
}
//const councilId = councilObj._id.toString();
const presidentOrgUnitId = councilObj.parent_unit_id;
const category = councilObj.category.toUpperCase();
// Resolve General Secretary and President for the council (server-side, tamper-proof)
const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
console.log(gensecObj._id);
const presidentPosition = await Position.findOne({
unit_id: presidentOrgUnitId,
title: /president/i,
});
if (!presidentPosition) {
return res
.status(500)
.json({ message: "President position not found for council" });
}
console.log(presidentPosition._id);
const presidentHolder = await PositionHolder.findOne({
position_id: presidentPosition._id,
});
const presidentId = presidentHolder.user_id.toString();
console.log(presidentId);
const presidentObj = await User.findById(presidentId);
console.log(presidentObj._id);
if (!gensecObj || !presidentObj) {
return res.status(500).json({ message: "Approvers not found" });
// Get coordinator's position and unit
const positionHolder = await PositionHolder.findOne({ user_id: id });
if (!positionHolder) {
return res
.status(403)
.json({ message: "You are not part of any position in a unit" });
}
console.log(positionHolder._id);
const position = await Position.findById(positionHolder.position_id);
if (!position) {
return res.status(403).json({ message: "Your position is invalid" });
}
console.log(position._id);
const userUnitId = position.unit_id.toString();
if (userUnitId !== unit_id) {
return res
.status(403)
.json({
message:
"You are not authorized to initiate batches outside of your club",
});
}
//const clubId = unit_id;
// Ensure unit_id is a Club
const unitObj = await OrganizationalUnit.findById(unit_id);
if (!unitObj || unitObj.type !== "Club") {
return res
.status(403)
.json({ message: "Invalid Data: unit is not a Club" });
}
console.log(unitObj._id);
// Get council (parent unit) and ensure it's a Council
if (!unitObj.parent_unit_id) {
return res
.status(403)
.json({ message: "Invalid Data: club does not belong to a council" });
}
console.log(unitObj.parent_unit_id);
const councilObj = await OrganizationalUnit.findById(unitObj.parent_unit_id);
if (!councilObj || councilObj.type !== "Council") {
return res.status(403).json({ message: "Invalid Data: council not found" });
}
//const councilId = councilObj._id.toString();
const presidentOrgUnitId = councilObj.parent_unit_id;
const category = councilObj.category.toUpperCase();
// Resolve General Secretary and President for the council (server-side, tamper-proof)
const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
if (!gensecObj) {
return res.status(500).json({ message: "Approvers not found" });
}
console.log(gensecObj._id);
const presidentPosition = await Position.findOne({
unit_id: presidentOrgUnitId,
title: /president/i,
});
if (!presidentPosition) {
return res
.status(500)
.json({ message: "President position not found for council" });
}
console.log(presidentPosition._id);
const presidentHolder = await PositionHolder.findOne({
position_id: presidentPosition._id,
});
if (!presidentHolder) {
return res
.status(500)
.json({ message: "President position holder not found" });
}
const presidentId = presidentHolder.user_id.toString();
console.log(presidentId);
const presidentObj = await User.findById(presidentId);
if (!presidentObj) {
return res.status(500).json({ message: "Approvers not found" });
}
console.log(presidentObj._id);
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 39 - 116, The code
dereferences and logs lookup results before null checks (e.g., positionHolder,
position, gensecObj, presidentHolder, presidentObj), which can throw if a record
is missing; move all console.log calls until after the corresponding existence
checks and add explicit guards for presidentHolder and presidentObj (returning
the proper error responses like the existing patterns) after
PositionHolder.findOne, Position.findById, User.findOne (gensecObj),
Position.findOne (presidentPosition), PositionHolder.findOne (presidentHolder)
and User.findById (presidentId) to ensure no property is accessed on
null/undefined.

Comment on lines +121 to +126
const userChecks = await Promise.all(
users.map(async (uid) => {
const validation = zodObjectId.safeParse(uid);
if (!validation) {
return { uid, ok: false, reason: "Invalid ID" };
}
Copy link
Contributor

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:

find . -type f -name "certificateController.js" | head -5

Repository: OpenLake/Student_Database_COSA

Length of output: 307


🌐 Web query:

Zod safeParse return value structure success

💡 Result:

z.safeParse(value) returns a discriminated union keyed by success:

  • Success case: { success: true; data: T }
  • Failure case: { success: false; error: ZodError }

So you typically do:

const result = schema.safeParse(input);

if (result.success) {
  result.data;  // typed as T
} else {
  result.error; // ZodError
}

Sources: Zod docs / method signatures show this exact return shape and that it’s a discriminated union. [1] [2]


Fix zodObjectId.safeParse result check.

safeParse returns { success: boolean, data?: T, error?: ZodError }; if (!validation) never triggers because safeParse always returns an object (truthy), so invalid IDs slip through and can cause cast errors. Check !validation.success instead.

💡 Suggested fix
       const validation = zodObjectId.safeParse(uid);
-      if (!validation) {
+      if (!validation.success) {
         return { uid, ok: false, reason: "Invalid ID" };
       }
📝 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 userChecks = await Promise.all(
users.map(async (uid) => {
const validation = zodObjectId.safeParse(uid);
if (!validation) {
return { uid, ok: false, reason: "Invalid ID" };
}
const userChecks = await Promise.all(
users.map(async (uid) => {
const validation = zodObjectId.safeParse(uid);
if (!validation.success) {
return { uid, ok: false, reason: "Invalid ID" };
}
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 121 - 126, The
check after calling zodObjectId.safeParse in the users.map Promise (used to
build userChecks) is incorrect because safeParse always returns an object;
change the conditional to check validation.success (e.g., if
(!validation.success)) and return the invalid response when success is false;
update any later uses that rely on validation.data to use validation.data only
when validation.success is true (inside the branch where the ID is valid).

Comment on lines +13 to +28
function jwtIsAuthenticated(req, res, next) {
let token;
const headerData = req.headers.authorization;
if (!headerData || !headerData.startsWith("Bearer ")) {
return res.status(401).json({ message: "User not authenticated " });
}

token = headerData.split(" ")[1];
try {
const userData = jwt.verify(token, process.env.JWT_SECRET_TOKEN);
req.user = userData;
//console.log(userData);
next();
} catch (err) {
res.status(401).json({ message: "Invalid or expired token sent" });
}
Copy link
Contributor

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 package.json | grep -A 5 -B 5 jsonwebtoken

Repository: OpenLake/Student_Database_COSA

Length of output: 118


🌐 Web query:

jsonwebtoken npm library algorithms option security best practices

💡 Result:

jsonwebtoken (npm i jsonwebtoken) — algorithms option security best practices

  1. Pin/upgrade to v9+ and keep it updated. Versions <= 8.5.1 had a verification-bypass issue when verify() was called without an algorithms allow-list and with a falsy key, causing a dangerous default to none (fixed in 9.0.0). [1][2]
  2. Always pass an explicit algorithms allow-list to jwt.verify() (don’t rely on defaults). Example: allow only what you actually issue. [3]
  3. Never allow none in production. In v9, accepting unsigned tokens requires explicitly allowing none via options.algorithms—don’t do this unless you have a very specific, safe use case. [4]
  4. Don’t mix symmetric and asymmetric algorithms (e.g., don’t accept both HS256 and RS256 for the same verifier). This helps avoid “algorithm confusion/downgrade” classes of mistakes. [5]
  5. Prefer asymmetric signing (e.g., RS256/ES256) for most real systems, especially when multiple services verify tokens or key rotation matters. Auth0 (and many deployments) recommend RS256 over HS256 because verifiers only need a public key, and rotation is easier. [6]
  6. Validate claims, not just the signature: set issuer and audience (and use expiration). This reduces token replay/misuse across services. [5]
  7. Use verify(), not decode(), for untrusted tokens. decode() does not validate signatures/claims; treat decoded payload as untrusted input unless verified. [7]

Minimal safe verification pattern

import jwt from "jsonwebtoken";

const payload = jwt.verify(token, publicKeyPem, {
  algorithms: ["RS256"],   // strict allow-list
  issuer: "https://issuer.example",
  audience: "my-api",
  // optionally: maxAge: "15m"
});

Sources: GitHub Security Advisory + NVD for CVE-2022-23540 [1][2]; jsonwebtoken npm docs on algorithms option/defaults [3]; v8→v9 migration note on explicitly allowing none [4]; express-jwt guidance on required algorithms and not mixing HS/RS + issuer/audience [5]; Auth0 guidance recommending RS256 [6]; Auth0 JWT validation note to use verify() (not decode()) and disallow none [7].


🏁 Script executed:

find . -name "package.json" -type f | head -20

Repository: OpenLake/Student_Database_COSA

Length of output: 120


🏁 Script executed:

rg "jwt\." --type js -A 2 -B 2 | head -100

Repository: OpenLake/Student_Database_COSA

Length of output: 1563


🏁 Script executed:

rg "JWT_SECRET_TOKEN" --type js -A 3 -B 3

Repository: OpenLake/Student_Database_COSA

Length of output: 2072


🏁 Script executed:

cat backend/package.json | jq '.dependencies.jsonwebtoken, .devDependencies.jsonwebtoken'

Repository: OpenLake/Student_Database_COSA

Length of output: 87


🏁 Script executed:

cat -n backend/middlewares/isAuthenticated.js

Repository: OpenLake/Student_Database_COSA

Length of output: 2711


🏁 Script executed:

cat -n backend/routes/auth.js | grep -A 10 -B 5 "jwt.sign\|jwt.verify"

Repository: OpenLake/Student_Database_COSA

Length of output: 2473


Explicitly restrict JWT algorithms and validate secret configuration.

jwt.verify() in jsonwebtoken v9+ requires passing an explicit algorithms allow-list; the current code relies on defaults, which bypasses algorithm-restriction security. Additionally, missing JWT_SECRET_TOKEN should return 500 (server misconfiguration) rather than 401 (auth failure).

Proposed fix
 function jwtIsAuthenticated(req, res, next) {
   let token;
   const headerData = req.headers.authorization;
   if (!headerData || !headerData.startsWith("Bearer ")) {
     return res.status(401).json({ message: "User not authenticated " });
   }

   token = headerData.split(" ")[1];
   try {
-    const userData = jwt.verify(token, process.env.JWT_SECRET_TOKEN);
+    const secret = process.env.JWT_SECRET_TOKEN;
+    if (!secret) {
+      return res.status(500).json({ message: "JWT secret not configured" });
+    }
+    const userData = jwt.verify(token, secret, {
+      algorithms: ["HS256"],
+    });
     req.user = userData;
     //console.log(userData);
     next();
   } catch (err) {
     res.status(401).json({ message: "Invalid or expired token sent" });
   }
 }
📝 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
function jwtIsAuthenticated(req, res, next) {
let token;
const headerData = req.headers.authorization;
if (!headerData || !headerData.startsWith("Bearer ")) {
return res.status(401).json({ message: "User not authenticated " });
}
token = headerData.split(" ")[1];
try {
const userData = jwt.verify(token, process.env.JWT_SECRET_TOKEN);
req.user = userData;
//console.log(userData);
next();
} catch (err) {
res.status(401).json({ message: "Invalid or expired token sent" });
}
function jwtIsAuthenticated(req, res, next) {
let token;
const headerData = req.headers.authorization;
if (!headerData || !headerData.startsWith("Bearer ")) {
return res.status(401).json({ message: "User not authenticated " });
}
token = headerData.split(" ")[1];
try {
const secret = process.env.JWT_SECRET_TOKEN;
if (!secret) {
return res.status(500).json({ message: "JWT secret not configured" });
}
const userData = jwt.verify(token, secret, {
algorithms: ["HS256"],
});
req.user = userData;
//console.log(userData);
next();
} catch (err) {
res.status(401).json({ message: "Invalid or expired token sent" });
}
}
🤖 Prompt for AI Agents
In `@backend/middlewares/isAuthenticated.js` around lines 13 - 28, In
jwtIsAuthenticated, ensure process.env.JWT_SECRET_TOKEN is present and return
res.status(500).json({ message: "Server misconfiguration: JWT secret missing" })
if not; when calling jwt.verify(token, process.env.JWT_SECRET_TOKEN) pass an
explicit algorithms allow-list (e.g. algorithms: ['HS256']) to prevent algorithm
downgrade attacks, assign the verified payload to req.user as before, and keep
the existing catch to return 401 for invalid/expired tokens.

Comment on lines +62 to +66
certificateId: {
type: String,
unique: true,
required: function () {
return this.status === "Approved";
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

certificateId unique constraint will reject multiple non-approved docs.

With unique: true, multiple pending records without certificateId can trigger duplicate-key errors. Use a partial unique index instead and remove unique: true from the field.

🧩 Proposed fix
     certificateId: {
       type: String,
-      unique: true,
       required: function () {
         return this.status === "Approved";
       },
     },
+certificateSchema.index(
+  { certificateId: 1 },
+  { unique: true, partialFilterExpression: { certificateId: { $exists: true } } },
+);
MongoDB unique index behavior with missing field values (null/absent)
🤖 Prompt for AI Agents
In `@backend/models/certificateSchema.js` around lines 62 - 66, The certificateId
field is marked unique which causes duplicate-key errors for multiple
non-approved docs; remove the unique: true from the certificateId field in the
certificateSchema and instead add a partial unique index on certificateId that
only applies when status === "Approved" (ensure the index also requires
certificateId exists). Locate the schema definition (certificateId in
certificateSchema / CertificateSchema) and replace the field-level unique
constraint with a schema-level index using partialFilterExpression for
status:"Approved" (and certificateId existence) so only approved documents
enforce uniqueness.

Comment on lines +45 to +51
return res.json(401).json({ message: "Invalid user credentials" });
}

const isValid = await bcrypt.compare(password, user.password);
if (!isValid) {
return res.json(401).json({ message: "Invalid user credentials" });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Fix 401 responses: res.json(401) sends a 200 and double-sends.

Use res.status(401).json(...) for proper status and to avoid “headers already sent.”

🔧 Proposed fix
-      return res.json(401).json({ message: "Invalid user credentials" });
+      return res.status(401).json({ message: "Invalid user credentials" });
...
-      return res.json(401).json({ message: "Invalid user credentials" });
+      return res.status(401).json({ message: "Invalid user credentials" });
...
-      return res
-        .json(401)
-        .json({ message: "Account with username already exists" });
+      return res
+        .status(401)
+        .json({ message: "Account with username already exists" });

Also applies to: 89-92

🤖 Prompt for AI Agents
In `@backend/routes/auth.js` around lines 45 - 51, Replace incorrect
res.json(401).json(...) calls with res.status(401).json(...) in the auth route
handler(s) where authentication fails (the checks using bcrypt.compare and the
earlier invalid-credentials check), and make the same change for the other
occurrence around the second failure block (the one referenced at lines 89-92);
update the two places that call res.json(401).json to use res.status(401).json
to set the HTTP status correctly and avoid double-sending headers.

Comment on lines +189 to 212
const feedbackId = req.params.id;
const { actions_taken, resolved_by } = req.body;
console.log(req.body);
console.log("User resolving feedback:", resolved_by);

if (!actions_taken || actions_taken.trim() === "") {
return res.status(400).json({ error: "Resolution comment is required." });
}

if (feedback.is_resolved) {
return res.status(400).json({ error: "Feedback is already resolved." });
}
try {
const feedback = await Feedback.findById(feedbackId);
if (!feedback) {
return res.status(404).json({ error: "Feedback not found" });
}

feedback.is_resolved = true;
feedback.resolved_at = new Date();
feedback.actions_taken = actions_taken;
feedback.resolved_by = resolved_by;
if (feedback.is_resolved) {
return res.status(400).json({ error: "Feedback is already resolved." });
}

await feedback.save();
feedback.is_resolved = true;
feedback.resolved_at = new Date();
feedback.actions_taken = actions_taken;
feedback.resolved_by = resolved_by;

Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Derive resolved_by from the authenticated user, not the request body.

Clients can spoof resolved_by today. Use req.user and ignore the body field to preserve audit integrity.

🔐 Proposed fix
-    const { actions_taken, resolved_by } = req.body;
+    const { actions_taken } = req.body;
+    const resolved_by = req.user?._id;
+    if (!resolved_by) {
+      return res.status(401).json({ error: "Unauthorized" });
+    }
     console.log(req.body);
     console.log("User resolving feedback:", resolved_by);
📝 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 feedbackId = req.params.id;
const { actions_taken, resolved_by } = req.body;
console.log(req.body);
console.log("User resolving feedback:", resolved_by);
if (!actions_taken || actions_taken.trim() === "") {
return res.status(400).json({ error: "Resolution comment is required." });
}
if (feedback.is_resolved) {
return res.status(400).json({ error: "Feedback is already resolved." });
}
try {
const feedback = await Feedback.findById(feedbackId);
if (!feedback) {
return res.status(404).json({ error: "Feedback not found" });
}
feedback.is_resolved = true;
feedback.resolved_at = new Date();
feedback.actions_taken = actions_taken;
feedback.resolved_by = resolved_by;
if (feedback.is_resolved) {
return res.status(400).json({ error: "Feedback is already resolved." });
}
await feedback.save();
feedback.is_resolved = true;
feedback.resolved_at = new Date();
feedback.actions_taken = actions_taken;
feedback.resolved_by = resolved_by;
const feedbackId = req.params.id;
const { actions_taken } = req.body;
const resolved_by = req.user?._id;
if (!resolved_by) {
return res.status(401).json({ error: "Unauthorized" });
}
console.log(req.body);
console.log("User resolving feedback:", resolved_by);
if (!actions_taken || actions_taken.trim() === "") {
return res.status(400).json({ error: "Resolution comment is required." });
}
try {
const feedback = await Feedback.findById(feedbackId);
if (!feedback) {
return res.status(404).json({ error: "Feedback not found" });
}
if (feedback.is_resolved) {
return res.status(400).json({ error: "Feedback is already resolved." });
}
feedback.is_resolved = true;
feedback.resolved_at = new Date();
feedback.actions_taken = actions_taken;
feedback.resolved_by = resolved_by;
🤖 Prompt for AI Agents
In `@backend/routes/feedbackRoutes.js` around lines 189 - 212, The route currently
trusts req.body.resolved_by which allows spoofing; instead ignore the body field
and derive the resolver from the authenticated user (req.user). In the feedback
resolution handler (where feedbackId, actions_taken, resolved_by are read and
Feedback.findById is called), remove usage of req.body.resolved_by and set
feedback.resolved_by = req.user.id or req.user.email (whichever is the canonical
identifier in your auth model) after verifying req.user exists; keep validation
for actions_taken and the rest of the update logic, and ensure the API does not
accept or persist a client-supplied resolved_by.

Comment on lines +80 to +90
router.delete("/photo-delete", isAuthenticated, async (req, res) => {
try {
const { ID_No } = req.query; // Get ID_No from frontend for DELETE
if (!ID_No) { return res.status(400).json({ error: "ID_No is required" }); }
if (!ID_No) {
return res.status(400).json({ error: "ID_No is required" });
}

const user = await user.findOne({ user_id: ID_No });
if (!user) { return res.status(404).json({ error: "User not found" }); }
if (!user) {
return res.status(404).json({ error: "User not found" });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Fix User lookup (Line 87 uses an undefined identifier).

Line 87 uses user.findOne(...), but user isn’t defined. This will throw at runtime.

🐛 Fix the model reference
-    const user = await user.findOne({ user_id: ID_No });
+    const user = await User.findOne({ user_id: ID_No });
📝 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
router.delete("/photo-delete", isAuthenticated, async (req, res) => {
try {
const { ID_No } = req.query; // Get ID_No from frontend for DELETE
if (!ID_No) { return res.status(400).json({ error: "ID_No is required" }); }
if (!ID_No) {
return res.status(400).json({ error: "ID_No is required" });
}
const user = await user.findOne({ user_id: ID_No });
if (!user) { return res.status(404).json({ error: "User not found" }); }
if (!user) {
return res.status(404).json({ error: "User not found" });
}
router.delete("/photo-delete", isAuthenticated, async (req, res) => {
try {
const { ID_No } = req.query; // Get ID_No from frontend for DELETE
if (!ID_No) {
return res.status(400).json({ error: "ID_No is required" });
}
const user = await User.findOne({ user_id: ID_No });
if (!user) {
return res.status(404).json({ error: "User not found" });
}
🧰 Tools
🪛 Biome (2.3.13)

[error] 87-87: This variable is used before its declaration.

The variable is declared here:

(lint/correctness/noInvalidUseBeforeDeclaration)

🤖 Prompt for AI Agents
In `@backend/routes/profile.js` around lines 80 - 90, The code in the
router.delete("/photo-delete", isAuthenticated, async (req, res) => { ... })
handler calls user.findOne(...) but `user` is undefined; update the handler to
use the correct User model reference (e.g., `User.findOne(...)`) and ensure the
model is imported/required at the top (or rename the local variable to avoid
shadowing, e.g., `const foundUser = await User.findOne({ user_id: ID_No })`),
then update subsequent references to use that variable (`foundUser`) instead of
the undefined `user`.

Comment on lines +3 to +10
const zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId");

const validateBatchSchema = zod.object({
title: zod.string().min(5, "Title is required"),
unit_id: zodObjectId,
commonData: zod.record(zod.string(), zod.string()),
template_id: zod.string(),
users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."),
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

ObjectId regex is too permissive.

[0-9a-zA-Z]{24} accepts non-hex characters; Mongo ObjectIds are hex-only.

✅ Proposed fix
-const zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId");
+const zodObjectId = zod
+  .string()
+  .regex(/^[a-f0-9]{24}$/i, "Invalid ObjectId");
📝 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 zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId");
const validateBatchSchema = zod.object({
title: zod.string().min(5, "Title is required"),
unit_id: zodObjectId,
commonData: zod.record(zod.string(), zod.string()),
template_id: zod.string(),
users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."),
const zodObjectId = zod
.string()
.regex(/^[a-f0-9]{24}$/i, "Invalid ObjectId");
const validateBatchSchema = zod.object({
title: zod.string().min(5, "Title is required"),
unit_id: zodObjectId,
commonData: zod.record(zod.string(), zod.string()),
template_id: zod.string(),
users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."),
🤖 Prompt for AI Agents
In `@backend/utils/batchValidate.js` around lines 3 - 10, The zodObjectId regex is
too permissive (allows non-hex chars); update the zodObjectId used by
validateBatchSchema to enforce hex-only ObjectIds by replacing the pattern with
a hex-only regex (e.g., /^[0-9a-fA-F]{24}$/) so zodObjectId =
zod.string().regex(...); ensure the same updated zodObjectId is used in
validateBatchSchema for unit_id and users array elements.

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