diff --git a/functions/lib/itunes-banner.js b/functions/lib/itunes-banner.js index 8458f47..e3518c0 100644 --- a/functions/lib/itunes-banner.js +++ b/functions/lib/itunes-banner.js @@ -107,9 +107,32 @@ export function isLandingShell(html) { return typeof html === 'string' && LANDING_MARKS.every((mark) => html.includes(mark)); } +/** + * A suffix that names a file rather than a campaign code. + * + * One list, used by both questions that ask it. They used to ask it with two + * different sets: the gate carried `jpg|jpeg|webp|ico|txt|xml` that the parser + * lacked, the parser carried `html` that the gate lacked. So `/invite/AB.HTML` + * passed the gate and then lost its code, while `/invite/AB.JSON` never + * reached the gate at all. + * + * The two still ask it of different things, and that is deliberate: the gate + * asks it of the whole path, because `/invite/AB12CD/logo.png` is an asset + * request whoever owns the first segment, and the parser asks it of the code + * segment alone, because that is the part that would become a code. For the + * two-segment shape a shared link actually has, the two therefore agree, and a + * case in test/itunes-banner-function.test.mjs holds that. + * + * `html` is deliberately not in the list, for one reason: a code that happens + * to end in `.HTML` stays a code. The shell needs no help from the list — + * parseLandingFromUrl names `index.html` outright — and the `308` that + * canonicalises it comes from the platform and is handed on either way. + */ +const ASSET_SUFFIX = /\.(js|css|map|png|svg|json|jpg|jpeg|webp|ico|txt|xml)$/i; + export function shouldRewriteItunesBanner(pathname) { const path = String(pathname || ''); - if (/\.(js|css|map|png|svg|json|jpg|jpeg|webp|ico|txt|xml)$/i.test(path)) { + if (ASSET_SUFFIX.test(path)) { return false; } return ( @@ -328,7 +351,7 @@ export function parseLandingFromUrl(urlLike) { const kind = (parts[0] || '').toLowerCase(); if (kind !== 'invite' && kind !== 'promo') return null; const segment = parts[1]; - if (segment && /\.(js|css|map|png|svg|json|html)$/i.test(segment)) { + if (segment && ASSET_SUFFIX.test(segment)) { return { kind, code: null }; } let code = segment && segment.toLowerCase() !== 'index.html' ? capCode(segment) : null; diff --git a/public/_headers b/public/_headers index b3afc0b..6a56f84 100644 --- a/public/_headers +++ b/public/_headers @@ -26,35 +26,30 @@ # Invite/promo HTML (SPA shells), including /invite/{code}. Short max-age # so copy-code / CTA markup rolls out without waiting on the zone Browser -# Cache TTL. /invite/invite.js is restated after the wildcard so the 1h -# script cache wins whether first- or last-match applies. +# Cache TTL. +# +# The bare path needs its own rule; the wildcard covers everything under it, +# /invite/ and /invite/index.html included. Rules for those two used to stand +# beside the wildcard, and every answer that matched both carried the value +# twice: measured on the deploy, /invite/ answered +# `cache-control: public, max-age=60, public, max-age=60`. Pages concatenates +# what every matching rule says rather than letting one win, and a parser takes +# the first, so /invite/invite.js keeps its own rule above the wildcard and is +# not restated below it — the restatement only added a third value nobody +# reads. On production the zone's Browser Cache TTL replaces the lot with +# max-age=14400 anyway. /invite Cache-Control: public, max-age=60 -/invite/ - Cache-Control: public, max-age=60 - -/invite/index.html - Cache-Control: public, max-age=60 - /invite/* Cache-Control: public, max-age=60 /promo Cache-Control: public, max-age=60 -/promo/ - Cache-Control: public, max-age=60 - -/promo/index.html - Cache-Control: public, max-age=60 - /promo/* Cache-Control: public, max-age=60 -/invite/invite.js - Cache-Control: public, max-age=3600 - /.well-known/apple-app-site-association Content-Type: application/json Cache-Control: public, max-age=3600 diff --git a/scripts/check-site.mjs b/scripts/check-site.mjs index b927ecf..cb5b8aa 100644 --- a/scripts/check-site.mjs +++ b/scripts/check-site.mjs @@ -545,20 +545,46 @@ if (!existsSync(headersPath)) { 'must be a non-immutable max-age', ); requireHeader(blocks, '/js/*', 'cache-control', notImmutable, 'must be a non-immutable max-age'); + // The wildcards cover the shells and every code-bearing path under them; the + // bare paths need their own rule because a wildcard does not match them. A + // rule for /invite/index.html beside the wildcard would match twice and the + // answer would carry the value twice, which is what it used to do. requireHeader( blocks, - '/invite/index.html', - 'cache-control', - notImmutable, - 'must be a non-immutable max-age', - ); - requireHeader( - blocks, - '/promo/index.html', + '/invite', 'cache-control', notImmutable, 'must be a non-immutable max-age', ); + requireHeader(blocks, '/promo', 'cache-control', notImmutable, 'must be a non-immutable max-age'); + // Any exact path a wildcard already covers, not a list of the ones that + // happened to be there: a rule added later would otherwise slip past. The + // script keeps its own rule on purpose — it wants a longer cache than the + // shells, and a parser takes the first value of the two. + const wildcardKeep = new Set(['/invite/invite.js']); + for (const path of blocks.keys()) { + if (wildcardKeep.has(path) || path.endsWith('*')) continue; + for (const prefix of ['/invite/', '/promo/']) { + if (path === prefix || path.startsWith(prefix)) { + fail( + `_headers: ${path} is already covered by ${prefix}*; two matches send the value twice`, + ); + } + } + } + // Two blocks for the same exact path send the value twice as surely as a + // path and a wildcard do, and parseCfHeaders merges them into one entry, so + // the map cannot show it. Count the block openers instead. + const declared = new Set(); + for (const raw of read(headersPath).split(/\r?\n/)) { + const line = raw.trimEnd(); + if (!line || line.startsWith(' ') || line.startsWith('\t')) continue; + if (line.trimStart().startsWith('#')) continue; + const path = line.trim(); + if (declared.has(path)) + fail(`_headers: ${path} is declared twice; both matches send the value`); + declared.add(path); + } requireHeader( blocks, '/invite/*', diff --git a/test/itunes-banner-function.test.mjs b/test/itunes-banner-function.test.mjs index 6350db0..33aae63 100644 --- a/test/itunes-banner-function.test.mjs +++ b/test/itunes-banner-function.test.mjs @@ -40,6 +40,82 @@ describe('shouldRewriteItunesBanner', () => { }); }); +describe('the asset-suffix rule', () => { + test('the two questions about a file-looking segment give the same answer', () => { + // They used to be asked with two different sets: the gate carried + // jpg|jpeg|webp|ico|txt|xml that the parser lacked, the parser carried html + // that the gate lacked. So /invite/AB.HTML passed the gate and then lost + // its code, and /invite/AB.JSON never reached the gate. For the two-segment + // shape a shared link has, the answer has to be the same on both sides. + const suffixes = [ + 'js', + 'css', + 'map', + 'png', + 'svg', + 'json', + 'jpg', + 'jpeg', + 'webp', + 'ico', + 'txt', + 'xml', + ]; + // Asked as one question, so a suffix that moves to one side and not the + // other fails here rather than in production. The sweep is a list of its + // own, not the rule's: it catches a divergence on what it enumerates, and + // a suffix neither side knows is nobody's disagreement. `html` is in the sweep too: the two + // have to agree on it as well, and they agree that it is a code. + // + // Two segments, which is the shape a shared link has. Deeper paths are a + // different question by design — the gate reads the whole path, so + // /invite/AB12CD/logo.png is an asset request, while the parser reads the + // code segment and still finds AB12CD there. + const owns = (path) => shouldRewriteItunesBanner(path); + const reads = (path) => parseLandingFromUrl(`https://realunit.app${path}`).code !== null; + for (const suffix of [...suffixes, 'html', 'pdf', 'HTML', 'JsOn']) { + const path = `/invite/AB12CD.${suffix}`; + expect([suffix, owns(path)]).toEqual([suffix, reads(path)]); + } + for (const suffix of suffixes) { + const path = `/invite/AB12CD.${suffix.toUpperCase()}`; + expect([suffix, shouldRewriteItunesBanner(path)]).toEqual([suffix, false]); + expect([suffix, parseLandingFromUrl(`https://realunit.app${path}`)]).toEqual([ + suffix, + { kind: 'invite', code: null }, + ]); + } + // And a suffix this rule does not call an asset is a code like any other, + // on both sides — otherwise the case would pass on a rule that swallowed + // every dot. + expect(shouldRewriteItunesBanner('/invite/AB12CD.PDF')).toBe(true); + expect(parseLandingFromUrl('https://realunit.app/invite/AB12CD.PDF')).toEqual({ + kind: 'invite', + code: 'AB12CD.PDF', + }); + // html is not in the list, so a code that happens to end in it stays a + // code. The shell needs no help from the list: parseLandingFromUrl names + // index.html outright, which is why it still reads as no code at all. + expect(shouldRewriteItunesBanner('/invite/index.html')).toBe(true); + expect(parseLandingFromUrl('https://realunit.app/invite/index.html')).toEqual({ + kind: 'invite', + code: null, + }); + expect(shouldRewriteItunesBanner('/invite/AB12CD.HTML')).toBe(true); + expect(parseLandingFromUrl('https://realunit.app/invite/AB12CD.HTML')).toEqual({ + kind: 'invite', + code: 'AB12CD.HTML', + }); + // And the deeper path the two answer differently, on purpose: an asset + // request the pass does not own, whose code segment is still a code. + expect(shouldRewriteItunesBanner('/invite/AB12CD/logo.png')).toBe(false); + expect(parseLandingFromUrl('https://realunit.app/invite/AB12CD/logo.png')).toEqual({ + kind: 'invite', + code: 'AB12CD', + }); + }); +}); + describe('parseLandingFromUrl', () => { test('path, query, hash, and nested URL', () => { expect(parseLandingFromUrl('https://realunit.app/invite/AB12CD')).toEqual({ diff --git a/tests/behavior.spec.mjs b/tests/behavior.spec.mjs index 6cb9b4b..71f594d 100644 --- a/tests/behavior.spec.mjs +++ b/tests/behavior.spec.mjs @@ -759,6 +759,42 @@ test.describe('invite and promo landing', () => { test.skip(testInfo.project.name !== 'desktop-chromium', 'desktop-only invite-flow checks'); }); + test('a lookup that never answers gives up after the fifteen-second budget', async ({ page }) => { + // The unit suite pinned the exported number against a literal and stopped + // there; nothing exercised what the number is for. This does: a request + // that never answers must not leave the visitor on the loading state for + // ever. + // Real time, not the fake clock: the budget's timer is armed while the page + // loads, and a clock installed before that leaves the page in a state this + // case is not about. + test.setTimeout(60_000); + let pending = 0; + await page.route(REFERRAL_CODE_ENDPOINT, () => { + pending += 1; // never fulfilled, never aborted + }); + await page.goto('/invite/AB12CD'); + // The page shows the code straight away and says it is checking it; the + // spinner section is only the step before that. + await expect(page.locator('#ok-code-hint')).toHaveText('Code wird geprüft…'); + // The clock starts here, not at the navigation: a slow page load would + // otherwise be counted against the budget and could push the upper bound + // over on a busy machine. + const started = Date.now(); + await expect(page.locator('#state-unavailable')).toBeVisible({ timeout: 25_000 }); + // Two separate claims, because either alone would let something through. + // The size is pinned outright: without it, a budget moved to five seconds + // or to twenty would still satisfy bounds computed from itself. The + // elapsed time is then measured against it, so a page that gave up for + // some other reason, or on some other timer, does not pass for the one + // this case is named after. + const budget = await page.evaluate(() => window.RealUnitInvite.LOOKUP_TIMEOUT_MS); + expect(budget).toBe(15_000); + const elapsed = Date.now() - started; + expect(elapsed).toBeGreaterThan(budget - 2_000); + expect(elapsed).toBeLessThan(budget + 6_000); + expect(pending).toBe(1); + }); + test('an invite path without a code is invalid and does not call the API', async ({ page }) => { const calls = []; await page.route(REFERRAL_CODE_ENDPOINT, (route) => { @@ -1191,10 +1227,18 @@ test.describe('invite and promo landing', () => { }), ); await page.goto('/invite/AB12CD'); + // The stub records what it was handed. Resolving and discarding it would + // let this case pass on any string at all, including none. await page.evaluate(() => { + window.__written = []; Object.defineProperty(navigator, 'clipboard', { configurable: true, - value: { writeText: () => Promise.resolve() }, + value: { + writeText: (text) => { + window.__written.push(text); + return Promise.resolve(); + }, + }, }); }); await expect(page.locator('#ok-copy-link')).toBeVisible(); @@ -1207,6 +1251,14 @@ test.describe('invite and promo landing', () => { await expect(page.locator('#ok-copy-link')).not.toHaveAttribute('aria-live'); await page.locator('#ok-copy-link').click(); await expect(page.locator('#ok-copy-link')).toHaveText('Kopiert'); + // The clipboard itself, not the label beside it: the label is written from + // the DOM and would read the same whatever was copied. Exactly one write, + // and the same href the canonical link names — the host differs between + // the deploy and this server, the path with the code does not. + await expect.poll(() => page.evaluate(() => window.__written.length)).toBe(1); + const canonical = await page.locator('link[rel="canonical"]').getAttribute('href'); + expect(canonical).toMatch(/\/invite\/AB12CD$/); + expect(await page.evaluate(() => window.__written[0])).toBe(canonical); await expect(page.locator('#ok-copy-link')).toHaveAttribute( 'aria-label', /Kopiert .*\/invite\/AB12CD$/,