Refactor/login register #222
Open
Conversation
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
|
@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
WalkthroughThis PR refactors authentication from Passport LocalStrategy to JWT tokens with bcrypt, introduces Google OAuth with IIT Bhilai email domain validation, reorganizes monolithic schema into separate model files, adds certificate batch management with multi-level approval workflows, and updates route imports for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server as Auth Handler
participant UserDB as User DB
participant BCrypt as BCrypt
participant JWT as JWT Signer
Client->>Server: POST /login (username, password)
Server->>Server: Validate input with loginValidate
alt Invalid Input
Server-->>Client: 400 Validation Error
end
Server->>UserDB: Query user by username
alt User Not Found
UserDB-->>Server: null
Server-->>Client: 401 Invalid credentials
end
UserDB-->>Server: User document
Server->>BCrypt: Verify password hash
alt Password Mismatch
BCrypt-->>Server: false
Server-->>Client: 401 Invalid credentials
end
BCrypt-->>Server: true
Server->>JWT: Sign token (user_id, 30 min expiry)
JWT-->>Server: JWT token
Server->>Server: Set httpOnly cookie
Server-->>Client: 200 Login success + cookie
sequenceDiagram
participant Client
participant GoogleOAuth as Google OAuth
participant Server as Callback Handler
participant UserDB as User DB
Client->>GoogleOAuth: Request Google auth
GoogleOAuth-->>Client: Redirect to Google login
Client->>GoogleOAuth: Authenticate
GoogleOAuth-->>Server: Callback with profile
Server->>Server: Validate IIT Bhilai email domain
alt Non-IIT Email
Server-->>Client: 403 Invalid domain
end
Server->>UserDB: Query user by email
alt User Exists
UserDB-->>Server: User document
else User Not Found
Server->>UserDB: Create new user (STUDENT role, google strategy)
UserDB-->>Server: New user document
end
Server->>Server: Serialize user
Server-->>Client: 200 Auth success
sequenceDiagram
participant Client
participant CertController as Certificate Controller
participant UserDB as User DB
participant OrgDB as Organization DB
participant ApproverDB as Approver Lookup
participant CertDB as Certificate DB
Client->>CertController: POST /api/certificate-batches (batch data)
CertController->>CertController: Validate with jwtIsAuthenticated
CertController->>UserDB: Fetch initiator user
alt User Not Found
UserDB-->>CertController: null
CertController-->>Client: 404 User not found
end
CertController->>CertController: Check role = CLUB_COORDINATOR
alt Unauthorized
CertController-->>Client: 403 Forbidden
end
CertController->>CertController: Validate batch schema (title, unit_id, users)
alt Validation Fails
CertController-->>Client: 400 Validation errors
end
CertController->>OrgDB: Fetch organization unit (club)
CertController->>CertController: Verify coordinator in unit
alt Not coordinator of club
CertController-->>Client: 403 Forbidden
end
CertController->>OrgDB: Validate parent (Council)
CertController->>ApproverDB: Resolve General Secretary
CertController->>ApproverDB: Resolve President
alt Approvers Not Found
ApproverDB-->>CertController: null
CertController-->>Client: 500 Approvers not found
end
CertController->>CertController: Validate each user ID
alt Invalid user ID
CertController-->>Client: 400 Invalid user
end
CertController->>CertDB: Create CertificateBatch
CertDB-->>CertController: Batch created
CertController-->>Client: 200 Batch created with details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ 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, },
Comment on lines
+14
to
+21
| async (accessToken, refreshToken, profile, done) => { | ||
| // Check if the user already exists in your database | ||
| if (!isIITBhilaiEmail(profile.emails[0].value)) { | ||
| console.log("Google OAuth blocked for: ", profile.emails[0].value); | ||
| return done(null, false, { | ||
| message: "Only @iitbhilai.ac.in emails are allowed.", | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
passport-google-oauth20 profile.emails availability scopes required
💡 Result:
In passport-google-oauth20, profile.emails is not guaranteed to be present unless you request an email-related scope.
-
To get
profile.emails, request at least theemailscope (commonly alongsideprofile):passport.authenticate('google', { scope: ['profile', 'email'] })
Google documents
emailas “See your primary Google Account email address.” [1] -
In Passport’s normalized profile, emails (when provided by the provider) are exposed as an array
profile.emails, with items like{ value: 'user@example.com' }(so you typically readprofile.emails[0].value). [2] -
If you don’t include
email(or an equivalent likehttps://www.googleapis.com/auth/userinfo.email),profile.emailsmay beundefined. [1][3]
Sources: Google OAuth/OpenID scopes [1], Passport normalized profile fields [2], Passport scope usage reference [3].
🌐 Web query:
Google OAuth strategy profile emails missing undefined passport
💡 Result:
This usually happens for one of these Passport/Google OAuth gotchas:
- You’re not actually requesting the
emailscope at login time (it gets overridden).
Even if you setscope: ['email','profile']in the strategy, a laterpassport.authenticate('google', { scope: ... })on your route overrides it. If that route only asks forprofile, Google won’t return emails, soprofile.emailsisundefined. Fix by ensuring the auth route includesemailtoo. (stackoverflow.com)
app.get('/auth/google',
passport.authenticate('google', { scope: ['profile', 'email'] })
);-
You’re using the wrong strategy/package (or wrong verify callback signature), so
profileisn’t what you think.
A common case is importingGoogleStrategyfrompassport-google-oidcbut using apassport-google-oauth*-style callback like(accessToken, refreshToken, profile, done). Thenprofilecan look like a string/undefined andprofile.emailsblows up. Use the correct verify signature for the package you installed, or switch topassport-google-oauth20if you want the OAuth2-styleprofile. (stackoverflow.com) -
Use the full userinfo scopes if needed.
If['profile','email']still doesn’t populate emails in your setup, request the explicit userinfo scopes. (stackoverflow.com)
scope: [
'https://www.googleapis.com/auth/userinfo.profile',
'https://www.googleapis.com/auth/userinfo.email',
]If you paste your passport.use(...) and your /auth/google route, I can point to the exact line where the scope/signature is going wrong.
Citations:
- 1: https://stackoverflow.com/questions/75076761/google-oauth-2-0-email-missing-from-profile?utm_source=openai
- 2: https://stackoverflow.com/questions/74996375/passport-js-googlestrategy-not-working-getting-error-cannot-read-properties-of
- 3: https://stackoverflow.com/questions/47719423/passport-google-oauth2-does-not-return-email?utm_source=openai
🏁 Script executed:
# Check if there are any other references to profile.emails in the codebase
rg "profile\.emails" --type jsRepository: OpenLake/Student_Database_COSA
Length of output: 1005
🏁 Script executed:
# Get the full content of backend/config/passportConfig.js to see scope initialization
cat -n backend/config/passportConfig.js | head -60Repository: OpenLake/Student_Database_COSA
Length of output: 2438
🏁 Script executed:
# Also check backend/models/passportConfig.js to see if there's a pattern
cat -n backend/models/passportConfig.js | head -60Repository: OpenLake/Student_Database_COSA
Length of output: 2424
🏁 Script executed:
# Check if there's any route that calls passport.authenticate with Google strategy
# Look for the /auth/google route or similar
rg "passport\.authenticate.*google" -A 2 -B 2 --type jsRepository: OpenLake/Student_Database_COSA
Length of output: 550
🏁 Script executed:
# Also check if there's any scope configuration elsewhere
rg "scope.*email" --type jsRepository: OpenLake/Student_Database_COSA
Length of output: 164
Add defensive check for missing Google profile emails.
Although the /auth/google route correctly requests the email scope, the code accesses profile.emails[0].value on lines 16, 17, 23, 31, and 36 without checking if the emails array exists. If the OAuth configuration is misconfigured or Google doesn't populate emails, this causes a crash. The code already guards profile.photos (line 38)—apply the same defensive pattern to emails:
🛡️ Suggested guard
async (accessToken, refreshToken, profile, done) => {
// Check if the user already exists in your database
- if (!isIITBhilaiEmail(profile.emails[0].value)) {
+ const email = profile.emails?.[0]?.value;
+ if (!email) {
+ return done(null, false, { message: "Email not available from Google." });
+ }
+ if (!isIITBhilaiEmail(email)) {
- console.log("Google OAuth blocked for: ", profile.emails[0].value);
+ console.log("Google OAuth blocked for: ", email);
return done(null, false, {
message: "Only `@iitbhilai.ac.in` emails are allowed.",
});
}
try {
- const user = await User.findOne({ username: profile.emails[0].value });
+ const user = await User.findOne({ username: email });
...
- username: profile.emails[0].value,
+ username: email,
...
- email: profile.emails[0].value,
+ email: email,🤖 Prompt for AI Agents
In `@backend/config/passportConfig.js` around lines 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.
Comment on lines
+54
to
+62
| passport.serializeUser((user, done) => { | ||
| done(null, user); | ||
| }); | ||
|
|
||
| passport.deserializeUser(async (userKey, done) => { | ||
| try { | ||
| let user = await User.findById(userKey._id); | ||
| done(null, user); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Locate and examine the passport config file
find . -type f -name "passportConfig.js" | head -5Repository: OpenLake/Student_Database_COSA
Length of output: 143
🌐 Web query:
passport.js serializeUser deserializeUser best practices store user id
💡 Result:
Best practice: serialize the minimum stable identifier (usually the database user id) into the session, and in deserializeUser load the user (or a safe subset) from your data store.
What to store in serializeUser
- Common/default: store only
user.idto keep the session small, and query the DB on requests. (passportjs.org) - Passport’s recommended tradeoff: store only what you need on every request (e.g.,
idplususername/photoif shown on every page) to reduce repeated DB queries. (passportjs.org) - Avoid storing the whole user object: it duplicates mutable/sensitive data and can become stale (e.g., username/permissions changed but session still has old values). (stackoverflow.com)
What to do in deserializeUser
- Treat
serializeUseranddeserializeUseras inverses:serializeUser(user) -> key,deserializeUser(key) -> user. (stackoverflow.com) deserializeUserruns on subsequent requests (viapassport.session()), attaches the result toreq.user. (stackoverflow.com)- Handle errors and “user not found” cleanly (e.g., return
falseif the id no longer exists). (stackoverflow.com)
Typical pattern (ID-only session)
passport.serializeUser((user, done) => done(null, user.id));
passport.deserializeUser(async (id, done) => {
try {
const user = await User.findById(id); // load from DB
if (!user) return done(null, false);
return done(null, user);
} catch (err) {
return done(err);
}
});This matches Passport’s “minimize session data, query as needed” example. (passportjs.org)
Citations:
- 1: https://www.passportjs.org/concepts/authentication/sessions/?utm_source=openai
- 2: https://www.passportjs.org/concepts/authentication/sessions/?utm_source=openai
- 3: https://stackoverflow.com/questions/55445075/why-should-i-only-store-user-id-with-passport-serializeuser?utm_source=openai
- 4: https://stackoverflow.com/questions/44255812/session-issues-with-passportjs?utm_source=openai
- 5: https://stackoverflow.com/questions/28691215/when-is-the-serialize-and-deserialize-passport-method-called-what-does-it-exact?utm_source=openai
- 6: https://stackoverflow.com/questions/44255812/session-issues-with-passportjs?utm_source=openai
- 7: https://www.passportjs.org/concepts/authentication/sessions/?utm_source=openai
🏁 Script executed:
cat -n backend/config/passportConfig.js | sed -n '50,65p'Repository: OpenLake/Student_Database_COSA
Length of output: 439
Serialize only the user ID instead of the entire user object.
Lines 54–61 serialize the full user object into the session, which unnecessarily stores sensitive data and bloats sessions. Per Passport.js best practices, serialize only the user ID and fetch the full user on subsequent requests in deserializeUser.
🛠️ Suggested change
passport.serializeUser((user, done) => {
- done(null, user);
+ done(null, user._id.toString());
});
passport.deserializeUser(async (userKey, done) => {
try {
- let user = await User.findById(userKey._id);
+ let user = await User.findById(userKey);
done(null, user);
} catch (err) {
done(err);
}
});📝 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
| passport.serializeUser((user, done) => { | |
| done(null, user); | |
| }); | |
| passport.deserializeUser(async (userKey, done) => { | |
| try { | |
| let user = await User.findById(userKey._id); | |
| done(null, user); | |
| } catch (err) { | |
| passport.serializeUser((user, done) => { | |
| done(null, user._id.toString()); | |
| }); | |
| passport.deserializeUser(async (userKey, done) => { | |
| try { | |
| let user = await User.findById(userKey); | |
| done(null, user); | |
| } catch (err) { | |
| done(err); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@backend/config/passportConfig.js` around lines 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.
…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
+10
to
+148
| async function createBatch(req, res) { | ||
| //console.log(req.user); | ||
| const id = req.user.id; | ||
| const user = await User.findById(id); | ||
| if (!user) { | ||
| return res.status(404).json({ messge: "Invalid data (User not found)" }); | ||
| } | ||
|
|
||
| if (user.role && user.role !== "CLUB_COORDINATOR") { | ||
| return res.status(403).json({ message: "Not authorized to perform the task" }); | ||
| } | ||
|
|
||
| //to get user club | ||
| // positionHolders({user_id: id}) -> positions({_id: position_id}) -> organizationalUnit({_id: unit_id}) -> unit_id = "Club name" | ||
| const { title, unit_id, commonData, template_id, users } = req.body; | ||
| const validation = validateBatchSchema.safeParse({ | ||
| title, | ||
| unit_id, | ||
| commonData, | ||
| template_id, | ||
| users, | ||
| }); | ||
|
|
||
| if (!validation.success) { | ||
| let errors = validation.error.issues.map(issue => issue.message); | ||
| return res.status(400).json({ message: errors }); | ||
| } | ||
|
|
||
| // Get coordinator's position and unit | ||
| const positionHolder = await PositionHolder.findOne({ user_id: id }); | ||
| if (!positionHolder) { | ||
| return res.status(403).json({ message: "You are not part of any position in a unit" }); | ||
| } | ||
|
|
||
| const position = await Position.findById(positionHolder.position_id); | ||
| console.log(position._id); | ||
| if (!position) { | ||
| return res.status(403).json({ message: "Your position is invalid" }); | ||
| } | ||
|
|
||
| const userUnitId = position.unit_id.toString(); | ||
| if (userUnitId !== unit_id) { | ||
| return res | ||
| .status(403) | ||
| .json({ | ||
| message: | ||
| "You are not authorized to initiate batches outside of your club", | ||
| }); | ||
| } | ||
|
|
||
| //const clubId = unit_id; | ||
| // Ensure unit_id is a Club | ||
| const unitObj = await OrganizationalUnit.findById(unit_id); | ||
| if (!unitObj || unitObj.type !== "Club") { | ||
| return res | ||
| .status(403) | ||
| .json({ message: "Invalid Data: unit is not a Club" }); | ||
| } | ||
| console.log(unitObj._id); | ||
|
|
||
| // Get council (parent unit) and ensure it's a Council | ||
| if (!unitObj.parent_unit_id) { | ||
| return res | ||
| .status(403) | ||
| .json({ message: "Invalid Data: club does not belong to a council" }); | ||
| } | ||
| console.log(unitObj.parent_unit_id); | ||
|
|
||
| const councilObj = await OrganizationalUnit.findById(unitObj.parent_unit_id); | ||
| if (!councilObj || councilObj.type !== "Council") { | ||
| return res.status(403).json({ message: "Invalid Data: council not found" }); | ||
| } | ||
|
|
||
| //const councilId = councilObj._id.toString(); | ||
| const presidentOrgUnitId = councilObj.parent_unit_id; | ||
| const category = councilObj.category.toUpperCase(); | ||
|
|
||
| // Resolve General Secretary and President for the council (server-side, tamper-proof) | ||
| const gensecObj = await User.findOne({ role: `GENSEC_${category}` }); | ||
| console.log(gensecObj._id); | ||
|
|
||
| const presidentPosition = await Position.findOne({ | ||
| unit_id: presidentOrgUnitId, | ||
| title: /president/i, | ||
| }); | ||
| if (!presidentPosition) { | ||
| return res | ||
| .status(500) | ||
| .json({ message: "President position not found for council" }); | ||
| } | ||
| console.log(presidentPosition._id); | ||
|
|
||
| const presidentHolder = await PositionHolder.findOne({ | ||
| position_id: presidentPosition._id, | ||
| }); | ||
| const presidentId = presidentHolder.user_id.toString(); | ||
| console.log(presidentId); | ||
| const presidentObj = await User.findById(presidentId); | ||
|
|
||
| console.log(presidentObj._id); | ||
| if (!gensecObj || !presidentObj) { | ||
| return res.status(500).json({ message: "Approvers not found" }); | ||
| } | ||
|
|
||
| const approverIds = [gensecObj._id.toString(), presidentId]; | ||
|
|
||
| const userChecks = await Promise.all( | ||
| users.map(async (uid) => { | ||
| const validation = zodObjectId.safeParse(uid); | ||
| if (!validation) { | ||
| return { uid, ok: false, reason: "Invalid ID" }; | ||
| } | ||
|
|
||
| const userObj = await User.findById(uid); | ||
| if (!userObj) return { uid, ok: false, reason: "User not found" }; | ||
|
|
||
| return { uid, ok: true }; | ||
| }), | ||
| ); | ||
|
|
||
| const invalidData = userChecks.filter((c) => !c.ok); | ||
| if (invalidData.length > 0) { | ||
| return res | ||
| .status(400) | ||
| .json({ message: "Invalid user data sent", details: invalidData }); | ||
| } | ||
|
|
||
| const newBatch = await CertificateBatch.create({ | ||
| title, | ||
| unit_id, | ||
| commonData, | ||
| templateId: template_id, | ||
| initiatedBy: id, | ||
| approverIds, | ||
| users, | ||
| }); | ||
|
|
||
| res.json({ message: "New Batch created successfully", details: newBatch }); | ||
| } |
Contributor
There was a problem hiding this comment.
No try/catch — any database error will crash the request as an unhandled rejection.
Wrap the entire handler body in a try/catch that returns a 500 response, or use an async error-handling wrapper middleware.
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 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.
Comment on lines
+18
to
+20
| if (user.role && user.role !== "CLUB_COORDINATOR") { | ||
| return res.status(403).json({ message: "Not authorized to perform the task" }); | ||
| } |
Contributor
There was a problem hiding this comment.
Authorization check allows users with no role.
user.role && user.role !== "CLUB_COORDINATOR" evaluates to false when user.role is undefined/null, so users without any role bypass this guard. The intent appears to be the opposite — only CLUB_COORDINATOR should be allowed.
Proposed fix
- if (user.role && user.role !== "CLUB_COORDINATOR") {
+ if (user.role !== "CLUB_COORDINATOR") {
return res.status(403).json({ message: "Not authorized to perform the task" });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (user.role && user.role !== "CLUB_COORDINATOR") { | |
| return res.status(403).json({ message: "Not authorized to perform the task" }); | |
| } | |
| if (user.role !== "CLUB_COORDINATOR") { | |
| return res.status(403).json({ message: "Not authorized to perform the task" }); | |
| } |
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 18 - 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.
Comment on lines
+44
to
+48
| const position = await Position.findById(positionHolder.position_id); | ||
| console.log(position._id); | ||
| if (!position) { | ||
| return res.status(403).json({ message: "Your position is invalid" }); | ||
| } |
Contributor
There was a problem hiding this comment.
Null-pointer crash: position._id accessed before the null check.
Line 45 accesses position._id via console.log, but the null check is on line 46. If findById returns null, this will throw a TypeError.
Proposed fix
const position = await Position.findById(positionHolder.position_id);
- console.log(position._id);
if (!position) {
return res.status(403).json({ message: "Your position is invalid" });
}🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 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.
Comment on lines
+88
to
+112
| const gensecObj = await User.findOne({ role: `GENSEC_${category}` }); | ||
| console.log(gensecObj._id); | ||
|
|
||
| const presidentPosition = await Position.findOne({ | ||
| unit_id: presidentOrgUnitId, | ||
| title: /president/i, | ||
| }); | ||
| if (!presidentPosition) { | ||
| return res | ||
| .status(500) | ||
| .json({ message: "President position not found for council" }); | ||
| } | ||
| console.log(presidentPosition._id); | ||
|
|
||
| const presidentHolder = await PositionHolder.findOne({ | ||
| position_id: presidentPosition._id, | ||
| }); | ||
| const presidentId = presidentHolder.user_id.toString(); | ||
| console.log(presidentId); | ||
| const presidentObj = await User.findById(presidentId); | ||
|
|
||
| console.log(presidentObj._id); | ||
| if (!gensecObj || !presidentObj) { | ||
| return res.status(500).json({ message: "Approvers not found" }); | ||
| } |
Contributor
There was a problem hiding this comment.
Multiple null-pointer crashes in approver resolution.
Several values are dereferenced before their null checks:
- Line 89:
gensecObj._idis accessed, but the null check forgensecObjis at line 110. - Line 102-105:
presidentHolderis never null-checked; if no holder exists, line 105 crashes. - Line 109:
presidentObj._idis accessed before the null check at line 110.
Move null checks immediately after each query, and remove the console.log statements that access potentially null objects.
Proposed fix (sketch)
const gensecObj = await User.findOne({ role: `GENSEC_${category}` });
- console.log(gensecObj._id);
+ if (!gensecObj) {
+ return res.status(500).json({ message: "General Secretary not found" });
+ }
const presidentPosition = await Position.findOne({
unit_id: presidentOrgUnitId,
title: /president/i,
});
if (!presidentPosition) {
return res
.status(500)
.json({ message: "President position not found for council" });
}
- console.log(presidentPosition._id);
const presidentHolder = await PositionHolder.findOne({
position_id: presidentPosition._id,
});
+ if (!presidentHolder) {
+ return res.status(500).json({ message: "President holder not found" });
+ }
const presidentId = presidentHolder.user_id.toString();
- console.log(presidentId);
const presidentObj = await User.findById(presidentId);
-
- console.log(presidentObj._id);
- if (!gensecObj || !presidentObj) {
- return res.status(500).json({ message: "Approvers not found" });
+ if (!presidentObj) {
+ return res.status(500).json({ message: "President user not found" });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const gensecObj = await User.findOne({ role: `GENSEC_${category}` }); | |
| console.log(gensecObj._id); | |
| const presidentPosition = await Position.findOne({ | |
| unit_id: presidentOrgUnitId, | |
| title: /president/i, | |
| }); | |
| if (!presidentPosition) { | |
| return res | |
| .status(500) | |
| .json({ message: "President position not found for council" }); | |
| } | |
| console.log(presidentPosition._id); | |
| const presidentHolder = await PositionHolder.findOne({ | |
| position_id: presidentPosition._id, | |
| }); | |
| const presidentId = presidentHolder.user_id.toString(); | |
| console.log(presidentId); | |
| const presidentObj = await User.findById(presidentId); | |
| console.log(presidentObj._id); | |
| if (!gensecObj || !presidentObj) { | |
| return res.status(500).json({ message: "Approvers not found" }); | |
| } | |
| const gensecObj = await User.findOne({ role: `GENSEC_${category}` }); | |
| if (!gensecObj) { | |
| return res.status(500).json({ message: "General Secretary not found" }); | |
| } | |
| const presidentPosition = await Position.findOne({ | |
| unit_id: presidentOrgUnitId, | |
| title: /president/i, | |
| }); | |
| if (!presidentPosition) { | |
| return res | |
| .status(500) | |
| .json({ message: "President position not found for council" }); | |
| } | |
| const presidentHolder = await PositionHolder.findOne({ | |
| position_id: presidentPosition._id, | |
| }); | |
| if (!presidentHolder) { | |
| return res.status(500).json({ message: "President holder not found" }); | |
| } | |
| const presidentId = presidentHolder.user_id.toString(); | |
| const presidentObj = await User.findById(presidentId); | |
| if (!presidentObj) { | |
| return res.status(500).json({ message: "President user not found" }); | |
| } |
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 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).
Comment on lines
+116
to
+128
| const userChecks = await Promise.all( | ||
| users.map(async (uid) => { | ||
| const validation = zodObjectId.safeParse(uid); | ||
| if (!validation) { | ||
| return { uid, ok: false, reason: "Invalid ID" }; | ||
| } | ||
|
|
||
| const userObj = await User.findById(uid); | ||
| if (!userObj) return { uid, ok: false, reason: "User not found" }; | ||
|
|
||
| return { uid, ok: true }; | ||
| }), | ||
| ); |
Contributor
There was a problem hiding this comment.
Validation check is always truthy — invalid IDs are never caught.
zodObjectId.safeParse(uid) returns a result object (e.g. { success: true/false, ... }), which is always truthy. Line 119 should check !validation.success instead of !validation.
Proposed fix
const validation = zodObjectId.safeParse(uid);
- if (!validation) {
+ if (!validation.success) {
return { uid, ok: false, reason: "Invalid ID" };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const userChecks = await Promise.all( | |
| users.map(async (uid) => { | |
| const validation = zodObjectId.safeParse(uid); | |
| if (!validation) { | |
| return { uid, ok: false, reason: "Invalid ID" }; | |
| } | |
| const userObj = await User.findById(uid); | |
| if (!userObj) return { uid, ok: false, reason: "User not found" }; | |
| return { uid, ok: true }; | |
| }), | |
| ); | |
| const userChecks = await Promise.all( | |
| users.map(async (uid) => { | |
| const validation = zodObjectId.safeParse(uid); | |
| if (!validation.success) { | |
| return { uid, ok: false, reason: "Invalid ID" }; | |
| } | |
| const userObj = await User.findById(uid); | |
| if (!userObj) return { uid, ok: false, reason: "User not found" }; | |
| return { uid, ok: true }; | |
| }), | |
| ); |
🤖 Prompt for AI Agents
In `@backend/controllers/certificateController.js` around lines 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.
| const isAuthenticated= require("../middlewares/isAuthenticated"); | ||
|
|
||
| const User = require("../models/userSchema"); | ||
| const isAuthenticated = require("../middlewares/isAuthenticated"); |
Contributor
There was a problem hiding this comment.
Critical: Import mismatch — isAuthenticated will be the exports object, not the middleware function.
The middleware module now exports { isAuthenticated, jwtIsAuthenticated }. Every other route file in this PR uses destructured import const { isAuthenticated } = require(...). This file uses a default-style import, so isAuthenticated here receives the entire exports object. When Express invokes it as middleware, it won't be a function, causing a runtime error on the /fetchAuth route (line 25).
🐛 Proposed fix
-const isAuthenticated = require("../middlewares/isAuthenticated");
+const { isAuthenticated } = require("../middlewares/isAuthenticated");🤖 Prompt for AI Agents
In `@backend/routes/auth.js` at 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.
| status: user.status, | ||
| }; | ||
|
|
||
| const token = jwt.sign(payload, secretKey, { expiresIn: "30m" }); |
Contributor
There was a problem hiding this comment.
Critical: secretKey is not defined — jwt.sign will throw a ReferenceError.
There is no secretKey variable declared or imported anywhere in this file. This will crash the login route at runtime. You likely need to read it from an environment variable.
🐛 Proposed fix — add a declaration near the top of the file (e.g., after line 15)
const bcrypt = require("bcrypt");
+
+const secretKey = process.env.JWT_SECRET_TOKEN;📝 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 token = jwt.sign(payload, secretKey, { expiresIn: "30m" }); | |
| const bcrypt = require("bcrypt"); | |
| const secretKey = process.env.JWT_SECRET_TOKEN; |
🤖 Prompt for AI Agents
In `@backend/routes/auth.js` at 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.
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.
| return res.status(400).json({ error: "ID_No is required" }); | ||
| } | ||
|
|
||
| const user = await user.findOne({ user_id: ID_No }); |
Contributor
There was a problem hiding this comment.
Critical: user.findOne should be User.findOne — variable used before its own declaration.
Line 87 declares const user and simultaneously calls user.findOne(...), which references the not-yet-initialized binding. This will throw a ReferenceError at runtime due to the temporal dead zone. The imported model on line 7 is User (capital U).
🐛 Proposed fix
- const user = await user.findOne({ user_id: ID_No });
+ const user = await User.findOne({ user_id: ID_No });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const user = await user.findOne({ user_id: ID_No }); | |
| const user = await User.findOne({ user_id: ID_No }); |
🧰 Tools
🪛 Biome (2.3.13)
[error] 87-87: This variable is used before its declaration.
The variable is declared here:
(lint/correctness/noInvalidUseBeforeDeclaration)
🤖 Prompt for AI Agents
In `@backend/routes/profile.js` at 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).
| @@ -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.
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