From 94140990c2b81403a2df02657d9379f004060238 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 12 Jul 2026 11:36:27 -0700 Subject: [PATCH] Add implementation plan: site builder hardening + asset picker Co-Authored-By: Claude Opus 4.8 (1M context) --- ...site-builder-hardening-and-asset-picker.md | 249 ++++++++++++++++++ 1 file changed, 249 insertions(+) create mode 100644 docs/superpowers/plans/2026-07-12-site-builder-hardening-and-asset-picker.md diff --git a/docs/superpowers/plans/2026-07-12-site-builder-hardening-and-asset-picker.md b/docs/superpowers/plans/2026-07-12-site-builder-hardening-and-asset-picker.md new file mode 100644 index 0000000..84b2477 --- /dev/null +++ b/docs/superpowers/plans/2026-07-12-site-builder-hardening-and-asset-picker.md @@ -0,0 +1,249 @@ +# Site Builder Hardening + Asset Picker Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix the full 2026-07-12 Fable audit — Critical stored-XSS + High data-loss bugs, the Medium/Low correctness/dead-code/a11y backlog — and ship the unified image/asset picker, across the Craft.js site builder. + +**Architecture:** Work proceeds in dependency-ordered phases. Phase A builds a shared escaping/URL util that many later tasks consume, so it lands first. Phases that edit overlapping component files are serialized; genuinely disjoint tasks within a phase may run in parallel. Every task is TDD where a behavior is testable, and ends in a commit. + +**Tech Stack:** Vite 6 + React 18 + TypeScript 5 (strict) + @craftjs/core 0.2.x + Vitest. DOMPurify already a dependency. + +## Global Constraints + +- Active source is `craft/` only. Never touch the legacy top-level GrapesJS builder. +- Verify with `cd craft && npx vitest run` and `cd craft && npm run build` (runs `tsc` — strict). Both must be green before a task is "done". +- No native `alert/confirm/prompt` in any UI intended for the WHP panel (WHP UI ban). Use existing WHP/in-app patterns; do NOT add `window.confirm`. +- `toHtml` output must stay deterministic (no `Math.random()`/`Date.now()` in export paths) so published HTML diffs/caches cleanly. +- Inline styles only for user content; editor chrome uses `editor.css`. Follow existing component pattern (render + `.craft` + static `toHtml`). +- Frequent commits: one per task minimum. Conventional-commit messages. End commit messages with the Co-Authored-By trailer used in this repo. +- Path alias `@/` → `craft/src/`. + +--- + +## Phase A — Escaping/URL foundation (Critical XSS). Blocks C, and precedes D–G edits on shared files. + +### Task A1: Shared `escape.ts` util + +**Files:** +- Create: `craft/src/utils/escape.ts` +- Test: `craft/src/utils/escape.test.ts` +- Modify: `craft/src/utils/html-export.ts` (delete local `escapeHtml` at ~:201, import from `escape.ts`) + +**Produces (consumed by nearly every later task):** +```ts +export function escapeHtml(s: string): string; // & < > " — text/content + double-quoted attrs +export function escapeAttr(s: string): string; // escapeHtml + ' + ` — any attribute context +export function safeUrl(s: string): string; // '' for javascript:/vbscript:/data:text/html schemes (case/space/entity-insensitive), else trimmed input +``` + +- [ ] **Step 1: Write failing tests** covering: `escapeHtml('a & b "d"')` → `a & b <c> "d"`; `escapeAttr("' \` ")` escapes single-quote and backtick; `safeUrl('javascript:alert(1)')` → `''`; `safeUrl(' JavaScript:alert(1)')` → `''`; `safeUrl('data:text/html;base64,x')` → `''`; `safeUrl('https://x.com/a?b=1&c=2')` unchanged; `safeUrl('/relative')` unchanged; `safeUrl('mailto:x@y.com')` unchanged; `safeUrl('#anchor')` unchanged; non-string / null-ish inputs coerce safely. +- [ ] **Step 2: Run** `npx vitest run src/utils/escape.test.ts` → FAIL. +- [ ] **Step 3: Implement** `escape.ts`. `safeUrl` normalizes by lowercasing, stripping whitespace and HTML entities/control chars, then rejecting a `^(javascript|vbscript|data:text/html)` scheme; allow all other/relative URLs. +- [ ] **Step 4: Update `html-export.ts`** to import `escapeHtml` from `escape.ts`; delete its private copy. +- [ ] **Step 5: Run** `npx vitest run src/utils/escape.test.ts` + `npm run build` → PASS. +- [ ] **Step 6: Commit** `feat(builder): add shared escape/safeUrl util`. + +### Task A2: Replace the 27 local `esc`/`escapeHtml` copies + +**Files (modify — import `escapeHtml`/`escapeAttr` from `@/utils/escape`, delete the local def):** every file matching `grep -rlE "const (esc|escapeHtml)\s*=|function (esc|escapeHtml)" craft/src`. Includes (non-exhaustive) `components/basic/{ButtonLink,Icon,SocialLinks,Logo,Menu,Navbar,Footer,StarRating}.tsx`, `components/sections/{HeroSimple,CTASection,CallToAction,FeaturesGrid,PricingTable,Gallery,ContentSlider,Testimonials,Countdown,Tabs,Accordion,SubscribeForm,MapEmbed,...}.tsx`, `components/layout/{Section,BackgroundSection,Container,ColumnLayout}.tsx`, `components/forms/*.tsx`, `components/basic/_cta-helpers.tsx`. + +**Interfaces — Consumes:** A1's `escapeHtml`/`escapeAttr`. + +- [ ] **Step 1:** For each file, replace the local escaper with the import. Use `escapeAttr` for values emitted inside attributes (`title=`, `alt=`, single/double-quoted attrs); `escapeHtml` for text-node content. This fixes the "omits `&`" and "omits quotes" variants at once. +- [ ] **Step 2:** `grep -rE "const (esc|escapeHtml)\s*=|function (esc|escapeHtml)" craft/src` → only `escape.ts` remains (0 others). +- [ ] **Step 3:** `npm run build` → PASS (tsc catches missed imports). +- [ ] **Step 4:** Run existing toHtml tests `npx vitest run` → PASS. +- [ ] **Step 5: Commit** `refactor(builder): use shared escaper everywhere, drop 27 copies`. + +*(May be split into 2-3 commits by directory for review sanity; behavior-preserving.)* + +### Task A3: `safeUrl` on all exported URLs + +**Files (modify `toHtml` href/src/action/`url(...)` emission):** `basic/ButtonLink.tsx:230`, `basic/Icon.tsx:303,320`, `basic/SocialLinks.tsx:440`, `basic/Logo.tsx:417`, `basic/Menu.tsx:497`, `basic/Navbar.tsx:856-918`, `basic/_cta-helpers.tsx:86`, `sections/ContentSlider.tsx:484`, `sections/FeaturesGrid.tsx:292`, `sections/PricingTable.tsx:447`, `media/ImageBlock.tsx:479`, `media/VideoBlock.tsx:725,792`, `sections/Gallery.tsx` img `src`, background-image emitters `layout/BackgroundSection.tsx:191`, `sections/HeroSimple.tsx:421,441`, `sections/CallToAction.tsx:379`, `layout/Section.tsx` bg. + +**Consumes:** A1 `safeUrl`, `escapeAttr`. + +- [ ] **Step 1:** Wrap each exported URL: `href="${escapeAttr(safeUrl(props.href||''))}"`; for CSS `url(...)` use `url('${escapeAttr(safeUrl(x))}')`. +- [ ] **Step 2: Write regression tests** (in each component's `.toHtml.test.ts`, or a new `toHtml-escaping.test.ts`): a `javascript:` href renders empty; a `"`-containing href stays inside its attribute; bg `url()` with `")` cannot break out. +- [ ] **Step 3:** `npx vitest run` + `npm run build` → PASS. +- [ ] **Step 4: Commit** `fix(builder): neutralize javascript:/breakout URLs in HTML export`. + +### Task A4: JS-string / raw-HTML context fixes + +**Files:** `basic/HtmlBlock.tsx:144`, `sections/Countdown.tsx:298`, `sections/Gallery.tsx:296`. + +- [ ] **Step 1: Tests** — HtmlBlock `toHtml({code:''})` output is DOMPurify-sanitized (no raw `