From 980940aa58f862da4e19372597bbc2a9f268d70b Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 22 Mar 2026 23:31:48 -0700 Subject: [PATCH] fix(synology-chat): fail closed shared webhook paths --- CHANGELOG.md | 2 + docs/channels/synology-chat.md | 5 ++ extensions/synology-chat/src/accounts.test.ts | 37 +++++++++ extensions/synology-chat/src/accounts.ts | 22 +++++ .../synology-chat/src/channel.test-mocks.ts | 2 + extensions/synology-chat/src/channel.test.ts | 82 +++++++++++++++++++ extensions/synology-chat/src/channel.ts | 2 +- .../synology-chat/src/gateway-runtime.ts | 42 +++++++++- extensions/synology-chat/src/types.ts | 3 + .../synology-chat/src/webhook-handler.test.ts | 2 + 10 files changed, 196 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 16155d9c587..26014ebf92d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -112,6 +112,8 @@ Docs: https://docs.openclaw.ai - Mattermost/threading: honor `replyToMode: "off"` for already-threaded inbound posts so threaded follow-ups can fall back to top-level replies when configured. (#52543) Thanks @RichardCao. - Security/exec approvals: escape blank Hangul filler code points in approval prompts across gateway/chat and the macOS native approval UI so visually empty Unicode padding cannot hide reviewed command text. - Security/network: harden explicit-proxy SSRF pinning by translating target-hop transport hints onto HTTPS proxy tunnels and failing closed for plain HTTP guarded fetches that cannot preserve pinned DNS. +- Security/Synology Chat: require explicit per-account webhook paths for multi-account setups by default, reject duplicate exact webhook paths fail-closed, and keep inherited-path behavior behind an explicit dangerous opt-in so shared routes can no longer collapse DM policy contexts across accounts. Thanks @tdjackey for reporting. +- Security/network: harden explicit-proxy SSRF pinning by translating target-hop transport hints onto HTTPS proxy tunnels and failing closed for plain HTTP guarded fetches that cannot preserve pinned DNS. - Telegram/replies: set `allow_sending_without_reply` on reply-targeted sends and media-error notices so deleted parent messages no longer drop otherwise valid replies. (#52524) Thanks @moltbot886. - Gateway/status: resolve env-backed `gateway.auth.*` SecretRefs before read-only probe auth checks so status no longer reports false probe failures when auth is configured through SecretRef. (#52513) Thanks @CodeForgeNet. - Agents/exec: return plain-text failed tool output for timeouts and other non-success exec outcomes so models no longer parrot raw JSON error payloads back to users. (#52508) Thanks @martingarramon. diff --git a/docs/channels/synology-chat.md b/docs/channels/synology-chat.md index 21b3245f866..3c711f97458 100644 --- a/docs/channels/synology-chat.md +++ b/docs/channels/synology-chat.md @@ -103,6 +103,11 @@ Multiple Synology Chat accounts are supported under `channels.synology-chat.acco Each account can override token, incoming URL, webhook path, DM policy, and limits. Direct-message sessions are isolated per account and user, so the same numeric `user_id` on two different Synology accounts does not share transcript state. +Give each enabled account a distinct `webhookPath`. OpenClaw now rejects duplicate exact paths +and refuses to start named accounts that only inherit a shared webhook path in multi-account setups. +If you need legacy inheritance for a named account, set +`dangerouslyAllowInheritedWebhookPath: true` on that account or at `channels.synology-chat`, +but duplicate exact paths are still rejected fail-closed. ```json5 { diff --git a/extensions/synology-chat/src/accounts.test.ts b/extensions/synology-chat/src/accounts.test.ts index 9428f69bffe..e978dd2e7f9 100644 --- a/extensions/synology-chat/src/accounts.test.ts +++ b/extensions/synology-chat/src/accounts.test.ts @@ -153,6 +153,43 @@ describe("resolveAccount", () => { expect(account.dangerouslyAllowNameMatching).toBe(false); }); + it("marks named multi-account webhookPath inheritance as dangerous-off by default", () => { + const cfg = { + channels: { + "synology-chat": { + token: "base-tok", + webhookPath: "/webhook/shared", + accounts: { + work: { token: "work-tok" }, + }, + }, + }, + }; + const account = resolveAccount(cfg, "work"); + expect(account.webhookPath).toBe("/webhook/shared"); + expect(account.hasExplicitWebhookPath).toBe(false); + expect(account.dangerouslyAllowInheritedWebhookPath).toBe(false); + }); + + it("allows named accounts to opt into inherited webhookPath resolution", () => { + const cfg = { + channels: { + "synology-chat": { + token: "base-tok", + webhookPath: "/webhook/shared", + dangerouslyAllowInheritedWebhookPath: true, + accounts: { + work: { token: "work-tok" }, + }, + }, + }, + }; + const account = resolveAccount(cfg, "work"); + expect(account.webhookPath).toBe("/webhook/shared"); + expect(account.hasExplicitWebhookPath).toBe(false); + expect(account.dangerouslyAllowInheritedWebhookPath).toBe(true); + }); + it("parses comma-separated allowedUserIds string", () => { const cfg = { channels: { diff --git a/extensions/synology-chat/src/accounts.ts b/extensions/synology-chat/src/accounts.ts index 0b0d1932e9e..4d2ae736ff3 100644 --- a/extensions/synology-chat/src/accounts.ts +++ b/extensions/synology-chat/src/accounts.ts @@ -17,6 +17,20 @@ function getChannelConfig(cfg: OpenClawConfig): SynologyChatChannelConfig | unde return cfg?.channels?.["synology-chat"]; } +function getRawAccountConfig( + channelCfg: SynologyChatChannelConfig, + accountId: string, +): SynologyChatChannelConfig { + if (accountId === DEFAULT_ACCOUNT_ID) { + return channelCfg; + } + return channelCfg.accounts?.[accountId] ?? {}; +} + +function hasExplicitWebhookPath(rawAccount: SynologyChatChannelConfig | undefined): boolean { + return typeof rawAccount?.webhookPath === "string" && rawAccount.webhookPath.trim().length > 0; +} + /** Parse allowedUserIds from string or array to string[]. */ function parseAllowedUserIds(raw: string | string[] | undefined): string[] { if (!raw) return []; @@ -68,6 +82,7 @@ export function resolveAccount( const id = accountId || DEFAULT_ACCOUNT_ID; const accountOverrides = id === DEFAULT_ACCOUNT_ID ? undefined : (channelCfg.accounts?.[id] ?? undefined); + const rawAccount = getRawAccountConfig(channelCfg, id); const merged = resolveMergedAccountConfig & SynologyChatChannelConfig>({ channelConfig: channelCfg as Record & SynologyChatChannelConfig, accounts: channelCfg.accounts as @@ -83,6 +98,11 @@ export function resolveAccount( const envAllowedUserIds = process.env.SYNOLOGY_ALLOWED_USER_IDS ?? ""; const envRateLimitValue = parseRateLimitPerMinute(process.env.SYNOLOGY_RATE_LIMIT); const envBotName = process.env.OPENCLAW_BOT_NAME ?? "OpenClaw"; + const explicitWebhookPath = hasExplicitWebhookPath(rawAccount); + const allowInheritedWebhookPath = + rawAccount.dangerouslyAllowInheritedWebhookPath ?? + channelCfg.dangerouslyAllowInheritedWebhookPath ?? + false; // Merge: account override > base channel config > env var return { @@ -96,6 +116,8 @@ export function resolveAccount( providerConfig: channelCfg, accountConfig: accountOverrides, }), + hasExplicitWebhookPath: explicitWebhookPath, + dangerouslyAllowInheritedWebhookPath: allowInheritedWebhookPath, 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 index 5d103291bdc..c6b04d376df 100644 --- a/extensions/synology-chat/src/channel.test-mocks.ts +++ b/extensions/synology-chat/src/channel.test-mocks.ts @@ -102,6 +102,8 @@ export function makeSecurityAccount(overrides: Record = {}) { nasHost: "h", webhookPath: "/w", dangerouslyAllowNameMatching: false, + hasExplicitWebhookPath: true, + dangerouslyAllowInheritedWebhookPath: 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 index 9eda9a29484..80a68a8d0ef 100644 --- a/extensions/synology-chat/src/channel.test.ts +++ b/extensions/synology-chat/src/channel.test.ts @@ -12,6 +12,7 @@ const mockSendMessage = vi.mocked(sendMessage); describe("createSynologyChatPlugin", () => { beforeEach(() => { mockSendMessage.mockClear(); + registerPluginHttpRouteMock.mockClear(); }); describe("meta", () => { @@ -108,6 +109,8 @@ describe("createSynologyChatPlugin", () => { nasHost: "h", webhookPath: "/w", dangerouslyAllowNameMatching: false, + hasExplicitWebhookPath: true, + dangerouslyAllowInheritedWebhookPath: false, dmPolicy: "allowlist" as const, allowedUserIds: ["user1"], rateLimitPerMinute: 30, @@ -358,6 +361,85 @@ describe("createSynologyChatPlugin", () => { expect(registerMock).not.toHaveBeenCalled(); }); + it("startAccount refuses named accounts without explicit webhookPath in multi-account setups", async () => { + const registerMock = registerPluginHttpRouteMock; + const plugin = createSynologyChatPlugin(); + const abortController = new AbortController(); + const ctx = { + cfg: { + channels: { + "synology-chat": { + enabled: true, + token: "shared-token", + incomingUrl: "https://nas/incoming", + webhookPath: "/webhook/synology-shared", + accounts: { + alerts: { + enabled: true, + token: "alerts-token", + incomingUrl: "https://nas/alerts", + dmPolicy: "allowlist", + allowedUserIds: ["123"], + }, + }, + }, + }, + }, + accountId: "alerts", + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + abortSignal: abortController.signal, + }; + + const result = plugin.gateway.startAccount(ctx); + await expectPendingStartAccountPromise(result, abortController); + expect(ctx.log.warn).toHaveBeenCalledWith( + expect.stringContaining("must set an explicit webhookPath"), + ); + expect(registerMock).not.toHaveBeenCalled(); + }); + + it("startAccount refuses duplicate exact webhook paths across accounts", async () => { + const registerMock = registerPluginHttpRouteMock; + const plugin = createSynologyChatPlugin(); + const abortController = new AbortController(); + const ctx = { + cfg: { + channels: { + "synology-chat": { + enabled: true, + accounts: { + default: { + enabled: true, + token: "default-token", + incomingUrl: "https://nas/default", + webhookPath: "/webhook/synology-shared", + dmPolicy: "allowlist", + allowedUserIds: ["123"], + }, + alerts: { + enabled: true, + token: "alerts-token", + incomingUrl: "https://nas/alerts", + webhookPath: "/webhook/synology-shared", + dmPolicy: "open", + }, + }, + }, + }, + }, + accountId: "alerts", + log: { info: vi.fn(), warn: vi.fn(), error: vi.fn() }, + abortSignal: abortController.signal, + }; + + const result = plugin.gateway.startAccount(ctx); + await expectPendingStartAccountPromise(result, abortController); + expect(ctx.log.warn).toHaveBeenCalledWith( + expect.stringContaining("conflicts on webhookPath"), + ); + expect(registerMock).not.toHaveBeenCalled(); + }); + it("deregisters stale route before re-registering same account/path", async () => { const unregisterFirst = vi.fn(); const unregisterSecond = vi.fn(); diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts index 9b495df8ea4..9f77efa9aab 100644 --- a/extensions/synology-chat/src/channel.ts +++ b/extensions/synology-chat/src/channel.ts @@ -200,7 +200,7 @@ export function createSynologyChatPlugin(): SynologyChatPlugin { startAccount: async (ctx: any) => { const { cfg, accountId, log } = ctx; const account = resolveAccount(cfg, accountId); - if (!validateSynologyGatewayAccountStartup({ account, accountId, log }).ok) { + if (!validateSynologyGatewayAccountStartup({ cfg, account, accountId, log }).ok) { return waitUntilAbort(ctx.abortSignal); } diff --git a/extensions/synology-chat/src/gateway-runtime.ts b/extensions/synology-chat/src/gateway-runtime.ts index dd887d50d8a..2758b9f0f4b 100644 --- a/extensions/synology-chat/src/gateway-runtime.ts +++ b/extensions/synology-chat/src/gateway-runtime.ts @@ -1,4 +1,10 @@ +import { + DEFAULT_ACCOUNT_ID, + listCombinedAccountIds, + type OpenClawConfig, +} from "openclaw/plugin-sdk/account-resolution"; import { registerPluginHttpRoute } from "openclaw/plugin-sdk/webhook-ingress"; +import { resolveAccount } from "./accounts.js"; import { dispatchSynologyChatInboundTurn } from "./inbound-turn.js"; import type { ResolvedSynologyChatAccount } from "./types.js"; import { createWebhookHandler, type WebhookHandlerDeps } from "./webhook-handler.js"; @@ -27,11 +33,12 @@ export function waitUntilAbort(signal?: AbortSignal, onAbort?: () => void): Prom } export function validateSynologyGatewayAccountStartup(params: { + cfg: OpenClawConfig; account: ResolvedSynologyChatAccount; accountId: string; log?: SynologyGatewayLog; }): { ok: true } | { ok: false } { - const { accountId, account, log } = params; + const { cfg, accountId, account, log } = params; if (!account.enabled) { log?.info?.(`Synology Chat account ${accountId} is disabled, skipping`); return { ok: false }; @@ -48,6 +55,38 @@ export function validateSynologyGatewayAccountStartup(params: { ); return { ok: false }; } + const accountIds = listCombinedAccountIds({ + configuredAccountIds: Object.keys(cfg.channels?.["synology-chat"]?.accounts ?? {}), + implicitAccountId: + cfg.channels?.["synology-chat"]?.token || process.env.SYNOLOGY_CHAT_TOKEN + ? DEFAULT_ACCOUNT_ID + : undefined, + }); + const isMultiAccount = accountIds.length > 1; + if ( + isMultiAccount && + accountId !== DEFAULT_ACCOUNT_ID && + !account.hasExplicitWebhookPath && + !account.dangerouslyAllowInheritedWebhookPath + ) { + log?.warn?.( + `Synology Chat account ${accountId} must set an explicit webhookPath in multi-account setups; refusing inherited shared path. Set channels.synology-chat.accounts.${accountId}.webhookPath or opt in with dangerouslyAllowInheritedWebhookPath=true.`, + ); + return { ok: false }; + } + const conflictingAccounts = accountIds.filter((candidateId) => { + if (candidateId === accountId) { + return false; + } + const candidate = resolveAccount(cfg, candidateId); + return candidate.enabled && candidate.webhookPath === account.webhookPath; + }); + if (conflictingAccounts.length > 0) { + log?.warn?.( + `Synology Chat account ${accountId} conflicts on webhookPath ${account.webhookPath} with ${conflictingAccounts.join(", ")}; refusing to start ambiguous shared route.`, + ); + return { ok: false }; + } return { ok: true }; } @@ -73,7 +112,6 @@ export function registerSynologyWebhookRoute(params: { const unregister = registerPluginHttpRoute({ path: account.webhookPath, auth: "plugin", - replaceExisting: true, pluginId: CHANNEL_ID, accountId: account.accountId, log: (msg: string) => log?.info?.(msg), diff --git a/extensions/synology-chat/src/types.ts b/extensions/synology-chat/src/types.ts index 154acf99e9e..297953e541c 100644 --- a/extensions/synology-chat/src/types.ts +++ b/extensions/synology-chat/src/types.ts @@ -9,6 +9,7 @@ type SynologyChatConfigFields = { nasHost?: string; webhookPath?: string; dangerouslyAllowNameMatching?: boolean; + dangerouslyAllowInheritedWebhookPath?: boolean; dmPolicy?: "open" | "allowlist" | "disabled"; allowedUserIds?: string | string[]; rateLimitPerMinute?: number; @@ -33,6 +34,8 @@ export interface ResolvedSynologyChatAccount { nasHost: string; webhookPath: string; dangerouslyAllowNameMatching: boolean; + hasExplicitWebhookPath: boolean; + dangerouslyAllowInheritedWebhookPath: 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 index c5b67e0e3c4..81a6e3540d5 100644 --- a/extensions/synology-chat/src/webhook-handler.test.ts +++ b/extensions/synology-chat/src/webhook-handler.test.ts @@ -25,6 +25,8 @@ function makeAccount( nasHost: "nas.example.com", webhookPath: "/webhook/synology", dangerouslyAllowNameMatching: false, + hasExplicitWebhookPath: true, + dangerouslyAllowInheritedWebhookPath: false, dmPolicy: "open", allowedUserIds: [], rateLimitPerMinute: 30,