From 0ee30361b8f6ef3f110f3a7b001da6dd3df96bb5 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Tue, 24 Feb 2026 22:55:03 +0000 Subject: [PATCH] fix(synology-chat): fail closed empty allowlist --- docs/channels/synology-chat.md | 1 + extensions/synology-chat/src/channel.test.ts | 19 +++++++++++++++ extensions/synology-chat/src/channel.ts | 5 ++++ extensions/synology-chat/src/security.test.ts | 4 ++-- extensions/synology-chat/src/security.ts | 3 +-- .../synology-chat/src/webhook-handler.test.ts | 20 ++++++++++++++++ .../synology-chat/src/webhook-handler.ts | 24 ++++++++++++------- 7 files changed, 63 insertions(+), 13 deletions(-) diff --git a/docs/channels/synology-chat.md b/docs/channels/synology-chat.md index 78beff43bc4..37c5151f1c9 100644 --- a/docs/channels/synology-chat.md +++ b/docs/channels/synology-chat.md @@ -72,6 +72,7 @@ Config values override env vars. - `dmPolicy: "allowlist"` is the recommended default. - `allowedUserIds` accepts a list (or comma-separated string) of Synology user IDs. +- In `allowlist` mode, an empty `allowedUserIds` list blocks all senders (use `dmPolicy: "open"` for allow-all). - `dmPolicy: "open"` allows any sender. - `dmPolicy: "disabled"` blocks DMs. - Pairing approvals work with: diff --git a/extensions/synology-chat/src/channel.test.ts b/extensions/synology-chat/src/channel.test.ts index 076339c4456..080cf4531c9 100644 --- a/extensions/synology-chat/src/channel.test.ts +++ b/extensions/synology-chat/src/channel.test.ts @@ -183,6 +183,25 @@ describe("createSynologyChatPlugin", () => { expect(warnings.some((w: string) => w.includes("open"))).toBe(true); }); + it("warns when dmPolicy is allowlist and allowedUserIds is empty", () => { + const plugin = createSynologyChatPlugin(); + const account = { + accountId: "default", + enabled: true, + token: "t", + incomingUrl: "https://nas/incoming", + nasHost: "h", + webhookPath: "/w", + dmPolicy: "allowlist" as const, + allowedUserIds: [], + rateLimitPerMinute: 30, + botName: "Bot", + allowInsecureSsl: false, + }; + const warnings = plugin.security.collectWarnings({ account }); + expect(warnings.some((w: string) => w.includes("empty allowedUserIds"))).toBe(true); + }); + it("returns no warnings for fully configured account", () => { const plugin = createSynologyChatPlugin(); const account = { diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts index 37d4a4216ba..287028abc24 100644 --- a/extensions/synology-chat/src/channel.ts +++ b/extensions/synology-chat/src/channel.ts @@ -141,6 +141,11 @@ export function createSynologyChatPlugin() { '- Synology Chat: dmPolicy="open" allows any user to message the bot. Consider "allowlist" for production use.', ); } + if (account.dmPolicy === "allowlist" && account.allowedUserIds.length === 0) { + warnings.push( + '- Synology Chat: dmPolicy="allowlist" with empty allowedUserIds blocks all senders. Add users or set dmPolicy="open".', + ); + } return warnings; }, }, diff --git a/extensions/synology-chat/src/security.test.ts b/extensions/synology-chat/src/security.test.ts index 11330dcddc8..7e639da9486 100644 --- a/extensions/synology-chat/src/security.test.ts +++ b/extensions/synology-chat/src/security.test.ts @@ -24,8 +24,8 @@ describe("validateToken", () => { }); describe("checkUserAllowed", () => { - it("allows any user when allowlist is empty", () => { - expect(checkUserAllowed("user1", [])).toBe(true); + it("rejects user when allowlist is empty", () => { + expect(checkUserAllowed("user1", [])).toBe(false); }); it("allows user in the allowlist", () => { diff --git a/extensions/synology-chat/src/security.ts b/extensions/synology-chat/src/security.ts index 43ff054b077..12336b876b9 100644 --- a/extensions/synology-chat/src/security.ts +++ b/extensions/synology-chat/src/security.ts @@ -22,10 +22,9 @@ export function validateToken(received: string, expected: string): boolean { /** * Check if a user ID is in the allowed list. - * Empty allowlist = allow all users. + * Allowlist mode must be explicit; empty lists should not match any user. */ export function checkUserAllowed(userId: string, allowedUserIds: string[]): boolean { - if (allowedUserIds.length === 0) return true; return allowedUserIds.includes(userId); } diff --git a/extensions/synology-chat/src/webhook-handler.test.ts b/extensions/synology-chat/src/webhook-handler.test.ts index 7e20c100610..1c8ef393ced 100644 --- a/extensions/synology-chat/src/webhook-handler.test.ts +++ b/extensions/synology-chat/src/webhook-handler.test.ts @@ -156,6 +156,26 @@ describe("createWebhookHandler", () => { }); }); + it("returns 403 when allowlist policy is set with empty allowedUserIds", async () => { + const deliver = vi.fn(); + const handler = createWebhookHandler({ + account: makeAccount({ + dmPolicy: "allowlist", + allowedUserIds: [], + }), + deliver, + log, + }); + + const req = makeReq("POST", validBody); + const res = makeRes(); + await handler(req, res); + + expect(res._status).toBe(403); + expect(res._body).toContain("Allowlist is empty"); + expect(deliver).not.toHaveBeenCalled(); + }); + it("returns 403 when DMs are disabled", async () => { await expectForbiddenByPolicy({ account: { dmPolicy: "disabled" }, diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts index d1dae50a673..0ed5dd844c5 100644 --- a/extensions/synology-chat/src/webhook-handler.ts +++ b/extensions/synology-chat/src/webhook-handler.ts @@ -138,20 +138,26 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { } // User allowlist check - if ( - account.dmPolicy === "allowlist" && - !checkUserAllowed(payload.user_id, account.allowedUserIds) - ) { - log?.warn(`Unauthorized user: ${payload.user_id}`); - respond(res, 403, { error: "User not authorized" }); - return; - } - if (account.dmPolicy === "disabled") { respond(res, 403, { error: "DMs are disabled" }); return; } + if (account.dmPolicy === "allowlist") { + if (account.allowedUserIds.length === 0) { + log?.warn("Synology Chat allowlist is empty while dmPolicy=allowlist; rejecting message"); + respond(res, 403, { + error: "Allowlist is empty. Configure allowedUserIds or use dmPolicy=open.", + }); + return; + } + if (!checkUserAllowed(payload.user_id, account.allowedUserIds)) { + log?.warn(`Unauthorized user: ${payload.user_id}`); + respond(res, 403, { error: "User not authorized" }); + return; + } + } + // Rate limit if (!rateLimiter.check(payload.user_id)) { log?.warn(`Rate limit exceeded for user: ${payload.user_id}`);