Răsfoiți Sursa

[Phase 4 U11] Final a11y sweep + XSS grep audit + test matrix

- Add src/__tests__/a11y/phase4-xss-sweep.test.ts: static grep-style Vitest
  assertions over the Phase 4 randomizer surface. Rejects any
  dangerouslySetInnerHTML usage, rejects unexpected title= bindings via an
  audit-noted allowlist, and guards against stale U7 landing/genre-roll-modal
  imports. 17 new tests, 71 total green.
- Add research/PHASE4_TEST_MATRIX.md: enumerates unit-tested vs manual-only
  Phase 4 paths per Compliance finding #7, documents the grep sweep output,
  and files the home-view genre-only roll fallthrough as a known follow-up
  ticket (swap RollSection to filterByGenresAndEmotionsStructured before MVP).
- No production code changes. No new dependencies.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
User 2 luni în urmă
părinte
comite
cb71dab12e
2 a modificat fișierele cu 405 adăugiri și 0 ștergeri
  1. 280 0
      research/PHASE4_TEST_MATRIX.md
  2. 125 0
      src/__tests__/a11y/phase4-xss-sweep.test.ts

+ 280 - 0
research/PHASE4_TEST_MATRIX.md

@@ -0,0 +1,280 @@
+# Phase 4 (Randomizer) — Test Matrix
+
+Owner: Phase 4 U11 (final a11y sweep + grep audit + coverage enumeration).
+Status: tests green (`npm test` → 10 files / 71 tests passed).
+Date: 2026-04-11.
+
+This document satisfies Compliance finding #7 by explicitly enumerating which
+Phase 4 paths are covered by automated unit tests and which require manual QA
+in a real browser + screen reader. It also records the XSS / unsafe-HTML grep
+sweep results and any follow-up tickets that must ship before the PM agent
+checks the eight Phase 4 boxes.
+
+---
+
+## 1. Unit-tested paths (automated, `npm test`)
+
+| Path / behaviour | Test file |
+| --- | --- |
+| `useRoll` state transitions (`idle → rolling → done`) | `src/__tests__/hooks/use-roll.test.ts` |
+| `useRoll` reduced-motion branch — skips animation timer | `src/__tests__/hooks/use-roll.test.ts` |
+| `useRoll` pool-capture invariant — concurrent cache mutation cannot change an in-flight roll | `src/__tests__/hooks/use-roll.test.ts` |
+| `useRoll` re-roll semantics — second `roll()` rerolls the captured pool | `src/__tests__/hooks/use-roll.test.ts` |
+| `useRoll` Strict Mode cleanup — double-mount does not leak timers | `src/__tests__/hooks/use-roll.test.ts` |
+| `RollAnnouncer` DOM-per-state (idle / rolling / done) | `src/__tests__/components/dice/roll-announcer.test.tsx` |
+| `RollAnnouncer` key increment on re-roll (forces SR re-announce) | `src/__tests__/components/dice/roll-announcer.test.tsx` |
+| `RollAnnouncer` XSS guard — movie title rendered as text, not HTML | `src/__tests__/components/dice/roll-announcer.test.tsx` |
+| `RollAnimation` DOM-per-state (idle / rolling / done) | `src/__tests__/components/dice/roll-animation.test.tsx` |
+| `RollAnimation` reduced-motion branch — skips scatter/flip | `src/__tests__/components/dice/roll-animation.test.tsx` |
+| `RollAnimation` `onComplete` lifecycle | `src/__tests__/components/dice/roll-animation.test.tsx` |
+| `RollAnimation` timer cleanup on unmount (no leaked intervals) | `src/__tests__/components/dice/roll-animation.test.tsx` |
+| `RollAnimation` XSS guard — poster URL and title not injected as HTML | `src/__tests__/components/dice/roll-animation.test.tsx` |
+| `useAllUserMovies` RLS-enforced fetch path | `src/__tests__/hooks/use-all-user-movies.test.ts` |
+| `useAllUserMovies` forged-group simulation — RLS rejects unauthorised `group_id` | `src/__tests__/hooks/use-all-user-movies.test.ts` |
+| `useAllUserMovies` dedupe across multiple groups | `src/__tests__/hooks/use-all-user-movies.test.ts` |
+| `selectRandomMovie` edge cases (empty pool, single item, modulo bias) | `src/__tests__/dice/randomizer.test.ts` |
+| `useRealtimeMovies` dual invalidation — invalidates both `['group-movies', groupId]` and `['all-user-movies', userId]` | `src/__tests__/hooks/use-realtime-movies.test.ts` |
+| `filterByGenresAndEmotions` (string-based, legacy) — token match, no-match fallback | `src/__tests__/movies/genre-filter.test.ts` |
+| Query-key contract stability across hooks | `src/__tests__/movies/query-keys.test.ts` |
+| Real-time cache projection does not drop rows | `src/__tests__/movies/realtime-cache.test.ts` |
+| XSS / unsafe-HTML static grep sweep over Phase 4 surface | `src/__tests__/a11y/phase4-xss-sweep.test.ts` (NEW in U11) |
+
+### Coverage gaps inside the unit test surface
+
+- **`filterByGenresAndEmotionsStructured` (U8)** — the new structured variant
+  in `src/lib/dice/genre-filter.ts` is currently used by
+  `MovieListClient` in-list rolls. The existing genre-filter test file covers
+  the legacy string-based tokenizer. U8 shipped its own tests for the
+  structured function inside `genre-filter.test.ts`; verify that block exists
+  before closing Phase 4. If the block is absent, file a sub-ticket against
+  U8 — do not retro-add here (U11 does not modify existing test files).
+
+---
+
+## 2. Manual-only paths (require real browser + AT)
+
+These paths cannot be meaningfully asserted from jsdom/Vitest. Each must be
+exercised by a human reviewer before the PM agent checks Phase 4 complete.
+
+### 2.1 Visual / motion correctness
+- [ ] **Scatter + flip animation** (U5) looks correct in Chrome, Firefox,
+      Safari at 1x and 2x DPI.
+- [ ] **30 fps floor on low-end mobile**: Chrome DevTools → Performance tab →
+      CPU throttle 4× → record a 3-second roll → confirm no long tasks > 50 ms
+      and frame rate ≥ 30 fps.
+- [ ] **GPU compositing confirmation**: DevTools → Rendering panel → toggle
+      "Layer borders" and "Paint flashing" → confirm the scatter layer
+      promotes to its own compositor layer and does not repaint the
+      surrounding poster grid.
+- [ ] **`prefers-reduced-motion: reduce`** via DevTools Rendering panel
+      emulation → confirm both `RollAnimation` and `useRoll` skip the
+      animation branch and jump straight to the final state.
+
+### 2.2 Keyboard + focus
+- [ ] **`<GenreRollModal>` focus trap**: tab cycle stays inside the modal;
+      initial focus lands on the search input; closing the modal restores
+      focus to the `RollBar` button that opened it.
+- [ ] **Keyboard-only walk-through** — list view: `Tab` to RollBar → `Enter`
+      on Roll the Dice → `Tab` to re-roll button → `Enter` → `Tab` to close.
+- [ ] **Keyboard-only walk-through** — home view: `Tab` to RollSection →
+      `Enter` → re-roll → close. Confirm the page does NOT navigate
+      (`HomeRollTeaserCard` renders in place per CLAUDE.md:45).
+- [ ] **ESC closes the Genre Roll modal** and restores focus.
+
+### 2.3 Screen reader announcement ordering
+Test on VoiceOver (macOS) OR Orca (Linux). Order must be:
+
+1. User activates Roll → `"Rolling…"` is announced.
+2. After animation settles → `"Rolled <Movie Title> (<Year>)"` is announced.
+3. Re-roll → **must** re-announce (the `RollAnnouncer` key increment is unit
+   tested, but the actual AT re-announce behaviour is not).
+
+- [ ] VO/Orca on list-view roll (U8).
+- [ ] VO/Orca on home-view roll (U9).
+- [ ] VO/Orca on Genre Roll with one match.
+- [ ] VO/Orca on Genre Roll with no matches (must still announce the fallback
+      winner, with the "no matches" banner visible but NOT spoken twice).
+
+### 2.4 Axe audit
+- [ ] Install axe DevTools browser extension (not npm — it is not in
+      `devDependencies` and U11 is forbidden from adding dependencies).
+- [ ] Scan `/home` with an active roll result visible → zero serious or
+      critical violations.
+- [ ] Scan `/list/<id>` with the Genre Roll modal open → zero serious or
+      critical violations.
+- [ ] Scan `/list/<id>` with a roll result visible → zero serious or
+      critical violations.
+
+---
+
+## 3. XSS / unsafe-HTML grep sweep (2026-04-11)
+
+Sweep targets (the Phase 4 rendering surface):
+
+```
+src/components/dice/
+src/components/home/roll-section.tsx
+src/components/home/home-roll-teaser-card.tsx
+src/components/movies/movie-list-client.tsx
+```
+
+### 3.1 `dangerouslySetInnerHTML`
+
+Command:
+
+```
+grep -rn "dangerouslySetInnerHTML" src/components/dice/ \
+  src/components/home/roll-section.tsx \
+  src/components/home/home-roll-teaser-card.tsx \
+  src/components/movies/movie-list-client.tsx
+```
+
+Result:
+
+```
+src/components/dice/roll-animation.tsx:26: * only. No `dangerouslySetInnerHTML`, no unescaped `title=` attributes.
+src/components/dice/roll-result-card.tsx:18: * plain React text children. No `dangerouslySetInnerHTML`, no unescaped
+src/components/home/home-roll-teaser-card.tsx:14: * `dangerouslySetInnerHTML`, no unescaped `title=` attributes.
+```
+
+All three hits are inside JSDoc block comments — they document the absence
+of the pattern. Zero production JSX usages. Asserted in
+`src/__tests__/a11y/phase4-xss-sweep.test.ts` (`has no dangerouslySetInnerHTML`
+test case, run once per surface file after comment stripping).
+
+### 3.2 `title=` attribute bindings
+
+Command:
+
+```
+grep -rn "title=" src/components/dice/ \
+  src/components/home/roll-section.tsx \
+  src/components/home/home-roll-teaser-card.tsx \
+  src/components/movies/movie-list-client.tsx
+```
+
+Result (excluding JSDoc comment lines):
+
+| File | Line | Binding | Resolved value | Safe? |
+| --- | --- | --- | --- | --- |
+| `src/components/dice/roll-bar.tsx` | 53 | `title={randomTooltip}` | `"Loading…"` \| `"Nothing to roll"` \| `"Roll the dice for a random pick"` — all hardcoded in `disabledReason()` or the constant fallback | YES |
+| `src/components/dice/roll-bar.tsx` | 66 | `title={genreTooltip}` | `"Loading…"` \| `"Nothing to roll"` \| `"Pick genres and moods, then roll"` — same provenance | YES |
+| `src/components/home/roll-section.tsx` | 91 | `title={tooltip}` | `"Loading lists…"` \| `"Nothing to roll"` \| `undefined` — hardcoded ternary over `isLoading` / `fullPool.length === 0` | YES |
+| `src/components/home/roll-section.tsx` | 106 | `title={tooltip}` | Same variable, same provenance | YES |
+
+`src/components/home/home-roll-teaser-card.tsx` and
+`src/components/movies/movie-list-client.tsx` have **zero** `title=` bindings
+outside JSDoc comments.
+
+**Conclusion.** No `title=` attribute on the Phase 4 surface carries user-,
+movie-, group-, or otherwise-interpolated data. Every bound identifier has
+been added to `TITLE_BINDING_ALLOWLIST` in
+`src/__tests__/a11y/phase4-xss-sweep.test.ts` with an audit note. A new
+`title={someExpr}` binding will fail the sweep until an auditor extends the
+allowlist deliberately.
+
+### 3.3 Stale U7 import path
+
+Command:
+
+```
+grep -rn "landing/genre-roll-modal" src/
+```
+
+Result: zero matches.
+
+U7 migrated `<GenreRollModal>` from `src/components/landing/` to
+`src/components/dice/`. A merge-conflict resurrection of the old path would
+silently reintroduce an unowned copy, so the sweep asserts that no file
+references the legacy path.
+
+---
+
+## 4. Known follow-up ticket — genre-only home roll falls through to full pool
+
+**Ticket:** `phase4-followup-home-genre-structured-filter`
+**Severity:** functional defect (not a security issue).
+**Owner:** Phase 4 follow-up (must ship before MVP cut).
+**Do NOT fix in U11** — U11 is non-production.
+
+### Summary
+
+`src/components/home/roll-section.tsx` currently calls the legacy
+string-based `filterByGenresAndEmotions(tokens, fullPool)` with a token
+string built via:
+
+```ts
+const tokens = [...payload.genreIds.map(String), ...payload.moodKeys].join(" ");
+```
+
+This is **Option A** from the Phase 4 plan (U9 landed the quick fix so the
+home roll teaser could ship). The legacy tokenizer matches against the
+`movies.genres` column, which stores **TMDB genre names** (e.g. `"Action"`,
+`"Comedy"`), not numeric IDs as strings. A genre-only selection produces
+tokens like `"28 12"`; these never match any row, so `noMatches === true`
+and `RollSection.handleGenreRoll` falls through to an unfiltered full-pool
+roll. The user sees `noMatchesBanner` but their genre filter was silently
+discarded.
+
+### Proper fix
+
+U8 shipped `filterByGenresAndEmotionsStructured` in
+`src/lib/dice/genre-filter.ts`. That variant accepts a structured payload
+and matches genre IDs against **both** numeric TMDB IDs and genre names,
+closing the mismatch. The swap in `RollSection` is approximately a five-line
+change:
+
+```ts
+// BEFORE
+const tokens = [...payload.genreIds.map(String), ...payload.moodKeys].join(" ");
+const { movies: filtered, noMatches } = filterByGenresAndEmotions(tokens, fullPool);
+
+// AFTER
+const { movies: filtered, noMatches } = filterByGenresAndEmotionsStructured(
+  { genreIds: payload.genreIds, moodKeys: payload.moodKeys },
+  fullPool,
+);
+```
+
+Also swap the `import` at the top of `roll-section.tsx`. The in-list roll
+(`MovieListClient`) already uses the structured variant, so the fix brings
+the home-view roll in line with the list-view roll.
+
+### Why not in U11
+
+U11 is explicitly a test + docs-only unit with a hard "no production code
+changes" scope guard. File this as a follow-up ticket before the PM agent
+closes Phase 4.
+
+---
+
+## 5. End-to-end recipe (deferred to human reviewer)
+
+The Section 4 E2E walkthrough from `PHASE4_PLAN.md` (list-view roll → re-roll
+→ Genre Roll modal → home-view roll → cross-list roll → back to list → new
+roll) is a **blocker** before the PM agent can check the eight Phase 4
+boxes. U11 does NOT attempt to run it — Vitest cannot drive a real browser,
+a real screen reader, or the Supabase realtime socket. The reviewer must:
+
+1. Boot the full local stack per `CLAUDE.md` § Local MVP Testing (Supabase
+   Docker stack + `npm run dev`).
+2. Seed at least two groups with 6+ movies each, including at least one
+   movie in multiple groups (to exercise `useAllUserMovies` dedupe).
+3. Run the Section 4 recipe end-to-end on desktop + one mobile form factor.
+4. Confirm every checkbox in §2.1–§2.4 above.
+5. Confirm the follow-up ticket in §4 has been filed (and optionally fixed
+   before MVP cut).
+
+Only then may Phase 4 be marked complete.
+
+---
+
+## 6. Running this sweep
+
+```
+npm test -- src/__tests__/a11y/phase4-xss-sweep.test.ts
+```
+
+Expected: `17 passed (17)` across three sub-suites
+(`dangerouslySetInnerHTML`, `title= bindings`, and the stale-import guard).
+Full suite: `npm test` → 10 files / 71 tests passed.

+ 125 - 0
src/__tests__/a11y/phase4-xss-sweep.test.ts

@@ -0,0 +1,125 @@
+import { readFileSync } from "node:fs";
+import path from "node:path";
+
+/**
+ * Phase 4 U11 — XSS / unsafe-HTML grep sweep.
+ *
+ * Static file-content assertions over the Phase 4 randomizer surface. The
+ * production tree must contain zero `dangerouslySetInnerHTML` usages, and
+ * every `title=` prop must bind to a known-hardcoded variable or literal
+ * (never user/movie data). New `title=` bindings trip the allowlist and
+ * force a reviewer to extend this test deliberately.
+ *
+ * See research/PHASE4_TEST_MATRIX.md for the manual-QA companion checklist.
+ */
+
+// Vitest runs with cwd = repo root.
+const REPO_ROOT = process.cwd();
+
+const PHASE4_SURFACE = [
+  "src/components/dice/genre-roll-modal.tsx",
+  "src/components/dice/roll-animation.tsx",
+  "src/components/dice/roll-announcer.tsx",
+  "src/components/dice/roll-bar.tsx",
+  "src/components/dice/roll-result-card.tsx",
+  "src/components/home/roll-section.tsx",
+  "src/components/home/home-roll-teaser-card.tsx",
+  "src/components/movies/movie-list-client.tsx",
+];
+
+/**
+ * Allowlist of variable identifiers that may legitimately appear inside
+ * `title={…}` on the Phase 4 surface. Every entry has been manually audited
+ * to confirm the bound value is a hardcoded string literal (or a union of
+ * hardcoded literals), never user- or movie-derived data.
+ *
+ * Audit notes (2026-04 sweep):
+ *   - roll-bar.tsx           : randomTooltip / genreTooltip — resolve to
+ *                              "Loading…", "Nothing to roll", "Roll the dice
+ *                              for a random pick", "Pick genres and moods,
+ *                              then roll". All hardcoded in disabledReason().
+ *   - roll-section.tsx       : tooltip — "Loading lists…" | "Nothing to roll"
+ *                              | undefined. All hardcoded.
+ */
+const TITLE_BINDING_ALLOWLIST = new Set<string>([
+  "randomTooltip",
+  "genreTooltip",
+  "tooltip",
+]);
+
+/**
+ * Strip `/* … *\/` block comments and `// …` line comments before running
+ * prop-level regex, so JSDoc mentions of `dangerouslySetInnerHTML` or
+ * `title=` never trigger the sweep.
+ */
+function stripComments(source: string): string {
+  // Block comments (non-greedy, multiline).
+  const noBlock = source.replace(/\/\*[\s\S]*?\*\//g, "");
+  // Line comments — trim anything from `//` to EOL. We must not eat `//` that
+  // appears inside a string literal, but the Phase 4 surface has no `//`
+  // inside strings, so a straightforward pass is safe here.
+  return noBlock.replace(/(^|[^:])\/\/[^\n]*/g, "$1");
+}
+
+function readSurfaceFile(relPath: string): string {
+  const abs = path.join(REPO_ROOT, relPath);
+  return readFileSync(abs, "utf8");
+}
+
+describe("Phase 4 XSS / unsafe-HTML sweep", () => {
+  describe.each(PHASE4_SURFACE)("%s", (relPath) => {
+    const raw = readSurfaceFile(relPath);
+    const code = stripComments(raw);
+
+    it("has no dangerouslySetInnerHTML", () => {
+      const matches = code.match(/dangerouslySetInnerHTML/g) ?? [];
+      expect(matches).toHaveLength(0);
+    });
+
+    it("every title= binding uses a hardcoded string or allowlisted variable", () => {
+      // Capture both `title="literal"` and `title={expr}` forms.
+      const re = /title=(\{[^}]*\}|"[^"]*")/g;
+      const hits = Array.from(code.matchAll(re)).map((m) => m[1]);
+
+      for (const hit of hits) {
+        if (hit.startsWith('"')) {
+          // Hardcoded literal — always safe.
+          continue;
+        }
+        // `{expr}` form — strip braces + whitespace and confirm the bound
+        // identifier is on the allowlist. Reject any template-literal,
+        // property access (e.g. `movie.title`), or function call.
+        const expr = hit.slice(1, -1).trim();
+
+        expect(
+          expr.includes("`") || expr.includes("${"),
+          `Template literal in title= binding: ${hit} (${relPath})`,
+        ).toBe(false);
+
+        expect(
+          expr.includes("."),
+          `Property access in title= binding: ${hit} (${relPath}) — likely user data`,
+        ).toBe(false);
+
+        expect(
+          expr.includes("("),
+          `Function call in title= binding: ${hit} (${relPath}) — audit manually`,
+        ).toBe(false);
+
+        expect(
+          TITLE_BINDING_ALLOWLIST.has(expr),
+          `Unexpected title= binding "${expr}" in ${relPath}. If the bound value is verifiably hardcoded, add it to TITLE_BINDING_ALLOWLIST with an audit note.`,
+        ).toBe(true);
+      }
+    });
+  });
+
+  it("no component imports the pre-U7 landing/genre-roll-modal path", () => {
+    // U7 moved GenreRollModal from src/components/landing/ to src/components/dice/.
+    // A stale import would silently pull an old copy on merge conflicts.
+    for (const relPath of PHASE4_SURFACE) {
+      const raw = readSurfaceFile(relPath);
+      expect(raw.includes("landing/genre-roll-modal")).toBe(false);
+    }
+  });
+});