-
Notifications
You must be signed in to change notification settings - Fork 109
Resolves #1537 - #1539: First pass at the review collection, with some endpoints for testing #1549
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
base: dev
Are you sure you want to change the base?
Conversation
const controller = require('./review-object.controller') | ||
const mw = require('../../middleware/middleware') | ||
|
||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix this issue, rate limiting should be explicitly added to the router handling these review object endpoints. The recommended approach is to use a well-known and widely used middleware such as express-rate-limit
. We should import this middleware and configure a reasonable rate limit (for example, 100 requests per 15 minutes per IP) and attach this limiter to the specific routes in this router file, so that each relevant route is protected against excessive requests. This requires:
- Adding
express-rate-limit
as a dependency (if not already present). - Importing the package in this file.
- Defining a rate limiter instance with a configuration suited to the needs of these (likely sensitive) endpoints.
- Attaching the rate limiter as a middleware to the relevant routes, before
mw.validateUser
and the rest.
Concrete changes:
- In
src/controller/review-object.controller/index.js
, addexpress-rate-limit
import at the top. - Define a limiter instance (e.g.,
const rateLimit = require('express-rate-limit')
and define configuration). - Add the limiter to each of the four routes as the first middleware in the stack.
-
Copy modified line R5 -
Copy modified lines R7-R47
@@ -2,9 +2,47 @@ | ||
const controller = require('./review-object.controller') | ||
const mw = require('../../middleware/middleware') | ||
|
||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) | ||
router.post('/review/org/', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) | ||
const rateLimit = require('express-rate-limit'); | ||
|
||
// Apply rate limiting: max 100 requests per 15 minutes per IP | ||
const reviewLimiter = rateLimit({ | ||
windowMs: 15 * 60 * 1000, // 15 minutes | ||
max: 100, // limit each IP to 100 requests per windowMs | ||
standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
}); | ||
|
||
router.get( | ||
'/review/org/:identifier', | ||
reviewLimiter, | ||
mw.useRegistry(), | ||
mw.validateUser, | ||
mw.onlySecretariat, | ||
controller.getReviewObjectByOrgIdentifier | ||
); | ||
router.get( | ||
'/review/orgs', | ||
reviewLimiter, | ||
mw.useRegistry(), | ||
mw.validateUser, | ||
mw.onlySecretariat, | ||
controller.getAllReviewObjects | ||
); | ||
router.put( | ||
'/review/org/:uuid', | ||
reviewLimiter, | ||
mw.useRegistry(), | ||
mw.validateUser, | ||
mw.onlySecretariat, | ||
controller.updateReviewObjectByReviewUUID | ||
); | ||
router.post( | ||
'/review/org/', | ||
reviewLimiter, | ||
mw.useRegistry(), | ||
mw.validateUser, | ||
mw.onlySecretariat, | ||
controller.createReviewObject | ||
); | ||
|
||
module.exports = router |
const mw = require('../../middleware/middleware') | ||
|
||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the missing rate limiting issue, a rate-limiting middleware should be added to the relevant routes. The standard and recommended package for this in Express is express-rate-limit
. The fix should import (require) express-rate-limit
, configure a suitable limiter (e.g., 100 requests per 15 minutes as in the example), and apply this limiter specifically to the sensitive routes, especially the /review/orgs
GET endpoint, to prevent possible abuse. This can be done by inserting the limiter as a middleware in the relevant router .get()
call. The fix must add the express-rate-limit
import and create the limiter instance if not already present in this file. The code to be edited is within src/controller/review-object.controller/index.js
, affecting line(s) where the route is defined and at the top for the import and limiter definition.
-
Copy modified lines R5-R12 -
Copy modified line R14
@@ -2,8 +2,16 @@ | ||
const controller = require('./review-object.controller') | ||
const mw = require('../../middleware/middleware') | ||
|
||
// Rate limiting middleware for review orgs endpoint | ||
const rateLimit = require('express-rate-limit'); | ||
const reviewOrgsLimiter = rateLimit({ | ||
windowMs: 15 * 60 * 1000, // 15 minutes | ||
max: 100, // limit each IP to 100 requests per windowMs | ||
standardHeaders: true, | ||
legacyHeaders: false, | ||
}); | ||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.get('/review/orgs', reviewOrgsLimiter, mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) | ||
router.post('/review/org/', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) | ||
|
|
||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
The best way to fix this issue is to add rate-limiting middleware to the endpoint(s) that update data, specifically line 7 (router.put('/review/org/:uuid', ...)
). The widely used library for Express—express-rate-limit
—should be imported and configured to set limits appropriate for update operations (e.g., a low burst rate to prevent DoS).
- Where to change: In
src/controller/review-object.controller/index.js
, import the rate limiter, configure a custom instance (e.g., allowing 10 attempts per 15 minutes), and append it to the relevant route(s). - Methods/imports/definitions needed: Import the package at the top, instantiate a limiter according to needs, and use it as middleware in the route.
-
Copy modified line R4 -
Copy modified lines R6-R12 -
Copy modified line R15
@@ -1,10 +1,18 @@ | ||
const router = require('express').Router() | ||
const controller = require('./review-object.controller') | ||
const mw = require('../../middleware/middleware') | ||
const rateLimit = require('express-rate-limit'); | ||
|
||
// Limit PUT requests to 10 per 15 minutes per IP | ||
const updateLimiter = rateLimit({ | ||
windowMs: 15 * 60 * 1000, // 15 minutes | ||
max: 10, // limit each IP to 10 requests per windowMs | ||
standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
}); | ||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, updateLimiter, controller.updateReviewObjectByReviewUUID) | ||
router.post('/review/org/', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) | ||
|
||
module.exports = router |
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) | ||
router.post('/review/org/', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 days ago
To fix the missing rate limiting on this sensitive POST endpoint, the best approach is to use a well-known rate limiting middleware designed for Express, such as express-rate-limit
. This should be imported and configured with sensible defaults (e.g., allowing a small number of POSTs per minute from a single IP), and then applied specifically to the POST /review/org/
route (or more routes, if desired). The relevant import for express-rate-limit
should be added at the top of the file. A new limiter instance needs to be created and inserted into the list of middleware for the POST route on line 8, without altering any existing functionality or other route handlers.
-
Copy modified line R4 -
Copy modified lines R6-R12 -
Copy modified line R16
@@ -1,10 +1,18 @@ | ||
const router = require('express').Router() | ||
const controller = require('./review-object.controller') | ||
const mw = require('../../middleware/middleware') | ||
const rateLimit = require('express-rate-limit') | ||
|
||
// Limit POSTs to /review/org/ to 5 per minute per IP | ||
const createReviewLimiter = rateLimit({ | ||
windowMs: 1 * 60 * 1000, // 1 minute | ||
max: 5, // limit each IP to 5 requests per windowMs | ||
message: 'Too many review objects created from this IP, please try again after a minute' | ||
}) | ||
|
||
router.get('/review/org/:identifier', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getReviewObjectByOrgIdentifier) | ||
router.get('/review/orgs', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.getAllReviewObjects) | ||
router.put('/review/org/:uuid', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.updateReviewObjectByReviewUUID) | ||
router.post('/review/org/', mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) | ||
router.post('/review/org/', createReviewLimiter, mw.useRegistry(), mw.validateUser, mw.onlySecretariat, controller.createReviewObject) | ||
|
||
module.exports = router |
Closes Issue #1537, #1538, #1539