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/<file>, 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 <noreply@anthropic.com>

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<string, unknown>)) {
+    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<string, unknown>)) 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([]);
+  });
+});

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<string, unknown>) => ({ type: 'patch' as const, message: 'm', ops: [{ op: 'update_props' as const, node_id: 't', props }] });
+  const propsOf = (r: ReturnType<typeof cleanAiResponse>) => (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: '<p>x</p>' }), as('HtmlBlock')))).toEqual({ code: '<p>x</p>' });
+    expect(propsOf(cleanAiResponse(patch({ code: '<p>x</p>', text: 'a' }), as('TextBlock')))).toEqual({ text: 'a' });
+    expect(propsOf(cleanAiResponse(patch({ code: '<p>x</p>' }), as(undefined)))).toEqual({});
+    expect(propsOf(cleanAiResponse(patch({ code: '<p>x</p>' })))).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: '<p>ok</p>' } } },
+      { op: 'replace_node', node_id: 'b', tree: { type: { resolvedName: 'Container' }, props: { code: '<p>no</p>', tag: 'div' } } },
+    ] });
+    if (r.type !== 'patch') throw new Error('type');
+    expect((r.ops[0] as any).tree.props).toEqual({ code: '<p>ok</p>' });
+    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));
+});

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/<file> 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<string> = 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/<file>, 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/<file>), 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<string, unknown> | undefined {
@@ -110,6 +137,8 @@ function cleanStyle(style: unknown): Record<string, unknown> | 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<string, unknown> = {};
   for (const [k, v] of Object.entries(obj as Record<string, unknown>)) {
     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<string, unknown> {
   const p = (props && typeof props === 'object' && !Array.isArray(props)) ? (props as Record<string, unknown>) : {};
   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<string, unknown>).code === 'string') props.code = (op.props as Record<string, unknown>).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;
 }

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[] = [];