-
Notifications
You must be signed in to change notification settings - Fork 235
Release v7.4.0 #10473
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: release/7.4.x
Are you sure you want to change the base?
Release v7.4.0 #10473
Conversation
…iv3-routes-biome-2 support: Configure biome for app apiv3 routes (app-settings, page)
| accessTokenParser([SCOPE.READ.FEATURES.PAGE], { acceptLegacy: true }), | ||
| certifySharedPage, loginRequired, validator.getPage, apiV3FormValidator, async(req, res) => { | ||
| certifySharedPage, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 14 hours ago
To fix this issue, we should add a rate-limiter middleware to the /page GET route. The standard approach in Express is to use the express-rate-limit library. We'll import it at the top of the file (using require per code style), configure a reasonable limit (e.g., 100 requests per 15 minutes), and add the limiter to the middleware chain for the /page route only. This keeps rate limiting specific to the expensive endpoint and avoids changing the behavior for unrelated endpoints.
Steps:
- Add
express-rate-limitimport. - Instantiate a limiter instance with configuration (e.g., 15 min window, 100 requests).
- Insert the limiter as middleware into the
/pageGET handler chain (between the common middlewares as appropriate). - This can be done entirely in the shown file:
apps/app/src/server/routes/apiv3/page/index.ts.
-
Copy modified line R60 -
Copy modified lines R88-R95 -
Copy modified line R194
| @@ -57,6 +57,7 @@ | ||
| const logger = loggerFactory('growi:routes:apiv3:page'); // eslint-disable-line no-unused-vars | ||
|
|
||
| const express = require('express'); | ||
| const RateLimit = require('express-rate-limit'); | ||
| const { body, query, param } = require('express-validator'); | ||
|
|
||
| const router = express.Router(); | ||
| @@ -84,6 +85,14 @@ | ||
| crowi, | ||
| true, | ||
| ); | ||
|
|
||
| // rate limiter: max 100 requests per 15 min per IP | ||
| const pageGetRateLimiter = RateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 min | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| standardHeaders: true, | ||
| legacyHeaders: false, | ||
| }); | ||
| const loginRequiredStrictly = require('../../../middlewares/login-required')( | ||
| crowi, | ||
| ); | ||
| @@ -182,6 +191,7 @@ | ||
| */ | ||
| router.get( | ||
| '/', | ||
| pageGetRateLimiter, | ||
| accessTokenParser([SCOPE.READ.FEATURES.PAGE], { acceptLegacy: true }), | ||
| certifySharedPage, | ||
| loginRequired, |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| const shareLink = await ShareLink.findOne({ _id: { $eq: shareLinkId } }); | ||
| const shareLink = await ShareLink.findOne({ | ||
| _id: { $eq: shareLinkId }, | ||
| }); | ||
| if (shareLink == null) { | ||
| throw new Error('ShareLink is not found'); | ||
| return res.apiv3Err('ShareLink is not found', 404); | ||
| } | ||
| page = await Page.findOne({ _id: getIdForRef(shareLink.relatedPage) }); | ||
| } | ||
| else if (pageId != null) { // prioritized | ||
| page = await Page.findByIdAndViewer(pageId, user); | ||
| pageWithMeta = await pageService.findPageAndMetaDataByViewer( | ||
| getIdStringForRef(shareLink.relatedPage), | ||
| path, | ||
| user, | ||
| true, | ||
| ); | ||
| } else if (!findAll) { | ||
| pageWithMeta = await pageService.findPageAndMetaDataByViewer( | ||
| pageId, | ||
| path, | ||
| user, | ||
| ); | ||
| } else { | ||
| pages = await Page.findByPathAndViewer( | ||
| path, | ||
| user, | ||
| null, | ||
| false, | ||
| includeEmpty, | ||
| ); | ||
| } | ||
| else if (!findAll) { | ||
| page = await Page.findByPathAndViewer(path, user, null, true, false); | ||
| } | ||
| else { | ||
| pages = await Page.findByPathAndViewer(path, user, null, false, includeEmpty); | ||
| } | ||
| } | ||
| catch (err) { | ||
| } catch (err) { | ||
| logger.error('get-page-failed', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
|
|
||
| if (page == null && (pages == null || pages.length === 0)) { | ||
| return res.apiv3Err('Page is not found', 404); | ||
| let { data: page } = pageWithMeta; | ||
| const { meta } = pageWithMeta; | ||
|
|
||
| // not found or forbidden | ||
| if ( | ||
| isIPageNotFoundInfo(meta) || | ||
| (Array.isArray(pages) && pages.length === 0) | ||
| ) { | ||
| if (isIPageNotFoundInfo(meta) && meta.isForbidden) { | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Page is forbidden', | ||
| 'page-is-forbidden', | ||
| undefined, | ||
| meta, | ||
| ), | ||
| 403, | ||
| ); | ||
| } | ||
| return res.apiv3Err( | ||
| new ErrorV3('Page is not found', 'page-not-found', undefined, meta), | ||
| 404, | ||
| ); | ||
| } | ||
|
|
||
| if (page != null) { | ||
| try { | ||
| page.initLatestRevisionField(revisionId); | ||
|
|
||
| // populate | ||
| page = await page.populateDataToShowRevision(); | ||
| } | ||
| catch (err) { | ||
| } catch (err) { | ||
| logger.error('populate-page-failed', err); | ||
| return res.apiv3Err(err, 500); | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Failed to populate page', | ||
| 'populate-page-failed', | ||
| undefined, | ||
| { err, meta }, | ||
| ), | ||
| 500, | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| return res.apiv3({ page, pages }); | ||
| }); | ||
| return res.apiv3({ page, pages, meta }); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 14 hours ago
To fix the problem, a rate limiting middleware should be added to the GET /page route. The best approach is to use the popular express-rate-limit middleware. This requires importing express-rate-limit, creating a limiter instance (with reasonable defaults, e.g. 100 requests/15 minutes), and adding it to the route definition for / (i.e., the handler at line 184). Because only the code you've been shown in this file can be changed, you'll need to import express-rate-limit near the top (after other imports), create a limiter variable (per the example), and update the handler array for / to include the limiter in the middleware list. That way, the rate limiting applies only to this database-accessing endpoint, without changing application behavior except for limiting request rates. No changes are needed for other routes or downstream logic.
-
Copy modified line R48 -
Copy modified lines R60-R64 -
Copy modified line R191
| @@ -45,6 +45,7 @@ | ||
| import loggerFactory from '~/utils/logger'; | ||
|
|
||
| import type { ApiV3Response } from '../interfaces/apiv3-response'; | ||
| import RateLimit from 'express-rate-limit'; | ||
| import { checkPageExistenceHandlersFactory } from './check-page-existence'; | ||
| import { createPageHandlersFactory } from './create-page'; | ||
| import { getPagePathsWithDescendantCountFactory } from './get-page-paths-with-descendant-count'; | ||
| @@ -56,6 +57,11 @@ | ||
|
|
||
| const logger = loggerFactory('growi:routes:apiv3:page'); // eslint-disable-line no-unused-vars | ||
|
|
||
| // Rate limiter for GET /page route | ||
| const getPageLimiter = RateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // max 100 requests per windowMs per IP | ||
| }); | ||
| const express = require('express'); | ||
| const { body, query, param } = require('express-validator'); | ||
|
|
||
| @@ -182,6 +188,7 @@ | ||
| */ | ||
| router.get( | ||
| '/', | ||
| getPageLimiter, | ||
| accessTokenParser([SCOPE.READ.FEATURES.PAGE], { acceptLegacy: true }), | ||
| certifySharedPage, | ||
| loginRequired, |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| router.get( | ||
| '/info', | ||
| accessTokenParser([SCOPE.READ.FEATURES.PAGE]), | ||
| certifySharedPage, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 14 hours ago
To fix the problem, a rate-limiting middleware should be added to the /info route to ensure no client can excessively access this endpoint and overload the server/database. The best way in Express-based applications is to use a well-known package such as express-rate-limit.
Steps:
- Add an import for
express-rate-limitat the top of the file, if it is not already present. - Define a rate limiter middleware instance with suitable configuration (e.g., max 100 requests per 15 minutes per IP, as used in the recommended fix).
- Apply this rate limiter as a middleware to the
/inforoute. - Ensure only the
/inforoute is affected (not all routes unless intended).
This requires only minimal changes: the import, the definition of the limiter, and adding it to the route's middleware stack.
-
Copy modified line R27 -
Copy modified lines R60-R64 -
Copy modified line R582
| @@ -24,6 +24,7 @@ | ||
| import type { Readable } from 'stream'; | ||
| import { pipeline } from 'stream/promises'; | ||
|
|
||
| import rateLimit from 'express-rate-limit'; | ||
| import { SupportedAction, SupportedTargetModel } from '~/interfaces/activity'; | ||
| import type { IPageGrantData } from '~/interfaces/page'; | ||
| import type { IRecordApplicableGrant } from '~/interfaces/page-grant'; | ||
| @@ -56,6 +57,11 @@ | ||
|
|
||
| const logger = loggerFactory('growi:routes:apiv3:page'); // eslint-disable-line no-unused-vars | ||
|
|
||
| // Rate limiter for expensive GET routes | ||
| const infoRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 100, // limit each IP to 100 requests per windowMs | ||
| }); | ||
| const express = require('express'); | ||
| const { body, query, param } = require('express-validator'); | ||
|
|
||
| @@ -573,6 +579,7 @@ | ||
| */ | ||
| router.get( | ||
| '/info', | ||
| infoRateLimiter, | ||
| accessTokenParser([SCOPE.READ.FEATURES.PAGE]), | ||
| certifySharedPage, | ||
| loginRequired, |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| throw new Error('Revision is not found'); | ||
| } | ||
|
|
||
| pagePath = page.path; | ||
|
|
||
| // Error if pageId and revison's pageIds do not match | ||
| if (page._id.toString() !== revision.pageId.toString()) { | ||
| return res.apiv3Err( | ||
| new ErrorV3("Haven't the right to see the page."), | ||
| 403, | ||
| ); | ||
| } | ||
| } catch (err) { | ||
| logger.error('Failed to get revision data', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
|
|
||
| pagePath = page.path; | ||
| // replace forbidden characters to '_' | ||
| // refer to https://kb.acronis.com/node/56475?ckattempt=1 | ||
| let fileName = sanitize(path.basename(pagePath), { replacement: '_' }); | ||
|
|
||
| // Error if pageId and revison's pageIds do not match | ||
| if (page._id.toString() !== revision.pageId.toString()) { | ||
| return res.apiv3Err(new ErrorV3("Haven't the right to see the page."), 403); | ||
| // replace root page name to '_top' | ||
| if (fileName === '') { | ||
| fileName = '_top'; | ||
| } | ||
| } | ||
| catch (err) { | ||
| logger.error('Failed to get revision data', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
|
|
||
| // replace forbidden characters to '_' | ||
| // refer to https://kb.acronis.com/node/56475?ckattempt=1 | ||
| let fileName = sanitize(path.basename(pagePath), { replacement: '_' }); | ||
| let stream: Readable; | ||
|
|
||
| try { | ||
| if (exportService == null) { | ||
| throw new Error('exportService is not initialized'); | ||
| } | ||
| stream = exportService.getReadStreamFromRevision(revision, format); | ||
| } catch (err) { | ||
| logger.error('Failed to create readStream', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
|
|
||
| // replace root page name to '_top' | ||
| if (fileName === '') { | ||
| fileName = '_top'; | ||
| } | ||
| res.set({ | ||
| 'Content-Disposition': `attachment;filename*=UTF-8''${encodeURIComponent(fileName)}.${format}`, | ||
| }); | ||
|
|
||
| let stream: Readable; | ||
| const parameters = { | ||
| ip: req.ip, | ||
| endpoint: req.originalUrl, | ||
| action: SupportedAction.ACTION_PAGE_EXPORT, | ||
| user: req.user?._id, | ||
| snapshot: { | ||
| username: req.user?.username, | ||
| }, | ||
| }; | ||
| await crowi.activityService.createActivity(parameters); | ||
|
|
||
| try { | ||
| if (exportService == null) { | ||
| throw new Error('exportService is not initialized'); | ||
| } | ||
| stream = exportService.getReadStreamFromRevision(revision, format); | ||
| } | ||
| catch (err) { | ||
| logger.error('Failed to create readStream', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
|
|
||
| res.set({ | ||
| 'Content-Disposition': `attachment;filename*=UTF-8''${encodeURIComponent(fileName)}.${format}`, | ||
| }); | ||
|
|
||
| const parameters = { | ||
| ip: req.ip, | ||
| endpoint: req.originalUrl, | ||
| action: SupportedAction.ACTION_PAGE_EXPORT, | ||
| user: req.user?._id, | ||
| snapshot: { | ||
| username: req.user?.username, | ||
| }, | ||
| }; | ||
| await crowi.activityService.createActivity(parameters); | ||
|
|
||
| await pipeline(stream, res); | ||
| }); | ||
| await pipeline(stream, res); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
This route handler performs
a database access
| async (req, res) => { | ||
| const { pageId } = req.params; | ||
| const { expandContentWidth } = req.body; | ||
|
|
||
| const isContainerFluidBySystem = configManager.getConfig('customize:isContainerFluid'); | ||
| const isContainerFluidBySystem = configManager.getConfig( | ||
| 'customize:isContainerFluid', | ||
| ); | ||
|
|
||
| try { | ||
| const updateQuery = expandContentWidth === isContainerFluidBySystem | ||
| ? { $unset: { expandContentWidth } } // remove if the specified value is the same to the system's one | ||
| : { $set: { expandContentWidth } }; | ||
| const updateQuery = | ||
| expandContentWidth === isContainerFluidBySystem | ||
| ? { $unset: { expandContentWidth } } // remove if the specified value is the same to the system's one | ||
| : { $set: { expandContentWidth } }; | ||
|
|
||
| const page = await Page.updateOne({ _id: pageId }, updateQuery); | ||
| return res.apiv3({ page }); | ||
| } | ||
| catch (err) { | ||
| } catch (err) { | ||
| logger.error('update-content-width-failed', err); | ||
| return res.apiv3Err(err, 500); | ||
| } | ||
| }); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 14 hours ago
To fix this, we should add a rate limiting middleware to the /api/v3/page/:pageId/content-width route. The recommended approach is to use the express-rate-limit package, as per the alert and best practices. The fix involves:
- Importing
express-rate-limitin this file. - Instantiating a rate limiter with suitable configuration (e.g., 100 requests per 15 minutes per IP).
- Adding this rate limiter middleware to the route definition for
PUT /:pageId/content-width(line 1307+).
The rate limiter should be placed before the route handler and after other authentication/authorization middlewares, so as not to interfere with them.
Ifexpress-rate-limitis not already installed, the package needs to be added to project dependencies.
-
Copy modified line R23 -
Copy modified line R1310
| @@ -20,6 +20,7 @@ | ||
| import type { HydratedDocument } from 'mongoose'; | ||
| import mongoose from 'mongoose'; | ||
| import path from 'path'; | ||
| import rateLimit from 'express-rate-limit'; | ||
| import sanitize from 'sanitize-filename'; | ||
| import type { Readable } from 'stream'; | ||
| import { pipeline } from 'stream/promises'; | ||
| @@ -1306,6 +1307,7 @@ | ||
| */ | ||
| router.put( | ||
| '/:pageId/content-width', | ||
| contentWidthLimiter, | ||
| accessTokenParser([SCOPE.WRITE.FEATURES.PAGE], { acceptLegacy: true }), | ||
| loginRequiredStrictly, | ||
| excludeReadOnlyUser, |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
…iv3-routes-biome-3 support: Configure biome for apiv3 routes (remaining ts files)
| async (req: any, res: any) => { | ||
| const { user } = req; | ||
| const { settings } = req.body; | ||
|
|
||
| // extract only necessary params | ||
| const updateData = { | ||
| currentSidebarContents: settings.currentSidebarContents, | ||
| currentProductNavWidth: settings.currentProductNavWidth, | ||
| preferCollapsedModeByUser: settings.preferCollapsedModeByUser, | ||
| }; | ||
| // extract only necessary params | ||
| const updateData = { | ||
| currentSidebarContents: settings.currentSidebarContents, | ||
| currentProductNavWidth: settings.currentProductNavWidth, | ||
| preferCollapsedModeByUser: settings.preferCollapsedModeByUser, | ||
| }; | ||
|
|
||
| if (user == null) { | ||
| if (req.session.uiSettings == null) { | ||
| req.session.uiSettings = {}; | ||
| if (user == null) { | ||
| if (req.session.uiSettings == null) { | ||
| req.session.uiSettings = {}; | ||
| } | ||
| Object.keys(updateData).forEach((setting) => { | ||
| if (updateData[setting] != null) { | ||
| req.session.uiSettings[setting] = updateData[setting]; | ||
| } | ||
| }); | ||
| return res.apiv3(updateData); | ||
| } | ||
| Object.keys(updateData).forEach((setting) => { | ||
| if (updateData[setting] != null) { | ||
| req.session.uiSettings[setting] = updateData[setting]; | ||
|
|
||
| // remove the keys that have null value | ||
| Object.keys(updateData).forEach((key) => { | ||
| if (updateData[key] == null) { | ||
| delete updateData[key]; | ||
| } | ||
| }); | ||
| return res.apiv3(updateData); | ||
| } | ||
|
|
||
|
|
||
| // remove the keys that have null value | ||
| Object.keys(updateData).forEach((key) => { | ||
| if (updateData[key] == null) { | ||
| delete updateData[key]; | ||
| } | ||
| }); | ||
|
|
||
| try { | ||
| const updatedSettings = await UserUISettings.findOneAndUpdate( | ||
| { user: user._id }, | ||
| { | ||
| $set: { | ||
| user: user._id, | ||
| ...updateData, | ||
| try { | ||
| const updatedSettings = await UserUISettings.findOneAndUpdate( | ||
| { user: user._id }, | ||
| { | ||
| $set: { | ||
| user: user._id, | ||
| ...updateData, | ||
| }, | ||
| }, | ||
| }, | ||
| { upsert: true, new: true }, | ||
| ); | ||
| return res.apiv3(updatedSettings); | ||
| } | ||
| catch (err) { | ||
| logger.error('Error', err); | ||
| return res.apiv3Err(new ErrorV3(err)); | ||
| } | ||
| }); | ||
| { upsert: true, new: true }, | ||
| ); | ||
| return res.apiv3(updatedSettings); | ||
| } catch (err) { | ||
| logger.error('Error', err); | ||
| return res.apiv3Err(new ErrorV3(err)); | ||
| } | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To address this issue, we should apply a rate-limiting middleware specifically to the PUT /user-ui-settings route to ensure that rapid, repeated requests to this endpoint are throttled. The best practice is to use a proven library, such as express-rate-limit. In this fix, we would:
- Import the
express-rate-limitlibrary in the file. - Configure a new rate limiter specifically for this endpoint with reasonable settings (for example, 10 requests per minute per IP).
- Insert this middleware at the appropriate place in the route, before the actual handler, but after validators.
- Avoid affecting other routes and preserve existing functionality and middleware order.
- If
express-rate-limitis not already installed in the project, note this as a required dependency.
Required code edits:
- Add an import for
express-rate-limit. - Define a
userUiSettingsLimiterwith suitable limits. - Add
userUiSettingsLimiterto therouter.put('/', ...)middleware stack (ideally after validation, before business logic).
-
Copy modified line R4 -
Copy modified lines R17-R28 -
Copy modified line R90
| @@ -1,6 +1,7 @@ | ||
| import { ErrorV3 } from '@growi/core/dist/models'; | ||
| import express from 'express'; | ||
| import { body } from 'express-validator'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| import { AllSidebarContentsType } from '~/interfaces/ui'; | ||
| import loggerFactory from '~/utils/logger'; | ||
| @@ -13,6 +14,18 @@ | ||
| const router = express.Router(); | ||
|
|
||
| module.exports = () => { | ||
| // Rate limiter for this endpoint: e.g., 10 requests per minute per IP | ||
| const userUiSettingsLimiter = rateLimit({ | ||
| windowMs: 60 * 1000, // 1 minute | ||
| max: 10, | ||
| message: { | ||
| errorCode: 'too_many_requests', | ||
| message: 'Too many user settings updates from this IP, please try again later.', | ||
| }, | ||
| standardHeaders: true, // Return rate limit info in the `RateLimit-*` headers | ||
| legacyHeaders: false, // Disable the `X-RateLimit-*` headers | ||
| }); | ||
|
|
||
| const validatorForPut = [ | ||
| body('settings') | ||
| .exists() | ||
| @@ -74,6 +87,7 @@ | ||
| '/', | ||
| validatorForPut, | ||
| apiV3FormValidator, | ||
| userUiSettingsLimiter, | ||
| async (req: any, res: any) => { | ||
| const { user } = req; | ||
| const { settings } = req.body; |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| async (req: FormRequest, res: ApiV3Response) => { | ||
| if (!req.form.isValid) { | ||
| const errors = req.form.errors; | ||
| return res.apiv3Err(errors, 400); | ||
| } | ||
| return res.apiv3Err(new ErrorV3(err, 'failed_to_install')); | ||
| } | ||
|
|
||
| await crowi.appService.setupAfterInstall(); | ||
|
|
||
| const parameters = { action: SupportedAction.ACTION_USER_REGISTRATION_SUCCESS }; | ||
| activityEvent.emit('update', res.locals.activity._id, parameters); | ||
|
|
||
| // login with passport | ||
| req.logIn(adminUser, (err) => { | ||
| if (err != null) { | ||
| return res.apiv3Err(new ErrorV3(err, 'failed_to_login_after_install')); | ||
| const registerForm = req.body.registerForm || {}; | ||
|
|
||
| const name = registerForm.name; | ||
| const username = registerForm.username; | ||
| const email = registerForm.email; | ||
| const password = registerForm.password; | ||
| const language = registerForm['app:globalLang'] || 'en_US'; | ||
|
|
||
| const installerService = new InstallerService(crowi); | ||
|
|
||
| let adminUser: IUser; | ||
| try { | ||
| adminUser = await installerService.install( | ||
| { | ||
| name, | ||
| username, | ||
| email, | ||
| password, | ||
| }, | ||
| language, | ||
| ); | ||
| } catch (err) { | ||
| if (err instanceof FailedToCreateAdminUserError) { | ||
| return res.apiv3Err( | ||
| new ErrorV3(err.message, 'failed_to_create_admin_user'), | ||
| ); | ||
| } | ||
| return res.apiv3Err(new ErrorV3(err, 'failed_to_install')); | ||
| } | ||
|
|
||
| return res.apiv3({ message: 'Installation completed (Logged in as an admin user)' }); | ||
| }); | ||
| }); | ||
| await crowi.appService.setupAfterInstall(); | ||
|
|
||
| const parameters = { | ||
| action: SupportedAction.ACTION_USER_REGISTRATION_SUCCESS, | ||
| }; | ||
| activityEvent.emit('update', res.locals.activity._id, parameters); | ||
|
|
||
| // login with passport | ||
| req.logIn(adminUser, (err) => { | ||
| if (err != null) { | ||
| return res.apiv3Err( | ||
| new ErrorV3(err, 'failed_to_login_after_install'), | ||
| ); | ||
| } | ||
|
|
||
| return res.apiv3({ | ||
| message: 'Installation completed (Logged in as an admin user)', | ||
| }); | ||
| }); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
authorization
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix the issue, we should add a rate-limiting middleware to the /installer POST route. The recommended approach is to use the well-known express-rate-limit library.
- Add an import for
express-rate-limit. - Create a rate limiter instance with appropriate configuration (for installer, very few requests should be allowed; e.g.,
max: 5per 15 minutes is reasonable). - Apply this middleware specifically to the installer POST route, inserted before the expensive handler and after any needed validation/authorization.
- Ensure no other logic or behavior is altered: just add the import and apply the middleware in the route handler sequence.
-
Copy modified line R5 -
Copy modified lines R36-R42 -
Copy modified line R95
| @@ -2,6 +2,7 @@ | ||
| import { ErrorV3 } from '@growi/core/dist/models'; | ||
| import type { Request, Router } from 'express'; | ||
| import express from 'express'; | ||
| import rateLimit from 'express-rate-limit'; | ||
|
|
||
| import { SupportedAction } from '~/interfaces/activity'; | ||
| import { configManager } from '~/server/service/config-manager'; | ||
| @@ -32,6 +33,13 @@ | ||
|
|
||
| const router = express.Router(); | ||
|
|
||
| // installer endpoint rate limiter | ||
| const installerRateLimiter = rateLimit({ | ||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||
| max: 5, // max 5 requests per windowMs | ||
| message: 'Too many install attempts from this IP, please try again later.', | ||
| }); | ||
|
|
||
| // check application is not installed yet | ||
| router.use( | ||
| applicationNotInstalled.generateCheckerMiddleware(crowi), | ||
| @@ -84,6 +92,7 @@ | ||
| // eslint-disable-next-line max-len | ||
| router.post( | ||
| '/', | ||
| installerRateLimiter, | ||
| registerRules(minPasswordLength), | ||
| registerValidation, | ||
| addActivity, |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
| catch (err) { | ||
| } catch (err) { | ||
| logger.error(err); | ||
| return res.apiv3Err(new ErrorV3('Failed to parse body.', 'parse_failed'), 500); | ||
| return res.apiv3Err( | ||
| new ErrorV3('Failed to parse body.', 'parse_failed'), | ||
| 500, | ||
| ); | ||
| } | ||
|
|
||
| try { | ||
| const { fileName, fileSize } = attachmentMap; | ||
| if (typeof fileName !== 'string' || fileName.length === 0 || fileName.length > 256) { | ||
| if ( | ||
| typeof fileName !== 'string' || | ||
| fileName.length === 0 || | ||
| fileName.length > 256 | ||
| ) { | ||
| logger.warn('Invalid fileName in attachment metadata.', { fileName }); | ||
| return res.apiv3Err(new ErrorV3('Invalid fileName in attachment metadata.', 'invalid_metadata'), 400); | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Invalid fileName in attachment metadata.', | ||
| 'invalid_metadata', | ||
| ), | ||
| 400, | ||
| ); | ||
| } | ||
| if (typeof fileSize !== 'number' || !Number.isInteger(fileSize) || fileSize < 0) { | ||
| if ( | ||
| typeof fileSize !== 'number' || | ||
| !Number.isInteger(fileSize) || | ||
| fileSize < 0 | ||
| ) { | ||
| logger.warn('Invalid fileSize in attachment metadata.', { fileSize }); | ||
| return res.apiv3Err(new ErrorV3('Invalid fileSize in attachment metadata.', 'invalid_metadata'), 400); | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Invalid fileSize in attachment metadata.', | ||
| 'invalid_metadata', | ||
| ), | ||
| 400, | ||
| ); | ||
| } | ||
| const count = await Attachment.countDocuments({ fileName, fileSize }); | ||
| if (count === 0) { | ||
| logger.warn('Attachment not found in collection.', { fileName, fileSize }); | ||
| return res.apiv3Err(new ErrorV3('Attachment not found in collection.', 'attachment_not_found'), 404); | ||
| logger.warn('Attachment not found in collection.', { | ||
| fileName, | ||
| fileSize, | ||
| }); | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Attachment not found in collection.', | ||
| 'attachment_not_found', | ||
| ), | ||
| 404, | ||
| ); | ||
| } | ||
| } | ||
| catch (err) { | ||
| } catch (err) { | ||
| logger.error(err); | ||
| return res.apiv3Err(new ErrorV3('Failed to check attachment existence.', 'attachment_check_failed'), 500); | ||
| return res.apiv3Err( | ||
| new ErrorV3( | ||
| 'Failed to check attachment existence.', | ||
| 'attachment_check_failed', | ||
| ), | ||
| 500, | ||
| ); | ||
| } | ||
|
|
||
| const fileStream = createReadStream(file.path, { | ||
| flags: 'r', mode: 0o666, autoClose: true, | ||
| flags: 'r', | ||
| mode: 0o666, | ||
| autoClose: true, | ||
| }); | ||
| try { | ||
| await g2gTransferReceiverService.receiveAttachment(fileStream, attachmentMap); | ||
| } | ||
| catch (err) { | ||
| await g2gTransferReceiverService.receiveAttachment( | ||
| fileStream, | ||
| attachmentMap, | ||
| ); | ||
| } catch (err) { | ||
| logger.error(err); | ||
| return res.apiv3Err(new ErrorV3('Failed to upload.', 'upload_failed'), 500); | ||
| return res.apiv3Err( | ||
| new ErrorV3('Failed to upload.', 'upload_failed'), | ||
| 500, | ||
| ); | ||
| } | ||
|
|
||
| return res.apiv3({ message: 'Successfully imported attached file.' }); | ||
| }); | ||
| }, |
Check failure
Code scanning / CodeQL
Missing rate limiting High
a database access
This route handler performs
a file system access
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To address the lack of rate limiting for the /attachment POST endpoint, we should add rate limiting middleware, preferably from a well-known package like express-rate-limit. The recommended approach is:
- Install
express-rate-limit. - Import it in
apps/app/src/server/routes/apiv3/g2g-transfer.ts. - Define a suitable rate limiter (e.g., limit to X requests per client/IP per time window, tailored for attachment uploads).
- Apply the middleware to the
/attachmentPOST route as the first handler (beforevalidateTransferKey, etc.), so that rate-limited requests are rejected before any file/database work is attempted.
All code edits will be in the shown code of apps/app/src/server/routes/apiv3/g2g-transfer.ts.
-
Copy modified line R5 -
Copy modified lines R32-R42 -
Copy modified line R454
| @@ -2,6 +2,7 @@ | ||
| import { ErrorV3 } from '@growi/core/dist/models'; | ||
| import type { NextFunction, Request, Router } from 'express'; | ||
| import express from 'express'; | ||
| import rateLimit from 'express-rate-limit'; | ||
| import { body } from 'express-validator'; | ||
| import { createReadStream } from 'fs'; | ||
| import multer from 'multer'; | ||
| @@ -28,6 +29,17 @@ | ||
| user?: any; | ||
| } | ||
|
|
||
| // Rate limiter for /attachment endpoint, e.g. 10 requests per 15 minutes per IP | ||
| const attachmentRateLimiter = 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 | ||
| message: { | ||
| error: 'Too many attachment import requests, please try again later.' | ||
| } | ||
| }); | ||
|
|
||
| const logger = loggerFactory('growi:routes:apiv3:transfer'); | ||
|
|
||
| const validator = { | ||
| @@ -439,6 +451,7 @@ | ||
| // This endpoint uses multer's MemoryStorage since the received data should be persisted directly on attachment storage. | ||
| receiveRouter.post( | ||
| '/attachment', | ||
| attachmentRateLimiter, | ||
| validateTransferKey, | ||
| uploadsForAttachment.single('content'), | ||
| async (req: Request & { file: any }, res: ApiV3Response) => { |
-
Copy modified lines R256-R257
| @@ -253,7 +253,8 @@ | ||
| "y-mongodb-provider": "^0.2.0", | ||
| "y-socket.io": "^1.1.3", | ||
| "yjs": "^13.6.18", | ||
| "zod": "^3.24.2" | ||
| "zod": "^3.24.2", | ||
| "express-rate-limit": "^8.2.1" | ||
| }, | ||
| "// comments for defDependencies": { | ||
| "bootstrap": "v5.3.3 has a bug. refs: https://github.com/twbs/bootstrap/issues/39798", |
| Package | Version | Security advisories |
| express-rate-limit (npm) | 8.2.1 | None |
…s into a single method
… into the update process
…roved maintainability
fix: Admin form degradation
feat: Add loading spinner to admin home
…ttachment-service
Release v7.3.7
…-into-the-invitation-form fix: Cannot enter text into the invitation form
Release v7.3.9
…mporters support: Omit importers for esa.io and Qiita
Prepare v7.3.10-RC.0
…-on-every-page-transition-within-general-page fix: Flickering occurs on every page transition within general page
…moving debug logs
…moving debug logs
fix: PageTree Virtualization is measuring size incorrectly
…-not-displayed-in-search-results fix: Some page content not displayed in search results
…tem-control fix: /page/info API and PageItemControl condition
💎 Features
🚀 Improvement
🐛 Bug Fixes
🧰 Maintenance