From 4a34781ab6d2ee6c7be3aa3b619c8055b4bfe2bf Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 1 Nov 2026 11:38:14 -0800 Subject: [PATCH] Sitesmith cleaner: patches use the target component's rules; CSS values checked against a positive grammar Review findings on clean-ai-response.ts: 1. update_props is now cleaned with the TARGET node's component (resolved on the live canvas by apply()); an unresolvable target gets the strictest rules (no code, url/src only assets/). code is kept only on an HtmlBlock. 2. cssValueAllowed() is a positive grammar: numbers/lengths/hex/keywords, an allow-list of functions (gradients, colour functions, calc/clamp, var(--*), transforms, filters), url() only of assets/, quoted strings only plain font names; image-set(, src(, cross-fade(, element(, paint(, attr(, env(, ':' or '//' outside url(assets/..), backslashes and control chars rejected. Regression: every style/colour value in the templates, section families and palettes passes (clean-ai-response.ordinary.test.ts). Also: tricky link forms (//host, backslashes, whitespace/control chars, mixed-case schemes, > 2048) rejected; comments state the server-side mirror is not implemented (deferred). Co-Authored-By: Claude Opus 5.5 --- .../clean-ai-response.ordinary.test.ts | 33 +++++++++ craft/src/sitesmith/clean-ai-response.test.ts | 64 ++++++++++++++++ craft/src/sitesmith/clean-ai-response.ts | 73 ++++++++++++++----- craft/src/utils/apply-ai-response.ts | 7 +- 4 files changed, 159 insertions(+), 18 deletions(-) create mode 100644 craft/src/sitesmith/clean-ai-response.ordinary.test.ts diff --git a/craft/src/sitesmith/clean-ai-response.ordinary.test.ts b/craft/src/sitesmith/clean-ai-response.ordinary.test.ts new file mode 100644 index 0000000..7e895f9 --- /dev/null +++ b/craft/src/sitesmith/clean-ai-response.ordinary.test.ts @@ -0,0 +1,33 @@ +import { describe, test, expect } from 'vitest'; +import { cssValueAllowed } from './clean-ai-response'; +import { allTemplates } from '../templates/definitions'; +import { BUILTIN_SECTIONS } from '../sections/families'; +import { PALETTES } from '../constants/palettes'; + +/** + * Behaviour stays identical for ordinary values: every style value and colour/background prop the + * editor's own templates, section families and palettes use passes the cleaner's CSS grammar, so a + * model that writes the same vocabulary loses nothing. + */ +const CSS_VALUE_KEY = /(color|colour|bg|background|gradientfrom|gradientto|bgvalue|shadow)$/i; +function collect(o: unknown, out: Array<[string, unknown]>, key = '', depth = 0): void { + if (depth > 40 || o === null || typeof o !== 'object') return; + if (Array.isArray(o)) { o.forEach((x) => collect(x, out, key, depth + 1)); return; } + for (const [k, v] of Object.entries(o as Record)) { + if (/^(style|[a-z][A-Za-z]*Style)$/.test(k) && v && typeof v === 'object' && !Array.isArray(v)) { + for (const [sk, sv] of Object.entries(v as Record)) out.push([`${k}.${sk}`, sv]); + } else if (typeof v === 'string' && CSS_VALUE_KEY.test(k) && !/^\s*([a-z][a-z0-9+.-]*:|\/|assets\/)/i.test(v)) { + out.push([k, v]); + } else collect(v, out, k, depth + 1); + } +} + +describe('ordinary editor vocabulary passes the CSS grammar', () => { + test('templates, section families and palettes: no value would be dropped', () => { + const vals: Array<[string, unknown]> = []; + collect(allTemplates, vals); collect(BUILTIN_SECTIONS, vals); collect(PALETTES, vals); + expect(vals.length).toBeGreaterThan(1000); + const rejected = vals.filter(([, v]) => v !== '' && v !== undefined && v !== null && typeof v !== 'boolean' && !cssValueAllowed(v)); + expect(rejected.map(([k, v]) => `${k}=${String(v)}`)).toEqual([]); + }); +}); diff --git a/craft/src/sitesmith/clean-ai-response.test.ts b/craft/src/sitesmith/clean-ai-response.test.ts index bb4b466..1772b9e 100644 --- a/craft/src/sitesmith/clean-ai-response.test.ts +++ b/craft/src/sitesmith/clean-ai-response.test.ts @@ -125,3 +125,67 @@ describe('cleanAiResponse', () => { expect(cleanAiResponse(a)).toBe(a); }); }); + +describe('patches are cleaned with the TARGET component\'s rules (review finding 1)', () => { + const patch = (props: Record) => ({ type: 'patch' as const, message: 'm', ops: [{ op: 'update_props' as const, node_id: 't', props }] }); + const propsOf = (r: ReturnType) => (r.type === 'patch' && r.ops[0].op === 'update_props' ? r.ops[0].props : null); + const as = (name: string | undefined) => () => name; + test('HtmlBlock: code kept only when the target IS an HtmlBlock', () => { + expect(propsOf(cleanAiResponse(patch({ code: '

x

' }), as('HtmlBlock')))).toEqual({ code: '

x

' }); + expect(propsOf(cleanAiResponse(patch({ code: '

x

', text: 'a' }), as('TextBlock')))).toEqual({ text: 'a' }); + expect(propsOf(cleanAiResponse(patch({ code: '

x

' }), as(undefined)))).toEqual({}); + expect(propsOf(cleanAiResponse(patch({ code: '

x

' })))).toEqual({}); + }); + test('VideoBlock: url is a video URL on a VideoBlock; unresolvable target -> strictest (assets only)', () => { + const yt = 'https://www.youtube.com/watch?v=abc'; + expect(propsOf(cleanAiResponse(patch({ url: yt }), as('VideoBlock')))).toEqual({ url: yt }); + expect(propsOf(cleanAiResponse(patch({ url: yt }), as(undefined)))).toEqual({}); + expect(propsOf(cleanAiResponse(patch({ url: 'assets/clip.mp4' }), as(undefined)))).toEqual({ url: 'assets/clip.mp4' }); + expect(propsOf(cleanAiResponse(patch({ videoUrl: 'https://example.com/v.mp4' }), as('VideoBlock')))).toEqual({}); + }); + test('ContactForm / FormContainer: destinations are never patchable, resolved or not', () => { + for (const name of ['ContactForm', 'FormContainer', 'SubscribeForm', undefined]) { + expect(propsOf(cleanAiResponse(patch({ recipientEmail: 'a@example.com', action: '/x', webhookUrl: 'https://example.com/h', heading: 'Hi' }), as(name)))).toEqual({ heading: 'Hi' }); + } + }); + test('ButtonLink href and ImageBlock src', () => { + expect(propsOf(cleanAiResponse(patch({ href: 'https://example.com/' }), as('ButtonLink')))).toEqual({ href: 'https://example.com/' }); + expect(propsOf(cleanAiResponse(patch({ href: 'javascript:x' }), as(undefined)))).toEqual({}); + const px = 'https://images.pexels.com/photos/1/a.jpeg'; + expect(propsOf(cleanAiResponse(patch({ src: px }), as('ImageBlock')))).toEqual({ src: px }); + expect(propsOf(cleanAiResponse(patch({ src: px }), as(undefined)))).toEqual({}); + expect(propsOf(cleanAiResponse(patch({ src: 'https://example.com/a.png' }), as('ImageBlock')))).toEqual({}); + }); + test('Navbar: links[].href and logoUrl / logoImage', () => { + expect(propsOf(cleanAiResponse(patch({ logoUrl: '/', logoImage: 'assets/logo.png', links: [{ text: 'A', href: '#a' }, { text: 'B', href: 'vbscript:x' }] }), as('Navbar')))) + .toEqual({ logoUrl: '/', logoImage: 'assets/logo.png', links: [{ text: 'A', href: '#a' }, { text: 'B' }] }); + }); + test('a componentOf that throws is treated as unresolvable (strict)', () => { + expect(propsOf(cleanAiResponse(patch({ code: 'x', src: 'https://images.pexels.com/a.jpg' }), () => { throw new Error('gone'); }))).toEqual({}); + }); + test('replace_node / insert_* trees carry their component names', () => { + const r = cleanAiResponse({ type: 'patch', message: 'm', ops: [ + { op: 'insert_after', node_id: 'a', tree: { type: { resolvedName: 'HtmlBlock' }, props: { code: '

ok

' } } }, + { op: 'replace_node', node_id: 'b', tree: { type: { resolvedName: 'Container' }, props: { code: '

no

', tag: 'div' } } }, + ] }); + if (r.type !== 'patch') throw new Error('type'); + expect((r.ops[0] as any).tree.props).toEqual({ code: '

ok

' }); + expect((r.ops[1] as any).tree.props).toEqual({ tag: 'div' }); + }); +}); + +describe('cssValueAllowed: positive grammar (review finding 2)', () => { + test.each([ + 'red', 'transparent', 'inherit', '#0f172a', 'rgb(0 0 0 / 50%)', 'rgba(15, 23, 42, 0.8)', 'hsl(210 40% 98%)', + '1.5rem', '-0.02em', '100%', '0 4px 12px rgba(0,0,0,.1)', 'clamp(2.25rem, 6vw, 4rem)', 'calc(100% - 2 * 24px)', + 'var(--wsb-primary)', 'var(--wsb-radius, 8px)', 'linear-gradient(135deg, var(--wsb-primary) 0%, #111 100%)', + 'radial-gradient(circle at center, #fff, transparent)', 'conic-gradient(from 90deg, red, blue)', + "'Playfair Display', serif", '"Open Sans", sans-serif', '4 / 3', 'translateY(-4px)', 'cubic-bezier(0.4, 0, 0.2, 1)', + 'repeat(3, minmax(0, 1fr))', 'url(assets/bg.jpg)', "url('assets/bg.jpg') center / cover no-repeat", + ])('allows %s', (v) => expect(cssValueAllowed(v)).toBe(true)); + test.each([ + 'image-set("assets/a.jpg" 1x)', '-webkit-image-set(url(assets/a.jpg) 1x)', 'src("assets/a.jpg")', 'cross-fade(20% red, blue)', + 'element(#x)', 'paint(foo)', 'attr(data-x)', 'env(safe-area-inset-top)', "'https//example.com'", '"a:b"', 'x//y', + 'url(https://example.com/x.png)', 'url(//example.com/x.png)', 'red \\9', 'red\u0001', 'var(x)', 'red !important', + ])('rejects %s', (v) => expect(cssValueAllowed(v)).toBe(false)); +}); diff --git a/craft/src/sitesmith/clean-ai-response.ts b/craft/src/sitesmith/clean-ai-response.ts index c5bc341..0231462 100644 --- a/craft/src/sitesmith/clean-ai-response.ts +++ b/craft/src/sitesmith/clean-ai-response.ts @@ -49,7 +49,9 @@ const PROP_NAME = /^[A-Za-z_][A-Za-z0-9_]{0,63}$/; const CSS_PROP_NAME = /^(--[A-Za-z0-9-]{1,60}|-?[A-Za-z][A-Za-z-]{0,60})$/; const ASSET = /^assets\/[A-Za-z0-9._%~+-]{1,200}$/; -type UrlKind = 'link' | 'image' | 'video'; +/** 'strict': a URL prop whose meaning depends on an unknown component (url/src on an unresolvable + * patch target) -- only the site's own assets/ is allowed. */ +type UrlKind = 'link' | 'image' | 'video' | 'strict'; function httpsHost(v: string, allowHttp: boolean): string | null { let u: URL; @@ -66,6 +68,7 @@ export function allowUrl(kind: UrlKind, raw: unknown): string | null { if (v === '') return ''; if (kind === 'link' && /^tel:\+?[0-9 ().-]{3,32}$/i.test(v)) return v; if (v.length > 2048 || /[\u0000-\u0020<>"'`\\\u007f]/.test(v)) return null; + if (kind === 'strict') return ASSET.test(v) ? v : null; if (kind === 'image') { if (ASSET.test(v)) return v; const h = httpsHost(v, false); @@ -86,16 +89,40 @@ export function allowUrl(kind: UrlKind, raw: unknown): string | null { return head.includes(':') ? null : v; } -/** True when a CSS value is on the allow-list of value shapes. */ +/** CSS functions a value may use (positive list); anything else -- image-set(, src(, element(, + * paint(, cross-fade(, expression( ... -- makes the value invalid. url( is handled separately. */ +const CSS_FUNCS: ReadonlySet = new Set([ + 'var', 'rgb', 'rgba', 'hsl', 'hsla', 'hwb', 'lab', 'lch', 'oklab', 'oklch', 'color-mix', + 'linear-gradient', 'radial-gradient', 'conic-gradient', 'repeating-linear-gradient', 'repeating-radial-gradient', 'repeating-conic-gradient', + 'calc', 'min', 'max', 'clamp', 'minmax', 'repeat', 'fit-content', + 'translate', 'translatex', 'translatey', 'translate3d', 'rotate', 'scale', 'scalex', 'scaley', 'skew', 'skewx', 'skewy', 'matrix', 'perspective', + 'cubic-bezier', 'steps', + 'blur', 'brightness', 'contrast', 'grayscale', 'saturate', 'sepia', 'opacity', 'invert', 'hue-rotate', 'drop-shadow', +]); + +/** + * True when a CSS value matches the positive value grammar: numbers / lengths / percentages, hex + * colours, keywords and named colours, the functions in CSS_FUNCS (var() only of custom properties), + * url() only of the site's own assets/, and quoted strings that are plain font names. Never: + * backslashes, control characters, < > { } ; ! `, ':' or '//' outside url(assets/..), or any other + * function. + */ export function cssValueAllowed(raw: unknown): boolean { if (typeof raw === 'number') return Number.isFinite(raw); if (typeof raw !== 'string') return false; - const v = raw; - if (v.length > 400 || /[<>{};\\\u0000-\u001f]/.test(v)) return false; - if (/@import|expression\s*\(|behavior|-moz-binding|javascript:|vbscript:/i.test(v)) return false; - for (const m of v.matchAll(/url\(\s*(['"]?)(.*?)\1\s*\)/gi)) if (!ASSET.test(m[2].trim())) return false; - if (/url\(/i.test(v.replace(/url\(\s*(['"]?)(.*?)\1\s*\)/gi, ''))) return false; - return true; + let v = raw; + if (v.length > 400 || /[\\\u0000-\u001f\u007f<>{};!`]/.test(v)) return false; + // url(): only url(assets/), optionally quoted + let bad = false; + v = v.replace(/url\(\s*(['"]?)([^'")]*)\1\s*\)/gi, (_m, _q, inner: string) => { if (!ASSET.test(inner.trim())) bad = true; return ' '; }); + if (bad || /url\s*\(/i.test(v)) return false; + // quoted strings: plain font names only + v = v.replace(/(['"])([^'"]*)\1/g, (_m, _q, inner: string) => { if (!/^[A-Za-z0-9 _-]{1,60}$/.test(inner)) bad = true; return ' '; }); + if (bad || /['"]/.test(v)) return false; + if (v.includes(':') || v.includes('//')) return false; + for (const m of v.matchAll(/([A-Za-z][A-Za-z0-9-]*)\s*\(/g)) if (!CSS_FUNCS.has(m[1].toLowerCase())) return false; + for (const m of v.matchAll(/var\(\s*([^,)]*)/gi)) if (!/^--[A-Za-z0-9-]{1,60}$/.test(m[1].trim())) return false; + return /^[A-Za-z0-9#%.,()+\-*\/ _\s]*$/.test(v); } function cleanStyle(style: unknown): Record | undefined { @@ -110,6 +137,8 @@ function cleanStyle(style: unknown): Record | undefined { function urlKindFor(key: string, component: string | undefined): UrlKind | null { if (component === 'VideoBlock' && (key === 'url' || key === 'src')) return 'video'; + // unknown component (e.g. an update_props whose target cannot be resolved): the strictest reading + if (component === undefined && (key === 'url' || key === 'src')) return 'strict'; if (VIDEO_KEYS.has(key)) return 'video'; if (IMAGE_KEYS.has(key)) return 'image'; if (LINK_KEYS.has(key) || /(Href|Url|Link)$/.test(key)) return 'link'; @@ -152,13 +181,17 @@ function cleanObject(obj: unknown, component: string | undefined, depth: number) const out: Record = {}; for (const [k, v] of Object.entries(obj as Record)) { if (!PROP_NAME.test(k) || /^on[A-Z]/.test(k) || AI_FORBIDDEN_PROPS.has(k)) continue; + if (k === 'code') continue; // raw HTML: only HtmlBlock.code, re-added by cleanAiProps() const c = cleanValue(k, v, component, depth); if (c !== undefined) out[k] = c; } return out; } -/** Clean the props of a node of type `component` (or of an update_props op, component unknown). */ +/** + * Clean the props of a node of type `component`. `undefined` = unknown component (an update_props + * whose target could not be resolved): the strictest rules apply (no `code`, url/src only assets/). + */ export function cleanAiProps(props: unknown, component?: string): Record { const p = (props && typeof props === 'object' && !Array.isArray(props)) ? (props as Record) : {}; const out = cleanObject(p, component, 0) ?? {}; @@ -174,13 +207,15 @@ function cleanTree(tree: SerializedTreeNode): SerializedTreeNode { return node; } -function cleanOp(op: SitesmithPatchOp): SitesmithPatchOp { +/** Resolves a patch op's node_id to the target's component name (resolvedName), or undefined. */ +export type ComponentOf = (aiNodeId: string) => string | undefined; + +function cleanOp(op: SitesmithPatchOp, componentOf?: ComponentOf): SitesmithPatchOp { switch (op.op) { case 'update_props': { - // Component unknown here; HtmlBlock code in a patch keeps its server-side sanitising. - const props = cleanAiProps(op.props); - if (op.props && typeof (op.props as Record).code === 'string') props.code = (op.props as Record).code; - return { ...op, props }; + let component: string | undefined; + try { component = componentOf?.(op.node_id); } catch { component = undefined; } + return { ...op, props: cleanAiProps(op.props, component) }; } case 'replace_node': case 'insert_after': case 'insert_before': return { ...op, tree: cleanTree(op.tree) }; @@ -189,8 +224,12 @@ function cleanOp(op: SitesmithPatchOp): SitesmithPatchOp { } } -/** The whole response, cleaned. 'ask' passes through (it is only shown as text). */ -export function cleanAiResponse(resp: SitesmithResponse): SitesmithResponse { +/** + * The whole response, cleaned. 'ask' passes through (it is only shown as text). `componentOf` + * resolves update_props targets on the current canvas so component-dependent rules apply; without + * it (or for an unresolvable target) the strictest rules are used. + */ +export function cleanAiResponse(resp: SitesmithResponse, componentOf?: ComponentOf): SitesmithResponse { if (resp.type === 'replace') { return { ...resp, @@ -199,6 +238,6 @@ export function cleanAiResponse(resp: SitesmithResponse): SitesmithResponse { ...(resp.footer ? { footer: { tree: cleanTree(resp.footer.tree) } } : {}), }; } - if (resp.type === 'patch') return { ...resp, ops: Array.isArray(resp.ops) ? resp.ops.map(cleanOp) : [] }; + if (resp.type === 'patch') return { ...resp, ops: Array.isArray(resp.ops) ? resp.ops.map((o) => cleanOp(o, componentOf)) : [] }; return resp; } diff --git a/craft/src/utils/apply-ai-response.ts b/craft/src/utils/apply-ai-response.ts index 45f032c..335f9f7 100644 --- a/craft/src/utils/apply-ai-response.ts +++ b/craft/src/utils/apply-ai-response.ts @@ -96,7 +96,12 @@ export function useApplyAiResponse() { // G8: allow-list cleaner (URLs, images, CSS, ids; form destinations and head code can never be // set by the model). Runs first, so nothing below -- contrast repair, the canvas -- sees a // value the cleaner would refuse. (Server-side mirror not implemented, deferred.) - resp = cleanAiResponse(resp); + resp = cleanAiResponse(resp, (aiNodeId) => { + const id = findNodeIdByAiNodeId(query, aiNodeId); + if (!id) return undefined; + const name = query.node(id).get()?.data?.name; + return typeof name === 'string' ? name : undefined; + }); // Contrast auto-repair (design "Settled calls" 8): fix failing text colours before they reach the canvas. let adjustments: ContrastAdjustment[] = [];