Browse Source

refactor(news): cap list limit at 100

Silently caps the caller-supplied `limit` query parameter to [1, 100]
on GET /news/list to prevent unbounded result sets being pulled into
memory by misuse of the API.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Ryotaro Nagahara 2 weeks ago
parent
commit
ad7687e3b2

+ 75 - 0
apps/app/src/features/news/server/routes/news.spec.ts

@@ -128,6 +128,81 @@ describe('News API routes', () => {
       );
     });
 
+    test('should silently cap limit at 100 when caller exceeds the upper bound', async () => {
+      mocks.listForUser.mockResolvedValue({
+        docs: [],
+        totalDocs: 0,
+        limit: 100,
+        offset: 0,
+        page: 1,
+        totalPages: 1,
+        hasNextPage: false,
+        hasPrevPage: false,
+        nextPage: null,
+        prevPage: null,
+        pagingCounter: 1,
+      });
+
+      const app = buildApp();
+      await request(app).get('/apiv3/news/list?limit=99999');
+
+      expect(mocks.listForUser).toHaveBeenCalledWith(
+        expect.anything(),
+        expect.any(Array),
+        expect.objectContaining({ limit: 100 }),
+      );
+    });
+
+    test('should fall back to default limit when caller passes a non-numeric value', async () => {
+      mocks.listForUser.mockResolvedValue({
+        docs: [],
+        totalDocs: 0,
+        limit: 10,
+        offset: 0,
+        page: 1,
+        totalPages: 1,
+        hasNextPage: false,
+        hasPrevPage: false,
+        nextPage: null,
+        prevPage: null,
+        pagingCounter: 1,
+      });
+
+      const app = buildApp();
+      await request(app).get('/apiv3/news/list?limit=abc');
+
+      expect(mocks.listForUser).toHaveBeenCalledWith(
+        expect.anything(),
+        expect.any(Array),
+        expect.objectContaining({ limit: 10 }),
+      );
+    });
+
+    test('should clamp limit up to 1 when caller passes a negative value', async () => {
+      mocks.listForUser.mockResolvedValue({
+        docs: [],
+        totalDocs: 0,
+        limit: 1,
+        offset: 0,
+        page: 1,
+        totalPages: 1,
+        hasNextPage: false,
+        hasPrevPage: false,
+        nextPage: null,
+        prevPage: null,
+        pagingCounter: 1,
+      });
+
+      const app = buildApp();
+      await request(app).get('/apiv3/news/list?limit=-5');
+
+      expect(mocks.listForUser).toHaveBeenCalledWith(
+        expect.anything(),
+        expect.any(Array),
+        expect.objectContaining({ limit: 1 }),
+      );
+    });
+
     test('should pass onlyUnread=true when query param is set', async () => {
       mocks.listForUser.mockResolvedValue({
         docs: [],

+ 22 - 4
apps/app/src/features/news/server/routes/news.ts

@@ -13,6 +13,14 @@ import { NewsService } from '../services/news-service';
 
 const logger = loggerFactory('growi:feature:news:routes');
 
+/**
+ * Maximum number of news items returnable per request.
+ * Caps caller-supplied `limit` so a misuse cannot make a single request
+ * pull an unbounded result set into memory.
+ */
+const MAX_LIST_LIMIT = 100;
+const DEFAULT_LIST_LIMIT = 10;
+
 type NewsRequest = CrowiRequest & { user: IUserHasId };
 
 /**
@@ -22,6 +30,19 @@ const getUserRoles = (user: IUserHasId): string[] => {
   return user.admin ? ['admin'] : ['general'];
 };
 
+/**
+ * Resolve the effective list limit from a query value.
+ * Falls back to `DEFAULT_LIST_LIMIT` for missing/invalid input,
+ * and silently caps the result to `[1, MAX_LIST_LIMIT]`.
+ */
+const resolveLimit = (raw: unknown): number => {
+  const requested =
+    raw != null
+      ? parseInt(String(raw), 10) || DEFAULT_LIST_LIMIT
+      : DEFAULT_LIST_LIMIT;
+  return Math.min(Math.max(requested, 1), MAX_LIST_LIMIT);
+};
+
 /**
  * Creates and returns the news Express router.
  * Accepts an optional Crowi instance for middleware setup.
@@ -50,10 +71,7 @@ export const createNewsRouter = (crowi?: Crowi): express.Router => {
         const user = req.user;
         const userRoles = getUserRoles(user);
 
-        const limit =
-          req.query.limit != null
-            ? parseInt(String(req.query.limit), 10) || 10
-            : 10;
+        const limit = resolveLimit(req.query.limit);
         const offset =
           req.query.offset != null
             ? parseInt(String(req.query.offset), 10) || 0