From 3bfc0e69d05bfac1683458545fcebb115447e187 Mon Sep 17 00:00:00 2001 From: Linus Rath <139418639+rathlinus@users.noreply.github.com> Date: Wed, 14 Oct 2026 02:58:47 +0200 Subject: [PATCH] fix: check attachments a plugin adds to the composer and keep callbacks out of shouldShow --- ...composer-plugin-attachment-source.test.tsx | 33 +++++++++++-- components/email/email-composer.tsx | 48 ++++++++++++++----- .../plugin-host-bridge-origin.test.ts | 13 +++++ lib/plugin-sandbox/host-bridge.ts | 18 ++++++- 4 files changed, 97 insertions(+), 15 deletions(-) diff --git a/components/email/__tests__/composer-plugin-attachment-source.test.tsx b/components/email/__tests__/composer-plugin-attachment-source.test.tsx index c95edad1b..03af74276 100644 --- a/components/email/__tests__/composer-plugin-attachment-source.test.tsx +++ b/components/email/__tests__/composer-plugin-attachment-source.test.tsx @@ -19,6 +19,10 @@ vi.mock('@/components/email/rich-text-editor', () => ({ vi.mock('@/hooks/use-plugin-slot-offers', () => ({ usePluginSlotOffers: (slot: string) => (slot === 'composer-attachment-source' ? [{ pluginId: 'p', order: 100 }] : []), })); +const attachState = vi.hoisted(() => ({ + next: { blobId: 'cloud-blob-1', name: 'vacation.jpg', type: 'image/jpeg', size: 42_000 } as Record, + allow: true, +})); vi.mock('@/components/plugins/plugin-slot', () => ({ PluginSlot: ({ name, extraProps }: { name: string; extraProps?: Record }) => { if (name !== 'composer-attachment-source') return null; @@ -30,8 +34,7 @@ vi.mock('@/components/plugins/plugin-slot', () => ({ { type: 'button', 'data-testid': 'fake-cloud-storage-plugin', - onClick: () => - onAttach?.({ blobId: 'cloud-blob-1', name: 'vacation.jpg', type: 'image/jpeg', size: 42_000 }), + onClick: () => onAttach?.(attachState.next as never), }, 'Attach from cloud storage', ); @@ -169,6 +172,7 @@ vi.mock('@/lib/plugin-hooks', () => ({ onBeforeEmailSend: { intercept: async () => true, interceptOutcome: async () => 'allowed' }, onComposeSend: { takeOver: async () => 'default' }, onTransformOutgoingEmail: { transform: async (email: unknown) => email }, + onBeforeAttachmentUpload: { intercept: async () => attachState.allow }, }, contactHooks: { search: { call: async () => [] }, @@ -212,6 +216,29 @@ describe('composer-attachment-source plugin slot', () => { afterEach(() => { useAuthStore.setState({ client: null }); vi.clearAllMocks(); + attachState.next = { blobId: 'cloud-blob-1', name: 'vacation.jpg', type: 'image/jpeg', size: 42_000 }; + attachState.allow = true; + }); + + it('ignores something from a plugin that is not an attachment', async () => { + attachState.next = { blobId: 'not a blob id', name: 'evil.jpg', type: 'image/jpeg', size: 1 }; + render(); + + fireEvent.click(screen.getByTestId('fake-cloud-storage-plugin')); + + await new Promise((r) => setTimeout(r, 50)); + expect(screen.queryByText('evil.jpg')).not.toBeInTheDocument(); + }); + + it('applies the same attachment checks as a local file', async () => { + // A plugin's onBeforeAttachmentUpload guard used to be skipped here. + attachState.allow = false; + render(); + + fireEvent.click(screen.getByTestId('fake-cloud-storage-plugin')); + + await new Promise((r) => setTimeout(r, 50)); + expect(screen.queryByText('vacation.jpg')).not.toBeInTheDocument(); }); it('adds a plugin-supplied blob as an attachment chip when onAttach is called', async () => { @@ -235,7 +262,7 @@ describe('composer-attachment-source plugin slot', () => { render(); fireEvent.click(screen.getByTestId('fake-cloud-storage-plugin')); - expect(screen.getByText('vacation.jpg')).toBeInTheDocument(); + expect(await screen.findByText('vacation.jpg')).toBeInTheDocument(); // Fake timers only from here on - the autosave debounce below needs // vi.advanceTimersByTimeAsync, but findBy*'s own internal polling (not diff --git a/components/email/email-composer.tsx b/components/email/email-composer.tsx index e91bb0cd1..cb045cb38 100644 --- a/components/email/email-composer.tsx +++ b/components/email/email-composer.tsx @@ -324,6 +324,22 @@ function getDefaultScheduleValue(): string { // the body instead (#1114). const PINNED_FIELDS_MIN_BODY_HEIGHT = 200; +/** + * An attachment a plugin uploaded itself, as the composer stores it, or null + * when the plugin sent something that isn't one. + */ +export function normalizePluginAttachment(raw: unknown): { name: string; type: string; size: number; blobId: string } | null { + if (!raw || typeof raw !== 'object') return null; + const r = raw as Record; + if (typeof r.blobId !== 'string' || !/^[A-Za-z0-9_-]{1,255}$/.test(r.blobId)) return null; + if (typeof r.size !== 'number' || !Number.isFinite(r.size) || r.size < 0) return null; + // eslint-disable-next-line no-control-regex -- removing control characters is the point + const name = typeof r.name === 'string' ? r.name.replace(/[\u0000-\u001f\u007f]/g, '').trim().slice(0, 255) : ''; + if (!name) return null; + const type = typeof r.type === 'string' && /^[\w.+-]+\/[\w.+-]+$/.test(r.type) ? r.type : 'application/octet-stream'; + return { name, type, size: Math.floor(r.size), blobId: r.blobId }; +} + export function EmailComposer({ onSend, onScheduledSendCreated, @@ -1936,17 +1952,27 @@ export function EmailComposer({ // offers "attach from there". No `file` is set, matching the existing // draft-part hydration path above (a blobId-only attachment is already a // supported shape, just never previously reachable from a plugin). - const handlePluginAttachmentAdd = useCallback((upload: PluginAttachmentUpload) => { - setAttachments(prev => [ - ...prev, - { - name: upload.name, - type: upload.type, - size: upload.size, - blobId: upload.blobId, - }, - ]); - }, []); + const handlePluginAttachmentAdd = useCallback(async (raw: PluginAttachmentUpload): Promise => { + // The values come from a plugin: check them, and run the same intercept + // and size limit every other way of attaching a file goes through. + const upload = normalizePluginAttachment(raw); + if (!upload) { + debug.warn('email', 'Refused a malformed attachment from a plugin', raw); + return false; + } + const ok = await emailHooks.onBeforeAttachmentUpload.intercept({ + name: upload.name, type: upload.type, size: upload.size, + }); + if (!ok) return false; + const maxTotal = composerClient?.getMaxSizeAttachmentsPerEmail?.() ?? 0; + const total = attachmentsRef.current.reduce((sum, att) => sum + (att.size || 0), 0); + if (maxTotal > 0 && total + upload.size > maxTotal) { + toast.error(t('attachments_total_too_large', { max: formatFileSize(maxTotal) })); + return false; + } + setAttachments(prev => [...prev, upload]); + return true; + }, [composerClient, t]); // Stable props, so a re-render of the composer doesn't post them to the // plugin's iframe again. const pluginAttachmentSourceProps = useMemo(() => ({ onAttach: handlePluginAttachmentAdd }), [handlePluginAttachmentAdd]); diff --git a/lib/__tests__/plugin-host-bridge-origin.test.ts b/lib/__tests__/plugin-host-bridge-origin.test.ts index 126e247f9..5dd2d1598 100644 --- a/lib/__tests__/plugin-host-bridge-origin.test.ts +++ b/lib/__tests__/plugin-host-bridge-origin.test.ts @@ -111,3 +111,16 @@ describe('slot sizing', () => { expect(inst.iframe.style.width).toBe('100%'); }); }); + +describe('shouldShow context', () => { + it('leaves host callbacks out instead of failing to post them', async () => { + const inst = createBackgroundInstance({ plugin: plugin(), code: '', locale: 'en', tier: 'untrusted' }); + instances.push(inst); + const sent = vi.spyOn(inst.iframe.contentWindow!, 'postMessage'); + + void inst.evaluateShouldShow('composer-attachment-source', { onAttach: () => {}, nested: { cb: () => {}, n: 1 } }); + + const msg = sent.mock.calls.map(([m]) => m as { type: string; context?: unknown }).find((m) => m.type === 'slot-should-show'); + expect(msg?.context).toEqual({ nested: { n: 1 } }); + }); +}); diff --git a/lib/plugin-sandbox/host-bridge.ts b/lib/plugin-sandbox/host-bridge.ts index 721fcf342..ac2c9bcad 100644 --- a/lib/plugin-sandbox/host-bridge.ts +++ b/lib/plugin-sandbox/host-bridge.ts @@ -327,7 +327,10 @@ export class SandboxInstance { const p = new Promise((resolve) => { this.pendingShouldShow.set(id, resolve); }); - this.send({ type: 'slot-should-show', id, slot, context }); + // shouldShow only decides; it gets the data, not the host callbacks. + // Functions can't be posted at all - sending them threw and took the + // composer or event form down with it. + this.send({ type: 'slot-should-show', id, slot, context: withoutFunctions(context) }); return p; } @@ -372,6 +375,19 @@ export class SandboxInstance { } } +/** Copy of a props object with every function left out. */ +export function withoutFunctions(value: unknown, depth = 0): unknown { + if (depth > 6 || typeof value === 'function') return undefined; + if (value === null || typeof value !== 'object') return value; + if (Array.isArray(value)) return value.map((v) => withoutFunctions(v, depth + 1)); + const out: Record = {}; + for (const [k, v] of Object.entries(value as Record)) { + if (typeof v === 'function') continue; + out[k] = withoutFunctions(v, depth + 1); + } + return out; +} + function uid(): string { return Math.random().toString(36).slice(2) + Date.now().toString(36); }