From e5f30a4a5689333adad361fe3a48d6a6b3a299bd Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 12 Jul 2026 17:27:22 -0700 Subject: [PATCH] Fix C1/C2: XSS via unsanitized style keys and animation attrs C1: cssPropsToString emitted the camelToKebab'd style-object KEY unsanitized while only sanitizing the VALUE. A malicious style key containing a quote (reachable via AI update_props or deserialized saved state) could close the style="..." attribute and inject a live element. Now validates each key against a CSS property/custom-prop allowlist and drops anything that doesn't match. C2: buildDataAttrs (html-export.ts) interpolated props.animation and props.animationDelay directly into data-animation="..."/ data-animation-delay="..." with no escaping, for every exported node. Now routes both through escapeAttr. Co-Authored-By: Claude Opus 4.8 (1M context) --- craft/src/utils/html-export.test.ts | 60 ++++++++++++++++++++++++ craft/src/utils/html-export.ts | 6 +-- craft/src/utils/style-helpers.test.ts | 66 +++++++++++++++++++++++++++ craft/src/utils/style-helpers.ts | 15 +++++- 4 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 craft/src/utils/html-export.test.ts diff --git a/craft/src/utils/html-export.test.ts b/craft/src/utils/html-export.test.ts new file mode 100644 index 0000000..25046d7 --- /dev/null +++ b/craft/src/utils/html-export.test.ts @@ -0,0 +1,60 @@ +import { describe, test, expect } from 'vitest'; +import { exportBodyHtml } from './html-export'; + +/** + * C2: buildDataAttrs (internal to html-export.ts) previously interpolated + * `props.animation` / `props.animationDelay` directly into + * `data-animation="${...}"` / `data-animation-delay="${...}"` with no + * escaping. Runs for EVERY node (via renderNode -> buildDataAttrs -> + * injectAttrs) and is reachable via AI `update_props` + deserialized saved + * state. A malicious value like `x">` breaks + * out of the attribute and injects a live element. + */ +describe('buildDataAttrs escapes animation/animationDelay (C2)', () => { + const makeState = (props: Record) => + JSON.stringify({ + ROOT: { + type: { resolvedName: 'Container' }, + isCanvas: true, + props: { tag: 'div', style: {}, ...props }, + displayName: 'Container', + custom: {}, + hidden: false, + nodes: [], + linkedNodes: {}, + }, + }); + + test('malicious animation value is escaped -- no raw breakout, no injected ', () => { + const payload = 'x">'; + const state = makeState({ animation: payload }); + const { html } = exportBodyHtml(state); + + expect(html).not.toContain(''); + expect(html).not.toMatch(/data-animation="x">/); + // The attribute value must be escaped, so no raw `"` or `<` survives + // inside the data-animation attribute region. + expect(html).toContain('data-animation="x"><img src=y onerror=alert(1)>"'); + }); + + test('malicious animationDelay value is escaped -- no raw breakout', () => { + const payload = 'x">'; + const state = makeState({ animation: 'fade-in', animationDelay: payload }); + const { html } = exportBodyHtml(state); + + expect(html).not.toContain(''); + expect(html).toContain('data-animation-delay="x"><script>alert(1)</script>"'); + }); + + test('normal animation value still emits data-animation="fade-in" unchanged', () => { + const state = makeState({ animation: 'fade-in' }); + const { html } = exportBodyHtml(state); + expect(html).toContain('data-animation="fade-in"'); + }); + + test('normal animationDelay value still emits unchanged', () => { + const state = makeState({ animation: 'fade-in', animationDelay: '0.5s' }); + const { html } = exportBodyHtml(state); + expect(html).toContain('data-animation-delay="0.5s"'); + }); +}); diff --git a/craft/src/utils/html-export.ts b/craft/src/utils/html-export.ts index 0179435..7da9a9e 100644 --- a/craft/src/utils/html-export.ts +++ b/craft/src/utils/html-export.ts @@ -1,6 +1,6 @@ import { componentResolver } from '../components/resolver'; import { cssPropsToString } from './style-helpers'; -import { escapeHtml } from './escape'; +import { escapeHtml, escapeAttr } from './escape'; export interface ExportOptions { title?: string; @@ -24,9 +24,9 @@ function buildDataAttrs(props: Record): string { if (props.hideOnTablet) attrs += ' data-hide-tablet'; if (props.hideOnMobile) attrs += ' data-hide-mobile'; if (props.animation && props.animation !== 'none') { - attrs += ` data-animation="${props.animation}"`; + attrs += ` data-animation="${escapeAttr(String(props.animation))}"`; if (props.animationDelay && props.animationDelay !== '0') { - attrs += ` data-animation-delay="${props.animationDelay}"`; + attrs += ` data-animation-delay="${escapeAttr(String(props.animationDelay))}"`; } } return attrs; diff --git a/craft/src/utils/style-helpers.test.ts b/craft/src/utils/style-helpers.test.ts index d79a2f6..3c2df45 100644 --- a/craft/src/utils/style-helpers.test.ts +++ b/craft/src/utils/style-helpers.test.ts @@ -54,3 +54,69 @@ describe('cssPropsToString sanitizes emitted values (A5)', () => { expect(withoutEntities).not.toContain(';'); }); }); + +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({ + color: 'red', + '">': '1', + } as any); + expect(out).not.toContain('"> is dropped', () => { + const out = cssPropsToString({ 'foo>bar': '1' } as any); + expect(out).toBe(''); + }); + + test('a key containing < is dropped', () => { + const out = cssPropsToString({ 'foo { + const out = cssPropsToString({ 'foo;bar': '1' } as any); + expect(out).toBe(''); + }); + + test('a key containing a space is dropped', () => { + const out = cssPropsToString({ 'foo bar': '1' } as any); + expect(out).toBe(''); + }); + + test('legit simple key "color" is preserved', () => { + const out = cssPropsToString({ color: 'red' } as any); + expect(out).toBe('color:red'); + }); + + test('legit camelCase key backgroundColor -> background-color is preserved', () => { + const out = cssPropsToString({ backgroundColor: 'blue' } as any); + expect(out).toBe('background-color:blue'); + }); + + test('a CSS custom property key (--custom-prop) is preserved', () => { + const out = cssPropsToString({ '--custom-prop': '10px' } as any); + expect(out).toBe('--custom-prop:10px'); + }); + + test('a vendor-prefixed key (-webkit-...) is preserved', () => { + const out = cssPropsToString({ WebkitBoxShadow: '0 0 1px red' } as any); + expect(out).toBe('-webkit-box-shadow:0 0 1px red'); + }); + + test('a digits-only key is dropped', () => { + const out = cssPropsToString({ '123': '1' } as any); + expect(out).toBe(''); + }); + + test('other legit keys in the same object still emit even when a malicious key is dropped', () => { + const out = cssPropsToString({ + color: 'red', + '">': 'x', + backgroundColor: 'blue', + } as any); + expect(out).toBe('color:red;background-color:blue'); + }); +}); diff --git a/craft/src/utils/style-helpers.ts b/craft/src/utils/style-helpers.ts index 9a84418..ea9f38f 100644 --- a/craft/src/utils/style-helpers.ts +++ b/craft/src/utils/style-helpers.ts @@ -43,11 +43,24 @@ function sanitizeCssValue(raw: string): string { return out; } +// A real CSS property name (`color`, `background-color`), vendor-prefixed +// property (`-webkit-box-shadow`), or custom property (`--custom-prop`) -- +// nothing else. This is a KEY allowlist, not a value sanitizer: it exists +// solely to stop a malicious style object KEY (e.g. +// `'">'`, reachable via AI `update_props` or +// deserialized saved state, which spread arbitrary keys into `p.style`) +// from being emitted unescaped into `style="${camelToKebab(k)}:${...}"` and +// closing the attribute early. Legitimate keys never contain `"`, `>`, `<`, +// `;`, whitespace, `{`, `}`, or digits-only, so this never rejects real CSS. +const VALID_CSS_KEY_RE = /^-{0,2}[a-z][a-z-]*$/; + export function cssPropsToString(style: CSSProperties | undefined): string { if (!style) return ''; return Object.entries(style) .filter(([, v]) => v !== undefined && v !== null && v !== '') - .map(([k, v]) => `${camelToKebab(k)}:${typeof v === 'string' ? sanitizeCssValue(v) : 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}`) .join(';'); }