refactor: move route logic into controllers#252
Merged
Ashish-Kumar-Dash merged 1 commit intoJun 1, 2026
Merged
Conversation
|
@KotapatiSaiMounika is attempting to deploy a commit to the openlake's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
WalkthroughThis PR extracts business logic from 10 route files into 11 new controller modules, establishing a cleaner separation of concerns across the backend. Route files now serve as thin wiring layers that delegate all request handling, validation, and data manipulation to controllers. The refactoring spans authentication, user profiles, achievements, skills, announcements, feedback, organizational management, and events. ChangesUnified Controller Extraction & Route Refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 17
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
backend/controllers/authController.js-211-258 (1)
211-258:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap initial database lookup in try-catch.
Same issue as
verifyResetPasswordToken: theUser.findOneat line 217 is outside the try-catch block. An invalid ObjectId or DB error will crash the request.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/authController.js` around lines 211 - 258, The User.findOne call in resetPassword is executed outside any try/catch and can throw (e.g., invalid ObjectId or DB error); wrap the initial database lookup (the User.findOne in resetPassword) inside the existing try/catch (or create a new try/catch around it) so any errors return an appropriate HTTP response instead of crashing—ensure you still perform the user existence check and proceed to jwt.verify, user.setPassword, and user.save inside the safe block and return 400/500 as appropriate on lookup errors.backend/controllers/onboardingController.js-18-22 (1)
18-22:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against undefined
Programbefore calling.trim().If
Programis not provided in the request body,Program.trim()will throwTypeError: Cannot read properties of undefined. Consider validating required fields or using optional chaining with a fallback.Proposed fix
academic_info: { - program: Program.trim(), + program: Program?.trim() || "", branch: discipline, batch_year: add_year, },Alternatively, add validation at the start of the function:
if (!Program || !discipline || !add_year) { return res.status(400).json({ message: "Missing required fields" }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/onboardingController.js` around lines 18 - 22, Guard against Program being undefined before calling .trim(): in the onboarding request handler (the function that builds academic_info) validate required fields (Program, discipline, add_year) at the top of the function and return a 400 response if any are missing, or use a safe fallback when constructing academic_info (e.g., program: (Program || "").trim()). Update the code where academic_info is built to use the safe expression or add the early validation check so Program.trim() cannot throw.backend/controllers/authController.js-175-209 (1)
175-209:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWrap database operations in try-catch.
User.findOneat line 183 is outside any try-catch. If the database throws (e.g., connection error, invalid ObjectId format for_id), the error will propagate unhandled and crash the request without a proper error response.Proposed fix
const verifyResetPasswordToken = async ( req, res ) => { const { id, token } = req.params; + + try { const user = await User.findOne({ _id: id, }); if (!user) { return res.status(404).json({ message: "User not found", }); } const secret = user._id + process.env.JWT_SECRET_TOKEN; - try { jwt.verify(token, secret); return res.status(200).json({ message: "Token verified successfully", }); } catch (error) { console.log(error); return res.status(400).json({ message: "Invalid or expired token", }); } };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/authController.js` around lines 175 - 209, The database lookup in verifyResetPasswordToken uses User.findOne outside any try-catch so DB errors (e.g., connection issues or invalid ObjectId) can crash the request; wrap the async operations — specifically the User.findOne call and the subsequent jwt.verify usage in the verifyResetPasswordToken function — in a try-catch block, return a 500 (or appropriate) JSON error response when the DB call throws, and preserve the existing 404 and token error responses inside that flow so all unexpected exceptions are handled gracefully.backend/controllers/skillController.js-27-40 (1)
27-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle malformed
:idas a 400 Bad Request.In
backend/controllers/skillController.js, multiple handlers callfindById/findByIdAndDeleteusingreq.params.id; with an invalid/malformed ObjectId, Mongoose rejects and yourcatchreturns 500, even though it’s a client input error. Validatereq.params.idbefore calling Mongoose (e.g.,mongoose.Types.ObjectId.isValid(id)) and return 400 when invalid, while keeping 404 for genuinely missing documents.
Lines 30, 48, 85, 106 (also applies to 44-64, 81-98, 102-122).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/skillController.js` around lines 27 - 40, The handler endorseUserSkill currently calls UserSkill.findById(req.params.id) without validating the id, causing malformed ObjectId errors to surface as 500; before calling Mongoose (in endorseUserSkill and other handlers using findById/findByIdAndDelete), validate req.params.id with mongoose.Types.ObjectId.isValid(id) and if invalid return res.status(400).json({ message: "Invalid id" }); otherwise proceed to call UserSkill.findById / UserSkill.findByIdAndDelete and keep the existing 404 handling for not-found documents.backend/controllers/eventControllers.js-66-80 (1)
66-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing 404 response when event is not found.
If
findByIdreturnsnull, the handler sendsnullin the response body (status 200). Other handlers likeisEventContact(line 144-148) anddeleteEvent(line 197-201) correctly return 404. Clients may not handle anullresponse gracefully.Proposed fix
exports.getEventById = async (req, res) => { try { const event = await Event.findById(req.params.id) .populate("organizing_unit_id", "name") .populate("organizers", "personal_info.name"); + if (!event) { + return res.status(404).json({ + message: "Event not found.", + }); + } + res.json(event); } catch (err) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/eventControllers.js` around lines 66 - 80, The getEventById controller currently returns null with a 200 when Event.findById yields no result; update exports.getEventById to check the retrieved event (from Event.findById(...).populate(...)) and if it's falsy send res.status(404).json({ message: "Event not found." }) instead of res.json(event), otherwise return the event as before; reference the getEventById function and the Event.findById call to locate the change.backend/routes/events.js-57-62 (1)
57-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPass array to
authorizeRoleinstead of string.
authorizeRole("PRESIDENT")passes a string, but the middleware usesallowedRoles.includes(userRole). String'sincludes()checks for substrings, not array element membership. It works here by coincidence, but is inconsistent with usages at line 52 that correctly pass arrays.Proposed fix
router.patch( "/room-requests/:requestId/status", isAuthenticated, - authorizeRole("PRESIDENT"), + authorizeRole([ROLES.PRESIDENT]), eventsController.updateRoomRequestStatus );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/events.js` around lines 57 - 62, The route currently calls authorizeRole with a string: change router.patch(..., authorizeRole("PRESIDENT"), ...) to pass an array instead (authorizeRole(["PRESIDENT"])) so the middleware’s allowedRoles.includes(userRole) works correctly; update the call that registers eventsController.updateRoomRequestStatus to use the array form consistent with other routes using authorizeRole.backend/controllers/announcementController.js-58-66 (1)
58-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing validation: non-General type without
targetIdentifiershould error.When
typeis non-General (e.g., "Event") buttargetIdentifieris missing, the announcement is created withtarget_id = null. This is inconsistent withupdateAnnouncement(lines 292-297) which enforces that non-General types require a target identifier.🛡️ Proposed fix
if (type !== "General" && targetIdentifier) { targetId = await findTargetId(type, targetIdentifier); if (!targetId) { return res.status(404).json({ error: `No ${type} found with that identifier`, }); } + } else if (type !== "General" && !targetIdentifier) { + return res.status(400).json({ + error: "targetIdentifier is required for non-General announcements", + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/announcementController.js` around lines 58 - 66, In the announcement creation flow in announcementController.js the code allows non-"General" announcements to proceed when targetIdentifier is missing, leaving target_id null; updateAnnouncement enforces this requirement. Modify the create logic handling type and targetIdentifier (the block using type, targetIdentifier, targetId and calling findTargetId) to first validate that if type !== "General" then targetIdentifier must be present and, if missing, return a 400 response with an explanatory error; otherwise call findTargetId as before and keep the existing 404 behavior when findTargetId returns falsy.backend/controllers/announcementController.js-120-127 (1)
120-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEscape regex special characters in search input to prevent ReDoS.
User-provided
searchis passed directly tonew RegExp()without escaping special regex characters. Malicious input like(a+)+$could cause catastrophic backtracking (ReDoS).🛡️ Proposed fix
+ const escapeRegex = (str) => + str.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + if (search) { - const regex = new RegExp(search, "i"); + const regex = new RegExp(escapeRegex(search), "i"); filter.$or = [ { title: regex }, { content: regex }, ]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/announcementController.js` around lines 120 - 127, The code constructs a RegExp from untrusted input (search) and assigns it into filter.$or for title/content, which enables ReDoS; fix by escaping regex metacharacters before calling new RegExp (e.g. add an escapeRegex helper and call const safe = escapeRegex(search) then const regex = new RegExp(safe, "i")), or use a safe text-search alternative, and replace the existing new RegExp(search, "i") usage in the block that builds filter.$or (title/content) with the escaped value.
🧹 Nitpick comments (5)
backend/routes/events.js (1)
3-4: ⚡ Quick winRemove unused imports after controller extraction.
Event,User,OrganizationalUnit, anduuidv4are no longer used in this file since logic moved to the controller.Proposed fix
const express = require("express"); const router = express.Router(); -const { Event, User, OrganizationalUnit } = require("../models/schema"); -const { v4: uuidv4 } = require("uuid"); const isAuthenticated = require("../middlewares/isAuthenticated");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/events.js` around lines 3 - 4, The top-level imports Event, User, OrganizationalUnit and the uuidv4 import are unused in this router (they were moved to the controller); remove the unused requires for Event, User, OrganizationalUnit and the uuidv4 import statement from events.js so only the router/controller imports remain, and run lint/CI to ensure no remaining references to these symbols (Event, User, OrganizationalUnit, uuidv4) in that file.backend/controllers/eventControllers.js (1)
535-541: 💤 Low valueConsider limiting fields when populating
participants.Populating
participantswithout field selection returns fullUserdocuments, potentially exposingpersonal_infodetails to any authenticated user. Consider selecting only needed fields (e.g.,name).Example
const events = await Event.find(query) .sort({ "schedule.start": 1 }) .populate("organizing_unit_id") .populate("organizers") - .populate("participants") + .populate("participants", "personal_info.name") .populate("winners.user") .populate("room_requests.reviewed_by");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/eventControllers.js` around lines 535 - 541, The query populates full User documents for participants (Event.find -> .populate("participants")) which may expose personal_info; change the populate call to explicitly select only needed fields (for example use populate with a path of "participants" and select only "name" and "_id" or otherwise exclude "personal_info") so participants do not return full User personal data; update the populate invocation for "participants" (and any other sensitive user-populates) to use the populate({ path: 'participants', select: 'name _id' }) pattern.backend/controllers/announcementController.js (2)
158-163: ⚡ Quick win
target_idnot populated when type filter is absent or "All".When no
typefilter is provided ortype === "All",filter.typeis undefined, sotarget_idis never populated—even for non-General announcements in the result set. Consider populatingtarget_idunconditionally or checking each document's type.♻️ Proposed fix - populate unconditionally
- if ( - filter.type && - filter.type !== "General" - ) { - query.populate("target_id"); - } + // Always populate target_id; General announcements will have null + query.populate("target_id");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/announcementController.js` around lines 158 - 163, The current conditional uses filter.type to decide whether to call query.populate("target_id"), which causes target_id to remain unpopulated when filter.type is absent or "All"; update the logic in announcementController (where query and filter.type are used) to always call query.populate("target_id") (i.e., populate unconditionally) so non-"General" announcements in the returned result have their target_id populated, or alternatively implement per-document population by checking each announcement's type before accessing target_id if you prefer lazy population.
15-22: 💤 Low valueUnused
objectIdvariable - use it or remove the conversion.The
objectIdvariable is created but never used. The query usesidentifierdirectly, which Mongoose will auto-cast. Either use the convertedobjectIdin the query or remove the conversion for clarity.♻️ Proposed fix
- const objectId = isObjectId - ? new mongoose.Types.ObjectId(identifier) - : null; - if (type === "Event") { target = await Event.findOne({ $or: [ - ...(objectId ? [{ _id: identifier }] : []), + ...(isObjectId ? [{ _id: identifier }] : []), { event_id: identifier }, ], });Apply the same pattern for
Organizational_UnitandPositionbranches.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/announcementController.js` around lines 15 - 22, The variable objectId (created from isObjectId) is unused — update the Event.findOne (and the Organizational_Unit and Position branches) to use objectId when available instead of the raw identifier, e.g., replace occurrences of identifier in the {_id: ...} clause with objectId, or alternatively remove the objectId conversion entirely; ensure you reference the existing isObjectId check and objectId variable and adjust Event.findOne, Organizational_Unit branch, and Position branch consistently.backend/controllers/feedbackController.js (1)
135-234: 🏗️ Heavy liftN+1 query pattern and missing pagination may cause performance issues.
Each feedback record triggers additional DB queries for target data resolution (lines 143-227), leading to potentially hundreds of queries for a large feedback table. Combined with no pagination, this endpoint could be slow and resource-intensive.
Consider adding pagination and batching target lookups by type.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/controllers/feedbackController.js` around lines 135 - 234, The current per-item resolution in populatedFeedback (the feedback.map async block handling fb.target_type) causes N+1 DB queries and lacks pagination; change the controller to accept pagination params (e.g., page/limit or skip/limit) and fetch a limited feedback slice first, then batch-resolve targets by collecting ids per type and issuing single queries like User.find({_id: {$in: ids}}).select(...), Event.find({_id: {$in: ids}}).select(...), OrganizationalUnit.find({_id: {$in: ids}}).select(...), Position.find({_id: {$in: ids}}).select(...), build maps from _id to document, and then replace the per-item switch in populatedFeedback to look up target data from those maps (use the same symbols: populatedFeedback, feedback.map, target_type, targetData, fb.toObject) to assemble the final array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/controllers/achievementController.js`:
- Around line 82-115: In addAchievement, do not trust req.body.user_id for
ownership: validate and enforce that the achievement's user_id is taken from
req.user.id (or equivalent) unless the authenticated user has an admin role; if
req.user.role indicates admin, allow using req.body.user_id as an override,
otherwise ignore req.body.user_id and set user_id = req.user.id before creating
the Achievement and saving it. Ensure you reference addAchievement, req.user,
and any role-check helper (e.g., req.user.role or isAdmin) when implementing the
change and return 403 if a non-admin attempts to set a different user_id.
- Around line 26-42: In verifyAchievement, stop trusting req.body.verified_by
and instead set achievement.verified_by from the authenticated principal (e.g.
req.user.id or req.user._id) obtained from the request context; remove or ignore
the { verified_by } destructuring and any use of req.body for that field,
validate that req.user exists before assigning, and persist the change via
achievement.save() so the audit trails correctly reflect the authenticating
user.
In `@backend/controllers/authController.js`:
- Around line 179-181: Remove the debug logging that prints req.params (which
exposes the reset token). In the handler where you destructure const { id, token
} = req.params, delete the console.log(req.params) call; if you must log for
diagnostics, log only the id or a masked token (not the full token) and never
print req.params or token directly. Ensure no other debug logs in
authController.js output the token.
- Line 164: The console.log(link) call in authController.js is exposing the
password reset URL; remove this log statement and any other logs that print the
raw reset link. Instead, if you need auditing, log a non-sensitive event (e.g.,
"password reset link generated for userId=<id>" or "password reset email sent")
without including the link itself; locate the console.log(link) in the
password-reset generation/send flow and delete it or replace it with a safe,
non-sensitive log referencing user identifier or request id.
In `@backend/controllers/eventControllers.js`:
- Around line 665-671: The code currently overwrites request.requested_at inside
the status update block, losing the original creation timestamp; update the
handler that mutates the request (the block referencing request.status,
request.requested_at, and request.reviewed_by) to stop changing requested_at and
instead set a new reviewed_at timestamp (e.g., request.reviewed_at = new Date())
when changing status and assign request.reviewed_by as before, or simply remove
the assignment to request.requested_at so the schema-created Date.now value is
preserved.
- Around line 343-348: The current registration check only blocks events with
status "completed"; update the status guard in the registration handler in
eventControllers.js (the block that checks event.status) to also block
"cancelled" events—e.g., change the condition to check for both "completed" and
"cancelled" (or use an array includes check) so registrations are rejected for
either status and return the same 400 response.
In `@backend/controllers/feedbackController.js`:
- Around line 255-294: The controller currently assigns resolved_by from
req.body; change it to use the authenticated admin user (req.user._id) instead:
in the resolve handler where you set feedback.resolved_by (and any earlier
validation that reads resolved_by), replace uses of resolved_by from req.body
with req.user._id (ensure req.user exists and is set by your auth middleware),
keep Feedback.findById, feedback.is_resolved, feedback.resolved_at and
actions_taken logic unchanged, and update any related tests or type checks that
expect resolved_by to come from the body.
- Around line 18-28: The controller currently reads feedback_by from req.body,
allowing spoofing; remove feedback_by from the destructured fields pulled from
req.body and instead set feedback_by to the authenticated user's id
(req.user._id) when constructing/saving the Feedback (e.g., in the create/insert
call inside the feedback handler in feedbackController.js). Update any
validation that checks for feedback_by to rely on req.user._id and ensure the
saved document uses that value rather than the request body.
In `@backend/controllers/onboardingController.js`:
- Line 7: Remove the debug console.log(req.user) that prints the full user
object; locate the usage in onboardingController (the route handler that
accesses req.user) and either delete the console.log call or replace it with a
minimal, non-PII log (e.g., only log a user identifier or an anonymized token)
using your app's logger. Ensure you do not emit name/email/phone fields from
req.user into logs and update any tests or lints that expect the debug log to be
absent.
In `@backend/controllers/orgUnitController.js`:
- Around line 124-129: The catch block in the orgUnitController is returning the
raw error object in res.status(500).json (the variable error), which can leak
internal Mongoose/driver details; instead, log the full error server-side (e.g.,
logger.error or console.error) inside the catch and return a stable
client-facing payload such as { message: "Server error while fetching
organizational units" } or include a non-sensitive errorCode, removing the error
property from the JSON response; update the catch in the controller function
that currently uses res.status(500).json({... error ...}) to implement this
change.
- Around line 136-165: The validation currently dereferences contact_info.email
and returns a 400 after starting a mongoose session/transaction, which can leak
an open transaction; move payload validation for req.body (including checking
that contact_info exists before accessing contact_info.email) to happen before
calling mongoose.startSession()/session.startTransaction(), and only start the
session/transaction after validation passes; update any early returns to occur
without an active session and ensure any later error paths properly
abortTransaction() and endSession() when the session exists.
In `@backend/controllers/positionController.js`:
- Around line 90-140: Replace the non-atomic countDocuments()+save() with a
MongoDB transaction: start a mongoose session (e.g., const session = await
mongoose.startSession()), session.startTransaction(), perform the capacity check
using PositionHolder.countDocuments({ position_id, tenure_year
}).session(session) and if it’s still < position.position_count then save the
new PositionHolder via newPH.save({ session }), then await
session.commitTransaction() and session.endSession(); on failure
abortTransaction() and return the 400. Use the existing symbols PositionHolder,
position.position_count, tenure_year, newPH.save and ensure all reads/writes in
the block pass the session so the check-and-insert is atomic.
- Around line 17-31: The position creation handler currently allows missing or
invalid position_count which disables capacity checks in addPositionHolder;
update the validation in the position creation flow (the block that constructs
new Position and sets position_count) to require position_count be present and a
positive integer (or at least >=1), and return a 400 error if it is missing or
invalid so Position instances are always created with a valid position_count
used by addPositionHolder.
In `@backend/controllers/profileController.js`:
- Around line 97-113: The updateStudentProfile handler currently trusts userId
from req.body and updates any profile; add an authorization check in
updateStudentProfile to ensure req.user exists and either req.user.user_id (or
req.user.id depending on your auth payload) matches the requested userId or
req.user.role === 'admin' before proceeding; if the check fails, return a 403
response and do not call User.findOne/perform updates. Make this check early
(before User.findOne) and use the same identifier fields used in authentication
to avoid mismatches.
In `@backend/routes/achievements.js`:
- Around line 37-38: The route router.get("/:userId", isAuthenticated,
achievementController.getUserAchievements) currently permits any authenticated
user to fetch another user's achievements; add a self-or-admin authorization
check after isAuthenticated (e.g., middleware named isSelfOrAdmin or inline
check) that compares req.user.id (or req.user._id) to req.params.userId and
allows access if equal or if req.user.role === "admin", otherwise respond 403,
and wire that middleware into the route before
achievementController.getUserAchievements so only the user themself or an admin
can retrieve the achievements.
In `@backend/routes/feedbackRoutes.js`:
- Line 15: The GET /view-feedback route (router.get("/view-feedback",
feedbackController.viewFeedback)) currently exposes all feedback with populated
feedback_by user info without any auth; confirm intended public access and
either add the authentication/authorization middleware (e.g., isAuthenticated or
role-check middleware) to the router declaration or modify
feedbackController.viewFeedback to redact/populate only non-sensitive fields for
unauthenticated requests. Update the route to use something like
router.get("/view-feedback", isAuthenticated, roleCheck('admin'),
feedbackController.viewFeedback) for protected access or adjust the controller
to return anonymized feedback when req.user is absent.
In `@backend/routes/por.js`:
- Around line 3-6: The /current route currently exposes personal_info.email and
profile data via router.get("/current", porController.getCurrentPORs); protect
it by either adding authentication/authorization middleware to the route (e.g.,
router.get("/current", requireAuth, porController.getCurrentPORs) or whichever
middleware your app uses) and/or update the controller function getCurrentPORs
to strip sensitive fields (remove personal_info.email and any profile fields)
from the returned POR objects before sending the response; make the change in
the router to attach the auth middleware and/or in controllers/porController.js
inside getCurrentPORs to explicitly delete or omit those fields from the result.
---
Minor comments:
In `@backend/controllers/announcementController.js`:
- Around line 58-66: In the announcement creation flow in
announcementController.js the code allows non-"General" announcements to proceed
when targetIdentifier is missing, leaving target_id null; updateAnnouncement
enforces this requirement. Modify the create logic handling type and
targetIdentifier (the block using type, targetIdentifier, targetId and calling
findTargetId) to first validate that if type !== "General" then targetIdentifier
must be present and, if missing, return a 400 response with an explanatory
error; otherwise call findTargetId as before and keep the existing 404 behavior
when findTargetId returns falsy.
- Around line 120-127: The code constructs a RegExp from untrusted input
(search) and assigns it into filter.$or for title/content, which enables ReDoS;
fix by escaping regex metacharacters before calling new RegExp (e.g. add an
escapeRegex helper and call const safe = escapeRegex(search) then const regex =
new RegExp(safe, "i")), or use a safe text-search alternative, and replace the
existing new RegExp(search, "i") usage in the block that builds filter.$or
(title/content) with the escaped value.
In `@backend/controllers/authController.js`:
- Around line 211-258: The User.findOne call in resetPassword is executed
outside any try/catch and can throw (e.g., invalid ObjectId or DB error); wrap
the initial database lookup (the User.findOne in resetPassword) inside the
existing try/catch (or create a new try/catch around it) so any errors return an
appropriate HTTP response instead of crashing—ensure you still perform the user
existence check and proceed to jwt.verify, user.setPassword, and user.save
inside the safe block and return 400/500 as appropriate on lookup errors.
- Around line 175-209: The database lookup in verifyResetPasswordToken uses
User.findOne outside any try-catch so DB errors (e.g., connection issues or
invalid ObjectId) can crash the request; wrap the async operations —
specifically the User.findOne call and the subsequent jwt.verify usage in the
verifyResetPasswordToken function — in a try-catch block, return a 500 (or
appropriate) JSON error response when the DB call throws, and preserve the
existing 404 and token error responses inside that flow so all unexpected
exceptions are handled gracefully.
In `@backend/controllers/eventControllers.js`:
- Around line 66-80: The getEventById controller currently returns null with a
200 when Event.findById yields no result; update exports.getEventById to check
the retrieved event (from Event.findById(...).populate(...)) and if it's falsy
send res.status(404).json({ message: "Event not found." }) instead of
res.json(event), otherwise return the event as before; reference the
getEventById function and the Event.findById call to locate the change.
In `@backend/controllers/onboardingController.js`:
- Around line 18-22: Guard against Program being undefined before calling
.trim(): in the onboarding request handler (the function that builds
academic_info) validate required fields (Program, discipline, add_year) at the
top of the function and return a 400 response if any are missing, or use a safe
fallback when constructing academic_info (e.g., program: (Program ||
"").trim()). Update the code where academic_info is built to use the safe
expression or add the early validation check so Program.trim() cannot throw.
In `@backend/controllers/skillController.js`:
- Around line 27-40: The handler endorseUserSkill currently calls
UserSkill.findById(req.params.id) without validating the id, causing malformed
ObjectId errors to surface as 500; before calling Mongoose (in endorseUserSkill
and other handlers using findById/findByIdAndDelete), validate req.params.id
with mongoose.Types.ObjectId.isValid(id) and if invalid return
res.status(400).json({ message: "Invalid id" }); otherwise proceed to call
UserSkill.findById / UserSkill.findByIdAndDelete and keep the existing 404
handling for not-found documents.
In `@backend/routes/events.js`:
- Around line 57-62: The route currently calls authorizeRole with a string:
change router.patch(..., authorizeRole("PRESIDENT"), ...) to pass an array
instead (authorizeRole(["PRESIDENT"])) so the middleware’s
allowedRoles.includes(userRole) works correctly; update the call that registers
eventsController.updateRoomRequestStatus to use the array form consistent with
other routes using authorizeRole.
---
Nitpick comments:
In `@backend/controllers/announcementController.js`:
- Around line 158-163: The current conditional uses filter.type to decide
whether to call query.populate("target_id"), which causes target_id to remain
unpopulated when filter.type is absent or "All"; update the logic in
announcementController (where query and filter.type are used) to always call
query.populate("target_id") (i.e., populate unconditionally) so non-"General"
announcements in the returned result have their target_id populated, or
alternatively implement per-document population by checking each announcement's
type before accessing target_id if you prefer lazy population.
- Around line 15-22: The variable objectId (created from isObjectId) is unused —
update the Event.findOne (and the Organizational_Unit and Position branches) to
use objectId when available instead of the raw identifier, e.g., replace
occurrences of identifier in the {_id: ...} clause with objectId, or
alternatively remove the objectId conversion entirely; ensure you reference the
existing isObjectId check and objectId variable and adjust Event.findOne,
Organizational_Unit branch, and Position branch consistently.
In `@backend/controllers/eventControllers.js`:
- Around line 535-541: The query populates full User documents for participants
(Event.find -> .populate("participants")) which may expose personal_info; change
the populate call to explicitly select only needed fields (for example use
populate with a path of "participants" and select only "name" and "_id" or
otherwise exclude "personal_info") so participants do not return full User
personal data; update the populate invocation for "participants" (and any other
sensitive user-populates) to use the populate({ path: 'participants', select:
'name _id' }) pattern.
In `@backend/controllers/feedbackController.js`:
- Around line 135-234: The current per-item resolution in populatedFeedback (the
feedback.map async block handling fb.target_type) causes N+1 DB queries and
lacks pagination; change the controller to accept pagination params (e.g.,
page/limit or skip/limit) and fetch a limited feedback slice first, then
batch-resolve targets by collecting ids per type and issuing single queries like
User.find({_id: {$in: ids}}).select(...), Event.find({_id: {$in:
ids}}).select(...), OrganizationalUnit.find({_id: {$in: ids}}).select(...),
Position.find({_id: {$in: ids}}).select(...), build maps from _id to document,
and then replace the per-item switch in populatedFeedback to look up target data
from those maps (use the same symbols: populatedFeedback, feedback.map,
target_type, targetData, fb.toObject) to assemble the final array.
In `@backend/routes/events.js`:
- Around line 3-4: The top-level imports Event, User, OrganizationalUnit and the
uuidv4 import are unused in this router (they were moved to the controller);
remove the unused requires for Event, User, OrganizationalUnit and the uuidv4
import statement from events.js so only the router/controller imports remain,
and run lint/CI to ensure no remaining references to these symbols (Event, User,
OrganizationalUnit, uuidv4) in that file.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f46decac-8728-40e6-a408-a5023c6b519d
📒 Files selected for processing (22)
backend/controllers/achievementController.jsbackend/controllers/announcementController.jsbackend/controllers/authController.jsbackend/controllers/eventControllers.jsbackend/controllers/feedbackController.jsbackend/controllers/onboardingController.jsbackend/controllers/orgUnitController.jsbackend/controllers/porController.jsbackend/controllers/positionController.jsbackend/controllers/profileController.jsbackend/controllers/skillController.jsbackend/routes/achievements.jsbackend/routes/announcements.jsbackend/routes/auth.jsbackend/routes/events.jsbackend/routes/feedbackRoutes.jsbackend/routes/onboarding.jsbackend/routes/orgUnit.jsbackend/routes/por.jsbackend/routes/positionRoutes.jsbackend/routes/profile.jsbackend/routes/skillsRoutes.js
Comment on lines
+26
to
+42
| const verifyAchievement = async (req, res) => { | ||
| const { id } = req.params; | ||
| const { verified_by } = req.body; | ||
|
|
||
| try { | ||
| const achievement = await Achievement.findById(id); | ||
|
|
||
| if (!achievement) { | ||
| return res.status(404).json({ | ||
| message: "Achievement not found.", | ||
| }); | ||
| } | ||
|
|
||
| achievement.verified = true; | ||
| achievement.verified_by = verified_by; | ||
|
|
||
| await achievement.save(); |
Contributor
There was a problem hiding this comment.
Derive verified_by from the authenticated user, not the request body.
Line 28 lets the caller choose verified_by, so an admin can verify an achievement while attributing it to someone else. That breaks the audit trail. Set this field from the authenticated principal instead of trusting client input.
Suggested fix
const verifyAchievement = async (req, res) => {
const { id } = req.params;
- const { verified_by } = req.body;
+ const verified_by = req.user._id; // or req.user.user_id, matching the schema
try {
const achievement = await Achievement.findById(id);
@@
achievement.verified = true;
achievement.verified_by = verified_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 verifyAchievement = async (req, res) => { | |
| const { id } = req.params; | |
| const { verified_by } = req.body; | |
| try { | |
| const achievement = await Achievement.findById(id); | |
| if (!achievement) { | |
| return res.status(404).json({ | |
| message: "Achievement not found.", | |
| }); | |
| } | |
| achievement.verified = true; | |
| achievement.verified_by = verified_by; | |
| await achievement.save(); | |
| const verifyAchievement = async (req, res) => { | |
| const { id } = req.params; | |
| const verified_by = req.user._id; // or req.user.user_id, matching the schema | |
| try { | |
| const achievement = await Achievement.findById(id); | |
| if (!achievement) { | |
| return res.status(404).json({ | |
| message: "Achievement not found.", | |
| }); | |
| } | |
| achievement.verified = true; | |
| achievement.verified_by = verified_by; | |
| await achievement.save(); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/achievementController.js` around lines 26 - 42, In
verifyAchievement, stop trusting req.body.verified_by and instead set
achievement.verified_by from the authenticated principal (e.g. req.user.id or
req.user._id) obtained from the request context; remove or ignore the {
verified_by } destructuring and any use of req.body for that field, validate
that req.user exists before assigning, and persist the change via
achievement.save() so the audit trails correctly reflect the authenticating
user.
Comment on lines
+82
to
+115
| const addAchievement = async (req, res) => { | ||
| try { | ||
| const { | ||
| title, | ||
| description, | ||
| category, | ||
| type, | ||
| level, | ||
| date_achieved, | ||
| position, | ||
| certificate_url, | ||
| event_id, | ||
| user_id, | ||
| } = req.body; | ||
|
|
||
| if (!title || !category || !date_achieved || !user_id) { | ||
| return res.status(400).json({ | ||
| message: "Missing required fields", | ||
| }); | ||
| } | ||
|
|
||
| const achievement = new Achievement({ | ||
| achievement_id: uuidv4(), | ||
| user_id, | ||
| title, | ||
| description, | ||
| category, | ||
| type, | ||
| level, | ||
| date_achieved, | ||
| position, | ||
| certificate_url, | ||
| event_id: event_id || null, | ||
| }); |
Contributor
There was a problem hiding this comment.
Block cross-user achievement creation.
Line 94 accepts user_id from the body on an endpoint that only requires authentication. As written, any logged-in student can create achievements for another user. Enforce req.user ownership here and only allow admins to override the target user.
Suggested fix
const addAchievement = async (req, res) => {
try {
const {
@@
- user_id,
+ user_id,
} = req.body;
+
+ const effectiveUserId =
+ req.user.role === "admin" ? user_id : req.user._id; // match the schema field
- if (!title || !category || !date_achieved || !user_id) {
+ if (!title || !category || !date_achieved || !effectiveUserId) {
return res.status(400).json({
message: "Missing required fields",
});
}
@@
const achievement = new Achievement({
achievement_id: uuidv4(),
- user_id,
+ user_id: effectiveUserId,
title,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/achievementController.js` around lines 82 - 115, In
addAchievement, do not trust req.body.user_id for ownership: validate and
enforce that the achievement's user_id is taken from req.user.id (or equivalent)
unless the authenticated user has an admin role; if req.user.role indicates
admin, allow using req.body.user_id as an override, otherwise ignore
req.body.user_id and set user_id = req.user.id before creating the Achievement
and saving it. Ensure you reference addAchievement, req.user, and any role-check
helper (e.g., req.user.role or isAdmin) when implementing the change and return
403 if a non-admin attempts to set a different user_id.
| } | ||
| ); | ||
|
|
||
| console.log(link); |
Contributor
There was a problem hiding this comment.
Remove password reset link from logs.
Logging the password reset link exposes it to anyone with log access (ops team, log aggregation services, compromised logging infrastructure), allowing unauthorized password resets.
Proposed fix
- console.log(link);📝 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(link); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/authController.js` at line 164, The console.log(link)
call in authController.js is exposing the password reset URL; remove this log
statement and any other logs that print the raw reset link. Instead, if you need
auditing, log a non-sensitive event (e.g., "password reset link generated for
userId=<id>" or "password reset email sent") without including the link itself;
locate the console.log(link) in the password-reset generation/send flow and
delete it or replace it with a safe, non-sensitive log referencing user
identifier or request id.
Comment on lines
+179
to
+181
| const { id, token } = req.params; | ||
|
|
||
| console.log(req.params); |
Contributor
There was a problem hiding this comment.
Remove debug logging of request params containing reset token.
Logging req.params exposes the reset token in logs, creating the same security risk as logging the reset link.
Proposed fix
const { id, token } = req.params;
-
- console.log(req.params);📝 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, token } = req.params; | |
| console.log(req.params); | |
| const { id, token } = req.params; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/authController.js` around lines 179 - 181, Remove the
debug logging that prints req.params (which exposes the reset token). In the
handler where you destructure const { id, token } = req.params, delete the
console.log(req.params) call; if you must log for diagnostics, log only the id
or a masked token (not the full token) and never print req.params or token
directly. Ensure no other debug logs in authController.js output the token.
Comment on lines
+343
to
+348
| if (event.status === "completed") { | ||
| return res.status(400).json({ | ||
| message: | ||
| "Registration is closed for this event.", | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Registration should also block "cancelled" events.
Per the Event schema, status can be planned, ongoing, completed, or cancelled. Currently, only completed events block registration. Users could register for a cancelled event.
Proposed fix
- if (event.status === "completed") {
+ if (event.status === "completed" || event.status === "cancelled") {
return res.status(400).json({
message:
"Registration is closed for this event.",
});
}📝 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 (event.status === "completed") { | |
| return res.status(400).json({ | |
| message: | |
| "Registration is closed for this event.", | |
| }); | |
| } | |
| if (event.status === "completed" || event.status === "cancelled") { | |
| return res.status(400).json({ | |
| message: | |
| "Registration is closed for this event.", | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/eventControllers.js` around lines 343 - 348, The current
registration check only blocks events with status "completed"; update the status
guard in the registration handler in eventControllers.js (the block that checks
event.status) to also block "cancelled" events—e.g., change the condition to
check for both "completed" and "cancelled" (or use an array includes check) so
registrations are rejected for either status and return the same 400 response.
Comment on lines
+90
to
+140
| const existingCount = | ||
| await PositionHolder.countDocuments({ | ||
| position_id, | ||
| tenure_year, | ||
| }); | ||
|
|
||
| if (existingCount >= position.position_count) { | ||
| return res.status(400).json({ | ||
| message: `Maximum position holders (${position.position_count}) already appointed for the year ${tenure_year}.`, | ||
| }); | ||
| } | ||
|
|
||
| const newPH = new PositionHolder({ | ||
| por_id: uuidv4(), | ||
| user_id, | ||
| position_id, | ||
| tenure_year, | ||
|
|
||
| appointment_details: | ||
| appointment_details && | ||
| (appointment_details.appointed_by || | ||
| appointment_details.appointment_date) | ||
| ? appointment_details | ||
| : undefined, | ||
|
|
||
| performance_metrics: { | ||
| events_organized: | ||
| performance_metrics && | ||
| performance_metrics.events_organized !== undefined | ||
| ? performance_metrics.events_organized | ||
| : 0, | ||
|
|
||
| budget_utilized: | ||
| performance_metrics && | ||
| performance_metrics.budget_utilized !== undefined | ||
| ? performance_metrics.budget_utilized | ||
| : 0, | ||
|
|
||
| feedback: | ||
| performance_metrics && | ||
| performance_metrics.feedback | ||
| ? performance_metrics.feedback.trim() | ||
| : undefined, | ||
| }, | ||
|
|
||
| status, | ||
| }); | ||
|
|
||
| const saved = await newPH.save(); | ||
|
|
||
| return res.status(201).json(saved); |
Contributor
There was a problem hiding this comment.
Make the capacity check atomic.
The countDocuments() + save() sequence is race-prone. Two concurrent requests can both observe free capacity and both insert, exceeding position_count for the same tenure_year.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/positionController.js` around lines 90 - 140, Replace the
non-atomic countDocuments()+save() with a MongoDB transaction: start a mongoose
session (e.g., const session = await mongoose.startSession()),
session.startTransaction(), perform the capacity check using
PositionHolder.countDocuments({ position_id, tenure_year }).session(session) and
if it’s still < position.position_count then save the new PositionHolder via
newPH.save({ session }), then await session.commitTransaction() and
session.endSession(); on failure abortTransaction() and return the 400. Use the
existing symbols PositionHolder, position.position_count, tenure_year,
newPH.save and ensure all reads/writes in the block pass the session so the
check-and-insert is atomic.
Contributor
There was a problem hiding this comment.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
+97
to
+113
| exports.updateStudentProfile = async (req, res) => { | ||
| try { | ||
| const { userId, updatedDetails } = req.body; | ||
|
|
||
| if (!userId || !updatedDetails) { | ||
| return res | ||
| .status(400) | ||
| .json({ success: false, message: "Missing required fields" }); | ||
| } | ||
|
|
||
| const user = await User.findOne({ user_id: userId }); // <-- updated from ID_No | ||
|
|
||
| if (!user) { | ||
| return res | ||
| .status(404) | ||
| .json({ success: false, message: "Student not found" }); | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Missing authorization check allows any user to modify any profile.
updateStudentProfile accepts userId from the request body and updates that user's profile without verifying that the authenticated user (req.user) is authorized to modify it. Any authenticated user can modify any other user's profile data including email, phone, and personal information.
Add authorization to ensure users can only update their own profiles (or check for admin role).
Proposed fix
exports.updateStudentProfile = async (req, res) => {
try {
const { userId, updatedDetails } = req.body;
if (!userId || !updatedDetails) {
return res
.status(400)
.json({ success: false, message: "Missing required fields" });
}
+ // Authorization: ensure the authenticated user can only update their own profile
+ if (req.user.user_id !== userId) {
+ return res
+ .status(403)
+ .json({ success: false, message: "Not authorized to update this profile" });
+ }
+
const user = await User.findOne({ user_id: userId });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/controllers/profileController.js` around lines 97 - 113, The
updateStudentProfile handler currently trusts userId from req.body and updates
any profile; add an authorization check in updateStudentProfile to ensure
req.user exists and either req.user.user_id (or req.user.id depending on your
auth payload) matches the requested userId or req.user.role === 'admin' before
proceeding; if the check fails, return a 403 response and do not call
User.findOne/perform updates. Make this check early (before User.findOne) and
use the same identifier fields used in authentication to avoid mismatches.
Contributor
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Comment on lines
37
to
+38
| //get all user achievements (endorsed + unendorsed) (must be accessible by the user themselves and admins, so all users) | ||
| router.get("/:userId", isAuthenticated, async (req, res) => { | ||
| const userId = req.params.userId; | ||
| try { | ||
| const userAchievements = await Achievement.find({ user_id: userId }) | ||
| .populate("event_id", "title description") | ||
| .populate("verified_by", "personal_info.name username user_id"); | ||
| res.json(userAchievements); | ||
| } catch (err) { | ||
| console.error("Failed to get user Achievements:", err); | ||
| res.status(500).json({ message: "Failed to get user Achievements." }); | ||
| } | ||
| }); | ||
| router.get("/:userId", isAuthenticated, achievementController.getUserAchievements); |
Contributor
There was a problem hiding this comment.
Add self-or-admin authorization to this route.
Lines 37-38 currently allow any authenticated user to fetch another user's achievements by changing :userId. That contradicts the route contract in the comment and leaks student achievement history.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/achievements.js` around lines 37 - 38, The route
router.get("/:userId", isAuthenticated,
achievementController.getUserAchievements) currently permits any authenticated
user to fetch another user's achievements; add a self-or-admin authorization
check after isAuthenticated (e.g., middleware named isSelfOrAdmin or inline
check) that compares req.user.id (or req.user._id) to req.params.userId and
allows access if equal or if req.user.role === "admin", otherwise respond 403,
and wire that middleware into the route before
achievementController.getUserAchievements so only the user themself or an admin
can retrieve the achievements.
| res.status(500).json({ message: "Failed to retrieve feedback" }); | ||
| } | ||
| }); | ||
| router.get("/view-feedback", feedbackController.viewFeedback); |
Contributor
There was a problem hiding this comment.
/view-feedback is publicly accessible—verify this is intentional.
This endpoint returns all feedback with populated user information (feedback_by) without authentication. Even anonymous feedback will have the submitter's info populated. If feedback data is sensitive, consider adding isAuthenticated or role-based protection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/feedbackRoutes.js` at line 15, The GET /view-feedback route
(router.get("/view-feedback", feedbackController.viewFeedback)) currently
exposes all feedback with populated feedback_by user info without any auth;
confirm intended public access and either add the authentication/authorization
middleware (e.g., isAuthenticated or role-check middleware) to the router
declaration or modify feedbackController.viewFeedback to redact/populate only
non-sensitive fields for unauthenticated requests. Update the route to use
something like router.get("/view-feedback", isAuthenticated, roleCheck('admin'),
feedbackController.viewFeedback) for protected access or adjust the controller
to return anonymized feedback when req.user is absent.
Comment on lines
+3
to
+6
| const porController = require( | ||
| "../controllers/porController" | ||
| ); | ||
| router.get("/current", porController.getCurrentPORs); |
Contributor
There was a problem hiding this comment.
Protect /current before returning user contact data.
This route is public, but the controller response includes each holder’s personal_info.email and profile data. That exposes a member directory to anyone who can hit the endpoint.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/routes/por.js` around lines 3 - 6, The /current route currently
exposes personal_info.email and profile data via router.get("/current",
porController.getCurrentPORs); protect it by either adding
authentication/authorization middleware to the route (e.g.,
router.get("/current", requireAuth, porController.getCurrentPORs) or whichever
middleware your app uses) and/or update the controller function getCurrentPORs
to strip sensitive fields (remove personal_info.email and any profile fields)
from the returned POR objects before sending the response; make the change in
the router to attach the auth middleware and/or in controllers/porController.js
inside getCurrentPORs to explicitly delete or omit those fields from the result.
18 tasks
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
2 participants
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
Refactored backend route files by moving route logic into dedicated controller files and updating routes to use controller functions.
Changes
Added controller files for:
Refactored event routes to use controller functions.
Updated route files to delegate business logic to controllers.
Testing
Summary by CodeRabbit
Release Notes