From 28dbd86d030e03e272fd4ab7edefe75474874cbe Mon Sep 17 00:00:00 2001 From: Xre0uS <36565320+Xre0uS@users.noreply.github.com> Date: Thu, 23 Apr 2026 16:06:56 +0800 Subject: [PATCH] fix(files): open attachments only in new tab (#840) window.open with noreferrer returns null, which triggered the popup-blocked download fallback in addition to the new-tab open. Use a target=_blank anchor click instead. --- client/src/utils/fileDownload.ts | 33 ++++++++-- client/tests/unit/utils/fileDownload.test.ts | 69 +++++++++++++++----- 2 files changed, 80 insertions(+), 22 deletions(-) diff --git a/client/src/utils/fileDownload.ts b/client/src/utils/fileDownload.ts index b9904472..10e05fd0 100644 --- a/client/src/utils/fileDownload.ts +++ b/client/src/utils/fileDownload.ts @@ -32,6 +32,13 @@ function triggerAnchorDownload(blobUrl: string, filename?: string): void { setTimeout(() => { URL.revokeObjectURL(blobUrl); a.remove() }, 100) } +// navigator.standalone is true only on iOS when running as an +// add-to-home-screen PWA. In that context, target="_blank" hands off to +// Safari, which cannot access blob URLs sandboxed to the WebView. +function isIosStandalone(): boolean { + return (navigator as any).standalone === true +} + /** * Fetches a protected file using cookie auth (credentials: include) and * triggers a browser download. Works inside PWA standalone mode because the @@ -56,7 +63,13 @@ export async function downloadFile(url: string, filename?: string): Promise click rather + * than window.open(). window.open() called with the "noreferrer"/"noopener" + * window feature returns null per spec, which previously made the popup-block + * fallback trigger a download in the *current* tab on top of the new-tab open + * — i.e. the file opened twice. The anchor approach avoids that ambiguity: + * the new tab is opened by the browser's normal link-handling path, and no + * spurious in-page download is triggered. */ export async function openFile(url: string, filename?: string): Promise { assertRelativeUrl(url) @@ -71,11 +84,19 @@ export async function openFile(url: string, filename?: string): Promise { return } - const win = window.open(blobUrl, '_blank', 'noreferrer') - if (win) { - setTimeout(() => URL.revokeObjectURL(blobUrl), 30_000) - } else { - // Popup blocked — fall back to download + // iOS PWA: target="_blank" would open Safari, which can't access the blob + if (isIosStandalone()) { triggerAnchorDownload(blobUrl, filename) + return } + + const a = document.createElement('a') + a.href = blobUrl + a.target = '_blank' + a.rel = 'noopener noreferrer' + document.body.appendChild(a) + a.click() + // Keep the blob URL alive long enough for the new tab to load it, then + // clean up the DOM node and revoke the URL. + setTimeout(() => { URL.revokeObjectURL(blobUrl); a.remove() }, 30_000) } diff --git a/client/tests/unit/utils/fileDownload.test.ts b/client/tests/unit/utils/fileDownload.test.ts index 89632017..b5a8833c 100644 --- a/client/tests/unit/utils/fileDownload.test.ts +++ b/client/tests/unit/utils/fileDownload.test.ts @@ -74,32 +74,42 @@ describe('downloadFile', () => { }) describe('openFile', () => { - it('fetches with credentials:include and opens blob URL in new tab', async () => { + it('fetches with credentials:include and opens blob URL via target=_blank anchor', async () => { vi.stubGlobal('fetch', makeFetchMock(200)) - const mockWin = { closed: false } - const openSpy = vi.spyOn(window, 'open').mockReturnValue(mockWin as Window) + const openSpy = vi.spyOn(window, 'open').mockReturnValue(null) + const clickSpy = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) await openFile('/uploads/files/doc.pdf') expect(window.fetch).toHaveBeenCalledWith('/uploads/files/doc.pdf', { credentials: 'include' }) expect(URL.createObjectURL).toHaveBeenCalled() - expect(openSpy).toHaveBeenCalledWith('blob:mock-url', '_blank', 'noreferrer') + // Must NOT call window.open — that path returns null when noreferrer is + // set, which previously caused the file to also open in the current tab. + expect(openSpy).not.toHaveBeenCalled() + expect(clickSpy).toHaveBeenCalledTimes(1) + + // The anchor used to open the new tab must be target=_blank, must NOT + // carry a `download` attribute (otherwise it would download in-page + // instead of opening), and must use rel=noopener noreferrer. + const appendCalls = (document.body.appendChild as ReturnType).mock.calls + const anchor = appendCalls[0]?.[0] as HTMLAnchorElement + expect(anchor.target).toBe('_blank') + expect(anchor.rel).toBe('noopener noreferrer') + expect(anchor.hasAttribute('download')).toBe(false) // Revoke happens after 30s timeout vi.runAllTimers() expect(URL.revokeObjectURL).toHaveBeenCalledWith('blob:mock-url') }) - it('falls back to anchor download when popup is blocked', async () => { + it('does not trigger a second in-page action for safe inline types (regression: no double-open)', async () => { vi.stubGlobal('fetch', makeFetchMock(200)) - vi.spyOn(window, 'open').mockReturnValue(null) const clickSpy = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) - await openFile('/uploads/files/doc.pdf') + await openFile('/uploads/files/doc.pdf', 'doc.pdf') - expect(clickSpy).toHaveBeenCalled() - vi.runAllTimers() - expect(URL.revokeObjectURL).toHaveBeenCalledWith('blob:mock-url') + // Exactly ONE anchor click — opening the new tab. No fallback download. + expect(clickSpy).toHaveBeenCalledTimes(1) }) it('throws on 401 response', async () => { @@ -108,28 +118,55 @@ describe('openFile', () => { expect(URL.createObjectURL).not.toHaveBeenCalled() }) - it('forces download for unsafe MIME types (HTML, SVG) instead of opening inline', async () => { + it('forces download for unsafe MIME types (HTML) instead of opening inline', async () => { const htmlBlob = new Blob([''], { type: 'text/html' }) vi.stubGlobal('fetch', makeFetchMock(200, htmlBlob)) const openSpy = vi.spyOn(window, 'open').mockReturnValue({} as Window) const clickSpy = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) - await openFile('/uploads/files/malicious.html') + await openFile('/uploads/files/malicious.html', 'malicious.html') // Must NOT open inline — download anchor clicked instead expect(openSpy).not.toHaveBeenCalled() - expect(clickSpy).toHaveBeenCalled() + expect(clickSpy).toHaveBeenCalledTimes(1) + + const appendCalls = (document.body.appendChild as ReturnType).mock.calls + const anchor = appendCalls[0]?.[0] as HTMLAnchorElement + expect(anchor.download).toBe('malicious.html') }) it('forces download for SVG MIME type', async () => { const svgBlob = new Blob([''], { type: 'image/svg+xml' }) vi.stubGlobal('fetch', makeFetchMock(200, svgBlob)) - vi.spyOn(window, 'open').mockReturnValue({} as Window) + const openSpy = vi.spyOn(window, 'open').mockReturnValue({} as Window) const clickSpy = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) await openFile('/uploads/files/malicious.svg') - expect(window.open).not.toHaveBeenCalled() - expect(clickSpy).toHaveBeenCalled() + expect(openSpy).not.toHaveBeenCalled() + expect(clickSpy).toHaveBeenCalledTimes(1) + }) + + it('falls back to download in iOS PWA standalone mode (blob URL inaccessible to Safari)', async () => { + vi.stubGlobal('fetch', makeFetchMock(200)) + const clickSpy = vi.spyOn(HTMLAnchorElement.prototype, 'click').mockImplementation(() => {}) + // Simulate iOS PWA (Add-to-Home-Screen) context + Object.defineProperty(navigator, 'standalone', { configurable: true, value: true }) + + try { + await openFile('/uploads/files/doc.pdf', 'doc.pdf') + + // Single anchor click — and it must be a DOWNLOAD anchor (no target=_blank), + // because target="_blank" in iOS PWA would hand off to Safari which cannot + // read the in-WebView blob URL. + expect(clickSpy).toHaveBeenCalledTimes(1) + const appendCalls = (document.body.appendChild as ReturnType).mock.calls + const anchor = appendCalls[0]?.[0] as HTMLAnchorElement + expect(anchor.target).toBe('') + expect(anchor.download).toBe('doc.pdf') + } finally { + // Clean up the non-standard iOS-only property we forced above. + delete (navigator as any).standalone + } }) })