Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented session management #421

Merged
merged 3 commits into from
Oct 29, 2024
Merged

Implemented session management #421

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52e95c3
docs(contributor): contrib-readme-action has updated readme
github-actions[bot] Oct 28, 2024
83b0d4b
implemented user session
Sourabh782 Oct 28, 2024
aae2662
Merge branch 'main' into session
Sourabh782 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 12 additions & 12 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,27 +273,20 @@ We extend our heartfelt gratitude to all the amazing contributors who have made
<sub><b>Haseeb Zaki</b></sub>
</a>
</td>
<td align="center">
<a href="https://github.com/Ashwinib26">
<img src="https://avatars.githubusercontent.com/u/149402720?v=4" width="100;" alt="Ashwinib26"/>
<br />
<sub><b>Ashwini_ab</b></sub>
</a>
</td>
</tr>
<tr>
<td align="center">
<a href="https://github.com/alo7lika">
<img src="https://avatars.githubusercontent.com/u/152315710?v=4" width="100;" alt="alo7lika"/>
<br />
<sub><b>alolika bhowmik</b></sub>
</a>
</td>
</tr>
<tr>
<td align="center">
<a href="https://github.com/tejasbenibagde">
<img src="https://avatars.githubusercontent.com/u/124677750?v=4" width="100;" alt="tejasbenibagde"/>
<a href="https://github.com/Ashwinib26">
<img src="https://avatars.githubusercontent.com/u/149402720?v=4" width="100;" alt="Ashwinib26"/>
<br />
<sub><b>Tejas Benibagde</b></sub>
<sub><b>Ashwini_ab</b></sub>
</a>
</td>
<td align="center">
Expand All @@ -303,6 +296,13 @@ We extend our heartfelt gratitude to all the amazing contributors who have made
<sub><b>Mahera Nayan</b></sub>
</a>
</td>
<td align="center">
<a href="https://github.com/tejasbenibagde">
<img src="https://avatars.githubusercontent.com/u/124677750?v=4" width="100;" alt="tejasbenibagde"/>
<br />
<sub><b>Tejas Benibagde</b></sub>
</a>
</td>
<td align="center">
<a href="https://github.com/Shirisha-16">
<img src="https://avatars.githubusercontent.com/u/148051550?v=4" width="100;" alt="Shirisha-16"/>
Expand Down
35 changes: 34 additions & 1 deletion backend/controller/customer.controller.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,17 @@ async function loginCustomer(req, res) {

try {
const customer = await Customer.findOne({ email: req.body.email });
console.log("1");

if (!customer) {
return res.status(401).json({ error: "Invalid email or password" });
}
const validPassword = await bcrypt.compare(
req.body.password,
customer.password
);
console.log("2");

if (!validPassword) {
return res.status(401).json({ error: "Invalid email or password" });
}
Expand All @@ -67,12 +71,29 @@ async function loginCustomer(req, res) {
role: "customer", // Optional
email: customer.email, // Optional
};
console.log("3");

const token = jwt.sign(
payload,
process.env.JWT_SECRET,
{ expiresIn: "1h" } // Expires in 1 hour
);
res.json({
console.log("4");
console.log(req.session);

req.session.user = {
id: customer._id,
name: customer.name,
};
Comment on lines +127 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.js lacks 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

console.log("5");

res.cookie("authToken", token, {
maxAge: 1000 * 60 * 60,
httpOnly: true,
secure: true,
});
Comment on lines +132 to +136
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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: '/',
});


return res.json({
message: "Login successful",
token,
role: "customer",
Expand All @@ -83,6 +104,8 @@ async function loginCustomer(req, res) {
},
});
} catch (error) {
console.log(error);

res.status(500).json({ error: "Internal server error" });
}
}
Expand Down Expand Up @@ -112,8 +135,18 @@ async function resetPassword(req, res) {
}
}

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!");
});
}
Comment on lines +181 to +188
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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!");
});
}


module.exports = {
createCustomer,
loginCustomer,
resetPassword,
logout
};
17 changes: 17 additions & 0 deletions backend/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ const passport = require("passport");
const { handleGoogleOAuth } = require("./controller/googleOAuth.controller");
const app = express();
const port = process.env.PORT || 3000;
const session = require("express-session");
const MongoStore = require("connect-mongo");

// CORS configuration
const corsOptions = {
Expand Down Expand Up @@ -42,6 +44,21 @@ mongoose
// Initialize passport middleware
app.use(passport.initialize());

app.use(
session({
secret: process.env.SECRET_KEY,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 for SECRET_KEY used in session configuration
  • backend/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

resave: false,
saveUninitialized: false,
cookie: {
maxAge: 1000 * 60 * 60 * 24,
secure: false,
},
store: MongoStore.create({
mongoUrl: process.env.MONGO_URI,
}),
})
);
Comment on lines +47 to +60
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Enhance session security configuration

The current session configuration needs several security improvements:

  1. Cookie settings should be environment-aware
  2. Missing important security options
  3. No error handling for session store

Apply these security enhancements:

 app.use(
   session({
     secret: process.env.SECRET_KEY,
     resave: false,
     saveUninitialized: false,
     cookie: {
       maxAge: 1000 * 60 * 60 * 24,
-      secure: false,
+      secure: process.env.NODE_ENV === 'production',
+      httpOnly: true,
+      sameSite: process.env.NODE_ENV === 'production' ? 'strict' : 'lax'
     },
     store: MongoStore.create({
       mongoUrl: process.env.MONGO_URI,
+      ttl: 24 * 60 * 60, // Session TTL in seconds
+      autoRemove: 'native',
+      touchAfter: 24 * 3600, // Refresh session only once per 24 hours
+      crypto: {
+        secret: process.env.SECRET_KEY
+      },
+      onError: function(error) {
+        logger.error('Session store error:', error);
+      }
     }),
+    name: 'sessionId', // Change default cookie name
+    rolling: true, // Refresh cookie on each response
   })
 );

Committable suggestion was skipped due to low confidence.


// API routes
app.use("/api", require("./routes/index"));

Expand Down
12 changes: 12 additions & 0 deletions backend/middlewares/sessionMiddleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
const sessionMiddleware = async (req, res, next)=>{
console.log(req.session.user);


Comment on lines +1 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove unnecessary async keyword and improve logging

The current implementation has several issues:

  1. The async keyword is unnecessary as there are no await operations
  2. Using console.log in production code is not recommended
  3. 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.");
}
Comment on lines +5 to +9
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

⚠️ Potential issue

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: true for 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:

  1. Session expiration checks
  2. CSRF token validation
  3. Session fingerprinting (IP, user agent)
  4. 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

}

module.exports = sessionMiddleware;
2 changes: 2 additions & 0 deletions backend/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,11 @@
"description": "",
"dependencies": {
"bcrypt": "^5.1.1",
"connect-mongo": "^5.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Session configuration is missing or incomplete

While the required dependencies (connect-mongo and express-session) are imported, the actual session middleware configuration is not found in index.js. This poses security risks as sessions would not be properly managed.

  • backend/index.js: Add session middleware configuration with: 
    app.use(session({
  secret: process.env.SESSION_SECRET,
  resave: false,
  saveUninitialized: false,
  store: MongoStore.create({ mongoUrl: process.env.MONGODB_URI }),
  cookie: {
    secure: process.env.NODE_ENV === 'production',
    httpOnly: true,
    sameSite: 'strict'
  }
}));
🔗 Analysis chain

LGTM! Session management dependencies are properly configured.

The addition of connect-mongo and express-session is appropriate for implementing secure session management with MongoDB as the session store.

Ensure the session configuration in index.js includes:

  • Secure cookie settings (httpOnly, secure, sameSite)
  • Session expiration
  • Session secret from environment variables
  • Proper MongoDB connection options

Let's verify the session configuration:

Also applies to: 23-23

Let me gather more information about the session configuration.

Let me try a different search pattern to locate the session configuration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check session configuration in index.js
# Expected: Should find session middleware setup with security options

# Test: Look for session configuration
rg -A 10 "express-session|connect-mongo" "backend/index.js"

Length of output: 349

Script:

#!/bin/bash
# Search for session middleware configuration after the imports
rg -A 20 "app\.use\(session\(" backend/index.js

Length of output: 49

Script:

#!/bin/bash
# Search for any session-related configuration with more context
rg -U -A 30 "MongoStore.*session" backend/index.js

Length of output: 52

"cors": "^2.8.5",
"dotenv": "^16.4.5",
"express": "^4.21.0",
"express-session": "^1.18.1",
"jsonwebtoken": "^9.0.2",
"mongoose": "^8.7.0",
"nodemailer": "^6.9.15",
Expand Down
4 changes: 4 additions & 0 deletions backend/routes/customerRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ const {
loginCustomer,
createCustomer,
resetPassword,
logout,
} = require("../controller/customer.controller");
const authenticateCustomer = require("../middlewares/authCustomer");
const passport = require("../config/passport.config");
Expand All @@ -27,6 +28,9 @@ router.get(
);

router.post("/register", createCustomer);

router.post("/logout", logout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
router.post("/logout", logout)
router.post("/logout", authenticateCustomer, logout)


router.get(
"/auth/google",
passport.authenticate("google", { scope: ["email"] })
Expand Down
3 changes: 2 additions & 1 deletion backend/routes/feedbackRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ const { createFeedback } = require("../controller/feedback.controller");
const router = express.Router();
const apiInfo = require("../config/api.info");
const logger = require("../config/logger"); // Import your logger
const sessionMiddleware = require("../middlewares/sessionMiddleware");

router.post("/create", createFeedback);
router.post("/create", sessionMiddleware, createFeedback);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Session middleware inconsistency detected across sensitive routes

Several routes handling sensitive operations are missing session validation:

  • newsletterRoute.js: /subscribe endpoint lacks session protection
  • adminRouter.js: Both /register and /login endpoints lack session checks
  • customerRouter.js: Multiple endpoints including /register, /login, /logout, and /reset-password missing session validation
  • eventRouter.js: /create endpoint lacks session protection
  • forgotRouter.js: Password reset flow endpoints (/verify-email, /verify-otp, /resetpassword) missing session checks

Only the following routes implement proper session validation:

  • feedbackRouter.js: /create
  • reservationRouter.js: /create
  • orderRouter.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


router.get("/", (req, res) => {
try {
Expand Down
2 changes: 1 addition & 1 deletion backend/routes/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ router.get("/", (req, res) => {

router.use("/event", eventRouter);
router.use("/admin", require("./adminRouter"));
router.use("/feedback", feedbackRouter);
router.use("/feedback", require("./feedbackRouter"));
router.use("/user", require("./customerRouter"));
router.use("/reservation", require("./reservationRouter"));
router.use("/newsletter", require("./newsletterRoute"));
Expand Down
7 changes: 4 additions & 3 deletions backend/routes/orderRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
const express = require("express");
const { createOrder, getOrders, deleteOrder } = require("../controller/order.controller.js");
const sessionMiddleware = require("../middlewares/sessionMiddleware.js");

const router = express.Router();


router.post("/create/:id", createOrder);
router.get("/get/:id", getOrders);
router.delete("/delete/:id", deleteOrder);
router.post("/create/:id", sessionMiddleware, createOrder);
router.get("/get/:id", sessionMiddleware, getOrders);
router.delete("/delete/:id", sessionMiddleware, deleteOrder);
Comment on lines +8 to +10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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:

  1. The session middleware only checks if a user exists but doesn't verify the user's identity against the customerId in the routes
  2. The createOrder and getOrders operations don't verify if the authenticated user matches the customerId
  3. Only deleteOrder partially validates ownership by checking if the order belongs to the customer
  4. 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 createOrder and getOrders similar to deleteOrder
  • 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:

  1. Validate that the authenticated user owns or has permission to access the order specified by :id
  2. 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


module.exports = router;
3 changes: 2 additions & 1 deletion backend/routes/reservationRouter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
const express = require("express");
const { createReservation } = require("../controller/reservation.controller");
const sessionMiddleware = require("../middlewares/sessionMiddleware");
const router = express.Router();

router.post("/create", createReservation);
router.post("/create", sessionMiddleware, createReservation);
router.get("/", (req, res) => {
res.json({
message: "Welcome to the restaurant reservation API!",
Expand Down
10 changes: 9 additions & 1 deletion frontend/src/components/Shared/Navbar.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider improving the API URL configuration.

While the implementation works, consider these improvements for better maintainability and security:

  1. Move this configuration to a dedicated config file (e.g., config.js)
  2. Add URL format validation
  3. 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 menuItems = [
{ name: 'HOME', path: '/' },
Expand Down Expand Up @@ -43,9 +44,16 @@ const Navbar = () => {
setIsMenuOpen(!isMenuOpen);
};

const handleLogout = () => {
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`, {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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: /logout in customerRouter.js

This path mismatch needs to be fixed either by:

  1. Updating the frontend URL to match the backend route, or
  2. 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

method: 'POST',
headers: {
'Content-Type': 'application/json',
},
})

Comment on lines +47 to +56
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling and cleanup commented code.

The logout implementation needs improvements in error handling and cleanup:

  1. Missing error handling for failed API calls
  2. Response status not verified
  3. Contains commented out code
  4. 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.

Suggested change
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
}
};

Cookies.remove('authToken');
setToken(null);
setIsModalOpen(false); // Close the modal
Expand Down
Loading