refactor(synology-chat): centralize DM auth and fail fast startup

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:

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<void>;
+};
+
+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<string, string>) {
+      res._status = statusCode;
+    },
+    end(body?: string) {
+      res._body = body ?? "";
+    },
+  } as any;
+  return res;
+}
+
+function makeFormBody(fields: Record<string, string>): 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();
+  });
+});

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"],
             },
           },
         },

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})`,

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");

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.

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
+    // DM policy authorization
-    if (account.dmPolicy === "disabled") {
+    const auth = authorizeUserForDm(payload.user_id, account.dmPolicy, account.allowedUserIds);
-      respond(res, 403, { error: "DMs are disabled" });
+    if (!auth.allowed) {
-      return;
+      if (auth.reason === "disabled") {
-    }
+        respond(res, 403, { error: "DMs are disabled" });
-
+        return;
-    if (account.dmPolicy === "allowlist") {
+      }
-      if (account.allowedUserIds.length === 0) {
+      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}`);
-        log?.warn(`Unauthorized user: ${payload.user_id}`);
+      respond(res, 403, { error: "User not authorized" });
-        respond(res, 403, { error: "User not authorized" });
+      return;
-        return;
-      }
     }
 
     // Rate limit