update app.js file for session handing #2677
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughReplaces in-memory/previous session storage with MongoDB-backed sessions via connect-mongo in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Server as Express App
participant Session as express-session
participant Mongo as MongoDB (connect-mongo)
Note right of Session: Session middleware now uses MongoStore (connect-mongo)
User->>Client: HTTP request
Client->>Server: Request (cookie may include session ID)
Server->>Session: middleware checks session
alt session ID present
Session->>Mongo: load session by ID
Mongo-->>Session: session data
else no/expired session
Session->>Mongo: create/store session
Mongo-->>Session: new session record
end
Session-->>Server: attach req.session
Server-->>Client: response (set-session-cookie if created)
Note over Client: cookie.maxAge = 7 days
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
backend/app.js(2 hunks)backend/models/Subscriber.js(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app.js (1)
backend/config/db.js (1)
mongoose(5-13)
🔇 Additional comments (4)
backend/models/Subscriber.js (1)
5-5: LGTM!The blank line improves code readability after the require statements. No functional changes.
backend/app.js (2)
6-6: LGTM!The MongoStore import is correct and follows the standard CommonJS require pattern.
34-34: LGTM!Using an environment variable with a fallback for the session secret is a good security practice.
package.json (1)
18-18: No changes needed:connect-mongo@^5.1.0is the latest stable version and has no reported security advisories.
| store: MongoStore.create({ | ||
| mongoUrl: process.env.MONGO_URI, | ||
| collectionName: 'sessions' // Optional: name of the collection to store sessions | ||
| }), | ||
| cookie: { | ||
| maxAge: 1000 * 60 * 60 * 24 * 7 // Optional: 1 week | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Critical: Environment variable mismatch and missing security configurations.
Several issues identified:
-
Environment variable inconsistency: Line 39 uses
process.env.MONGO_URI, but the related code snippet frombackend/config/db.jsshowsconfig.mongoURIis used. Verify these are the same variable or if there's a mismatch. -
Missing error handling: No error handling if
MongoStore.create()fails. This could cause the application to crash silently. -
Missing cookie security options: The cookie configuration lacks essential security flags:
httpOnly: true- prevents client-side JS access to the cookiesecure: true- ensures cookie is only sent over HTTPS (should be conditional on production)sameSite- protects against CSRF attacks
-
No session TTL: Consider adding a
ttloption to control how long sessions persist in MongoDB (independent of cookie expiration). -
No store error handlers: MongoDB connection issues won't be caught.
First, verify the environment variable naming:
Apply this diff to add error handling and security options:
// 2. CONFIGURE THE MONGOSTORE
store: MongoStore.create({
mongoUrl: process.env.MONGO_URI,
- collectionName: 'sessions' // Optional: name of the collection to store sessions
+ collectionName: 'sessions',
+ ttl: 1000 * 60 * 60 * 24 * 7 // Match cookie maxAge (7 days)
}),
cookie: {
- maxAge: 1000 * 60 * 60 * 24 * 7 // Optional: 1 week
+ maxAge: 1000 * 60 * 60 * 24 * 7, // 1 week
+ httpOnly: true, // Prevent XSS attacks
+ secure: process.env.NODE_ENV === 'production', // HTTPS only in production
+ sameSite: 'lax' // CSRF protection
}
})
);
+
+// Handle store errors
+app.use(session({}).store.on('error', (error) => {
+ console.error('Session store error:', error);
+}));Note: After verifying the environment variable, you may need to adjust line 39 to use the correct variable name (either process.env.MONGO_URI or access it through a config object).
🏁 Script executed:
#!/bin/bash
# Check for MONGO_URI usage across the codebase
echo "=== Searching for MONGO_URI usage ==="
rg -n "MONGO_URI" --type js
echo -e "\n=== Searching for mongoURI usage ==="
rg -n "mongoURI" --type js
echo -e "\n=== Checking backend/config/db.js for config object ==="
cat backend/config/db.jsLength of output: 781
Consolidate session config & enforce security (backend/app.js:38-44)
- Switch to
config.mongoURI(frombackend/config/config.js) instead ofprocess.env.MONGO_URI. - Attach
store.on('error', …)to handle session-store failures. - Harden cookies:
httpOnly: true,secure: process.env.NODE_ENV==='production',sameSite: 'lax'. - Add a
ttlinMongoStore.createto match the cookie’smaxAge.
🤖 Prompt for AI Agents
In backend/app.js around lines 38 to 44, the session store and cookie config
need consolidation and hardening: replace process.env.MONGO_URI with
config.mongoURI (import from backend/config/config.js), pass a ttl to
MongoStore.create equal to the cookie maxAge in seconds, capture the returned
store instance and attach store.on('error', err => processLogger.error('Session
store error', err)) to handle store failures, and harden the cookie options by
adding httpOnly: true, secure: process.env.NODE_ENV === 'production', and
sameSite: 'lax' while keeping maxAge as-is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Android/.idea/caches/deviceStreaming.xml (1)
1-915: Drop IDE cache from source control
DeviceStreaming.xmlis an Android Studio cache file with user/device-specific selections. Keeping it under version control creates noisy diffs and accidental churn, and it is regenerated automatically by the IDE. Please remove it from the repo and add it to the appropriate ignore list.Android/.idea/runConfigurations.xml (1)
1-17: Exclude user-specific run configuration preferencesThis file toggles Android Studio run configuration producers, which is a local IDE preference. Checking it in will override other contributors’ defaults and produces noisy history. Recommend dropping it from the repo and adding it to the ignore list instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
Android/.idea/.name(1 hunks)Android/.idea/AndroidProjectSystem.xml(1 hunks)Android/.idea/caches/deviceStreaming.xml(1 hunks)Android/.idea/compiler.xml(1 hunks)Android/.idea/migrations.xml(1 hunks)Android/.idea/modules.xml(0 hunks)Android/.idea/runConfigurations.xml(1 hunks)Android/app/kotlin/new.properties(1 hunks)Android/app/src/main/res/font/inria_sans.xml(1 hunks)Android/app/src/main/res/font/inria_sans_bold.xml(1 hunks)Android/app/src/main/res/xml/data_extraction_rules.xml(1 hunks)
💤 Files with no reviewable changes (1)
- Android/.idea/modules.xml
✅ Files skipped from review due to trivial changes (8)
- Android/app/src/main/res/font/inria_sans_bold.xml
- Android/.idea/.name
- Android/.idea/AndroidProjectSystem.xml
- Android/.idea/migrations.xml
- Android/app/src/main/res/font/inria_sans.xml
- Android/app/kotlin/new.properties
- Android/app/src/main/res/xml/data_extraction_rules.xml
- Android/.idea/compiler.xml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/controllers/authController.js (1)
82-84: Consider more robust phone validation for international support.The current validation only accepts exactly 10 unformatted digits, which is restrictive and likely region-specific. This approach:
- Doesn't support international phone numbers with country codes
- Doesn't allow common formatting (parentheses, dashes, spaces)
- May frustrate users who expect to enter their phone numbers in familiar formats
If your application needs to support international users or flexible phone number formats, consider using a library like
libphonenumber-jsfor more robust validation. If the application is specifically for a single region (e.g., India), the current implementation may be acceptable.Example implementation with libphonenumber-js:
+const { parsePhoneNumber, isValidPhoneNumber } = require('libphonenumber-js'); + // ... in signup function ... - if (!/^\d{10}$/.test(phone)) { - return res.status(400).json({ message: "Phone must be 10 digits." }); + if (!isValidPhoneNumber(phone, 'IN')) { // 'IN' for India, or omit for international + return res.status(400).json({ message: "Invalid phone number." }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/authController.js(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/controllers/authController.js (1)
src/User/pages/UserAuth/UserAuth.jsx (3)
phone(35-35)username(33-33)e(97-144)
🔇 Additional comments (3)
backend/controllers/authController.js (3)
79-93: LGTM! Improved error messages for better UX.The validation error messages are now more specific and informative, providing clear feedback to users about what went wrong with their input. This improves the user experience without introducing security concerns.
130-130: LGTM! Login error messages follow security best practices.The updated error messages correctly:
- Provide clear feedback for missing credentials (line 130)
- Use generic "Invalid email or password." messages (lines 138, 163) to prevent user enumeration attacks
This approach aligns with security best practices for authentication flows.
Also applies to: 138-138, 163-163
281-281: LGTM! OTP error messages provide good UX with reasonable security.The updated OTP error messages strike a good balance:
- Line 281: Generic "Invalid email or OTP." prevents revealing which field is wrong
- Line 296: Specific "OTP expired." helps users understand they need to request a new OTP
- Line 301: Clear "Invalid code." provides direct feedback
Also applies to: 296-296, 301-301
| // Check uniqueness separately | ||
| const existingUserByEmail = await User.findOne({ email }).lean(); | ||
| if (existingUserByEmail) { | ||
| return res.status(400).json({ message: "Email already exists." }); | ||
| } | ||
|
|
||
| if (existingUserByEmail || existingUserByUsername) { | ||
| // Generic message | ||
| return res.status(400).json({ message: "Unable to process request." }); | ||
| const existingUserByUsername = await User.findOne({ username }).lean(); | ||
| if (existingUserByUsername) { | ||
| return res.status(400).json({ message: "Username already exists." }); | ||
| } |
There was a problem hiding this comment.
User enumeration vulnerability introduced by split uniqueness checks.
The split uniqueness checks with distinct error messages ("Email already exists." vs "Username already exists.") create a user enumeration vector. An attacker can probe the signup endpoint to discover which emails and usernames are registered in the system.
While this approach improves UX by providing specific feedback, it trades off security. Note that your login and OTP endpoints correctly prevent enumeration with generic messages (lines 138, 212).
If user enumeration is a concern for your application's threat model, consider:
- Returning a generic message like "Email or username already exists." or
- Returning a generic "Invalid request." message for all validation failures, or
- Accepting the trade-off if UX is prioritized over this specific security concern
Apply this diff if you want to prevent user enumeration:
- // Check uniqueness separately
- const existingUserByEmail = await User.findOne({ email }).lean();
- if (existingUserByEmail) {
- return res.status(400).json({ message: "Email already exists." });
- }
-
- const existingUserByUsername = await User.findOne({ username }).lean();
- if (existingUserByUsername) {
- return res.status(400).json({ message: "Username already exists." });
+ // Check uniqueness (prevent enumeration with generic message)
+ const existingUserByEmail = await User.findOne({ email }).lean();
+ const existingUserByUsername = await User.findOne({ username }).lean();
+ if (existingUserByEmail || existingUserByUsername) {
+ return res.status(400).json({ message: "Email or username already exists." });
}📝 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.
| // Check uniqueness separately | |
| const existingUserByEmail = await User.findOne({ email }).lean(); | |
| if (existingUserByEmail) { | |
| return res.status(400).json({ message: "Email already exists." }); | |
| } | |
| if (existingUserByEmail || existingUserByUsername) { | |
| // Generic message | |
| return res.status(400).json({ message: "Unable to process request." }); | |
| const existingUserByUsername = await User.findOne({ username }).lean(); | |
| if (existingUserByUsername) { | |
| return res.status(400).json({ message: "Username already exists." }); | |
| } | |
| // Check uniqueness (prevent enumeration with generic message) | |
| const existingUserByEmail = await User.findOne({ email }).lean(); | |
| const existingUserByUsername = await User.findOne({ username }).lean(); | |
| if (existingUserByEmail || existingUserByUsername) { | |
| return res.status(400).json({ message: "Email or username already exists." }); | |
| } |
🤖 Prompt for AI Agents
In backend/controllers/authController.js around lines 95 to 104, the separate
uniqueness checks leak whether an email or username exists; change the responses
to a single, non-distinct message to prevent user enumeration. Replace the two
distinct 400 responses with one generic 400 response (e.g., "Email or username
already exists." or "Invalid request.") whenever either existingUserByEmail or
existingUserByUsername is true, so callers cannot distinguish which field
triggered the failure.
|
Congratulations, Your pull request has been successfully merged 🥳🎉 Thank you for your valuable contribution to the project 🚀 Keep contributing!! ✨ 📢 Don’t forget to share your VigyBag PR on LinkedIn and tag @VigyBag and me — we’d love to celebrate your achievement with you! 🔗💡 |
1 participant
Fixes Issue
Changes proposed
Screenshots
Note to reviewers
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Chores