docs/superpowers/specs/2026-07-12-site-builder-security-hardening-design.md

@@ -0,0 +1,128 @@
+# Site Builder — security & data-loss hardening (2026-07-12)
+
+Fixes the **Critical** (stored-XSS / HTML-breakout) and **High** (state-corruption
+and save data-loss) findings from the 2026-07-12 Fable audit of the Craft.js site
+builder (`/workspace/site-builder/craft/`). Medium/Low findings are deferred to the
+audit backlog (in memory), not this spec. Sequenced **before** the asset-picker
+work per user decision. Every claim below was verified against the code before
+writing this spec.
+
+## 1 — Escaping cluster (Critical, do first: closes the most with one change)
+
+**Verified root cause:** `escapeHtml` exists once (`utils/html-export.ts:201`) but is
+**not exported** and no component uses it; there are **27** local `esc`/`escapeHtml`
+definitions across `src/`, in three incompatible variants (some omit `&`, some omit
+quotes entirely, none escape `'`), and several land in attribute or JS-string
+contexts where HTML-entity escaping is wrong anyway.
+
+### 1a — Shared escaping/URL util
+New `craft/src/utils/escape.ts` (exported, single source of truth):
+
+```ts
+export function escapeHtml(s: string): string;   // & < > "  — text/content context
+export function escapeAttr(s: string): string;   // escapeHtml + '  — attribute context
+export function safeUrl(s: string): string;      // returns '' (or '#') for javascript:/data:text/html/vbscript: schemes
+```
+
+- Move the correct body from `html-export.ts` into `escape.ts`; `html-export.ts`
+  imports it. Delete all 27 local copies and import from `escape.ts`.
+- `safeUrl` wraps every exported `href`/`src`/`action`/`url()` value: `ButtonLink`,
+  `Icon`, `SocialLinks`, `Logo`, `Menu`, `Navbar`, `_cta-helpers`, `ContentSlider`,
+  `FeaturesGrid`, `PricingTable`, `ImageBlock`, `VideoBlock`, `Gallery`, background
+  images. Attribute values use `escapeAttr`; text nodes use `escapeHtml`.
+
+### 1b — JS-string / raw-HTML contexts (entity escaping is NOT enough here)
+- **HtmlBlock** (`basic/HtmlBlock.tsx:144`): `toHtml` returns `props.code` raw while
+  the render sanitizes via the file's own `purifyHtml`. Fix: `return { html:
+  purifyHtml(props.code || '') }`. (DOMPurify already a dep.)
+- **Countdown** (`sections/Countdown.tsx:298`): `new Date("${targetDate}")` inside an
+  inline `<script>`. Fix: validate `/^\d{4}-\d{2}-\d{2}(T…)?$/` and/or emit via
+  `JSON.stringify(targetDate)`; reject/blank otherwise.
+- **Gallery** (`sections/Gallery.tsx:296`): `onclick="${galleryId}_open('${esc(img.src)}')"`
+  — `esc` doesn't escape `'`. Fix: emit `data-src="${escapeAttr(img.src)}"` and read
+  it via `getAttribute` in the lightbox handler (no inline string interpolation).
+
+### 1c — `cssPropsToString` (`utils/style-helpers.ts`)
+Does zero escaping, so free-text style props (e.g. Section "Background Gradient",
+background-image `url()`) can break out of `style="…"`. Escape values destined for
+`style` (strip `"`/`;` appropriately, `safeUrl` for `url()` contents).
+
+## 2 — Copy / Paste / Duplicate reuse node IDs (High, state corruption)
+
+**Verified:** `ContextMenu.duplicate` (`:65`) computes a fresh clone
+(`parseSerializedNode(...).toNode()`, `:75`) then **discards it** and calls
+`actions.addNodeTree(freshTree, parentId)` with the original tree — reusing original
+ids. `ContextMenu.pasteNode` (`:97`) and `useKeyboardShortcuts` duplicate (`:76`)
+both `toNodeTree()` → `addNodeTree` with original ids. Duplicating/pasting while the
+source still exists yields duplicate node ids → corrupted editor state.
+
+**Fix:** one shared helper that regenerates ids for a whole tree before insertion —
+use Craft's `query.parseFreshNode` per node, or clone the `NodeTree` remapping every
+`id` (and `nodes`/`parent`/`linkedNodes` refs) to fresh ids. Apply in all three call
+sites. Also: `pasteNode` targets the right-clicked node as parent (`:97`) — pasting
+onto a leaf throws; insert as a sibling (use the target's parent, fall back to ROOT).
+
+## 3 — Header/Footer save corruption (High, silent data loss)
+
+**Verified:** `useWhpApi.save` (`hooks/useWhpApi.ts:12`) serializes the live canvas
+(`query.serialize()`) into `craft_state` and into whichever page has
+`id === activePageId`, while `header_craft_state`/`footer_craft_state` and
+`header_html`/`footer_html` are exported from the **stale** `headerPage.craftState`/
+`footerPage.craftState` (`:32,:44,:102-103`). When editing the header/footer,
+`activePageId` is `__header__`/`__footer__` — matching no page — so live header/footer
+edits are written into top-level `craft_state`/`html` (mislabeled) and the real
+header/footer slots stay stale. Auto-save fires every 30s (`TopBar.tsx`) regardless
+of edit mode → silent loss.
+
+**Fix:** before serializing, commit the live canvas into the active zone's state
+(PageContext's existing save-current-state path — confirm exact name during
+implementation), then branch in `save()` on `isEditingHeader`/`isEditingFooter`:
+route the live serialization to `header_craft_state`/`footer_craft_state` (and
+`header_html`/`footer_html`) rather than the top-level page slots, and skip
+mis-assigning it to a non-existent active page.
+
+## 4 — AI-boundary validation (High)
+
+`utils/apply-ai-response.ts` deserializes AI output into craft state with no
+guards: `buildNodeTree` reads `node.props.node_id`/`node.type.resolvedName`
+unchecked (`:32,:37,:75,:81`); `update_props` does blind `Object.assign(p, op.props)`
+(`:247`) — can overwrite `node_id`/`style`; AI-supplied `node_id` is used directly as
+a map key (`:32`) with no `ROOT`/uniqueness check; an unknown `resolvedName` throws
+at `addNodeTree`. The live `PageContext.treeToState` already guards (`:378,:380`) —
+this path doesn't.
+
+**Fix:** validate every node's `resolvedName` against `componentResolver` before
+insertion; allowlist `update_props` keys per component (never `node_id`); ensure
+generated/patched ids are unique and non-`ROOT`; fail soft (skip the op + log)
+rather than throwing into `addNodeTree`. Reuse the `treeToState` guard logic (ties
+into backlog item "consolidate the three tree-flatteners", but only the validation
+is in scope here).
+
+## Testing
+
+Currently no tests cover `toHtml` escaping or the state layer. Add:
+- `craft/src/utils/escape.test.ts` — `escapeHtml`/`escapeAttr`/`safeUrl` incl.
+  `javascript:`/`data:text/html` rejection and `'` handling.
+- `toHtml` escaping regression tests for the fixed components (HtmlBlock purifies;
+  Countdown rejects a malicious date; Gallery uses `data-src`; a `"`-containing
+  `href`/`src` stays inside its attribute; `javascript:` href is neutralized).
+- Node-id regeneration test: duplicate/paste yields all-fresh ids (no collision with
+  the source subtree).
+- Header/footer save test: editing the header then saving writes live content to
+  `header_craft_state`/`header_html`, not the top-level page slot.
+- AI-boundary: unknown `resolvedName` and a `node_id`-overwriting `update_props` are
+  rejected without throwing.
+- Full `cd craft && npx vitest run` + `npm run build` green.
+
+## Sequencing / shippability
+
+Independent, each shippable alone; recommended order = the section order above (1
+first — highest risk, closes the most, and the escaping tests lock in the rest).
+Ships via the standard WHP release pipeline (`whp-deploy-release` skill) after
+implementation + review. The asset-picker spec
+(`2026-07-12-site-builder-asset-picker-design.md`) follows this work.
+
+## Explicitly out of scope (→ audit backlog in memory)
+All Medium/Low findings: dead `related.settings`/`ui/` code, `HeaderZone`/`FooterZone`
+resolver gap, slug dedupe, VideoBlock URL parsing, a11y pass, `any`-typing,
+tree-flattener consolidation (beyond the AI-guard reuse), stale dev proxy, etc.