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

Add feat/contactus-page branch #394

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

// data require form .env file : EMAIL_USER, EMAIL_PASS

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 environment variables validation on startup.

The code requires EMAIL_USER and EMAIL_PASS environment variables but doesn't validate their presence. This could lead to runtime errors if they're missing.

Add this validation at the beginning of the file:

 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(', ')}`);
+}
📝 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 { z } = require("zod");
const nodemailer = require("nodemailer");
require("dotenv").config();
// data require form .env file : EMAIL_USER, EMAIL_PASS
const { z } = require("zod");
const nodemailer = require("nodemailer");
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(', ')}`);
}
// data require form .env file : EMAIL_USER, EMAIL_PASS

// 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."),
});
Comment on lines +16 to +20
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 validation constraints for better security.

Consider adding these safety measures to prevent abuse:

  • Maximum length constraints for all fields
  • Consistent error messages (add one for email)
  • Rate limiting headers in the response
 const contactSchema = z.object({
-  mail: z.string().email(),
+  mail: z.string().email("Please provide a valid email address").max(255, "Email must not exceed 255 characters"),
-  subject: z.string().min(5, "Subject must be at least 5 characters long."),
+  subject: z.string().min(5, "Subject must be at least 5 characters long.").max(200, "Subject must not exceed 200 characters"),
-  message: z.string().min(5, "Message must be at least 5 characters long."),
+  message: z.string().min(5, "Message must be at least 5 characters long.").max(5000, "Message must not exceed 5000 characters"),
 });
📝 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 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 contactSchema = z.object({
mail: z.string().email("Please provide a valid email address").max(255, "Email must not exceed 255 characters"),
subject: z.string().min(5, "Subject must be at least 5 characters long.").max(200, "Subject must not exceed 200 characters"),
message: z.string().min(5, "Message must be at least 5 characters long.").max(5000, "Message must not exceed 5000 characters"),
});


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

if (!validation.success) {
console.error("Error at validation");
return res.status(400).json({
status: "error",
errors: validation.error.errors,
});
}
Comment on lines +25 to +31
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

Use logger instead of console.error for consistency.

The error logging should use the logger instance that's already imported.

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

Committable suggestion was skipped due to low confidence.


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

Security and reliability concerns in email transport configuration.

  1. Disabling SSL verification (rejectUnauthorized: false) is a significant security risk that could expose the application to man-in-the-middle attacks.
  2. Missing important transport configurations:
    • Connection timeout
    • Connection pool
    • Retry mechanism
 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
-  },
+  pool: true,
+  maxConnections: 5,
+  maxMessages: 100,
+  rateDelta: 1000,
+  rateLimit: 5,
+  timeout: 5000,
 });
📝 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 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 transporter = nodemailer.createTransport({
service: "gmail",
host: "smtp.gmail.com",
port: 587,
secure: false,
auth: {
user: process.env.EMAIL_USER,
pass: process.env.EMAIL_PASS,
},
pool: true,
maxConnections: 5,
maxMessages: 100,
rateDelta: 1000,
rateLimit: 5,
timeout: 5000,
});


const mailOptions = {
from: mail,
to: process.env.EMAIL_USER,
subject: subject,
text: message,
};

// Send mail with defined transport object
await new Promise((resolve, reject) => {
transporter.sendMail(mailOptions, (error, info) => {
if (error) {
console.error("Error occurred: " + error.message);
reject(error);
}
resolve(info);
});
});
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

Sanitize user input and improve email handling.

The current implementation directly uses user input in email fields, which could be exploited for email injection attacks. Additionally, the Promise wrapper could be simplified using nodemailer's built-in promises.

+ const sanitizeInput = (str) => {
+   return str.replace(/[<>]/g, '').trim();
+ };
+
 const mailOptions = {
-  from: mail,
+  from: `"Contact Form" <${process.env.EMAIL_USER}>`,
+  replyTo: sanitizeInput(mail),
   to: process.env.EMAIL_USER,
-  subject: subject,
-  text: message,
+  subject: sanitizeInput(subject),
+  text: sanitizeInput(message),
 };

- // Send mail with defined transport object
- await new Promise((resolve, reject) => {
-   transporter.sendMail(mailOptions, (error, info) => {
-     if (error) {
-       console.error("Error occurred: " + error.message);
-       reject(error);
-     }
-     resolve(info);
-   });
- });
+ // Use built-in promise interface
+ await transporter.sendMail(mailOptions);
📝 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 mailOptions = {
from: mail,
to: process.env.EMAIL_USER,
subject: subject,
text: message,
};
// Send mail with defined transport object
await new Promise((resolve, reject) => {
transporter.sendMail(mailOptions, (error, info) => {
if (error) {
console.error("Error occurred: " + error.message);
reject(error);
}
resolve(info);
});
});
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.

🛠️ Refactor suggestion

Simplify email sending using native promises.

The manual Promise wrapper is unnecessary as nodemailer supports promises natively.

Replace the Promise wrapper with direct await:

-    await new Promise((resolve, reject) => {
-      transporter.sendMail(mailOptions, (error, info) => {
-        if (error) {
-          console.error("Error occurred: " + error.message);
-          reject(error);
-        }
-        resolve(info);
-      });
-    });
+    await transporter.sendMail(mailOptions);
📝 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 new Promise((resolve, reject) => {
transporter.sendMail(mailOptions, (error, info) => {
if (error) {
console.error("Error occurred: " + error.message);
reject(error);
}
resolve(info);
});
});
await transporter.sendMail(mailOptions);


res.status(200).json({
status: "success",
message: "Your contact request has been successfully received.",
});
} catch (err) {
console.error(`Error at transport: ${err}`);
res.status(500).json({
status: "error",
message:
"There was an error sending your message. Please try again later.",
});
}
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 error handling and logging.

The current error handling lacks proper error classification and logging structure. Consider implementing:

  1. Structured logging with request IDs
  2. Different error responses based on error types
  3. Monitoring for email sending failures
+ const logger = require('../utils/logger');  // Implement a proper logger
+
 } catch (err) {
-  console.error(`Error at transport: ${err}`);
+  logger.error('Email sending failed', {
+    error: err.message,
+    requestId: req.id,
+    userEmail: mail,
+  });
+
+  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(500).json({
+  res.status(statusCode).json({
     status: "error",
-    message:
-      "There was an error sending your message. Please try again later.",
+    message: errorMessage,
+    requestId: req.id,
   });
 }

Committable suggestion was skipped due to low confidence.

};

module.exports = { createContactUs };
7 changes: 7 additions & 0 deletions backend/routes/contactUsRouter.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const express = require("express");
const router = express.Router();
const { createContactUs } = require("../controller/contact.controller");
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 adding middleware imports for security.

The basic router setup looks good, but contact forms need protection against abuse.

Consider adding these essential middleware:

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

Committable suggestion was skipped due to low confidence.


router.post("/contactus", createContactUs);
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 security middleware to the route.

The route needs protection against spam and validation before reaching the controller.

Apply this diff to add essential protections:

-router.post("/contactus", createContactUs);
+const contactFormLimiter = rateLimit({
+  windowMs: 60 * 60 * 1000, // 1 hour window
+  max: 5, // limit each IP to 5 requests per window
+  message: "Too many contact requests, please try again later"
+});
+
+router.post("/contactus",
+  contactFormLimiter,
+  [
+    body("email").isEmail().normalizeEmail(),
+    body("message").trim().notEmpty().escape(),
+    body("name").trim().notEmpty().escape()
+  ],
+  createContactUs
+);

Committable suggestion was skipped due to low confidence.


💡 Codebase verification

Route path misalignment detected - needs correction

The verification confirms there is a path misalignment:

  • The main router mounts the contact routes at /contact
  • The route is defined as /contactus

This means the full path would be /contact/contactus, which seems redundant and possibly not the intended path structure. Either:

  • Change the route definition to / in contactUsRouter.js, or
  • Update the mount point in index.js to /
🔗 Analysis chain

Verify the route path alignment.

The AI summary mentions this router is mounted at "/contact" in the main router, but the route is defined as "/contactus".

Let's verify the route mounting configuration:


Let's check the actual mounting configuration in the index.js file to verify the path alignment.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check route mounting configuration
# Expected: Find the route mounting point to verify path alignment

rg -l 'router\.use.*contact' --type js

Length of output: 64


Script:

#!/bin/bash
# Check the actual mounting configuration in index.js
rg "router\.use.*contact" --type js -A 2 -B 2

Length of output: 462


module.exports = router;
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

Add error handling middleware.

The router should have error handling middleware to ensure proper response formatting.

Add this error handling setup:

 const express = require("express");
 const router = express.Router();
 const { createContactUs } = require("../controller/contact.controller"); 
+const { validationResult } = require("express-validator");
+
+// Error handling middleware
+router.use((err, req, res, next) => {
+  console.error(err.stack);
+  res.status(500).json({ 
+    status: "error",
+    message: "An error occurred processing your request"
+  });
+});

 router.post("/contactus", createContactUs);

 module.exports = router;

Committable suggestion was skipped due to low confidence.

60 changes: 24 additions & 36 deletions backend/routes/index.js
Original file line number Diff line number Diff line change
@@ -1,57 +1,45 @@
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();

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" });
};
}
// Utility function to safely load modules and handle errors
const safeRequire = (modulePath, fallbackMessage) => {
try {
return require(modulePath);
} catch (error) {
logger.error(`Error loading ${modulePath}:`, error);
return (req, res) => {
res.status(500).json({ error: fallbackMessage });
};
}
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 error handling and security in safeRequire.

The current implementation has several areas for improvement:

  1. Error responses expose internal paths in production
  2. All errors return 500, which might not always be appropriate
  3. Creating new closures for each error is inefficient

Consider this improved implementation:

 const safeRequire = (modulePath, fallbackMessage) => {
   try {
     return require(modulePath);
   } catch (error) {
-    logger.error(`Error loading ${modulePath}:`, 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);
   }
-    return (req, res) => {
-      res.status(500).json({ error: fallbackMessage });
-    };
 };
+
+// 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}`
+  });
+};
📝 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 safeRequire = (modulePath, fallbackMessage) => {
try {
return require(modulePath);
} catch (error) {
logger.error(`Error loading ${modulePath}:`, error);
return (req, res) => {
res.status(500).json({ error: fallbackMessage });
};
}
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);
}
};
// 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}`
});
};

};

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

router.use("/event", eventRouter);
router.use("/admin", require("./adminRouter"));
router.use("/admin", safeRequire("./adminRouter", "Admin functionality is currently unavailable"));
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("/user", safeRequire("./customerRouter", "User functionality is currently unavailable"));
router.use("/reservation", safeRequire("./reservationRouter", "Reservation functionality is currently unavailable"));
router.use("/newsletter", safeRequire("./newsletterRoute", "Newsletter functionality is currently unavailable"));
router.use("/forgot", safeRequire("./forgotRouter", "Forgot password functionality is currently unavailable"));
router.use("/contact", contactUsRouter);
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 organizing router configurations for better maintainability.

While the implementations are correct, consider grouping related endpoints together and adding comments for better organization.

Consider this structure:

+ // 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);
+
+ // Feedback and communication routes
 router.use("/feedback", feedbackRouter);
 router.use("/contact", contactUsRouter);
 router.use("/newsletter", safeRequire("./newsletterRoute", "Newsletter functionality is currently unavailable"));
📝 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("/event", eventRouter);
router.use("/admin", require("./adminRouter"));
router.use("/admin", safeRequire("./adminRouter", "Admin functionality is currently unavailable"));
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("/user", safeRequire("./customerRouter", "User functionality is currently unavailable"));
router.use("/reservation", safeRequire("./reservationRouter", "Reservation functionality is currently unavailable"));
router.use("/newsletter", safeRequire("./newsletterRoute", "Newsletter functionality is currently unavailable"));
router.use("/forgot", safeRequire("./forgotRouter", "Forgot password functionality is currently unavailable"));
router.use("/contact", contactUsRouter);
// 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);
// Feedback and communication routes
router.use("/feedback", feedbackRouter);
router.use("/contact", contactUsRouter);
router.use("/newsletter", safeRequire("./newsletterRoute", "Newsletter functionality is currently unavailable"));


module.exports = router;
185 changes: 185 additions & 0 deletions frontend/src/components/Pages/ContactUs.jsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,185 @@
import { useState } from 'react';
import { motion } from 'framer-motion';
import { useInView } from 'react-intersection-observer';
import chess from '../../assets/img/chess.gif';

const ContactUs = () => {
const { ref, inView } = useInView({
threshold: 0.2,
triggerOnce: true,
});

const animationVariants = {
hidden: { opacity: 0, y: 50 },
visible: { opacity: 1, y: 0, transition: { duration: 0.5 } },
};

// Use an environment variable for backend URL
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.

⚠️ Potential issue

Enhance environment variable handling and URL validation.

The current implementation could be improved to handle malformed URLs and validate the protocol.

Consider this safer implementation:

-const API_URL = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000';
+const DEFAULT_URL = 'http://localhost:3000';
+const API_URL = (() => {
+  try {
+    const url = new URL(import.meta.env.VITE_BACKEND_URL || DEFAULT_URL);
+    if (!['http:', 'https:'].includes(url.protocol)) {
+      throw new Error('Invalid protocol');
+    }
+    return url.toString();
+  } catch (e) {
+    console.warn('Invalid VITE_BACKEND_URL, using default:', DEFAULT_URL);
+    return DEFAULT_URL;
+  }
+})();
📝 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
// Use an environment variable for backend URL
const API_URL = import.meta.env.VITE_BACKEND_URL || 'http://localhost:3000';
// Use an environment variable for backend URL
const DEFAULT_URL = 'http://localhost:3000';
const API_URL = (() => {
try {
const url = new URL(import.meta.env.VITE_BACKEND_URL || DEFAULT_URL);
if (!['http:', 'https:'].includes(url.protocol)) {
throw new Error('Invalid protocol');
}
return url.toString();
} catch (e) {
console.warn('Invalid VITE_BACKEND_URL, using default:', DEFAULT_URL);
return DEFAULT_URL;
}
})();

const [mail, setMail] = useState('');
const [subject, setSubject] = useState('');
const [message, setMessage] = useState('');
const [submitted, setSubmitted] = useState(false);
const [error, setError] = useState(null);
const [isLoading, setIsLoading] = useState(false);

const handleSubmit = async (e) => {
e.preventDefault();

// Basic client-side validation for security
const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;

if (!mail || !subject || !message) {
setError('All fields are required.');
return;
}

if (!emailRegex.test(mail)) {
setError('Please enter a valid email address.');
return;
}

if (subject.length > 100) {
setError('Subject must be less than 100 characters.');
return;
}

if (message.length > 1000) {
setError('Message must be less than 1000 characters.');
return;
}
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 input validation and security measures.

While basic validation is present, there are opportunities to improve security and validation robustness:

  1. The email regex is simplistic
  2. No input sanitization against XSS
  3. No rate limiting on client side

Consider these improvements:

-const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/;
+const emailRegex = /^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$/;

+const sanitizeInput = (input) => {
+  return input.replace(/[<>]/g, '');
+};

+// Add to state
+const [lastSubmissionTime, setLastSubmissionTime] = useState(0);

 if (!emailRegex.test(mail)) {
   setError('Please enter a valid email address.');
   return;
 }

+// Rate limiting
+const now = Date.now();
+if (now - lastSubmissionTime < 60000) { // 1 minute
+  setError('Please wait before submitting again.');
+  return;
+}

+// Sanitize inputs before sending
+const sanitizedSubject = sanitizeInput(subject);
+const sanitizedMessage = sanitizeInput(message);

Committable suggestion was skipped due to low confidence.


setError(null);
setIsLoading(true);

try {
const response = await fetch(`${API_URL}/api/contact/contactus`, {
method: 'POST',
headers: {
'Content-Type': 'application/json',
},
body: JSON.stringify({ mail, subject, message }),
});

if (!response.ok) {
throw new Error(`Network response was not ok: ${response.status}`);
}

setSubmitted(true);
setTimeout(() => {
setMail('');
setSubject('');
setMessage('');
setSubmitted(false);
}, 4000);
} catch (error) {
setError('An error occurred while sending Mail...');
console.error('Mail sending failed : ', error);
} finally {
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

Improve API communication resilience.

The current implementation lacks timeout handling and retry logic for transient failures.

Consider implementing these improvements:

+const fetchWithTimeout = async (url, options, timeout = 5000) => {
+  const controller = new AbortController();
+  const id = setTimeout(() => controller.abort(), timeout);
+  try {
+    const response = await fetch(url, {
+      ...options,
+      signal: controller.signal
+    });
+    clearTimeout(id);
+    return response;
+  } catch (error) {
+    clearTimeout(id);
+    throw error;
+  }
+};

+const retryWithBackoff = async (fn, retries = 3, backoff = 300) => {
+  try {
+    return await fn();
+  } catch (error) {
+    if (retries === 0) throw error;
+    await new Promise(resolve => setTimeout(resolve, backoff));
+    return retryWithBackoff(fn, retries - 1, backoff * 2);
+  }
+};

 try {
-  const response = await fetch(`${API_URL}/api/contact/contactus`, {
-    method: 'POST',
-    headers: {
-      'Content-Type': 'application/json',
-    },
-    body: JSON.stringify({ mail, subject, message }),
-  });
+  const response = await retryWithBackoff(() => 
+    fetchWithTimeout(
+      `${API_URL}/api/contact/contactus`,
+      {
+        method: 'POST',
+        headers: {
+          'Content-Type': 'application/json',
+        },
+        body: JSON.stringify({ 
+          mail, 
+          subject: sanitizedSubject, 
+          message: sanitizedMessage 
+        }),
+      }
+    )
+  );

Committable suggestion was skipped due to low confidence.

setIsLoading(false);
}
};

return (
<div className="bg-amber-100 h-full py-24 px-4 sm:px-6 lg:px-8">
<div className="max-w-7xl mx-auto">
<motion.div
ref={ref}
initial="hidden"
animate={inView ? 'visible' : 'hidden'}
variants={animationVariants}
className="lg:grid lg:grid-cols-2 lg:gap-8 lg:items-center"
>
<div className="mt-8 mb-8 lg:mb-0 relative">
<h2 className="text-5xl font-black text-[#004D43]">
Feel Free To Mail Us..
</h2>
<p className="mt-5 text-lg text-gray-700 pb-3">
Have questions or need assistance ? Reach out to us, and we'll be
happy to help !!
</p>
<div className="flex md:h-[40vh] md:w-[60vh] ml-20 mt-20 items-center justify-center mt-12">
<img
src={chess}
alt="Chess"
loading="lazy"
className="md:p-10 p-5 object-contain bg-[#004D43] rounded-full shadow-2xl"
/>
</div>
</div>

<div className="bg-[#004D43] rounded-xl p-3 pt-4 mt-40 h-fit">
<form onSubmit={handleSubmit} className="space-y-4">
<div>
<label htmlFor="mail" className="sr-only">
Email Address
</label>
<input
type="email"
id="mail"
value={mail}
placeholder="Email ID"
aria-label="Email Address"
onChange={(e) => setMail(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43]"
/>
</div>
<div>
<input
type="text"
id="text"
placeholder="Subject"
value={subject}
onChange={(e) => setSubject(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43]"
/>
</div>
<div>
<textarea
id="message"
placeholder="Write your message..."
rows="6"
value={message}
onChange={(e) => setMessage(e.target.value)}
required
className="mt-1 block w-full border border-gray-300 rounded-md shadow-sm py-2 px-3 focus:outline-none focus:ring-[#004D43] focus:border-[#004D43] resize-none"
></textarea>
</div>
<div>
<button
type="submit"
className="w-full flex justify-center py-2 px-4 border border-transparent rounded-md shadow-sm text-sm font-medium text-white bg-[#09342e] hover:bg-[#072d28] focus:outline-none focus:ring-2 focus:ring-offset-2 focus:ring-[#004D43]"
disabled={isLoading}
>
{isLoading ? 'Sending...' : 'Send Mail'}
</button>
</div>
</form>
{submitted && (
<motion.div
initial={{ opacity: 0, y: -10, display: 'none', height: 0 }}
animate={{ opacity: 1, y: 0, display: 'block', height: 'auto' }}
className="mt-4 p-4 bg-green-100 border border-green-400 text-green-700 rounded"
>
Thank you, We will reply you soon...
</motion.div>
)}
{error && (
<motion.div
initial={{ opacity: 0, y: -10 }}
animate={{ opacity: 1, y: 0 }}
className="mt-4 p-4 bg-red-100 border border-red-400 text-red-700 rounded"
>
{error}
</motion.div>
)}
</div>
</motion.div>
</div>
</div>
);
};

export default ContactUs;
1 change: 1 addition & 0 deletions frontend/src/components/Shared/Navbar.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const Navbar = () => {
{ name: 'RESERVATION', path: '/reservation' },
{ name: 'BOARDGAMES', path: '/boardgame' },
{ name: 'MEMBERSHIP', path: '/membership' }, // Add Membership here
{ name: 'CONTACTUS', path: '/contactus'}
];

useEffect(() => {
Expand Down
4 changes: 3 additions & 1 deletion frontend/src/router/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import VerifyOtp from '../components/Pages/VerifyOtp';
import EmailVerify from '../components/Pages/EmailVerify';
import Membership from '../components/Membership';
import HelpAndSupport from '../components/Pages/HelpAndSupport';
import ContactUs from '../components/Pages/ContactUs';

const router = createBrowserRouter(
createRoutesFromElements(
<Route path="/" element={<App />}>
Expand All @@ -41,7 +43,7 @@ const router = createBrowserRouter(
<Route path="/email-verify" element={<EmailVerify />} />
<Route path="/membership" element={<Membership />} />
<Route path="/help" element={<HelpAndSupport />} />

<Route path="/contactus" element={<ContactUs />} />
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

Critical: Move this route before the catch-all route.

The catch-all route <Route path="*" element={<NotFound />} /> will intercept all requests, including /contactus, preventing this route from being reached. Move this route before the catch-all route to ensure it's accessible.

Apply this change:

      <Route path="/membership" element={<Membership />} />
      <Route path="/help" element={<HelpAndSupport />} />
+     <Route path="/contactus" element={<ContactUs />} />
      <Route path="*" element={<NotFound />} />
-     <Route path="/contactus" element={<ContactUs />} />
📝 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
<Route path="/contactus" element={<ContactUs />} />
<Route path="/membership" element={<Membership />} />
<Route path="/help" element={<HelpAndSupport />} />
<Route path="/contactus" element={<ContactUs />} />
<Route path="*" element={<NotFound />} />

</Route>
)
);
Expand Down