From a2d217273fc1201ca44d425d9533e30e593563cf Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:24:56 -0700 Subject: [PATCH] fix(feedback): harden image delivery --- .../ipc/feedback-image-attachments.test.ts | 26 ++++--- src/main/ipc/feedback-image-attachments.ts | 36 ++++++---- src/main/ipc/feedback.test.ts | 67 +++++++++++++++++-- src/main/ipc/feedback.ts | 23 ++++--- .../sidebar/SidebarFeedbackDialog.test.tsx | 48 ++++++++++++- .../sidebar/SidebarFeedbackDialog.tsx | 4 +- .../SidebarFeedbackImageAttachments.tsx | 8 ++- src/renderer/src/i18n/locales/en.json | 2 +- src/renderer/src/i18n/locales/es.json | 2 +- src/renderer/src/i18n/locales/ja.json | 2 +- src/renderer/src/i18n/locales/ko.json | 2 +- src/renderer/src/i18n/locales/zh.json | 2 +- 12 files changed, 175 insertions(+), 47 deletions(-) diff --git a/src/main/ipc/feedback-image-attachments.test.ts b/src/main/ipc/feedback-image-attachments.test.ts index a4592afa9b1..187537cadcd 100644 --- a/src/main/ipc/feedback-image-attachments.test.ts +++ b/src/main/ipc/feedback-image-attachments.test.ts @@ -2,9 +2,9 @@ import { describe, expect, it } from 'vitest' import { MAX_FEEDBACK_IMAGE_BYTES, MAX_FEEDBACK_IMAGE_COUNT, + appendFeedbackImagesToFormData, feedbackImageFilename, isSupportedFeedbackImageContentType, - normalizeFeedbackImageBytes, validateFeedbackImages } from './feedback-image-attachments' @@ -27,9 +27,7 @@ describe('isSupportedFeedbackImageContentType', () => { 'rejects the inherited member %s', (contentType) => { expect(isSupportedFeedbackImageContentType(contentType)).toBe(false) - expect(validateFeedbackImages([image(contentType)])).toBe( - `Unsupported image type ${contentType}.` - ) + expect(validateFeedbackImages([image(contentType)])).toBe('Unsupported image type.') } ) }) @@ -49,15 +47,17 @@ describe('feedbackImageFilename', () => { }) }) -describe('normalizeFeedbackImageBytes', () => { - it('preserves exact typed-array bounds without copying the backing buffer', () => { +describe('appendFeedbackImagesToFormData', () => { + it('frames only the exact typed-array view', async () => { const backing = new Uint8Array([0, 1, 2, 3]) const view = backing.subarray(1, 3) + const formData = new FormData() - const normalized = normalizeFeedbackImageBytes(view) + appendFeedbackImagesToFormData(formData, [{ contentType: 'image/png', data: view }]) - expect(normalized).toBe(view) - expect([...normalized]).toEqual([1, 2]) + const blob = formData.get('feedbackImage') + expect(blob).toBeInstanceOf(Blob) + expect(new Uint8Array(await (blob as Blob).arrayBuffer())).toEqual(new Uint8Array([1, 2])) }) }) @@ -78,6 +78,14 @@ describe('validateFeedbackImages', () => { ).toBe(`Attach ${MAX_FEEDBACK_IMAGE_COUNT} images or fewer.`) }) + it('rejects malformed attachment collections and byte payloads', () => { + expect(validateFeedbackImages('image/png')).toBe('Image attachments must be a list.') + expect(validateFeedbackImages([null])).toBe('Invalid image attachment.') + expect(validateFeedbackImages([{ contentType: 'image/png', data: '8388608' }])).toBe( + 'Invalid image attachment bytes.' + ) + }) + it('rejects an empty attachment', () => { expect(validateFeedbackImages([image('image/png', 0)])).toBe('Image attachment is empty.') }) diff --git a/src/main/ipc/feedback-image-attachments.ts b/src/main/ipc/feedback-image-attachments.ts index 8c975164192..413c57fc4b9 100644 --- a/src/main/ipc/feedback-image-attachments.ts +++ b/src/main/ipc/feedback-image-attachments.ts @@ -1,3 +1,5 @@ +import { readFetchResponseJsonWithinLimit } from '../../shared/fetch-response-body' + // Why: mirrors the server allow-list. Slack picks a renderer from the filename // extension, so every accepted type needs one. const FEEDBACK_IMAGE_EXTENSIONS: Record = { @@ -9,6 +11,7 @@ const FEEDBACK_IMAGE_EXTENSIONS: Record = { export const MAX_FEEDBACK_IMAGE_COUNT = 4 export const MAX_FEEDBACK_IMAGE_BYTES = 8 * 1024 * 1024 +export const MAX_FEEDBACK_IMAGE_RESPONSE_BYTES = 64 * 1024 export const FEEDBACK_IMAGE_FORM_FIELD = 'feedbackImage' export type FeedbackImageAttachment = { @@ -26,24 +29,30 @@ export function getSupportedFeedbackImageContentTypes(): string[] { return Object.keys(FEEDBACK_IMAGE_EXTENSIONS) } -export function normalizeFeedbackImageBytes(data: Uint8Array): Uint8Array { - // Why: a Uint8Array already carries exact view bounds; copying it here adds - // another 32 MiB at the four-image cap before Blob framing copies it again. - return data instanceof Uint8Array ? data : new Uint8Array(data) -} - export function feedbackImageFilename(index: number, contentType: string): string { return `feedback-image-${index + 1}.${FEEDBACK_IMAGE_EXTENSIONS[contentType]}` } /** Defence in depth: the renderer validates first, but this channel is reachable directly. */ -export function validateFeedbackImages(images: FeedbackImageAttachment[]): string | null { +export function validateFeedbackImages(images: unknown): string | null { + if (!Array.isArray(images)) { + return 'Image attachments must be a list.' + } if (images.length > MAX_FEEDBACK_IMAGE_COUNT) { return `Attach ${MAX_FEEDBACK_IMAGE_COUNT} images or fewer.` } for (const image of images) { + if (!image || typeof image !== 'object') { + return 'Invalid image attachment.' + } + if (typeof image.contentType !== 'string') { + return 'Invalid image attachment content type.' + } if (!isSupportedFeedbackImageContentType(image.contentType)) { - return `Unsupported image type ${image.contentType}.` + return 'Unsupported image type.' + } + if (!(image.data instanceof Uint8Array)) { + return 'Invalid image attachment bytes.' } if (image.data.byteLength === 0) { return 'Image attachment is empty.' @@ -68,13 +77,16 @@ export function appendFeedbackImagesToFormData( } } -/** Older servers omit this field; any settled 2xx still confirms the feedback text landed. */ +/** A 2xx confirms the text; only the response contract can confirm its images. */ export async function readFeedbackImagesDelivered(response: Response): Promise { try { - const parsed: unknown = await response.json() + const parsed: unknown = await readFetchResponseJsonWithinLimit( + response, + MAX_FEEDBACK_IMAGE_RESPONSE_BYTES + ) if (typeof parsed === 'object' && parsed !== null && 'imagesDelivered' in parsed) { - return (parsed as { imagesDelivered?: unknown }).imagesDelivered !== false + return (parsed as { imagesDelivered?: unknown }).imagesDelivered === true } } catch {} - return true + return false } diff --git a/src/main/ipc/feedback.test.ts b/src/main/ipc/feedback.test.ts index a91ad78a342..b73873e5e96 100644 --- a/src/main/ipc/feedback.test.ts +++ b/src/main/ipc/feedback.test.ts @@ -16,6 +16,7 @@ vi.mock('electron', () => ({ net: { fetch: (...args: unknown[]) => fetchMock(...args) } })) +import { MAX_FEEDBACK_IMAGE_RESPONSE_BYTES } from './feedback-image-attachments' import { registerFeedbackHandlers, submitFeedback } from './feedback' function okResponse(): Response { @@ -446,16 +447,16 @@ describe('submitFeedback', () => { }) }) - it('treats a 2xx without the field as delivered so older servers still succeed', async () => { + it('reports unconfirmed delivery when a 2xx omits the image result', async () => { fetchMock.mockResolvedValue(jsonResponse({ ok: true })) await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ ok: true, - imagesDelivered: true + imagesDelivered: false }) }) - it('treats a settled non-JSON 2xx as delivered for legacy servers', async () => { + it('reports unconfirmed delivery for a settled non-JSON 2xx', async () => { fetchMock.mockResolvedValue({ ok: true, status: 202, @@ -466,7 +467,34 @@ describe('submitFeedback', () => { await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ ok: true, - imagesDelivered: true + imagesDelivered: false + }) + }) + + it('reports unconfirmed delivery when the response body aborts before the deadline', async () => { + fetchMock.mockResolvedValue({ + ok: true, + status: 202, + json: async () => { + throw new TypeError('terminated') + } + } as unknown as Response) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false + }) + expect(requestInit().signal).toMatchObject({ aborted: false }) + }) + + it('bounds the image-delivery response body', async () => { + fetchMock.mockResolvedValue( + new Response('x'.repeat(MAX_FEEDBACK_IMAGE_RESPONSE_BYTES + 1), { status: 202 }) + ) + + await expect(submitFeedback(imageSubmitArgs([pngImage()]))).resolves.toEqual({ + ok: true, + imagesDelivered: false }) }) @@ -514,7 +542,36 @@ describe('submitFeedback', () => { images: [{ contentType: 'constructor', data: new Uint8Array(4).fill(1) }] })) as { ok: boolean; error?: string } - expect(result).toMatchObject({ ok: false, error: 'Unsupported image type constructor.' }) + expect(result).toMatchObject({ ok: false, error: 'Unsupported image type.' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('rejects malformed IPC bytes before typed-array normalization', async () => { + registerFeedbackHandlers() + const result = (await handlers.get('feedback:submit')?.(null, { + feedback: 'images attached', + githubLogin: null, + githubEmail: null, + images: [{ contentType: 'image/png', data: '8388608' }] + })) as { ok: boolean; error?: string } + + expect(result).toMatchObject({ ok: false, error: 'Invalid image attachment bytes.' }) + expect(fetchMock).not.toHaveBeenCalled() + }) + + it('rejects oversized IPC batches before normalizing their entries', async () => { + registerFeedbackHandlers() + const result = (await handlers.get('feedback:submit')?.(null, { + feedback: 'images attached', + githubLogin: null, + githubEmail: null, + images: Array.from({ length: 5 }, () => ({ + contentType: 'image/png', + data: '8388608' + })) + })) as { ok: boolean; error?: string } + + expect(result).toMatchObject({ ok: false, error: 'Attach 4 images or fewer.' }) expect(fetchMock).not.toHaveBeenCalled() }) diff --git a/src/main/ipc/feedback.ts b/src/main/ipc/feedback.ts index 635ccd595ff..f0ec8dbe088 100644 --- a/src/main/ipc/feedback.ts +++ b/src/main/ipc/feedback.ts @@ -2,7 +2,6 @@ import os from 'node:os' import { app, ipcMain, net } from 'electron' import { appendFeedbackImagesToFormData, - normalizeFeedbackImageBytes, readFeedbackImagesDelivered, validateFeedbackImages, type FeedbackImageAttachment @@ -285,7 +284,7 @@ export async function submitFeedback( ): Promise { // Why: buildSubmitBody drops images on the crash lane, so validating them // there would abort a crash report over attachments it never meant to send. - if (args.submissionType !== 'crash' && args.images?.length) { + if (args.submissionType !== 'crash' && args.images !== undefined) { const imageError = validateFeedbackImages(args.images) if (imageError) { return { ok: false, status: null, error: imageError } @@ -343,16 +342,20 @@ export async function submitFeedback( export function registerFeedbackHandlers(): void { ipcMain.removeHandler('feedback:submit') - ipcMain.handle('feedback:submit', (_event, args: FeedbackSubmitArgs) => + ipcMain.handle('feedback:submit', (_event, args: FeedbackSubmitArgs) => { + // Why: validate the raw clone before normalization so a tiny hostile value + // cannot become a large main-process typed-array allocation. + if (args.images !== undefined) { + const imageError = validateFeedbackImages(args.images) + if (imageError) { + return { ok: false, status: null, error: imageError } + } + } // Why: crash submissions are main-only. A compromised renderer can invoke // this channel directly, so force the public feedback lane at the boundary. - submitFeedback({ + return submitFeedback({ ...args, - submissionType: 'feedback', - images: args.images?.map((image) => ({ - contentType: image.contentType, - data: normalizeFeedbackImageBytes(image.data) - })) + submissionType: 'feedback' }) - ) + }) } diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx index c9eab01cdcf..bc53ad52b9f 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.test.tsx @@ -74,6 +74,14 @@ afterEach(() => { }) describe('SidebarFeedbackDialog image submission', () => { + it('keeps the dialog scrollable within short windows', () => { + const { container } = render() + const content = container.querySelector('.scrollbar-sleek') + + expect(content?.className).toContain('max-h-[calc(100vh-3rem)]') + expect(content?.className).toContain('overflow-y-auto') + }) + it('waits for in-flight image reads before enabling Send', async () => { let finishRead: ((value: unknown) => void) | undefined mocks.readFeedbackImageFiles.mockReturnValue( @@ -112,7 +120,9 @@ describe('SidebarFeedbackDialog image submission', () => { }) await waitFor(() => expect((send as HTMLButtonElement).disabled).toBe(false)) - expect(screen.getByRole('button', { name: 'Remove shot.png' })).toBeTruthy() + const remove = screen.getByRole('button', { name: 'Remove shot.png' }) + expect(remove.dataset.slot).toBe('button') + expect(remove.dataset.size).toBe('icon-xs') fireEvent.click(send) await waitFor(() => expect(mocks.submit).toHaveBeenCalledTimes(1)) expect(mocks.submit.mock.calls[0]?.[0].images).toEqual([ @@ -120,6 +130,42 @@ describe('SidebarFeedbackDialog image submission', () => { ]) }) + it('warns when the server cannot confirm image delivery', async () => { + mocks.readFeedbackImageFiles.mockResolvedValue({ + images: [ + { + id: 'shot', + name: 'shot.png', + contentType: 'image/png', + bytes: 1, + data: new Uint8Array([1]), + previewUrl: 'blob:shot' + } + ], + errors: [] + }) + mocks.submit.mockResolvedValue({ ok: true, imagesDelivered: false }) + const onOpenChange = vi.fn() + const { container } = render() + fireEvent.change(screen.getByPlaceholderText('What could we improve?'), { + target: { value: 'Screenshot attached' } + }) + const input = container.querySelector('input[type="file"]') + fireEvent.change(input!, { + target: { files: [new File(['x'], 'shot.png', { type: 'image/png' })] } + }) + await screen.findByRole('button', { name: 'Remove shot.png' }) + + fireEvent.click(screen.getByRole('button', { name: 'Send' })) + + await waitFor(() => + expect(mocks.toastWarning).toHaveBeenCalledWith( + 'Feedback sent, but image delivery could not be confirmed.' + ) + ) + expect(onOpenChange).toHaveBeenCalledWith(false) + }) + it('does not consume text when the pasted image cannot be attached', () => { mocks.readFeedbackImageFiles.mockResolvedValue({ images: [], diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx index 8268a04a50c..783c8dff058 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackDialog.tsx @@ -229,7 +229,7 @@ export function SidebarFeedbackDialog({ toast.warning( translate( 'auto.components.sidebar.SidebarFeedbackDialog.imagesNotDelivered', - 'Feedback sent, but the images could not be attached.' + 'Feedback sent, but image delivery could not be confirmed.' ) ) } else { @@ -266,7 +266,7 @@ export function SidebarFeedbackDialog({ { event.preventDefault() feedbackTextareaRef.current?.focus() diff --git a/src/renderer/src/components/sidebar/SidebarFeedbackImageAttachments.tsx b/src/renderer/src/components/sidebar/SidebarFeedbackImageAttachments.tsx index e06c904e84a..5a726000bae 100644 --- a/src/renderer/src/components/sidebar/SidebarFeedbackImageAttachments.tsx +++ b/src/renderer/src/components/sidebar/SidebarFeedbackImageAttachments.tsx @@ -80,8 +80,10 @@ export function SidebarFeedbackImageAttachments({ alt={image.name} className="size-14 rounded border border-border object-cover" /> - + {formatFeedbackImageSize(image.bytes)} diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index 82f36e8d41a..a32b4cce24f 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -4228,7 +4228,7 @@ "69969ba364": "Sending…", "imageReadFailed": "Could not read the attached images. Try attaching them again.", "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", - "imagesNotDelivered": "Feedback sent, but the images could not be attached." + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "Add project", diff --git a/src/renderer/src/i18n/locales/es.json b/src/renderer/src/i18n/locales/es.json index 763449df30d..5f82047797c 100644 --- a/src/renderer/src/i18n/locales/es.json +++ b/src/renderer/src/i18n/locales/es.json @@ -4190,7 +4190,7 @@ "69969ba364": "Envío…", "imageReadFailed": "Could not read the attached images. Try attaching them again.", "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", - "imagesNotDelivered": "Feedback sent, but the images could not be attached." + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "Agregar proyecto", diff --git a/src/renderer/src/i18n/locales/ja.json b/src/renderer/src/i18n/locales/ja.json index df7dac80988..a1feb6a4073 100644 --- a/src/renderer/src/i18n/locales/ja.json +++ b/src/renderer/src/i18n/locales/ja.json @@ -4171,7 +4171,7 @@ "69969ba364": "送信中…", "imageReadFailed": "Could not read the attached images. Try attaching them again.", "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", - "imagesNotDelivered": "Feedback sent, but the images could not be attached." + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "プロジェクトの追加", diff --git a/src/renderer/src/i18n/locales/ko.json b/src/renderer/src/i18n/locales/ko.json index 424cace63db..64c1a31a05a 100644 --- a/src/renderer/src/i18n/locales/ko.json +++ b/src/renderer/src/i18n/locales/ko.json @@ -4171,7 +4171,7 @@ "69969ba364": "전송 중…", "imageReadFailed": "Could not read the attached images. Try attaching them again.", "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", - "imagesNotDelivered": "Feedback sent, but the images could not be attached." + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "프로젝트 추가", diff --git a/src/renderer/src/i18n/locales/zh.json b/src/renderer/src/i18n/locales/zh.json index b7fb100e2f5..8317b3c6b76 100644 --- a/src/renderer/src/i18n/locales/zh.json +++ b/src/renderer/src/i18n/locales/zh.json @@ -4171,7 +4171,7 @@ "69969ba364": "正在发送...", "imageReadFailed": "Could not read the attached images. Try attaching them again.", "attachWhileSending": "Wait for the current feedback to finish sending before attaching more images.", - "imagesNotDelivered": "Feedback sent, but the images could not be attached." + "imagesNotDelivered": "Feedback sent, but image delivery could not be confirmed." }, "SidebarFilter": { "e3b3898218": "添加项目",