fix: gate synology chat reply name matching

steipete · steipete · commit 7ade3553b74e · 2026-03-22T23:06:38.000-07:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -88,6 +88,7 @@ Docs: https://docs.openclaw.ai
 - Web tools/Exa: align the bundled Exa plugin with the current Exa API by supporting newer search types and richer `contents` options, while fixing the result-count cap to honor Exa's higher limit. Thanks @vincentkoc.
 - Plugins/Matrix: move bundled plugin `KeyedAsyncQueue` imports onto the stable `plugin-sdk/core` surface so Matrix Docker/runtime builds do not depend on the brittle keyed-async-queue subpath. Thanks @ecohash-co and @vincentkoc.
 - Nostr/security: enforce inbound DM policy before decrypt, route Nostr DMs through the standard reply pipeline, and add pre-crypto rate and size guards so unknown senders cannot bypass pairing or force unbounded crypto work. Thanks @kuranikaran.
+- Synology Chat/security: keep reply delivery bound to stable numeric `user_id` by default, and gate mutable username/nickname recipient lookup behind `dangerouslyAllowNameMatching` with new regression coverage.
 - Agents/default timeout: raise the shared default agent timeout from `600s` to `48h` so long-running ACP and agent sessions do not fail unless you configure a shorter limit.
 - Gateway/Linux: auto-detect nvm-managed Node TLS CA bundle needs before CLI startup and refresh installed services that are missing `NODE_EXTRA_CA_CERTS`. (#51146) Thanks @GodsBoy.
 - Android/pairing: resolve portless secure setup URLs to `443` while preserving direct cleartext gateway defaults and explicit `:80` manual endpoints in onboarding. (#43540) Thanks @fmercurio.
diff --git a/docs/channels/synology-chat.md b/docs/channels/synology-chat.md
@@ -79,6 +79,7 @@ Config values override env vars.
 - 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.
+- Reply recipient binding stays on stable numeric `user_id` by default. `channels.synology-chat.dangerouslyAllowNameMatching: true` is break-glass compatibility mode that re-enables mutable username/nickname lookup for reply delivery.
 - Pairing approvals work with:
   - `openclaw pairing list synology-chat`
   - `openclaw pairing approve synology-chat <CODE>`
@@ -132,3 +133,4 @@ on two different Synology accounts does not share transcript state.
 - Keep `allowInsecureSsl: false` unless you explicitly trust a self-signed local NAS cert.
 - Inbound webhook requests are token-verified and rate-limited per sender.
 - Prefer `dmPolicy: "allowlist"` for production.
+- Keep `dangerouslyAllowNameMatching` off unless you explicitly need legacy username-based reply delivery.
diff --git a/docs/gateway/security/index.md b/docs/gateway/security/index.md
@@ -313,6 +313,8 @@ schema:
 - `channels.googlechat.dangerouslyAllowNameMatching`
 - `channels.googlechat.accounts.<accountId>.dangerouslyAllowNameMatching`
 - `channels.msteams.dangerouslyAllowNameMatching`
+- `channels.synology-chat.dangerouslyAllowNameMatching` (extension channel)
+- `channels.synology-chat.accounts.<accountId>.dangerouslyAllowNameMatching` (extension channel)
 - `channels.zalouser.dangerouslyAllowNameMatching` (extension channel)
 - `channels.irc.dangerouslyAllowNameMatching` (extension channel)
 - `channels.irc.accounts.<accountId>.dangerouslyAllowNameMatching` (extension channel)
diff --git a/extensions/synology-chat/src/accounts.test.ts b/extensions/synology-chat/src/accounts.test.ts
@@ -66,6 +66,7 @@ describe("resolveAccount", () => {
     expect(account.accountId).toBe("default");
     expect(account.enabled).toBe(true);
     expect(account.webhookPath).toBe("/webhook/synology");
+    expect(account.dangerouslyAllowNameMatching).toBe(false);
     expect(account.dmPolicy).toBe("allowlist");
     expect(account.rateLimitPerMinute).toBe(30);
     expect(account.botName).toBe("OpenClaw");
@@ -100,15 +101,56 @@ describe("resolveAccount", () => {
         "synology-chat": {
           token: "base-tok",
           botName: "BaseName",
+          dangerouslyAllowNameMatching: false,
           accounts: {
-            work: { token: "work-tok", botName: "WorkBot" },
+            work: {
+              token: "work-tok",
+              botName: "WorkBot",
+              dangerouslyAllowNameMatching: true,
+            },
           },
         },
       },
     };
     const account = resolveAccount(cfg, "work");
     expect(account.token).toBe("work-tok");
     expect(account.botName).toBe("WorkBot");
+    expect(account.dangerouslyAllowNameMatching).toBe(true);
+  });
+
+  it("inherits dangerous name matching from base config when not overridden", () => {
+    const cfg = {
+      channels: {
+        "synology-chat": {
+          dangerouslyAllowNameMatching: true,
+          accounts: {
+            work: { token: "work-tok" },
+          },
+        },
+      },
+    };
+
+    const account = resolveAccount(cfg, "work");
+    expect(account.dangerouslyAllowNameMatching).toBe(true);
+  });
+
+  it("allows a named account to disable inherited dangerous name matching", () => {
+    const cfg = {
+      channels: {
+        "synology-chat": {
+          dangerouslyAllowNameMatching: true,
+          accounts: {
+            work: {
+              token: "work-tok",
+              dangerouslyAllowNameMatching: false,
+            },
+          },
+        },
+      },
+    };
+
+    const account = resolveAccount(cfg, "work");
+    expect(account.dangerouslyAllowNameMatching).toBe(false);
   });
 
   it("parses comma-separated allowedUserIds string", () => {
diff --git a/extensions/synology-chat/src/accounts.ts b/extensions/synology-chat/src/accounts.ts
@@ -89,6 +89,7 @@ export function resolveAccount(
     incomingUrl: merged.incomingUrl ?? envIncomingUrl,
     nasHost: merged.nasHost ?? envNasHost,
     webhookPath: merged.webhookPath ?? "/webhook/synology",
+    dangerouslyAllowNameMatching: merged.dangerouslyAllowNameMatching ?? false,
     dmPolicy: merged.dmPolicy ?? "allowlist",
     allowedUserIds: parseAllowedUserIds(merged.allowedUserIds ?? envAllowedUserIds),
     rateLimitPerMinute: merged.rateLimitPerMinute ?? envRateLimitValue,
diff --git a/extensions/synology-chat/src/channel.test-mocks.ts b/extensions/synology-chat/src/channel.test-mocks.ts
@@ -101,6 +101,7 @@ export function makeSecurityAccount(overrides: Record<string, unknown> = {}) {
     incomingUrl: "https://nas/incoming",
     nasHost: "h",
     webhookPath: "/w",
+    dangerouslyAllowNameMatching: false,
     dmPolicy: "allowlist" as const,
     allowedUserIds: [],
     rateLimitPerMinute: 30,
diff --git a/extensions/synology-chat/src/channel.test.ts b/extensions/synology-chat/src/channel.test.ts
@@ -107,6 +107,7 @@ describe("createSynologyChatPlugin", () => {
         incomingUrl: "u",
         nasHost: "h",
         webhookPath: "/w",
+        dangerouslyAllowNameMatching: false,
         dmPolicy: "allowlist" as const,
         allowedUserIds: ["user1"],
         rateLimitPerMinute: 30,
@@ -171,6 +172,13 @@ describe("createSynologyChatPlugin", () => {
       expect(warnings.some((w: string) => w.includes("SSL"))).toBe(true);
     });
 
+    it("warns when dangerous name matching is enabled", () => {
+      const plugin = createSynologyChatPlugin();
+      const account = makeSecurityAccount({ dangerouslyAllowNameMatching: true });
+      const warnings = plugin.security.collectWarnings({ account });
+      expect(warnings.some((w: string) => w.includes("dangerouslyAllowNameMatching"))).toBe(true);
+    });
+
     it("warns when dmPolicy is open", () => {
       const plugin = createSynologyChatPlugin();
       const account = makeSecurityAccount({ dmPolicy: "open" });
diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts
@@ -29,7 +29,13 @@ import { synologyChatSetupAdapter, synologyChatSetupWizard } from "./setup-surfa
 import type { ResolvedSynologyChatAccount } from "./types.js";
 
 const CHANNEL_ID = "synology-chat";
-const SynologyChatConfigSchema = buildChannelConfigSchema(z.object({}).passthrough());
+const SynologyChatConfigSchema = buildChannelConfigSchema(
+  z
+    .object({
+      dangerouslyAllowNameMatching: z.boolean().optional(),
+    })
+    .passthrough(),
+);
 
 const resolveSynologyChatDmPolicy = createScopedDmSecurityResolver<ResolvedSynologyChatAccount>({
   channelKey: CHANNEL_ID,
@@ -51,6 +57,7 @@ const synologyChatConfigAdapter = createHybridChannelConfigAdapter<ResolvedSynol
     "incomingUrl",
     "nasHost",
     "webhookPath",
+    "dangerouslyAllowNameMatching",
     "dmPolicy",
     "allowedUserIds",
     "rateLimitPerMinute",
@@ -73,6 +80,9 @@ const collectSynologyChatSecurityWarnings =
     (account) =>
       account.allowInsecureSsl &&
       "- Synology Chat: SSL verification is disabled (allowInsecureSsl=true). Only use this for local NAS with self-signed certificates.",
+    (account) =>
+      account.dangerouslyAllowNameMatching &&
+      "- Synology Chat: dangerouslyAllowNameMatching=true re-enables mutable username/nickname recipient matching for replies. Prefer stable numeric user IDs.",
     (account) =>
       account.dmPolicy === "open" &&
       '- Synology Chat: dmPolicy="open" allows any user to message the bot. Consider "allowlist" for production use.',
diff --git a/extensions/synology-chat/src/config-schema.test.ts b/extensions/synology-chat/src/config-schema.test.ts
@@ -0,0 +1,17 @@
+import { describe, expect, it } from "vitest";
+import { SynologyChatChannelConfigSchema } from "./config-schema.js";
+
+describe("SynologyChatChannelConfigSchema", () => {
+  it("exports dangerouslyAllowNameMatching in the JSON schema", () => {
+    const properties = (SynologyChatChannelConfigSchema.schema.properties ?? {}) as Record<
+      string,
+      { type?: string }
+    >;
+
+    expect(properties.dangerouslyAllowNameMatching?.type).toBe("boolean");
+  });
+
+  it("keeps the schema open for plugin-specific passthrough fields", () => {
+    expect(SynologyChatChannelConfigSchema.schema.additionalProperties).toBe(true);
+  });
+});
diff --git a/extensions/synology-chat/src/config-schema.ts b/extensions/synology-chat/src/config-schema.ts
@@ -1,4 +1,10 @@
 import { buildChannelConfigSchema } from "openclaw/plugin-sdk/channel-config-schema";
 import { z } from "zod";
 
-export const SynologyChatChannelConfigSchema = buildChannelConfigSchema(z.object({}).passthrough());
+export const SynologyChatChannelConfigSchema = buildChannelConfigSchema(
+  z
+    .object({
+      dangerouslyAllowNameMatching: z.boolean().optional(),
+    })
+    .passthrough(),
+);
diff --git a/extensions/synology-chat/src/types.ts b/extensions/synology-chat/src/types.ts
@@ -8,6 +8,7 @@ type SynologyChatConfigFields = {
   incomingUrl?: string;
   nasHost?: string;
   webhookPath?: string;
+  dangerouslyAllowNameMatching?: boolean;
   dmPolicy?: "open" | "allowlist" | "disabled";
   allowedUserIds?: string | string[];
   rateLimitPerMinute?: number;
@@ -31,6 +32,7 @@ export interface ResolvedSynologyChatAccount {
   incomingUrl: string;
   nasHost: string;
   webhookPath: string;
+  dangerouslyAllowNameMatching: boolean;
   dmPolicy: "open" | "allowlist" | "disabled";
   allowedUserIds: string[];
   rateLimitPerMinute: number;
diff --git a/extensions/synology-chat/src/webhook-handler.test.ts b/extensions/synology-chat/src/webhook-handler.test.ts
@@ -1,4 +1,5 @@
 import { describe, it, expect, vi, beforeEach } from "vitest";
+import { resolveChatUserId, sendMessage } from "./client.js";
 import { makeFormBody, makeReq, makeRes, makeStalledReq } from "./test-http-utils.js";
 import type { ResolvedSynologyChatAccount } from "./types.js";
 import type { WebhookHandlerDeps } from "./webhook-handler.js";
@@ -23,6 +24,7 @@ function makeAccount(
     incomingUrl: "https://nas.example.com/incoming",
     nasHost: "nas.example.com",
     webhookPath: "/webhook/synology",
+    dangerouslyAllowNameMatching: false,
     dmPolicy: "open",
     allowedUserIds: [],
     rateLimitPerMinute: 30,
@@ -327,6 +329,111 @@ describe("createWebhookHandler", () => {
     );
   });
 
+  it("keeps replies bound to payload.user_id by default", async () => {
+    const deliver = vi.fn().mockResolvedValue("Bot reply");
+    const handler = createWebhookHandler({
+      account: makeAccount({ accountId: "stable-id-test-" + Date.now() }),
+      deliver,
+      log,
+    });
+
+    const req = makeReq("POST", validBody);
+    const res = makeRes();
+    await handler(req, res);
+
+    expect(res._status).toBe(204);
+    expect(resolveChatUserId).not.toHaveBeenCalled();
+    expect(deliver).toHaveBeenCalledWith(
+      expect.objectContaining({
+        from: "123",
+        chatUserId: "123",
+      }),
+    );
+    expect(sendMessage).toHaveBeenCalledWith(
+      "https://nas.example.com/incoming",
+      "Bot reply",
+      "123",
+      true,
+    );
+  });
+
+  it("only resolves reply recipient by username when break-glass mode is enabled", async () => {
+    vi.mocked(resolveChatUserId).mockResolvedValueOnce(456);
+    const deliver = vi.fn().mockResolvedValue("Bot reply");
+    const handler = createWebhookHandler({
+      account: makeAccount({
+        accountId: "dangerous-name-match-test-" + Date.now(),
+        dangerouslyAllowNameMatching: true,
+      }),
+      deliver,
+      log,
+    });
+
+    const req = makeReq("POST", validBody);
+    const res = makeRes();
+    await handler(req, res);
+
+    expect(res._status).toBe(204);
+    expect(resolveChatUserId).toHaveBeenCalledWith(
+      "https://nas.example.com/incoming",
+      "testuser",
+      true,
+      log,
+    );
+    expect(deliver).toHaveBeenCalledWith(
+      expect.objectContaining({
+        from: "123",
+        chatUserId: "456",
+      }),
+    );
+    expect(sendMessage).toHaveBeenCalledWith(
+      "https://nas.example.com/incoming",
+      "Bot reply",
+      "456",
+      true,
+    );
+  });
+
+  it("falls back to payload.user_id when break-glass resolution does not find a match", async () => {
+    vi.mocked(resolveChatUserId).mockResolvedValueOnce(undefined);
+    const deliver = vi.fn().mockResolvedValue("Bot reply");
+    const handler = createWebhookHandler({
+      account: makeAccount({
+        accountId: "dangerous-name-fallback-test-" + Date.now(),
+        dangerouslyAllowNameMatching: true,
+      }),
+      deliver,
+      log,
+    });
+
+    const req = makeReq("POST", validBody);
+    const res = makeRes();
+    await handler(req, res);
+
+    expect(res._status).toBe(204);
+    expect(resolveChatUserId).toHaveBeenCalledWith(
+      "https://nas.example.com/incoming",
+      "testuser",
+      true,
+      log,
+    );
+    expect(log.warn).toHaveBeenCalledWith(
+      'Could not resolve Chat API user_id for "testuser" — falling back to webhook user_id 123. Reply delivery may fail.',
+    );
+    expect(deliver).toHaveBeenCalledWith(
+      expect.objectContaining({
+        from: "123",
+        chatUserId: "123",
+      }),
+    );
+    expect(sendMessage).toHaveBeenCalledWith(
+      "https://nas.example.com/incoming",
+      "Bot reply",
+      "123",
+      true,
+    );
+  });
+
   it("sanitizes input before delivery", async () => {
     const deliver = vi.fn().mockResolvedValue(null);
     const handler = createWebhookHandler({
diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts
@@ -374,6 +374,10 @@ async function resolveSynologyReplyUserId(params: {
   payload: SynologyWebhookPayload;
   log?: WebhookHandlerDeps["log"];
 }): Promise<string> {
+  if (!params.account.dangerouslyAllowNameMatching) {
+    return params.payload.user_id;
+  }
+
   const chatUserId = await resolveChatUserId(
     params.account.incomingUrl,
     params.payload.username,
