From 8739432d4ce6e321c2881ae337339bf0c1910c92 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Fri, 14 Aug 2026 21:10:48 -0700 Subject: [PATCH] fix(ui,gateway): single attachment admission funnel + WS-frame-budget clamp on advertised ceiling (#123977) * fix(ui,gateway): make attachment admission one funnel and clamp advertised ceiling to the WS frame budget Three residual gaps from the #123654 size-limit fix shared one root cause: attachment admission policy was scattered instead of owned. - hello-ok advertised the decoded config ceiling (20MB default, higher with mediaMaxMb) without accounting for base64 4/3 expansion against the 25MiB WS frame cap, so a 19.6-20MB attachment passed the client guard and the encoded chat.send frame still hard-dropped the connection (1009) for every pane. The policy owner now clamps the advertised maxBytes to what one frame can carry. - Large-text paste, data-URL image paste, and browser-annotation handoff constructed attachments without any size check, bypassing the guard that only lived inline in appendAttachmentFiles. All intake paths now share one admission funnel (chat-attachment-admission.ts). - Zero-byte files rendered a normal chip, then the payload assembler silently dropped them on send; the funnel rejects them at intake with a named toast. * refactor(ui): drop unused exported type from attachment admission module --- src/gateway/chat-attachment-policy.test.ts | 17 ++++- src/gateway/chat-attachment-policy.ts | 12 +++- .../chat/chat-pane-browser-annotation.ts | 6 +- .../components/chat-attachment-admission.ts | 39 ++++++++++++ .../chat/components/chat-attachments.test.ts | 62 +++++++++++++++++++ .../pages/chat/components/chat-attachments.ts | 46 ++++++-------- 6 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 ui/src/pages/chat/components/chat-attachment-admission.ts diff --git a/src/gateway/chat-attachment-policy.test.ts b/src/gateway/chat-attachment-policy.test.ts index 35f3ba32cc20..692d95a9e3f4 100644 --- a/src/gateway/chat-attachment-policy.test.ts +++ b/src/gateway/chat-attachment-policy.test.ts @@ -46,14 +46,27 @@ describe("resolveChatAttachmentMaxBytes", () => { }); }); +// Frame budget mirrored from the policy module: base64 expands 4/3 and the +// JSON envelope needs slack, so the advertised ceiling must fit one WS frame. +const MAX_ADVERTISED_BYTES = Math.floor(((25 * MB - 256 * 1024) * 3) / 4); + describe("resolveChatAttachmentPolicy", () => { it("advertises the configured ceiling with the image hydration cap applied", () => { - expect(resolveChatAttachmentPolicy(cfgWithMediaMaxMb(20))).toEqual({ - maxBytes: 20 * MB, + expect(resolveChatAttachmentPolicy(cfgWithMediaMaxMb(10))).toEqual({ + maxBytes: 10 * MB, maxImageBytes: MAX_IMAGE_BYTES, }); }); + it("clamps the advertised ceiling to what one WS frame can carry as base64", () => { + // The 20MB default and any raised mediaMaxMb both exceed the frame budget: + // advertising them would let the client encode a frame the server + // hard-drops with 1009. + expect(resolveChatAttachmentPolicy({} as OpenClawConfig).maxBytes).toBe(MAX_ADVERTISED_BYTES); + expect(resolveChatAttachmentPolicy(cfgWithMediaMaxMb(50)).maxBytes).toBe(MAX_ADVERTISED_BYTES); + expect(MAX_ADVERTISED_BYTES).toBeLessThan(20 * MB); + }); + it("clamps maxImageBytes to the configured ceiling when it is the smaller limit", () => { expect(resolveChatAttachmentPolicy(cfgWithMediaMaxMb(1))).toEqual({ maxBytes: MB, diff --git a/src/gateway/chat-attachment-policy.ts b/src/gateway/chat-attachment-policy.ts index 0332f41f2701..e9423b95d4a4 100644 --- a/src/gateway/chat-attachment-policy.ts +++ b/src/gateway/chat-attachment-policy.ts @@ -3,9 +3,19 @@ // does not pull the media probe/store graph in just to read two numbers. import { MAX_IMAGE_BYTES } from "@openclaw/media-core/constants"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { MAX_PAYLOAD_BYTES } from "./server-constants.js"; const DEFAULT_CHAT_ATTACHMENT_MAX_MB = 20; +// A chat.send frame carries attachments as base64 (4/3 expansion) plus the +// JSON envelope and message text. Advertising more than one WS frame can carry +// lets the client encode a payload the server hard-drops with 1009 for every +// pane — the exact failure the hello-ok policy exists to prevent. +const WS_FRAME_ENVELOPE_SLACK_BYTES = 256 * 1024; +const MAX_ADVERTISED_ATTACHMENT_BYTES = Math.floor( + ((MAX_PAYLOAD_BYTES - WS_FRAME_ENVELOPE_SLACK_BYTES) * 3) / 4, +); + /** Default decoded-size ceiling when `agents.defaults.mediaMaxMb` is unset or invalid. */ export const DEFAULT_CHAT_ATTACHMENT_MAX_BYTES = DEFAULT_CHAT_ATTACHMENT_MAX_MB * 1024 * 1024; @@ -37,6 +47,6 @@ type ChatAttachmentPolicy = { * cannot be stated once per connection. */ export function resolveChatAttachmentPolicy(cfg: OpenClawConfig): ChatAttachmentPolicy { - const maxBytes = resolveChatAttachmentMaxBytes(cfg); + const maxBytes = Math.min(resolveChatAttachmentMaxBytes(cfg), MAX_ADVERTISED_ATTACHMENT_BYTES); return { maxBytes, maxImageBytes: Math.min(maxBytes, MAX_IMAGE_BYTES) }; } diff --git a/ui/src/pages/chat/chat-pane-browser-annotation.ts b/ui/src/pages/chat/chat-pane-browser-annotation.ts index 532bc973d6e6..4d4b7555f1e3 100644 --- a/ui/src/pages/chat/chat-pane-browser-annotation.ts +++ b/ui/src/pages/chat/chat-pane-browser-annotation.ts @@ -40,7 +40,11 @@ export function receiveBrowserAnnotation( (event as BrowserAnnotationEvent).rejection = "limit"; return false; } - const attachment = chatAttachmentFromDataUrl(detail.dataUrl, detail.fileName || "annotation"); + const attachment = chatAttachmentFromDataUrl( + detail.dataUrl, + detail.fileName || "annotation", + state.hello?.policy?.attachments, + ); if (!attachment) { return false; } diff --git a/ui/src/pages/chat/components/chat-attachment-admission.ts b/ui/src/pages/chat/components/chat-attachment-admission.ts new file mode 100644 index 000000000000..ba4053ea3257 --- /dev/null +++ b/ui/src/pages/chat/components/chat-attachment-admission.ts @@ -0,0 +1,39 @@ +// Single admission funnel for every composer attachment intake path (file +// input, drop, image paste, large-text paste, data-URL paste, annotation +// handoff). Enforces the hello-advertised decoded-size ceilings before +// encoding — an oversized base64 frame would exceed the gateway's WS payload +// cap and hard-drop the whole connection (1009) for every pane — and rejects +// zero-byte files, which the payload assembler would otherwise drop silently +// after send. +import { t } from "../../../i18n/index.ts"; +import { showToast } from "../../../lib/toast.ts"; + +function skippedFilesToast(messageKey: string, skipped: readonly File[]): void { + if (skipped.length === 0) { + return; + } + showToast({ + message: t(messageKey, { + names: skipped + .slice(0, 3) + .map((file) => file.name) + .join(", "), + more: skipped.length > 3 ? ` +${skipped.length - 3}` : "", + }), + }); +} + +export function admitAttachmentFiles( + candidates: readonly File[], + limits: { maxBytes: number; maxImageBytes: number } | undefined, +): File[] { + const fileLimit = (file: File) => + file.type.startsWith("image/") ? limits?.maxImageBytes : limits?.maxBytes; + const empty = candidates.filter((file) => file.size === 0); + const oversized = candidates.filter( + (file) => file.size > 0 && limits !== undefined && file.size > (fileLimit(file) ?? Infinity), + ); + skippedFilesToast("chat.attachments.readFailed", empty); + skippedFilesToast("chat.attachments.tooLarge", oversized); + return candidates.filter((file) => !empty.includes(file) && !oversized.includes(file)); +} diff --git a/ui/src/pages/chat/components/chat-attachments.test.ts b/ui/src/pages/chat/components/chat-attachments.test.ts index cce9c68749c0..b0592961ba5c 100644 --- a/ui/src/pages/chat/components/chat-attachments.test.ts +++ b/ui/src/pages/chat/components/chat-attachments.test.ts @@ -121,6 +121,68 @@ describe("chat attachment read failures", () => { expect(onAttachmentsChange).not.toHaveBeenCalled(); }); + it("rejects a zero-byte file instead of silently dropping it after send", async () => { + const onAttachmentsChange = vi.fn(); + handleChatAttachmentPaste( + pasteEventWithFiles([new File([], "empty.png", { type: "image/png" })]), + { attachments: [], onAttachmentsChange }, + ); + await toastHost.updateComplete; + await vi.waitFor(() => { + expect(toastHost.querySelector(".app-toast__message")?.textContent).toContain("empty.png"); + }); + expect(onAttachmentsChange).not.toHaveBeenCalled(); + }); + + it("blocks a large text paste that exceeds the non-image ceiling", async () => { + const onAttachmentsChange = vi.fn(); + const text = "x".repeat(2048); + handleChatAttachmentPaste( + { + preventDefault: () => {}, + clipboardData: { + items: [], + getData: (type: string) => (type === "text/plain" ? text : ""), + }, + } as unknown as ClipboardEvent, + { + attachmentLimits: { maxBytes: 1024, maxImageBytes: 1024 }, + attachments: [], + onAttachmentsChange, + }, + ); + await toastHost.updateComplete; + await vi.waitFor(() => { + expect(toastHost.querySelector(".app-toast__message")?.textContent).toContain("pasted-text"); + }); + expect(onAttachmentsChange).not.toHaveBeenCalled(); + }); + + it("blocks a pasted data-URL image that exceeds the image ceiling", async () => { + const onAttachmentsChange = vi.fn(); + const bigBase64 = btoa("p".repeat(64)); + handleChatAttachmentPaste( + { + preventDefault: () => {}, + clipboardData: { + items: [], + getData: (type: string) => + type === "text/plain" ? `data:image/png;base64,${bigBase64}` : "", + }, + } as unknown as ClipboardEvent, + { + attachmentLimits: { maxBytes: 1024, maxImageBytes: 16 }, + attachments: [], + onAttachmentsChange, + }, + ); + await toastHost.updateComplete; + await vi.waitFor(() => { + expect(toastHost.querySelector(".app-toast__message")?.textContent).toContain("pasted-image"); + }); + expect(onAttachmentsChange).not.toHaveBeenCalled(); + }); + it("does not toast when every read succeeds", async () => { const onAttachmentsChange = vi.fn(); handleChatAttachmentPaste( diff --git a/ui/src/pages/chat/components/chat-attachments.ts b/ui/src/pages/chat/components/chat-attachments.ts index 62c15305bdc8..95ad094dd3e5 100644 --- a/ui/src/pages/chat/components/chat-attachments.ts +++ b/ui/src/pages/chat/components/chat-attachments.ts @@ -15,6 +15,7 @@ import { registerChatAttachmentPayload, releaseChatAttachmentPayload, } from "../attachment-payload-store.ts"; +import { admitAttachmentFiles } from "./chat-attachment-admission.ts"; const CHAT_ATTACHMENT_ACCEPT = "image/*,audio/*,video/*,application/pdf,text/*,.csv,.json,.md,.txt,.zip," + @@ -138,10 +139,7 @@ function encodeTextAsDataUrl(text: string): string { return `data:${LARGE_PASTE_TEXT_MIME_TYPE};base64,${btoa(chunks.join(""))}`; } -function createLargePastedTextAttachment(text: string): ChatAttachment { - const file = new File([text], `${LARGE_PASTE_TEXT_FILE_PREFIX}${Date.now()}.txt`, { - type: LARGE_PASTE_TEXT_MIME_TYPE, - }); +function createLargePastedTextAttachment(text: string, file: File): ChatAttachment { const attachment = chatAttachmentFromFile(file, encodeTextAsDataUrl(text)); largePastedTextAttachments.add(attachment); const preview = compactPastedTextPreview(text); @@ -210,7 +208,14 @@ function handleLargeTextPaste(e: ClipboardEvent, props: ChatAttachmentControlsPr return false; } e.preventDefault(); - const attachment = createLargePastedTextAttachment(text); + const file = new File([text], `${LARGE_PASTE_TEXT_FILE_PREFIX}${Date.now()}.txt`, { + type: LARGE_PASTE_TEXT_MIME_TYPE, + }); + if (admitAttachmentFiles([file], props.attachmentLimits).length === 0) { + // The rejection toast named the file; the clipboard still holds the text. + return true; + } + const attachment = createLargePastedTextAttachment(text, file); props.onAttachmentsChange([...currentAttachments(props), attachment]); return true; } @@ -249,10 +254,14 @@ function dataImageClipboardFile( export function chatAttachmentFromDataUrl( dataUrl: string, fileName: string, + limits?: ChatAttachmentControlsProps["attachmentLimits"], ): ChatAttachment | null { const baseName = fileName.replace(/\.[a-z0-9]+$/i, "") || "image"; const parsed = dataImageClipboardFile(dataUrl, baseName); - return parsed ? chatAttachmentFromFile(parsed.file, parsed.dataUrl) : null; + if (!parsed || admitAttachmentFiles([parsed.file], limits).length === 0) { + return null; + } + return chatAttachmentFromFile(parsed.file, parsed.dataUrl); } function readAttachmentFile( @@ -301,27 +310,7 @@ async function appendAttachmentFiles( if (!props.onAttachmentsChange || candidates.length === 0) { return; } - // Enforce the hello-advertised decoded-size ceilings up front: an oversized - // base64 frame would exceed the gateway's WS payload cap and hard-drop the - // whole connection (1009) for every pane, so it must never start encoding. - const limits = props.attachmentLimits; - const fileLimit = (file: File) => - file.type.startsWith("image/") ? limits?.maxImageBytes : limits?.maxBytes; - const oversized = limits - ? candidates.filter((file) => file.size > (fileLimit(file) ?? Infinity)) - : []; - if (oversized.length > 0) { - showToast({ - message: t("chat.attachments.tooLarge", { - names: oversized - .slice(0, 3) - .map((file) => file.name) - .join(", "), - more: oversized.length > 3 ? ` +${oversized.length - 3}` : "", - }), - }); - } - const files = limits ? candidates.filter((file) => !oversized.includes(file)) : [...candidates]; + const files = admitAttachmentFiles(candidates, props.attachmentLimits); if (files.length === 0) { return; } @@ -378,6 +367,9 @@ export function handleChatAttachmentPaste(e: ClipboardEvent, props: ChatAttachme return; } e.preventDefault(); + if (admitAttachmentFiles([pasted.file], props.attachmentLimits).length === 0) { + return; + } props.onAttachmentsChange([ ...currentAttachments(props), chatAttachmentFromFile(pasted.file, pasted.dataUrl),