Ver Fonte

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

When the upload method is "Local", attachments are stored under
publicDir/uploads/** and were served directly by express.static(publicDir).
Direct access (e.g. /uploads/attachment/<file>) bypassed the /attachment and
/download routes, which apply authorization, Content-Disposition and
Content-Security-Policy headers — enabling stored XSS and access-control bypass.

Add a deny middleware that blanket-blocks the whole /uploads prefix and register
it before express.static so uploads are reachable only via the proxied routes.
Blocking the prefix (not individual subdirectories) ensures future storage
prefixes never silently re-open the hole.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Shun Miyazawa há 1 semana atrás
pai
commit
89eb0fcfdb

+ 6 - 0
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',

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

@@ -0,0 +1,19 @@
+import type { Request, Response } from 'express';
+
+import { denyUploadsDirectAccess } from './deny-uploads-direct-access';
+
+describe('denyUploadsDirectAccess', () => {
+  test('responds with 403 Forbidden', () => {
+    const send = vi.fn();
+    const status = vi.fn().mockReturnValue({ send });
+    const req = {
+      originalUrl: '/uploads/attachment/evil.html',
+    } as unknown as Request;
+    const res = { status } as unknown as Response;
+
+    denyUploadsDirectAccess(req, res);
+
+    expect(status).toHaveBeenCalledWith(403);
+    expect(send).toHaveBeenCalledWith('Forbidden');
+  });
+});

+ 25 - 0
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');
+};