From 9bbdcc4ec95c6086e0d4b6be497cd4a8c122917c Mon Sep 17 00:00:00 2001 From: Via Date: Thu, 16 Jul 2026 18:07:58 +0000 Subject: [PATCH] fix(studio,runtime): CSS.escape ids so digit-leading selectors don't crash MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The runtime picker built raw `#${id}` selectors while its sibling attribute-selector branches (data-composition-id, data-composition-src, data-track-index) already CSS.escape'd their values. When a user composition has an element with a digit-leading id (e.g. `id="0"`), the picker emits the selector `#0` which is invalid per the CSS spec — downstream `document.querySelector` throws SyntaxError. Same failure mode reached the Studio thumbnail: getElementScreenshotClip called `document.querySelectorAll(selector)` unguarded, so an invalid selector bubbling out of page.evaluate failed the whole thumbnail and returned 500 to the browser (broken thumbnail image). Fixes: - packages/core/src/runtime/picker.ts — CSS.escape the id, matching the sibling branches on lines 100/102/104. - packages/studio-server/src/helpers/screenshotClip.ts — catch SyntaxError from an invalid selector and return undefined so the caller falls back to a full-page screenshot, so the user still sees a thumbnail instead of a broken image. Regression tests for both. Reported via #hf-cli-feedback (Slack ts=1784218060, darwin/arm64, CLI 0.7.60): "digit-leading worker IDs broke Studio thumbnail querySelectorAll". — Via --- packages/core/src/runtime/picker.test.ts | 81 ++++++++++++++++++- packages/core/src/runtime/picker.ts | 5 +- .../src/helpers/screenshotClip.test.ts | 54 +++++++++++++ .../src/helpers/screenshotClip.ts | 16 +++- 4 files changed, 151 insertions(+), 5 deletions(-) create mode 100644 packages/studio-server/src/helpers/screenshotClip.test.ts diff --git a/packages/core/src/runtime/picker.test.ts b/packages/core/src/runtime/picker.test.ts index 7e04a6670..a6a41ac95 100644 --- a/packages/core/src/runtime/picker.test.ts +++ b/packages/core/src/runtime/picker.test.ts @@ -1,6 +1,37 @@ -import { describe, it, expect, vi, afterEach } from "vitest"; +import { describe, it, expect, vi, afterEach, beforeAll } from "vitest"; import { createPickerModule } from "./picker"; +// jsdom does not implement CSS.escape — polyfill with a spec-adjacent version. +// (Parallel polyfills already live in compositionLoader.test.ts / +// startResolver.test.ts, but each test file runs in an isolated environment.) +beforeAll(() => { + const css = globalThis.CSS as { escape?: (input: string) => string } | undefined; + if (!css || typeof css.escape !== "function") { + (globalThis as { CSS?: { escape: (input: string) => string } }).CSS = { + ...(css ?? {}), + escape: (value: string) => { + let out = ""; + for (let i = 0; i < value.length; i += 1) { + const ch = value[i] ?? ""; + const code = ch.charCodeAt(0); + const isDigit = code >= 48 && code <= 57; + const isAlpha = (code >= 65 && code <= 90) || (code >= 97 && code <= 122); + const isWordSafe = isAlpha || code === 45 || code === 95 || code >= 128; // - _ non-ASCII + const leadingDigit = i === 0 && isDigit; + if (leadingDigit) { + out += `\\${code.toString(16)} `; + } else if (isDigit || isWordSafe) { + out += ch; + } else { + out += `\\${ch}`; + } + } + return out; + }, + }; + } +}); + function createMockPostMessage() { return vi.fn(); } @@ -181,4 +212,52 @@ describe("createPickerModule", () => { expect(document.body.classList.contains("__hf-pick-active")).toBe(true); }); }); + + describe("buildElementSelector escapes digit-leading ids", () => { + it('produces a CSS-valid selector for id="0" and picks the element back', () => { + // Regression: a user's HTML with id="0" (or any digit-leading id) used + // to produce the raw selector "#0", which is invalid per the CSS spec — + // downstream querySelector calls threw SyntaxError. buildElementSelector + // now CSS.escapes the id. + const picker = createPickerModule({ postMessage: createMockPostMessage() }); + picker.installPickerApi(); + const el = document.createElement("div"); + el.id = "0"; + Object.assign(el.style, { + position: "absolute", + left: "0px", + top: "0px", + width: "40px", + height: "40px", + }); + document.body.appendChild(el); + + // Force elementsFromPoint to hit our div so we exercise the real code + // path that calls buildElementSelector via extractElementInfo. + const originalElementsFromPoint = document.elementsFromPoint; + Object.defineProperty(document, "elementsFromPoint", { + configurable: true, + value: () => [el], + }); + try { + const api = ( + window as { + __HF_PICKER_API?: { + pickAtPoint?: (x: number, y: number) => { selector: string } | null; + }; + } + ).__HF_PICKER_API; + const picked = api?.pickAtPoint?.(10, 10); + expect(picked?.selector).toBe("#\\30 "); + // And the round trip must find the element back through querySelector. + expect(() => document.querySelector(picked?.selector ?? "")).not.toThrow(); + expect(document.querySelector(picked?.selector ?? "")).toBe(el); + } finally { + Object.defineProperty(document, "elementsFromPoint", { + configurable: true, + value: originalElementsFromPoint, + }); + } + }); + }); }); diff --git a/packages/core/src/runtime/picker.ts b/packages/core/src/runtime/picker.ts index 5d7310252..38747c934 100644 --- a/packages/core/src/runtime/picker.ts +++ b/packages/core/src/runtime/picker.ts @@ -95,7 +95,10 @@ export function createPickerModule(deps: PickerModuleDeps): PickerModule { function buildElementSelector(el: Element): string { const htmlEl = el as HTMLElement; - if (htmlEl.id) return `#${htmlEl.id}`; + // Escape the ID so digit-leading or otherwise CSS-illegal ids (e.g. `#0`, + // `#1`) produce valid selectors — `document.querySelector("#0")` throws + // SyntaxError per the CSS spec. Sibling branches below already escape. + if (htmlEl.id) return `#${CSS.escape(htmlEl.id)}`; const compositionId = el.getAttribute("data-composition-id"); if (compositionId) return `[data-composition-id="${CSS.escape(compositionId)}"]`; const compositionSrc = el.getAttribute("data-composition-src"); diff --git a/packages/studio-server/src/helpers/screenshotClip.test.ts b/packages/studio-server/src/helpers/screenshotClip.test.ts new file mode 100644 index 000000000..6ea7d3580 --- /dev/null +++ b/packages/studio-server/src/helpers/screenshotClip.test.ts @@ -0,0 +1,54 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { getElementScreenshotClip } from "./screenshotClip"; + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("getElementScreenshotClip", () => { + it("returns undefined (not throws) when the selector is CSS-invalid", () => { + // Regression: an HTML element with `id="0"` produces the selector `#0`, + // which is invalid per the CSS spec — `document.querySelectorAll('#0')` + // throws SyntaxError. Puppeteer surfaces that as a page.evaluate error, + // which used to bubble up and fail the whole thumbnail. The clip helper + // now swallows the SyntaxError so callers fall back to a full-page shot. + const el = document.createElement("div"); + el.id = "0"; + Object.assign(el.style, { + width: "100px", + height: "80px", + }); + document.body.appendChild(el); + + expect(() => getElementScreenshotClip("#0")).not.toThrow(); + expect(getElementScreenshotClip("#0")).toBeUndefined(); + }); + + it("returns undefined (not throws) for garbage selectors", () => { + expect(() => getElementScreenshotClip("::: garbage :::")).not.toThrow(); + expect(getElementScreenshotClip("::: garbage :::")).toBeUndefined(); + }); + + it("returns a clip for a well-formed selector matching a visible element", () => { + const el = document.createElement("div"); + el.id = "hero"; + el.getBoundingClientRect = () => + ({ + left: 10, + top: 20, + width: 100, + height: 80, + right: 110, + bottom: 100, + x: 10, + y: 20, + toJSON: () => ({}), + }) as DOMRect; + document.body.appendChild(el); + + const clip = getElementScreenshotClip("#hero"); + expect(clip).toBeDefined(); + expect(clip?.width).toBeGreaterThan(0); + expect(clip?.height).toBeGreaterThan(0); + }); +}); diff --git a/packages/studio-server/src/helpers/screenshotClip.ts b/packages/studio-server/src/helpers/screenshotClip.ts index a1db59033..36f244245 100644 --- a/packages/studio-server/src/helpers/screenshotClip.ts +++ b/packages/studio-server/src/helpers/screenshotClip.ts @@ -9,9 +9,19 @@ export function getElementScreenshotClip( selector: string, selectorIndex?: number, ): ScreenshotClip | undefined { - const matches = Array.from(document.querySelectorAll(selector)).filter( - (el): el is HTMLElement => el instanceof HTMLElement, - ); + // Guard against invalid CSS selectors (e.g. `#0` — a digit-leading id from + // user HTML that upstream producers forgot to CSS.escape). querySelectorAll + // throws SyntaxError on those, which bubbles out of page.evaluate and fails + // the whole thumbnail. Returning undefined here falls back to a full-page + // screenshot, so the user still sees a thumbnail instead of a broken image. + let matches: HTMLElement[]; + try { + matches = Array.from(document.querySelectorAll(selector)).filter( + (el): el is HTMLElement => el instanceof HTMLElement, + ); + } catch { + return undefined; + } const safeIndex = Math.max(0, Math.min(matches.length - 1, Math.floor(selectorIndex ?? 0))); const el = matches[safeIndex] ?? null; if (!(el instanceof HTMLElement)) return undefined;