mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 06:00:43 +00:00
fix(synology): validate webhook file urls (#69784)
* fix(synology): validate webhook file urls * fix(synology): restore file send throttle * docs(changelog): note synology webhook file_url SSRF guard (#69784) --------- Co-authored-by: Devin Robison <drobison@nvidia.com>
This commit is contained in:
@@ -18,6 +18,7 @@ Docs: https://docs.openclaw.ai
|
||||
- Auth/commands: require owner identity (an owner-candidate match or internal `operator.admin`) for owner-enforced commands instead of treating wildcard channel `allowFrom` or empty owner-candidate lists as sufficient, so non-owner senders can no longer reach owner-only commands through a permissive fallback when `enforceOwnerForCommands=true` and `commands.ownerAllowFrom` is unset. (#69774) Thanks @drobison00.
|
||||
- Control UI/CSP: tighten `img-src` to `'self' data:` only, and make Control UI avatar helpers drop remote `http(s)` and protocol-relative URLs so the UI falls back to the built-in logo/badge instead of issuing arbitrary remote image fetches. Same-origin avatar routes (relative paths) and `data:image/...` avatars still render. (#69773)
|
||||
- CLI/channels: keep `status`, `health`, `channels list`, and `channels status` on read-only channel metadata when Telegram, Slack, Discord, or third-party channel plugins are configured, avoiding full bundled plugin runtime imports on those cold paths. Fixes #69042. (#69479) Thanks @gumadeiras.
|
||||
- Synology Chat: validate outbound webhook `file_url` values against the shared SSRF policy before forwarding to the NAS, rejecting malformed URLs, non-`http(s)` schemes, and private/blocked network targets so the NAS cannot be used as a confused deputy to fetch internal addresses. (#69784) Thanks @eleqtrizit.
|
||||
|
||||
## 2026.4.20
|
||||
|
||||
|
||||
@@ -113,6 +113,7 @@ openclaw message send --channel synology-chat --target synology-chat:123456 --te
|
||||
```
|
||||
|
||||
Media sends are supported by URL-based file delivery.
|
||||
Outbound file URLs must use `http` or `https`, and private or otherwise blocked network targets are rejected before OpenClaw forwards the URL to the NAS webhook.
|
||||
|
||||
## Multi-account
|
||||
|
||||
|
||||
@@ -2,6 +2,10 @@ import { EventEmitter } from "node:events";
|
||||
import type { ClientRequest, IncomingMessage, RequestOptions } from "node:http";
|
||||
import { describe, it, expect, vi, beforeAll, beforeEach, afterEach } from "vitest";
|
||||
|
||||
const ssrfMocks = {
|
||||
resolvePinnedHostnameWithPolicy: vi.fn(),
|
||||
};
|
||||
|
||||
// Mock http and https modules before importing the client
|
||||
vi.mock("node:https", () => {
|
||||
const httpsRequest = vi.fn();
|
||||
@@ -16,6 +20,11 @@ vi.mock("node:http", () => {
|
||||
return { default: { request: httpRequest, get: httpGet }, request: httpRequest, get: httpGet };
|
||||
});
|
||||
|
||||
vi.mock("openclaw/plugin-sdk/ssrf-runtime", () => ({
|
||||
formatErrorMessage: (err: unknown) => (err instanceof Error ? err.message : String(err)),
|
||||
resolvePinnedHostnameWithPolicy: ssrfMocks.resolvePinnedHostnameWithPolicy,
|
||||
}));
|
||||
|
||||
const https = await import("node:https");
|
||||
let fakeNowMs = 1_700_000_000_000;
|
||||
let sendMessage: typeof import("./client.js").sendMessage;
|
||||
@@ -33,7 +42,7 @@ type MockRequestHandler = (
|
||||
function createMockResponseEmitter(statusCode: number): IncomingMessage {
|
||||
const res = new EventEmitter() as Partial<IncomingMessage>;
|
||||
res.statusCode = statusCode;
|
||||
return res as IncomingMessage;
|
||||
return res as unknown as IncomingMessage;
|
||||
}
|
||||
|
||||
function createMockRequestEmitter(): ClientRequest {
|
||||
@@ -41,7 +50,7 @@ function createMockRequestEmitter(): ClientRequest {
|
||||
req.write = vi.fn() as ClientRequest["write"];
|
||||
req.end = vi.fn() as ClientRequest["end"];
|
||||
req.destroy = vi.fn() as ClientRequest["destroy"];
|
||||
return req as ClientRequest;
|
||||
return req as unknown as ClientRequest;
|
||||
}
|
||||
|
||||
async function settleTimers<T>(promise: Promise<T>): Promise<T> {
|
||||
@@ -83,6 +92,10 @@ function installFakeTimerHarness() {
|
||||
vi.useFakeTimers();
|
||||
fakeNowMs += 10_000;
|
||||
vi.setSystemTime(fakeNowMs);
|
||||
ssrfMocks.resolvePinnedHostnameWithPolicy.mockResolvedValue({
|
||||
hostname: "example.com",
|
||||
addresses: ["93.184.216.34"],
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
@@ -156,6 +169,51 @@ describe("sendFileUrl", () => {
|
||||
const httpsRequest = vi.mocked(https.request);
|
||||
expect(httpsRequest.mock.calls[0]?.[1]).toMatchObject({ rejectUnauthorized: true });
|
||||
});
|
||||
|
||||
it("respects the shared send interval before posting a file URL", async () => {
|
||||
mockSuccessResponse();
|
||||
await settleTimers(sendMessage("https://nas.example.com/incoming", "hello"));
|
||||
vi.mocked(https.request).mockClear();
|
||||
|
||||
const promise = sendFileUrl("https://nas.example.com/incoming", "https://example.com/file.png");
|
||||
await Promise.resolve();
|
||||
expect(vi.mocked(https.request)).not.toHaveBeenCalled();
|
||||
|
||||
await vi.advanceTimersByTimeAsync(499);
|
||||
expect(vi.mocked(https.request)).not.toHaveBeenCalled();
|
||||
|
||||
await vi.advanceTimersByTimeAsync(1);
|
||||
await promise;
|
||||
expect(vi.mocked(https.request)).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
|
||||
it("rejects malformed file URLs before making a request", async () => {
|
||||
const result = await settleTimers(sendFileUrl("https://nas.example.com/incoming", "not-a-url"));
|
||||
expect(result).toBe(false);
|
||||
expect(ssrfMocks.resolvePinnedHostnameWithPolicy).not.toHaveBeenCalled();
|
||||
expect(vi.mocked(https.request)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects non-http file URLs before making a request", async () => {
|
||||
const result = await settleTimers(
|
||||
sendFileUrl("https://nas.example.com/incoming", "file:///tmp/secret.txt"),
|
||||
);
|
||||
expect(result).toBe(false);
|
||||
expect(ssrfMocks.resolvePinnedHostnameWithPolicy).not.toHaveBeenCalled();
|
||||
expect(vi.mocked(https.request)).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects SSRF-blocked hosts before making a request", async () => {
|
||||
ssrfMocks.resolvePinnedHostnameWithPolicy.mockRejectedValueOnce(
|
||||
new Error("Blocked private network target"),
|
||||
);
|
||||
const result = await settleTimers(
|
||||
sendFileUrl("https://nas.example.com/incoming", "http://169.254.169.254/latest/meta-data"),
|
||||
);
|
||||
expect(result).toBe(false);
|
||||
expect(ssrfMocks.resolvePinnedHostnameWithPolicy).toHaveBeenCalledWith("169.254.169.254");
|
||||
expect(vi.mocked(https.request)).not.toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
|
||||
// Helper to mock the user_list API response for fetchChatUsers / resolveLegacyWebhookNameToChatUserId
|
||||
|
||||
@@ -6,6 +6,10 @@
|
||||
import * as http from "node:http";
|
||||
import * as https from "node:https";
|
||||
import { safeParseJsonWithSchema, safeParseWithSchema } from "openclaw/plugin-sdk/extension-shared";
|
||||
import {
|
||||
formatErrorMessage,
|
||||
resolvePinnedHostnameWithPolicy,
|
||||
} from "openclaw/plugin-sdk/ssrf-runtime";
|
||||
import { z } from "zod";
|
||||
|
||||
const MIN_SEND_INTERVAL_MS = 500;
|
||||
@@ -131,9 +135,16 @@ export async function sendFileUrl(
|
||||
userId?: string | number,
|
||||
allowInsecureSsl = false,
|
||||
): Promise<boolean> {
|
||||
const body = buildWebhookBody({ file_url: fileUrl }, userId);
|
||||
|
||||
try {
|
||||
const safeFileUrl = await assertSafeWebhookFileUrl(fileUrl);
|
||||
const body = buildWebhookBody({ file_url: safeFileUrl }, userId);
|
||||
|
||||
const now = Date.now();
|
||||
const elapsed = now - lastSendTime;
|
||||
if (elapsed < MIN_SEND_INTERVAL_MS) {
|
||||
await sleep(MIN_SEND_INTERVAL_MS - elapsed);
|
||||
}
|
||||
|
||||
const ok = await doPost(incomingUrl, body, allowInsecureSsl);
|
||||
lastSendTime = Date.now();
|
||||
return ok;
|
||||
@@ -209,6 +220,22 @@ export async function fetchChatUsers(
|
||||
});
|
||||
}
|
||||
|
||||
async function assertSafeWebhookFileUrl(fileUrl: string): Promise<string> {
|
||||
let parsed: URL;
|
||||
try {
|
||||
parsed = new URL(fileUrl);
|
||||
} catch (err) {
|
||||
throw new Error(`Invalid Synology Chat file URL: ${formatErrorMessage(err)}`, { cause: err });
|
||||
}
|
||||
|
||||
if (parsed.protocol !== "http:" && parsed.protocol !== "https:") {
|
||||
throw new Error("Synology Chat file URL must use HTTP or HTTPS");
|
||||
}
|
||||
|
||||
await resolvePinnedHostnameWithPolicy(parsed.hostname);
|
||||
return parsed.toString();
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve a mutable webhook username/nickname to the correct Chat API user_id.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user