Open
Conversation
|
@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. |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a comprehensive authentication system refactoring, moving from passport-local-mongoose plugins to explicit User schema management with support for both local and Google OAuth 2.0 strategies. It separates a monolithic schema.js into dedicated model files (User, Event, Achievement, Feedback, etc.), implements JWT-based middleware alongside Passport, adds a certificate batch system with multi-level approval workflows, and refactors frontend authentication logic to use hook-based context consumption. Dependencies are updated to use bcrypt, connect-mongo, cookie-parser, and zod validation. Changes
Sequence DiagramssequenceDiagram
actor User
participant Client as Frontend
participant Server as Backend
participant Passport as Passport.js
participant DB as MongoDB
participant Mailer as Email Service
User->>Client: Enter credentials (username, password)
Client->>Server: POST /api/auth/login
Server->>Passport: passport.authenticate("local")
Passport->>DB: Find User by username
DB-->>Passport: User document
Passport->>Passport: bcrypt compare password
alt Password valid
Passport-->>Server: User object
Server->>Server: req.login(user, callback)
Server->>DB: Update session store
DB-->>Server: Session created
Server-->>Client: {success: true, data: user}
Client->>Client: handleLogin(user)
Client-->>User: Redirect to /onboarding
else Password invalid
Passport-->>Server: Authentication failed
Server-->>Client: {success: false, error}
Client-->>User: Display error toast
end
sequenceDiagram
actor Coordinator as Club Coordinator
participant Client as Frontend
participant Server as Backend
participant AuthMW as JWT Middleware
participant Ctrl as Certificate Controller
participant DB as MongoDB
participant Models as Schema Models
Coordinator->>Client: Submit certificate batch (title, users, etc.)
Client->>Server: POST /api/certificate-batches
Server->>AuthMW: Verify JWT token from cookies
AuthMW->>DB: Decode and validate token
DB-->>AuthMW: Valid user_id
AuthMW-->>Server: Attach user to req
Server->>Ctrl: createBatch(req, res)
Ctrl->>DB: Fetch User by ID
DB-->>Ctrl: User document
Ctrl->>Ctrl: Verify role is CLUB_COORDINATOR
Ctrl->>DB: Fetch PositionHolder & Position
DB-->>Ctrl: Position details
Ctrl->>Ctrl: Validate batch schema (title, unit_id, users)
Ctrl->>DB: Fetch Organizational_Unit (Club)
DB-->>Ctrl: Unit document
Ctrl->>Ctrl: Verify unit is Club, belongs to Council
Ctrl->>DB: Find council GENSEC and President
DB-->>Ctrl: Approver users
Ctrl->>DB: Validate all user IDs exist
alt All users valid
Ctrl->>DB: Create CertificateBatch
DB-->>Ctrl: Batch created with status PendingL1
Ctrl-->>Server: {success: true, batch}
Server-->>Client: Batch ID & status
Client-->>Coordinator: Show success toast
else User validation fails
Ctrl-->>Server: {error: "Invalid user"}
Server-->>Client: Error response
Client-->>Coordinator: Display error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/auth.js (1)
208-226:user.setPassword()does not exist on the User model and will throw at runtime.The User schema does not have the
passport-local-mongooseplugin attached, so thesetPasswordmethod is unavailable. Line 218 will crash with "user.setPassword is not a function" when a password reset is attempted. The inconsistency is problematic—registerandloginboth usebcrypt, butreset-passwordrelies on a non-existent method.Use
bcrypt.hash()to hash the password, consistent with theregisterroute pattern:Suggested fix
try { jwt.verify(token, secret); - user.setPassword(password, async (error) => { - if (error) { - return res.status(500).json({ message: "Error resetting password" }); - } - await user.save(); - return res - .status(200) - .json({ message: "Password has been reset successfully" }); - }); + const hashed = await bcrypt.hash(password, Number(process.env.SALT)); + user.password = hashed; + await user.save(); + return res + .status(200) + .json({ message: "Password has been reset successfully" }); } catch (error) { console.log(error); return res.status(400).json({ message: "Invalid or expired token" }); }
🤖 Fix all issues with AI agents
In `@backend/config/passportConfig.js`:
- Around line 1-2: The import for GoogleStrategy is wrong: change the require
for GoogleStrategy so it references the Strategy property from the
passport-google-oauth20 package (so that new GoogleStrategy(...) is a valid
constructor); locate the GoogleStrategy import statement (currently const
GoogleStrategy = require("passport-google-oauth20")) and update it to pull
.Strategy to avoid the "not a constructor" runtime error when instantiating
GoogleStrategy in passport configuration.
- Around line 54-62: Change passport.serializeUser to store only the user's
unique identifier (e.g., user._id) instead of the whole user object, and adjust
passport.deserializeUser to accept that id (e.g., userId) and call
User.findById(userId) to load the full user; update references to
passport.serializeUser, passport.deserializeUser, and User.findById (remove
reliance on userKey._id) so sessions contain only the ID and deserialization
fetches the user record.
- Around line 14-21: The verify callback for the Google Passport strategy
accesses profile.emails[0].value without checking for existence; add a defensive
guard similar to the profile.photos check: ensure profile.emails is an array and
has at least one entry before reading .value (e.g., if (!profile.emails ||
profile.emails.length === 0) return done(null, false, { message: "Google profile
has no email" })), then use profile.emails[0].value for isIITBhilaiEmail and
logging; update all usages in the async (accessToken, refreshToken, profile,
done) => callback to avoid crashes when emails are missing.
In `@backend/routes/auth.js`:
- Around line 70-101: Remove trusting client-supplied role: stop extracting role
from req.body and do not pass it into User.create; instead determine role
server-side (e.g., set a default like "user" or map/whitelist based on
authenticated context) before creating the user. Update the destructuring that
currently includes role (from req.body) and replace the role passed to
User.create with the server-controlled value (not the request value) and, if you
need role assignment rules, implement a small whitelist/mapping in the route
handler to derive role securely.
- Around line 44-52: Fix the response syntax and guard against non-local
accounts before comparing passwords: replace the incorrect
res.json(401).json(...) calls with res.status(401).json(...), and before calling
bcrypt.compare(password, user.password) (after User.findOne({ username })),
ensure the account is a local/password account—e.g., check for a local strategy
flag (user.authStrategy === 'local') or at minimum that user.password exists; if
not, return res.status(401).json({ message: "Invalid user credentials" })
instead of attempting bcrypt.compare. Use the existing User.findOne, username,
bcrypt.compare and res.status symbols to locate where to add the guard and
change the response call.
- Around line 44-52: The error responses in the authentication flow use
res.json(401).json(...) which incorrectly sets a 200 status; update each
occurrence to use res.status(401).json(...) instead — specifically change the
early-return after User.findOne (the block using User.findOne({ username }) and
returning on missing user) and the password check block using
bcrypt.compare(password, user.password), plus any other similar occurrences
around the authentication routes in auth.js, so the responses call
res.status(401).json({ message: "Invalid user credentials" }) to correctly set
the HTTP 401 status.
- Around line 95-107: The registration response returns the created Mongoose
document `newUser` which includes the hashed `password`; before calling
`res.json` remove or omit the password field (e.g., convert `newUser` to a plain
object and delete the `password` property or re-query with a projection) so that
`res.json({ message: "Registered Successfully", user: ... })` never contains the
hashed password; update the code around `User.create`/`newUser` and the
`res.json` call to return the sanitized user object instead.
In `@backend/utils/validate.js`:
- Line 1: The module backend/utils/validate.js requires the zod package (const
zod = require("zod")), but zod is missing from backend/package.json; add "zod"
to the dependencies in backend/package.json (e.g., use npm install --save zod or
add the dependency entry) and run the install so the require("zod") in
validate.js resolves at runtime.
🧹 Nitpick comments (3)
backend/utils/validate.js (1)
3-14: DRY the IIT Bhilai email regex to avoid drift.Lines 3-13 duplicate the same regex (and another check exists in
backend/utils/isIITBhilaiEmail). Extract a shared constant/helper so all auth entry points stay consistent.♻️ Proposed refactor
+const IITB_EMAIL_REGEX = /^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i; + const loginValidate = zod.object({ - username: zod.string().regex(/^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i), + username: zod.string().regex(IITB_EMAIL_REGEX), password: zod.string().min(8), }); const registerValidate = zod.object({ - username: zod.string().regex(/^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i), + username: zod.string().regex(IITB_EMAIL_REGEX), password: zod.string().min(8), user_id: zod.string().min(2), name: zod.string().min(5), role: zod.string().min(5), });backend/config/db.js (1)
17-25: Return the close promise so shutdown can await completion.Line 17 doesn’t return/await the close operation, so callers can’t reliably wait for cleanup.
♻️ Suggested refactor
-const closeDB = () => { - mongoose.connection - .close() - .then(() => { - console.log("MongoDB connection closed"); - }) - .catch((err) => { - console.error("Error while closing MongoDB connection:", err); - }); -}; +const closeDB = async () => { + try { + await mongoose.connection.close(); + console.log("MongoDB connection closed"); + } catch (err) { + console.error("Error while closing MongoDB connection:", err); + } +};backend/models/schema.js (1)
22-28: Consider addingselect: falseto prevent accidental password hash exposure.The
minLength: 8option is correctly spelled and fully supported in Mongoose v7.6.8 (bothminLengthandminlengthare valid). However, addselect: falseto ensure password hashes are not returned in default queries.🛠️ Suggested update
password: { type: String, required: function () { return this.strategy === "local"; }, minLength: 8, + select: false, },
…batches for clubs
…batches for clubs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/models/passportConfig.js (3)
31-43:⚠️ Potential issue | 🔴 Critical
role: "STUDENT"is set but theUserschema has norolefield — this value will be silently dropped.The
userSchema.jsmodel does not define arolefield. With Mongoose's default strict mode, this property will be discarded on save. If roles are needed for authorization, add the field to the schema.#!/bin/bash # Verify that the User schema does not have a role field rg -n "role" backend/models/userSchema.js
55-57:⚠️ Potential issue | 🟠 MajorSerializing the entire user object into the session is inefficient and leaks the password hash.
serializeUsershould store only the user ID. The full document (including the hashed password) ends up in the session store.Proposed fix
passport.serializeUser((user, done) => { - done(null, user); + done(null, user._id); }); -passport.deserializeUser(async (userKey, done) => { +passport.deserializeUser(async (id, done) => { try { - let user = await User.findById(userKey._id); + let user = await User.findById(id); done(null, user); } catch (err) { done(err); } });
17-18:⚠️ Potential issue | 🟡 MinorLogging user email on OAuth rejection is a PII/compliance concern.
Consider logging a sanitized identifier or omitting the email entirely.
Proposed fix
- console.log("Google OAuth blocked for: ", profile.emails[0].value); + console.log("Google OAuth blocked: non-institutional email domain");backend/routes/auth.js (1)
224-247:⚠️ Potential issue | 🔴 Critical
user.setPassword()will fail at runtime — passport-local-mongoose was removed.The
/reset-passwordPOST handler at line 234 callsuser.setPassword(password, callback), which is apassport-local-mongooseplugin method that no longer exists. This will throw aTypeError: user.setPassword is not a functionat runtime.Replace this with direct password assignment and save, relying on the pre-save hook at
backend/models/userSchema.js:97-100to handle bcrypt hashing:Proposed fix
try { jwt.verify(token, secret); - user.setPassword(password, async (error) => { - if (error) { - return res.status(500).json({ message: "Error resetting password" }); - } - await user.save(); - return res - .status(200) - .json({ message: "Password has been reset successfully" }); - }); + user.password = password; + await user.save(); + return res + .status(200) + .json({ message: "Password has been reset successfully" }); } catch (error) {
🤖 Fix all issues with AI agents
In `@backend/models/organizationSchema.js`:
- Around line 70-78: The organizationSchema currently defines created_at and
updated_at with default: Date.now so updated_at won't change on document
updates; instead remove those manual fields and enable Mongoose's automatic
timestamps by adding the timestamps: true option when defining
organizationSchema (matching userSchema and positionHolderSchema), or
alternatively implement a pre('save') / pre('findOneAndUpdate') hook on
organizationSchema to set updated_at = Date.now() on modifications; update any
code that referenced the manual created_at/updated_at fields if their names
change (e.g., to createdAt/updatedAt) to match Mongoose's timestamp naming if
you choose timestamps: true.
In `@backend/models/userSchema.js`:
- Around line 97-100: The pre-save hook userSchema.pre currently passes
Number(process.env.SALT) into bcrypt.hash which becomes NaN if SALT is
undefined; update the hook to validate/parses the env value and provide a safe
default (e.g., 10) before calling bcrypt.hash, or throw a clear error at startup
if SALT must be provided; specifically ensure the value passed to bcrypt.hash is
a finite integer (use parsing + fallback) and add a clear error or default at
initialization so bcrypt.hash never receives NaN.
- Around line 31-39: personal_info.email currently lacks a uniqueness constraint
and can diverge from username via the profile update endpoint; add a uniqueness
index to the Mongoose schema (set personal_info.email: { type: String, required:
true, unique: true }) in userSchema.js and also add a schema-level validation or
pre('save')/pre('findOneAndUpdate') hook that enforces personal_info.email ===
username (or rejects updates that attempt to change email independently) so the
profile update handler cannot create duplicates—also update the profile update
logic to validate/reject mismatched email changes to keep lookups consistent.
In `@backend/routes/auth.js`:
- Around line 57-67: The current access token created by jwt.sign(payload,
secretKey, { expiresIn: "5m" }) and set in res.cookie("token", token, { maxAge:
5 * 60 * 1000 }) is too short-lived; either increase the session duration by
updating the expiresIn and corresponding maxAge to a realistic value (e.g.,
hours/days) or implement a refresh-token flow: issue a short-lived access token
via jwt.sign and a long-lived refresh token (stored as an HttpOnly cookie), add
a /refreshToken endpoint that validates and rotates the refresh token
(persisting it or its identifier server-side, e.g., in the User model), and
update jwt-signing and cookie logic in the login handler (the payload creation
in this file) to support issuing both tokens and renewing access tokens when
/refreshToken is called.
- Line 120: Remove the insecure console.log(newUser) that prints the entire user
document (including hashed password); instead either delete the log entirely or
replace it with a safe log that only outputs non-sensitive identifiers (e.g.,
newUser.id or newUser.email) or a simple confirmation message. Locate the
statement referencing newUser in the user creation flow (the console.log call)
and remove or change it so no password or full document is emitted to server
logs.
- Around line 48-50: The route handler incorrectly continues executing after
sending the 401 response for Google-authenticated users; add an immediate return
after res.status(401).json({ message: "Use Google sign-in for this acount." })
so execution does not fall through to bcrypt.compare(password, user.password)
(and avoid calling bcrypt.compare when user.password is undefined). Ensure the
early-return happens in the same conditional that checks user.strategy !==
"local" || !user.password.
🧹 Nitpick comments (7)
backend/package.json (1)
56-58: Consider removing unusedpassport-localandpassport-local-mongoosedependencies.The LocalStrategy import is commented out in
passportConfig.js, and the PR replaces Passport-based local auth with manual bcrypt+JWT. These packages appear to be dead weight now.backend/models/positionSchema.js (1)
1-56: Inconsistent timestamp approach across models.This schema manually defines
created_atwithoutupdated_at, whileuserSchema.jsandpositionHolderSchema.jsuse Mongoose's{ timestamps: true }. Consider standardizing on one approach across all models for consistency.backend/models/feedbackSchema.js (2)
14-22:target_idlacks areforrefPath—populate()won't work automatically.If you intend to populate
target_idbased ontarget_type, use Mongoose'srefPathto enable dynamic references:Proposed fix using refPath
target_id: { type: mongoose.Schema.Types.ObjectId, - //required: true, - // We'll dynamically interpret this field based on target_type + refPath: "target_type", }, target_type: { type: String, required: true, + enum: ["Event", "Organizational_Unit", "Position"], // constrain to valid model names },
28-32: Remove commented-outcategorycode.If this is no longer needed, remove it to keep the schema clean. It can always be retrieved from version control.
backend/routes/auth.js (2)
5-5: Remove commented-out import.Dead code — if
isIITBhilaiEmailis no longer used in this file, remove the comment entirely.🧹 Proposed fix
-//const isIITBhilaiEmail = require("../utils/isIITBhilaiEmail");
78-88:roleis hardcoded but still validated byregisterValidate— consider removing it from validation.
roleis hardcoded to"STUDENT"on line 81 (good — addresses the previous privilege-escalation concern), but it's still passed toregisterValidate.safeParse()which validatesrole: zod.string().min(5). This validation of a server-controlled constant is unnecessary. Consider removingrolefrom the Zod schema or from the safeParse call to keep the validation schema aligned with actual user input.backend/models/eventSchema.js (1)
107-115:updated_atwon't auto-update on document modifications.
default: Date.nowonly sets the value on creation. On subsequent updates,updated_atwill remain stale unless manually set. Consider using Mongoose's built-intimestampsoption which automatically manages bothcreatedAtandupdatedAt:♻️ Proposed refactor
Remove the manual
created_atandupdated_atfields and addtimestamps: trueto the schema options:- created_at: { - type: Date, - default: Date.now, - }, - updated_at: { - type: Date, - default: Date.now, - }, -}); +}); + +eventSchema.set("timestamps", true);Or pass it as a schema option:
const eventSchema = new mongoose.Schema({ /* ... */ }, { timestamps: true });Note: This renames the fields to
createdAt/updatedAt(camelCase). If you need snake_case, you can configure it:{ timestamps: { createdAt: 'created_at', updatedAt: 'updated_at' } }
…cal authentication
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
3 similar comments
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
backend/routes/onboarding.js (2)
11-11:⚠️ Potential issue | 🟠 MajorAvoid logging the full
req.userobject — it may contain PII.
console.log(req.user)can emit emails, names, and other personal information into logs. Replace with a scoped log (e.g., only logreq.user._id) or remove it entirely.Proposed fix
- console.log(req.user); + console.log("Onboarding request for user:", req.user._id);
33-36:⚠️ Potential issue | 🟠 MajorRaw
errorobject is leaked to the client.Passing the caught
errordirectly in the JSON response ({ message: "Onboarding failed", error }) can expose stack traces and internal details. Return only a safe message and log the full error server-side.Proposed fix
console.error("Onboarding failed:", error); - res.status(500).json({ message: "Onboarding failed", error }); + res.status(500).json({ message: "Onboarding failed" });backend/routes/feedbackRoutes.js (1)
52-52:⚠️ Potential issue | 🟡 MinorPre-existing typo:
"ture"should be"true".
is_anonymous === "ture"will never match a client sending the string"true". This appears to be a pre-existing bug, but since this area is being touched, it's worth fixing now.Proposed fix
- is_anonymous: is_anonymous === "ture" || is_anonymous === true, + is_anonymous: is_anonymous === "true" || is_anonymous === true,backend/routes/profile.js (1)
110-111:⚠️ Potential issue | 🟠 MajorRemove or redact debug logging of user-supplied data.
console.log("Received userId:", userId)andconsole.log("Received updatedDetails:", updatedDetails)can leak PII (names, emails, phone numbers, dates of birth, social links) to server logs in production.🛡️ Proposed fix
- console.log("Received userId:", userId); - console.log("Received updatedDetails:", updatedDetails);backend/routes/auth.js (1)
232-242:⚠️ Potential issue | 🔴 Critical
user.setPassword()is apassport-local-mongoosemethod that was removed in this PR.This PR removed
passport-local-mongoosefrom the project and switched to manual bcrypt-based authentication. ThesetPasswordmethod no longer exists on the User model, so the password-reset route will throw aTypeErrorat runtime. Replace it with bcrypt hashing and a direct save.🐛 Proposed fix
jwt.verify(token, secret); - user.setPassword(password, async (error) => { - if (error) { - return res.status(500).json({ message: "Error resetting password" }); - } - await user.save(); - return res - .status(200) - .json({ message: "Password has been reset successfully" }); - }); + user.password = password; // pre-save hook will hash it + await user.save(); + return res + .status(200) + .json({ message: "Password has been reset successfully" });backend/routes/announcements.js (2)
103-105:⚠️ Potential issue | 🟠 MajorUser-supplied
searchis passed directly tonew RegExp()— ReDoS risk.An attacker can craft a search string with pathological regex patterns (e.g.,
(a+)+$) that cause catastrophic backtracking. Escape special regex characters before constructing theRegExp, or use MongoDB's$textindex for search.🛡️ Proposed fix — escape regex special characters
+ 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 }]; }
4-9:⚠️ Potential issue | 🔴 CriticalFix broken imports:
Event,OrganizationalUnit, andPositionare not exported fromschema.jsand will beundefinedat runtime.These models were moved to separate files but the imports here were not updated. The
findTargetIdfunction will crash on line 18, 25, or 32 when calling.findOne()on undefined models.Fix: Update imports to use the standalone model files
-const { - Announcement, - Event, - OrganizationalUnit, - Position, -} = require("../models/schema"); +const { Announcement } = require("../models/schema"); +const Event = require("../models/eventSchema"); +const OrganizationalUnit = require("../models/organizationSchema"); +const Position = require("../models/positionSchema");backend/index.js (2)
68-70:⚠️ Potential issue | 🟠 MajorDuplicate route mount for announcements.
/api/announcementsis mounted twice (lines 68 and 70), which meansannouncementRouteshandlers will be registered on the same path twice. This can cause handlers to fire twice or produce unexpected behavior.Proposed fix
app.use("/api/announcements", announcementRoutes); app.use("/api/dashboard", dashboardRoutes); -app.use("/api/announcements", announcementRoutes); app.use("/api/analytics", analyticsRoutes);
40-40:⚠️ Potential issue | 🟠 MajorHardcoded session secret.
"keyboard cat"is a well-known placeholder. This should be loaded from an environment variable (e.g.process.env.SESSION_SECRET) to avoid session forgery in production.
🤖 Fix all issues with AI agents
In `@backend/controllers/certificateController.js`:
- Around line 10-148: The createBatch handler currently has no try/catch so any
DB or runtime error will crash the request; wrap the entire async body of
createBatch (all awaits like User.findById, PositionHolder.findOne,
OrganizationalUnit.findById, Promise.all user checks, CertificateBatch.create,
etc.) in a try/catch (or use your existing async error-wrapper middleware) and
in the catch log the error and return res.status(500).json({ message: "Internal
server error", error: /* brief error message or id */ }) to ensure unhandled
rejections are converted to proper HTTP 500 responses.
- Around line 116-128: The validation logic inside the users.map is wrong
because zodObjectId.safeParse(uid) returns an object, so change the check to
inspect validation.success; in the block that builds userChecks (where users.map
calls zodObjectId.safeParse), replace the condition that uses !validation with
!validation.success so invalid IDs return { uid, ok: false, reason: "Invalid ID"
} and subsequent lookups in User.findById only run for successful validations.
- Around line 44-48: The code logs position._id before verifying the result of
Position.findById, which can cause a TypeError if position is null; update the
handler in certificateController.js so you first check the returned value from
Position.findById(positionHolder.position_id) (i.e., if (!position) return
res.status(403).json(...)) and only after that call console.log(position._id) or
otherwise safely access properties of position.
- Around line 18-20: The authorization guard currently lets users with no role
pass because it checks user.role && user.role !== "CLUB_COORDINATOR"; change it
to explicitly deny when role is missing or not the coordinator, e.g. replace the
condition with if (!user.role || user.role !== "CLUB_COORDINATOR") and keep the
existing response using res.status(403).json({ message: "Not authorized to
perform the task" }); so only users whose user.role === "CLUB_COORDINATOR" are
allowed.
- Around line 88-112: The code dereferences query results before null-checking,
causing crashes: after calling User.findOne for gensecObj, Position.findOne for
presidentPosition, PositionHolder.findOne for presidentHolder, and User.findById
for presidentObj, add immediate null/undefined checks and return the 500
response if any are missing (use the same error messages), and remove or move
any console.log calls that access properties like ._id or .user_id until after
those checks; specifically update the logic around gensecObj, presidentPosition,
presidentHolder, and presidentObj to validate each result right after its query
(and only then access ._id or .user_id).
In `@backend/index.js`:
- Around line 29-31: The current connectDB() call silently continues on
failures; update either the connectDB implementation in config/db.js to re-throw
the caught error or call process.exit(1) inside its catch block, or change the
startup flow in backend/index.js to await connectDB() and wrap it in a try/catch
that logs the error and exits (e.g., catch(err) { processLogger.error(..., err);
process.exit(1); }). Ensure you reference the connectDB function (connectDB and
the connectDB() call) and the catch block in config/db.js so the process fails
fast when DB connection fails.
In `@backend/middlewares/isAuthenticated.js`:
- Around line 39-42: The routes import is receiving the entire exported object
instead of the middleware function: update the require in auth.js to pull the
named isAuthenticated export (so the value passed to router.get("/fetchAuth",
isAuthenticated, ...) is the middleware function, not the object) and remove the
large commented-out block in isAuthenticated.js; ensure the module still exports
the named symbols isAuthenticated and jwtIsAuthenticated so other files can
destructure them.
In `@backend/models/certificateSchema.js`:
- Around line 6-9: The ref string on the unit_id field in the certificate schema
is misspelled as "Oraganizational_Unit" causing populate('unit_id') to fail;
update the unit_id schema definition (the ref value) in certificateSchema.js to
the correct registered model name "Organizational_Unit" so it matches the model
created in organizationSchema.js.
- Around line 62-68: The schema-level unique on certificateId causes MongoDB to
block multiple documents with null/undefined (so only one Pending/Rejected
certificate is allowed); remove unique: true from the certificateId field in the
schema and rely on the existing partial index that enforces uniqueness only for
approved certificates (or if you prefer the alternative, replace unique: true
with { unique: true, sparse: true }); update/verify the partial index creation
logic (the index that targets certificateId when status === "Approved") to
ensure uniqueness is enforced at the index level instead of the schema.
In `@backend/models/schema.js`:
- Around line 110-114: schema.js now only exports UserSkill, Skill, Announcement
which breaks many modules that still destructure models like User, Event,
OrganizationalUnit, Position, PositionHolder, Achievement, Feedback; update each
affected import (e.g., backend/routes/announcements.js, profile.js, orgUnit.js,
events.js, onboarding.js, feedbackRoutes.js, positionRoutes.js and controllers
dashboardController.js, certificateController.js, analyticsController.js,
eventControllers.js, middleware isEventContact.js) to import the missing models
directly from their actual schema modules (e.g., userSchema.js for User,
eventSchema.js for Event, organizationSchema.js for OrganizationalUnit,
positionSchema.js for Position/PositionHolder, feedbackSchema.js for Feedback,
achievementSchema.js for Achievement) and adjust the destructuring names
accordingly, or alternatively re-export those models from schema.js if you
intend a single entrypoint; ensure no file destructures undefined symbols after
the change.
In `@backend/package.json`:
- Around line 34-37: The package.json contains unresolved Git merge conflict
markers around the body-parser dependency; remove the conflict markers (<<<<<<<,
=======, >>>>>>>) and delete the "body-parser" entry so package.json becomes
valid JSON; ensure the remaining JSON is properly comma-separated and valid (no
trailing commas) and that nothing else in package.json references body-parser
(index.js uses express.json()), so run npm install to verify.
In `@backend/routes/analytics.js`:
- Around line 25-30: The route registers middleware in the wrong order so
authorizeRole runs before isAuthenticated; update the router.get call that uses
router.get("/club-coordinator", authorizeRole(["CLUB_COORDINATOR"]),
isAuthenticated, controller.getClubCoordinatorAnalytics) to call isAuthenticated
before authorizeRole so req.user is populated (i.e., use
router.get("/club-coordinator", isAuthenticated,
authorizeRole(["CLUB_COORDINATOR"]), controller.getClubCoordinatorAnalytics));
keep controller.getClubCoordinatorAnalytics unchanged.
In `@backend/routes/auth.js`:
- Line 13: The import in backend/routes/auth.js is grabbing the whole exports
object instead of the middleware function; replace the default-style require
with a destructured import so isAuthenticated refers to the function exported by
the module (e.g., change the require to const { isAuthenticated } =
require("../middlewares/isAuthenticated")); ensure any other references to
jwtIsAuthenticated are similarly imported via destructuring if needed so Express
receives actual middleware functions when mounting routes like /fetchAuth.
- Line 64: The jwt.sign call uses an undefined secretKey which will throw a
ReferenceError at runtime; declare and read the secret from an environment
variable (e.g., process.env.JWT_SECRET) near the top of the file and use that
value for secretKey in the token creation (update the jwt.sign call that uses
secretKey). Also add a small guard in the auth initialization (or before issuing
tokens) to throw/log a clear error if the JWT secret is missing so the login
route fails fast with a helpful message.
In `@backend/routes/feedbackRoutes.js`:
- Around line 188-211: The route handler in feedbackRoutes.js is taking
resolved_by from req.body and logging req.body; change it to derive the resolver
from the authenticated user (use req.user._id) and remove or restrict the
console.log of the full body to avoid sensitive data exposure. Specifically, in
the async handler where feedbackId, actions_taken, and resolved_by are read and
where you set feedback.resolved_by, stop reading resolved_by from req.body and
instead set feedback.resolved_by = req.user._id (ensure req.user exists and
return 401 if not authenticated); also remove the console.log(req.body) (or
replace with a scoped debug log that excludes sensitive fields) and keep the
check for actions_taken as-is.
In `@backend/routes/profile.js`:
- Line 87: Replace the incorrect reference to the uninitialized local binding by
calling the imported model: change the call using user.findOne(...) to
User.findOne(...), and ensure you don't shadow the imported User with a local
variable named user (e.g., keep const user = await User.findOne({ user_id: ID_No
}); so the imported symbol User is used to query and the local variable user
holds the result).
In `@backend/utils/batchValidate.js`:
- Line 3: zodObjectId currently uses a regex that allows non-hex characters;
update the pattern used in the zodObjectId constant (zodObjectId) to only allow
24 hex characters by replacing the character class with the correct hex range
and keep the same error message or adjust it to "Invalid ObjectId" — i.e., use a
regex that enforces /^[0-9a-fA-F]{24}$/ so validations only accept valid MongoDB
ObjectId hex strings.
🟡 Minor comments (5)
backend/middlewares/isAuthenticated.js-29-36 (1)
29-36:⚠️ Potential issue | 🟡 Minor
jwtIsAuthenticateddoes not propagate the error reason on token verification failure.The
catchblock swallows the actual JWT error (e.g.,TokenExpiredErrorvsJsonWebTokenError). Consider loggingerr.nameorerr.messageat debug level for observability.Also, note that
req.useris set to the raw decoded payload. If downstream code expects a Mongoose document (e.g.,req.user._idas an ObjectId), this could cause subtle mismatches sincejwt.verifyreturns a plain object with string fields.backend/utils/batchValidate.js-6-6 (1)
6-6:⚠️ Potential issue | 🟡 MinorMisleading error message for
min(5).
min(5)enforces a minimum length of 5, but the message says "Title is required," which better suits an emptiness check. Consider something like"Title must be at least 5 characters".backend/utils/batchValidate.js-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorTypo: "Atleast" → "At least".
✏️ Fix
- users: zod.array(zodObjectId).min(1, "Atleast 1 user must be associated."), + users: zod.array(zodObjectId).min(1, "At least 1 user must be associated."),backend/models/organizationSchema.js-15-19 (1)
15-19:⚠️ Potential issue | 🟡 MinorInconsistent casing in
typeenum values.
"Council","Club","Committee"use PascalCase while"independent_position"uses snake_case. This can cause subtle bugs when comparing/filtering by type. Consider normalizing to a single convention.backend/controllers/certificateController.js-15-15 (1)
15-15:⚠️ Potential issue | 🟡 MinorTypo:
"messge"→"message".Proposed fix
- return res.status(404).json({ messge: "Invalid data (User not found)" }); + return res.status(404).json({ message: "Invalid data (User not found)" });
🧹 Nitpick comments (8)
backend/middlewares/isAuthenticated.js (1)
14-22: Remove commented-out code.There are three blocks of dead commented-out code: the Bearer token alternative (Lines 15–22), the debug log (Line 32), and a large block of leftover controller logic (Lines 44–84). These add noise and should be removed — version control preserves history.
Also applies to: 32-32, 44-84
backend/routes/events.js (1)
442-465: Verbose debug logging should not ship to production.Lines 442–465 log the full event object (before and after update) with
JSON.stringify. This can leak sensitive data to logs and degrades performance on large documents. These appear to be temporary debug aids — consider removing them or gating behind a debug flag/log level.package.json (1)
1-6: Remove unused dependencies from rootpackage.json.The root
package.jsoncontains onlypassport-localandpassport-local-mongoose, which are unused. There are no root-level JavaScript files, no monorepo workspaces, and no scripts that reference these packages. Since the backend uses JWT/bcrypt for auth (not Passport-Local), these dependencies are unnecessary and should be removed. Either delete the rootpackage.jsonentirely if it serves no other purpose, or confirm its intended use if this is part of a planned monorepo setup.backend/utils/authValidate.js (1)
3-6: Extract shared email regex to a constant to reduce duplication.The IIT Bhilai email regex is duplicated across
loginValidateandregisterValidate. Extract it to a shared constant.Proposed refactor
const zod = require("zod"); + +const iitbhilaiEmailRegex = /^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i; const loginValidate = zod.object({ - username: zod.string().regex(/^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i), + username: zod.string().regex(iitbhilaiEmailRegex), password: zod.string().min(8), }); const registerValidate = zod.object({ - username: zod.string().regex(/^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$/i), + username: zod.string().regex(iitbhilaiEmailRegex), password: zod.string().min(8),Also applies to: 8-10
backend/package.json (1)
56-57:nodemonandnpmshould not be runtime dependencies.
nodemon(line 56) is a dev tool and belongs indevDependencies.npm(line 57) is the package manager itself and generally should not be listed as a dependency at all. These inflate the production install unnecessarily.Proposed fix
"morgan": "^1.10.0", "multer": "^2.0.1", "nodemailer": "^7.0.3", - "nodemon": "^3.1.10", - "npm": "^10.2.0", "passport": "^0.7.0",Then add
nodemonunderdevDependencies:"devDependencies": { "cookie": "^0.5.0", "eslint": "^8.56.0", "eslint-plugin-node": "^11.1.0", "husky": "^8.0.3", "lint-staged": "^15.2.0", + "nodemon": "^3.1.10", "prettier": "^3.1.1" }backend/routes/announcements.js (1)
226-226: ManualupdatedAtassignment is redundant — schema usestimestamps: true.The
announcementSchemainschema.js(line 104) now includes{ timestamps: true }, which means Mongoose automatically updatesupdatedAton every.save(). This manual assignment is unnecessary and could interfere with Mongoose's timestamp management.♻️ Proposed fix
- announcement.updatedAt = Date.now(); await announcement.save();backend/controllers/certificateController.js (2)
11-11: Remove debugconsole.logstatements before merging.There are ~8
console.logcalls (some commented out) that leak internal IDs to stdout. These should be removed or replaced with a proper logger at debug level.Also applies to: 45-45, 68-68, 76-76, 89-89, 100-100, 106-106, 109-109
147-147: Return201 Createdfor successful resource creation.
res.json(...)defaults to status 200. Since a new batch is being created,res.status(201).json(...)is more semantically correct.
Comment on lines
+188
to
+211
| async (req, res) => { | ||
| 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; |
Contributor
There was a problem hiding this comment.
resolved_by should come from the authenticated user, not from req.body.
Taking resolved_by from the request body (line 190, 211) allows a client to impersonate any user as the resolver. Since this route already requires isAuthenticated, use req.user._id instead.
Additionally, console.log(req.body) on line 191 may log sensitive data — remove or scope down.
Proposed fix
async (req, res) => {
const feedbackId = req.params.id;
- const { actions_taken, resolved_by } = req.body;
- console.log(req.body);
- console.log("User resolving feedback:", resolved_by);
+ const { actions_taken } = req.body;
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;
+ feedback.resolved_by = req.user._id;🤖 Prompt for AI Agents
In `@backend/routes/feedbackRoutes.js` around lines 188 - 211, The route handler
in feedbackRoutes.js is taking resolved_by from req.body and logging req.body;
change it to derive the resolver from the authenticated user (use req.user._id)
and remove or restrict the console.log of the full body to avoid sensitive data
exposure. Specifically, in the async handler where feedbackId, actions_taken,
and resolved_by are read and where you set feedback.resolved_by, stop reading
resolved_by from req.body and instead set feedback.resolved_by = req.user._id
(ensure req.user exists and return 401 if not authenticated); also remove the
console.log(req.body) (or replace with a scoped debug log that excludes
sensitive fields) and keep the check for actions_taken as-is.
| @@ -0,0 +1,16 @@ | |||
| const zod = require("zod"); | |||
|
|
|||
| const zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId"); | |||
Contributor
There was a problem hiding this comment.
ObjectId regex accepts invalid characters.
MongoDB ObjectIds are 24-character hex strings. The current regex [0-9a-zA-Z] allows non-hex characters (e.g., G–Z, g–z), which would pass validation but fail on database lookup.
🐛 Proposed fix
-const zodObjectId = zod.string().regex(/^[0-9a-zA-Z]{24}$/, "Invalid ObjectId");
+const zodObjectId = zod.string().regex(/^[0-9a-fA-F]{24}$/, "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 zodObjectId = zod.string().regex(/^[0-9a-fA-F]{24}$/, "Invalid ObjectId"); |
🤖 Prompt for AI Agents
In `@backend/utils/batchValidate.js` at line 3, zodObjectId currently uses a regex
that allows non-hex characters; update the pattern used in the zodObjectId
constant (zodObjectId) to only allow 24 hex characters by replacing the
character class with the correct hex range and keep the same error message or
adjust it to "Invalid ObjectId" — i.e., use a regex that enforces
/^[0-9a-fA-F]{24}$/ so validations only accept valid MongoDB ObjectId hex
strings.
Switched to session-based authentication and configured persistent session storage with connect-mongo.
…ession-based authentication and configured persistent session storage with connect-mongo.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
backend/routes/auth.js (3)
196-212:⚠️ Potential issue | 🟡 MinorPassword reset GET route lacks error handling for DB query.
Line 200 (
await User.findOne({ _id: id })) is not wrapped in a try-catch — ifidis not a valid ObjectId, Mongoose will throw aCastErrorthat becomes an unhandled promise rejection.Proposed fix
router.get("/reset-password/:id/:token", async (req, res) => { const { id, token } = req.params; - console.log(req.params); - 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 { + 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; 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 the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 196 - 212, The GET route handler router.get("/reset-password/:id/:token", async (req, res) => { ... }) currently calls User.findOne({ _id: id }) without guarding against invalid ObjectId or DB errors; wrap the DB lookup in a try-catch (or pre-validate id with mongoose.Types.ObjectId.isValid) so a malformed id doesn't throw an unhandled CastError—if id is invalid return a 400 with a clear message, and if the DB lookup throws return a 500 and log the error; keep the existing jwt.verify block but ensure errors from User.findOne are handled before calling jwt.verify.
189-189:⚠️ Potential issue | 🟠 MajorPassword reset link logged to console — leaks reset tokens in production logs.
console.log(link)outputs the full password reset URL (including the JWT token) to server logs. An attacker with log access could use these to reset any user's password.Proposed fix
- console.log(link);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` at line 189, Remove the unsafe console.log(link) that emits the full password reset URL (including JWT) to logs; instead, either stop logging the reset link entirely or conditionally log only in non-production (check NODE_ENV) and sanitize the output by removing or masking the token portion of the URL before logging. Locate the console.log call referencing the variable "link" in the password reset flow and replace it with a secure logging approach (e.g., omit token, log only the route/hostname or a masked token) or use the application logger to emit a non-sensitive message.
224-232:⚠️ Potential issue | 🔴 Critical
user.setPassword()will crash — this method doesn't exist.The
passport-local-mongooseplugin is not registered on the User schema. The code at line 224 callsuser.setPassword(), which is a method provided by that plugin, and will throw "user.setPassword is not a function" at runtime.The User schema instead uses a pre-save hook (lines 102–106 in
backend/models/userSchema.js) to hash passwords with bcrypt. Replace this call with direct password assignment:Suggested fix
jwt.verify(token, secret); - user.setPassword(password, async (error) => { - if (error) { - return res.status(500).json({ message: "Error resetting password" }); - } - await user.save(); - return res - .status(200) - .json({ message: "Password has been reset successfully" }); - }); + user.password = password; // pre-save hook will hash it + await user.save(); + return res + .status(200) + .json({ message: "Password has been reset successfully" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 224 - 232, The code calls user.setPassword(), which doesn't exist because passport-local-mongoose isn't used; replace that call by assigning the new plaintext password to the User model field that the pre-save bcrypt hook expects (e.g., user.password = password), then await user.save() and handle errors: on save failure return res.status(500).json({ message: "Error resetting password" }) otherwise return the 200 success JSON; remove the user.setPassword callback and rely on the schema's pre-save hashing logic so the password is hashed before persisting.backend/index.js (1)
76-78:⚠️ Potential issue | 🟠 MajorDuplicate route mount for
/api/announcements.Lines 76 and 78 both mount
announcementRouteson the same path. This causes announcement route handlers to execute twice per request.Proposed fix — remove the duplicate
app.use("/api/announcements", announcementRoutes); app.use("/api/dashboard", dashboardRoutes); -app.use("/api/announcements", announcementRoutes); app.use("/api/analytics", analyticsRoutes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/index.js` around lines 76 - 78, There's a duplicate mount of announcementRoutes via app.use("/api/announcements", announcementRoutes) causing handlers to run twice; remove the redundant call so announcementRoutes is only mounted once (keep a single app.use("/api/announcements", announcementRoutes) and delete the extra occurrence) and verify no other duplicate mounts exist for announcementRoutes.backend/routes/onboarding.js (1)
32-34:⚠️ Potential issue | 🟡 MinorRaw
errorobject leaked in the 500 response.Passing the Mongoose/Node error object directly in the JSON response can expose internal details (stack traces, field names, schema structure) to the client.
Proposed fix
} catch (error) { console.error("Onboarding failed:", error); - res.status(500).json({ message: "Onboarding failed", error }); + res.status(500).json({ message: "Onboarding failed" }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/onboarding.js` around lines 32 - 34, The catch block currently returns the raw error object to the client (see the catch(error) handler, console.error and res.status(500).json call). Log the full error on the server (keep console.error(error) or use your app logger) but change the JSON response to avoid leaking internals: replace res.status(500).json({ message: "Onboarding failed", error }) with a sanitized response such as res.status(500).json({ message: "Onboarding failed" }) or, if you must expose something, only include a non-sensitive string like error.message conditionally in non-production (e.g. process.env.NODE_ENV !== 'production' ? { error: error.message } : {}).frontend/src/services/auth.js (1)
13-25:⚠️ Potential issue | 🔴 CriticalField names in
registerUserdon't match the backend's expected payload — registration will silently fail.The backend
/auth/registerroute destructures{ username, password, user_id, name }fromreq.body(line 71 ofbackend/routes/auth.js), and validates againstregisterValidateschema which requires these exact fields. This function sends{ name, ID, email, password }— soIDarrives as an unrecognized key (backend expectsuser_id), andusername). Zod validation will reject the request sinceusernameanduser_idwill beundefined, returning a 400 error.🐛 Proposed fix
-export async function registerUser(name, ID, email, password) { +export async function registerUser(name, user_id, username, password) { try { const response = await api.post(`/auth/register`, { name, - ID, - email, + user_id, + username, password, }); return response.data.user || null;Update all call sites to pass arguments in the new order:
registerUser(name, user_id, username, password).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/auth.js` around lines 13 - 25, The registerUser function is sending the wrong payload keys so the backend's registerValidate (and the /auth/register handler that destructures { username, password, user_id, name }) rejects the request; change registerUser to post { name, user_id, username, password } instead of { name, ID, email, password } and update all call sites to the new signature/order registerUser(name, user_id, username, password) so the backend receives the expected fields (refer to function registerUser and the /auth/register handler).frontend/src/Components/Auth/UserOnboarding.jsx (2)
41-51:⚠️ Potential issue | 🟡 MinorAdmission year validation has no upper bound.
Line 46 checks
userData.add_year < 2016but does not enforce an upper bound. A user could enter a far-future year (e.g., 9999). Consider addinguserData.add_year > new Date().getFullYear()as an additional check.Proposed fix
- if (!userData.add_year || userData.add_year < 2016) + if (!userData.add_year || userData.add_year < 2016 || userData.add_year > new Date().getFullYear()) newErrors.add_year = "Invalid admission year";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/UserOnboarding.jsx` around lines 41 - 51, The validate function currently only checks that userData.add_year is at least 2016 but allows future years; update validate (in UserOnboarding.jsx) to also check that userData.add_year is not greater than the current year (new Date().getFullYear()) and set newErrors.add_year to an appropriate message when it exceeds the current year; keep the existing lower-bound check so the admission year must be between 2016 and the current calendar year.
128-137:⚠️ Potential issue | 🟠 MajorRead-only
ID_Nofield can create a UX dead end if fetch fails.The Student ID field is now
readOnly(line 131), but validation at line 43 still requiresID_Noto be non-empty. IffetchCredentialsfails or returns nouser_id, the user is stuck — they can't edit the field and can't submit the form. Consider either showing an error state that directs the user to retry/contact support, or falling back to an editable field when the value is empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/UserOnboarding.jsx` around lines 128 - 137, The read-only Student ID (userData.ID_No) can lock the user out if fetchCredentials fails; update the UI to allow input when no ID is present and provide a retry path: change the input to be editable when userData.ID_No is falsy (e.g., set readOnly based on userData.ID_No) and add a visible error state tied to errors.ID_No with a "Retry" action that calls fetchCredentials (or a "Contact support" hint) so users can either re-fetch or manually enter their ID; reference userData.ID_No, errors.ID_No, and fetchCredentials to locate and implement these changes.
🧹 Nitpick comments (10)
frontend/src/Components/Dashboard/Dashboard.jsx (1)
19-32: Loading guard could be more precise.When
isUserLoggedInisnull(auth still resolving), this shows "Loading Dashboard....." — which is misleading since the dashboard isn't loading, auth state is. Consider checkingisLoadingfrom the context (asRoleRedirect.jsxdoes) for a clearer UX, or redirecting unauthenticated users to/logininstead of showing a loading spinner.That said, the
useAdminContext()migration itself is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Dashboard/Dashboard.jsx` around lines 19 - 32, The loading guard currently uses isUserLoggedIn and isOnboardingComplete which treats a null/undefined auth state as "Loading Dashboard....."; update the Dashboard component to read isLoading from useAdminContext (same source used in RoleRedirect.jsx) and show the loading UI only when isLoading === true, and when isLoading === false and isUserLoggedIn !== true perform a redirect to /login (or render the unauthenticated view) instead of the spinner; adjust checks around isOnboardingComplete to only gate the dashboard once auth finished (isLoading false) and onboarding is explicitly true.frontend/src/services/auth.js (2)
29-31: Clean up debug artifacts.Empty line and commented-out
console.logare leftover debug artifacts.Proposed fix
try { - - const res = await api.post("/auth/login", { username, password }); - //console.log("Response is: ", res); + const res = await api.post("/auth/login", { username, password }); return res.data;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/auth.js` around lines 29 - 31, Remove the leftover debug artifacts in the auth login call: delete the extra blank line and the commented-out console.log following the const res = await api.post("/auth/login", { username, password }); statement (in the login function that calls api.post("/auth/login")). Ensure no debug console statements remain in frontend/src/services/auth.js.
1-1: Remove commented-out import.Dead code —
axiosis no longer used directly in this file.Proposed fix
-//import axios from "axios";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/services/auth.js` at line 1, Remove the dead commented import for axios in frontend/src/services/auth.js by deleting the line "//import axios from \"axios\";" so the file contains no unused/commented imports; this cleans up the codebase and avoids misleading readers about unused dependencies.backend/routes/auth.js (1)
23-26: Response keymessagecarries user data — misleading contract.The
fetchAuthendpoint returns user data under themessagekey. This is confusing sincemessageconventionally holds a human-readable string. The frontend (useAuth.jsline 22) readsresponse.messageto get the user object, creating a fragile coupling. Consider usingdataoruseras the key.Proposed fix (backend)
- res.json({message: {personal_info, role, onboardingComplete, user_id}, success: true}); + res.json({user: {personal_info, role, onboardingComplete, user_id}, success: true});Then update
frontend/src/hooks/useAuth.jsaccordingly:- const user = response.message; + const user = response.user;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 23 - 26, The fetchAuth route currently returns the authenticated user fields under a misleading key `message`; change the JSON response in the router.get("/fetchAuth", isAuthenticated, ...) handler so it returns the user object under a clear key such as `user` or `data` (e.g., include { user: { personal_info, role, onboardingComplete, user_id } , success: true } instead of `message`), and update the frontend consumer (useAuth.js) to read the new key (replace response.message usages with response.user or response.data) to keep the API contract explicit and avoid fragile coupling.frontend/src/hooks/useAuth.js (1)
21-25: The backend usesmessageto carry user data — fragile coupling.
response.messagecontains the actual user object (frombackend/routes/auth.jsline 25:res.json({message: {personal_info, role, onboardingComplete, user_id}, success: true})). The field namemessageis conventionally used for human-readable strings, making this contract confusing and easy to break during future refactors. Consider renaming touserordataon the backend response.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/hooks/useAuth.js` around lines 21 - 25, The frontend currently reads the user from response.message (in useAuth.js around fetchCredentials and handleLogin), which is fragile; update the API contract and usage so the backend returns a clearly named field (e.g., user or data) instead of message, then update the frontend to read that field (replace response.message with response.user or response.data in the fetchCredentials consumer and pass that object into handleLogin). Ensure both the backend response creation (in auth.js) and the frontend usage (fetchCredentials handling in useAuth.js) are changed together to keep the contract consistent.frontend/src/Components/Auth/UserOnboarding.jsx (2)
1-1:useContextis still imported but no longer used.Same issue as in
Login.jsx— the switch touseAdminContextmakes theuseContextimport dead code.Proposed fix
-import { useState, useEffect, useContext } from "react"; +import { useState, useEffect } from "react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/UserOnboarding.jsx` at line 1, The import list in UserOnboarding.jsx still includes useContext even though the component now uses useAdminContext; remove the dead import by deleting useContext from the import statement (the import line containing useState, useEffect, useContext) so only the actually used hooks (useState, useEffect) remain, and ensure there are no other references to useContext in the UserOnboarding component.
60-60: Removeconsole.logdebug statement.This will emit validation errors to the browser console on every submit attempt in production.
Proposed fix
const validationErrors = validate(); - console.log("Validation errors: ",validationErrors); if (Object.keys(validationErrors).length > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/UserOnboarding.jsx` at line 60, Remove the development console logging in the UserOnboarding component: delete the console.log("Validation errors: ", validationErrors) statement (or guard it behind a NODE_ENV === "development" check) so validationErrors is no longer emitted to the browser console on submit; locate this call in the UserOnboarding.jsx submit/validation handler where validationErrors is referenced and remove or conditionally wrap it.frontend/src/App.jsx (2)
23-25: Remove commented-out debug/dead code.Multiple commented-out blocks (
console.log,role/navItemsvariables,SidebarProviderwrapper) add noise. If these are planned for future work, track them in an issue rather than leaving them commented out in the codebase.Also applies to: 35-35, 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.jsx` around lines 23 - 25, Remove the commented-out debug/dead code in frontend/src/App.jsx: delete the commented console.log("User data is: ", authData), the commented role and navItems variable lines (role, navItems), and any commented SidebarProvider wrapper references noted in the diff (and similar comments at lines 35 and 64). If this functionality is needed later, open an issue or feature ticket rather than keeping commented code; ensure no behavior changes to functions/components that reference authData, role, navItems, or SidebarProvider.
17-18: Unused imports:NavbarConfigandSidebarProvider.These imports are only referenced in commented-out code (lines 24–25, 35, 64). If the sidebar integration is planned for a future PR, consider removing these imports and the associated commented blocks to keep the file clean. They can be re-added when the feature is ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/App.jsx` around lines 17 - 18, Remove the unused imports NavbarConfig and SidebarProvider from App.jsx and delete the associated commented-out blocks that reference them (the commented lines around the navbar/sidebar usage) to keep the file clean; if sidebar/navbar will be implemented soon, instead add a short TODO comment referencing NavbarConfig and SidebarProvider and leave no unused imports in the module.frontend/src/Components/Auth/Login.jsx (1)
1-1:useContextis still imported but no longer used.Line 2 switches to
useAdminContext, making theuseContextimport on line 1 dead code.Proposed fix
-import React, { useState, useContext } from "react"; +import React, { useState } from "react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/Login.jsx` at line 1, The import list in Login.jsx still includes the unused symbol useContext; remove useContext from the React import so only React and useState (and any other used hooks) are imported, since the component now uses useAdminContext instead; update the import line to eliminate the dead import and ensure no other references to useContext remain in the file (e.g., check for useContext(...) usages and replace them with useAdminContext where appropriate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/config/passportConfig.js`:
- Around line 59-92: The LocalStrategy in passportConfig.js is rejecting
non-@iitbhilai.ac.in usernames because loginValidate (in
backend/utils/authValidate.js) uses a strict domain regex; update the validation
or strategy to avoid locking out existing users: either relax or change
loginValidate to accept legacy usernames/emails (e.g., allow plain usernames or
any email) or modify the LocalStrategy to accept the raw username first and only
enforce the IIT Bhilai domain for new registrations (e.g., perform domain check
in the signup flow instead of in loginValidate), and add a short DB
audit/migration plan to handle existing non-IIT users; refer to the
LocalStrategy declaration (passport.use(new LocalStrategy(...))) and the
loginValidate schema to locate where to change validation and where to add
fallback logic.
In `@backend/routes/onboarding.js`:
- Around line 7-9: The onboarding PUT handler (router.put("/", isAuthenticated,
async (req, res) => {...})) currently destructures Program, discipline,
add_year, mobile_no from req.body and later calls Program.trim(), which will
throw if missing; add Zod validation (or at minimum a guard) for the onboarding
payload mirroring the auth routes: define a Zod schema for { Program:
string().min(1), discipline: string().min(1), add_year:
number().int().optional(), mobile_no: string().optional() } and parse req.body
at the start of the handler (or check Program/discipline existence before
calling trim), then use the validated values instead of raw req.body variables
to prevent TypeError and ensure consistent input validation.
In `@frontend/src/Components/Auth/Login.jsx`:
- Line 26: Remove the trailing space from the toast message in the Login
component: update the toast.success call inside Login.jsx (the one that
currently reads toast.success("Login successful ")) to use "Login successful"
instead; ensure any other similar toast messages in the Login component (or
surrounding functions like the submit/login handler) do not contain unintended
trailing whitespace.
- Around line 22-27: The Login.jsx code assumes loginUser(email, password)
always returns an object but it can return null; add a null guard before
accessing response.success: after calling loginUser, check if response === null
(or !response) and handle it by showing an error toast (e.g., toast.error with a
clear message), avoid calling handleLogin/navigate in that case, and return
early; otherwise continue to check response.success and proceed to call
handleLogin(response.data), toast.success, and navigate. Ensure this change
references the call to loginUser and the subsequent use of
response.success/handleLogin so you prevent the TypeError and surface the real
network/API error to the user.
In `@frontend/src/Components/Auth/UserOnboarding.jsx`:
- Around line 26-33: The code assumes response.message exists; guard against a
missing or undefined message by checking response?.message before using it:
obtain user via a safe access (e.g., const user = response?.message || {}) or
early-return if missing, and update setUserData using user?.personal_info?.name,
user?.personal_info?.email, and user?.user_id (or sensible defaults) so
accessing personal_info cannot throw; locate the block where fetchCredentials()
is called and the setUserData update is performed to apply this fix.
In `@frontend/src/hooks/useAuth.js`:
- Line 23: Remove the console.log that prints user data in useAuth (the line
logging "User is:", user). Either delete the console.log entirely or replace it
with a non-production-only debug/log call (e.g., use a debug/logger and guard
with NODE_ENV !== 'production') so PII is not emitted in production; ensure the
change targets the useAuth hook where the user variable is logged.
---
Outside diff comments:
In `@backend/index.js`:
- Around line 76-78: There's a duplicate mount of announcementRoutes via
app.use("/api/announcements", announcementRoutes) causing handlers to run twice;
remove the redundant call so announcementRoutes is only mounted once (keep a
single app.use("/api/announcements", announcementRoutes) and delete the extra
occurrence) and verify no other duplicate mounts exist for announcementRoutes.
In `@backend/routes/auth.js`:
- Around line 196-212: The GET route handler
router.get("/reset-password/:id/:token", async (req, res) => { ... }) currently
calls User.findOne({ _id: id }) without guarding against invalid ObjectId or DB
errors; wrap the DB lookup in a try-catch (or pre-validate id with
mongoose.Types.ObjectId.isValid) so a malformed id doesn't throw an unhandled
CastError—if id is invalid return a 400 with a clear message, and if the DB
lookup throws return a 500 and log the error; keep the existing jwt.verify block
but ensure errors from User.findOne are handled before calling jwt.verify.
- Line 189: Remove the unsafe console.log(link) that emits the full password
reset URL (including JWT) to logs; instead, either stop logging the reset link
entirely or conditionally log only in non-production (check NODE_ENV) and
sanitize the output by removing or masking the token portion of the URL before
logging. Locate the console.log call referencing the variable "link" in the
password reset flow and replace it with a secure logging approach (e.g., omit
token, log only the route/hostname or a masked token) or use the application
logger to emit a non-sensitive message.
- Around line 224-232: The code calls user.setPassword(), which doesn't exist
because passport-local-mongoose isn't used; replace that call by assigning the
new plaintext password to the User model field that the pre-save bcrypt hook
expects (e.g., user.password = password), then await user.save() and handle
errors: on save failure return res.status(500).json({ message: "Error resetting
password" }) otherwise return the 200 success JSON; remove the user.setPassword
callback and rely on the schema's pre-save hashing logic so the password is
hashed before persisting.
In `@backend/routes/onboarding.js`:
- Around line 32-34: The catch block currently returns the raw error object to
the client (see the catch(error) handler, console.error and res.status(500).json
call). Log the full error on the server (keep console.error(error) or use your
app logger) but change the JSON response to avoid leaking internals: replace
res.status(500).json({ message: "Onboarding failed", error }) with a sanitized
response such as res.status(500).json({ message: "Onboarding failed" }) or, if
you must expose something, only include a non-sensitive string like
error.message conditionally in non-production (e.g. process.env.NODE_ENV !==
'production' ? { error: error.message } : {}).
In `@frontend/src/Components/Auth/UserOnboarding.jsx`:
- Around line 41-51: The validate function currently only checks that
userData.add_year is at least 2016 but allows future years; update validate (in
UserOnboarding.jsx) to also check that userData.add_year is not greater than the
current year (new Date().getFullYear()) and set newErrors.add_year to an
appropriate message when it exceeds the current year; keep the existing
lower-bound check so the admission year must be between 2016 and the current
calendar year.
- Around line 128-137: The read-only Student ID (userData.ID_No) can lock the
user out if fetchCredentials fails; update the UI to allow input when no ID is
present and provide a retry path: change the input to be editable when
userData.ID_No is falsy (e.g., set readOnly based on userData.ID_No) and add a
visible error state tied to errors.ID_No with a "Retry" action that calls
fetchCredentials (or a "Contact support" hint) so users can either re-fetch or
manually enter their ID; reference userData.ID_No, errors.ID_No, and
fetchCredentials to locate and implement these changes.
In `@frontend/src/services/auth.js`:
- Around line 13-25: The registerUser function is sending the wrong payload keys
so the backend's registerValidate (and the /auth/register handler that
destructures { username, password, user_id, name }) rejects the request; change
registerUser to post { name, user_id, username, password } instead of { name,
ID, email, password } and update all call sites to the new signature/order
registerUser(name, user_id, username, password) so the backend receives the
expected fields (refer to function registerUser and the /auth/register handler).
---
Duplicate comments:
In `@backend/config/passportConfig.js`:
- Around line 18-27: The code extracts email into the variable `email` but then
inconsistently uses `profile.emails[0].value` when checking `isIITBhilaiEmail`
and logging; update those references to use the already-defensive `email`
variable instead. Specifically, in the Google OAuth callback block where `const
email = profile.emails?.[0]?.value` and `if (!email) return done(...)` are
present, replace `profile.emails[0].value` with `email` in the
`isIITBhilaiEmail(email)` call and in the `console.log` so the validation and
logging use the validated value.
In `@backend/index.js`:
- Around line 84-90: connectDB() is currently swallowing errors so await
connectDB() never rejects and the server (app.listen) starts even when DB
connection failed; either make connectDB (in config/db.js) rethrow the caught
error or handle it here by wrapping await connectDB() in a try/catch and exit
the process on failure (e.g., log the error via console.error/processLogger and
call process.exit(1)) before calling app.listen; reference connectDB() and
app.listen to locate the code to change.
- Around line 48-52: The MongoStore.create call is passing ttl in milliseconds
(60*60*1000) but connect-mongo expects seconds, so change the ttl to seconds to
match the cookie maxAge; either set ttl: 60*60 (1 hour) or compute it from the
session cookie (e.g., Math.floor(sessionOptions.cookie.maxAge / 1000)) so
MongoStore.ttl and the cookie maxAge remain consistent; update the ttl value in
the MongoStore.create(...) options.
---
Nitpick comments:
In `@backend/routes/auth.js`:
- Around line 23-26: The fetchAuth route currently returns the authenticated
user fields under a misleading key `message`; change the JSON response in the
router.get("/fetchAuth", isAuthenticated, ...) handler so it returns the user
object under a clear key such as `user` or `data` (e.g., include { user: {
personal_info, role, onboardingComplete, user_id } , success: true } instead of
`message`), and update the frontend consumer (useAuth.js) to read the new key
(replace response.message usages with response.user or response.data) to keep
the API contract explicit and avoid fragile coupling.
In `@frontend/src/App.jsx`:
- Around line 23-25: Remove the commented-out debug/dead code in
frontend/src/App.jsx: delete the commented console.log("User data is: ",
authData), the commented role and navItems variable lines (role, navItems), and
any commented SidebarProvider wrapper references noted in the diff (and similar
comments at lines 35 and 64). If this functionality is needed later, open an
issue or feature ticket rather than keeping commented code; ensure no behavior
changes to functions/components that reference authData, role, navItems, or
SidebarProvider.
- Around line 17-18: Remove the unused imports NavbarConfig and SidebarProvider
from App.jsx and delete the associated commented-out blocks that reference them
(the commented lines around the navbar/sidebar usage) to keep the file clean; if
sidebar/navbar will be implemented soon, instead add a short TODO comment
referencing NavbarConfig and SidebarProvider and leave no unused imports in the
module.
In `@frontend/src/Components/Auth/Login.jsx`:
- Line 1: The import list in Login.jsx still includes the unused symbol
useContext; remove useContext from the React import so only React and useState
(and any other used hooks) are imported, since the component now uses
useAdminContext instead; update the import line to eliminate the dead import and
ensure no other references to useContext remain in the file (e.g., check for
useContext(...) usages and replace them with useAdminContext where appropriate).
In `@frontend/src/Components/Auth/UserOnboarding.jsx`:
- Line 1: The import list in UserOnboarding.jsx still includes useContext even
though the component now uses useAdminContext; remove the dead import by
deleting useContext from the import statement (the import line containing
useState, useEffect, useContext) so only the actually used hooks (useState,
useEffect) remain, and ensure there are no other references to useContext in the
UserOnboarding component.
- Line 60: Remove the development console logging in the UserOnboarding
component: delete the console.log("Validation errors: ", validationErrors)
statement (or guard it behind a NODE_ENV === "development" check) so
validationErrors is no longer emitted to the browser console on submit; locate
this call in the UserOnboarding.jsx submit/validation handler where
validationErrors is referenced and remove or conditionally wrap it.
In `@frontend/src/Components/Dashboard/Dashboard.jsx`:
- Around line 19-32: The loading guard currently uses isUserLoggedIn and
isOnboardingComplete which treats a null/undefined auth state as "Loading
Dashboard....."; update the Dashboard component to read isLoading from
useAdminContext (same source used in RoleRedirect.jsx) and show the loading UI
only when isLoading === true, and when isLoading === false and isUserLoggedIn
!== true perform a redirect to /login (or render the unauthenticated view)
instead of the spinner; adjust checks around isOnboardingComplete to only gate
the dashboard once auth finished (isLoading false) and onboarding is explicitly
true.
In `@frontend/src/hooks/useAuth.js`:
- Around line 21-25: The frontend currently reads the user from response.message
(in useAuth.js around fetchCredentials and handleLogin), which is fragile;
update the API contract and usage so the backend returns a clearly named field
(e.g., user or data) instead of message, then update the frontend to read that
field (replace response.message with response.user or response.data in the
fetchCredentials consumer and pass that object into handleLogin). Ensure both
the backend response creation (in auth.js) and the frontend usage
(fetchCredentials handling in useAuth.js) are changed together to keep the
contract consistent.
In `@frontend/src/services/auth.js`:
- Around line 29-31: Remove the leftover debug artifacts in the auth login call:
delete the extra blank line and the commented-out console.log following the
const res = await api.post("/auth/login", { username, password }); statement (in
the login function that calls api.post("/auth/login")). Ensure no debug console
statements remain in frontend/src/services/auth.js.
- Line 1: Remove the dead commented import for axios in
frontend/src/services/auth.js by deleting the line "//import axios from
\"axios\";" so the file contains no unused/commented imports; this cleans up the
codebase and avoids misleading readers about unused dependencies.
Comment on lines
59
to
92
| //Local Strategy | ||
| passport.use(new LocalStrategy(async (username, password, done) => { | ||
|
|
||
| const result = loginValidate.safeParse({ username, password }); | ||
|
|
||
| if (!result.success) { | ||
| let errors = result.error.issues.map((issue) => issue.message); | ||
| return done(null, false, {message: errors}); | ||
| } | ||
|
|
||
| try{ | ||
|
|
||
| const user = await User.findOne({ username }); | ||
| if (!user) { | ||
| return done(null, false, {message: "Invalid user credentials"}); | ||
| } | ||
|
|
||
|
|
||
| if (user.strategy !== "local" || !user.password) { | ||
| return done(null, false, { message: "Invalid login method" }); | ||
| } | ||
|
|
||
| const isValid = await bcrypt.compare(password, user.password); | ||
| if (!isValid) { | ||
| return done(null, false, { message: "Invalid user credentials" }); | ||
| } | ||
|
|
||
|
|
||
| return done(null, user); | ||
| }catch(err){ | ||
| return done(err); | ||
| } | ||
|
|
||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat backend/utils/authValidate.jsRepository: OpenLake/Student_Database_COSA
Length of output: 548
LocalStrategy enforces IIT Bhilai email-only access; existing non-IIT users will be blocked.
The loginValidate schema in backend/utils/authValidate.js enforces a strict regex pattern (^[a-zA-Z0-9._%+-]+@iitbhilai\.ac\.in$) that rejects any username lacking the @iitbhilai.ac.in domain. This means any existing user account not using an IIT Bhilai email will be permanently locked out. Before deploying this change, audit the database for non-IIT Bhilai usernames and have a migration strategy ready (or disable this validation if it's unintended).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/config/passportConfig.js` around lines 59 - 92, The LocalStrategy in
passportConfig.js is rejecting non-@iitbhilai.ac.in usernames because
loginValidate (in backend/utils/authValidate.js) uses a strict domain regex;
update the validation or strategy to avoid locking out existing users: either
relax or change loginValidate to accept legacy usernames/emails (e.g., allow
plain usernames or any email) or modify the LocalStrategy to accept the raw
username first and only enforce the IIT Bhilai domain for new registrations
(e.g., perform domain check in the signup flow instead of in loginValidate), and
add a short DB audit/migration plan to handle existing non-IIT users; refer to
the LocalStrategy declaration (passport.use(new LocalStrategy(...))) and the
loginValidate schema to locate where to change validation and where to add
fallback logic.
Comment on lines
+7
to
9
| router.put("/", isAuthenticated, async (req, res) => { | ||
| const { add_year, Program, discipline, mobile_no } = req.body; | ||
|
|
Contributor
There was a problem hiding this comment.
No input validation — Program.trim() will throw if the field is missing.
Unlike the auth routes which use Zod validation, onboarding accepts raw req.body without validation. If Program, discipline, or add_year are undefined, line 20 (Program.trim()) will throw a TypeError, and the other fields will silently store undefined.
Consider adding Zod validation for the onboarding payload, consistent with the pattern used in auth routes.
🛡️ Proposed fix — add a guard or validation
router.put("/", isAuthenticated, async (req, res) => {
const { add_year, Program, discipline, mobile_no } = req.body;
+
+ if (!add_year || !Program || !discipline) {
+ return res.status(400).json({ message: "add_year, Program, and discipline are required." });
+ }
try {📝 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.put("/", isAuthenticated, async (req, res) => { | |
| const { add_year, Program, discipline, mobile_no } = req.body; | |
| router.put("/", isAuthenticated, async (req, res) => { | |
| const { add_year, Program, discipline, mobile_no } = req.body; | |
| if (!add_year || !Program || !discipline) { | |
| return res.status(400).json({ message: "add_year, Program, and discipline are required." }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/routes/onboarding.js` around lines 7 - 9, The onboarding PUT handler
(router.put("/", isAuthenticated, async (req, res) => {...})) currently
destructures Program, discipline, add_year, mobile_no from req.body and later
calls Program.trim(), which will throw if missing; add Zod validation (or at
minimum a guard) for the onboarding payload mirroring the auth routes: define a
Zod schema for { Program: string().min(1), discipline: string().min(1),
add_year: number().int().optional(), mobile_no: string().optional() } and parse
req.body at the start of the handler (or check Program/discipline existence
before calling trim), then use the validated values instead of raw req.body
variables to prevent TypeError and ensure consistent input validation.
Comment on lines
22
to
27
| const response = await loginUser(email, password); | ||
| //console.log(response); | ||
| if (response.success) { | ||
| handleLogin(response.data); | ||
| toast.success("Login successful "); | ||
| navigate("/onboarding", { replace: true }); |
Contributor
There was a problem hiding this comment.
Null-pointer risk: loginUser returns null on network/API errors.
Per frontend/src/services/auth.js (lines 26–36), loginUser returns null when the request fails. Accessing response.success on null at line 24 will throw a TypeError. While the outer catch block will handle it, the error logged will be a misleading TypeError rather than the actual failure reason, and the control flow is fragile.
Add a null guard before accessing response.success:
Proposed fix
const response = await loginUser(email, password);
- //console.log(response);
- if (response.success) {
+ if (response?.success) {
handleLogin(response.data);
toast.success("Login successful");
navigate("/onboarding", { replace: true });📝 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 response = await loginUser(email, password); | |
| //console.log(response); | |
| if (response.success) { | |
| handleLogin(response.data); | |
| toast.success("Login successful "); | |
| navigate("/onboarding", { replace: true }); | |
| const response = await loginUser(email, password); | |
| if (response?.success) { | |
| handleLogin(response.data); | |
| toast.success("Login successful"); | |
| navigate("/onboarding", { replace: true }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/Login.jsx` around lines 22 - 27, The Login.jsx
code assumes loginUser(email, password) always returns an object but it can
return null; add a null guard before accessing response.success: after calling
loginUser, check if response === null (or !response) and handle it by showing an
error toast (e.g., toast.error with a clear message), avoid calling
handleLogin/navigate in that case, and return early; otherwise continue to check
response.success and proceed to call handleLogin(response.data), toast.success,
and navigate. Ensure this change references the call to loginUser and the
subsequent use of response.success/handleLogin so you prevent the TypeError and
surface the real network/API error to the user.
| //console.log(response); | ||
| if (response.success) { | ||
| handleLogin(response.data); | ||
| toast.success("Login successful "); |
Contributor
There was a problem hiding this comment.
Trailing space in toast message.
"Login successful " has a trailing space. Minor text quality nit.
Proposed fix
- toast.success("Login successful ");
+ toast.success("Login successful"); 📝 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
| toast.success("Login successful "); | |
| toast.success("Login successful"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/Login.jsx` at line 26, Remove the trailing space
from the toast message in the Login component: update the toast.success call
inside Login.jsx (the one that currently reads toast.success("Login successful
")) to use "Login successful" instead; ensure any other similar toast messages
in the Login component (or surrounding functions like the submit/login handler)
do not contain unintended trailing whitespace.
Comment on lines
26
to
33
| const response = await fetchCredentials(); | ||
| const user = response.message; | ||
| setUserData((prev) => ({ | ||
| ...prev, | ||
| name: user.personal_info.name, | ||
| email: user.personal_info.email, | ||
| name: user.personal_info?.name, | ||
| email: user.personal_info?.email, | ||
| ID_No: user.user_id, | ||
| })); |
Contributor
There was a problem hiding this comment.
Null-safety gap: response.message may be undefined, causing user.personal_info to throw.
If fetchCredentials() returns an unexpected shape (e.g., the API response lacks a message field), user will be undefined and user.personal_info?.name at line 30 will throw a TypeError. The optional chaining only guards personal_info, not user itself.
Proposed fix
const response = await fetchCredentials();
const user = response.message;
+ if (!user) return;
setUserData((prev) => ({
...prev,
name: user.personal_info?.name,
email: user.personal_info?.email,
ID_No: user.user_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 response = await fetchCredentials(); | |
| const user = response.message; | |
| setUserData((prev) => ({ | |
| ...prev, | |
| name: user.personal_info.name, | |
| email: user.personal_info.email, | |
| name: user.personal_info?.name, | |
| email: user.personal_info?.email, | |
| ID_No: user.user_id, | |
| })); | |
| const response = await fetchCredentials(); | |
| const user = response.message; | |
| if (!user) return; | |
| setUserData((prev) => ({ | |
| ...prev, | |
| name: user.personal_info?.name, | |
| email: user.personal_info?.email, | |
| ID_No: user.user_id, | |
| })); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/UserOnboarding.jsx` around lines 26 - 33, The
code assumes response.message exists; guard against a missing or undefined
message by checking response?.message before using it: obtain user via a safe
access (e.g., const user = response?.message || {}) or early-return if missing,
and update setUserData using user?.personal_info?.name,
user?.personal_info?.email, and user?.user_id (or sensible defaults) so
accessing personal_info cannot throw; locate the block where fetchCredentials()
is called and the setUserData update is performed to apply this fix.
| console.log("Onboarding complete:", user.onboardingComplete); | ||
| const response = await fetchCredentials(); | ||
| const user = response.message; | ||
| console.log("User is:", user); |
Contributor
There was a problem hiding this comment.
Remove console.log from production code path.
This runs on every app initialization and logs user data (potentially including PII) to the browser console.
Proposed fix
const user = response.message;
- console.log("User is:", user);
if (response?.success) {📝 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("User is:", user); | |
| const user = response.message; | |
| if (response?.success) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/hooks/useAuth.js` at line 23, Remove the console.log that prints
user data in useAuth (the line logging "User is:", user). Either delete the
console.log entirely or replace it with a non-production-only debug/log call
(e.g., use a debug/logger and guard with NODE_ENV !== 'production') so PII is
not emitted in production; ensure the change targets the useAuth hook where the
user variable is logged.
…ce error handling, and remove unused code
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/routes/auth.js (1)
237-260:⚠️ Potential issue | 🔴 Critical
user.setPassworddoes not exist — the entire password-reset POST route will crash.
setPasswordis injected by thepassport-local-mongooseplugin. The newuserSchema.jsdoes not use that plugin (module.exports = Userwith only bcrypt and no.plugin()call), souser.setPasswordisundefined. Calling it throwsTypeError: user.setPassword is not a function, making password resets completely non-functional.The fix is to directly assign to
user.passwordand callsave(). The pre-save hook inuserSchema.jswill hash it automatically.Additionally, the
User.findOnecall at line 240 and the entire route body are outside atry/catch, so any DB error also propagates as an unhandled rejection.🐛 Proposed fix
router.post("/reset-password/:id/:token", async (req, res) => { const { id, token } = req.params; const { password } = req.body; - 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 { + 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; jwt.verify(token, secret); - user.setPassword(password, async (error) => { - if (error) { - return res.status(500).json({ message: "Error resetting password" }); - } - await user.save(); - return res - .status(200) - .json({ message: "Password has been reset successfully" }); - }); + user.password = password; // pre-save hook will hash it + await user.save(); + return res.status(200).json({ message: "Password has been reset successfully" }); } catch (error) { console.log(error); return res.status(400).json({ message: "Invalid or expired token" }); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/auth.js` around lines 237 - 260, The POST /reset-password handler calls user.setPassword (in the User.findOne result) which no longer exists because passport-local-mongoose was removed, causing a TypeError; fix by replacing the setPassword call with direct assignment user.password = password and await user.save() (the userSchema pre-save hook will hash the password), and wrap the DB operations (User.findOne, user.save) and jwt.verify in a try/catch to properly handle and return DB or verification errors instead of letting unhandled rejections crash the route.backend/routes/profile.js (3)
109-111:⚠️ Potential issue | 🟠 Major
console.logstatements logupdatedDetailswhich can contain PII.
updatedDetailsmay includepersonal_infofields (name, email, phone, date of birth). Logging this to the server console is a privacy concern and should be removed before production.Proposed fix
- console.log("Received userId:", userId); - console.log("Received updatedDetails:", updatedDetails);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/profile.js` around lines 109 - 111, Remove the debug console.log statements that print sensitive request data: do not log req.body.updatedDetails or req.body.userId in production; instead either delete the two console.log lines or replace them with a non-sensitive, structured log (e.g., log only the request trace ID or userId hash) inside the route handler that processes updatedDetails (refer to the variables userId and updatedDetails in the profile update handler). Ensure no personal_info fields (name, email, phone, dob) are ever output to logs.
220-220:⚠️ Potential issue | 🟡 Minor
user.updated_atis not a schema field — this line is dead code.The schema uses
timestamps: true, which creates and managesupdatedAt(camelCase) automatically on everysave().updated_at(snake_case) is not defined in the schema and is silently discarded. Remove this line.Proposed fix
- user.updated_at = new Date(); - // Save changes await user.save();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/profile.js` at line 220, Remove the dead assignment to user.updated_at (snake_case) — the Mongoose schema uses timestamps: true and manages updatedAt (camelCase) automatically, so the line setting user.updated_at should be deleted; locate the code around the user object update in the route handler (the line with user.updated_at = new Date()) and remove it, relying on the model's save() or update call to set updatedAt instead.
225-229:⚠️ Potential issue | 🟠 Major
updatedStudent: userreturns the hashed password in the API response.The full Mongoose document (including
password) is serialized into the response. Strip the password before returning:🐛 Proposed fix
+ const userSafe = user.toObject(); + delete userSafe.password; return res.status(200).json({ success: true, message: "Student profile updated successfully", - updatedStudent: user, + updatedStudent: userSafe, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/profile.js` around lines 225 - 229, The response currently returns the full Mongoose document as updatedStudent (variable user), which includes the hashed password; convert the document to a plain object (e.g., via user.toObject() or user.toJSON()) or ensure you fetched it with .select('-password'), remove or unset the password property on that plain object (delete obj.password or set it to undefined), and return the sanitized object as updatedStudent in the res.status(200).json(...) response so no password is leaked.
🧹 Nitpick comments (5)
backend/config/passportConfig.js (1)
24-29: Lines 24–25 still bypass the extractedThe guard on lines 18–22 already ensures
profile.emails[0].value, making the earlier extraction partially dead and the intent unclear.♻️ Proposed fix
- if (!isIITBhilaiEmail(profile.emails[0].value)) { - console.log("Google OAuth blocked for: ", profile.emails[0].value); + if (!isIITBhilaiEmail(email)) { + console.log("Google OAuth blocked for: ", email);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/config/passportConfig.js` around lines 24 - 29, The code still uses profile.emails[0].value instead of the previously extracted email variable; update the Google OAuth guard to use the extracted email (email) everywhere for consistency and clarity—specifically replace any remaining profile.emails[0].value usages in the block that calls isIITBhilaiEmail and the console.log/done message so isIITBhilaiEmail(email) and console.log("Google OAuth blocked for:", email) / done(null, false, { message: ... , email }) (or include email in the message) are used, keeping the existing guard that ensures email is non-null.frontend/src/Components/Auth/RoleRedirect.jsx (1)
1-1:useContextis imported but no longer used.
useAdminContext()on line 7 wrapsuseContextinternally; the direct import is now dead.♻️ Proposed fix
-import React, { useContext } from "react"; +import React from "react";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/RoleRedirect.jsx` at line 1, The import of useContext in RoleRedirect.jsx is dead; remove useContext from the import statement so the line reads only import React (or import React, { ... } if other hooks remain), since useAdminContext() (used in this file) wraps useContext internally; update the import of React accordingly and ensure no other references to useContext remain in the file (useAdminContext is the single context access point).backend/routes/events.js (1)
444-477: Development debug logs should be removed before merging.The PUT
/:eventIdhandler emits full JSON-stringified event documents and structural metadata viaconsole.log. These are clearly left from development and will spam production logs with potentially sensitive event data.♻️ Proposed fix
- // 🔍 DEBUG LOGS - START - console.log("\n=== 📝 UPDATE EVENT DEBUG ==="); - console.log("Event ID:", eventId); - console.log( - "Updates received (full body):", - JSON.stringify(updates, null, 2), - ); - console.log("Number of fields to update:", Object.keys(updates).length); - console.log("Fields being updated:", Object.keys(updates)); - console.log("========================\n"); - - // Fetch the event BEFORE update to compare - const eventBefore = await Event.findById(eventId); - console.log("Event BEFORE update:", JSON.stringify(eventBefore, null, 2)); - const event = await Event.findByIdAndUpdate(eventId, updates, { new: true, runValidators: true, }); - - console.log("\n=== ✅ UPDATE RESULT ==="); - console.log("Event AFTER update:", JSON.stringify(event, null, 2)); - console.log("Update successful:", !!event); - console.log("========================\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/routes/events.js` around lines 444 - 477, Remove the development console.log debug block from the PUT '/:eventId' route handler in backend/routes/events.js (the section that logs eventId, updates, eventBefore, event AFTER update, etc.), and replace it with minimal, non-sensitive logging: log only the operation outcome and the eventId (or use the existing app logger at debug level) rather than JSON-stringifying full documents or request bodies; keep the Event.findByIdAndUpdate(..., updates, { new: true, runValidators: true }) logic and error handling unchanged, and ensure any logging does not include sensitive fields from the "event" or "updates" objects.frontend/src/Components/Auth/Login.jsx (1)
1-9: Combine duplicatereact-router-domimports and remove unuseduseContext.
useNavigate(line 4) andLink(line 9) are both fromreact-router-domand can be merged into a single import.useContextis imported on line 1 but unused since the refactor touseAdminContext.♻️ Proposed fix
-import React, { useState, useContext } from "react"; +import React, { useState } from "react"; import { useAdminContext } from "../../context/AdminContext"; import { loginUser } from "../../services/auth"; -import { useNavigate } from "react-router-dom"; +import { useNavigate, Link } from "react-router-dom"; import GoogleIcon from "@mui/icons-material/Google"; import cosa from "../../assets/COSA.png"; import backgroundImage from "../../assets/iitbh.jpg"; import { toast } from "react-toastify"; -import {Link} from "react-router-dom"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/Login.jsx` around lines 1 - 9, The import list in Login.jsx has duplicate imports from react-router-dom and an unused useContext; update the top imports by removing the unused useContext from the React import and merging Link into the existing react-router-dom import that already brings in useNavigate (i.e., import { useNavigate, Link } from "react-router-dom"), leaving useAdminContext and other imports unchanged so there are no duplicate or unused imports.frontend/src/Components/Auth/Register.jsx (1)
158-171: Remove commented-out dead code.Lines 158–171 contain a fully commented-out
<Link>-wrapped button block that was replaced by the active button on line 172. This should be deleted to keep the file clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/Auth/Register.jsx` around lines 158 - 171, Remove the dead commented-out Google sign-up block in the Register component (the commented <Link> ... <button> ... <GoogleIcon /> block) so the file is clean; also remove any now-unused imports related to Link or GoogleIcon in frontend/src/Components/Auth/Register.jsx (e.g., React Router's Link or GoogleIcon) to avoid unused-import warnings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/controllers/certificateController.js`:
- Line 15: The JSON response in certificateController.js uses a misspelled key
"messge" in the error return (the res.status(404).json call); change the key to
"message" so the error payload is { message: "Invalid data (User not found)" }
to ensure consumers receive the standard "message" field.
In `@backend/controllers/eventControllers.js`:
- Around line 11-13: The 404 guard is using `if (!latestEvents)` but
`Event.find()` always returns an array, so replace that check with a length
check (e.g., `if (latestEvents.length === 0)`) in the controller handling the
`Event.find()` result (look for `latestEvents` and the `Event.find()` call in
the event controller function) so an empty result returns the intended 404 JSON
response instead of 200 with an empty array.
- Around line 6-9: The sort call on Event.find uses the wrong field name
'updated_at' so it doesn't sort; update the sort to use the Mongoose timestamps
field 'updatedAt' (e.g., change .sort({updated_at: -1}) to .sort({updatedAt:
-1}) for latestEvents) and scan for any other .sort calls or selects in this
controller that reference 'updated_at' to similarly switch them to 'updatedAt'
to ensure sorting works as intended.
In `@backend/index.js`:
- Around line 48-52: The ttl passed to MongoStore.create is using milliseconds
(60*60*1000) but connect-mongo expects seconds, causing sessions to live ~41
days; update the ttl to be in seconds (e.g., 60*60) or compute it from the
session cookie maxAge (Math.floor(session.cookie.maxAge / 1000) or use the same
constant) so ttl and cookie maxAge match; locate the MongoStore.create call and
replace the ttl expression accordingly to ensure sessions expire from the DB
when the cookie does.
- Line 53: The session middleware in backend/index.js sets the cookie name to
"token", which collides with jwtIsAuthenticated in
backend/middlewares/isAuthenticated.js that expects req.cookies.token to be a
JWT; rename the session cookie (e.g., to "sid" or "sessionId") in the session
configuration (the object containing name: "token") and update any server code
that reads that cookie name accordingly so jwtIsAuthenticated continues to read
req.cookies.token for JWTs while the session middleware writes the new cookie
name; ensure no other logic assumes the old "token" cookie name.
In `@backend/routes/auth.js`:
- Line 223: Remove the sensitive logging of the password-reset token by deleting
the console.log(req.params) line (or replace it with a safe log that omits
req.params.token); specifically locate the route handler that accesses
req.params.token and remove any console.log calls that print req.params, or if
you must log, log only non-sensitive keys or use your logger to redact/omit
token before logging.
In `@backend/routes/profile.js`:
- Line 7: The require in profile routes is destructuring a default export so
User becomes undefined; replace the destructuring import "const { User } =
require('../models/userSchema')" with a default import "const User =
require('../models/userSchema')" so that calls like User.findOne(...) and other
User.* usages reference the exported model correctly.
In `@frontend/src/Components/Auth/Login.jsx`:
- Line 136: The Login.jsx page currently displays the text "Sign up with Google"
inside the Google auth button (the span within the Login component); update that
label to "Sign in with Google" so the action matches the Login page—locate the
span/button in the Login component and replace the displayed string accordingly.
In `@frontend/src/Components/Auth/Register.jsx`:
- Line 16: The code incorrectly calls useNavigate with an argument — change the
call to const navigate = useNavigate(); (in Register.jsx) and, where a default
redirect is intended, call navigate('/') explicitly at the appropriate place
(e.g., after successful registration in the submit/handleRegister function)
rather than passing "/" into useNavigate; ensure any existing tests or usages
reference navigate('/') and not the hook argument.
- Around line 40-54: The toast shows empty text when the caught error has no
HTTP response (network failure) because the existing logic only inspects
response/response.data; update the error-extraction in Register.jsx (the block
using response, respData, errorMessage and calling toast.error) to handle the
case where response is undefined by falling back to the original error.message
(or a generic "Network error" string) returned by registerUser in
frontend/src/services/auth.js so toast.error is always given a non-empty,
user-facing message.
In `@frontend/src/Components/Auth/RoleRedirect.jsx`:
- Around line 10-12: The current check Object.keys(isUserLoggedIn) crashes when
isUserLoggedIn is null; update the guard in RoleRedirect.jsx to handle
null/undefined (e.g., use Object.keys(isUserLoggedIn ?? {}).length === 0 or
simply if (!isUserLoggedIn) return <Navigate to="/login" replace />) and ensure
you consider the isLoading flag from useAuth.js so you only redirect after
loading completes.
In `@frontend/src/services/auth.js`:
- Around line 4-7: fetchCredentials lacks a try/catch and can cause an unhandled
rejection; wrap the api.get call in a try/catch inside fetchCredentials, catch
any network/error, log the error (e.g., console.error or existing logger) and
return the same fallback value the other functions in this module use (e.g.,
null or an empty object) so callers won’t receive an unhandled rejection —
update the function name fetchCredentials accordingly and keep the returned
shape consistent with other auth service functions.
---
Outside diff comments:
In `@backend/routes/auth.js`:
- Around line 237-260: The POST /reset-password handler calls user.setPassword
(in the User.findOne result) which no longer exists because
passport-local-mongoose was removed, causing a TypeError; fix by replacing the
setPassword call with direct assignment user.password = password and await
user.save() (the userSchema pre-save hook will hash the password), and wrap the
DB operations (User.findOne, user.save) and jwt.verify in a try/catch to
properly handle and return DB or verification errors instead of letting
unhandled rejections crash the route.
In `@backend/routes/profile.js`:
- Around line 109-111: Remove the debug console.log statements that print
sensitive request data: do not log req.body.updatedDetails or req.body.userId in
production; instead either delete the two console.log lines or replace them with
a non-sensitive, structured log (e.g., log only the request trace ID or userId
hash) inside the route handler that processes updatedDetails (refer to the
variables userId and updatedDetails in the profile update handler). Ensure no
personal_info fields (name, email, phone, dob) are ever output to logs.
- Line 220: Remove the dead assignment to user.updated_at (snake_case) — the
Mongoose schema uses timestamps: true and manages updatedAt (camelCase)
automatically, so the line setting user.updated_at should be deleted; locate the
code around the user object update in the route handler (the line with
user.updated_at = new Date()) and remove it, relying on the model's save() or
update call to set updatedAt instead.
- Around line 225-229: The response currently returns the full Mongoose document
as updatedStudent (variable user), which includes the hashed password; convert
the document to a plain object (e.g., via user.toObject() or user.toJSON()) or
ensure you fetched it with .select('-password'), remove or unset the password
property on that plain object (delete obj.password or set it to undefined), and
return the sanitized object as updatedStudent in the res.status(200).json(...)
response so no password is leaked.
---
Duplicate comments:
In `@backend/config/passportConfig.js`:
- Around line 65-95: The LocalStrategy callback uses loginValidate which
currently enforces an `@iitbhilai.ac.in` domain and therefore blocks
legacy/non-IIT usernames; update the Zod schema referenced by loginValidate in
backend/utils/authValidate.js to permit existing non-IIT usernames (either
remove the domain regex, allow other domains, or accept plain usernames) and
re-run validation so passport.use(new LocalStrategy(...)) won't reject those
accounts; also add a one-time DB audit script to find User records where
username does not match /@iitbhilai\.ac\.in$/ so you can review and migrate/flag
legacy accounts before deploying.
In `@backend/controllers/certificateController.js`:
- Around line 44-48: The code calls Position.findById(...) and immediately
accesses position._id before verifying the result, which can cause an NPE;
update the certificateController.js code around Position.findById and the
console.log so you first check if (position) (or if (!position) return
res.status(403)...), and only then access or log position._id; alternatively
move or remove the console.log(position._id) to after the null-check to ensure
position is not null before dereferencing.
- Around line 105-115: The code accesses presidentHolder.user_id and
presidentObj._id without null checks; update the president resolution flow
(around PositionHolder.findOne and User.findById) to first check that
presidentHolder is non-null before reading .user_id (only compute presidentId if
presidentHolder exists and return/res.status(404|500) early if not), then call
User.findById(presidentId) and only access or log presidentObj._id after
verifying presidentObj is non-null (move or remove the console.log of
presidentObj._id until after the null check); reference PositionHolder.findOne,
presidentHolder, presidentId, User.findById, and presidentObj when making these
changes.
- Around line 121-124: zodObjectId.safeParse(uid) returns an object with a
success boolean, so checking `if (!validation)` never detects invalid IDs;
update the code in the certificateController to inspect `validation.success` (or
use `validation.success === false`) and return `{ uid, ok: false, reason:
"Invalid ID" }` when false, or use the validated value from `validation.data`
when success before calling `User.findById`, so invalid IDs don't fall through
and cause a CastError.
In `@backend/index.js`:
- Around line 83-89: The connectDB function currently swallows connection
failures in its catch block (it only console.error's and returns), so modify the
catch in connectDB to terminate the process after logging the error — e.g.,
after the existing console.error call in the catch of connectDB, add
process.exit(1) so the IIFE awaiting connectDB() fails fast and the server does
not start without a DB connection.
In `@backend/routes/onboarding.js`:
- Around line 7-9: The handler for router.put currently destructures Program
from req.body and then calls Program.trim(), which will throw if Program is
undefined; update the router.put async handler to validate and guard these
inputs before using them: check that Program (and other required fields like
add_year, discipline, mobile_no if needed) exists and is a string (or coerce via
String(Program) or use (Program || "").trim()), and return a 4xx response when a
required field is missing; locate the router.put(...) function and the code that
calls Program.trim() and replace the direct trim call with a safe guarded trim
or explicit presence check and error response.
In `@frontend/src/Components/Auth/Login.jsx`:
- Line 26: In Login.jsx inside the Login component where toast.success("Login
successful ") is called, remove the trailing space from the message so it
becomes "Login successful"; update the toast.success invocation accordingly to
avoid the accidental extra whitespace in the UI notification.
- Around line 22-27: The code assumes loginUser(...) always returns an object,
but frontend/src/services/auth.js can return null on errors, so guard against a
null response before accessing response.success; in the Login component wrap the
await loginUser(email, password) result with a null check (e.g., if (!response)
{ toast.error(...) ; return; }) or treat response as falsy and handle
error/logging appropriately, only calling handleLogin(response.data),
toast.success, and navigate when response && response.success are true; ensure
you reference the loginUser call, the response variable, and the
handleLogin/navigate usage when applying the fix.
---
Nitpick comments:
In `@backend/config/passportConfig.js`:
- Around line 24-29: The code still uses profile.emails[0].value instead of the
previously extracted email variable; update the Google OAuth guard to use the
extracted email (email) everywhere for consistency and clarity—specifically
replace any remaining profile.emails[0].value usages in the block that calls
isIITBhilaiEmail and the console.log/done message so isIITBhilaiEmail(email) and
console.log("Google OAuth blocked for:", email) / done(null, false, { message:
... , email }) (or include email in the message) are used, keeping the existing
guard that ensures email is non-null.
In `@backend/routes/events.js`:
- Around line 444-477: Remove the development console.log debug block from the
PUT '/:eventId' route handler in backend/routes/events.js (the section that logs
eventId, updates, eventBefore, event AFTER update, etc.), and replace it with
minimal, non-sensitive logging: log only the operation outcome and the eventId
(or use the existing app logger at debug level) rather than JSON-stringifying
full documents or request bodies; keep the Event.findByIdAndUpdate(..., updates,
{ new: true, runValidators: true }) logic and error handling unchanged, and
ensure any logging does not include sensitive fields from the "event" or
"updates" objects.
In `@frontend/src/Components/Auth/Login.jsx`:
- Around line 1-9: The import list in Login.jsx has duplicate imports from
react-router-dom and an unused useContext; update the top imports by removing
the unused useContext from the React import and merging Link into the existing
react-router-dom import that already brings in useNavigate (i.e., import {
useNavigate, Link } from "react-router-dom"), leaving useAdminContext and other
imports unchanged so there are no duplicate or unused imports.
In `@frontend/src/Components/Auth/Register.jsx`:
- Around line 158-171: Remove the dead commented-out Google sign-up block in the
Register component (the commented <Link> ... <button> ... <GoogleIcon /> block)
so the file is clean; also remove any now-unused imports related to Link or
GoogleIcon in frontend/src/Components/Auth/Register.jsx (e.g., React Router's
Link or GoogleIcon) to avoid unused-import warnings.
In `@frontend/src/Components/Auth/RoleRedirect.jsx`:
- Line 1: The import of useContext in RoleRedirect.jsx is dead; remove
useContext from the import statement so the line reads only import React (or
import React, { ... } if other hooks remain), since useAdminContext() (used in
this file) wraps useContext internally; update the import of React accordingly
and ensure no other references to useContext remain in the file (useAdminContext
is the single context access point).
| const id = req.user.id; | ||
| const user = await User.findById(id); | ||
| if (!user) { | ||
| return res.status(404).json({ messge: "Invalid data (User not found)" }); |
Contributor
There was a problem hiding this comment.
Typo in response key: "messge" → "message".
🐛 Proposed fix
- return res.status(404).json({ messge: "Invalid data (User not found)" });
+ return res.status(404).json({ message: "Invalid data (User not found)" });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/certificateController.js` at line 15, The JSON response
in certificateController.js uses a misspelled key "messge" in the error return
(the res.status(404).json call); change the key to "message" so the error
payload is { message: "Invalid data (User not found)" } to ensure consumers
receive the standard "message" field.
Comment on lines
6
to
+9
| const latestEvents = await Event.find({}) | ||
| .sort({updated_at: -1}) | ||
| .limit(4) | ||
| .select('title updated_at schedule.venue status'); | ||
| .select('title updatedAt schedule.venue status'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Event schema and model files
fd -t f -e js | grep -iE "(event|schema)" | head -20Repository: OpenLake/Student_Database_COSA
Length of output: 541
🏁 Script executed:
# Search for the Event model definition and timestamp configuration
rg -n "timestamps|updated_at|updatedAt" --type js -A2 -B2Repository: OpenLake/Student_Database_COSA
Length of output: 4841
🏁 Script executed:
# Get more context from eventControllers.js around the lines mentioned
fd -t f -name "*eventController*" -o -name "*eventcontroller*"Repository: OpenLake/Student_Database_COSA
Length of output: 304
🏁 Script executed:
cat -n backend/models/eventSchema.js | tail -20Repository: OpenLake/Student_Database_COSA
Length of output: 629
🏁 Script executed:
cat -n backend/controllers/eventControllers.js | head -30Repository: OpenLake/Student_Database_COSA
Length of output: 1243
Sort field updated_at doesn't match the Mongoose timestamp field updatedAt.
Lines 9 and 17 both reference updatedAt (Mongoose timestamps field), but the sort on line 7 uses the snake_case form updated_at, which doesn't exist on the document. The sort is silently a no-op.
🐛 Proposed fix
- .sort({updated_at: -1})
+ .sort({updatedAt: -1})📝 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 latestEvents = await Event.find({}) | |
| .sort({updated_at: -1}) | |
| .limit(4) | |
| .select('title updated_at schedule.venue status'); | |
| .select('title updatedAt schedule.venue status'); | |
| const latestEvents = await Event.find({}) | |
| .sort({updatedAt: -1}) | |
| .limit(4) | |
| .select('title updatedAt schedule.venue status'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/eventControllers.js` around lines 6 - 9, The sort call on
Event.find uses the wrong field name 'updated_at' so it doesn't sort; update the
sort to use the Mongoose timestamps field 'updatedAt' (e.g., change
.sort({updated_at: -1}) to .sort({updatedAt: -1}) for latestEvents) and scan for
any other .sort calls or selects in this controller that reference 'updated_at'
to similarly switch them to 'updatedAt' to ensure sorting works as intended.
Comment on lines
+11
to
+13
| if(!latestEvents){ | ||
| return res.status(404).json({message: "No events are created"}); | ||
| } |
Contributor
There was a problem hiding this comment.
Event.find() always returns an array — this 404 guard is dead code.
Mongoose's .find() resolves to [] when no documents exist, never null or undefined. !latestEvents is always false after a successful query, so a request with no events returns 200 [] instead of the intended 404.
🐛 Proposed fix
- if(!latestEvents){
- return res.status(404).json({message: "No events are created"});
- }
+ if(!latestEvents || latestEvents.length === 0){
+ return res.status(404).json({message: "No events are created"});
+ }📝 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(!latestEvents){ | |
| return res.status(404).json({message: "No events are created"}); | |
| } | |
| if(!latestEvents || latestEvents.length === 0){ | |
| return res.status(404).json({message: "No events are created"}); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/controllers/eventControllers.js` around lines 11 - 13, The 404 guard
is using `if (!latestEvents)` but `Event.find()` always returns an array, so
replace that check with a length check (e.g., `if (latestEvents.length === 0)`)
in the controller handling the `Event.find()` result (look for `latestEvents`
and the `Event.find()` call in the event controller function) so an empty result
returns the intended 404 JSON response instead of 200 with an empty array.
Comment on lines
+48
to
+52
| store: MongoStore.create({ | ||
| mongoUrl: process.env.MONGODB_URI, | ||
| ttl: 60*60*1000, | ||
| collectionName: "sessions" | ||
| }), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/index.js | sed -n '40,60p'Repository: OpenLake/Student_Database_COSA
Length of output: 825
🌐 Web query:
connect-mongo ttl parameter unit seconds documentation
💡 Result:
In connect-mongo, the ttl option is specified in seconds (default 1209600, i.e., 14 days). The docs describe it as “maximum lifetime (in seconds)” and examples set it as 14 * 24 * 60 * 60. [1], [2]
ttl is in seconds but is set to a millisecond value — sessions will persist for ~41.7 days.
The connect-mongo ttl parameter expects seconds. With ttl: 60*60*1000 (3,600,000 seconds), sessions persist for approximately 41.7 days in MongoDB, while the cookie's maxAge is 3,600,000 milliseconds (1 hour). This mismatch causes expired session cookies to remain in the database, wasting storage and keeping stale session data accessible.
Proposed fix
store: MongoStore.create({
mongoUrl: process.env.MONGODB_URI,
- ttl: 60*60*1000,
+ ttl: 60 * 60, // 3600 seconds = 1 hour (matches cookie maxAge)
collectionName: "sessions"
}),📝 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
| store: MongoStore.create({ | |
| mongoUrl: process.env.MONGODB_URI, | |
| ttl: 60*60*1000, | |
| collectionName: "sessions" | |
| }), | |
| store: MongoStore.create({ | |
| mongoUrl: process.env.MONGODB_URI, | |
| ttl: 60 * 60, // 3600 seconds = 1 hour (matches cookie maxAge) | |
| collectionName: "sessions" | |
| }), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/index.js` around lines 48 - 52, The ttl passed to MongoStore.create
is using milliseconds (60*60*1000) but connect-mongo expects seconds, causing
sessions to live ~41 days; update the ttl to be in seconds (e.g., 60*60) or
compute it from the session cookie maxAge (Math.floor(session.cookie.maxAge /
1000) or use the same constant) so ttl and cookie maxAge match; locate the
MongoStore.create call and replace the ttl expression accordingly to ensure
sessions expire from the DB when the cookie does.
| ttl: 60*60*1000, | ||
| collectionName: "sessions" | ||
| }), | ||
| name: "token" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's check the structure of the backend directory
echo "=== Backend directory structure ==="
find backend -type f -name "*.js" | head -20Repository: OpenLake/Student_Database_COSA
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Run the provided verification script
rg -n "cookies\|token\|req\.cookies" backend/middlewares/isAuthenticated.js -A2 -B2Repository: OpenLake/Student_Database_COSA
Length of output: 56
🏁 Script executed:
#!/bin/bash
# Check the context around line 53 in backend/index.js
echo "=== Context around line 53 in backend/index.js ==="
sed -n '40,70p' backend/index.jsRepository: OpenLake/Student_Database_COSA
Length of output: 1025
🏁 Script executed:
#!/bin/bash
# Search for where jwtIsAuthenticated is used
echo "=== Usage of jwtIsAuthenticated ==="
rg "jwtIsAuthenticated" backend -n -A1 -B1Repository: OpenLake/Student_Database_COSA
Length of output: 789
🏁 Script executed:
#!/bin/bash
# Read the entire isAuthenticated.js file to see the implementation
cat -n backend/middlewares/isAuthenticated.jsRepository: OpenLake/Student_Database_COSA
Length of output: 2903
🏁 Script executed:
#!/bin/bash
# Check the certificate routes to understand the flow
cat -n backend/routes/certificateRoutes.jsRepository: OpenLake/Student_Database_COSA
Length of output: 399
Session cookie named "token" will conflict with JWT-based middleware.
The session middleware is configured with name: "token", which means it sets a cookie called "token" containing a session ID. However, jwtIsAuthenticated in the certificate routes reads req.cookies.token (line 24 of backend/middlewares/isAuthenticated.js) and attempts to verify it as a JWT. When jwt.verify() is called on the session ID string, verification will fail, causing all certificate endpoints to return a 401 Unauthorized error.
Rename the session cookie to avoid the collision:
🐛 Proposed fix
- name: "token"
+ name: "sid"📝 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
| name: "token" | |
| name: "sid" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/index.js` at line 53, The session middleware in backend/index.js sets
the cookie name to "token", which collides with jwtIsAuthenticated in
backend/middlewares/isAuthenticated.js that expects req.cookies.token to be a
JWT; rename the session cookie (e.g., to "sid" or "sessionId") in the session
configuration (the object containing name: "token") and update any server code
that reads that cookie name accordingly so jwtIsAuthenticated continues to read
req.cookies.token for JWTs while the session middleware writes the new cookie
name; ensure no other logic assumes the old "token" cookie name.
| className="w-full bg-[#23659C] text-white py-2 rounded-md flex items-center justify-center space-x-2 hover:opacity-90 font-medium" | ||
| > | ||
| <span>Sign in with Google</span> | ||
| <span>Sign up with Google</span> |
Contributor
There was a problem hiding this comment.
"Sign up with Google" button label is wrong on the Login page.
This is the Login page, so the action is sign-in, not sign-up. The label should read "Sign in with Google" to avoid confusing users.
Proposed fix
- <span>Sign up with Google</span>
+ <span>Sign in with Google</span>📝 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
| <span>Sign up with Google</span> | |
| <span>Sign in with Google</span> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/Login.jsx` at line 136, The Login.jsx page
currently displays the text "Sign up with Google" inside the Google auth button
(the span within the Login component); update that label to "Sign in with
Google" so the action matches the Login page—locate the span/button in the Login
component and replace the displayed string accordingly.
| name: "" | ||
| }) | ||
| const [loading, setLoading] = useState(false); | ||
| const navigate = useNavigate("/"); |
Contributor
There was a problem hiding this comment.
useNavigate does not accept arguments.
useNavigate() in react-router-dom v6 takes no parameters; the "/" argument is silently ignored. The returned navigate function still works, but this is misleading — it looks like a default path is being configured.
Proposed fix
- const navigate = useNavigate("/");
+ const navigate = useNavigate();📝 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 navigate = useNavigate("/"); | |
| const navigate = useNavigate(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/Register.jsx` at line 16, The code incorrectly
calls useNavigate with an argument — change the call to const navigate =
useNavigate(); (in Register.jsx) and, where a default redirect is intended, call
navigate('/') explicitly at the appropriate place (e.g., after successful
registration in the submit/handleRegister function) rather than passing "/" into
useNavigate; ensure any existing tests or usages reference navigate('/') and not
the hook argument.
Comment on lines
+40
to
+54
| let errorMessage = ""; | ||
| const respData = response && response.data; | ||
| if (respData) { | ||
| const msg = respData.message; | ||
| if (Array.isArray(msg)) { | ||
| errorMessage = msg.join(". "); | ||
| } else if (typeof msg === "string") { | ||
| errorMessage = msg; | ||
| } else if (msg && msg.message) { | ||
| errorMessage = msg.message; | ||
| } | ||
| } else if (response && response.status) { | ||
| errorMessage = response.statusText; | ||
| } | ||
| toast.error(errorMessage); |
Contributor
There was a problem hiding this comment.
Empty error toast when response is undefined (network error).
registerUser in frontend/src/services/auth.js (line 30) returns error.response, which is undefined for network-level failures (no HTTP response). In that case, neither branch of the error-extraction logic is entered, errorMessage stays "", and toast.error("") fires silently with no visible message.
🐛 Proposed fix
+ if (!response) {
+ toast.error("Registration failed. Network error, please try again.");
+ return;
+ }
// success response is the full axios response
if (response && response.status === 200 && response.data && response.data.success) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/Register.jsx` around lines 40 - 54, The toast
shows empty text when the caught error has no HTTP response (network failure)
because the existing logic only inspects response/response.data; update the
error-extraction in Register.jsx (the block using response, respData,
errorMessage and calling toast.error) to handle the case where response is
undefined by falling back to the original error.message (or a generic "Network
error" string) returned by registerUser in frontend/src/services/auth.js so
toast.error is always given a non-empty, user-facing message.
Comment on lines
+10
to
12
| if (Object.keys(isUserLoggedIn).length === 0) { | ||
| return <Navigate to="/login" replace />; | ||
| } |
Contributor
There was a problem hiding this comment.
Object.keys(null) throws a TypeError when isUserLoggedIn is null.
isUserLoggedIn is initialized as null in useAuth.js (line 4), so once isLoading flips to false for an unauthenticated user, this call crashes the component before the redirect can happen.
🐛 Proposed fix
- if (Object.keys(isUserLoggedIn).length === 0) {
+ if (!isUserLoggedIn || Object.keys(isUserLoggedIn).length === 0) {
return <Navigate to="/login" replace />;
}📝 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 (Object.keys(isUserLoggedIn).length === 0) { | |
| return <Navigate to="/login" replace />; | |
| } | |
| if (!isUserLoggedIn || Object.keys(isUserLoggedIn).length === 0) { | |
| return <Navigate to="/login" replace />; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/Components/Auth/RoleRedirect.jsx` around lines 10 - 12, The
current check Object.keys(isUserLoggedIn) crashes when isUserLoggedIn is null;
update the guard in RoleRedirect.jsx to handle null/undefined (e.g., use
Object.keys(isUserLoggedIn ?? {}).length === 0 or simply if (!isUserLoggedIn)
return <Navigate to="/login" replace />) and ensure you consider the isLoading
flag from useAuth.js so you only redirect after loading completes.
Comment on lines
4
to
7
| export async function fetchCredentials() { | ||
| const response = await api.get(`/auth/fetchAuth`); | ||
| const response = await api.get("/auth/fetchAuth"); | ||
| return response.data; | ||
| } |
Contributor
There was a problem hiding this comment.
fetchCredentials has no error handling — unhandled rejection on failure.
Every other function in this file wraps the API call in try/catch. fetchCredentials is likely called during app bootstrap (e.g., to hydrate auth state); an uncaught network error here will propagate as an unhandled promise rejection and can break the entire initialization flow.
🐛 Proposed fix
export async function fetchCredentials() {
- const response = await api.get("/auth/fetchAuth");
- return response.data;
+ try {
+ const response = await api.get("/auth/fetchAuth");
+ return response.data;
+ } catch (error) {
+ console.error("Failed to fetch credentials:", error.response?.data || error.message);
+ return null;
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/services/auth.js` around lines 4 - 7, fetchCredentials lacks a
try/catch and can cause an unhandled rejection; wrap the api.get call in a
try/catch inside fetchCredentials, catch any network/error, log the error (e.g.,
console.error or existing logger) and return the same fallback value the other
functions in this module use (e.g., null or an empty object) so callers won’t
receive an unhandled rejection — update the function name fetchCredentials
accordingly and keep the returned shape consistent with other auth service
functions.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
name: "Pull Request"
about: Propose and submit changes to the project for review
title: "PR: Refactor login/register and add Zod validation"
labels: ""
assignees: harshitap1305, sakshi1755
Changes Introduced
configfolder for cleaner project structurelogin®isterroutes work as intendedUser.register()and plugins)"iitbhilai.ac.in"directly in Zodutilsfolder for modularityWhy This Change?
User.register()implementation was brokenScreenshots
Testing
npm test)Documentation Updates
README.mdwith new instructionsChecklist
refactor/login-register)Deployment Notes
Additional Notes
Summary by CodeRabbit
Release Notes
New Features
Improvements
Refactor