Enhance security and input validation across the application#12
Enhance security and input validation across the application#12hrtechplus wants to merge 6 commits into
Conversation
…ub.com/y4-systems/ssd-project into hasindu/injection-fixes-and-dependancy
…ication, role validation, and improved error handling
…ng across multiple files
…es-and-dependancy
|
| router.post('/createfeedback',controller.addFeedback); | ||
| router.post('/updatefeedback',controller.updateFeedback); | ||
| router.post('/deletefeedback',controller.deleteFeedback); | ||
| router.get("/feedbacks", controller.getFeedback); |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the missing rate limiting on the /feedbacks route, add a rate-limiting middleware before the controller.getFeedback handler on this route. The best practice is to use a well-known rate-limiting package, express-rate-limit, which is easy to use and effective. Add the import for express-rate-limit, define a limiter with a reasonable limit (e.g., 100 requests per 15 minutes), and insert the limiter as middleware to the /feedbacks route. Restrict changes strictly to the code you've been shown: only edit feedbackapp/backend/routes/feedbackRouter.js and add minimal required code in visible regions.
| @@ -1,9 +1,16 @@ | ||
| const express = require("express"); | ||
| const router = express.Router(); | ||
| const RateLimit = require("express-rate-limit"); | ||
| const controller = require("../controllers/feedbackController"); | ||
| const { basicAuth, requireAdmin } = require("../../../middleware/basicAuth.js"); | ||
|
|
||
| router.get("/feedbacks", controller.getFeedback); | ||
| // set up rate limiter: maximum of 100 requests per 15 minutes | ||
| const feedbacksLimiter = RateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100 // Limit each IP to 100 requests per windowMs | ||
| }); | ||
|
|
||
| router.get("/feedbacks", feedbacksLimiter, controller.getFeedback); | ||
| router.post("/createfeedback", basicAuth, controller.addFeedback); | ||
| router.post( | ||
| "/updatefeedback", |
| @@ -13,6 +13,7 @@ | ||
| "dependencies": { | ||
| "cors": "^2.8.5", | ||
| "express": "^4.19.2", | ||
| "mongoose": "^8.3.0" | ||
| "mongoose": "^8.3.0", | ||
| "express-rate-limit": "^8.1.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.1.0 | None |
| "/updatefeedback", | ||
| basicAuth, | ||
| requireAdmin, | ||
| controller.updateFeedback |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix this problem, we should apply a rate-limiting middleware to the database-modifying routes: /updatefeedback and /deletefeedback (and optionally /createfeedback and /feedbacks for completeness). The standard way is to use the express-rate-limit npm package, which is specifically designed for Express applications. The solution involves:
- Importing
express-rate-limitin this router file. - Creating a rate limiter instance for these routes (ideally, set moderate limits suitable for admin actions).
- Adding the rate limiter as middleware in the relevant route definitions, just before the controller functions.
- Minimal changes elsewhere; no change in route logic or authorization.
This involves (1) adding the required import, (2) instantiating a rate limiter, and (3) editing the route definitions to use the rate limiter.
| @@ -2,19 +2,28 @@ | ||
| const router = express.Router(); | ||
| const controller = require("../controllers/feedbackController"); | ||
| const { basicAuth, requireAdmin } = require("../../../middleware/basicAuth.js"); | ||
| const rateLimit = require("express-rate-limit"); | ||
|
|
||
| // Create a rate limiter for admin routes (adjust as needed) | ||
| const adminLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 20, // limit each IP to 20 requests per windowMs for admin actions | ||
| message: "Too many requests, please try again later.", | ||
| }); | ||
| router.get("/feedbacks", controller.getFeedback); | ||
| router.post("/createfeedback", basicAuth, controller.addFeedback); | ||
| router.post( | ||
| "/updatefeedback", | ||
| basicAuth, | ||
| requireAdmin, | ||
| adminLimiter, | ||
| controller.updateFeedback | ||
| ); | ||
| router.post( | ||
| "/deletefeedback", | ||
| basicAuth, | ||
| requireAdmin, | ||
| adminLimiter, | ||
| controller.deleteFeedback | ||
| ); | ||
|
|
| @@ -13,6 +13,7 @@ | ||
| "dependencies": { | ||
| "cors": "^2.8.5", | ||
| "express": "^4.19.2", | ||
| "mongoose": "^8.3.0" | ||
| "mongoose": "^8.3.0", | ||
| "express-rate-limit": "^8.1.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.1.0 | None |
| "/deletefeedback", | ||
| basicAuth, | ||
| requireAdmin, | ||
| controller.deleteFeedback |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the issue, add rate limiting middleware to the /deletefeedback route (and potentially other sensitive, database-modifying routes). The best practice is to use a well-known, battle-tested middleware such as express-rate-limit. This can be required at the top of the file, configured with reasonable defaults (e.g., allowing a modest number of requests per minute), and then added as middleware specifically to the affected route(s) (e.g., /deletefeedback). Only apply it to the relevant routes to avoid overly restricting the rest of the API.
Specifically:
- Import and configure
express-rate-limitnear the top offeedbackRouter.js. - Create a limiter instance (e.g., allowing 10 delete requests per 15 minutes).
- Add this limiter as middleware to the
/deletefeedbackPOST route (between authentication and controller). - Add the required import at the top of the file.
| @@ -3,6 +3,14 @@ | ||
| const controller = require("../controllers/feedbackController"); | ||
| const { basicAuth, requireAdmin } = require("../../../middleware/basicAuth.js"); | ||
|
|
||
| // Rate limiting for sensitive modifying routes | ||
| const rateLimit = require('express-rate-limit'); | ||
| const deleteFeedbackLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 10, // limit each IP to 10 delete requests per windowMs | ||
| message: 'Too many delete requests from this IP, please try again after 15 minutes' | ||
| }); | ||
|
|
||
| router.get("/feedbacks", controller.getFeedback); | ||
| router.post("/createfeedback", basicAuth, controller.addFeedback); | ||
| router.post( | ||
| @@ -15,6 +23,7 @@ | ||
| "/deletefeedback", | ||
| basicAuth, | ||
| requireAdmin, | ||
| deleteFeedbackLimiter, | ||
| controller.deleteFeedback | ||
| ); | ||
|
|
| @@ -13,6 +13,7 @@ | ||
| "dependencies": { | ||
| "cors": "^2.8.5", | ||
| "express": "^4.19.2", | ||
| "mongoose": "^8.3.0" | ||
| "mongoose": "^8.3.0", | ||
| "express-rate-limit": "^8.1.0" | ||
| } | ||
| } |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.1.0 | None |
| try { | ||
| let result = await Service.findByIdAndUpdate( | ||
| req.params.id, | ||
| { $set: req.body }, |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix the problem, the handler should use a whitelist approach: only allow specific fields from req.body to be updated. This can be done by extracting only the allowed properties from req.body and using those in the $set operation. This prevents injection of arbitrary MongoDB operators and restricts updates to safe fields.
To implement this, specify a list (array) of allowed updatable fields (e.g., "name", "description", "price", etc.), and construct an update object by picking only these keys from req.body.
All code changes can be accomplished within the updateService function of vendorapp/backend/controllers/serviceController.js. No new libraries are needed, but you may add a helper function within this file to pick whitelisted fields.
| @@ -65,11 +65,28 @@ | ||
| } | ||
| }; | ||
|
|
||
| // Only allow specific fields to be updated | ||
| const updateService = async (req, res) => { | ||
| try { | ||
| // Whitelist of fields that can be updated | ||
| const allowedFields = [ | ||
| "name", | ||
| "description", | ||
| "price", | ||
| "category", | ||
| "available", | ||
| // Add other legitimate updatable fields here as needed | ||
| ]; | ||
| const updateObj = {}; | ||
| allowedFields.forEach((field) => { | ||
| if (Object.prototype.hasOwnProperty.call(req.body, field)) { | ||
| updateObj[field] = req.body[field]; | ||
| } | ||
| }); | ||
|
|
||
| let result = await Service.findByIdAndUpdate( | ||
| req.params.id, | ||
| { $set: req.body }, | ||
| { $set: updateObj }, | ||
| { new: true } | ||
| ); | ||
|
|
| const existingVendorByEmail = await Vendor.findOne({ | ||
| email: req.body.email, | ||
| }); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix this vulnerability, we should ensure that user-controlled values (such as req.body.email and req.body.shopName) are interpreted as literals and not as query objects. For MongoDB/Mongoose, a reliable way to achieve this is to use the $eq operator for these fields in the query. Alternatively, we could validate that input values are strings before embedding them in queries.
The single best fix involves editing the database queries on lines 16, 17, and 19 to ensure that user input is wrapped in an object with a $eq operator, i.e., { email: { $eq: req.body.email } }. We should do the same for shopName.
No changes to imports or method signatures are needed; only lines 16–19 need editing in file vendorapp/backend/controllers/vendorController.js. Optionally, you might add a comment for clarity.
| @@ -14,9 +14,9 @@ | ||
| }); | ||
|
|
||
| const existingVendorByEmail = await Vendor.findOne({ | ||
| email: req.body.email, | ||
| email: { $eq: req.body.email }, | ||
| }); | ||
| const existingShop = await Vendor.findOne({ shopName: req.body.shopName }); | ||
| const existingShop = await Vendor.findOne({ shopName: { $eq: req.body.shopName } }); | ||
|
|
||
| if (existingVendorByEmail) { | ||
| res.send({ message: "Email already exists" }); |
| const existingVendorByEmail = await Vendor.findOne({ | ||
| email: req.body.email, | ||
| }); | ||
| const existingShop = await Vendor.findOne({ shopName: req.body.shopName }); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
To fix this problem, we need to ensure that the shopName used in the MongoDB query is a primitive string, not a potentially malicious object. There are two principal ways to do this: (1) use the $eq operator in the query to interpret user input as a literal value, or (2) explicitly check that req.body.shopName is a string before using it in the query. The preferred and most robust fix is to use the $eq operator in the query: { shopName: { $eq: req.body.shopName } }. This is a minimal change that prevents the query object from being manipulated by malicious input, without changing existing functionality or requiring broad input validation. Make this change only on the line where the tainted query object is built: replace line 19.
| @@ -16,7 +16,7 @@ | ||
| const existingVendorByEmail = await Vendor.findOne({ | ||
| email: req.body.email, | ||
| }); | ||
| const existingShop = await Vendor.findOne({ shopName: req.body.shopName }); | ||
| const existingShop = await Vendor.findOne({ shopName: { $eq: req.body.shopName } }); | ||
|
|
||
| if (existingVendorByEmail) { | ||
| res.send({ message: "Email already exists" }); |
| if (validated) { | ||
| vendor.password = undefined; | ||
| if (req.body.email && req.body.password) { | ||
| let vendor = await Vendor.findOne({ email: req.body.email }); |
Check failure
Code scanning / CodeQL
Database query built from user-controlled sources High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 10 months ago
The safest and best fix is to ensure that the value inserted into the query is not interpreted as an object containing operators, but as a plain string. This can be done in two standard ways:
- Use the
$eqoperator in the query:{ email: { $eq: req.body.email } }, which forces MongoDB to interpret the user input as a value, not as an object holding a query operator. - Alternatively, explicitly check the type of
req.body.emailbefore querying, and reject the request if it is not a string.
Following the recommendation in the background, and to minimize changes, the best fix is to update the query argument on line 45 to use the $eq operator, i.e.:
let vendor = await Vendor.findOne({ email: { $eq: req.body.email } });This ensures any injected object is interpreted as a value, not as an operator, and does not otherwise affect the rest of the code.
Only the line containing the database query (line 45) needs to be changed. The change is small and does not require new dependencies or imports.
| @@ -42,7 +42,7 @@ | ||
|
|
||
| const vendorLogIn = async (req, res) => { | ||
| if (req.body.email && req.body.password) { | ||
| let vendor = await Vendor.findOne({ email: req.body.email }); | ||
| let vendor = await Vendor.findOne({ email: { $eq: req.body.email } }); | ||
| if (vendor) { | ||
| const validated = await bcrypt.compare( | ||
| req.body.password, |




Improvements include enhanced JWT verification, input validation for search queries, and secure package upload and deletion endpoints with role-based authentication. CORS configuration and error handling have also been refined to bolster overall security.