feat: add applicant image upload functionality #2564
+313
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Added integration tests for the new image upload endpoint `/applications/picture`, covering authentication and file validation scenarios. - Enhanced unit tests for the applications controller to include image upload functionality. - Implemented middleware validation for image files, ensuring only valid formats (PNG, JPEG) are accepted and handling errors appropriately.
Summary by CodeRabbit
WalkthroughA new application picture upload feature is added, including error message constants, Cloudinary configuration, a POST endpoint with file validation middleware, image service integration for Cloudinary uploads, and comprehensive test coverage for both success and failure scenarios. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant Authenticate as Auth Middleware
participant ValidatePic as Validator Middleware
participant Controller
participant ImageService
participant Cloudinary
Client->>Router: POST /applications/picture (file, auth token)
Router->>Authenticate: Check authentication
Authenticate->>Router: User verified
Router->>ValidatePic: Validate file
ValidatePic->>ValidatePic: Check file presence & MIME type
ValidatePic->>Router: Validation passed
Router->>Controller: Forward request
Controller->>ImageService: uploadApplicationImage({file, userId})
ImageService->>ImageService: Encode to Base64 Data URI
ImageService->>Cloudinary: Upload with public_id
Cloudinary-->>ImageService: Return publicId & URL
ImageService-->>Controller: Return {publicId, url}
Controller-->>Client: 201 Created {message, image}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Important Action Needed: IP Allowlist UpdateIf your organization protects your Git platform with IP whitelisting, please add the new CodeRabbit IP address to your allowlist:
Reviews will stop working after February 8, 2026 if the new IP is not added to your allowlist. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ); | ||
| router.post( | ||
| "/picture", | ||
| authenticate, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
In general, to fix missing rate limiting on sensitive or expensive endpoints, we should add a rate‑limiting middleware that restricts how many requests a client can make within a certain time window. In an Express app, this is commonly done with the express-rate-limit package, which can be applied globally (app.use(limiter)) or per‑route (router.post('/path', limiter, handler)).
For this specific file, the best minimal change is to introduce a rate limiter instance (using express-rate-limit) and apply it to the /picture POST route, which is performing authentication and file upload and is the one directly reported by CodeQL. To avoid altering existing behavior beyond rate limiting, we will insert the limiter middleware into the chain for that route, after authenticate and before upload.single("profile"). This ordering ensures that: (1) only authenticated users are counted for this limiter (if identification is based on user), and (2) rate limiting happens before the actual upload processing, so we avoid wasting resources on rejected requests.
Concretely:
- At the top of
routes/applications.ts, addconst rateLimit = require("express-rate-limit");. - Define a limiter, e.g.
const pictureUploadLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100 });(or a stricter limit if desired) near where the router is created. - Update the
/pictureroute declaration to includepictureUploadLimiterin the middleware list. No other routes need to be changed to satisfy the specific alert, though they could be similarly protected if desired.
Suggested changeset
2
routes/applications.ts
| @@ -6,9 +6,15 @@ | ||
| const { authorizeOwnOrSuperUser } = require("../middlewares/authorizeOwnOrSuperUser"); | ||
| const applicationValidator = require("../middlewares/validators/application"); | ||
| const { upload } = require("../utils/multer"); | ||
| const rateLimit = require("express-rate-limit"); | ||
|
|
||
| const router = express.Router(); | ||
|
|
||
| const pictureUploadLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 picture upload requests per windowMs | ||
| }); | ||
|
|
||
| router.get( | ||
| "/", | ||
| authenticate, | ||
| @@ -19,6 +22,7 @@ | ||
| router.post( | ||
| "/picture", | ||
| authenticate, | ||
| pictureUploadLimiter, | ||
| upload.single("profile"), | ||
| applicationValidator.validateApplicationPicture, | ||
| applications.postUserPicture |
package.json
Outside changed files
| @@ -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.
Member
|
@coderabbitai review bro |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@routes/applications.ts`:
- Around line 19-25: The POST /picture route lacks rate limiting—add the
existing commonRateLimiter middleware into the router.post middleware chain
before the upload handler so requests hit rate limits; specifically update the
router.post call that currently lists authenticate, upload.single("profile"),
applicationValidator.validateApplicationPicture, and
applications.postUserPicture to include commonRateLimiter (imported from
middlewares/rateLimiting.js) in the middleware sequence (e.g., after
authenticate and before upload.single) so the route uses commonRateLimiter and
still calls applicationValidator.validateApplicationPicture and
applications.postUserPicture.
In `@test/integration/application.test.ts`:
- Around line 361-397: Add integration tests for the validation/error paths
around the POST /applications/picture route: add a test that sends no attachment
and asserts the response is the expected validation error from
validateApplicationPicture (e.g., 400 and appropriate error message), a test
that attaches an invalid file type (e.g., .txt or .gif) and asserts the
middleware returns the correct validation response, and a test where
sinon.stub(imageService, "uploadApplicationImage") is made to reject to assert
the route returns the service error (status/code and message) and is handled
correctly; place these alongside the existing tests in application.test.ts and
reuse the same auth cookie setup (cookieName/secondUserJwt) and
mockUploadResponse reference but adjust stubbing/attachment to trigger each
path.
- Around line 381-396: The test stubs imageService.uploadApplicationImage but
never restores it, risking test pollution; update the test suite to restore the
stub after the case by adding an afterEach that calls sinon.restore() (or
explicitly calls uploadApplicationImage.restore() if you capture the stub
variable) so the stub created in the "should return 201 and image data when
authenticated with valid image" test is removed before other tests run; locate
the stub usage in the test that posts to "/applications/picture" and add the
afterEach cleanup alongside other per-test cleanup blocks.
Comment on lines
+361
to
+397
| describe("POST /applications/picture", function () { | ||
| const mockUploadResponse = { | ||
| publicId: "applications/test-user/profile", | ||
| url: "https://res.cloudinary.com/example/applications/test-user/profile.png", | ||
| }; | ||
|
|
||
| it("should return 401 when not authenticated", function (done) { | ||
| chai | ||
| .request(app) | ||
| .post("/applications/picture") | ||
| .type("form") | ||
| .attach("profile", Buffer.from("fake-image-data"), "test.png") | ||
| .end((err, res) => { | ||
| if (err) return done(err); | ||
| expect(res).to.have.status(401); | ||
| expect(res.body.error).to.equal("Unauthorized"); | ||
| return done(); | ||
| }); | ||
| }); | ||
|
|
||
| it("should return 201 and image data when authenticated with valid image", function (done) { | ||
| sinon.stub(imageService, "uploadApplicationImage").resolves(mockUploadResponse); | ||
| chai | ||
| .request(app) | ||
| .post("/applications/picture") | ||
| .set("cookie", `${cookieName}=${secondUserJwt}`) | ||
| .type("form") | ||
| .attach("profile", Buffer.from("fake-image-data"), "test.png") | ||
| .end((err, res) => { | ||
| if (err) return done(err); | ||
| expect(res).to.have.status(201); | ||
| expect(res.body.message).to.equal("Image uploaded successfully"); | ||
| expect(res.body.image).to.deep.equal(mockUploadResponse); | ||
| return done(); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding tests for validation error paths.
The test suite covers authentication and the happy path, but lacks integration tests for validation scenarios handled by validateApplicationPicture middleware:
- Missing file (no attachment)
- Invalid file type (e.g.,
.txtor.gif) - Service failure (when
uploadApplicationImagerejects)
These would ensure the middleware and error handling work correctly end-to-end.
🤖 Prompt for AI Agents
In `@test/integration/application.test.ts` around lines 361 - 397, Add integration
tests for the validation/error paths around the POST /applications/picture
route: add a test that sends no attachment and asserts the response is the
expected validation error from validateApplicationPicture (e.g., 400 and
appropriate error message), a test that attaches an invalid file type (e.g.,
.txt or .gif) and asserts the middleware returns the correct validation
response, and a test where sinon.stub(imageService, "uploadApplicationImage") is
made to reject to assert the route returns the service error (status/code and
message) and is handled correctly; place these alongside the existing tests in
application.test.ts and reuse the same auth cookie setup
(cookieName/secondUserJwt) and mockUploadResponse reference but adjust
stubbing/attachment to trigger each path.
Member
There was a problem hiding this comment.
can you show me the code for working test for this point
There was a problem hiding this comment.
@MayankBansal12, here are the three working test cases for the validation error paths:
describe("POST /applications/picture", function () {
const mockUploadResponse = {
publicId: "applications/test-user/profile",
url: "https://res.cloudinary.com/example/applications/test-user/profile.png",
};
afterEach(function () {
sinon.restore();
});
it("should return 401 when not authenticated", function (done) {
chai
.request(app)
.post("/applications/picture")
.type("form")
.attach("profile", Buffer.from("fake-image-data"), "test.png")
.end((err, res) => {
if (err) return done(err);
expect(res).to.have.status(401);
expect(res.body.error).to.equal("Unauthorized");
return done();
});
});
it("should return 201 and image data when authenticated with valid image", function (done) {
sinon.stub(imageService, "uploadApplicationImage").resolves(mockUploadResponse);
chai
.request(app)
.post("/applications/picture")
.set("cookie", `${cookieName}=${secondUserJwt}`)
.type("form")
.attach("profile", Buffer.from("fake-image-data"), "test.png")
.end((err, res) => {
if (err) return done(err);
expect(res).to.have.status(201);
expect(res.body.message).to.equal("Image uploaded successfully");
expect(res.body.image).to.deep.equal(mockUploadResponse);
return done();
});
});
it("should return 400 when no file is attached", function (done) {
chai
.request(app)
.post("/applications/picture")
.set("cookie", `${cookieName}=${secondUserJwt}`)
.type("form")
.end((err, res) => {
if (err) return done(err);
expect(res).to.have.status(400);
expect(res.body.error).to.equal("Bad Request");
expect(res.body.message).to.equal(APPLICATION_ERROR_MESSAGES.PICTURE_FILE_MISSING);
return done();
});
});
it("should return 400 when file type is not supported", function (done) {
chai
.request(app)
.post("/applications/picture")
.set("cookie", `${cookieName}=${secondUserJwt}`)
.type("form")
.attach("profile", Buffer.from("fake-text-data"), "test.txt")
.end((err, res) => {
if (err) return done(err);
expect(res).to.have.status(400);
expect(res.body.error).to.equal("Bad Request");
expect(res.body.message).to.include("Only image/png and image/jpeg formats are supported");
return done();
});
});
it("should return 500 when image upload service fails", function (done) {
sinon.stub(imageService, "uploadApplicationImage").rejects(new Error("Cloudinary service error"));
chai
.request(app)
.post("/applications/picture")
.set("cookie", `${cookieName}=${secondUserJwt}`)
.type("form")
.attach("profile", Buffer.from("fake-image-data"), "test.png")
.end((err, res) => {
if (err) return done(err);
expect(res).to.have.status(500);
expect(res.body.error).to.equal("Internal Server Error");
return done();
});
});
});Key additions:
- Missing file test - Simply omits the
.attach()call to trigger thePICTURE_FILE_MISSINGvalidation error - Invalid file type test - Attaches a
.txtfile to trigger the MIME type validation error - Service failure test - Stubs
uploadApplicationImageto reject with an error, expecting a 500 response - Added
afterEach- Restores all sinon stubs after each test to prevent side effects between tests
These tests cover the middleware validation and error handling end-to-end as requested.
Comment on lines
+381
to
+396
| it("should return 201 and image data when authenticated with valid image", function (done) { | ||
| sinon.stub(imageService, "uploadApplicationImage").resolves(mockUploadResponse); | ||
| chai | ||
| .request(app) | ||
| .post("/applications/picture") | ||
| .set("cookie", `${cookieName}=${secondUserJwt}`) | ||
| .type("form") | ||
| .attach("profile", Buffer.from("fake-image-data"), "test.png") | ||
| .end((err, res) => { | ||
| if (err) return done(err); | ||
| expect(res).to.have.status(201); | ||
| expect(res.body.message).to.equal("Image uploaded successfully"); | ||
| expect(res.body.image).to.deep.equal(mockUploadResponse); | ||
| return done(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Stub not restored after test, risking test pollution.
The sinon.stub(imageService, "uploadApplicationImage") created on line 382 is never restored. While there's a global sinon.restore() in the after hook, subsequent tests within this file could be affected if they rely on the real imageService. Other test blocks like "PATCH /applications/:applicationId/nudge" properly use afterEach for cleanup.
🧹 Proposed fix: Add afterEach for cleanup
describe("POST /applications/picture", function () {
const mockUploadResponse = {
publicId: "applications/test-user/profile",
url: "https://res.cloudinary.com/example/applications/test-user/profile.png",
};
+
+ afterEach(function () {
+ sinon.restore();
+ });
it("should return 401 when not authenticated", function (done) {🤖 Prompt for AI Agents
In `@test/integration/application.test.ts` around lines 381 - 396, The test stubs
imageService.uploadApplicationImage but never restores it, risking test
pollution; update the test suite to restore the stub after the case by adding an
afterEach that calls sinon.restore() (or explicitly calls
uploadApplicationImage.restore() if you capture the stub variable) so the stub
created in the "should return 201 and image data when authenticated with valid
image" test is removed before other tests run; locate the stub usage in the test
that posts to "/applications/picture" and add the afterEach cleanup alongside
other per-test cleanup blocks.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Date: 3 Feb, 2026
Developer Name: @AnujChhikara
Issue Ticket Number
Description
Documentation Updated?
Under Feature Flag
Database Changes
Breaking Changes
Development Tested?
Screenshots
Screenshot 1
Screen.Recording.2026-02-03.at.1.50.48.AM.mov
Test Coverage
Screenshot 1
Additional Notes