From 7de2bda71d02472c30bb5d00af803c42ce6088cb Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Sun, 14 Jun 2026 06:55:06 -0700 Subject: [PATCH] fix(notifications): bound mark-read ids --- src/db/repositories.ts | 18 +++++++++++++++-- src/mcp/server.ts | 7 ++++++- test/unit/mcp-notifications.test.ts | 27 ++++++++++++++++++++++++- test/unit/notifications-service.test.ts | 21 +++++++++++++++++++ 4 files changed, 69 insertions(+), 4 deletions(-) diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 0dafdc7c99..bfba11e11d 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -1388,19 +1388,33 @@ export async function listNotificationDeliveriesForRecipient( return rows.map(toNotificationDeliveryRecord); } +export const MAX_NOTIFICATION_MARK_READ_IDS = 100; +export const MAX_NOTIFICATION_DELIVERY_ID_LENGTH = 128; + // Marks a recipient's delivered notifications read (the badge-clear action). Scoped to recipientLogin so a -// caller can never clear another user's notifications. Returns the number of rows transitioned. +// caller can never clear another user's notifications. Passing an empty ids array is a no-op. +// Returns the number of rows transitioned. export async function markNotificationDeliveriesRead( env: Env, recipientLogin: string, ids?: string[], ): Promise { + if (ids) { + if (ids.length === 0) return 0; + if (ids.length > MAX_NOTIFICATION_MARK_READ_IDS) { + throw new RangeError(`ids must contain at most ${MAX_NOTIFICATION_MARK_READ_IDS} entries`); + } + if (ids.some((id) => id.length > MAX_NOTIFICATION_DELIVERY_ID_LENGTH)) { + throw new RangeError(`ids entries must be at most ${MAX_NOTIFICATION_DELIVERY_ID_LENGTH} characters`); + } + } + const db = getDb(env.DB); const conditions: SQL[] = [ eq(notificationDeliveries.recipientLogin, recipientLogin.toLowerCase()), eq(notificationDeliveries.status, "delivered"), ]; - if (ids && ids.length > 0) conditions.push(inArray(notificationDeliveries.id, ids)); + if (ids) conditions.push(inArray(notificationDeliveries.id, ids)); const updated = await db .update(notificationDeliveries) .set({ status: "read", readAt: nowIso() }) diff --git a/src/mcp/server.ts b/src/mcp/server.ts index ce8484ad90..4fbeb315ea 100644 --- a/src/mcp/server.ts +++ b/src/mcp/server.ts @@ -29,6 +29,8 @@ import { listRepoSyncSegments, listRepoSyncStates, listRepositories, + MAX_NOTIFICATION_DELIVERY_ID_LENGTH, + MAX_NOTIFICATION_MARK_READ_IDS, markNotificationDeliveriesRead, recordProductUsageEvent, } from "../db/repositories"; @@ -371,7 +373,10 @@ const listNotificationsShape = { const markNotificationsReadShape = { login: z.string().min(1), - ids: z.array(z.string().min(1)).optional(), + ids: z + .array(z.string().min(1).max(MAX_NOTIFICATION_DELIVERY_ID_LENGTH)) + .max(MAX_NOTIFICATION_MARK_READ_IDS) + .optional(), }; const explainRepoDecisionOutputSchema = { diff --git a/test/unit/mcp-notifications.test.ts b/test/unit/mcp-notifications.test.ts index 6a4310c17b..8aa2d706a3 100644 --- a/test/unit/mcp-notifications.test.ts +++ b/test/unit/mcp-notifications.test.ts @@ -3,7 +3,12 @@ import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js"; import { describe, expect, it } from "vitest"; import { GittensoryMcp } from "../../src/mcp/server"; import { createSessionForGitHubUser, type AuthIdentity } from "../../src/auth/security"; -import { insertNotificationDeliveryIfAbsent, markNotificationDeliveryDelivered } from "../../src/db/repositories"; +import { + MAX_NOTIFICATION_DELIVERY_ID_LENGTH, + MAX_NOTIFICATION_MARK_READ_IDS, + insertNotificationDeliveryIfAbsent, + markNotificationDeliveryDelivered, +} from "../../src/db/repositories"; import { createTestEnv } from "../helpers/d1"; async function connect(env: Env, identity?: AuthIdentity) { @@ -50,6 +55,26 @@ describe("MCP notification tools", () => { expect((after.structuredContent as { unreadCount: number }).unreadCount).toBe(0); }); + it("rejects oversized mark-read id filters", async () => { + const env = createTestEnv(); + const client = await connect(env); + + const tooManyIds = await client.callTool({ + name: "gittensory_mark_notifications_read", + arguments: { + login: "miner", + ids: Array.from({ length: MAX_NOTIFICATION_MARK_READ_IDS + 1 }, (_, index) => `id-${index}`), + }, + }); + expect(tooManyIds.isError).toBe(true); + + const tooLongId = await client.callTool({ + name: "gittensory_mark_notifications_read", + arguments: { login: "miner", ids: ["x".repeat(MAX_NOTIFICATION_DELIVERY_ID_LENGTH + 1)] }, + }); + expect(tooLongId.isError).toBe(true); + }); + it("forbids reading or clearing another login's notifications from a scoped session", async () => { const env = createTestEnv(); const { session } = await createSessionForGitHubUser(env, { login: "miner", id: 1 }); diff --git a/test/unit/notifications-service.test.ts b/test/unit/notifications-service.test.ts index 24ca57f5d3..c8fece9301 100644 --- a/test/unit/notifications-service.test.ts +++ b/test/unit/notifications-service.test.ts @@ -9,6 +9,8 @@ import { } from "../../src/notifications/service"; import { getNotificationDeliveryById, + MAX_NOTIFICATION_DELIVERY_ID_LENGTH, + MAX_NOTIFICATION_MARK_READ_IDS, insertNotificationDeliveryIfAbsent, listNotificationDeliveriesForRecipient, listNotificationSubscriptionsForLogin, @@ -206,6 +208,11 @@ describe("notification repository helpers", () => { await deliverNotification(env, first!.id); await deliverNotification(env, second!.id); + // Empty ids selects no specific notifications. + expect(await markNotificationDeliveriesRead(env, "miner", [])).toBe(0); + expect((await getNotificationDeliveryById(env, first!.id))?.status).toBe("delivered"); + expect((await getNotificationDeliveryById(env, second!.id))?.status).toBe("delivered"); + // Mark only the first by id. expect(await markNotificationDeliveriesRead(env, "miner", [first!.id])).toBe(1); expect((await getNotificationDeliveryById(env, first!.id))?.status).toBe("read"); @@ -219,6 +226,20 @@ describe("notification repository helpers", () => { expect(feed.unreadCount).toBe(0); }); + it("rejects oversized mark-read id filters before building SQL", async () => { + const env = createTestEnv(); + await expect( + markNotificationDeliveriesRead( + env, + "miner", + Array.from({ length: MAX_NOTIFICATION_MARK_READ_IDS + 1 }, (_, index) => `id-${index}`), + ), + ).rejects.toThrow(/at most/); + await expect(markNotificationDeliveriesRead(env, "miner", ["x".repeat(MAX_NOTIFICATION_DELIVERY_ID_LENGTH + 1)])).rejects.toThrow( + /at most/, + ); + }); + it("returns null for an unknown delivery id", async () => { const env = createTestEnv(); expect(await getNotificationDeliveryById(env, "missing")).toBeNull();