From ba4b359d67006ca6b42f13ec7a36424fc43889bf Mon Sep 17 00:00:00 2001 From: Peter Lee Date: Thu, 27 Aug 2026 12:42:28 -0500 Subject: [PATCH] fix(nextcloud-talk): redact reflected credentials in error diagnostics (#119976) Share bounded, complete error-body redaction across message sends, reactions, and bot preflight. Preserve status fallbacks, request deadlines, UTF-16 display bounds, and accepted-send receipts. Consolidate duplicate receipt and feature-mask fixtures while adding real HTTP reflection, truncation, and idle-deadline regressions. Co-authored-by: Peter Steinberger --- CHANGELOG.md | 1 + .../nextcloud-talk/src/bot-preflight.test.ts | 80 +++------ .../nextcloud-talk/src/bot-preflight.ts | 16 +- .../nextcloud-talk/src/guarded-response.ts | 21 +++ .../src/send.cfg-threading.test.ts | 167 ++++++------------ .../src/send.fetch-timeout.test.ts | 68 +++++++ extensions/nextcloud-talk/src/send.ts | 35 +--- 7 files changed, 187 insertions(+), 201 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e83380bdd10..0815235893e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -70,6 +70,7 @@ Docs: https://docs.openclaw.ai ### Fixes - **Android settings:** keep form fields and actions reachable above the keyboard, and respect bottom system insets without duplicating navigation padding. +- **Nextcloud Talk diagnostics:** redact reflected credentials before displaying send, reaction, and bot-preflight errors, and suppress incomplete error bodies. (#119976) Thanks @xialonglee. - Codex image attachments: decode mixed-case `file://` URLs as local image paths while preserving existing file URL validation and platform behavior. (#121611) Thanks @sunlit-deng. - **Android gateway discovery:** resolve nearby gateways one at a time on Android 12 and 13 so simultaneously advertised gateways are not silently omitted. diff --git a/extensions/nextcloud-talk/src/bot-preflight.test.ts b/extensions/nextcloud-talk/src/bot-preflight.test.ts index 1428a2f35a2a..0012bc2a18fc 100644 --- a/extensions/nextcloud-talk/src/bot-preflight.test.ts +++ b/extensions/nextcloud-talk/src/bot-preflight.test.ts @@ -90,31 +90,21 @@ describe("probeNextcloudTalkBotResponseFeature", () => { hoisted.fetchWithSsrFGuard.mockReset(); }); - it("passes when the matching bot has the response feature bit", async () => { - mockBotAdmin(1 | 2 | 8); + it.each([1 | 2 | 8, "+011"])( + "accepts numeric or signed decimal response features: %j", + async (features) => { + mockBotAdmin(features); - await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ - ok: true, - code: "ok", - botId: "7", - botName: "OpenClaw", - features: 11, - message: 'Nextcloud Talk bot "OpenClaw" has the response feature.', - }); - }); - - it("normalizes signed decimal bot feature strings through the shared parser", async () => { - mockBotAdmin("+011"); - - await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ - ok: true, - code: "ok", - botId: "7", - botName: "OpenClaw", - features: 11, - message: 'Nextcloud Talk bot "OpenClaw" has the response feature.', - }); - }); + await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ + ok: true, + code: "ok", + botId: "7", + botName: "OpenClaw", + features: 11, + message: 'Nextcloud Talk bot "OpenClaw" has the response feature.', + }); + }, + ); it("reports missing response feature for the matching webhook bot", async () => { mockBotAdmin(1 | 8); @@ -130,31 +120,21 @@ describe("probeNextcloudTalkBotResponseFeature", () => { }); }); - it("does not coerce partial bot feature strings", async () => { - mockBotAdmin("2response"); + it.each(["2response", -1])( + "rejects malformed or negative response features: %j", + async (features) => { + mockBotAdmin(features); - await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ - ok: false, - code: "missing_response_feature", - botId: "7", - botName: "OpenClaw", - message: - 'Nextcloud Talk bot "OpenClaw" (7) is missing the response feature; outbound replies will fail. Run ./occ talk:bot:state --feature webhook --feature response --feature reaction 7 1 or reinstall the bot with --feature response.', - }); - }); - - it("does not treat negative feature masks as having every feature", async () => { - mockBotAdmin(-1); - - await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ - ok: false, - code: "missing_response_feature", - botId: "7", - botName: "OpenClaw", - message: - 'Nextcloud Talk bot "OpenClaw" (7) is missing the response feature; outbound replies will fail. Run ./occ talk:bot:state --feature webhook --feature response --feature reaction 7 1 or reinstall the bot with --feature response.', - }); - }); + await expect(probeNextcloudTalkBotResponseFeature({ account: account() })).resolves.toEqual({ + ok: false, + code: "missing_response_feature", + botId: "7", + botName: "OpenClaw", + message: + 'Nextcloud Talk bot "OpenClaw" (7) is missing the response feature; outbound replies will fail. Run ./occ talk:bot:state --feature webhook --feature response --feature reaction 7 1 or reinstall the bot with --feature response.', + }); + }, + ); it("reports malformed bot admin JSON with a stable channel error", async () => { hoisted.fetchWithSsrFGuard.mockResolvedValueOnce({ @@ -190,9 +170,7 @@ describe("probeNextcloudTalkBotResponseFeature", () => { ok: false, code: "api_error", status: 503, - message: expect.stringContaining( - "Nextcloud Talk bot response feature probe failed (503): nextcloud bot admin failure", - ), + message: "Nextcloud Talk bot response feature probe failed (503)", }); expect(textSpy).not.toHaveBeenCalled(); expect(tracked.wasCanceled()).toBe(true); diff --git a/extensions/nextcloud-talk/src/bot-preflight.ts b/extensions/nextcloud-talk/src/bot-preflight.ts index 7cd1eece2c08..7b5f9cd78696 100644 --- a/extensions/nextcloud-talk/src/bot-preflight.ts +++ b/extensions/nextcloud-talk/src/bot-preflight.ts @@ -1,18 +1,17 @@ // Nextcloud Talk plugin module implements bot preflight behavior. import { formatErrorMessage } from "openclaw/plugin-sdk/error-runtime"; import { parseStrictNonNegativeInteger } from "openclaw/plugin-sdk/number-runtime"; -import { - readProviderJsonResponse, - readResponseTextLimited, -} from "openclaw/plugin-sdk/provider-http"; +import { readProviderJsonResponse } from "openclaw/plugin-sdk/provider-http"; import { fetchWithSsrFGuard } from "../runtime-api.js"; import type { ResolvedNextcloudTalkAccount } from "./accounts.js"; import { resolveNextcloudTalkApiCredentials } from "./api-credentials.js"; -import { releaseNextcloudTalkGuardedResponse } from "./guarded-response.js"; +import { + readNextcloudTalkErrorBody, + releaseNextcloudTalkGuardedResponse, +} from "./guarded-response.js"; import { ssrfPolicyFromPrivateNetworkOptIn } from "./send.runtime.js"; const BOT_FEATURE_RESPONSE = 2; -const BOT_PREFLIGHT_ERROR_BODY_LIMIT_BYTES = 8 * 1024; type NextcloudTalkBotAdminEntry = { id?: number | string; @@ -130,10 +129,7 @@ export async function probeNextcloudTalkBotResponseFeature(params: { }); try { if (!response.ok) { - const body = await readResponseTextLimited( - response, - BOT_PREFLIGHT_ERROR_BODY_LIMIT_BYTES, - ).catch(() => ""); + const body = await readNextcloudTalkErrorBody(response, auth, credentials.apiPassword); return { ok: false, code: "api_error", diff --git a/extensions/nextcloud-talk/src/guarded-response.ts b/extensions/nextcloud-talk/src/guarded-response.ts index 1f1de6af3272..b5870f966a21 100644 --- a/extensions/nextcloud-talk/src/guarded-response.ts +++ b/extensions/nextcloud-talk/src/guarded-response.ts @@ -1,3 +1,6 @@ +import { redactToolPayloadText } from "openclaw/plugin-sdk/logging-core"; +import { readProviderTextResponse } from "openclaw/plugin-sdk/provider-http"; + // Nextcloud Talk guarded fetches own their dispatcher until the response body // settles. Cancel unread bodies before release so streaming responses cannot // keep the dispatcher alive after an early return. @@ -10,3 +13,21 @@ export async function releaseNextcloudTalkGuardedResponse(params: { } await params.release(); } +export async function readNextcloudTalkErrorBody( + response: Response, + ...credentials: string[] +): Promise { + try { + // Never expose a truncated credential: redact only complete, bounded bodies. + let body = await readProviderTextResponse(response, "Nextcloud Talk error", { + maxBytes: 8 * 1024, + chunkTimeoutMs: 10_000, + }); + for (const credential of credentials) { + body = body.replaceAll(credential, "***"); + } + return redactToolPayloadText(body); + } catch { + return ""; + } +} diff --git a/extensions/nextcloud-talk/src/send.cfg-threading.test.ts b/extensions/nextcloud-talk/src/send.cfg-threading.test.ts index ad5890ca7e9c..282fc1e256d9 100644 --- a/extensions/nextcloud-talk/src/send.cfg-threading.test.ts +++ b/extensions/nextcloud-talk/src/send.cfg-threading.test.ts @@ -140,52 +140,60 @@ describe("nextcloud-talk send cfg threading", () => { expect(fetchMock).toHaveBeenCalledOnce(); }); - it("uses provided cfg for sendMessage and skips runtime loadConfig", async () => { - const cfg = { source: "provided" } as const; - mockNextcloudMessageResponse(12345, 1_706_000_000); + it.each([true, false])( + "preserves cfg and receipts with runtime initialized=%s", + async (initialized) => { + const cfg = { source: "provided" } as const; + if (!initialized) { + hoisted.record.mockImplementation(() => { + throw new Error("Nextcloud Talk runtime not initialized"); + }); + } + mockNextcloudMessageResponse(12345, 1_706_000_000); - const result = await sendMessageNextcloudTalk("room:abc123", "hello", { - cfg, - accountId: "work", - }); + const result = await sendMessageNextcloudTalk("room:abc123", "hello", { + cfg, + accountId: "work", + }); - expectProvidedMessageCfgThreading(cfg); - expect(hoisted.record).toHaveBeenCalledWith({ - channel: "nextcloud-talk", - accountId: "default", - direction: "outbound", - }); - expect(fetchMock).toHaveBeenCalledTimes(1); - expect(result).toEqual({ - messageId: "12345", - receipt: { - platformMessageIds: ["12345"], - primaryPlatformMessageId: "12345", - parts: [ - { - index: 0, - kind: "text", - platformMessageId: "12345", - raw: { + expectProvidedMessageCfgThreading(cfg); + expect(hoisted.record).toHaveBeenCalledWith({ + channel: "nextcloud-talk", + accountId: "default", + direction: "outbound", + }); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(result).toEqual({ + messageId: "12345", + receipt: { + platformMessageIds: ["12345"], + primaryPlatformMessageId: "12345", + parts: [ + { + index: 0, + kind: "text", + platformMessageId: "12345", + raw: { + channel: "nextcloud-talk", + conversationId: "abc123", + messageId: "12345", + }, + }, + ], + raw: [ + { channel: "nextcloud-talk", conversationId: "abc123", messageId: "12345", }, - }, - ], - raw: [ - { - channel: "nextcloud-talk", - conversationId: "abc123", - messageId: "12345", - }, - ], - sentAt: fixedSentAt, - }, - roomToken: "abc123", - timestamp: 1_706_000_000, - }); - }); + ], + sentAt: fixedSentAt, + }, + roomToken: "abc123", + timestamp: 1_706_000_000, + }); + }, + ); it("strips mixed-case provider and room prefixes before sending", async () => { const cfg = { source: "provided" } as const; @@ -229,50 +237,6 @@ describe("nextcloud-talk send cfg threading", () => { ); }); - it("sends with provided cfg even when the runtime store is not initialized", async () => { - const cfg = { source: "provided" } as const; - hoisted.record.mockImplementation(() => { - throw new Error("Nextcloud Talk runtime not initialized"); - }); - mockNextcloudMessageResponse(12346, 1_706_000_001); - - const result = await sendMessageNextcloudTalk("room:abc123", "hello", { - cfg, - accountId: "work", - }); - - expectProvidedMessageCfgThreading(cfg); - expect(result).toEqual({ - messageId: "12346", - receipt: { - platformMessageIds: ["12346"], - primaryPlatformMessageId: "12346", - parts: [ - { - index: 0, - kind: "text", - platformMessageId: "12346", - raw: { - channel: "nextcloud-talk", - conversationId: "abc123", - messageId: "12346", - }, - }, - ], - raw: [ - { - channel: "nextcloud-talk", - conversationId: "abc123", - messageId: "12346", - }, - ], - sentAt: fixedSentAt, - }, - roomToken: "abc123", - timestamp: 1_706_000_001, - }); - }); - it("preserves reply ids in receipts", async () => { const cfg = { source: "provided" } as const; mockNextcloudMessageResponse(12347, 1_706_000_002); @@ -531,7 +495,7 @@ describe("nextcloud-talk send bounded response reads", () => { expect(result.timestamp).toBeUndefined(); }); - it("bounds an oversized error body into a short send-failure snippet", async () => { + it("omits an oversized error body from the send failure", async () => { fetchMock.mockResolvedValueOnce( streamingResponse({ status: 400, @@ -543,10 +507,10 @@ describe("nextcloud-talk send bounded response reads", () => { await expect( sendMessageNextcloudTalk("room:abc", "hello", { cfg: { source: "provided" } }), - ).rejects.toThrow(/Nextcloud Talk: bad request/); + ).rejects.toThrow(new Error("Nextcloud Talk: bad request - invalid message format")); }); - it("bounds an oversized reaction error body into a short snippet", async () => { + it("omits an oversized error body from the reaction failure", async () => { fetchMock.mockResolvedValueOnce( streamingResponse({ status: 500, @@ -556,31 +520,8 @@ describe("nextcloud-talk send bounded response reads", () => { }), ); - let caught: unknown; - try { - await sendReactionNextcloudTalk("room:abc", "m-1", "👍", { cfg: { source: "provided" } }); - } catch (error) { - caught = error; - } - - expect(caught).toBeInstanceOf(Error); - // The collapsed snippet caps the message far below the streamed 17 MiB body. - expect((caught as Error).message.length).toBeLessThan(4_000); - }); - - it("still parses a normal small success body", async () => { - fetchMock.mockResolvedValueOnce( - new Response(JSON.stringify({ ocs: { data: { id: 99, timestamp: 1_700_000_000 } } }), { - status: 200, - headers: { "content-type": "application/json" }, - }), - ); - - const result = await sendMessageNextcloudTalk("room:abc", "hello", { - cfg: { source: "provided" }, - }); - - expect(result.messageId).toBe("99"); - expect(result.timestamp).toBe(1_700_000_000); + await expect( + sendReactionNextcloudTalk("room:abc", "m-1", "👍", { cfg: { source: "provided" } }), + ).rejects.toThrow(new Error("Nextcloud Talk reaction failed: 500")); }); }); diff --git a/extensions/nextcloud-talk/src/send.fetch-timeout.test.ts b/extensions/nextcloud-talk/src/send.fetch-timeout.test.ts index e0011e4df476..d48809e33b2d 100644 --- a/extensions/nextcloud-talk/src/send.fetch-timeout.test.ts +++ b/extensions/nextcloud-talk/src/send.fetch-timeout.test.ts @@ -1,5 +1,7 @@ import { withServer } from "openclaw/plugin-sdk/test-env"; import { describe, expect, it } from "vitest"; +import { resolveNextcloudTalkAccount } from "./accounts.js"; +import { probeNextcloudTalkBotResponseFeature } from "./bot-preflight.js"; import { sendMessageNextcloudTalk, sendReactionNextcloudTalk } from "./send.js"; import type { CoreConfig } from "./types.js"; @@ -11,6 +13,9 @@ function createTalkConfig(baseUrl: string): CoreConfig { "nextcloud-talk": { baseUrl, botSecret: "test-secret", + apiUser: "test-admin", + apiPassword: "test-password", + webhookPublicUrl: "https://bot.example.test/hook", network: { dangerouslyAllowPrivateNetwork: true }, }, }, @@ -47,6 +52,69 @@ async function expectHangingTalkRequestTimesOut(params: { } describe("nextcloud-talk send error responses", () => { + it.each(["message", "reaction", "preflight"])( + "redacts reflected credentials and drops incomplete %s error bodies", + async (operation) => { + for (const mode of ["complete", "display-boundary", "oversized", "stalled"]) { + let credential = ""; + await withServer( + (request, response) => { + request.resume(); + credential = String( + request.headers["x-nextcloud-talk-bot-signature"] ?? + request.headers.authorization?.replace(/^Basic /, ""), + ); + expect(credential).not.toBe("undefined"); + response.writeHead(500, { "content-type": "text/plain" }); + // Put a secret across the read cap: exposing a prefix defeats exact redaction. + const body = + mode === "oversized" + ? `${"x".repeat(8192 - 16)}${credential}` + : `upstream rejected ${mode === "display-boundary" ? "x".repeat(160) : ""}${credential}; password=fixture-private-value${ + request.headers.authorization ? "; decoded test-password" : "" + }`; + if (mode === "stalled") { + response.write(body); + } else { + response.end(body); + } + }, + async (baseUrl) => { + const cfg = createTalkConfig(baseUrl); + const result = + operation === "preflight" + ? await probeNextcloudTalkBotResponseFeature({ + account: resolveNextcloudTalkAccount({ cfg }), + }) + : await ( + operation === "message" + ? sendMessageNextcloudTalk("room:abc123", "hello", { cfg }) + : sendReactionNextcloudTalk("room:abc123", "m-1", "ok", { cfg }) + ).catch((error: unknown) => error); + const message = + typeof result === "object" && result !== null && "message" in result + ? result.message + : undefined; + expect(message).toBeTypeOf("string"); + expect(message).not.toContain(credential); + expect(message).not.toContain(credential.slice(0, 16)); + expect(message).not.toContain("fixture-private-value"); + expect(message).not.toContain("test-password"); + if (mode === "oversized" || mode === "stalled") { + expect(message).not.toContain("xxxxxxxx"); + expect(message).not.toContain("upstream rejected"); + expect(message).toContain("500"); + } else { + expect(message).toContain("upstream rejected"); + expect(message).toContain("***"); + } + }, + ); + } + }, + 15_000, + ); + it("keeps send error body snippets UTF-16 safe", async () => { const prefix = "e".repeat(199); const errorBody = `${prefix}\u{1F600}tail`; diff --git a/extensions/nextcloud-talk/src/send.ts b/extensions/nextcloud-talk/src/send.ts index 933e117dd491..1bfd3d5442e9 100644 --- a/extensions/nextcloud-talk/src/send.ts +++ b/extensions/nextcloud-talk/src/send.ts @@ -1,15 +1,15 @@ // Nextcloud Talk plugin module implements send behavior. import { createMessageReceiptFromOutboundResults } from "openclaw/plugin-sdk/channel-outbound"; -import { - readProviderJsonResponse, - readResponseTextLimited, -} from "openclaw/plugin-sdk/provider-http"; +import { readProviderJsonResponse } from "openclaw/plugin-sdk/provider-http"; import { FormatCapabilityProfile, renderMarkdownWithMarkers, } from "openclaw/plugin-sdk/text-chunking"; import { truncateUtf16Safe } from "openclaw/plugin-sdk/text-utility-runtime"; -import { releaseNextcloudTalkGuardedResponse } from "./guarded-response.js"; +import { + readNextcloudTalkErrorBody, + releaseNextcloudTalkGuardedResponse, +} from "./guarded-response.js"; import { stripNextcloudTalkTargetPrefix } from "./normalize.js"; import { convertMarkdownTables, @@ -23,11 +23,6 @@ import { } from "./send.runtime.js"; import type { CoreConfig, NextcloudTalkSendResult } from "./types.js"; -// Nextcloud Talk runs against self-hosted servers whose responses are not -// trusted to be small. Cap error bodies so a hostile or misbehaving endpoint -// cannot stream an unbounded body into memory. (Success JSON is bounded by the -// shared readProviderJsonResponse helper.) -const NEXTCLOUD_TALK_ERROR_SNIPPET_MAX_BYTES = 8 * 1024; const NEXTCLOUD_TALK_ERROR_SNIPPET_MAX_CHARS = 200; const NEXTCLOUD_TALK_SEND_TIMEOUT_MS = 30_000; @@ -44,7 +39,7 @@ function renderNextcloudTalkMarkdown(markdown: string): string { ); } -/** Collapses whitespace and caps an error-body prefix to a short, log-safe snippet. */ +/** Collapses and caps an already-redacted error body for display. */ function collapseErrorSnippet(text: string): string { const collapsed = text.replace(/\s+/g, " ").trim(); if (collapsed.length > NEXTCLOUD_TALK_ERROR_SNIPPET_MAX_CHARS) { @@ -53,20 +48,6 @@ function collapseErrorSnippet(text: string): string { return collapsed; } -/** Reads a bounded, collapsed error-body snippet without buffering hostile responses. */ -async function readNextcloudTalkErrorSnippet(response: Response): Promise { - try { - // readResponseTextLimited caps the read at the byte budget and cancels the - // upstream stream once full, so a hostile endpoint cannot stream an - // unbounded body into memory. Collapse the bounded prefix locally to keep a - // short, log-safe error snippet (no new plugin SDK surface required). - const text = await readResponseTextLimited(response, NEXTCLOUD_TALK_ERROR_SNIPPET_MAX_BYTES); - return collapseErrorSnippet(text); - } catch { - return ""; - } -} - type NextcloudTalkSendOpts = { cfg: CoreConfig; baseUrl?: string; @@ -217,7 +198,7 @@ export async function sendMessageNextcloudTalk( try { if (!response.ok) { - const errorBody = await readNextcloudTalkErrorSnippet(response); + const errorBody = collapseErrorSnippet(await readNextcloudTalkErrorBody(response, signature)); const status = response.status; let errorMsg = `Nextcloud Talk send failed (${status})`; @@ -317,7 +298,7 @@ export async function sendReactionNextcloudTalk( try { if (!response.ok) { - const errorBody = await readNextcloudTalkErrorSnippet(response); + const errorBody = collapseErrorSnippet(await readNextcloudTalkErrorBody(response, signature)); throw new Error(`Nextcloud Talk reaction failed: ${response.status} ${errorBody}`.trim()); }