Site builder: security & data-loss hardening + unified asset picker + audit backlog #3
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: cloud-hosting-platform/site-builder#3
Reference in New Issue
Block a user
Delete Branch "builder-hardening-2026-07"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Implements the two design specs (
docs/superpowers/specs/2026-07-12-*) and the full 2026-07-12 Fable audit backlog.51 commits · +6,241 / −9,580 (net −3,339) · 339 tests (from 29) · bundle 807 kB → 634 kB · build clean.
Each task was implemented TDD and passed an independent adversarial review + fix loop; a final whole-branch review (which found & fixed a missed CSS-value XSS sink class) returned READY TO MERGE.
Security (Critical)
escape.ts(escapeHtml/escapeAttr/safeUrl) replacing 27 divergent escapers;safeUrlon every exported URL (+ MapEmbed, form relay).cssValue()across ~20 sinks/18 components) — closes</style><script>breakout via color/size/gap props.Data-loss / state (High)
Asset picker
AssetPicker(upload / browse-uploaded / URL; full+compact) wired into all 6 live StylePanels — every image field can now select an uploaded image.Correctness / dead-code / a11y / maintainability
toHtml; no moreMath.random).useNodeProp, array-item editor,Modal; typed AI patch boundary; CLAUDE.md updated.Known follow-ups (not blocking)
SiteDesign.headCodeisn't forwarded to the backend inbuildSavePayload— head code works in Preview but never reaches published sites (needs WHP/PHP work).addPagesame-ms id collision).🤖 Generated with Claude Code
Entity-escaping alone doesn't protect JS-string or raw-HTML sinks: - HtmlBlock.toHtml exported props.code raw; now runs it through the same purifyHtml (DOMPurify) config already used for the live editor preview, so <script>/on*= payloads can't survive export either. - Countdown.toHtml interpolated targetDate directly into `new Date("${targetDate}")` inside an inline <script> -- a value like `2026-01-01");alert(1)//` broke out of the string literal. Now validated against a strict date/datetime shape and JSON.stringify'd before embedding, falling back to `new Date()` for anything invalid. - Gallery.toHtml's lightbox used `onclick="${id}_open('${esc(img.src)}')"`, which a single quote in img.src could break out of. Replaced with a `data-lb-src` attribute per thumbnail and one delegated click listener on the grid (`e.target.closest('[data-lb-src]')`) instead of a per-item inline handler string. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>PageContext.treeToState, apply-ai-response.buildNodeTree, and the dead serializeTreeForCraft each re-implemented the same ColumnLayout-linkedNodes + SHELL_INNER walk and had drifted (buildNodeTree gained guards the others lacked). Move sanitizeAiTree (resolvedName allowlist + id/ROOT repair) here too and add flattenTreeForCraft, the shared structural walk (linkedNodes, SHELL_INNER wrapping, style:[]->{} normalization) both real callers will consolidate onto. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>buildNodeTree now delegates its structural walk (ColumnLayout linkedNodes, SHELL_INNER wrapping, style:[]->{} normalization) to flattenTreeForCraft, materializing each flat node into a real Craft.js Node via query.parseFreshNode — except the synthetic SHELL_INNER wrapper, which is still hand-built (parseFreshNode would merge in Container's default craft.props and change its look). sanitizeAiTree now lives in craft-tree.ts and is re-exported here for existing callers. Delete the dead serializeTreeForCraft (only its own tests called it) and its now-orphaned tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>- InputField/TextareaField/ContactForm: every control gets a deterministic id (slugId() in utils/escape.ts, derived from the field's name/label + index for ContactForm's looped fields -- no Math.random) with a matching <label for=>; fields with no visible label get an aria-label from the placeholder/name instead. - StarRating: wrapped in role="img" aria-label="Rating: N out of M", individual star glyphs marked aria-hidden. - Navbar: the mobile hamburger toggle gets aria-label="Toggle navigation menu", aria-controls="navbar-links", and aria-expanded wired to flip true/false in the inline onclick handler. - VideoBlock and MapEmbed: every exported <iframe> gets a title (generic "Embedded video", or "Map of {address}" for MapEmbed). - Decorative Font Awesome icons (ContentSlider arrows already covered in the prior commit; SocialLinks, SearchBar, Testimonials stars) are aria-hidden; SocialLinks' icon-only links get an aria-label naming the platform alongside the existing title tooltip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Migrates Menu, ColumnLayout, Countdown, Gallery, Tabs, InputField, and TextareaField from Math.random()/content-hash scope ids to the threaded Craft node id (via scopeId()), keeping every emitted element id, aria-controls/aria-labelledby/for, and inline <script> function name/getElementById() call consistently scoped per component. Resolves the two Important id-collision review findings: - Tabs: tabId was djb2(anchorId||labels) -- two default Tabs instances produced identical aria-controls/aria-labelledby ids, so one instance's arrow-key script clobbered the other's tab/panel wiring. - InputField/TextareaField: fieldId was `field-${name}` -- two fields sharing a (often default) name produced duplicate <label for>/<input id> pairs, breaking the for/id association for one of them. Also eliminates the remaining Math.random() scope ids in Menu (hover CSS class scope) and ColumnLayout (nth-child width CSS class scope), so no export component is non-deterministic anymore. All fall back to a stable content hash (never Math.random) for legacy 2-arg toHtml() call sites without a node id. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>New src/ui/Modal.tsx ({ open, onClose, title?, children, width?, closeOnEscape?, closeOnBackdropClick?, backdropStyle?, backdropProps? }) owns the backdrop, Escape-to-close, backdrop-click-to-close, and a new body-scroll-lock while open. Adopted by TemplateModal, HeadCodeModal, and SitesmithModal; each keeps its own panel styling/header/footer as children since those differ per modal. - TemplateModal: Escape/backdrop-click still close the confirm-template sub-dialog first via a wrapped onClose passed to Modal; the header's X button keeps using the raw onClose prop (always fully closes), matching prior asymmetric behavior. - HeadCodeModal: straightforward adoption, no prior custom close logic. - SitesmithModal: previously had no `open` prop, no Escape-to-close, and no backdrop-click-to-close. Preserved via open (always mounted-open by its parent already), closeOnEscape={false}, closeOnBackdropClick={false}; role and aria-modal are passed through via backdropProps to keep them on the same backdrop element as before. Body scroll-lock while a modal is open is a small new addition (requested by the task) applied uniformly; it has no visible effect since each modal's opaque fixed-position backdrop already fully covers the viewport. Added a light Modal.test.tsx (renders children, Escape/backdrop-click close behavior, scroll-lock) using the existing react-dom/client + act harness. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Adds a single cssValue() sanitizer (src/utils/escape.ts) that strips <>{};"'\ and neutralizes url(), safe for both style="..." attribute and <style>...</style> element contexts. Applies it at every raw user-prop CSS-value interpolation sink found via grep across src/components (colors, sizes, gaps interpolated directly into style strings/<style> blocks), including the highest-risk <style>-context sinks: ColumnLayout gap, Menu/Navbar hover and background colors. Also Number()-coerces the `columns` grid-template-columns sinks in Gallery/Testimonials/NumberCounter as defense in depth. Regression tests assert </style><script> payloads are neutralized and normal colors still render. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>slugify('!!!') stripped down to '' since punctuation-only names have no a-z0-9 characters left; buildSavePayload then wrote filename = '' + '.html'. slugify now falls back to 'page' when the computed slug is empty, so uniqueSlug's existing dedupe (page, page-2, ...) applies same as any other collision. Landing page slug stays forced to 'index'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>Adversarial re-review found the C1 fix incomplete plus an adjacent same-class XSS, both reachable via the AI update_props path and deserialized saved state: - cssPropsToString only ran sanitizeCssValue on typeof-string values, so a non-string style value (array/object) with a valid key skipped sanitization entirely and was template-coerced raw into style="...", e.g. { color: ['red', '"><img src=x onerror=alert(1)>'] }. Now every non-number value is coerced with String() and sanitized; numbers stay raw. sanitizeBreakoutChars also now escapes < and > (previously only ; and ") as defense-in-depth, since values can reach it from non-string sources. - props.tag (Container) and props.level (Heading) were interpolated raw into the tag position of exported HTML (`<${tag}`, `<${level}`) with no runtime validation, letting a malicious value break out of the tag entirely. Both are now allowlisted/clamped against their known-safe sets (div/section/article/header/footer/main; h1-h6), falling back to div/h2. Applied in Container's live render + toHtml, Heading's live render + toHtml, and the typeName==='div' fallback branch in html-export.ts's renderNode (hit for unresolved/legacy node types). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>