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()