fix: check attachments a plugin adds to the composer and keep callbacks out of shouldShow

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<string, unknown>,
+  allow: true,
+}));
 vi.mock('@/components/plugins/plugin-slot', () => ({
   PluginSlot: ({ name, extraProps }: { name: string; extraProps?: Record<string, unknown> }) => {
     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(<EmailComposer onClose={vi.fn()} />);
+
+    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(<EmailComposer onClose={vi.fn()} />);
+
+    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(<EmailComposer onClose={vi.fn()} />);
     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

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<string, unknown>;
+  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<boolean> => {
+    // 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]);

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 } });
+  });
+});

lib/plugin-sandbox/host-bridge.ts

@@ -327,7 +327,10 @@ export class SandboxInstance {
     const p = new Promise<boolean>((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<string, unknown> = {};
+  for (const [k, v] of Object.entries(value as Record<string, unknown>)) {
+    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);
 }