diff --git a/.changeset/attachment-error-envelope.md b/.changeset/attachment-error-envelope.md new file mode 100644 index 000000000..54d9e96cd --- /dev/null +++ b/.changeset/attachment-error-envelope.md @@ -0,0 +1,21 @@ +--- +"@object-ui/app-shell": patch +--- + +fix(attachments): read the storage service's new error envelope so gated downloads keep their friendly copy (objectstack#3675) + +`RecordAttachmentsPanel` mapped the server's fail-closed 40x codes +(`AUTH_REQUIRED`, `ATTACHMENT_DOWNLOAD_DENIED`) to human copy by reading +`code` off the top level of the error body. The storage service has moved that +code into the envelope its contract declares — +`{ success: false, error: { code, message } }` — so the top-level read now +returns `undefined`, and every gated download would have degraded from +"You don't have access to download this attachment." to the generic +"Download failed (403)". + +The download handler now reads `body?.error?.code ?? body?.code`, mirroring how +the success branch two lines below already reads `body?.url ?? body?.data?.url`. +Both dialects on purpose: the console ships independently of the server it +talks to, so a current console must keep understanding an older one. A test +covers each shape, and the fix is mutation-checked — dropping the nested read +fails the two new-envelope cases. diff --git a/packages/app-shell/src/views/RecordAttachmentsPanel.tsx b/packages/app-shell/src/views/RecordAttachmentsPanel.tsx index b44c33add..54cbad5ef 100644 --- a/packages/app-shell/src/views/RecordAttachmentsPanel.tsx +++ b/packages/app-shell/src/views/RecordAttachmentsPanel.tsx @@ -211,7 +211,17 @@ export const RecordAttachmentsPanel: React.FC = ({ if (!res.ok) { let code: string | undefined; try { - code = (await res.json())?.code; + // Read BOTH error dialects, the same way the success branch below + // reads both `url` shapes. The storage service moved its error + // code from a sibling of `error` into the declared + // `{ success: false, error: { code, message } }` envelope + // (objectstack#3675); a console build is deployed independently of + // the server it talks to, so it has to keep understanding the + // older top-level `code` too. Without the nested branch the 401/403 + // downgrade silently to "Download failed (403)" and the friendly + // copy below never fires. + const body = (await res.json()) as { code?: string; error?: { code?: string } } | null; + code = body?.error?.code ?? body?.code; } catch { /* non-JSON body */ } diff --git a/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx b/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx index fb6e96390..3e160c914 100644 --- a/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx +++ b/packages/app-shell/src/views/__tests__/RecordAttachmentsPanel.test.tsx @@ -124,10 +124,13 @@ describe('RecordAttachmentsPanel — authenticated signed-URL download (#2970)', it('maps a 403 ATTACHMENT_DOWNLOAD_DENIED to friendly copy and does not open a tab', async () => { const openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); vi.spyOn(globalThis, 'fetch').mockResolvedValue( - new Response(JSON.stringify({ code: 'ATTACHMENT_DOWNLOAD_DENIED' }), { - status: 403, - headers: { 'Content-Type': 'application/json' }, - }), + new Response( + JSON.stringify({ + success: false, + error: { code: 'ATTACHMENT_DOWNLOAD_DENIED', message: 'You do not have access…' }, + }), + { status: 403, headers: { 'Content-Type': 'application/json' } }, + ), ); setup(makeDataSource()); await waitFor(() => expect(screen.getByText('report.pdf')).toBeInTheDocument()); @@ -145,8 +148,33 @@ describe('RecordAttachmentsPanel — authenticated signed-URL download (#2970)', it('maps a 401 AUTH_REQUIRED to friendly copy', async () => { vi.spyOn(window, 'open').mockImplementation(() => null); vi.spyOn(globalThis, 'fetch').mockResolvedValue( - new Response(JSON.stringify({ code: 'AUTH_REQUIRED' }), { - status: 401, + new Response( + JSON.stringify({ + success: false, + error: { code: 'AUTH_REQUIRED', message: 'Authentication required to download this file' }, + }), + { status: 401, headers: { 'Content-Type': 'application/json' } }, + ), + ); + setup(makeDataSource()); + await waitFor(() => expect(screen.getByText('report.pdf')).toBeInTheDocument()); + + await userEvent.setup().click(screen.getByRole('button', { name: 'Download' })); + + await waitFor(() => + expect(screen.getByRole('alert')).toHaveTextContent('Please sign in to download this attachment.'), + ); + }); + + // The console ships independently of the server it talks to, so it must keep + // reading the PRE-objectstack#3675 body — code as a sibling of `error`, not a + // field of it. Without this, pointing a new console at an older server turns + // every gated download into the generic "Download failed (403)". + it('still maps the legacy top-level `code` shape (older server)', async () => { + const openSpy = vi.spyOn(window, 'open').mockImplementation(() => null); + vi.spyOn(globalThis, 'fetch').mockResolvedValue( + new Response(JSON.stringify({ error: 'You do not have access…', code: 'ATTACHMENT_DOWNLOAD_DENIED' }), { + status: 403, headers: { 'Content-Type': 'application/json' }, }), ); @@ -156,7 +184,10 @@ describe('RecordAttachmentsPanel — authenticated signed-URL download (#2970)', await userEvent.setup().click(screen.getByRole('button', { name: 'Download' })); await waitFor(() => - expect(screen.getByRole('alert')).toHaveTextContent('Please sign in to download this attachment.'), + expect(screen.getByRole('alert')).toHaveTextContent( + "You don't have access to download this attachment.", + ), ); + expect(openSpy).not.toHaveBeenCalled(); }); });