fix(core,studio): escape user values in querySelector attribute selectors

[miguel-heygen]

Summary

• Extracts queryByAttr(root, attr, value, tag?) to packages/core/src/utils/cssSelector.ts — queries DOM by attribute presence then compares with exact ===, zero injection surface
• Fixes all querySelector sites across core and studio that interpolated raw user-authored values into attribute selectors
• Node-side (core compiler/parser): uses queryByAttr — no selector string interpolation at all
• Browser-side (runtime, studio): uses native CSS.escape()
• Bundler scope-building path: uses regex escaping for \ and " (CSS string context)

A " in a composition ID, script src URL, or data-start element reference would produce a malformed CSS selector that throws Attribute selector didn't terminate in css-select, crashing the entire pipeline.

Supersedes #1568 which fixed only the 3 bundler sites.

Approach

Instead of escaping values and interpolating into selector strings, queryByAttr queries for attribute presence ([attr]) then compares with ===. The user value never touches a CSS selector — there is nothing to escape and nothing to inject.

Test plan

• 10 injection tests for queryByAttr (double quotes, backslashes, brackets, injection attempts, newlines, leading digits, tag filtering)
• Bundler tests pass (36/36)
• Full CI green

[james-russo-rames-d-jusso]

Reviewed at 22ff0dd — direction is right (selector-side injection vector is real, queryByAttr is a clean shape), but this lands with two failing CI tests caused by the PR itself, and the diff has a few asymmetries / a self-introduced regression that need fixes before merge.

Blockers

1. CI is red — CSS is not defined in two existing Studio tests

CI ran the new code through Studio's vitest+jsdom env and both fixed sites blow up:

• packages/studio/src/components/editor/domEditingElement.ts:245 — CSS.escape(selection.hfId) → ReferenceError: CSS is not defined (test: domEditing.test.ts > findElementForSelection finds element by data-hf-id when no id or selector).
• packages/studio/src/player/lib/timelineElementHelpers.ts:286-287 — CSS.escape(id) → same (test: useTimelinePlayer.test.ts > findTimelineDomNodeForClip > matches anonymous manifest clips back to repeated DOM nodes in timeline order).

jsdom doesn't ship CSS.escape. Repo precedent is already at packages/core/src/compiler/compositionScoping.ts:343-345:

if (typeof CSS !== "undefined" && CSS && typeof CSS.escape === "function") {
  return root.querySelector("#" + CSS.escape(idValue)) || null;
}

Two fixes that would clear both tests:

• Use the new queryByAttr helper at these sites (it's already Node/SSR-safe by construction — that's the helper's whole point), OR
• Wrap with the typeof CSS !== "undefined" guard + fall back to getAttribute-compare.

Re your PR body: "Browser-side (runtime, studio): uses native CSS.escape()" — Studio's runtime hits a real browser, but Studio's tests and any SSR-ish path don't. The helper you introduced is the answer for both.

2. Self-introduced regression in cssAttributeSelector

packages/core/src/compiler/htmlBundler.ts:280-282 — the PR removes the prior backslash/quote escaping:

-  return `[${attr}="${value.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"]`;
+  return `[${attr}="${value}"]`;

But cssAttributeSelector still has two live callers in the same file:

• htmlBundler.ts:800 — buildScopeSelector for inlineSubCompositions (consumed by compositionScoping.ts:scopeCssToComposition as scopeSelectorOverride, which skips the internal escapeCssAttributeValue when an override is passed — compositionScoping.ts:175-177).
• htmlBundler.ts:864 — runtimeScope for scopeCssToComposition directly.

A " in a compId / runtimeCompId now writes a malformed scope selector into the bundled CSS — the exact failure mode this PR is fixing for querySelector, recreated in the bundler output. Either re-add the escape inside cssAttributeSelector, migrate the two callers to escapeCssAttributeValue directly, or delete cssAttributeSelector (only 2 callers).

Concerns

3. Sibling-asymmetry: identical sites not migrated

grep for un-escaped [data-composition-id="${var}"] selectors with user-controllable input:

• packages/studio/src/components/nle/NLELayout.tsx:191-192 — [data-composition-id="${compId}"][data-composition-src] where compId = element.id (TimelineElement). Same vector as the fixed timelineDOM.ts:126.
• packages/studio/src/player/lib/timelineElementHelpers.ts:165-168 — getTimelineElementSelector returns [data-composition-id="${compId}"] un-escaped (and #${el.id} at :166, .${firstClass} at :172). Returned selector strings flow into downstream querySelector consumers — same blast radius.

PR body says "all 12 sites" — these three (at minimum) make it not-quite-all. Worth one more sweep with `git grep -nE 'querySelector(All)?(.*\$\{' packages/' filtered by un-escaped.

4. Zero new test coverage for the security fix

PR body says "Bundler tests pass (36/36)" — but no new tests added for:

• queryByAttr helper itself (empty value, ===-collision semantics, multiple attribute matches).
• Special-char round-trip: ", ', \\, ], newline, NULL, leading-digit (CSS.escape quirk), the canonical "][data-evil] injection payload.
• A regression test that a comp-id with " in it round-trips bundle → preview cleanly (covers both the helper and the cssAttributeSelector regression in #2).

For a security PR superseding #1568, a 10-line vitest pinning the behavior is cheap insurance against the next refactor silently reverting it.

Nits

• packages/core/src/utils/cssSelector.ts:1 — the // ponytail: comment prefix is unfamiliar; drop or expand if it's a project convention I'm missing.

Questions

• Is there a known migration path / exploit-state for the deployed cohort? The PR says values like " produce "malformed selector that throws Attribute selector didn't terminate" — has that been seen in prod logs, or is this purely defense-in-depth?

— Rames D Jusso

[vanceingalls]

Reviewed at 2223b08 — concur with @james-russo-rames-d-jusso on every blocker and concern. The shape (queryByAttr helper + CSS.escape at browser sites) is the right long-term move, but as it stands the diff has a self-introduced bundler regression and an incomplete sweep, plus the CI red was a real bug not a flake.

Concurring blockers (already in Rames's review — verified independently):

• CI red — CSS is not defined in jsdom (domEditingElement.ts:245, timelineElementHelpers.ts:286-287). The newly-added head commit appears to attempt a fix via packages/studio/src/test-setup.ts + vite.config.ts:198 setupFiles, and required checks are now green at 2223b08. But the test-setup polyfill is a band-aid (pattern #1 — boundary mismatch): tests get a regex-based CSS.escape, production gets the spec-compliant browser one, and they're allowed to drift on edge cases (Unicode, control chars, leading digit). Stronger fix: route both Studio sites through queryByAttr (Node/SSR-safe by construction, which was the helper's whole point) — then the polyfill goes away entirely. Worth at least leaving a soft-ask comment on domEditingElement.ts:245 to migrate.

• Self-introduced regression in cssAttributeSelector (htmlBundler.ts:280-282). PR removed the prior \\ + \" escaping from the helper, but its two live callers — :770 (buildScopeSelector → inlineSubCompositions → scopeCssToComposition as scopeSelectorOverride) and :833-834 (runtimeScope → scopeCssToComposition directly) — feed into compositionScoping.ts:175-177, which only applies escapeCssAttributeValue on the fallback branch, not when an override is passed. So a \" in compId / runtimeCompId now writes malformed scope selectors into bundled CSS, AND into __hfRootSelector (compositionScoping.ts:228-229,307) which is then fed to window.document.querySelector(__hfRootSelector) at runtime — recreating in the bundler output the exact failure mode this PR is closing for querySelector. Pattern #1 (duplicate validation drift at a boundary): escapeCssAttributeValue lives 4 lines away in the same file and IS exported-internally. Cleanest fix: route both callers through escapeCssAttributeValue (or delete cssAttributeSelector entirely — 2 callers).

• Sibling-asymmetry — un-migrated identical sites (pattern #3 scope gap). Verified at 2223b08:

  • packages/studio/src/components/nle/NLELayout.tsx:191-192 — [data-composition-id=\"${compId}\"][data-composition-src] where compId = element.id. Identical vector to the fixed timelineDOM.ts:126.
  • packages/studio/src/player/lib/timelineElementHelpers.ts:165-168 — getTimelineElementSelector returns [data-composition-id=\"${compId}\"] un-escaped, AND #${el.id} (:166), AND .${firstClass} (:172). The returned selector string is consumed downstream by timelineEditingHelpers.ts:55 doc.querySelectorAll(element.selector) and elsewhere — full transitive injection surface.

  PR body claims "all 12 sites" — at least these three are still raw at HEAD.

Concurring soft asks:

• Zero new test coverage for the security fix. No vitest for queryByAttr (===-comparison semantics, empty value, attribute-presence-but-no-match, multiple matches → first-in-document-order contract), no regression test pinning the bundler \"-in-comp-id round-trip (which would also catch the cssAttributeSelector regression in #2 above), no test pinning CSS.escape behavior at the studio sites. For a security PR superseding #1568, ~10 lines of vitest is cheap insurance against the next refactor silently reverting it. (Rames already flagged.)

• // ponytail: prefix on cssSelector.ts:1 — unfamiliar comment convention; either drop or expand if it's intentional.

Net-new from me (small):

• cssSelector.ts — queryByAttr is Element | null. Several patched sites had querySelectorAll-style downstream use (e.g., inlineSubCompositions.ts:228 fell through to :229 querySelector(\"[data-composition-id]\") when no compId — same semantics retained, fine). But the helper has no queryAllByAttr sibling for the cases where multiple matches matter — file that under "future work, not blocking," but worth a note in the helper's jsdoc so the next caller doesn't reach for Array.from(root.querySelectorAll(...)) and re-introduce the injection sneakily.

• The test-setup.ts polyfill regex /([^\\w-])/g over-escapes safe characters (., :, [) inside quoted attribute values — fine for selector strings broadly but the studio tests assert specific selector shapes; if any test diff-compares the produced selector text, the regex-polyfill output won't match what real CSS.escape produces. Belt-and-suspenders: assert the polyfill is only loaded under vitest, not browser dev.

Verified clean:

• picker.ts:100-102 — all three branches consistently escaped (compositionId, compositionSrc, track).
• parsers/htmlParser.ts:520 — getElementById fallback preserved; new queryByAttr path is correct.
• core/src/index.ts:154 — queryByAttr is exported from the public surface.

Verdict: request-changes-level (blockers #1, #2, #3 from Rames stand). Direction good, execution needs the bundler escape restored / re-routed through escapeCssAttributeValue, the three missed call sites swept, and either the Studio polyfill replaced with queryByAttr or at minimum justified in a code comment as a temp shim.

Review by Via