Conversation
backend/src/routes/v2/routes.ts
Outdated
| { url: match('/permissions/mine'), method: ['GET'] }, | ||
| ], | ||
| }, | ||
| mirroredModel: { allowAll: true }, |
There was a problem hiding this comment.
There's already quite a few checks to block endpoints on mirrored models. Is this meant to be a replacement for that functionality?
| ) { | ||
| return next() | ||
| } | ||
| throw BadReq('Deny') |
There was a problem hiding this comment.
Could we give the user the EntryKind as context?
github-actions
bot
added
javascript
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| router.use('/model/:modelId', entryKindCheck(entryAllowList)) | ||
|
|
||
| // Needs to be applied after authentication middleware as it requires the user details | ||
| router.use('/api/v2/models', escalateUser) |
Check failure
Code scanning / CodeQL
Missing rate limiting High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 20 days ago
In general, the problem is fixed by adding a rate-limiting middleware in front of the sensitive route(s) (here, the /api/v2/models subtree) so that each client can only trigger the protected logic (including escalateUser and the model handlers) at a controlled rate. A standard solution in Express is to use express-rate-limit to define a limiter and apply it with router.use() before the expensive or security-sensitive middlewares.
For this specific file, the minimal change that doesn’t alter existing behavior is:
- Import
express-rate-limit. - Create a limiter configured with reasonable defaults (e.g., 100 requests per 15 minutes per IP; these values can be tuned later but provide a baseline).
- Apply this limiter to the same route prefix as
escalateUserand place it immediately beforeescalateUserso that rate limiting happens first, and only then does the app perform the authorization escalation.
Concretely:
- In
backend/src/routes/v2/routes.ts, addimport rateLimit from 'express-rate-limit'alongside the other imports. - Define
const modelsRateLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 100 })somewhere near the router initialization. - Change the middleware chain around line 126 so that
router.use('/api/v2/models', modelsRateLimiter, escalateUser)is used instead of onlyescalateUser.
This keeps the existing route structure and handlers intact while adding a protective layer against request floods.
| @@ -5,6 +5,7 @@ | ||
| import { AllowList, entryKindCheck } from '../middleware/entryType.js' | ||
| import { expressErrorHandler } from '../middleware/expressErrorHandler.js' | ||
| import { escalateUser } from '../middleware/userEscalation.js' | ||
| import rateLimit from 'express-rate-limit' | ||
| import { getArtefactScanningInfo } from './artefactScanning/getArtefactScanningInfo.js' | ||
| import { putFileScan } from './artefactScanning/putFileScan.js' | ||
| import { getCurrentUser } from './entities/getCurrentUser.js' | ||
| @@ -92,6 +93,11 @@ | ||
|
|
||
| const router = Router() | ||
|
|
||
| const modelsRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }) | ||
|
|
||
| const entryAllowList: { | ||
| model: AllowList | ||
| 'data-card': AllowList | ||
| @@ -123,7 +129,7 @@ | ||
| router.use('/model/:modelId', entryKindCheck(entryAllowList)) | ||
|
|
||
| // Needs to be applied after authentication middleware as it requires the user details | ||
| router.use('/api/v2/models', escalateUser) | ||
| router.use('/api/v2/models', modelsRateLimiter, escalateUser) | ||
|
|
||
| router.get('/system/status', ...getSystemStatus) | ||
| router.get('/system/peers', ...getPeerStatus) |
| @@ -80,7 +80,8 @@ | ||
| "uuid": "^13.0.0", | ||
| "yargs": "^18.0.0", | ||
| "zod": "^3.25.67", | ||
| "zod-error": "^1.5.0" | ||
| "zod-error": "^1.5.0", | ||
| "express-rate-limit": "^8.3.1" | ||
| }, | ||
| "devDependencies": { | ||
| "@anatine/zod-mock": "^3.14.0", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.3.1 | None |
3 participants
No description provided.