Merge branch 'feat/181356-news-inappnotification-impl' into refactor/181356-182878-news-inappnotification-stale-item-deletion

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: [],

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

apps/app/src/features/news/server/services/feed-parser.ts

@@ -0,0 +1,65 @@
+import { z } from 'zod';
+
+import loggerFactory from '~/utils/logger';
+
+const logger = loggerFactory('growi:feature:news:feed-parser');
+
+const FeedItemSchema = z.object({
+  id: z.string().min(1),
+  type: z.string().optional(),
+  emoji: z.string().optional(),
+  title: z.record(z.string()),
+  body: z.record(z.string()).optional(),
+  url: z.string().optional(),
+  publishedAt: z.string().min(1),
+  conditions: z
+    .object({
+      targetRoles: z.array(z.string()).optional(),
+      growiVersionRegExps: z.array(z.string()).optional(),
+    })
+    .optional(),
+});
+
+const FeedJsonSchema = z.object({
+  version: z.string(),
+  // Items are parsed individually so a single bad item does not abort the batch
+  items: z.array(z.unknown()),
+});
+
+export type FeedItem = z.infer<typeof FeedItemSchema>;
+
+export interface FeedJson {
+  version: string;
+  items: FeedItem[];
+}
+
+/**
+ * Validate parsed JSON against the feed schema.
+ * Items failing per-item validation are skipped (logged), allowing the rest to be processed.
+ * Returns null when the top-level shape is invalid.
+ */
+export const parseFeedJson = (raw: unknown): FeedJson | null => {
+  const topResult = FeedJsonSchema.safeParse(raw);
+  if (!topResult.success) {
+    logger.error(
+      { issues: topResult.error.issues },
+      'News feed JSON top-level shape invalid',
+    );
+    return null;
+  }
+
+  const validItems: FeedItem[] = [];
+  for (const rawItem of topResult.data.items) {
+    const itemResult = FeedItemSchema.safeParse(rawItem);
+    if (itemResult.success) {
+      validItems.push(itemResult.data);
+    } else {
+      logger.warn(
+        { issues: itemResult.error.issues },
+        'News feed item failed validation, skipping',
+      );
+    }
+  }
+
+  return { version: topResult.data.version, items: validItems };
+};

apps/app/src/features/news/server/services/news-cron-service.spec.ts

@@ -50,6 +50,16 @@ const VALID_FEED = {
   ],
 };
 
+/** Build a Response-like mock that exposes `text()` returning the JSON-stringified body. */
+const mockResponse = (
+  body: unknown,
+  init?: { ok?: boolean; status?: number },
+) => ({
+  ok: init?.ok ?? true,
+  status: init?.status ?? 200,
+  text: () => Promise.resolve(JSON.stringify(body)),
+});
+
 describe('NewsCronService', () => {
   let service: NewsCronService;
   const originalEnv = process.env.NEWS_FEED_URL;
@@ -99,10 +109,7 @@ describe('NewsCronService', () => {
 
     test('should allow https:// URLs', async () => {
       process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(VALID_FEED),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(VALID_FEED));
 
       await service.executeJob();
 
@@ -114,10 +121,7 @@ describe('NewsCronService', () => {
 
     test('should allow http://localhost URLs', async () => {
       process.env.NEWS_FEED_URL = 'http://localhost:8099/feed.json';
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(VALID_FEED),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(VALID_FEED));
 
       await service.executeJob();
 
@@ -126,10 +130,7 @@ describe('NewsCronService', () => {
 
     test('should allow http://127.0.0.1 URLs', async () => {
       process.env.NEWS_FEED_URL = 'http://127.0.0.1:8099/feed.json';
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(VALID_FEED),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(VALID_FEED));
 
       await service.executeJob();
 
@@ -138,10 +139,7 @@ describe('NewsCronService', () => {
 
     test('should upsert items on successful fetch', async () => {
       process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(VALID_FEED),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(VALID_FEED));
 
       await service.executeJob();
 
@@ -191,10 +189,7 @@ describe('NewsCronService', () => {
           },
         ],
       };
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(feedWithVersionFilter),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(feedWithVersionFilter));
 
       await service.executeJob();
 
@@ -222,10 +217,7 @@ describe('NewsCronService', () => {
           },
         ],
       };
-      mocks.mockFetch.mockResolvedValue({
-        ok: true,
-        json: () => Promise.resolve(feedWithInvalidRegex),
-      });
+      mocks.mockFetch.mockResolvedValue(mockResponse(feedWithInvalidRegex));
 
       await service.executeJob();
 
@@ -272,6 +264,62 @@ describe('NewsCronService', () => {
       mocks.mockFetch.mockResolvedValue({
         ok: true,
         json: () => Promise.resolve(feed),
+    test('should skip when response body exceeds size limit (5 MiB)', async () => {
+      process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
+      // Build a string that exceeds 5 MiB
+      const oversizedText = 'x'.repeat(5 * 1024 * 1024 + 1);
+      mocks.mockFetch.mockResolvedValue({
+        ok: true,
+        text: () => Promise.resolve(oversizedText),
+      });
+
+      await service.executeJob();
+
+      expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
+      expect(mocks.deleteItemsNotInFeed).not.toHaveBeenCalled();
+    });
+
+    test('should abort when top-level shape is invalid', async () => {
+      process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
+      // Missing `items` field — top-level schema check fails
+      mocks.mockFetch.mockResolvedValue(mockResponse({ version: '1.0' }));
+
+      await service.executeJob();
+
+      expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
+      expect(mocks.deleteItemsNotInFeed).not.toHaveBeenCalled();
+    });
+
+    test('should skip individual invalid items but keep valid ones', async () => {
+      process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
+      const feedWithMixedItems = {
+        version: '1.0',
+        items: [
+          // Missing required fields (title, publishedAt) → skipped
+          { id: 'broken-item' },
+          // Valid item
+          {
+            id: 'good-item',
+            title: { ja_JP: '正常' },
+            publishedAt: '2026-01-01T00:00:00Z',
+          },
+        ],
+      };
+      mocks.mockFetch.mockResolvedValue(mockResponse(feedWithMixedItems));
+
+      await service.executeJob();
+
+      const upsertCall = mocks.upsertNewsItems.mock.calls[0][0];
+      expect(upsertCall.map((i: { id: string }) => i.id)).toEqual([
+        'good-item',
+      ]);
+    });
+
+    test('should skip when response body is not valid JSON', async () => {
+      process.env.NEWS_FEED_URL = 'https://example.com/feed.json';
+      mocks.mockFetch.mockResolvedValue({
+        ok: true,
+        text: () => Promise.resolve('not-a-json{'),
       });
 
       await service.executeJob();
@@ -285,6 +333,7 @@ describe('NewsCronService', () => {
         'still-present-2',
         'version-filtered',
       ]);
+      expect(mocks.upsertNewsItems).not.toHaveBeenCalled();
     });
   });
 });

apps/app/src/features/news/server/services/news-cron-service.ts

@@ -3,6 +3,7 @@ import { getGrowiVersion } from '~/utils/growi-version';
 import loggerFactory from '~/utils/logger';
 
 import type { INewsItemInput } from '../../interfaces/news-item';
+import { type FeedItem, parseFeedJson } from './feed-parser';
 import { NewsService } from './news-service';
 
 const logger = loggerFactory('growi:feature:news:cron');
@@ -13,24 +14,12 @@ const MAX_RANDOM_SLEEP_MS = 5 * 60 * 60 * 1000;
 /** HTTP fetch timeout in ms */
 const FETCH_TIMEOUT_MS = 10_000;
 
-interface FeedItem {
-  id: string;
-  type?: string;
-  emoji?: string;
-  title: Record<string, string>;
-  body?: Record<string, string>;
-  url?: string;
-  publishedAt: string;
-  conditions?: {
-    targetRoles?: string[];
-    growiVersionRegExps?: string[];
-  };
-}
-
-interface FeedJson {
-  version: string;
-  items: FeedItem[];
-}
+/**
+ * Maximum response body size (5 MiB).
+ * Sanity limit for the trust boundary at the news feed adapter — caps how much
+ * an external endpoint (broken or compromised) can push into our process memory.
+ */
+const MAX_RESPONSE_SIZE_BYTES = 5 * 1024 * 1024;
 
 /**
  * Check if the given URL is allowed for fetching
@@ -95,7 +84,7 @@ export class NewsCronService extends CronService {
     // Random sleep to distribute requests across multiple GROWI instances
     await randomSleep(MAX_RANDOM_SLEEP_MS);
 
-    let feedJson: FeedJson;
+    let rawJson: unknown;
     try {
       const response = await fetch(feedUrl, {
         signal: AbortSignal.timeout(FETCH_TIMEOUT_MS),
@@ -106,12 +95,25 @@ export class NewsCronService extends CronService {
         return;
       }
 
-      feedJson = (await response.json()) as FeedJson;
+      const text = await response.text();
+      if (Buffer.byteLength(text, 'utf8') > MAX_RESPONSE_SIZE_BYTES) {
+        logger.error(
+          `News feed response exceeds size limit (${MAX_RESPONSE_SIZE_BYTES} bytes), skipping`,
+        );
+        return;
+      }
+
+      rawJson = JSON.parse(text);
     } catch (err) {
       logger.error('Error fetching news feed, keeping existing data', err);
       return;
     }
 
+    const feedJson = parseFeedJson(rawJson);
+    if (feedJson == null) {
+      return;
+    }
+
     const currentVersion = getGrowiVersion();
     const filteredItems = feedJson.items.filter((item) =>
       matchesGrowiVersion(item, currentVersion),