Merge pull request #11257 from growilabs/fix/uploads-direct-access-xss

fix(attachment): deny direct access to /uploads to prevent stored XSS

apps/app/src/server/crowi/express-init.js

@@ -10,6 +10,7 @@ import {
 } from '../../features/growi-plugin/server/consts';
 import loggerFactory from '../../utils/logger';
 import CertifyOrigin from '../middlewares/certify-origin';
+import { denyUploadsDirectAccess } from '../middlewares/deny-uploads-direct-access';
 import registerSafeRedirectFactory from '../middlewares/safe-redirect';
 
 const logger = loggerFactory('growi:crowi:express-init');
@@ -92,6 +93,11 @@ module.exports = (crowi, app) => {
   app.set('port', crowi.port);
 
   const staticOption = crowi.node_env === 'production' ? { maxAge: '30d' } : {};
+  // Deny direct access to uploaded files (publicDir/uploads/**) BEFORE static
+  // serving. Uploads must be served only via the /attachment and /download
+  // routes, which apply authorization, Content-Disposition and CSP headers.
+  // see: src/server/middlewares/deny-uploads-direct-access.ts
+  app.use('/uploads', denyUploadsDirectAccess);
   app.use(express.static(crowi.publicDir, staticOption));
   app.use(
     '/static/preset-themes',

apps/app/src/server/middlewares/deny-uploads-direct-access.spec.ts

@@ -0,0 +1,21 @@
+import type { Request, Response } from 'express';
+import { mock } from 'vitest-mock-extended';
+
+import { denyUploadsDirectAccess } from './deny-uploads-direct-access';
+
+describe('denyUploadsDirectAccess', () => {
+  test('responds with 403 Forbidden', () => {
+    const req = mock<Request>();
+    req.originalUrl = '/uploads/attachment/evil.html';
+
+    const res = mock<Response>();
+    // res.status(...) returns `this` (Response) in Express, enabling the
+    // status().send() chain. Mirror that so the chained send() can be asserted.
+    res.status.mockReturnValue(res);
+
+    denyUploadsDirectAccess(req, res);
+
+    expect(res.status).toHaveBeenCalledWith(403);
+    expect(res.send).toHaveBeenCalledWith('Forbidden');
+  });
+});

apps/app/src/server/middlewares/deny-uploads-direct-access.ts

@@ -0,0 +1,25 @@
+import type { Request, Response } from 'express';
+
+import loggerFactory from '~/utils/logger';
+
+const logger = loggerFactory('growi:middleware:deny-uploads-direct-access');
+
+/**
+ * Deny direct access to uploaded files stored under `publicDir/uploads/**`.
+ *
+ * When the upload method is "Local", attachments are written under
+ * `publicDir/uploads/**`, which would otherwise be served directly by
+ * `express.static(publicDir)`. Serving them statically bypasses the
+ * `/attachment` and `/download` routes that apply authorization,
+ * `Content-Disposition` and `Content-Security-Policy` headers, enabling stored
+ * XSS and access-control bypass.
+ *
+ * This middleware blanket-denies the whole `/uploads` prefix (attachment, user,
+ * page-bulk-export, audit-log-bulk-export and any future subdirectory) so that
+ * adding a new storage prefix never silently re-opens the hole. It MUST be
+ * registered BEFORE `express.static(publicDir)`.
+ */
+export const denyUploadsDirectAccess = (req: Request, res: Response): void => {
+  logger.debug(`Blocked direct access to an uploaded file: ${req.originalUrl}`);
+  res.status(403).send('Forbidden');
+};