Skip to content
Merged
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
25 changes: 25 additions & 0 deletions controllers/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -561,6 +561,30 @@ const updateSelf = async (req, res, next) => {
}
};

const handleUserPictureUpload = async (req, res) => {
if (req.body.type === "application") {
return postApplicationUserPicture(req, res);
}
return postUserPicture(req, res);
};

const postApplicationUserPicture = async (req, res) => {
const { file } = req;
const { id: userId } = req.userData;
const { coordinates } = req.body;
try {
const coordinatesObject = coordinates && JSON.parse(coordinates);
const imageData = await imageService.uploadProfilePicture({ file, userId, coordinates: coordinatesObject });
return res.status(201).json({
message: "Application picture uploaded successfully!",
image: imageData,
});
} catch (error) {
logger.error(`Error while uploading application picture: ${error}`);
return res.boom.badImplementation(INTERNAL_SERVER_ERROR);
}
};

/**
* Post user profile picture
*
Expand Down Expand Up @@ -1186,4 +1210,5 @@ module.exports = {
getIdentityStats,
updateUsernames,
updateProfile,
handleUserPictureUpload,
};
8 changes: 8 additions & 0 deletions middlewares/pictureRouteMiddleware.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
const skipWhenApplicationType = (middleware) => {
return (req, res, next) => {
if (req.body.type === "application") return next();
return middleware(req, res, next);
};
};

module.exports = skipWhenApplicationType;
9 changes: 8 additions & 1 deletion routes/users.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
const { devFlagMiddleware } = require("../middlewares/devFlag");
const { userAuthorization } = require("../middlewares/userAuthorization");
const conditionalMiddleware = require("../middlewares/conditionalMiddleware");
const skipWhenApplicationType = require("../middlewares/pictureRouteMiddleware");

router.post("/", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), users.markUnverified);
router.post("/update-in-discord", authenticate, authorizeRoles([SUPERUSER]), users.setInDiscordScript);
Expand Down Expand Up @@ -65,7 +66,13 @@
);

// upload.single('profile') -> multer inmemory storage of file for type multipart/form-data
router.post("/picture", authenticate, checkIsVerifiedDiscord, upload.single("profile"), users.postUserPicture);
router.post(
"/picture",
authenticate,

Check failure

Code scanning / CodeQL

Missing rate limiting High

This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.
This route handler performs
authorization
, but is not rate-limited.

Copilot Autofix

AI 7 days ago

In general, this should be fixed by introducing a rate-limiting middleware (for example using the well-known express-rate-limit package) and applying it to the sensitive route(s) that perform authentication/authorization and potentially expensive operations. The middleware should be configured with reasonable windowMs and max values to balance protection and usability, and should be placed early enough in the middleware chain that expensive handlers (like file parsing and business logic) are not invoked when the limit is exceeded.

For this specific code, the most targeted change that does not alter existing functionality is:

  • Import express-rate-limit at the top of routes/users.js.
  • Define a limiter specifically for the /picture upload route (e.g., something like userPictureUploadLimiter allowing a small number of uploads per minute per IP).
  • Insert this limiter in the middleware chain for the /picture POST route before the Multer upload.single("profile") middleware, so that excessive requests are rejected before file parsing and subsequent authorization/Discord checks.

Concretely:

  • At the top of routes/users.js after the existing require statements, add const rateLimit = require("express-rate-limit"); and a new const userPictureUploadLimiter = rateLimit({ ... }) definition.
  • Modify the router.post("/picture", ...) route to insert userPictureUploadLimiter between authenticate and upload.single("profile").
    No other routes need to be changed to address this specific alert; the change remains tightly scoped to the flagged route and uses a standard, well-known dependency.
Suggested changeset 2
routes/users.js

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/routes/users.js b/routes/users.js
--- a/routes/users.js
+++ b/routes/users.js
@@ -16,7 +16,15 @@
 const { userAuthorization } = require("../middlewares/userAuthorization");
 const conditionalMiddleware = require("../middlewares/conditionalMiddleware");
 const skipWhenApplicationType = require("../middlewares/pictureRouteMiddleware");
+const rateLimit = require("express-rate-limit");
 
+const userPictureUploadLimiter = rateLimit({
+  windowMs: 15 * 60 * 1000, // 15 minutes
+  max: 50, // limit each IP to 50 picture upload requests per windowMs
+  standardHeaders: true,
+  legacyHeaders: false,
+});
+
 router.post("/", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), users.markUnverified);
 router.post("/update-in-discord", authenticate, authorizeRoles([SUPERUSER]), users.setInDiscordScript);
 router.post("/verify", authenticate, users.verifyUser);
@@ -69,6 +76,7 @@
 router.post(
   "/picture",
   authenticate,
+  userPictureUploadLimiter,
   upload.single("profile"),
   skipWhenApplicationType(checkIsVerifiedDiscord),
   users.handleUserPictureUpload
EOF
@@ -16,7 +16,15 @@
const { userAuthorization } = require("../middlewares/userAuthorization");
const conditionalMiddleware = require("../middlewares/conditionalMiddleware");
const skipWhenApplicationType = require("../middlewares/pictureRouteMiddleware");
const rateLimit = require("express-rate-limit");

const userPictureUploadLimiter = rateLimit({
windowMs: 15 * 60 * 1000, // 15 minutes
max: 50, // limit each IP to 50 picture upload requests per windowMs
standardHeaders: true,
legacyHeaders: false,
});

router.post("/", authorizeAndAuthenticate([ROLES.SUPERUSER], [Services.CRON_JOB_HANDLER]), users.markUnverified);
router.post("/update-in-discord", authenticate, authorizeRoles([SUPERUSER]), users.setInDiscordScript);
router.post("/verify", authenticate, users.verifyUser);
@@ -69,6 +76,7 @@
router.post(
"/picture",
authenticate,
userPictureUploadLimiter,
upload.single("profile"),
skipWhenApplicationType(checkIsVerifiedDiscord),
users.handleUserPictureUpload
package.json
Outside changed files

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -42,7 +42,8 @@
     "passport-github2": "0.1.12",
     "passport-google-oauth20": "^2.0.0",
     "rate-limiter-flexible": "5.0.3",
-    "winston": "3.13.0"
+    "winston": "3.13.0",
+    "express-rate-limit": "^8.2.1"
   },
   "devDependencies": {
     "@types/chai": "4.3.16",
EOF
@@ -42,7 +42,8 @@
"passport-github2": "0.1.12",
"passport-google-oauth20": "^2.0.0",
"rate-limiter-flexible": "5.0.3",
"winston": "3.13.0"
"winston": "3.13.0",
"express-rate-limit": "^8.2.1"
},
"devDependencies": {
"@types/chai": "4.3.16",
This fix introduces these dependencies
Package Version Security advisories
express-rate-limit (npm) 8.2.1 None
Copilot is powered by AI and may make mistakes. Always verify output.
upload.single("profile"),
skipWhenApplicationType(checkIsVerifiedDiscord),
users.handleUserPictureUpload
);
router.patch(
"/picture/verify/:id",
authenticate,
Expand Down
42 changes: 42 additions & 0 deletions test/integration/application.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ const applicationModel = require("../../models/applications");
const applicationsData = require("../fixtures/applications/applications")();
const cookieName = config.get("userToken.cookieName");
const { APPLICATION_ERROR_MESSAGES, API_RESPONSE_MESSAGES, APPLICATION_SCORE } = require("../../constants/application");
const imageService = require("../../services/imageService");
const { Buffer } = require("node:buffer");

const appOwner = userData[3];
const superUser = userData[4];
Expand Down Expand Up @@ -996,4 +998,44 @@ describe("Application", function () {
});
});
});

describe("POST /users/picture (application type)", function () {
it("should return 201 when uploading with type=application and valid file", function (done) {
const mockImageResponse = { publicId: "profile/test-id/image", url: "https://res.cloudinary.com/example/image.png" };
const uploadStub = sinon.stub(imageService, "uploadProfilePicture").resolves(mockImageResponse);
chai
.request(app)
.post("/users/picture")
.type("form")
.set("cookie", `${cookieName}=${jwt}`)
.attach("profile", Buffer.from("fake-image-data", "utf-8"), "image.png")
.field("type", "application")
.end((err, res) => {
uploadStub.restore();
if (err) return done(err);
expect(res).to.have.status(201);
expect(res.body.message).to.equal("Application picture uploaded successfully!");
expect(res.body.image).to.deep.equal(mockImageResponse);
return done();
});
});

it("should return 500 when upload fails", function (done) {
const uploadStub = sinon.stub(imageService, "uploadProfilePicture").rejects(new Error("Upload failed"));
chai
.request(app)
.post("/users/picture")
.type("form")
.set("cookie", `${cookieName}=${jwt}`)
.attach("profile", Buffer.from("fake-image-data", "utf-8"), "image.png")
.field("type", "application")
.end((err, res) => {
uploadStub.restore();
if (err) return done(err);
expect(res).to.have.status(500);
expect(res.body.error).to.equal("Internal Server Error");
return done();
});
});
});
});
Loading