From 5a63095d94bf767a28d974dc4c2b258b0535857b Mon Sep 17 00:00:00 2001 From: Alexander Brandon Coles Date: Mon, 10 Aug 2026 20:41:29 +0200 Subject: [PATCH] fix: anchor selectors with :scope per alternative MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README promised lookups "scoped to the controller's own element", but querySelector matches selectors document-wide and only filters results to descendants: ".menu li" bound via a .menu ancestor outside the controller element. Prefix :scope onto every top-level comma-separated alternative, split by a depth/quote/escape/comment-aware scanner — a single prefix would be bypassed by the comma (":scope .a, .b" leaves .b unanchored), and a :scope :is() wrap was rejected because combinator left-hand sides inside :is() still match ancestors outside the root. CSS comments are stripped while scanning: their contents would otherwise corrupt the quote tracking (".none/*\"*/, .x .item" hid the comma). Only a leading :scope proves an alternative is rooted and passes through; a non-leading :scope (":not(:scope) .item", ".outer :scope .item") does not anchor and is prefixed too — at worst unmatchable, failing closed instead of leaking. Relative selectors like "> li" now work. The guarantee is pinned twice: unit tests on the pure anchorToScope rewrite, and scopedQuery tests that exercise the real leak — the suite runs in actual Chromium (vitest browser mode), where unanchored combinators genuinely match through outside ancestors. Also record the selector trust model (overrides are as trusted as any Stimulus data-* attribute; CSS.escape is user guidance, not a library concern) in README and CONTEXT.md. --- .changeset/proud-scopes-anchor.md | 5 ++ CONTEXT.md | 28 +++++++- README.md | 17 +++++ src/query.ts | 69 ++++++++++++++++++- test/query.test.ts | 108 ++++++++++++++++++++++++++++-- 5 files changed, 218 insertions(+), 9 deletions(-) create mode 100644 .changeset/proud-scopes-anchor.md diff --git a/.changeset/proud-scopes-anchor.md b/.changeset/proud-scopes-anchor.md new file mode 100644 index 0000000..a18b209 --- /dev/null +++ b/.changeset/proud-scopes-anchor.md @@ -0,0 +1,5 @@ +--- +"@openproject/stimulus-elements": minor +--- + +Anchor element lookups with `:scope` so "scoped to the controller element" is real: previously `root.querySelector(".menu li")` could match via a `.menu` ancestor *outside* the controller element. Every comma-separated alternative is now prefixed with `:scope` (commas inside quotes, parentheses, brackets, escapes, or CSS comments are respected, and comments are stripped), so the anchor cannot be bypassed with a selector list. Only alternatives *starting* with `:scope` pass through untouched — a non-leading `:scope` does not anchor and gets the prefix too. Relative selectors like `> li` now work. Results were always confined to descendants of the controller element; what changes is which of them a combinator selector can match. diff --git a/CONTEXT.md b/CONTEXT.md index 3b4ed02..ac3f076 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -28,7 +28,8 @@ accessor triples via element definitions. **Scoped query** The single DOM-lookup module (`src/query.ts`): `scopedQuery(root, selector)` returns `{ first, all, exists }`. Owns the falsy-root guard, invalid-selector -handling, and the warn-once policy; callers never see those concerns. +handling, :scope anchoring, and the warn-once policy; callers never see those +concerns. ## Recorded decisions @@ -49,6 +50,31 @@ handling, and the warn-once policy; callers never see those concerns. is a `WeakMap` keyed by the query root, not a process-global set. Lifetime is an implementation detail: warnings die with the element, tests need no reset hook, and each controller element reports a bad selector once. +- **Selectors are :scope-anchored per alternative.** `querySelector(sel)` + matches selectors document-wide and only filters *results* to descendants, + so `.menu li` can bind via a `.menu` ancestor outside the controller + element. `anchorToScope` prefixes `:scope ` onto every *top-level* + comma-separated alternative (split by a depth/quote/escape/comment-aware + scanner — commas only nest inside quotes, parens, brackets, escapes, or + `/* … */` comments in valid selector syntax; comments are stripped since + their contents would otherwise corrupt the quote tracking). Only a + *leading* `:scope` proves an alternative is rooted and passes through; + a non-leading `:scope` (`:not(:scope) .item`, `.outer :scope .item`) does + not anchor and gets the prefix too — at worst that makes the alternative + unmatchable, which fails closed instead of leaking. A `:scope :is(...)` + wrap was rejected: it anchors only the subject, combinator left-hand sides + inside `:is()` still match ancestors outside the root. Note: happy-dom + already restricts + combinator matching to the subtree (non-spec), so the leak only reproduces + in real browsers — the guarantee is pinned by unit tests on the pure + rewrite, verified manually in Chrome. +- **Selector trust model.** Override attributes are as trusted as any + Stimulus `data-*` attribute — attribute injection already grants + `data-controller`/`data-action`, which is strictly stronger. Selector + evaluation is read-only, fails closed, and returns only descendants of the + controller element. `CSS.escape` has no application point inside this + library (nothing is interpolated into a selector template); it is user + guidance for dynamically built selector values. - **Type/runtime lockstep.** `Camelize` mirrors the runtime `camelize` regex exactly (ASCII-only, tail-recursive). Twin sample tables live in `test/element-definition.test.ts` and `test/types.test-d.ts` — keep in sync. diff --git a/README.md b/README.md index aa8e732..d5351a4 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,12 @@ For each entry `foo: ""` (key camelized) you get: Lookups are **scoped to the controller's own element**, read **live** on every access, and never throw — an invalid selector warns once and yields `null` / `[]`. +Scoping is anchored: every selector (and every comma-separated alternative in +it) is evaluated as if prefixed with `:scope`, so combinators cannot reach +through ancestors outside the controller element — `.menu li` only matches +when `.menu` itself is inside the controller. Selectors starting with +`:scope` are left untouched, and relative selectors like `> li` work as-is. + ## Overriding selectors from the DOM Any declared element's selector can be overridden per instance from the controller @@ -80,6 +86,17 @@ to the static selector. Overrides are read live, like all lookups. Keep element names to simple camelCase words — an embedded acronym like `htmlURL` dasherizes to `html-u-r-l`, which is hard to predict in the attribute. +### Security + +Selector evaluation is read-only and fails closed, and results are always +descendants of the controller element. Override attributes carry the same +trust level as any Stimulus `data-*` attribute: markup that can inject +`data-*-element` attributes can already inject `data-controller` and +`data-action`, which is strictly more powerful. If you sanitize user-supplied +HTML, strip or allowlist `data-*` attributes. If you build selector values +from user input yourself, escape the dynamic parts with +[`CSS.escape()`](https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static). + ## TypeScript ```ts diff --git a/src/query.ts b/src/query.ts index 966a2f2..6dff032 100644 --- a/src/query.ts +++ b/src/query.ts @@ -14,6 +14,70 @@ function warnOnce(root: Element, selector: string, reason: unknown): void { ) } +// Splits a selector list on top-level commas only, stripping CSS comments +// as it scans. Commas nested inside parentheses, brackets, quotes, or after +// a backslash escape are not separators in valid selector syntax, and +// comment contents must not affect any of that state (a quote inside +// /* … */ would otherwise jam the quote tracking). Comments are removed +// without inserting whitespace, matching CSS tokenizer semantics. Invalid +// input may split oddly, but the rewritten selector then throws in +// querySelector and fails closed. +function splitSelectorList(selector: string): string[] { + const parts: string[] = [] + let depth = 0 + let quote: string | null = null + let current = "" + for (let i = 0; i < selector.length; i++) { + const char = selector[i] + if (char === "\\") { + current += selector.slice(i, i + 2) + i++ + continue + } + if (!quote && char === "/" && selector[i + 1] === "*") { + const end = selector.indexOf("*/", i + 2) + if (end === -1) break // unterminated comment consumes the rest + i = end + 1 + continue + } + if (quote) { + if (char === quote) quote = null + } else if (char === '"' || char === "'") { + quote = char + } else if (char === "(" || char === "[") { + depth++ + } else if (char === ")" || char === "]") { + if (depth > 0) depth-- + } else if (char === "," && depth === 0) { + parts.push(current) + current = "" + continue + } + current += char + } + parts.push(current) + return parts +} + +// Anchors every top-level alternative to the query root, so combinators +// cannot match through ancestors outside it (".menu li" must find ".menu" +// inside the root). Only a LEADING :scope proves the alternative is rooted — +// that is the author's explicit anchoring and passes through untouched. +// Everything else gets the prefix, including alternatives mentioning :scope +// elsewhere (":not(:scope) .item", ".outer :scope .item"): a non-leading +// :scope does not anchor, and prefixing at worst makes the alternative +// unmatchable, which fails closed instead of leaking. +const LEADING_SCOPE = /^:scope(?![\w-])/i + +export function anchorToScope(selector: string): string { + return splitSelectorList(selector) + .map((part) => { + const trimmed = part.trim() + return LEADING_SCOPE.test(trimmed) ? trimmed : `:scope ${trimmed}` + }) + .join(", ") +} + export interface ScopedQuery { first(): Element | null all(): Element[] @@ -35,9 +99,10 @@ export function scopedQuery( warnOnce(root, selector, "selector is empty") return EMPTY_QUERY } + const anchored = anchorToScope(selector) const first = (): Element | null => { try { - return root.querySelector(selector) + return root.querySelector(anchored) } catch (error) { warnOnce(root, selector, error) return null @@ -47,7 +112,7 @@ export function scopedQuery( first, all() { try { - return Array.from(root.querySelectorAll(selector)) + return Array.from(root.querySelectorAll(anchored)) } catch (error) { warnOnce(root, selector, error) return [] diff --git a/test/query.test.ts b/test/query.test.ts index a8d8a16..f29b16a 100644 --- a/test/query.test.ts +++ b/test/query.test.ts @@ -1,13 +1,17 @@ import { test, expect, beforeEach, vi } from "vitest" -import { scopedQuery } from "../src/query" +import { scopedQuery, anchorToScope } from "../src/query" beforeEach(() => { document.body.innerHTML = ` -
- 1 - 2 - -
+
+
+ 1 + 2 + + +
d
+
+
outside ` }) @@ -76,6 +80,98 @@ test("empty / whitespace selector warns once per root and returns null / []", () warn.mockRestore() }) +// The :scope anchoring guarantee: unit tests pin the pure selector rewrite, +// and (since the suite runs in a real browser) the scopedQuery tests below +// exercise the actual leak — ".wrap .item" matching via an ancestor OUTSIDE +// the root. See harness.test.ts for the environment guard. +test("anchorToScope prefixes a bare selector", () => { + expect(anchorToScope(".wrap .item")).toBe(":scope .wrap .item") +}) + +test("anchorToScope prefixes every comma-separated alternative", () => { + expect(anchorToScope(".a, .b")).toBe(":scope .a, :scope .b") +}) + +test("anchorToScope does not split on commas inside parentheses", () => { + expect(anchorToScope(":is(.a, .b) > li")).toBe(":scope :is(.a, .b) > li") +}) + +test("anchorToScope does not split on commas inside quoted attribute values", () => { + expect(anchorToScope('[data-x="a,b"]')).toBe(':scope [data-x="a,b"]') + expect(anchorToScope("[data-x='a,b']")).toBe(":scope [data-x='a,b']") +}) + +test("anchorToScope does not split on escaped commas", () => { + expect(anchorToScope(".a\\,b")).toBe(":scope .a\\,b") +}) + +test("anchorToScope leaves alternatives with a LEADING :scope untouched", () => { + expect(anchorToScope(":scope > .item")).toBe(":scope > .item") + expect(anchorToScope(":SCOPE .a, .b")).toBe(":SCOPE .a, :scope .b") + expect(anchorToScope(":scope.foo .item")).toBe(":scope.foo .item") +}) + +test("anchorToScope prefixes alternatives where :scope is not the leading anchor", () => { + // a non-leading :scope must not disable anchoring — these could otherwise + // match through ancestors outside the root + expect(anchorToScope(":not(:scope) .item")).toBe(":scope :not(:scope) .item") + expect(anchorToScope(".outer :scope .item")).toBe(":scope .outer :scope .item") + expect(anchorToScope('[data-x=":scope"] .item')).toBe(':scope [data-x=":scope"] .item') + // ":scope" glued to an identifier tail is not the :scope pseudo-class + expect(anchorToScope(":scoped .item")).toBe(":scope :scoped .item") +}) + +test("anchorToScope strips CSS comments so they cannot confuse the scanner", () => { + // a quote inside a comment must not jam the quote state and hide the comma + expect(anchorToScope('.none/*"*/, .outer .item')).toBe(":scope .none, :scope .outer .item") + // comments are removed without inserting whitespace (CSS tokenizer semantics) + expect(anchorToScope(".a/*x*/.b")).toBe(":scope .a.b") + // unterminated comment consumes the rest of the selector + expect(anchorToScope(".a/*, .outer .item")).toBe(":scope .a") +}) + +test("anchorToScope makes relative selectors explicit", () => { + expect(anchorToScope("> .item")).toBe(":scope > .item") +}) + +test("combinators cannot match through ancestors outside the root", () => { + expect(scopedQuery(root(), ".wrap .item").first()).toBeNull() + expect(scopedQuery(root(), ".wrap .item").all()).toEqual([]) +}) + +test("combinators still work when the full path is inside the root", () => { + const deep = scopedQuery(root(), ".inner .deep-item").first() + expect(deep!.textContent).toBe("d") +}) + +test("every comma-separated alternative is anchored to the root", () => { + // second alternative must not escape the anchor via the comma + expect(scopedQuery(root(), ".none, .wrap .item").first()).toBeNull() + // but comma alternatives that are inside the root still match + expect(scopedQuery(root(), ".none, .item").all().length).toBe(2) +}) + +test("relative selectors match direct children of the root", () => { + const items = scopedQuery(root(), "> .item").all() + expect(items.map((el) => el.textContent)).toEqual(["1", "2"]) + expect(scopedQuery(root(), "> .deep-item").first()).toBeNull() +}) + +test("commas inside :is() are not treated as list separators", () => { + expect(scopedQuery(root(), ":is(.item, .none)").all().length).toBe(2) +}) + +test("commas inside quoted attribute values are not treated as list separators", () => { + expect(scopedQuery(root(), '[data-x="a,b"]').first()).toBe( + document.getElementById("attr-comma"), + ) +}) + +test("an explicit :scope in the selector is left untouched", () => { + const items = scopedQuery(root(), ":scope > .item").all() + expect(items.length).toBe(2) +}) + test("a fresh root gets its own warning — registry is per element, no reset needed", () => { const warn = vi.spyOn(console, "warn").mockImplementation(() => {}) scopedQuery(root(), "###").first()