From 9a3800d8e6e69bc0a125dca5760d47515e746454 Mon Sep 17 00:00:00 2001 From: Jean-Marc Date: Mon, 2 Mar 2026 20:50:58 +0100 Subject: [PATCH] fix(synology-chat): resolve Chat API user_id for reply delivery (#23709) * fix(synology-chat): resolve Chat API user_id for reply delivery Synology Chat outgoing webhooks use a per-integration user_id that differs from the global Chat API user_id required by method=chatbot. This caused reply messages to fail silently when the IDs diverged. Changes: - Add fetchChatUsers() and resolveChatUserId() to resolve the correct Chat API user_id via the user_list endpoint (cached 5min) - Use resolved user_id for all sendMessage() calls in webhook handler and channel dispatcher - Add Provider field to MsgContext so the agent runner correctly identifies the message channel (was "unknown", now "synology-chat") - Log warnings when user_list API fails or when falling back to unresolved webhook user_id - Add 5 tests for user_id resolution (nickname, username, case, not-found, URL rewrite) Co-Authored-By: Claude Opus 4.6 * fix(synology-chat): use Readable stream in integration test for Windows compat Replace EventEmitter + process.nextTick with Readable stream for request body simulation. The process.nextTick approach caused the test to hang on Windows CI (120s timeout) because events were not reliably delivered to readBody() listeners. Co-Authored-By: Claude Opus 4.6 * fix: harden synology reply user resolution and cache scope (#23709) (thanks @druide67) --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: Peter Steinberger --- CHANGELOG.md | 1 + .../src/channel.integration.test.ts | 4 +- extensions/synology-chat/src/channel.ts | 6 +- extensions/synology-chat/src/client.test.ts | 127 +++++++++++++++++- extensions/synology-chat/src/client.ts | 123 +++++++++++++++++ .../synology-chat/src/webhook-handler.test.ts | 3 +- .../synology-chat/src/webhook-handler.ts | 34 ++++- 7 files changed, 287 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e12722932..038539822 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,7 @@ Docs: https://docs.openclaw.ai - Synology Chat/webhook compatibility: accept JSON and alias payload fields, allow token resolution from body/query/header sources, and ACK webhook requests with `204` to avoid persistent `Processing...` states in Synology Chat clients. (#26635) Thanks @memphislee09-source. - Synology Chat/webhook ingress hardening: enforce bounded body reads (size + timeout) via shared request-body guards to prevent unauthenticated slow-body hangs before token validation. (#25831) Thanks @bmendonca3. +- Synology Chat/reply delivery: resolve webhook usernames to Chat API `user_id` values for outbound chatbot replies, avoiding mismatches between webhook user IDs and `method=chatbot` recipient IDs in multi-account setups. (#23709) Thanks @druide67. - Auto-reply/followup queue: avoid stale callback reuse across idle-window restarts by caching the followup runner only when a drain actually starts, preserving enqueue ordering after empty-finalize paths. (#31902) Thanks @Lanfei. - Gateway/Heartbeat model reload: treat `models.*` and `agents.defaults.model` config updates as heartbeat hot-reload triggers so heartbeat picks up model changes without a full gateway restart. (#32046) Thanks @stakeswky. - Slack/inbound debounce routing: isolate top-level non-DM message debounce keys by message timestamp to avoid cross-thread collisions, preserve DM batching, and flush pending top-level buffers before immediate non-debounce follow-ups to keep ordering stable. (#31951) Thanks @scoootscooob. diff --git a/extensions/synology-chat/src/channel.integration.test.ts b/extensions/synology-chat/src/channel.integration.test.ts index e901da43d..dd2b6273b 100644 --- a/extensions/synology-chat/src/channel.integration.test.ts +++ b/extensions/synology-chat/src/channel.integration.test.ts @@ -102,6 +102,8 @@ describe("Synology channel wiring integration", () => { expect(res._body).toContain("not authorized"); expect(dispatchReplyWithBufferedBlockDispatcher).not.toHaveBeenCalled(); - started.stop(); + if (started && typeof started === "object" && "stop" in started) { + (started as { stop: () => void }).stop(); + } }); }); diff --git a/extensions/synology-chat/src/channel.ts b/extensions/synology-chat/src/channel.ts index 2ce7e1c98..61fbc7450 100644 --- a/extensions/synology-chat/src/channel.ts +++ b/extensions/synology-chat/src/channel.ts @@ -243,6 +243,10 @@ export function createSynologyChatPlugin() { const rt = getSynologyRuntime(); const currentCfg = await rt.config.loadConfig(); + // The Chat API user_id (for sending) may differ from the webhook + // user_id (used for sessions/pairing). Use chatUserId for API calls. + const sendUserId = msg.chatUserId ?? msg.from; + // Build MsgContext using SDK's finalizeInboundContext for proper normalization const msgCtx = rt.channel.reply.finalizeInboundContext({ Body: msg.body, @@ -275,7 +279,7 @@ export function createSynologyChatPlugin() { await sendMessage( account.incomingUrl, text, - msg.from, + sendUserId, account.allowInsecureSsl, ); } diff --git a/extensions/synology-chat/src/client.test.ts b/extensions/synology-chat/src/client.test.ts index edb483069..ef5ff06be 100644 --- a/extensions/synology-chat/src/client.test.ts +++ b/extensions/synology-chat/src/client.test.ts @@ -4,16 +4,18 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; // Mock http and https modules before importing the client vi.mock("node:https", () => { const mockRequest = vi.fn(); - return { default: { request: mockRequest }, request: mockRequest }; + const mockGet = vi.fn(); + return { default: { request: mockRequest, get: mockGet }, request: mockRequest, get: mockGet }; }); vi.mock("node:http", () => { const mockRequest = vi.fn(); - return { default: { request: mockRequest }, request: mockRequest }; + const mockGet = vi.fn(); + return { default: { request: mockRequest, get: mockGet }, request: mockRequest, get: mockGet }; }); // Import after mocks are set up -const { sendMessage, sendFileUrl } = await import("./client.js"); +const { sendMessage, sendFileUrl, fetchChatUsers, resolveChatUserId } = await import("./client.js"); const https = await import("node:https"); let fakeNowMs = 1_700_000_000_000; @@ -111,3 +113,122 @@ describe("sendFileUrl", () => { expect(result).toBe(false); }); }); + +// Helper to mock the user_list API response for fetchChatUsers / resolveChatUserId +function mockUserListResponse( + users: Array<{ user_id: number; username: string; nickname: string }>, +) { + const httpsGet = vi.mocked((https as any).get); + httpsGet.mockImplementation((_url: any, _opts: any, callback: any) => { + const res = new EventEmitter() as any; + res.statusCode = 200; + process.nextTick(() => { + callback(res); + res.emit("data", Buffer.from(JSON.stringify({ success: true, data: { users } }))); + res.emit("end"); + }); + const req = new EventEmitter() as any; + req.destroy = vi.fn(); + return req; + }); +} + +function mockUserListResponseOnce( + users: Array<{ user_id: number; username: string; nickname: string }>, +) { + const httpsGet = vi.mocked((https as any).get); + httpsGet.mockImplementationOnce((_url: any, _opts: any, callback: any) => { + const res = new EventEmitter() as any; + res.statusCode = 200; + process.nextTick(() => { + callback(res); + res.emit("data", Buffer.from(JSON.stringify({ success: true, data: { users } }))); + res.emit("end"); + }); + const req = new EventEmitter() as any; + req.destroy = vi.fn(); + return req; + }); +} + +describe("resolveChatUserId", () => { + const baseUrl = + "https://nas.example.com/webapi/entry.cgi?api=SYNO.Chat.External&method=chatbot&version=2&token=%22test%22"; + const baseUrl2 = + "https://nas2.example.com/webapi/entry.cgi?api=SYNO.Chat.External&method=chatbot&version=2&token=%22test-2%22"; + + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers(); + // Advance time to invalidate any cached user list from previous tests + fakeNowMs += 10 * 60 * 1000; + vi.setSystemTime(fakeNowMs); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("resolves user by nickname (webhook username = Chat nickname)", async () => { + mockUserListResponse([ + { user_id: 4, username: "jmn67", nickname: "jmn" }, + { user_id: 7, username: "she67", nickname: "sarah" }, + ]); + const result = await resolveChatUserId(baseUrl, "jmn"); + expect(result).toBe(4); + }); + + it("resolves user by username when nickname does not match", async () => { + mockUserListResponse([ + { user_id: 4, username: "jmn67", nickname: "" }, + { user_id: 7, username: "she67", nickname: "sarah" }, + ]); + // Advance time to invalidate cache + fakeNowMs += 10 * 60 * 1000; + vi.setSystemTime(fakeNowMs); + const result = await resolveChatUserId(baseUrl, "jmn67"); + expect(result).toBe(4); + }); + + it("is case-insensitive", async () => { + mockUserListResponse([{ user_id: 4, username: "JMN67", nickname: "JMN" }]); + fakeNowMs += 10 * 60 * 1000; + vi.setSystemTime(fakeNowMs); + const result = await resolveChatUserId(baseUrl, "jmn"); + expect(result).toBe(4); + }); + + it("returns undefined when user is not found", async () => { + mockUserListResponse([{ user_id: 4, username: "jmn67", nickname: "jmn" }]); + fakeNowMs += 10 * 60 * 1000; + vi.setSystemTime(fakeNowMs); + const result = await resolveChatUserId(baseUrl, "unknown_user"); + expect(result).toBeUndefined(); + }); + + it("uses method=user_list instead of method=chatbot in the API URL", async () => { + mockUserListResponse([]); + fakeNowMs += 10 * 60 * 1000; + vi.setSystemTime(fakeNowMs); + await resolveChatUserId(baseUrl, "anyone"); + const httpsGet = vi.mocked((https as any).get); + expect(httpsGet).toHaveBeenCalledWith( + expect.stringContaining("method=user_list"), + expect.any(Object), + expect.any(Function), + ); + }); + + it("keeps user cache scoped per incoming URL", async () => { + mockUserListResponseOnce([{ user_id: 4, username: "jmn67", nickname: "jmn" }]); + mockUserListResponseOnce([{ user_id: 9, username: "jmn67", nickname: "jmn" }]); + + const result1 = await resolveChatUserId(baseUrl, "jmn"); + const result2 = await resolveChatUserId(baseUrl2, "jmn"); + + expect(result1).toBe(4); + expect(result2).toBe(9); + const httpsGet = vi.mocked((https as any).get); + expect(httpsGet).toHaveBeenCalledTimes(2); + }); +}); diff --git a/extensions/synology-chat/src/client.ts b/extensions/synology-chat/src/client.ts index 316a38799..95240e556 100644 --- a/extensions/synology-chat/src/client.ts +++ b/extensions/synology-chat/src/client.ts @@ -9,6 +9,28 @@ import * as https from "node:https"; const MIN_SEND_INTERVAL_MS = 500; let lastSendTime = 0; +// --- Chat user_id resolution --- +// Synology Chat uses two different user_id spaces: +// - Outgoing webhook user_id: per-integration sequential ID (e.g. 1) +// - Chat API user_id: global internal ID (e.g. 4) +// The chatbot API (method=chatbot) requires the Chat API user_id in the +// user_ids array. We resolve via the user_list API and cache the result. + +interface ChatUser { + user_id: number; + username: string; + nickname: string; +} + +type ChatUserCacheEntry = { + users: ChatUser[]; + cachedAt: number; +}; + +// Cache user lists per bot endpoint to avoid cross-account bleed. +const chatUserCache = new Map(); +const CACHE_TTL_MS = 5 * 60 * 1000; // 5 minutes + /** * Send a text message to Synology Chat via the incoming webhook. * @@ -92,6 +114,107 @@ export async function sendFileUrl( } } +/** + * Fetch the list of Chat users visible to this bot via the user_list API. + * Results are cached for CACHE_TTL_MS to avoid excessive API calls. + * + * The user_list endpoint uses the same base URL as the chatbot API but + * with method=user_list instead of method=chatbot. + */ +export async function fetchChatUsers( + incomingUrl: string, + allowInsecureSsl = true, + log?: { warn: (...args: unknown[]) => void }, +): Promise { + const now = Date.now(); + const listUrl = incomingUrl.replace(/method=\w+/, "method=user_list"); + const cached = chatUserCache.get(listUrl); + if (cached && now - cached.cachedAt < CACHE_TTL_MS) { + return cached.users; + } + + return new Promise((resolve) => { + let parsedUrl: URL; + try { + parsedUrl = new URL(listUrl); + } catch { + log?.warn("fetchChatUsers: invalid user_list URL, using cached data"); + resolve(cached?.users ?? []); + return; + } + const transport = parsedUrl.protocol === "https:" ? https : http; + + transport + .get(listUrl, { rejectUnauthorized: !allowInsecureSsl } as any, (res) => { + let data = ""; + res.on("data", (c: Buffer) => { + data += c.toString(); + }); + res.on("end", () => { + try { + const result = JSON.parse(data); + if (result.success && result.data?.users) { + const users = result.data.users.map((u: any) => ({ + user_id: u.user_id, + username: u.username || "", + nickname: u.nickname || "", + })); + chatUserCache.set(listUrl, { + users, + cachedAt: now, + }); + resolve(users); + } else { + log?.warn( + `fetchChatUsers: API returned success=${result.success}, using cached data`, + ); + resolve(cached?.users ?? []); + } + } catch { + log?.warn("fetchChatUsers: failed to parse user_list response"); + resolve(cached?.users ?? []); + } + }); + }) + .on("error", (err) => { + log?.warn(`fetchChatUsers: HTTP error — ${err instanceof Error ? err.message : err}`); + resolve(cached?.users ?? []); + }); + }); +} + +/** + * Resolve a webhook username to the correct Chat API user_id. + * + * Synology Chat outgoing webhooks send a user_id that may NOT match the + * Chat-internal user_id needed by the chatbot API (method=chatbot). + * The webhook's "username" field corresponds to the Chat user's "nickname". + * + * @param incomingUrl - Bot incoming webhook URL (used to derive user_list URL) + * @param webhookUsername - The username from the outgoing webhook payload + * @param allowInsecureSsl - Skip TLS verification + * @returns The correct Chat user_id, or undefined if not found + */ +export async function resolveChatUserId( + incomingUrl: string, + webhookUsername: string, + allowInsecureSsl = true, + log?: { warn: (...args: unknown[]) => void }, +): Promise { + const users = await fetchChatUsers(incomingUrl, allowInsecureSsl, log); + const lower = webhookUsername.toLowerCase(); + + // Match by nickname first (webhook "username" field = Chat "nickname") + const byNickname = users.find((u) => u.nickname.toLowerCase() === lower); + if (byNickname) return byNickname.user_id; + + // Then by username + const byUsername = users.find((u) => u.username.toLowerCase() === lower); + if (byUsername) return byUsername.user_id; + + return undefined; +} + function doPost(url: string, body: string, allowInsecureSsl = true): Promise { return new Promise((resolve, reject) => { let parsedUrl: URL; diff --git a/extensions/synology-chat/src/webhook-handler.test.ts b/extensions/synology-chat/src/webhook-handler.test.ts index ee87e3698..2f6bd8778 100644 --- a/extensions/synology-chat/src/webhook-handler.test.ts +++ b/extensions/synology-chat/src/webhook-handler.test.ts @@ -7,9 +7,10 @@ import { createWebhookHandler, } from "./webhook-handler.js"; -// Mock sendMessage to prevent real HTTP calls +// Mock sendMessage and resolveChatUserId to prevent real HTTP calls vi.mock("./client.js", () => ({ sendMessage: vi.fn().mockResolvedValue(true), + resolveChatUserId: vi.fn().mockResolvedValue(undefined), })); function makeAccount( diff --git a/extensions/synology-chat/src/webhook-handler.ts b/extensions/synology-chat/src/webhook-handler.ts index a0bcb6412..197ec2cee 100644 --- a/extensions/synology-chat/src/webhook-handler.ts +++ b/extensions/synology-chat/src/webhook-handler.ts @@ -10,7 +10,7 @@ import { readRequestBodyWithLimit, requestBodyErrorToText, } from "openclaw/plugin-sdk"; -import { sendMessage } from "./client.js"; +import { sendMessage, resolveChatUserId } from "./client.js"; import { validateToken, authorizeUserForDm, sanitizeInput, RateLimiter } from "./security.js"; import type { SynologyWebhookPayload, ResolvedSynologyChatAccount } from "./types.js"; @@ -225,6 +225,8 @@ export interface WebhookHandlerDeps { chatType: string; sessionKey: string; accountId: string; + /** Chat API user_id for sending replies (may differ from webhook user_id) */ + chatUserId?: string; }) => Promise; log?: { info: (...args: unknown[]) => void; @@ -330,8 +332,29 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { // ACK immediately so Synology Chat won't remain in "Processing..." respondNoContent(res); + // Default to webhook user_id; may be replaced with Chat API user_id below. + let replyUserId = payload.user_id; + // Deliver to agent asynchronously (with 120s timeout to match nginx proxy_read_timeout) try { + // Resolve the Chat-internal user_id for sending replies. + // Synology Chat outgoing webhooks use a per-integration user_id that may + // differ from the global Chat API user_id required by method=chatbot. + // We resolve via the user_list API, matching by nickname/username. + const chatUserId = await resolveChatUserId( + account.incomingUrl, + payload.username, + account.allowInsecureSsl, + log, + ); + if (chatUserId !== undefined) { + replyUserId = String(chatUserId); + } else { + log?.warn( + `Could not resolve Chat API user_id for "${payload.username}" — falling back to webhook user_id ${payload.user_id}. Reply delivery may fail.`, + ); + } + const sessionKey = `synology-chat-${payload.user_id}`; const deliverPromise = deliver({ body: cleanText, @@ -341,6 +364,7 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { chatType: "direct", sessionKey, accountId: account.accountId, + chatUserId: replyUserId, }); const timeoutPromise = new Promise((_, reject) => @@ -349,11 +373,11 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { const reply = await Promise.race([deliverPromise, timeoutPromise]); - // Send reply back to Synology Chat + // Send reply back to Synology Chat using the resolved Chat user_id if (reply) { - await sendMessage(account.incomingUrl, reply, payload.user_id, account.allowInsecureSsl); + await sendMessage(account.incomingUrl, reply, replyUserId, account.allowInsecureSsl); const replyPreview = reply.length > 100 ? `${reply.slice(0, 100)}...` : reply; - log?.info(`Reply sent to ${payload.username} (${payload.user_id}): ${replyPreview}`); + log?.info(`Reply sent to ${payload.username} (${replyUserId}): ${replyPreview}`); } } catch (err) { const errMsg = err instanceof Error ? `${err.message}\n${err.stack}` : String(err); @@ -361,7 +385,7 @@ export function createWebhookHandler(deps: WebhookHandlerDeps) { await sendMessage( account.incomingUrl, "Sorry, an error occurred while processing your message.", - payload.user_id, + replyUserId, account.allowInsecureSsl, ); }