From 65b2e7a4e337e7f1ea483ff0b617ccda9b95cb13 Mon Sep 17 00:00:00 2001 From: Alix-007 Date: Thu, 9 Jul 2026 19:12:30 +0800 Subject: [PATCH] fix(mattermost): add timeout to REST client requests (#102027) * fix(mattermost): add timeout to REST client requests * fixup: preserve Mattermost DM retry timeout * test(mattermost): reuse hanging server helper * test(mattermost): satisfy timeout lint --------- Co-authored-by: Peter Steinberger --- .../mattermost/client.fetch-timeout.test.ts | 123 ++++++++++++++++++ .../mattermost/src/mattermost/client.ts | 56 +++++++- .../test-helpers/http-test-server.ts | 5 +- 3 files changed, 177 insertions(+), 7 deletions(-) create mode 100644 extensions/mattermost/src/mattermost/client.fetch-timeout.test.ts diff --git a/extensions/mattermost/src/mattermost/client.fetch-timeout.test.ts b/extensions/mattermost/src/mattermost/client.fetch-timeout.test.ts new file mode 100644 index 000000000000..51cc2f02cf1d --- /dev/null +++ b/extensions/mattermost/src/mattermost/client.fetch-timeout.test.ts @@ -0,0 +1,123 @@ +// Mattermost tests cover real REST client timeout behavior. +import { withServer } from "openclaw/plugin-sdk/test-env"; +import { describe, expect, it } from "vitest"; +import { + createMattermostClient, + createMattermostDirectChannelWithRetry, + fetchMattermostMe, +} from "./client.js"; + +type OperationOutcome = + | { status: "resolved" } + | { status: "rejected"; error: unknown } + | { + status: "pending"; + }; + +function delay(ms: number): Promise { + return new Promise((resolve) => { + setTimeout(resolve, ms); + }); +} + +async function settleWithin( + promise: Promise, + timeoutMs: number, +): Promise { + return await Promise.race([ + promise.then( + () => ({ status: "resolved" as const }), + (error: unknown) => ({ status: "rejected" as const, error }), + ), + delay(timeoutMs).then(() => ({ status: "pending" as const })), + ]); +} + +async function expectTimeoutRejection(promise: Promise, timeoutMs: number): Promise { + const outcome = await settleWithin(promise, timeoutMs); + expect(outcome.status).toBe("rejected"); + if (outcome.status !== "rejected") { + throw new Error(`expected timeout rejection, got ${outcome.status}`); + } + expect(outcome.error).toBeInstanceOf(Error); + expect(outcome.error instanceof Error ? outcome.error.name : "").toMatch( + /^(AbortError|TimeoutError)$/, + ); +} + +async function withHangingMattermostServer( + run: (server: { + baseUrl: string; + received: Promise; + requestCount: () => number; + }) => Promise, +): Promise { + let requestCount = 0; + let notifyRequest: () => void = () => {}; + const received = new Promise((resolve) => { + notifyRequest = resolve; + }); + await withServer( + (request) => { + requestCount += 1; + notifyRequest(); + request.resume(); + }, + async (baseUrl) => run({ baseUrl, received, requestCount: () => requestCount }), + ); +} + +describe("Mattermost REST client fetch timeout", () => { + it("rejects a hanging real loopback request at the configured client timeout", async () => { + await withHangingMattermostServer(async (server) => { + const client = createMattermostClient({ + baseUrl: server.baseUrl, + botToken: "bot-token", + allowPrivateNetwork: true, + timeoutMs: 50, + }); + const request = fetchMattermostMe(client); + + await server.received; + expect(server.requestCount()).toBe(1); + await expectTimeoutRejection(request, 750); + }); + }); + + it("preserves a caller AbortSignal while applying the default request timeout", async () => { + await withHangingMattermostServer(async (server) => { + const client = createMattermostClient({ + baseUrl: server.baseUrl, + botToken: "bot-token", + allowPrivateNetwork: true, + timeoutMs: 30_000, + }); + const controller = new AbortController(); + const request = client.request("/users/me", { signal: controller.signal }); + + await server.received; + controller.abort(); + await expectTimeoutRejection(request, 750); + }); + }); + + it("preserves configured DM retry timeouts longer than the client default", async () => { + await withHangingMattermostServer(async (server) => { + const client = createMattermostClient({ + baseUrl: server.baseUrl, + botToken: "bot-token", + allowPrivateNetwork: true, + timeoutMs: 50, + }); + const request = createMattermostDirectChannelWithRetry(client, ["bot-user", "dm-user"], { + maxRetries: 0, + timeoutMs: 250, + }); + request.catch(() => undefined); + + await server.received; + expect(await settleWithin(request, 120)).toEqual({ status: "pending" }); + await expectTimeoutRejection(request, 600); + }); + }); +}); diff --git a/extensions/mattermost/src/mattermost/client.ts b/extensions/mattermost/src/mattermost/client.ts index db32c0690dd9..afed6a8222c5 100644 --- a/extensions/mattermost/src/mattermost/client.ts +++ b/extensions/mattermost/src/mattermost/client.ts @@ -1,4 +1,5 @@ // Mattermost plugin module implements client behavior. +import { buildTimeoutAbortSignal } from "openclaw/plugin-sdk/extension-shared"; import { resolveTimerTimeoutMs } from "openclaw/plugin-sdk/number-runtime"; import { readProviderJsonResponse, @@ -17,6 +18,7 @@ import { import { z } from "zod"; const MATTERMOST_ERROR_BODY_LIMIT_BYTES = 8 * 1024; +const MATTERMOST_REQUEST_TIMEOUT_MS = 30_000; // Mattermost REST control-plane JSON (posts, users, channels, file-upload // results) stays well under a megabyte; cap successful JSON the same way the // shared provider path is capped so an untrusted/self-hosted homeserver cannot @@ -27,12 +29,15 @@ const MATTERMOST_TEXT_RESPONSE_LIMIT_BYTES = 64 * 1024; const NULL_BODY_STATUSES = new Set([101, 204, 205, 304]); export type MattermostFetch = (input: RequestInfo | URL, init?: RequestInit) => Promise; +export type MattermostRequestInit = RequestInit & { + timeoutMs?: number; +}; export type MattermostClient = { baseUrl: string; apiBaseUrl: string; token: string; - request: (path: string, init?: RequestInit) => Promise; + request: (path: string, init?: MattermostRequestInit) => Promise; /** Guarded fetch implementation; use in place of raw fetch for outbound requests. */ fetchImpl: MattermostFetch; }; @@ -174,6 +179,8 @@ export function createMattermostClient(params: { baseUrl: string; botToken: string; fetchImpl?: MattermostFetch; + /** Timeout for REST requests in milliseconds (default: 30000). */ + timeoutMs?: number; /** Allow requests to private/internal IPs (self-hosted/LAN deployments). */ allowPrivateNetwork?: boolean; }): MattermostClient { @@ -183,26 +190,56 @@ export function createMattermostClient(params: { } const apiBaseUrl = `${baseUrl}/api/v4`; const token = params.botToken.trim(); + const requestTimeoutMs = resolveTimerTimeoutMs(params.timeoutMs, MATTERMOST_REQUEST_TIMEOUT_MS); // When no custom fetchImpl is provided (production path), use an SSRF-guarded wrapper // that validates the target URL before making the request (DNS rebinding protection etc.). // A custom fetchImpl is accepted for testing and special cases. const externalFetchImpl = params.fetchImpl; - const guardedFetchImpl: MattermostFetch = async (input, init) => { + const guardedFetchImpl = async ( + input: RequestInfo | URL, + init?: MattermostRequestInit, + ): Promise => { const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + const { timeoutMs: initTimeoutMs, ...requestInit } = init ?? {}; + const timeoutMs = resolveTimerTimeoutMs(initTimeoutMs, requestTimeoutMs); const { response, release } = await fetchWithSsrFGuard({ url, - init, + init: requestInit, auditContext: "mattermost-api", policy: ssrfPolicyFromPrivateNetworkOptIn(params.allowPrivateNetwork), + signal: requestInit.signal ?? undefined, + timeoutMs, }); return responseWithRelease(response, release); }; - const fetchImpl = externalFetchImpl ?? guardedFetchImpl; + const timedExternalFetchImpl: + | ((input: RequestInfo | URL, init?: MattermostRequestInit) => Promise) + | undefined = externalFetchImpl + ? async (input, init) => { + const url = + typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + const { timeoutMs: initTimeoutMs, ...requestInit } = init ?? {}; + const timeoutMs = resolveTimerTimeoutMs(initTimeoutMs, requestTimeoutMs); + const { signal, cleanup } = buildTimeoutAbortSignal({ + timeoutMs, + signal: requestInit.signal ?? undefined, + operation: "mattermost-api", + url, + }); + try { + return await externalFetchImpl(input, { ...requestInit, signal }); + } finally { + cleanup(); + } + } + : undefined; - const request = async (path: string, init?: RequestInit): Promise => { + const fetchImpl = timedExternalFetchImpl ?? guardedFetchImpl; + + const request = async (path: string, init?: MattermostRequestInit): Promise => { const url = buildMattermostApiUrl(baseUrl, path); const headers = new Headers(init?.headers); headers.set("Authorization", `Bearer ${token}`); @@ -287,11 +324,13 @@ export async function createMattermostDirectChannel( client: MattermostClient, userIds: string[], signal?: AbortSignal, + timeoutMs?: number, ): Promise { return await client.request("/channels/direct", { method: "POST", body: JSON.stringify(userIds), signal, + timeoutMs, }); } @@ -406,7 +445,12 @@ export async function createMattermostDirectChannelWithRetry( const timeoutId = setTimeout(() => controller.abort(), timeoutMs); try { - const result = await createMattermostDirectChannel(client, userIds, controller.signal); + const result = await createMattermostDirectChannel( + client, + userIds, + controller.signal, + timeoutMs, + ); return result; } finally { clearTimeout(timeoutId); diff --git a/src/plugin-sdk/test-helpers/http-test-server.ts b/src/plugin-sdk/test-helpers/http-test-server.ts index 9edb708b28f7..8fba27f26494 100644 --- a/src/plugin-sdk/test-helpers/http-test-server.ts +++ b/src/plugin-sdk/test-helpers/http-test-server.ts @@ -15,8 +15,11 @@ export async function withServer(handler: RequestListener, fn: (baseUrl: string) try { await fn(`http://127.0.0.1:${address.port}`); } finally { - await new Promise((resolve) => { + const closed = new Promise((resolve) => { server.close(() => resolve()); }); + // Hanging-response tests must still release active sockets when their assertion fails. + server.closeAllConnections(); + await closed; } }