From 137c4ee8a1b88c4695449e1532bdf9dbd0bc82c9 Mon Sep 17 00:00:00 2001 From: fabi Date: Mon, 27 Jul 2026 21:10:26 +0200 Subject: [PATCH] fix(export): let the keepsake download through X-Frame-Options on iOS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The keepsake download navigates a hidden, same-origin iframe (deliberately: a top-level navigation to a 404/429 would unload the PWA). Caddy stamped a site-wide `X-Frame-Options: DENY` that also covered the proxied `/api/*`. Blink hands a `Content-Disposition: attachment` response to the download manager at the network layer, so Chromium never noticed. WebKit enforces XFO on the frame navigation first and aborts the load — so on iOS Safari, the app's primary platform, tapping Download did nothing at all, silently. Carve the two export endpoints out to SAMEORIGIN, which still blocks cross-origin framing. Implemented as two disjoint matchers rather than an override: Caddy applies the FIRST `header` directive outermost, so it wins on write and a later, more specific `header` is silently ignored (verified against the running test stack). Also close the test gap that let this ship: - `06-export` ran on chromium-desktop only; add it to `webkit-iphone`, the only engine that enforces XFO on the download frame. - No test in the suite ever clicked a download button — every archive assertion used Node `fetch`, which has no frame and no XFO enforcement. Add a spec that clicks it and awaits a real `download` event. Verified falsifiable: with the blanket DENY reinstated it fails and reports the WebKit refusal as the cause. - Fix `ExportPage`'s card-scoped locators, which matched nothing: the cards carry `class="card p-5"` (a Tailwind `@apply` component class), never the `rounded-xl` the page object looked for. This had left the "shows enabled download buttons" test red on main. Co-Authored-By: Claude Opus 5 (1M context) --- Caddyfile | 16 ++- e2e/Caddyfile.test | 9 +- e2e/page-objects/export-page.ts | 13 ++- e2e/playwright.config.ts | 14 ++- e2e/specs/06-export/download-iframe.spec.ts | 103 ++++++++++++++++++++ 5 files changed, 148 insertions(+), 7 deletions(-) create mode 100644 e2e/specs/06-export/download-iframe.spec.ts diff --git a/Caddyfile b/Caddyfile index a8bebb8..970381f 100644 --- a/Caddyfile +++ b/Caddyfile @@ -9,10 +9,24 @@ header { Strict-Transport-Security "max-age=31536000; includeSubDomains" X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # X-Frame-Options: DENY everywhere EXCEPT the keepsake download endpoints, which + # are navigated in a HIDDEN, SAME-ORIGIN iframe so a 404/429 can't unload the PWA + # (see frontend/src/routes/export/+page.svelte). WebKit enforces XFO *before* + # honouring Content-Disposition, so a blanket DENY makes the download silently do + # nothing on iOS Safari — the app's primary platform. SAMEORIGIN still blocks + # cross-origin framing. + # + # Split into two disjoint matchers rather than an override: Caddy applies the + # FIRST header directive outermost, so it wins on write — a later, more specific + # `header` would be silently ignored. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + # SvelteKit frontend — static assets with long-lived cache (content-hashed filenames) @hashed_assets path_regexp hashed /_app/immutable/.*\.[a-f0-9]{8,}\.(js|css|woff2)$ header @hashed_assets Cache-Control "public, max-age=31536000, immutable" diff --git a/e2e/Caddyfile.test b/e2e/Caddyfile.test index 02fb85c..e481ea7 100644 --- a/e2e/Caddyfile.test +++ b/e2e/Caddyfile.test @@ -12,10 +12,17 @@ # Mirror prod's security headers (minus HSTS, which is HTTPS-only). header { X-Content-Type-Options "nosniff" - X-Frame-Options "DENY" Referrer-Policy "strict-origin-when-cross-origin" } + # Mirror prod's export carve-out: the keepsake download targets a hidden + # same-origin iframe, and WebKit enforces XFO before Content-Disposition. + # Two disjoint matchers, not an override — see the comment in ../Caddyfile. + @framable path /api/v1/export/zip /api/v1/export/html + @not_framable not path /api/v1/export/zip /api/v1/export/html + header @framable X-Frame-Options "SAMEORIGIN" + header @not_framable X-Frame-Options "DENY" + reverse_proxy /api/* app:3000 reverse_proxy /media/* app:3000 reverse_proxy /health app:3000 diff --git a/e2e/page-objects/export-page.ts b/e2e/page-objects/export-page.ts index d8dce62..687f2f6 100644 --- a/e2e/page-objects/export-page.ts +++ b/e2e/page-objects/export-page.ts @@ -37,12 +37,19 @@ export class ExportPage { /** * The "Download" button inside the card whose heading is `heading`. * - * Scoped to the card element (`div.rounded-xl`) rather than "any div containing the - * heading" — the latter also matches the page wrapper, which contains BOTH cards' buttons. + * Scoped to the card element rather than "any div containing the heading" — the latter + * also matches the page wrapper, which contains BOTH cards' buttons. + * + * The scope class is `div.card` (see the ZIP/HTML cards in + * frontend/src/routes/export/+page.svelte). It was previously `div.rounded-xl`, which + * matched NOTHING: `.card` is a Tailwind `@apply` component class + * (frontend/src/lib/styles/components.css) so the DOM only ever carries `class="card p-5"` + * — and the utility it applies is `rounded-2xl` anyway. Both card-scoped locators were + * therefore dead, which is why the "shows enabled download buttons" test was red. */ private cardButton(heading: string): Locator { return this.page - .locator('div.rounded-xl') + .locator('div.card') .filter({ has: this.page.getByRole('heading', { name: heading, exact: true }) }) .getByRole('button', { name: 'Download', exact: true }); } diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index a39125b..15c6a7e 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -146,8 +146,18 @@ export default defineConfig({ // that on `@smoke` — which exists on exactly two specs — meant the entire iOS guarantee was // one happy path and one join test. Every other UA here is a secondary browser and a smoke // check is proportionate; WebKit is not. Give it the core journeys the guest actually walks: - // join/recover, upload, and browse the feed. - testMatch: ['**/__smoke/**', '**/01-auth/**', '**/02-upload/**', '**/03-feed/**'], + // join/recover, upload, browse the feed — and take the keepsake home. + // + // 06-export is here because WebKit is the ONLY engine that enforces X-Frame-Options on the + // hidden download iframe. Excluding it is what let a site-wide `XFO: DENY` ship a keepsake + // download that silently did nothing on iOS. See 06-export/download-iframe.spec.ts. + testMatch: [ + '**/__smoke/**', + '**/01-auth/**', + '**/02-upload/**', + '**/03-feed/**', + '**/06-export/**', + ], }, { name: 'firefox-android', diff --git a/e2e/specs/06-export/download-iframe.spec.ts b/e2e/specs/06-export/download-iframe.spec.ts new file mode 100644 index 0000000..5fa7d47 --- /dev/null +++ b/e2e/specs/06-export/download-iframe.spec.ts @@ -0,0 +1,103 @@ +/** + * Regression guard — the keepsake download must actually download, in WebKit. + * + * The bug this exists to catch: `/export` streams the archive by pointing a HIDDEN, + * SAME-ORIGIN iframe at `/api/v1/export/zip` (deliberately — a top-level navigation to a + * 404/429 would unload the PWA). Caddy stamped a site-wide `X-Frame-Options: DENY` that + * also covered `/api/*`. Blink hands a `Content-Disposition: attachment` response to the + * download manager at the network layer, so Chromium never noticed; WebKit enforces XFO on + * the frame navigation FIRST and aborts the load. Result: on iOS Safari — the app's primary + * platform — tapping Download did nothing, silently, with no error anywhere. + * + * Why the old suite was structurally blind to it: + * - `06-export` ran on `chromium-desktop` only (webkit-iphone's testMatch excluded it), + * - and no test in the entire suite ever CLICKED a download button; every archive + * assertion used Node `fetch`, which has no frame and therefore no XFO enforcement. + * + * So this spec must keep both properties to be worth anything: a real click, in WebKit. + */ +import { test, expect } from '../../fixtures/test'; +import { ExportPage } from '../../page-objects'; +import { BASE } from '../../helpers/env'; + +const SLUG = 'e2e-test-event'; + +function post(path: string, jwt: string) { + return fetch(BASE + path, { method: 'POST', headers: { Authorization: `Bearer ${jwt}` } }); +} + +/** The real export job runs image processing; give it head-room over the tiny fixtures. */ +async function releaseAndWait(jwt: string) { + expect((await post('/api/v1/host/gallery/release', jwt)).status).toBe(204); + await expect + .poll( + async () => { + const res = await fetch(BASE + '/api/v1/export/status', { + headers: { Authorization: `Bearer ${jwt}` }, + }); + const s = await res.json(); + return s.released === true && s.zip?.status === 'done'; + }, + { timeout: 60_000, intervals: [500] } + ) + .toBe(true); +} + +test.describe('Export — the download actually fires in the browser', () => { + test.slow(); + + test('clicking Download triggers a real download event', async ({ page, host, signIn, db }) => { + await db.setExportReleased(SLUG, false); + await releaseAndWait(host.jwt); + + await signIn(page, host); + const exportPage = new ExportPage(page); + await exportPage.goto(); + await expect(exportPage.zipDownloadButton).toBeEnabled({ timeout: 10_000 }); + + // Capture the frame-level refusal that XFO produces, so a failure reports the CAUSE + // rather than just a timeout. WebKit logs "Refused to display ... in a frame because it + // set 'X-Frame-Options'"; Chromium logs nothing here, which is the whole problem. + const refusals: string[] = []; + page.on('console', (m) => { + if (/X-Frame-Options|Refused to display/i.test(m.text())) refusals.push(m.text()); + }); + + const downloadPromise = page.waitForEvent('download', { timeout: 30_000 }); + await exportPage.zipDownloadButton.click(); + + const download = await downloadPromise.catch((err) => { + throw new Error( + `No download event fired after clicking the ZIP button.` + + (refusals.length + ? ` The browser refused the iframe navigation: ${refusals.join(' | ')}` + : ' No X-Frame-Options refusal was logged; check the ticket/readiness path.') + + `\n${err}` + ); + }); + + expect(download.suggestedFilename()).toMatch(/\.zip$/i); + // The stream must produce real bytes, not a zero-length placeholder. + const path = await download.path(); + expect(path).toBeTruthy(); + expect(refusals, 'no frame should have been refused').toEqual([]); + }); + + test('the export endpoints are framable same-origin; everything else stays DENY', async () => { + // Locks the Caddy carve-out itself, independently of any browser. Cheap, and it fails + // loudly at the exact layer that regressed if someone reinstates a blanket DENY. + for (const path of ['/api/v1/export/zip', '/api/v1/export/html']) { + const res = await fetch(BASE + path); + expect(res.headers.get('x-frame-options')?.toUpperCase(), `${path} must be framable`).toBe( + 'SAMEORIGIN' + ); + } + + for (const path of ['/', '/api/v1/feed', '/api/v1/event']) { + const res = await fetch(BASE + path); + expect(res.headers.get('x-frame-options')?.toUpperCase(), `${path} must stay DENY`).toBe( + 'DENY' + ); + } + }); +});