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.

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

Feat/contactus page #404

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
84 changes: 84 additions & 0 deletions backend/controller/contact.controller.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
const { z } = require("zod");
const nodemailer = require("nodemailer");
const logger = require("../utils/logger");
require("dotenv").config();

const requiredEnvVars = ["EMAIL_USER", "EMAIL_PASS"];
const missingEnvVars = requiredEnvVars.filter((envVar) => !process.env[envVar]);

if (missingEnvVars.length > 0) {
throw new Error(
`Missing required environment variables: ${missingEnvVars.join(", ")}`
);
}

// Define the Zod schema for contact form validation
const contactSchema = z.object({
mail: z.string().email(),
subject: z.string().min(5, "Subject must be at least 5 characters long."),
message: z.string().min(5, "Message must be at least 5 characters long."),
});

const createContactUs = async (req, res) => {
const validation = contactSchema.safeParse(req.body);

if (!validation.success) {
console.error("Error at validation");
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

Use logger instead of console.error for consistent logging

Replace console.error with logger.error to maintain consistent logging practices throughout the application.

Apply this diff:

26   -    console.error("Error at validation");
26   +    logger.error("Validation failed", { requestId: req.id });

Committable suggestion was skipped due to low confidence.

return res.status(400).json({
status: "error",
errors: validation.error.errors,
});
}

const { mail, subject, message } = req.body;

try {
const transporter = nodemailer.createTransport({
service: "gmail",
host: "smtp.gmail.com",
port: 587,
secure: false,
auth: {
user: process.env.EMAIL_USER,
pass: process.env.EMAIL_PASS,
},
tls: {
rejectUnauthorized: false, // Disable strict SSL verification
},
});

const sanitizeInput = (str) => {
return str.replace(/[<>]/g, "").trim();
};

const mailOptions = {
from: `"Contact Form" <${process.env.EMAIL_USER}>`,
replyTo: sanitizeInput(mail),
to: process.env.EMAIL_USER,
subject: sanitizeInput(subject),
text: sanitizeInput(message),
};

// Use built-in promise interface
await transporter.sendMail(mailOptions);
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

Missing success response after sending email

After successfully sending the email, the function does not send a response back to the client, which may cause the client to hang or timeout.

Apply this diff to send a success response:

63     await transporter.sendMail(mailOptions);
+    res.status(200).json({
+      status: "success",
+      message: "Your message has been sent 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
await transporter.sendMail(mailOptions);
await transporter.sendMail(mailOptions);
res.status(200).json({
status: "success",
message: "Your message has been sent successfully.",
});

} catch (err) {
logger.error("Validation failed", {
errors: validation.error.errors,
requestId: req.id,
});
Comment on lines +65 to +68
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

Incorrect logging of validation errors in error handler

In the error handler, logging validation.error.errors may not be appropriate if the error occurs during email sending, as validation has already succeeded. This could lead to misleading logs or additional errors if validation.error.errors is undefined.

Consider logging the actual error message instead.

Apply this diff to correct the logging:

65     logger.error("Validation failed", {
-      errors: validation.error.errors,
+      error: err.message,
       requestId: req.id,
     });

Committable suggestion was skipped due to low confidence.


const statusCode = err.code === "EAUTH" ? 401 : 500;
const errorMessage =
process.env.NODE_ENV === "production"
? "There was an error sending your message. Please try again later."
: err.message;

res.status(statusCode).json({
status: "error",
message: errorMessage,
requestId: req.id,
});
}
};

module.exports = { createContactUs };
46 changes: 46 additions & 0 deletions backend/routes/contactUsRouter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
const express = require("express");
const router = express.Router();
const { createContactUs } = require("../controller/contact.controller");
const rateLimit = require("express-rate-limit");
const { body } = require("express-validator");
Comment on lines +1 to +5
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 missing import for validationResult

The validationResult function is used on line 38 but hasn't been imported from express-validator.

Add the following import:

-const { body } = require("express-validator");
+const { body, validationResult } = require("express-validator");
📝 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 express = require("express");
const router = express.Router();
const { createContactUs } = require("../controller/contact.controller");
const rateLimit = require("express-rate-limit");
const { body } = require("express-validator");
const express = require("express");
const router = express.Router();
const { createContactUs } = require("../controller/contact.controller");
const rateLimit = require("express-rate-limit");
const { body, validationResult } = require("express-validator");


// Error handling middleware
router.use((err, req, res, next) => {
console.error(err.stack);
const statusCode = err.statusCode || 500;
const errorType = err.type || "InternalServerError";
res.status(500).json({
status: "error",
message: err.message || "An error occurred processing your request",
type: errorType,
...(process.env.NODE_ENV === "development" && { stack: err.stack }),
});
});
Comment on lines +8 to +18
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

Fix inconsistent HTTP status code usage

The middleware captures a dynamic statusCode but always responds with 500. This could lead to incorrect error reporting for client-side errors (4xx).

Apply this fix:

-  res.status(500).json({
+  res.status(statusCode).json({
📝 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.use((err, req, res, next) => {
console.error(err.stack);
const statusCode = err.statusCode || 500;
const errorType = err.type || "InternalServerError";
res.status(500).json({
status: "error",
message: err.message || "An error occurred processing your request",
type: errorType,
...(process.env.NODE_ENV === "development" && { stack: err.stack }),
});
});
router.use((err, req, res, next) => {
console.error(err.stack);
const statusCode = err.statusCode || 500;
const errorType = err.type || "InternalServerError";
res.status(statusCode).json({
status: "error",
message: err.message || "An error occurred processing your request",
type: errorType,
...(process.env.NODE_ENV === "development" && { stack: err.stack }),
});
});


const contactFormLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 5, // limit each IP to 5 requests per window
message: {
status: "error",
message: "Too many requests, please try again later",
},
});

router.post(
"/",
contactFormLimiter,
[
body("email").isEmail().normalizeEmail(),
body("name").trim().isLength({ min: 2 }).escape(),
body("message").trim().isLength({ min: 10, max: 1000 }).escape(),
],
async (req, res, next) => {
const errors = validationResult(req);
if (!errors.isEmpty()) {
return res.status(400).json({ status: "error", errors: errors.array() });
}
await createContactUs(req, res, next);
}
Comment on lines +37 to +43
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 proper error handling for async route

The async route handler lacks try-catch block for error handling, which could lead to unhandled promise rejections.

Implement proper error handling:

 async (req, res, next) => {
-    const errors = validationResult(req);
-    if (!errors.isEmpty()) {
-      return res.status(400).json({ status: "error", errors: errors.array() });
-    }
-    await createContactUs(req, res, next);
+    try {
+      const errors = validationResult(req);
+      if (!errors.isEmpty()) {
+        return res.status(400).json({ status: "error", errors: errors.array() });
+      }
+      await createContactUs(req, res, next);
+    } catch (error) {
+      next(error);
+    }
 }
📝 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 (req, res, next) => {
const errors = validationResult(req);
if (!errors.isEmpty()) {
return res.status(400).json({ status: "error", errors: errors.array() });
}
await createContactUs(req, res, next);
}
async (req, res, next) => {
try {
const errors = validationResult(req);
if (!errors.isEmpty()) {
return res.status(400).json({ status: "error", errors: errors.array() });
}
await createContactUs(req, res, next);
} catch (error) {
next(error);
}
}

);

module.exports = router;
Comment on lines +1 to +46
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

Add request size limits and CSRF protection

The codebase has proper CORS configuration with specific allowed origins, but still needs additional security measures:

  • Add request size limits to express.json() middleware to prevent large payload attacks
  • Implement CSRF protection using a package like csurf for browser-based submissions
  • Current rate limiting is good but only applied to contact form endpoint

Example implementation:

app.use(express.json({ limit: '10kb' }));
🔗 Analysis chain

Consider adding additional security measures

While the current implementation includes rate limiting and input sanitization, consider these security enhancements:

  1. Add CORS configuration to restrict access to trusted domains
  2. Implement request size limits to prevent payload attacks
  3. Consider adding CSRF protection if the form is used in a browser context

Let's check if CORS is configured in the main app:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CORS configuration
rg -l 'cors' --type js

Length of output: 39


Script:

#!/bin/bash
# Check the CORS configuration in index.js
rg 'cors' -A 5 -B 5 backend/index.js

Length of output: 1178


Script:

#!/bin/bash
# Check for request size limits configuration
rg -i 'limit|bodyparser|express.json' -A 5 -B 5 backend/index.js

# Check for CSRF protection
rg -i 'csrf|csurf' --type js

Length of output: 279

114 changes: 78 additions & 36 deletions backend/routes/index.js
Original file line number Diff line number Diff line change
@@ -1,57 +1,99 @@
const express = require("express");
const logger = require("../config/logger"); // Import your Winston logger
const logger = require("../config/logger"); // Import Winston logger
require("dotenv").config();

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 router = express.Router();

// Utility function to safely load modules and handle errors
const safeRequire = (modulePath, fallbackMessage) => {
try {
return require(modulePath);
} catch (error) {
const errorDetails = {
module: modulePath.split("/").pop(),
message: error.message,
stack: process.env.NODE_ENV === "development" ? error.stack : undefined,
};
logger.error("Module loading error:", errorDetails);

// Return a pre-defined handler to avoid creating closures
return safeRequire.errorHandler(fallbackMessage);
}
};

const router = express.Router();
// Pre-defined error handler to avoid creating closures
safeRequire.errorHandler = (message) => (req, res) => {
res.status(503).json({
status: "error",
message:
process.env.NODE_ENV === "production"
? message
: `Service unavailable: ${message}`,
});
};
Comment on lines +7 to +33
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 enhancing error handling flexibility.

While the safeRequire utility is well-structured, consider these improvements:

  1. The 503 status code implies temporary unavailability, which might not always be accurate.
  2. Exposing stack traces in development mode could pose a security risk if accidentally deployed.

Consider this enhanced implementation:

 const safeRequire = (modulePath, fallbackMessage) => {
   try {
     return require(modulePath);
   } catch (error) {
     const errorDetails = {
       module: modulePath.split("/").pop(),
       message: error.message,
-      stack: process.env.NODE_ENV === "development" ? error.stack : undefined,
+      // Log stack trace but don't include in error details
     };
+    if (process.env.NODE_ENV === "development") {
+      logger.debug("Stack trace:", error.stack);
+    }
     logger.error("Module loading error:", errorDetails);
     return safeRequire.errorHandler(fallbackMessage);
   }
 };

-safeRequire.errorHandler = (message) => (req, res) => {
+safeRequire.errorHandler = (message, statusCode = 500) => (req, res) => {
   res.status(503).json({
     status: "error",
     message:
       process.env.NODE_ENV === "production"
         ? message
         : `Service unavailable: ${message}`,
   });
 };

Committable suggestion was skipped due to low confidence.


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

let eventRouter;
try {
eventRouter = require("./eventRouter");
} catch (error) {
logger.error("Error loading eventRouter:", error); // Log the error with Winston
eventRouter = (req, res) => {
res
.status(500)
.json({ error: "Event functionality is currently unavailable" });
};
}
// Safely load routers with error handling
const feedbackRouter = safeRequire(
"./feedbackRouter",
"Feedback functionality is currently unavailable"
);
const contactUsRouter = safeRequire(
"./contactUsRouter",
"Contact Us functionality is currently unavailable"
);
const eventRouter = safeRequire(
"./eventRouter",
"Event functionality is currently unavailable"
);

router.get("/", (req, res) => {
return res.json({
message: "Welcome to the restaurant API!",
version: "1.0.0",
endpoints: {
Reservation: "/reservation",
Feedback: "/feedback", // Added feedback endpoint documentation
Feedback: "/feedback",
},
documentation: "https://api-docs-url.com",
});
});

Comment on lines 48 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

Update API documentation to include new endpoints.

The API documentation is missing the newly added contact endpoint. Please update the endpoints object to include it.

 endpoints: {
   Reservation: "/reservation",
   Feedback: "/feedback",
+  Contact: "/contact",
 },
📝 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.get("/", (req, res) => {
return res.json({
message: "Welcome to the restaurant API!",
version: "1.0.0",
endpoints: {
Reservation: "/reservation",
Feedback: "/feedback", // Added feedback endpoint documentation
Feedback: "/feedback",
},
documentation: "https://api-docs-url.com",
});
});
router.get("/", (req, res) => {
return res.json({
message: "Welcome to the restaurant API!",
version: "1.0.0",
endpoints: {
Reservation: "/reservation",
Feedback: "/feedback",
Contact: "/contact",
},
documentation: "https://api-docs-url.com",
});
});

// Authentication routes
router.use(
"/admin",
safeRequire("./adminRouter", "Admin functionality is currently unavailable")
);
router.use(
"/user",
safeRequire("./customerRouter", "User functionality is currently unavailable")
);
router.use(
"/forgot",
safeRequire(
"./forgotRouter",
"Forgot password functionality is currently unavailable"
)
);

// Core feature routes
router.use(
"/reservation",
safeRequire(
"./reservationRouter",
"Reservation functionality is currently unavailable"
)
);
router.use("/event", eventRouter);
router.use("/admin", require("./adminRouter"));

// Feedback and communication routes
router.use("/feedback", feedbackRouter);
router.use("/user", require("./customerRouter"));
router.use("/reservation", require("./reservationRouter"));
router.use("/newsletter", require("./newsletterRoute"));
router.use("/forgot", require("./forgotRouter"));
router.use("/contact", contactUsRouter);
router.use(
"/newsletter",
safeRequire(
"./newsletterRoute",
"Newsletter functionality is currently unavailable"
)
);
Comment on lines +61 to +97
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

Found validation inconsistency in contact route implementation

There are a few issues in the implementation that need to be addressed:

  • The router uses express-validator but validationResult is not imported
  • The controller uses zod schema validation which conflicts with the router's express-validator
  • The request body field names don't match between router (email, name, message) and controller (mail, subject, message)
  • The error handling middleware in the router is placed before the routes, which is incorrect
🔗 Analysis chain

LGTM! Route organization is clean and consistent.

The routes are well-organized into logical groups (authentication, core features, communication) with consistent error handling. The new contact route is properly integrated.

Let's verify the contact route implementation:


Let me verify the implementation details of these files to ensure everything is properly connected.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the contact route implementation and dependencies

# Check if the referenced contact router exists
echo "Checking for contactUsRouter..."
fd -t f "contactUsRouter.js" backend/routes

# Check for the controller implementation
echo "Checking for contact controller..."
fd -t f "contact.controller.js" backend/controller

# Look for contact-related middleware
echo "Checking for validation middleware..."
rg -l "contact.*validation" backend/

Length of output: 486


Script:

#!/bin/bash
# Check the implementation of contactUsRouter and its controller

# Check contactUsRouter implementation
echo "=== Contact Router Implementation ==="
cat backend/routes/contactUsRouter.js

# Check controller implementation
echo -e "\n=== Contact Controller Implementation ==="
cat backend/controller/contact.controller.js

# Check for any validation middleware imports
echo -e "\n=== Checking Validation Imports ==="
rg "require.*validation" backend/routes/contactUsRouter.js backend/controller/contact.controller.js

Length of output: 4142


module.exports = router;
Loading