diff --git a/docs/channels/synology-chat.md b/docs/channels/synology-chat.md index 37c5151f1c9..89e96b318a3 100644 --- a/docs/channels/synology-chat.md +++ b/docs/channels/synology-chat.md @@ -72,7 +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). +- In `allowlist` mode, an empty `allowedUserIds` list is treated as misconfiguration and the webhook route will not start (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.integration.test.ts b/extensions/synology-chat/src/channel.integration.test.ts new file mode 100644 index 00000000000..6005cbd923b --- /dev/null +++ b/extensions/synology-chat/src/channel.integration.test.ts @@ -0,0 +1,129 @@ +import { EventEmitter } from "node:events"; +import type { IncomingMessage, ServerResponse } from "node:http"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +type RegisteredRoute = { + path: string; + accountId: string; + handler: (req: IncomingMessage, res: ServerResponse) => Promise; +}; + +const registerPluginHttpRouteMock = vi.fn<(params: RegisteredRoute) => () => void>(() => vi.fn()); +const dispatchReplyWithBufferedBlockDispatcher = vi.fn().mockResolvedValue({ counts: {} }); + +vi.mock("openclaw/plugin-sdk", () => ({ + DEFAULT_ACCOUNT_ID: "default", + setAccountEnabledInConfigSection: vi.fn((_opts: any) => ({})), + registerPluginHttpRoute: registerPluginHttpRouteMock, + buildChannelConfigSchema: vi.fn((schema: any) => ({ schema })), +})); + +vi.mock("./runtime.js", () => ({ + getSynologyRuntime: vi.fn(() => ({ + config: { loadConfig: vi.fn().mockResolvedValue({}) }, + channel: { + reply: { + dispatchReplyWithBufferedBlockDispatcher, + }, + }, + })), +})); + +vi.mock("./client.js", () => ({ + sendMessage: vi.fn().mockResolvedValue(true), + sendFileUrl: vi.fn().mockResolvedValue(true), +})); + +const { createSynologyChatPlugin } = await import("./channel.js"); + +function makeReq(method: string, body: string): IncomingMessage { + const req = new EventEmitter() as IncomingMessage; + req.method = method; + req.socket = { remoteAddress: "127.0.0.1" } as any; + process.nextTick(() => { + req.emit("data", Buffer.from(body)); + req.emit("end"); + }); + return req; +} + +function makeRes(): ServerResponse & { _status: number; _body: string } { + const res = { + _status: 0, + _body: "", + writeHead(statusCode: number, _headers: Record) { + res._status = statusCode; + }, + end(body?: string) { + res._body = body ?? ""; + }, + } as any; + return res; +} + +function makeFormBody(fields: Record): string { + return Object.entries(fields) + .map(([k, v]) => `${encodeURIComponent(k)}=${encodeURIComponent(v)}`) + .join("&"); +} + +describe("Synology channel wiring integration", () => { + beforeEach(() => { + registerPluginHttpRouteMock.mockClear(); + dispatchReplyWithBufferedBlockDispatcher.mockClear(); + }); + + it("registers real webhook handler with resolved account config and enforces allowlist", async () => { + const plugin = createSynologyChatPlugin(); + const ctx = { + cfg: { + channels: { + "synology-chat": { + enabled: true, + accounts: { + alerts: { + enabled: true, + token: "valid-token", + incomingUrl: "https://nas.example.com/incoming", + webhookPath: "/webhook/synology-alerts", + dmPolicy: "allowlist", + allowedUserIds: ["456"], + }, + }, + }, + }, + }, + accountId: "alerts", + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + }; + + const started = await plugin.gateway.startAccount(ctx); + expect(registerPluginHttpRouteMock).toHaveBeenCalledTimes(1); + + const firstCall = registerPluginHttpRouteMock.mock.calls[0]; + expect(firstCall).toBeTruthy(); + if (!firstCall) throw new Error("Expected registerPluginHttpRoute to be called"); + const registered = firstCall[0]; + expect(registered.path).toBe("/webhook/synology-alerts"); + expect(registered.accountId).toBe("alerts"); + expect(typeof registered.handler).toBe("function"); + + const req = makeReq( + "POST", + makeFormBody({ + token: "valid-token", + user_id: "123", + username: "unauthorized-user", + text: "Hello", + }), + ); + const res = makeRes(); + await registered.handler(req, res); + + expect(res._status).toBe(403); + expect(res._body).toContain("not authorized"); + expect(dispatchReplyWithBufferedBlockDispatcher).not.toHaveBeenCalled(); + + started.stop(); + }); +}); diff --git a/extensions/synology-chat/src/channel.test.ts b/extensions/synology-chat/src/channel.test.ts index 080cf4531c9..bc6c00a4712 100644 --- a/extensions/synology-chat/src/channel.test.ts +++ b/extensions/synology-chat/src/channel.test.ts @@ -357,6 +357,33 @@ describe("createSynologyChatPlugin", () => { expect(typeof result.stop).toBe("function"); }); + it("startAccount refuses allowlist accounts with empty allowedUserIds", async () => { + const registerMock = vi.mocked(registerPluginHttpRoute); + registerMock.mockClear(); + + const plugin = createSynologyChatPlugin(); + const ctx = { + cfg: { + channels: { + "synology-chat": { + enabled: true, + token: "t", + incomingUrl: "https://nas/incoming", + dmPolicy: "allowlist", + allowedUserIds: [], + }, + }, + }, + accountId: "default", + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + }; + + const result = await plugin.gateway.startAccount(ctx); + expect(typeof result.stop).toBe("function"); + expect(ctx.log.warn).toHaveBeenCalledWith(expect.stringContaining("empty allowedUserIds")); + expect(registerMock).not.toHaveBeenCalled(); + }); + it("deregisters stale route before re-registering same account/path", async () => { const unregisterFirst = vi.fn(); const unregisterSecond = vi.fn(); @@ -372,6 +399,8 @@ describe("createSynologyChatPlugin", () => { token: "t", incomingUrl: "https://nas/incoming", webhookPath: "/webhook/synology", + dmPolicy: "allowlist", + allowedUserIds: ["123"], }, }, }, diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts index 287028abc24..431dfd2cbd2 100644 --- a/extensions/synology-chat/src/channel.ts +++ b/extensions/synology-chat/src/channel.ts @@ -226,6 +226,12 @@ export function createSynologyChatPlugin() { ); return { stop: () => {} }; } + if (account.dmPolicy === "allowlist" && account.allowedUserIds.length === 0) { + log?.warn?.( + `Synology Chat account ${accountId} has dmPolicy=allowlist but empty allowedUserIds; refusing to start route`, + ); + return { stop: () => {} }; + } log?.info?.( `Starting Synology Chat channel (account: ${accountId}, path: ${account.webhookPath})`, diff --git a/extensions/synology-chat/src/security.test.ts b/extensions/synology-chat/src/security.test.ts index 7e639da9486..c6f697810af 100644 --- a/extensions/synology-chat/src/security.test.ts +++ b/extensions/synology-chat/src/security.test.ts @@ -1,5 +1,11 @@ import { describe, it, expect } from "vitest"; -import { validateToken, checkUserAllowed, sanitizeInput, RateLimiter } from "./security.js"; +import { + validateToken, + checkUserAllowed, + authorizeUserForDm, + sanitizeInput, + RateLimiter, +} from "./security.js"; describe("validateToken", () => { it("returns true for matching tokens", () => { @@ -37,6 +43,39 @@ describe("checkUserAllowed", () => { }); }); +describe("authorizeUserForDm", () => { + it("allows any user when dmPolicy is open", () => { + expect(authorizeUserForDm("user1", "open", [])).toEqual({ allowed: true }); + }); + + it("rejects all users when dmPolicy is disabled", () => { + expect(authorizeUserForDm("user1", "disabled", ["user1"])).toEqual({ + allowed: false, + reason: "disabled", + }); + }); + + it("rejects when dmPolicy is allowlist and list is empty", () => { + expect(authorizeUserForDm("user1", "allowlist", [])).toEqual({ + allowed: false, + reason: "allowlist-empty", + }); + }); + + it("rejects users not in allowlist", () => { + expect(authorizeUserForDm("user9", "allowlist", ["user1"])).toEqual({ + allowed: false, + reason: "not-allowlisted", + }); + }); + + it("allows users in allowlist", () => { + expect(authorizeUserForDm("user1", "allowlist", ["user1", "user2"])).toEqual({ + allowed: true, + }); + }); +}); + describe("sanitizeInput", () => { it("returns normal text unchanged", () => { expect(sanitizeInput("hello world")).toBe("hello world"); diff --git a/extensions/synology-chat/src/security.ts b/extensions/synology-chat/src/security.ts index 12336b876b9..2e1b1431273 100644 --- a/extensions/synology-chat/src/security.ts +++ b/extensions/synology-chat/src/security.ts @@ -4,6 +4,10 @@ import * as crypto from "node:crypto"; +export type DmAuthorizationResult = + | { allowed: true } + | { allowed: false; reason: "disabled" | "allowlist-empty" | "not-allowlisted" }; + /** * Validate webhook token using constant-time comparison. * Prevents timing attacks that could leak token bytes. @@ -28,6 +32,30 @@ export function checkUserAllowed(userId: string, allowedUserIds: string[]): bool return allowedUserIds.includes(userId); } +/** + * Resolve DM authorization for a sender across all DM policy modes. + * Keeps policy semantics in one place so webhook/startup behavior stays consistent. + */ +export function authorizeUserForDm( + userId: string, + dmPolicy: "open" | "allowlist" | "disabled", + allowedUserIds: string[], +): DmAuthorizationResult { + if (dmPolicy === "disabled") { + return { allowed: false, reason: "disabled" }; + } + if (dmPolicy === "open") { + return { allowed: true }; + } + if (allowedUserIds.length === 0) { + return { allowed: false, reason: "allowlist-empty" }; + } + if (!checkUserAllowed(userId, allowedUserIds)) { + return { allowed: false, reason: "not-allowlisted" }; + } + return { allowed: true }; +} + /** * Sanitize user input to prevent prompt injection attacks. * Filters known dangerous patterns and truncates long messages. diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts index 0ed5dd844c5..b077e61fc7c 100644 --- a/extensions/synology-chat/src/webhook-handler.ts +++ b/extensions/synology-chat/src/webhook-handler.ts @@ -6,7 +6,7 @@ import type { IncomingMessage, ServerResponse } from "node:http"; import * as querystring from "node:querystring"; import { sendMessage } from "./client.js"; -import { validateToken, checkUserAllowed, sanitizeInput, RateLimiter } from "./security.js"; +import { validateToken, authorizeUserForDm, sanitizeInput, RateLimiter } from "./security.js"; import type { SynologyWebhookPayload, ResolvedSynologyChatAccount } from "./types.js"; // One rate limiter per account, created lazily @@ -137,25 +137,23 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { return; } - // User allowlist check - if (account.dmPolicy === "disabled") { - respond(res, 403, { error: "DMs are disabled" }); - return; - } - - if (account.dmPolicy === "allowlist") { - if (account.allowedUserIds.length === 0) { + // DM policy authorization + const auth = authorizeUserForDm(payload.user_id, account.dmPolicy, account.allowedUserIds); + if (!auth.allowed) { + if (auth.reason === "disabled") { + respond(res, 403, { error: "DMs are disabled" }); + return; + } + if (auth.reason === "allowlist-empty") { 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; - } + log?.warn(`Unauthorized user: ${payload.user_id}`); + respond(res, 403, { error: "User not authorized" }); + return; } // Rate limit