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

# 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.