mirror of
https://github.com/openclaw/openclaw.git
synced 2026-03-24 00:11:31 +00:00
fix(synology-chat): fail closed shared webhook paths
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
{
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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<Record<string, unknown> & SynologyChatChannelConfig>({
|
||||
channelConfig: channelCfg as Record<string, unknown> & 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,
|
||||
|
||||
@@ -102,6 +102,8 @@ export function makeSecurityAccount(overrides: Record<string, unknown> = {}) {
|
||||
nasHost: "h",
|
||||
webhookPath: "/w",
|
||||
dangerouslyAllowNameMatching: false,
|
||||
hasExplicitWebhookPath: true,
|
||||
dangerouslyAllowInheritedWebhookPath: false,
|
||||
dmPolicy: "allowlist" as const,
|
||||
allowedUserIds: [],
|
||||
rateLimitPerMinute: 30,
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -25,6 +25,8 @@ function makeAccount(
|
||||
nasHost: "nas.example.com",
|
||||
webhookPath: "/webhook/synology",
|
||||
dangerouslyAllowNameMatching: false,
|
||||
hasExplicitWebhookPath: true,
|
||||
dangerouslyAllowInheritedWebhookPath: false,
|
||||
dmPolicy: "open",
|
||||
allowedUserIds: [],
|
||||
rateLimitPerMinute: 30,
|
||||
|
||||
Reference in New Issue
Block a user