Escape titleSlam data-text attribute instead of composing raw HTML

[Brad-Edwards]

Review finding

The titleSlam template escapes the visible word content but interpolates the raw word into a data-text attribute in an innerHTML string.

Evidence

• src/system/templates/title-slam.ts:35 builds <span class="..." data-text="${w}">${markedTextHtml(w)}</span>.
• markedTextHtml(w) protects the element body, but data-text="${w}" is not attribute-escaped.
• A title token containing a quote can break out of the data-text attribute and create additional attributes on the generated span.

Impact

Scene modules are trusted today, but L2 templates are content-facing APIs. If generated, imported, or otherwise user-authored deck content reaches this template, a title word can mutate the DOM structure or attach executable attributes.

Recommended fix

Stop constructing these spans with string HTML. Build each span with DOM APIs and assign textContent plus dataset.text / setAttribute() so the browser performs attribute escaping.

Acceptance checks

• titleSlam creates word spans with ownerDoc.createElement('span'), className / classList, textContent, and an escaped data-text assignment.
• A test covers a title word containing ", <, and > and proves no additional attributes or elements are created.
• .pulsar-glitch behavior still has the expected data-text value for CSS rendering.

[Brad-Edwards]

🛠️ Picked up by /implement — driver Codex, branch 149-title-slam-escape, 2026-05-23T16:05:25Z.

[Brad-Edwards]

gc workflow phase recorded: preflight (issue #149). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.

[Brad-Edwards]

gc workflow phase recorded: plan (issue #149). Posted by the MCP server to enforce ordering between workflow steps (issue #794 MVP-2). Do not edit or delete — used by downstream tools to gate phase prerequisites.

Implementation Plan

• Change src/system/templates/title-slam.ts so title word spans are created with ownerDoc.createElement('span'); assign class, textContent, and dataset.text directly. Keep existing tokenization, captions, timeline labels, template root, subtitle path, and .pulsar-glitch class names.
• Add regression coverage in tests/system/templates-options.test.ts using the existing fake DOM and dataset proxy. The test will use a word containing ", <, and >, then assert exactly one word span, the raw data-text value, visible text content, the expected glitch class, no injected attributes or elements, and no innerHTML span assembly.
• Add changelog.d/149.security.md.

Security: DOM property assignment is the output-encoding boundary. No manual escaping layer, sanitizer library, title schema change, auth/env/network/storage access, or error-envelope change.

Maintainability: reuse the _shared.ts template lifecycle and existing fake DOM test pattern. No new dependency and no generalized DOM builder for one caller.

Extensibility: a reusable marker-to-DOM renderer belongs beside markedTextHtml only when multiple templates need that API. This issue only removes raw HTML assembly from titleSlam.

Whole-repo view: affected surfaces are the L2 template implementation, jsdom-free system tests, and towncrier fragment. Documentation coverage returned no required doc target.

Verification: write the regression test first and confirm it fails, implement the production fix, run pnpm exec vitest tests/system/templates-options.test.ts, then run pnpm lint && pnpm typecheck && pnpm test.

[Brad-Edwards]

gc_codex_review — cycle 1 of 1 (pre-push) on issue #149 (branch 149-title-slam-escape)

Core review

Verdict: ship

This change is shaped correctly: title-slam moves from interpolated HTML to explicit DOM construction inside the existing template buildChildren seam, which is the right boundary for per-word rendering behavior. The shared DOM contract expands only for className, and the existing set/text APIs carry the escaping concern without introducing a new sanitizer abstraction. The test pins the production behavior that matters here: word content remains text, glitch data mirrors the token, and hostile-looking input cannot become attributes or child markup. It does not foreclose the obvious next variation, such as adding more per-word classes or data attributes.

No blocking findings.

Security review

Verdict: ship

This change is shaped correctly: it moves title word rendering at the template DOM boundary from string-assembled innerHTML to explicit element creation with textContent and dataset assignment, which removes an HTML parsing surface rather than adding one. The cross-cutting concern is user-authored template content flowing into DOM attributes/text; the seam stays localized to the title-slam renderer and the test covers the relevant injection-shaped input. I do not see a new exploitable security path introduced by this diff.

No blocking findings.

[Brad-Edwards]

gc_codex_review pre-push cycle 1 of 1 complete for issue #149 on branch '149-title-slam-escape'. Posted by the MCP server to enforce the pre-push hard-cap-1 contract (issues #796, #804, #906). Do not edit or delete — used by the next gc_codex_review (uncommitted) invocation to count cycles.

[Brad-Edwards]

Review decision record — codex cycle 1 (issue #149)

Reviewer: codex
Cycle: 1

Architectural read:

Core reviewer: This change is shaped correctly: title-slam moves from interpolated HTML to explicit DOM construction inside the existing template buildChildren seam, which is the right boundary for per-word rendering behavior. The shared DOM contract expands only for className, and the existing set/text APIs carry the escaping concern without introducing a new sanitizer abstraction. The test pins the production behavior that matters here: word content remains text, glitch data mirrors the token, and hostile-looking input cannot become attributes or child markup. It does not foreclose the obvious next variation, such as adding more per-word classes or data attributes.

Security reviewer: This change is shaped correctly: it moves title word rendering at the template DOM boundary from string-assembled innerHTML to explicit element creation with textContent and dataset assignment, which removes an HTML parsing surface rather than adding one. The cross-cutting concern is user-authored template content flowing into DOM attributes/text; the seam stays localized to the title-slam renderer and the test covers the relevant injection-shaped input. I do not see a new exploitable security path introduced by this diff.

Blocking findings: 0 (clean run)

[Brad-Edwards]

gc_test_quality_review cycle 1 of 1 — issue #149

Reviewer: test-quality (claude-sonnet-4-6 via gc_test_quality_review)
Branch: 149-title-slam-escape
Cycle: 1 / 1
Findings: 1

Finding 1 — [warning] tests/system/templates-options.test.ts

Problem: Tests that exercise optional branches (eyebrow, subtitle, headline, bg, doc, signoff, etc.) assert only that the lifecycle completes without throwing. They do not verify the optional DOM element was actually created. Deleting the conditional rendering block for any optional feature leaves these tests green. The metricTicker test is the most misleading instance: its name promises interval-teardown verification, but expect(() => scene.cleanup(...)).not.toThrow() cannot distinguish a working teardown from a no-op cleanup.
Why it matters: Silent regressions in optional rendering (e.g., a merge strips the eyebrow conditional) go undetected. The test file's own header says these branches 'flip a piece of DOM the template otherwise wouldn't render' — but none of the assertLifecycle-only tests verify that claim.
Fix: After each assertLifecycle call, query the stage for the optional element and assert it exists with the expected content. Pattern: const root = stage.querySelector('[data-pulsar-template="..."]'); expect(root?.childNodes.some(n => n.textContent === 'eb')).toBe(true). The outro test's expect(scene.assets).toEqual(['/qr.png']) is the right model — extend that pattern to DOM state for every optional branch.

[Brad-Edwards]

gc_test_quality_review cycle 1 of 1 complete for issue #149 on branch '149-title-slam-escape'. Posted by the MCP server to enforce the gc_test_quality_review hard-cap-1 contract (issue #884 follow-up, default lowered in #906). Do not edit or delete — used by the next gc_test_quality_review invocation to count cycles.

[Brad-Edwards]

Review decision record — test-quality cycle 1 (issue #149)

Reviewer: test-quality
Cycle: 1

Architectural read:

The branch adds a targeted XSS regression test for the titleSlam glitch path. The fake-DOM infrastructure (makeDataset proxy, flat textContent/innerHTML split) is purpose-built and faithful enough to real browser semantics that the new security test genuinely catches the regression it guards. The rest of templates-options.test.ts is a no-crash smoke harness: assertLifecycle asserts only that the lifecycle contract is satisfied (non-null timeline), never that optional DOM branches were actually rendered. The file header says each test "flips a piece of DOM", but the assertLifecycle-only tests cannot verify that claim. The shape of the security test itself is correct; the surrounding suite has a long-standing coverage gap.

Blocking findings: 1

Finding 1 — class (19 instances)

• ID: F1
• Title: (no title)
• Decision: fix
• Rationale: Addressed by next cycle
• Instances:
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::titleSlam with subtitle + glitch + 5-word title (>4 word stagger)
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::statRow / statPairGrid / definitionTable / quoteStack / bulletList with eyebrow
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::introGrid roles with logos and primary flag
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::compare with headline
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::screenshotCallouts with multiple callouts
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::metricTicker create + cleanup tears down the interval
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::terminal exercises every script step kind
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::placard with no subtitle + quote with no attribution + centerpiece with no attr
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::actHeader + outlineTitle with custom prefix
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::incidentPlate with bg + sub
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::operatorDossier renders all rows
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::splitPaneTerminalDoc with eyebrow + headline + canted doc + caption
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::activityFeedPayoff with eyebrow + headline + all entry types + payoff title
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::haulCitations with eyebrow + headline + hot row mod + citation title
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::chatTranscript with title + alert flag + doc
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::cardCarousel with eyebrow + sub + src + per-card dwell
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::splitDialogueEmail with eyebrow + headline + signoff + footer + base override
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::dropList with eyebrow + headline + sub + hot mod
  • tests/system/templates-options.test.ts::L2 templates — optional-branch coverage::chatPickList with promptHandle + sub + pickCaption

[Brad-Edwards]

Implemented in PR #156.