Follow-ups from the #34 and #37 reviews

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;

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

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/*',

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({

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$/,