Merge pull request #804 from Automattic/fix/region-effect-analyzer-fail-closed

fix(blocks-engine): make region-effect analyzer fail closed and typed

Author: chubes4

packages/blocks-engine/CHANGELOG.md

@@ -8,6 +8,12 @@ This project follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 
 This package uses [Semantic Versioning](https://semver.org/). Deprecations are warned one minor version ahead of removal.
 
+## [Unreleased]
+
+### Fixed
+
+- `analyzeRuntimeRegionEffects` (unreleased) now fails closed on unparseable source — the manifest carries a single whole-source unit with `reason: 'parse_failed'` instead of an empty, effect-free-looking unit list — and its shared-state detection registers every binding a top-level statement contributes outside function bodies (destructuring, `function`/`class` declarations, loop heads, nested blocks), which previously escaped it and could mark shared-state effects as independently suppressible. `getElementById` targets that are not plain CSS identifiers are emitted as escaped `[id="…"]` selectors.
+
 ## [0.2.2] - 2026-06-30
 
 ### Added

packages/blocks-engine/src/escape.ts

@@ -1,7 +1,7 @@
 // src/lib/html-escape.ts
 //
-// The single home for HTML entity escaping. Three escalating variants: pick by
-// context, not convenience.
+// The single home for HTML entity and CSS selector escaping. Pick the variant
+// by context, not convenience.
 
 /** Escape &, <, >: the minimal set for HTML text-node content. */
 export function escapeHtmlText(s: string): string {
@@ -17,3 +17,20 @@ export function escapeHtmlAttr(s: string): string {
 export function escapeHtml(s: string): string {
   return escapeHtmlAttr(s).replace(/'/g, '&#039;');
 }
+
+// CSS selector escaping, for synthesizing selectors from attribute values.
+
+/** True when `value` is a plain CSS identifier usable directly after `#`. */
+export function cssSimpleIdent(value: string): boolean {
+  return /^[A-Za-z_][A-Za-z0-9_-]*$/.test(value);
+}
+
+/** Escape a value for embedding in a double-quoted CSS attribute selector. */
+export function escapeCssAttrValue(value: string): string {
+  return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
+}
+
+/** Selector for an element id: `#id` when plain, else `tag[id="…"]`. */
+export function cssIdSelector(id: string, tag = ''): string {
+  return cssSimpleIdent(id) ? `#${id}` : `${tag}[id="${escapeCssAttrValue(id)}"]`;
+}

packages/blocks-engine/src/runtime/region-effect-manifest.test.ts

@@ -15,4 +15,31 @@ describe('analyzeRuntimeRegionEffects', () => {
     const dynamic = analyzeRuntimeRegionEffects(`document.querySelector(selector).addEventListener('click', () => {});`);
     expect(dynamic.units[0].reason).toBe('dynamic_selector');
   });
+
+  it('fails closed on unparseable source instead of reporting it effect-free', () => {
+    const broken = analyzeRuntimeRegionEffects(`document.querySelector('.carousel'`);
+    expect(broken.units).toHaveLength(1);
+    expect(broken.units[0].status).toBe('shared_or_unsplittable');
+    expect(broken.units[0].reason).toBe('parse_failed');
+    expect(broken.units[0].source).toMatchObject({ start: 0, end: broken.units[0].source.end });
+    const inert = analyzeRuntimeRegionEffects(`/* nothing to do */`);
+    expect(inert.units).toEqual([]);
+  });
+
+  it('fails closed for state shared through destructuring, function declarations, and loop heads', () => {
+    const destructured = analyzeRuntimeRegionEffects(`const { active } = window.state;\ndocument.querySelector('.carousel').addEventListener('click', () => active.toggle());`);
+    expect(destructured.units[1].reason).toBe('shared_state');
+    const arrayPattern = analyzeRuntimeRegionEffects(`let [first] = window.items;\ndocument.querySelector('.reveal').addEventListener('click', () => first.show());`);
+    expect(arrayPattern.units[1].reason).toBe('shared_state');
+    const declaredFunction = analyzeRuntimeRegionEffects(`function advance() {}\ndocument.querySelector('.carousel').addEventListener('click', () => advance());`);
+    expect(declaredFunction.units[1].reason).toBe('shared_state');
+    const loopHead = analyzeRuntimeRegionEffects(`for (const step of window.steps) { window.register(step); }\ndocument.querySelector('.carousel').addEventListener('click', () => window.play(step));`);
+    expect(loopHead.units[1].reason).toBe('shared_state');
+  });
+
+  it('escapes getElementById targets that are not plain CSS identifiers', () => {
+    const manifest = analyzeRuntimeRegionEffects(`document.getElementById('hero').addEventListener('click', () => {});\ndocument.getElementById('2col "grid"').addEventListener('click', () => {});`);
+    expect(manifest.units[0].targets).toEqual(['#hero']);
+    expect(manifest.units[1].targets).toEqual(['[id="2col \\"grid\\""]']);
+  });
 });

packages/blocks-engine/src/runtime/region-effect-manifest.ts

@@ -1,5 +1,7 @@
 import { createHash } from 'node:crypto';
 import { parse } from 'acorn';
+import type { AnyNode, Pattern, Program } from 'acorn';
+import { cssIdSelector } from '../escape.js';
 
 export type RuntimeEffectUnit = {
   id: string;
@@ -9,7 +11,7 @@ export type RuntimeEffectUnit = {
   mutations: string[];
   dependencies: string[];
   status: 'independently_suppressible' | 'shared_or_unsplittable';
-  reason?: 'dynamic_selector' | 'shared_state' | 'unrecognized_pattern';
+  reason?: 'dynamic_selector' | 'shared_state' | 'unrecognized_pattern' | 'parse_failed';
 };
 
 export type RegionEffectManifest = {
@@ -18,39 +20,99 @@ export type RegionEffectManifest = {
   units: RuntimeEffectUnit[];
 };
 
+const SCHEMA = 'blocks-engine/runtime-region-effects/v1' as const;
+
+const CLASS_MUTATORS = new Set(['add', 'remove', 'toggle', 'replace']);
+
 const hash = (value: string) => createHash('sha256').update(value).digest('hex');
 
 /**
  * Produces an ownership manifest from top-level DOM-effect statements. This is
- * deliberately bounded: unsupported AST shapes remain retained as a whole.
+ * deliberately bounded: unsupported AST shapes remain retained as a whole, and
+ * unparseable source yields a single whole-source unretirable unit rather than
+ * an empty (effect-free-looking) manifest.
  */
 export function analyzeRuntimeRegionEffects(source: string): RegionEffectManifest {
   const sourceHash = hash(source);
-  let program: any;
+  let program: Program;
   try {
     program = parse(source, { ecmaVersion: 'latest', sourceType: 'script' });
   } catch {
-    return { schema: 'blocks-engine/runtime-region-effects/v1', sourceHash, units: [] };
+    return {
+      schema: SCHEMA,
+      sourceHash,
+      units: [
+        {
+          id: `effect_1_${sourceHash.slice(0, 12)}`,
+          source: { start: 0, end: source.length, hash: sourceHash },
+          targets: [],
+          events: [],
+          mutations: [],
+          dependencies: [],
+          status: 'shared_or_unsplittable',
+          reason: 'parse_failed',
+        },
+      ],
+    };
   }
 
-  const declared = new Map<string, number>();
-  for (const statement of program.body) {
-    if (statement.type === 'VariableDeclaration') {
-      for (const declaration of statement.declarations) {
-        if (declaration.id.type === 'Identifier') declared.set(declaration.id.name, (declared.get(declaration.id.name) ?? 0) + 1);
-      }
-    }
-  }
+  const declared = new Set<string>();
+  for (const statement of program.body) collectSharedBindings(statement, declared);
 
   return {
-    schema: 'blocks-engine/runtime-region-effects/v1',
+    schema: SCHEMA,
     sourceHash,
-    units: program.body.map((statement: any, index: number) => unitFor(statement, index, source, declared)),
+    units: program.body.map((statement, index) => unitFor(statement, index, source, declared)),
   };
 }
 
-function unitFor(statement: any, index: number, source: string, declared: Map<string, number>): RuntimeEffectUnit {
+/**
+ * Registers every binding a top-level statement can contribute to program
+ * scope: declarations in nested blocks and loop heads included, function
+ * bodies excluded (their bindings are local). Block-scoped bindings in nested
+ * blocks over-collect, which only fails closed.
+ */
+function collectSharedBindings(value: unknown, names: Set<string>): void {
+  if (Array.isArray(value)) {
+    for (const child of value) collectSharedBindings(child, names);
+    return;
+  }
+  if (!isAstNode(value)) return;
+  if ((value.type === 'FunctionDeclaration' || value.type === 'ClassDeclaration') && value.id) names.add(value.id.name);
+  if (value.type === 'FunctionDeclaration' || value.type === 'FunctionExpression' || value.type === 'ArrowFunctionExpression') return;
+  if (value.type === 'VariableDeclaration') {
+    for (const declaration of value.declarations) collectPatternNames(declaration.id, names);
+  }
+  for (const child of Object.values(value)) collectSharedBindings(child, names);
+}
+
+function collectPatternNames(pattern: Pattern, names: Set<string>): void {
+  switch (pattern.type) {
+    case 'Identifier':
+      names.add(pattern.name);
+      break;
+    case 'ObjectPattern':
+      for (const property of pattern.properties) {
+        collectPatternNames(property.type === 'RestElement' ? property.argument : property.value, names);
+      }
+      break;
+    case 'ArrayPattern':
+      for (const element of pattern.elements) {
+        if (element) collectPatternNames(element, names);
+      }
+      break;
+    case 'RestElement':
+      collectPatternNames(pattern.argument, names);
+      break;
+    case 'AssignmentPattern':
+      collectPatternNames(pattern.left, names);
+      break;
+  }
+}
+
+function unitFor(statement: AnyNode, index: number, source: string, declared: Set<string>): RuntimeEffectUnit {
   const slice = source.slice(statement.start, statement.end);
+  const sliceHash = hash(slice);
   const targets = new Set<string>();
   const events = new Set<string>();
   const mutations = new Set<string>();
@@ -65,32 +127,37 @@ function unitFor(statement: any, index: number, source: string, declared: Map<st
       recognized = true;
       const argument = node.arguments[0];
       if (argument.type !== 'Literal' || typeof argument.value !== 'string') dynamicSelector = true;
-      else targets.add(name === 'getElementById' ? `#${argument.value}` : argument.value);
+      else targets.add(name === 'getElementById' ? cssIdSelector(argument.value) : argument.value);
     }
     if (name === 'addEventListener' && node.arguments[0]?.type === 'Literal' && typeof node.arguments[0].value === 'string') {
       recognized = true;
       events.add(node.arguments[0].value);
     }
-    if (['add', 'remove', 'toggle', 'replace'].includes(name) && node.callee.object?.type === 'MemberExpression' && node.callee.object.property?.name === 'classList') mutations.add('class');
+    if (CLASS_MUTATORS.has(name) && node.callee.object.type === 'MemberExpression' && node.callee.object.property.type === 'Identifier' && node.callee.object.property.name === 'classList') mutations.add('class');
     if (name === 'setAttribute' && node.arguments[0]?.type === 'Literal' && typeof node.arguments[0].value === 'string') mutations.add(`attribute:${node.arguments[0].value}`);
   });
-  const shared = [...declared.keys()].some((name) => identifiers.has(name));
-  const reason = dynamicSelector ? 'dynamic_selector' : shared ? 'shared_state' : !recognized || !targets.size ? 'unrecognized_pattern' : undefined;
+  const dependencies = [...identifiers].filter((name) => declared.has(name)).sort();
+  const reason = dynamicSelector ? 'dynamic_selector' : dependencies.length ? 'shared_state' : !recognized || !targets.size ? 'unrecognized_pattern' : undefined;
   const unit: RuntimeEffectUnit = {
-    id: `effect_${index + 1}_${hash(slice).slice(0, 12)}`,
-    source: { start: statement.start, end: statement.end, hash: hash(slice) },
+    id: `effect_${index + 1}_${sliceHash.slice(0, 12)}`,
+    source: { start: statement.start, end: statement.end, hash: sliceHash },
     targets: [...targets].sort(), events: [...events].sort(), mutations: [...mutations].sort(),
-    dependencies: [...identifiers].filter((name) => declared.has(name)).sort(),
+    dependencies,
     status: reason ? 'shared_or_unsplittable' : 'independently_suppressible',
   };
   return reason ? { ...unit, reason } : unit;
 }
 
-function walk(node: any, visit: (node: any) => void) {
-  if (!node || typeof node !== 'object' || typeof node.type !== 'string') return;
-  visit(node);
-  for (const value of Object.values(node)) {
-    if (Array.isArray(value)) value.forEach((child) => walk(child, visit));
-    else walk(value, visit);
+function isAstNode(value: unknown): value is AnyNode {
+  return !!value && typeof value === 'object' && typeof (value as { type?: unknown }).type === 'string';
+}
+
+function walk(value: unknown, visit: (node: AnyNode) => void): void {
+  if (Array.isArray(value)) {
+    for (const child of value) walk(child, visit);
+    return;
   }
+  if (!isAstNode(value)) return;
+  visit(value);
+  for (const child of Object.values(value)) walk(child, visit);
 }

packages/blocks-engine/src/theme/section-extract.ts

@@ -2,6 +2,7 @@ import * as cheerio from 'cheerio';
 import type { CheerioAPI } from 'cheerio';
 import type { InteractionModel, SectionSpec, SectionSpecImage } from './section-spec.js';
 import type { SitePage } from './types.js';
+import { cssIdSelector, escapeCssAttrValue } from '../escape.js';
 
 type ElementNode = NonNullable<Parameters<CheerioAPI>[0]> & {
   type?: string;
@@ -341,16 +342,16 @@ function parseDimension(value: string | undefined): number {
 function selectorForElement($: CheerioAPI, el: ElementNode): string {
   const tag = tagName(el);
   const id = attr(el, 'id');
-  if (id) return simpleIdent(id) ? `#${id}` : `${tag}[id="${escapeAttr(id)}"]`;
+  if (id) return cssIdSelector(id, tag);
 
   const labelledBy = attr(el, 'aria-labelledby');
-  if (labelledBy) return `${tag}[aria-labelledby="${escapeAttr(labelledBy)}"]`;
+  if (labelledBy) return `${tag}[aria-labelledby="${escapeCssAttrValue(labelledBy)}"]`;
 
   const ariaLabel = attr(el, 'aria-label');
-  if (ariaLabel) return `${tag}[aria-label="${escapeAttr(ariaLabel)}"]`;
+  if (ariaLabel) return `${tag}[aria-label="${escapeCssAttrValue(ariaLabel)}"]`;
 
   const role = attr(el, 'role');
-  if (role) return `${tag}[role="${escapeAttr(role)}"]`;
+  if (role) return `${tag}[role="${escapeCssAttrValue(role)}"]`;
 
   if (!/^h[1-6]$/.test(tag) && $(tag).length === 1) return tag;
 
@@ -408,10 +409,3 @@ function pushUnique(values: string[], text: string): void {
   if (text && !values.includes(text)) values.push(text);
 }
 
-function simpleIdent(value: string): boolean {
-  return /^[A-Za-z_][A-Za-z0-9_-]*$/.test(value);
-}
-
-function escapeAttr(value: string): string {
-  return value.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
-}