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

@@ -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<void
  * (including text/html and image/svg+xml which can execute script) are forced
  * to download so that an uploaded file cannot run code in the TREK origin.
  *
- * Falls back to a download trigger if the popup is blocked.
+ * Uses a synthetic <a target="_blank" rel="noopener noreferrer"> 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<void> {
   assertRelativeUrl(url)
@@ -71,11 +84,19 @@ export async function openFile(url: string, filename?: string): Promise<void> {
     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)
 }

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<typeof vi.fn>).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(['<script>alert(1)</script>'], { 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<typeof vi.fn>).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(['<svg><script>alert(1)</script></svg>'], { 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<typeof vi.fn>).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
+    }
   })
 })