Implemented session management #421
Conversation
|
@Sourabh782 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe pull request introduces enhancements to user session management within the application. Key changes include the addition of a logout feature in the customer controller, session validation middleware, and the integration of session management using Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
backend/middlewares/sessionMiddleware.js (1)
12-12: Add JSDoc documentation for the middlewareAdd comprehensive documentation to improve maintainability:
+/** + * Middleware to validate user session + * @param {import('express').Request} req - Express request object + * @param {import('express').Response} res - Express response object + * @param {import('express').NextFunction} next - Express next middleware function + */ const sessionMiddleware = (req, res, next)=>{backend/routes/customerRouter.js (1)
31-33: Consider reorganizing routes for better maintainability.The logout route is placed between registration and Google OAuth routes. For better organization and maintainability, consider grouping related routes together:
- Authentication routes (login, logout)
- Registration routes
- Password management routes
- OAuth routes
Apply this diff to improve route organization:
router.post("/register", createCustomer); -router.post("/logout", logout) - router.get( "/auth/google", passport.authenticate("google", { scope: ["email"] }) ); router.post("/login", loginCustomer); +router.post("/logout", authenticateCustomer, logout); router.post("/reset-password", resetPassword);backend/routes/index.js (4)
Line range hint
11-22: Remove duplicate router loading logic.The feedbackRouter is being loaded twice:
- Through try-catch block with error handling
- Direct require on line 52
This creates unnecessary complexity and potential inconsistencies.
Apply this diff to resolve the duplication:
-let feedbackRouter; - -try { - feedbackRouter = require("./feedbackRouter"); -} catch (error) { - logger.error("Error loading feedbackRouter:", error); // Log the error with Winston - feedbackRouter = (req, res) => { - res - .status(500) - .json({ error: "Feedback functionality is currently unavailable" }); - }; -} // ... later in the file ... -router.use("/feedback", require("./feedbackRouter")); +router.use("/feedback", feedbackRouter);Also applies to: 52-52
Line range hint
4-9: Security: Move sensitive configuration to a separate config file.The JWT_SECRET and Google OAuth credentials should not be directly exposed in the router file. Consider moving these to a dedicated config module.
Create a new file
config/auth.js:require("dotenv").config(); module.exports = { JWT_SECRET: process.env.JWT_SECRET, GOOGLE_CLIENT_ID: process.env.GOOGLE_CLIENT_ID, GOOGLE_CLIENT_SECRET: process.env.GOOGLE_CLIENT_SECRET, };Then update this file:
-const config = { - JWT_SECRET: process.env.JWT_SECRET, - GOOGLE_CLIENT_ID: process.env.GOOGLE_CLIENT_ID, - GOOGLE_CLIENT_SECRET: process.env.GOOGLE_CLIENT_SECRET, -}; +const config = require("../config/auth");
Line range hint
39-49: Consider adding API versioning to the routes.The root route indicates API version 1.0.0, but the routes aren't versioned in the URL structure. This could make future API updates more challenging.
Consider prefixing routes with version:
-router.use("/event", eventRouter); -router.use("/admin", require("./adminRouter")); +router.use("/v1/event", eventRouter); +router.use("/v1/admin", require("./adminRouter"));
Session middleware integration requires attention
The verification revealed several security concerns:
/user(customerRouter.js) lacks session protection but likely needs it for user operations/newsletter(newsletterRoute.js) and/forgot(forgotRouter.js) appropriately don't use session middleware as they're typically public routes/feedback,/reservation, and/orderroutes are properly protected with session middleware- The session middleware implementation correctly checks for user session and returns 401 on invalid sessions
🔗 Analysis chain
Line range hint
52-57: Verify session middleware integration.Since this PR implements session management, we should verify that these routes are properly protected with session middleware.
Let's check the session middleware implementation in these routes:
Let me check the actual implementation of session middleware in these files to verify its proper integration.
Let me check the actual implementation of the session middleware to ensure it's properly securing these routes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for session middleware usage in router files echo "Checking session middleware implementation in routes..." rg -l "sessionMiddleware|session.*middleware" backend/routes/Length of output: 283
Script:
#!/bin/bash # Check the actual session middleware implementation and usage echo "=== Checking feedbackRouter.js ===" rg "sessionMiddleware|session.*middleware" -A 3 backend/routes/feedbackRouter.js echo -e "\n=== Checking reservationRouter.js ===" rg "sessionMiddleware|session.*middleware" -A 3 backend/routes/reservationRouter.js echo -e "\n=== Checking orderRouter.js ===" rg "sessionMiddleware|session.*middleware" -A 3 backend/routes/orderRouter.js # Also check if there's a central session middleware definition echo -e "\n=== Checking for session middleware definition ===" rg "const.*sessionMiddleware|function.*sessionMiddleware" backend/Length of output: 1848
Script:
#!/bin/bash # Check the session middleware implementation echo "=== Session Middleware Implementation ===" cat backend/middlewares/sessionMiddleware.js # Check if any routes are missing session middleware echo -e "\n=== Checking customerRouter for session middleware ===" rg "sessionMiddleware" backend/routes/customerRouter.js || echo "No session middleware found in customerRouter.js" echo -e "\n=== Checking newsletterRoute for session middleware ===" rg "sessionMiddleware" backend/routes/newsletterRoute.js || echo "No session middleware found in newsletterRoute.js" echo -e "\n=== Checking forgotRouter for session middleware ===" rg "sessionMiddleware" backend/routes/forgotRouter.js || echo "No session middleware found in forgotRouter.js"Length of output: 1281
backend/index.js (2)
Line range hint
7-7: Fix the typo in error middleware pathDespite the comment indicating a corrected typo, the import path still contains a typo:
errrorMiddleware(three 'r's). This should beerrorMiddleware.Apply this fix:
-const errorMiddleware = require("./middlewares/errrorMiddleware"); +const errorMiddleware = require("./middlewares/errorMiddleware");
Line range hint
16-21: Fix session integration issuesSeveral integration issues need to be addressed:
- CORS needs to be configured for credentials
- Session middleware should be placed before passport initialization
- Consider session handling for OAuth
Apply these fixes:
- Update CORS configuration:
const corsOptions = { origin: ["http://localhost:5173", "https://play-cafe.vercel.app"], optionsSuccessStatus: 200, + credentials: true };
- Move the session middleware before passport initialization:
-// Initialize passport middleware -app.use(passport.initialize()); - app.use( session({ // ... session config }) ); + +// Initialize passport middleware +app.use(passport.initialize()); +app.use(passport.session()); // Add this if you want persistent login sessionsAlso applies to: 47-47
backend/controller/customer.controller.js (2)
Line range hint
54-108: Remove debug console.log statements from production code.The numerous console.log statements (1-5) appear to be debugging artifacts that should not be in production code.
Apply this diff to remove the debug statements:
- console.log("1"); - console.log("2"); - console.log("3"); - console.log("4"); - console.log(req.session); - console.log("5"); - console.log(error);
Line range hint
1-151: Consider implementing these architectural improvements:
- Create a centralized error handling middleware to standardize error responses across the application.
- Move session configuration (timeouts, cookie settings) to a configuration file for easier management.
- Implement security headers middleware (helmet.js) to enhance security.
- Consider rate limiting for login/logout endpoints to prevent brute force attacks.
Would you like me to provide example implementations for any of these suggestions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- README.md (2 hunks)
- backend/controller/customer.controller.js (4 hunks)
- backend/index.js (2 hunks)
- backend/middlewares/sessionMiddleware.js (1 hunks)
- backend/package.json (1 hunks)
- backend/routes/customerRouter.js (2 hunks)
- backend/routes/feedbackRouter.js (1 hunks)
- backend/routes/index.js (1 hunks)
- backend/routes/orderRouter.js (1 hunks)
- backend/routes/reservationRouter.js (1 hunks)
- frontend/src/components/Shared/Navbar.jsx (2 hunks)
🧰 Additional context used
🪛 Markdownlint
README.md
283-283: Column: 1
Hard tabs(MD010, no-hard-tabs)
284-284: Column: 1
Hard tabs(MD010, no-hard-tabs)
🔇 Additional comments (10)
backend/middlewares/sessionMiddleware.js (1)
1-12: Verify consistent middleware implementation across routesLet's verify the middleware implementation across all routes:
#!/bin/bash # Check middleware usage in route files echo "Checking route files for sessionMiddleware usage..." rg -A 3 "sessionMiddleware" --type js # Verify all protected routes use the middleware echo "Checking for potentially unprotected routes..." rg -A 3 "router\.(post|put|delete)" --type jsThis will help ensure that:
- The middleware is consistently applied
- No sensitive routes are left unprotected
backend/routes/orderRouter.js (2)
3-3: LGTM! Clean import addition.The sessionMiddleware import is properly placed and follows the project's import conventions.
8-10: LGTM! Session protection added to order routes.The sessionMiddleware has been correctly applied to all order operations, ensuring authenticated access.
backend/routes/reservationRouter.js (2)
3-3: LGTM! Clean middleware import.The session middleware import follows proper conventions and maintains good code organization.
6-6: Verify session handling implementation details.The addition of session middleware to protect the reservation creation endpoint is appropriate from a security perspective. However, let's verify a few important aspects:
- Session expiry handling
- Error responses for invalid/expired sessions
- Consistent session protection across all reservation-related endpoints
Let's verify the session implementation:
backend/routes/feedbackRouter.js (1)
6-6: LGTM!Clean import statement following the existing code style.
backend/routes/customerRouter.js (2)
6-6: LGTM!The logout controller import is correctly added to the destructured imports.
32-32: Verify session handling in the logout controller.Since this PR implements session management, let's verify that the logout controller properly handles session destruction.
✅ Verification successful
Session handling in logout controller is properly implemented
The logout controller correctly handles session destruction by:
- Using
req.session.destroy()to completely remove the session- Including proper error handling for failed session destruction
- Sending appropriate response messages to the client
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the logout controller properly handles session destruction # Test 1: Check the logout controller implementation echo "Checking logout controller implementation..." ast-grep --pattern 'logout = async (req, res) => { $$$ req.session.$$$ $$$ }' # Test 2: Check for error handling echo "Checking error handling in logout..." rg -A 5 "catch.*logout" ../controller/customer.controller.jsLength of output: 407
Script:
#!/bin/bash # Let's first locate the correct file path for the controller echo "Finding the logout controller file..." fd -t f "customer.*.js" # Once we find the file, let's check its content for logout implementation echo "Checking content of controller files for logout implementation..." rg -l "logout.*=.*async" # Let's examine the actual implementation echo "Examining logout implementation..." rg -A 10 "logout.*=.*async"Length of output: 531
Script:
#!/bin/bash # Let's examine the content of the controller file directly echo "Checking customer controller content..." cat backend/controller/customer.controller.js # Also check the router file to see the import and usage echo "Checking router file content..." cat backend/routes/customerRouter.jsLength of output: 5519
backend/controller/customer.controller.js (2)
147-151: LGTM!The module exports have been correctly updated to include the new logout function.
84-87: Verify session data storage requirements.The session storage includes minimal user data. Consider whether additional user information needs to be stored in the session for your application's requirements.
✅ Verification successful
The previous search might have failed due to escaping issues. Let's try broader patterns to understand session usage and requirements in the codebase.
Session data storage is appropriately minimal and sufficient
Based on the codebase analysis, the current session data storage (user ID and name) is adequate:
- The session is only used for basic authentication in
sessionMiddleware.js, which checks if a user is logged in- No other parts of the codebase require additional session data
- The stored information (id and name) is sufficient for identifying and authenticating the user
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for session data usage across the codebase rg -A 3 "req\.session\.user\."Length of output: 32
Script:
#!/bin/bash # Let's try a broader search for session usage patterns rg -A 3 "session\.user" # Also search for middleware that might be handling session data rg -A 5 "session\." backend/ # Check for authentication-related files that might define session requirements fd -e js auth middleware session -x cat {}Length of output: 2445
| const sessionMiddleware = async (req, res, next)=>{ | ||
| console.log(req.session.user); | ||
|
|
||
|
|
There was a problem hiding this comment.
Remove unnecessary async keyword and improve logging
The current implementation has several issues:
- The
asynckeyword is unnecessary as there are noawaitoperations - Using
console.login production code is not recommended - Remove empty lines that serve no purpose
Apply this diff to improve the implementation:
-const sessionMiddleware = async (req, res, next)=>{
- console.log(req.session.user);
-
-
+const sessionMiddleware = (req, res, next) => {
+ req.log.debug('Session user:', req.session.user);Consider using a proper logging library like Winston or Pino for structured logging in production.
Committable suggestion was skipped due to low confidence.
| if (req.session.user !== undefined) { | ||
| next(); | ||
| } else { | ||
| res.status(401).send("Invalid session. Please log in again."); | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide the final response:
Session security improvements needed, but with modified recommendations
The current session setup already includes several security measures:
- MongoDB session store with 24-hour expiration
- Secure session cookies with httpOnly flag
- Proper session initialization with express-session
However, the original review's suggested changes should be modified:
- if (req.session.user !== undefined) {
+ if (!req.session) {
+ return res.status(500).json({
+ error: 'Session middleware not properly configured'
+ });
+ }
+
+ if (req.session.user &&
+ typeof req.session.user === 'object' &&
+ req.session.cookie.expires > Date.now()) {
next();
} else {
- res.status(401).send("Invalid session. Please log in again.");
+ res.status(401).json({
+ error: 'Authentication required',
+ message: 'Please log in to continue'
+ });
}Additional recommendations:
- Enable
secure: truefor session cookies in production - Add CSRF protection using a package like
csurf - Consider implementing rate limiting for auth endpoints
🔗 Analysis chain
Enhance session validation security and error handling
The current session validation could be more robust and secure.
Apply this diff to improve security and error handling:
- if (req.session.user !== undefined) {
+ if (!req.session) {
+ return res.status(500).json({
+ error: 'Session middleware not properly configured'
+ });
+ }
+
+ if (req.session.user && typeof req.session.user === 'object') {
next();
} else {
- res.status(401).send("Invalid session. Please log in again.");
+ res.status(401).json({
+ error: 'Authentication required',
+ message: 'Please log in to continue'
+ });
}Consider implementing these additional security measures:
- Session expiration checks
- CSRF token validation
- Session fingerprinting (IP, user agent)
- Rate limiting
Let's verify if CSRF protection is implemented elsewhere:
Let me gather more information about the session configuration and CSRF protection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for CSRF middleware or protection
rg -i "csrf|csurf" --type js
Length of output: 4524
Script:
#!/bin/bash
# Check for session configuration and security middleware
echo "Checking for session configuration..."
rg -A 5 "express-session|cookie|csrf" --type js
echo -e "\nChecking main application setup..."
rg -A 10 "const express|const app" --type js
Length of output: 9913
| router.post("/create/:id", sessionMiddleware, createOrder); | ||
| router.get("/get/:id", sessionMiddleware, getOrders); | ||
| router.delete("/delete/:id", sessionMiddleware, deleteOrder); |
There was a problem hiding this comment.
💡 Codebase verification
Additional security measures needed for order operations
Based on the code analysis, while there are some security measures in place, important improvements are needed:
- The session middleware only checks if a user exists but doesn't verify the user's identity against the customerId in the routes
- The
createOrderandgetOrdersoperations don't verify if the authenticated user matches the customerId - Only
deleteOrderpartially validates ownership by checking if the order belongs to the customer - There's no request body validation middleware for the order creation payload
Recommended actions:
- Add user-customer relationship validation in the session middleware or a separate authorization middleware
- Implement request validation middleware for the order creation endpoint to validate items array structure
- Add ownership verification in
createOrderandgetOrderssimilar todeleteOrder - Consider adding rate limiting for these endpoints
🔗 Analysis chain
Consider additional security measures for order operations.
While session validation is good, consider these additional security measures:
- Validate that the authenticated user owns or has permission to access the order specified by
:id - Add request validation middleware to ensure proper request body format for the create operation
Let's check if other routes in the application implement similar security measures:
Let's check the implementation of these order operations to verify if they include ownership validation and request validation.
Let's check the actual implementation of these order operations by looking for their definitions in the controllers, and also examine any existing validation patterns in the codebase.
Let's examine the content of the order controller file to see the actual implementation of these operations.
Let's check the session middleware to understand the current authentication implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for routes using :id parameter to compare security implementations
rg -A 3 "router\.(post|get|delete).*/:id" backend/routes/
Length of output: 411
Script:
#!/bin/bash
# Search for the implementation of order operations
ast-grep --pattern 'const createOrder = async (req, res) {
$$$
}'
ast-grep --pattern 'const getOrders = async (req, res) {
$$$
}'
ast-grep --pattern 'const deleteOrder = async (req, res) {
$$$
}'
# Also check if there's any validation middleware
rg -l "validator|validate|check" backend/
Length of output: 403
Script:
#!/bin/bash
# Find and show the order controller file
fd "order.*controller" backend/
# Look for order operation implementations with context
rg -A 10 "^(const|async function) (createOrder|getOrders|deleteOrder)" backend/
# Check validation patterns in existing files
rg -A 5 "validate|check" backend/models/
Length of output: 973
Script:
#!/bin/bash
# Show the content of the order controller file
cat backend/controller/order.controller.js
# Also check for any middleware files that might contain validation
fd "middleware" backend/
Length of output: 2115
Script:
#!/bin/bash
# Show the content of the session middleware
cat backend/middlewares/sessionMiddleware.js
# Check the order model for any validation schemas
cat backend/models/order.model.js
Length of output: 1113
|
|
||
| router.post("/create", createFeedback); | ||
| router.post("/create", sessionMiddleware, createFeedback); |
There was a problem hiding this comment.
💡 Codebase verification
Session middleware inconsistency detected across sensitive routes
Several routes handling sensitive operations are missing session validation:
newsletterRoute.js:/subscribeendpoint lacks session protectionadminRouter.js: Both/registerand/loginendpoints lack session checkscustomerRouter.js: Multiple endpoints including/register,/login,/logout, and/reset-passwordmissing session validationeventRouter.js:/createendpoint lacks session protectionforgotRouter.js: Password reset flow endpoints (/verify-email,/verify-otp,/resetpassword) missing session checks
Only the following routes implement proper session validation:
feedbackRouter.js:/createreservationRouter.js:/createorderRouter.js:/create/:id,/delete/:id
🔗 Analysis chain
LGTM! Verify consistent session handling across routes.
Good security practice to protect feedback submission with session validation.
Let's verify consistent session protection across similar routes:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent session middleware usage across routes
# Expected: All sensitive routes should use sessionMiddleware
# Check routes files for sessionMiddleware usage
echo "Checking routes for sessionMiddleware usage:"
rg -A 1 "router\.(post|put|delete)" backend/routes/
Length of output: 1822
| @@ -27,6 +28,9 @@ router.get( | |||
| ); | |||
|
|
|||
| router.post("/register", createCustomer); | |||
|
|
|||
| router.post("/logout", logout) | |||
There was a problem hiding this comment.
Add authentication middleware to protect the logout route.
The logout route should be protected with the authenticateCustomer middleware to ensure only authenticated users can access it. This prevents potential security issues where unauthenticated users could trigger session destruction.
Apply this diff to secure the route:
-router.post("/logout", logout)
+router.post("/logout", authenticateCustomer, logout)📝 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.
| router.post("/logout", logout) | |
| router.post("/logout", authenticateCustomer, logout) |
| @@ -42,6 +44,21 @@ | |||
| // Initialize passport middleware | |||
| app.use(passport.initialize()); | |||
|
|
|||
| app.use( | |||
| session({ | |||
| secret: process.env.SECRET_KEY, | |||
There was a problem hiding this comment.
💡 Codebase verification
Environment variables need validation at startup
The application uses several critical environment variables (SECRET_KEY, MONGO_URI, JWT_SECRET, etc.) but lacks proper validation at startup. While dotenv is configured, there's no explicit validation to ensure required variables are present before the application starts.
Key locations requiring attention:
backend/index.js: Add validation forSECRET_KEYused in session configurationbackend/config/secret.js: Add validation for all exported environment variables
Example validation to add at application startup:
const requiredEnvVars = ['SECRET_KEY', 'MONGO_URI', 'JWT_SECRET'];
for (const envVar of requiredEnvVars) {
if (!process.env[envVar]) {
throw new Error(`Missing required environment variable: ${envVar}`);
}
}🔗 Analysis chain
Verify required environment variables
The session configuration depends on SECRET_KEY. Ensure this environment variable is properly set and validated at startup.
Let me gather more information about how SECRET_KEY is used and if there's any validation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if required environment variables are documented
# Test: Check if SECRET_KEY is documented in .env.example
if [ -f .env.example ]; then
echo "Checking .env.example for SECRET_KEY..."
grep "SECRET_KEY" .env.example
fi
# Test: Look for environment variable validation
echo "Checking for environment variable validation..."
rg -l "SECRET_KEY" --type js
Length of output: 176
Script:
#!/bin/bash
# Check the context around SECRET_KEY usage and look for validation patterns
rg "SECRET_KEY" -B 3 -A 3 --type js
# Look for common environment variable validation patterns
rg "(process\.env\.|dotenv|envalid|env\.required)" --type js -B 2 -A 2
Length of output: 8630
| res.cookie("authToken", token, { | ||
| maxAge: 1000 * 60 * 60, | ||
| httpOnly: true, | ||
| secure: true, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance cookie security settings.
While the current cookie settings include basic security measures, they could be strengthened further.
Consider adding these additional security options:
res.cookie("authToken", token, {
maxAge: 1000 * 60 * 60,
httpOnly: true,
secure: true,
+ sameSite: 'strict',
+ path: '/',
});📝 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.
| res.cookie("authToken", token, { | |
| maxAge: 1000 * 60 * 60, | |
| httpOnly: true, | |
| secure: true, | |
| }); | |
| res.cookie("authToken", token, { | |
| maxAge: 1000 * 60 * 60, | |
| httpOnly: true, | |
| secure: true, | |
| sameSite: 'strict', | |
| path: '/', | |
| }); |
| @@ -13,6 +13,7 @@ const Navbar = () => { | |||
| const [token, setToken] = useState(Cookies.get('authToken')); | |||
| const location = useLocation(); | |||
| const navigate = useNavigate(); // Correctly initialize useNavigate | |||
| const API_URL = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000'; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the API URL configuration.
While the implementation works, consider these improvements for better maintainability and security:
- Move this configuration to a dedicated config file (e.g.,
config.js) - Add URL format validation
- Consider using relative URLs in production to avoid CORS issues
Example implementation:
// config.js
export const getApiUrl = () => {
const url = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000';
try {
new URL(url);
return url;
} catch (e) {
console.error('Invalid VITE_BACKEND_URL:', e);
return 'http://localhost:3000';
}
};| const handleLogout = async () => { | ||
| // setisloggedIn(false); // Set isLoggedIn to false on confirmation | ||
| //managing log in , logout using jwt tokens | ||
| const response = await fetch(`${API_URL}/api/user/logout`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| }, | ||
| }) | ||
|
|
There was a problem hiding this comment.
Add error handling and cleanup commented code.
The logout implementation needs improvements in error handling and cleanup:
- Missing error handling for failed API calls
- Response status not verified
- Contains commented out code
- Token removal should only occur after successful logout
Here's the suggested implementation:
const handleLogout = async () => {
- // setisloggedIn(false); // Set isLoggedIn to false on confirmation
- //managing log in , logout using jwt tokens
- const response = await fetch(`${API_URL}/api/user/logout`, {
- method: 'POST',
- headers: {
- 'Content-Type': 'application/json',
- },
- })
-
- Cookies.remove('authToken');
- setToken(null);
- setIsModalOpen(false); // Close the modal
- setIsMenuOpen(false); // after getting logged out close the menu if it is open
- navigate('/login'); //navigate to login after get logged out
+ try {
+ const response = await fetch(`${API_URL}/api/user/logout`, {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ credentials: 'include', // Important for cookies
+ });
+
+ if (!response.ok) {
+ throw new Error('Logout failed');
+ }
+
+ Cookies.remove('authToken');
+ setToken(null);
+ setIsModalOpen(false);
+ setIsMenuOpen(false);
+ navigate('/login');
+ } catch (error) {
+ console.error('Logout error:', error);
+ // Consider showing an error message to the user
+ }
};📝 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.
| const handleLogout = async () => { | |
| // setisloggedIn(false); // Set isLoggedIn to false on confirmation | |
| //managing log in , logout using jwt tokens | |
| const response = await fetch(`${API_URL}/api/user/logout`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| }) | |
| const handleLogout = async () => { | |
| try { | |
| const response = await fetch(`${API_URL}/api/user/logout`, { | |
| method: 'POST', | |
| headers: { | |
| 'Content-Type': 'application/json', | |
| }, | |
| credentials: 'include', // Important for cookies | |
| }); | |
| if (!response.ok) { | |
| throw new Error('Logout failed'); | |
| } | |
| Cookies.remove('authToken'); | |
| setToken(null); | |
| setIsModalOpen(false); | |
| setIsMenuOpen(false); | |
| navigate('/login'); | |
| } catch (error) { | |
| console.error('Logout error:', error); | |
| // Consider showing an error message to the user | |
| } | |
| }; |
| // setisloggedIn(false); // Set isLoggedIn to false on confirmation | ||
| //managing log in , logout using jwt tokens | ||
| const response = await fetch(`${API_URL}/api/user/logout`, { |
There was a problem hiding this comment.
💡 Codebase verification
Backend logout endpoint exists but path mismatch detected
The frontend is using /api/user/logout but the backend route is configured differently. The logout endpoint is defined in customerRouter.js as /logout and would be mounted under a different base path.
- The frontend makes request to:
/api/user/logout - The backend defines route as:
/logoutin customerRouter.js
This path mismatch needs to be fixed either by:
- Updating the frontend URL to match the backend route, or
- Updating the backend route configuration to match the frontend expectation
🔗 Analysis chain
Verify backend logout endpoint configuration.
Let's ensure the logout endpoint matches the backend route configuration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the logout route definition in backend files
rg -l "router\.(post|all)\(['\"]\/api\/user\/logout" .
Length of output: 192
Script:
#!/bin/bash
# Search for logout route definition with proper escaping
rg -l "router\.(post|all)\(.*?['\"]\/api\/user\/logout['\"]" .
# Also search for any file containing logout route pattern
rg -l "\/api\/user\/logout" .
# Search for Express router definitions related to user/logout
ast-grep --pattern 'router.$_("/api/user/logout"'
Length of output: 382
Script:
#!/bin/bash
# Search for logout route without escaping
rg -l "router\.(post|get|all)\(.*?'/api/user/logout'" .
# Search for any file containing the logout endpoint
rg -l "/api/user/logout" .
# Search for Express router files
fd "routes?.js|routes?.ts"
Length of output: 197
Script:
#!/bin/bash
# Let's check the content of the routes file
cat backend/routes/newsletterRoute.js
# Search for other potential route files in backend
fd . backend --type f -e js -e ts | rg -i "(route|api)"
# Search for auth or user related files
fd . backend --type f | rg -i "(auth|user)"
Length of output: 880
Script:
#!/bin/bash
# Check content of auth middleware and OAuth files
cat backend/middlewares/authCustomer.js
cat backend/config/oauth.config.js
cat backend/controller/googleOAuth.controller.js
# Check content of potential user-related routers
cat backend/routes/adminRouter.js
cat backend/routes/customerRouter.js
Length of output: 4974
|
@Sourabh782 can you make the admin router private like no one will access except admin |
|
@Sourabh782 also resolve conflicts |
sure will look into it |
actually we already have admin middleware: |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- backend/controller/customer.controller.js (4 hunks)
- backend/routes/customerRouter.js (2 hunks)
- frontend/src/components/Shared/Navbar.jsx (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/routes/customerRouter.js
- frontend/src/components/Shared/Navbar.jsx
🔇 Additional comments (1)
backend/controller/customer.controller.js (1)
191-196: LGTM!The module exports are correctly updated to include the new logout function while maintaining existing functionality.
| async function logout(req, res){ | ||
| req.session.destroy((err) => { | ||
| if (err) { | ||
| return res.status(500).send("Failed to log out."); | ||
| } | ||
| res.send("Logged out successfully!"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Add session validation before logout.
The current implementation doesn't verify if a user is actually logged in before attempting to destroy the session. This could lead to unnecessary session operations and unclear error states.
Apply this diff to enhance the logout implementation:
async function logout(req, res){
+ if (!req.session.user) {
+ return res.status(401).send("No active session");
+ }
req.session.destroy((err) => {
if (err) {
+ console.error('Session destruction failed:', err);
return res.status(500).send("Failed to log out.");
}
res.send("Logged out successfully!");
});
}📝 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.
| async function logout(req, res){ | |
| req.session.destroy((err) => { | |
| if (err) { | |
| return res.status(500).send("Failed to log out."); | |
| } | |
| res.send("Logged out successfully!"); | |
| }); | |
| } | |
| async function logout(req, res){ | |
| if (!req.session.user) { | |
| return res.status(401).send("No active session"); | |
| } | |
| req.session.destroy((err) => { | |
| if (err) { | |
| console.error('Session destruction failed:', err); | |
| return res.status(500).send("Failed to log out."); | |
| } | |
| res.send("Logged out successfully!"); | |
| }); | |
| } |
| req.session.user = { | ||
| id: customer._id, | ||
| name: customer.name, | ||
| }; |
There was a problem hiding this comment.
Configure session expiry to match token expiry.
The JWT token expires in 1 hour, but the session doesn't have an explicit expiry. Consider adding session expiry configuration to maintain consistency with token lifetime.
Add this before setting the session:
+ req.session.cookie.expires = new Date(Date.now() + 60 * 60 * 1000); // 1 hour
req.session.user = {
id: customer._id,
name: customer.name,
};📝 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.
| req.session.user = { | |
| id: customer._id, | |
| name: customer.name, | |
| }; | |
| req.session.cookie.expires = new Date(Date.now() + 60 * 60 * 1000); // 1 hour | |
| req.session.user = { | |
| id: customer._id, | |
| name: customer.name, | |
| }; |
💡 Codebase verification
Session middleware integration is properly configured but needs security enhancements
The session middleware is correctly integrated with proper MongoStore configuration for persistence, but there are a few security concerns:
- Cookie security options are missing in the session configuration (httpOnly, secure, sameSite)
- Session middleware implementation in
sessionMiddleware.jslacks proper type checking for user session - Inconsistent session usage: mixing session-based auth with JWT-based auth (passport JWT with
session: false)
Recommended fixes:
- Add security options to the session configuration:
cookie: {
maxAge: 1000 * 60 * 60 * 24,
httpOnly: true,
secure: process.env.NODE_ENV === 'production',
sameSite: 'lax'
}- Strengthen session middleware check:
if (req.session?.user?.id) {
next();
}🔗 Analysis chain
Verify session middleware integration.
Let's ensure that the session middleware is properly configured and applied to the routes.
Let me gather more information about the session middleware configuration and implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify session middleware configuration and usage
# Check session middleware configuration
echo "Checking session middleware configuration..."
rg -l "express-session|connect-mongo" backend/
# Check session middleware usage in routes
echo "Checking session middleware usage in routes..."
rg "session" backend/routes/
Length of output: 1255
Script:
#!/bin/bash
# Check session middleware configuration in index.js
echo "Checking session configuration in index.js..."
rg "session" -A 5 -B 5 backend/index.js
# Check session middleware implementation
echo "Checking session middleware implementation..."
cat backend/middlewares/sessionMiddleware.js
# Check if MongoStore is properly configured
echo "Checking MongoStore configuration..."
rg "MongoStore" -A 5 backend/index.js
Length of output: 1852
RamakrushnaBiswal
merged commit 4156b76
into
RamakrushnaBiswal:main
RamakrushnaBiswal
added
enhancement
Closes #144
frontend session:
https://github.com/user-attachments/assets/73c16a49-a704-415d-aaf2-83db32ff8f9b
bakcend session on api:
https://github.com/user-attachments/assets/fd382930-48f4-41bc-8036-0d7faefa73f6
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation