From 7ba91d98294fe92c587e617f6bc4c332318524ac Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 12 Jul 2026 17:44:59 -0700 Subject: [PATCH] fix(builder): sanitize non-string style values + allowlist element tags (XSS) 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', '">'] }. 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) --- .../components/basic/Heading.toHtml.test.ts | 40 +++++++++++++++++++ craft/src/components/basic/Heading.tsx | 15 ++++++- .../layout/Container.toHtml.test.ts | 30 ++++++++++++++ craft/src/components/layout/Container.tsx | 18 ++++++++- craft/src/utils/html-export.test.ts | 39 ++++++++++++++++++ craft/src/utils/html-export.ts | 7 +++- craft/src/utils/style-helpers.test.ts | 33 +++++++++++++++ craft/src/utils/style-helpers.ts | 23 ++++++++--- 8 files changed, 195 insertions(+), 10 deletions(-) create mode 100644 craft/src/components/basic/Heading.toHtml.test.ts diff --git a/craft/src/components/basic/Heading.toHtml.test.ts b/craft/src/components/basic/Heading.toHtml.test.ts new file mode 100644 index 0000000..f13d115 --- /dev/null +++ b/craft/src/components/basic/Heading.toHtml.test.ts @@ -0,0 +1,40 @@ +import { describe, test, expect } from 'vitest'; +import { Heading } from './Heading'; + +const toHtml = (Heading as any).toHtml; + +describe('Heading.toHtml level allowlist (adversarial re-review, same class as C1)', () => { + test('a malicious level value clamps to h2 -- no injected , no broken-out tag', () => { + const { html } = toHtml({ text: 'x', level: 'h2>')).toBe(true); + }); + + test('a numeric out-of-range level (99) clamps to h2', () => { + const { html } = toHtml({ text: 'x', level: 99 as any }, ''); + expect(html.startsWith('')).toBe(true); + }); + + test('a non-heading string level clamps to h2', () => { + const { html } = toHtml({ text: 'x', level: 'script' as any }, ''); + expect(html.startsWith(' { + const { html } = toHtml({ text: 'x', level: 'h4' }, ''); + expect(html).toContain(''); + }); + + test('all valid levels h1-h6 still work', () => { + for (const level of ['h1', 'h2', 'h3', 'h4', 'h5', 'h6']) { + const { html } = toHtml({ text: 'x', level }, ''); + expect(html.startsWith(`<${level}`)).toBe(true); + expect(html.endsWith(``)).toBe(true); + } + }); +}); diff --git a/craft/src/components/basic/Heading.tsx b/craft/src/components/basic/Heading.tsx index 158f911..da3e654 100644 --- a/craft/src/components/basic/Heading.tsx +++ b/craft/src/components/basic/Heading.tsx @@ -4,6 +4,16 @@ import { cssPropsToString } from '../../utils/style-helpers'; type HeadingLevel = 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6'; +// `level` is settable via the AI `update_props` path and from deserialized +// saved state -- neither type-checked at runtime -- and is interpolated +// directly into the tag position (`React.createElement(level, ...)` / +// `<${tag}` in `toHtml`). A malicious value like `h2> + (ALLOWED_HEADING_LEVELS as readonly unknown[]).includes(level) ? (level as HeadingLevel) : 'h2'; + interface HeadingProps { text?: string; level?: HeadingLevel; @@ -30,6 +40,7 @@ export const Heading: UserComponent = ({ selected: node.events.selected, })); + const safeLevel = sanitizeHeadingLevel(level); const elRef = useRef(null); const editedTextRef = useRef(null); @@ -59,7 +70,7 @@ export const Heading: UserComponent = ({ } }, [text, selected]); - return React.createElement(level, { + return React.createElement(safeLevel, { ref: (ref: HTMLElement | null): void => { elRef.current = ref; if (ref) connect(drag(ref)); @@ -98,7 +109,7 @@ Heading.craft = { }; (Heading as any).toHtml = (props: HeadingProps, _childrenHtml: string) => { - const tag = props.level || 'h2'; + const tag = sanitizeHeadingLevel(props.level); const safeText = (props.text || '').replace(//g, '>'); const styleStr = cssPropsToString(props.style); return { html: `<${tag}${styleStr ? ` style="${styleStr}"` : ''}>${safeText}` }; diff --git a/craft/src/components/layout/Container.toHtml.test.ts b/craft/src/components/layout/Container.toHtml.test.ts index d26be50..a08a0cb 100644 --- a/craft/src/components/layout/Container.toHtml.test.ts +++ b/craft/src/components/layout/Container.toHtml.test.ts @@ -33,3 +33,33 @@ describe('Container.toHtml cssId/cssClass', () => { expect(html).toContain('id="anchor-id"'); }); }); + +describe('Container.toHtml tag allowlist (adversarial re-review, same class as C1)', () => { + test('a malicious tag value falls back to div -- no injected , no broken-out attrs', () => { + const { html } = toHtml({ tag: 'div>')).toBe(true); + }); + + test('a tag value outside the known-safe set falls back to div', () => { + const { html } = toHtml({ tag: 'script' }, 'child'); + expect(html.startsWith(' { + const { html } = toHtml({ tag: 'section' }, 'child'); + expect(html).toContain(''); + }); + + test('all other allowlisted tags still work', () => { + for (const tag of ['div', 'article', 'header', 'footer', 'main']) { + const { html } = toHtml({ tag }, 'child'); + expect(html.startsWith(`<${tag}`)).toBe(true); + expect(html.endsWith(``)).toBe(true); + } + }); +}); diff --git a/craft/src/components/layout/Container.tsx b/craft/src/components/layout/Container.tsx index 904c8e7..892ed2a 100644 --- a/craft/src/components/layout/Container.tsx +++ b/craft/src/components/layout/Container.tsx @@ -3,6 +3,19 @@ import { useNode, UserComponent } from '@craftjs/core'; import { cssPropsToString } from '../../utils/style-helpers'; import { escapeAttr } from '../../utils/escape'; +// The only tag names Container actually supports (matches the TS union +// below and the `tag` default in `.craft.props`). `tag` is settable via the +// AI `update_props` path and from deserialized saved state -- neither is +// type-checked at runtime -- so a malicious value like +// `div> + (ALLOWED_CONTAINER_TAGS as readonly unknown[]).includes(tag) ? (tag as ContainerTag) : 'div'; + interface ContainerProps { style?: CSSProperties; tag?: 'div' | 'section' | 'article' | 'header' | 'footer' | 'main'; @@ -42,6 +55,7 @@ export const Container: UserComponent = ({ }) => { const { connectors: { connect, drag } } = useNode(); + const safeTag = sanitizeContainerTag(tag); const needsBoxedWrapper = contentWidth === 'boxed'; const flexStyles = flexAlignFromTextAlign(style.textAlign); @@ -58,7 +72,7 @@ export const Container: UserComponent = ({ const idValue = cssId || anchorId || undefined; const el = React.createElement( - tag, + safeTag, { ref: (ref: HTMLElement | null): void => { if (ref) connect(drag(ref)); }, style: outerStyle, @@ -97,7 +111,7 @@ Container.craft = { /* ---------- HTML export ---------- */ (Container as any).toHtml = (props: ContainerProps, childrenHtml: string) => { - const tag = props.tag || 'div'; + const tag = sanitizeContainerTag(props.tag); const isBoxed = props.contentWidth === 'boxed'; const flexStyles = flexAlignFromTextAlign(props.style?.textAlign); diff --git a/craft/src/utils/html-export.test.ts b/craft/src/utils/html-export.test.ts index 25046d7..ba92cde 100644 --- a/craft/src/utils/html-export.test.ts +++ b/craft/src/utils/html-export.test.ts @@ -58,3 +58,42 @@ describe('buildDataAttrs escapes animation/animationDelay (C2)', () => { expect(html).toContain('data-animation-delay="0.5s"'); }); }); + +/** + * Adversarial re-review of C1: the `typeName === 'Container' || typeName === + * 'div'` fallback in renderNode (hit when a node's `resolvedName` isn't in + * the component resolver, e.g. legacy/tampered saved state) interpolated + * `props.tag` raw into `<${tag}` with no validation -- the same breakout + * class as the fixed Container.toHtml bug, reachable via deserialized saved + * state. + */ +describe('renderNode div-fallback allowlists props.tag', () => { + const makeUnresolvedDivState = (tag: unknown) => + JSON.stringify({ + ROOT: { + type: { resolvedName: 'div' }, // not in componentResolver -> hits the fallback branch + isCanvas: true, + props: { tag, style: {} }, + displayName: 'div', + custom: {}, + hidden: false, + nodes: [], + linkedNodes: {}, + }, + }); + + test('a malicious tag value falls back to div -- no injected ', () => { + const state = makeUnresolvedDivState('div> { + const state = makeUnresolvedDivState('section'); + const { html } = exportBodyHtml(state); + expect(html).toContain(''); + }); +}); diff --git a/craft/src/utils/html-export.ts b/craft/src/utils/html-export.ts index 7da9a9e..fcf06dd 100644 --- a/craft/src/utils/html-export.ts +++ b/craft/src/utils/html-export.ts @@ -1,6 +1,7 @@ import { componentResolver } from '../components/resolver'; import { cssPropsToString } from './style-helpers'; import { escapeHtml, escapeAttr } from './escape'; +import { sanitizeContainerTag } from '../components/layout/Container'; export interface ExportOptions { title?: string; @@ -94,7 +95,11 @@ function renderNode(nodes: Record, nodeId: string): { html: string // Fallback: wrap children in a div with inline styles if (typeName === 'Container' || typeName === 'div') { const styleStr = cssPropsToString(props.style); - const tag = props.tag || 'div'; + // `props.tag` reaches this fallback the same way it reaches + // `Container.toHtml` -- via AI `update_props` or deserialized saved + // state, neither type-checked at runtime -- so it must be allowlisted + // before hitting the `<${tag}` template position below. + const tag = sanitizeContainerTag(props.tag); return { html: `<${tag}${dataAttrs}${styleStr ? ` style="${styleStr}"` : ''}>${allChildrenHtml}`, }; diff --git a/craft/src/utils/style-helpers.test.ts b/craft/src/utils/style-helpers.test.ts index 3c2df45..43fcbec 100644 --- a/craft/src/utils/style-helpers.test.ts +++ b/craft/src/utils/style-helpers.test.ts @@ -55,6 +55,39 @@ describe('cssPropsToString sanitizes emitted values (A5)', () => { }); }); +describe('cssPropsToString sanitizes emitted VALUES that are not strings (adversarial re-review of C1)', () => { + test('an array value containing an attribute-breakout payload is neutralized', () => { + const out = cssPropsToString({ + color: ['red', '">'], + } as any); + // No raw `"` (would close style="...") and no raw `<`/`>` (defense in + // depth) may survive -- they must come back as HTML entities, leaving + // the payload as inert text rather than a live tag. + expect(out).not.toMatch(/[<>"]/); + expect(out).toContain('<img'); + expect(out).toContain('"'); + }); + + test('an object value containing an attribute-breakout payload is neutralized', () => { + const out = cssPropsToString({ + color: { toString: () => '">' }, + } as any); + expect(out).not.toMatch(/[<>"]/); + expect(out).toContain('<img'); + expect(out).toContain('"'); + }); + + test('a number value is emitted raw/unchanged', () => { + const out = cssPropsToString({ zIndex: 5 } as any); + expect(out).toBe('z-index:5'); + }); + + test('a normal string value still works after the fix', () => { + const out = cssPropsToString({ color: 'red' } as any); + expect(out).toBe('color:red'); + }); +}); + describe('cssPropsToString sanitizes emitted KEYS (C1 -- style-object key breakout)', () => { test('a key containing a double-quote is dropped entirely, not emitted', () => { const out = cssPropsToString({ diff --git a/craft/src/utils/style-helpers.ts b/craft/src/utils/style-helpers.ts index ea9f38f..3288b55 100644 --- a/craft/src/utils/style-helpers.ts +++ b/craft/src/utils/style-helpers.ts @@ -9,10 +9,16 @@ const URL_RE = /url\(\s*(['"]?)([\s\S]*?)\1\s*\)/gi; // Outside of a url(...) reference, a `;` is never legitimate (declarations // are separated by it) -- stray semicolons are how a breakout injects a // second property -- and a raw `"` would close the `style="..."` attribute -// early. Inside url('...') the content has already been made safe via -// escapeAttr(safeUrl(...)), including any `;` required by data-URI syntax -// (`data:;base64,`), so this must never be applied there. -const sanitizeBreakoutChars = (s: string): string => s.replace(/;/g, '').replace(/"/g, '"'); +// early. `<`/`>` are escaped too as defense-in-depth: they're inert inside a +// properly quote-terminated `style="..."` attribute, but a value can reach +// this function from a non-string source (array/object coerced via +// `String(v)`, see `cssPropsToString` below) so we don't want to rely solely +// on the outer quote holding. Inside url('...') the content has already been +// made safe via escapeAttr(safeUrl(...)), including any `;` required by +// data-URI syntax (`data:;base64,`), so this must never be +// applied there. +const sanitizeBreakoutChars = (s: string): string => + s.replace(/;/g, '').replace(/"/g, '"').replace(//g, '>'); /** * Sanitizes a single CSS declaration value so it can never terminate the @@ -60,7 +66,14 @@ export function cssPropsToString(style: CSSProperties | undefined): string { .filter(([, v]) => v !== undefined && v !== null && v !== '') .map(([k, v]) => [camelToKebab(k), v] as const) .filter(([k]) => VALID_CSS_KEY_RE.test(k)) - .map(([k, v]) => `${k}:${typeof v === 'string' ? sanitizeCssValue(v) : v}`) + // Only a genuine `number` is safe to interpolate raw (numbers can never + // contain a `"`/`<`/`>`/`;` breakout character). Every other type -- + // string, array, object, etc. -- must be coerced to a string and run + // through `sanitizeCssValue`. Without this, a non-string value (e.g. an + // array like `['red', '">']`) skips + // sanitization entirely and is template-coerced (`${v}`) raw into the + // `style="..."` attribute, breaking out via the un-escaped `"`. + .map(([k, v]) => `${k}:${typeof v === 'number' ? v : sanitizeCssValue(String(v))}`) .join(';'); }