fix(security): stop tab ids reaching Alpine as expression source (#32)

CHANGELOG.md

@@ -2,6 +2,41 @@
 
 ## [Unreleased]
 
+### Security — tab ids no longer reach Alpine as expression source (#32)
+
+- **A request-controlled `tab.id` executed as JavaScript on page load.** Each
+  tabs template spliced it into four attributes Alpine evaluates as source
+  (`:class`, `:aria-selected`, `:tabindex`, `@click.prevent`), so an id
+  containing an apostrophe closed its string literal, ran, and reopened it —
+  no user interaction, and the surrounding expression still parsed, so Alpine
+  logged nothing. Sixteen sites across bulma and daisy, plus eight more each in
+  bootstrap and foundation, which carried the same fix in on their own branches
+  (#29, #30) rather than merging with a known injection. Fomantic follows the
+  same way. Fixing it in two themes now instead of five later is the whole
+  reason this went ahead of the expansion epic.
+
+  HTML escaping does not mitigate it and could not: the parser decodes `'`
+  back to `'` while building the DOM, and Alpine reads the decoded attribute.
+  The value has to stop being source. Every binding now reads
+  `$el.dataset.cfTab` — `data-cf-tab` was already on each tab for the roving
+  tabindex, so the fix adds plumbing only for the wrapper element some themes
+  put the active class on.
+
+  This closes the *execution* path, not every use of a hostile id. Attribute
+  escaping is a separate guarantee, and `install_cf_ui` still leaves JinjaX's
+  `autoescape` off, so a double quote in `tab.id` can break out of
+  `data-cf-tab` itself under FastAPI/Litestar. Django/cotton is unaffected.
+  Tracked as #36, and called out in `docs/accessibility.md` with a workaround
+  in the meantime.
+
+  `cf_ui_alpine.js` already stated this rule in `initTabs()` and already
+  followed it for `data-cf-active`; it simply was not carried one level down.
+  `tests/unit/test_alpine_expression_safety.py` now enforces it over every
+  template in the package, so a sixth theme cannot reintroduce it by copying a
+  fifth, and `tests/e2e/test_alpine_injection.py` proves the payload is inert in
+  a real browser — asserting both that it did not run and that the bindings did
+  evaluate, since a fix that made Alpine throw would satisfy the first alone.
+
 ### Added — Foundation 6 theme (#23)
 
 All 14 components in both template sets, replacing the `PLANNED.md` stub at
@@ -79,13 +114,6 @@ All 14 components in both template sets, replacing the `PLANNED.md` stub at
   alone — which is the useful part, because it means the JS question gets
   re-asked for free at the moment it is cheapest to answer.
 
-### Fixed — tab ids no longer reach Alpine as expression source (#32)
-
-- The four Alpine bindings on each tab now read `$el.dataset.cfTab` instead of
-  an interpolated `'{{ tab.id }}'`. Same fix as the two shipped themes get in
-  #32; applied here so this theme does not land with the bug and need patching
-  twice. See that ticket for why HTML escaping cannot address it.
-
 ### Added — Bootstrap 5 theme (#22)
 
 All 14 components in both template sets, replacing the `PLANNED.md` stubs at

docs/accessibility.md

@@ -144,14 +144,54 @@ separately rather than smuggled in here.
 
 ## Passing state into Alpine
 
-New state crosses into Alpine through `data-` attributes, read in an `x-init`
-hook — `data-cf-active` → `initTabs()`, `data-cf-open` → `initPanel()` — rather
-than through an interpolated `x-data="cfTabs('{{ active }}')"`.
+**No template interpolation inside an attribute Alpine evaluates.** Not in
+`x-data`, not in `x-init`, not in a `:binding`, not in an `@handler`. That is
+the whole rule, and `tests/unit/test_alpine_expression_safety.py` enforces it
+over every template in the package.
 
-The value is request-controlled. A template engine escapes an *attribute*
-correctly; it has no idea it is writing JavaScript source, so a single
-apostrophe in `active` breaks out of the expression. The data-attribute route
-has no such seam.
+Initial state crosses in through `data-` attributes read in an `x-init` hook —
+`data-cf-active` → `initTabs()`, `data-cf-open` → `initPanel()` — rather than
+through an interpolated `x-data="cfTabs('{{ active }}')"`.
+
+Per-item values do the same thing, one level down. Each tab carries
+`data-cf-tab="{{ tab.id }}"`, and every binding on it reads that back:
+
+```html
+:aria-selected="active === $el.dataset.cfTab"
+:tabindex="tabIndexFor($el.dataset.cfTab)"
+@click.prevent="setActive($el.dataset.cfTab)"
+```
+
+not:
+
+```html
+:tabindex="tabIndexFor('{{ tab.id }}')"   <!-- #32 -->
+```
+
+**Escaping cannot fix an interpolated expression.** The template engine escapes
+an *attribute* correctly, but it has no idea it is writing JavaScript source,
+and the escaping is gone before Alpine ever sees the value: the HTML parser
+decodes `&#x27;` back to `'` while building the DOM, and Alpine reads the
+decoded attribute. A quote-bearing `tab.id` therefore closes its string literal
+and runs, on page load, with no user interaction — which is exactly what #32
+was. A `data-` attribute has no such seam: its value is never parsed as source,
+so the worst a hostile value can do there is be a wrong string.
+
+> **Attribute escaping is a separate guarantee, and one cf-ui does not yet make
+> on the JinjaX path.** `jinjax.Catalog()` builds its environment with
+> `autoescape` off and adopts it only from a caller-supplied `jinja_env`;
+> `install_cf_ui` does not change that. So under FastAPI/Litestar a `tab.id`
+> containing a double quote can still break out of `data-cf-tab` itself and add
+> attributes of its own. Django/cotton is unaffected — Django autoescapes by
+> default. Until #36 lands, pass a `jinja_env` with `autoescape` enabled to
+> `Catalog(...)`, or escape ids before they reach a component.
+
+If a binding needs a value the server knows, the answer is always another
+`data-` attribute on the element carrying the binding — never a wider
+expression. `$el.dataset.*` resolves against the element the directive sits on,
+so an element that reads `$el.dataset.cfTab` has to carry `data-cf-tab` itself;
+reaching into a child or parent instead would put theme-specific DOM structure
+back into `cf_ui_alpine.js`, which is the split this package exists to keep.
 
 ---
 
@@ -162,10 +202,20 @@ tests could not have caught what they claimed to:
 
 * `tests/unit/test_accessibility.py` — claims that *are* markup: a role, an
   `aria-*` value, a server-rendered class. All four template sets, every case.
+* `tests/unit/test_alpine_expression_safety.py` — the rule above, as a guard
+  over the whole template tree rather than a per-theme check, so the next theme
+  cannot reintroduce an interpolated expression by copying the last one.
 * `tests/e2e/` — claims that are behavior: where focus lands after open, where
   it lands after close, that `Tab` cannot leave the dialog. Parameterized over
   `js_on` / `js_off`.
 
+`tests/e2e/test_alpine_injection.py` is in that last group for a reason worth
+naming: the unit tier can assert a rendered attribute holds no splice point, but
+only a browser can show what happens when it does — the decode-then-evaluate
+sequence that makes the bug possible needs a real HTML parser and a real Alpine.
+It asserts the payload did not run **and** that the bindings did evaluate; a fix
+that made Alpine throw would pass the first half while leaving tabs dead.
+
 `expect(role_is_dialog)` proves nothing about focus, and
 `expect(tab).to_be_attached()` passes against markup nobody can use. Assert the
 behavior.

src/cf_ui/static/cf_ui/cf_ui_alpine.js

@@ -163,6 +163,10 @@ document.addEventListener('alpine:init', () => {
             // `x-data="cfTabs('{{ active }}')"`. The value is request-controlled,
             // and a template engine escapes an attribute correctly but has no
             // way to escape JavaScript source text.
+            //
+            // The same rule governs every id these methods receive: the
+            // templates pass `$el.dataset.cfTab`, never `'{{ tab.id }}'`. #32
+            // was that rule being applied here and not one level down.
             this.active = this.$el.dataset.cfActive || null;
         },
 

src/cf_ui/templates/cotton/_themes/bulma/tabs.html

@@ -7,15 +7,16 @@
       {% for tab in tabs %}
       <li role="presentation"
           class="{% if tab.id == active %}is-active{% endif %}"
-          :class="{ 'is-active': active === '{{ tab.id }}' }">
+          data-cf-tab="{{ tab.id }}"
+          :class="{ 'is-active': active === $el.dataset.cfTab }">
         <a role="tab"
            data-cf-tab="{{ tab.id }}"
            aria-controls="{{ hx_target }}"
            aria-selected="{% if tab.id == active %}true{% else %}false{% endif %}"
            tabindex="{% if tab.id == active or not active and forloop.first %}0{% else %}-1{% endif %}"
-           :aria-selected="active === '{{ tab.id }}'"
-           :tabindex="tabIndexFor('{{ tab.id }}')"
-           @click.prevent="setActive('{{ tab.id }}')"
+           :aria-selected="active === $el.dataset.cfTab"
+           :tabindex="tabIndexFor($el.dataset.cfTab)"
+           @click.prevent="setActive($el.dataset.cfTab)"
            hx-get="{{ tab.url }}"
            hx-target="#{{ hx_target }}">{{ tab.id }}</a>
       </li>

src/cf_ui/templates/cotton/_themes/daisy/tabs.html

@@ -10,10 +10,10 @@
        aria-controls="{{ hx_target }}"
        aria-selected="{% if tab.id == active %}true{% else %}false{% endif %}"
        tabindex="{% if tab.id == active or not active and forloop.first %}0{% else %}-1{% endif %}"
-       :class="{ 'tab-active': active === '{{ tab.id }}' }"
-       :aria-selected="active === '{{ tab.id }}'"
-       :tabindex="tabIndexFor('{{ tab.id }}')"
-       @click.prevent="setActive('{{ tab.id }}')"
+       :class="{ 'tab-active': active === $el.dataset.cfTab }"
+       :aria-selected="active === $el.dataset.cfTab"
+       :tabindex="tabIndexFor($el.dataset.cfTab)"
+       @click.prevent="setActive($el.dataset.cfTab)"
        hx-get="{{ tab.url }}"
        hx-target="#{{ hx_target }}">{{ tab.id }}</a>
     {% endfor %}

src/cf_ui/templates/jinja/bulma/Tabs.jinja

@@ -12,15 +12,16 @@
       {% for tab in tabs %}
       <li role="presentation"
           class="{% if tab.id == active %}is-active{% endif %}"
-          :class="{ 'is-active': active === '{{ tab.id }}' }">
+          data-cf-tab="{{ tab.id }}"
+          :class="{ 'is-active': active === $el.dataset.cfTab }">
         <a role="tab"
            data-cf-tab="{{ tab.id }}"
            aria-controls="{{ hx_target }}"
            aria-selected="{% if tab.id == active %}true{% else %}false{% endif %}"
            tabindex="{% if tab.id == active or (not active and loop.first) %}0{% else %}-1{% endif %}"
-           :aria-selected="active === '{{ tab.id }}'"
-           :tabindex="tabIndexFor('{{ tab.id }}')"
-           @click.prevent="setActive('{{ tab.id }}')"
+           :aria-selected="active === $el.dataset.cfTab"
+           :tabindex="tabIndexFor($el.dataset.cfTab)"
+           @click.prevent="setActive($el.dataset.cfTab)"
            hx-get="{{ tab.url }}"
            hx-target="#{{ hx_target }}">{{ tab.id }}</a>
       </li>

src/cf_ui/templates/jinja/daisy/Tabs.jinja

@@ -16,10 +16,10 @@
        aria-controls="{{ hx_target }}"
        aria-selected="{% if tab.id == active %}true{% else %}false{% endif %}"
        tabindex="{% if tab.id == active or (not active and loop.first) %}0{% else %}-1{% endif %}"
-       :class="{ 'tab-active': active === '{{ tab.id }}' }"
-       :aria-selected="active === '{{ tab.id }}'"
-       :tabindex="tabIndexFor('{{ tab.id }}')"
-       @click.prevent="setActive('{{ tab.id }}')"
+       :class="{ 'tab-active': active === $el.dataset.cfTab }"
+       :aria-selected="active === $el.dataset.cfTab"
+       :tabindex="tabIndexFor($el.dataset.cfTab)"
+       @click.prevent="setActive($el.dataset.cfTab)"
        hx-get="{{ tab.url }}"
        hx-target="#{{ hx_target }}">{{ tab.id }}</a>
     {% endfor %}

tests/e2e/test_alpine_injection.py

@@ -0,0 +1,104 @@
+"""The hostile tab id does not execute in a real browser (#32).
+
+This is the tier that actually proves the fix. The unit tier can only assert
+that the rendered attribute holds no splice point; it cannot show what the
+browser does with one, because the whole mechanism depends on two things
+pytest does not have — an HTML parser that decodes entity escapes while
+building the DOM, and Alpine's expression evaluator reading the decoded value
+back out.
+
+The page is assembled here rather than served by the demo app on purpose: the
+demo app should not grow a route whose job is to render a payload. The
+templates, `cf_ui_alpine.js`, and the pinned Alpine build are the real ones.
+
+Two assertions, and both are load-bearing:
+
+* the payload did not run, and
+* the bindings *did* evaluate — a fix that made Alpine throw on the expression
+  would satisfy the first assertion while leaving the widget dead.
+"""
+
+from pathlib import Path
+
+import pytest
+from jinja2 import Environment, FileSystemLoader, StrictUndefined, select_autoescape
+
+from cf_ui import themes as cf_ui_themes
+from cf_ui.templatetags.cf_ui import _ALPINE_CDN, _DEFAULTS
+
+PACKAGE_DIR = Path(__file__).parent.parent.parent / "src" / "cf_ui"
+JINJA_DIR = PACKAGE_DIR / "templates" / "jinja"
+ALPINE_LOCAL = PACKAGE_DIR / "static" / "cf_ui" / "cf_ui_alpine.js"
+
+#: Resolved from the package's own pin rather than repeated here — testing
+#: against a different Alpine than cf-ui ships would be testing the wrong
+#: evaluator, and a hardcoded copy is a pin that drifts silently.
+ALPINE_CDN = _ALPINE_CDN.format(v=_DEFAULTS["alpinejs"])
+
+#: From the registry, so a new theme is covered the moment it is accepted.
+THEMES = list(cf_ui_themes.THEMES)
+
+#: Closes the string literal, runs, and reopens it, so the surrounding
+#: expression still parses and Alpine reports no error. See the same constant
+#: in tests/unit/test_alpine_expression_safety.py.
+HOSTILE_ID = "');window.cfPwned=true;('"
+
+PAGE = """<!doctype html>
+<html>
+<head>
+<script>{alpine_local}</script>
+<script src="{alpine_cdn}" defer></script>
+</head>
+<body>
+{component}
+</body>
+</html>
+"""
+
+
+def _build_page(theme: str, tmp_path: Path) -> Path:
+    env = Environment(
+        loader=FileSystemLoader(JINJA_DIR / theme),
+        autoescape=select_autoescape(["html", "jinja"]),
+        undefined=StrictUndefined,
+    )
+    component = env.get_template("Tabs.jinja").render(
+        tabs=[{"id": HOSTILE_ID, "url": "/x/"}, {"id": "safe", "url": "/safe/"}],
+        hx_target="tc",
+        active=HOSTILE_ID,
+        content="",
+        extra_class="",
+    )
+    assert "cfPwned" in component, "the hostile id never rendered — nothing to test"
+
+    page = tmp_path / f"injection_{theme}.html"
+    page.write_text(
+        PAGE.format(
+            alpine_local=ALPINE_LOCAL.read_text(encoding="utf-8"),
+            alpine_cdn=ALPINE_CDN,
+            component=component,
+        ),
+        encoding="utf-8",
+    )
+    return page
+
+
+@pytest.mark.parametrize("theme", THEMES)
+def test_a_hostile_tab_id_does_not_execute(page, tmp_path, theme):
+    page.goto(_build_page(theme, tmp_path).as_uri())
+    page.wait_for_function("() => window.Alpine !== undefined", timeout=15000)
+
+    # The bindings have to have been evaluated before "it did not run" means
+    # anything. aria-selected is computed by Alpine from the tab id it read out
+    # of the data attribute, so a true here is proof the expression ran and
+    # resolved the hostile id as a plain string.
+    page.wait_for_function(
+        "() => document.querySelector('[role=tab]').getAttribute('aria-selected') === 'true'",
+        timeout=5000,
+    )
+    first_tab = page.locator('[role="tab"]').first
+    assert first_tab.get_attribute("data-cf-tab") == HOSTILE_ID
+
+    assert page.evaluate("() => window.cfPwned === true") is False, (
+        "the tab id executed as JavaScript — it reached an Alpine expression as source"
+    )

tests/unit/cotton/test_daisy.py

@@ -180,5 +180,7 @@ def test_breadcrumb_uses_daisy_breadcrumbs_class(daisy_render):
 def test_tabs_keeps_the_alpine_contract(daisy_render):
     html = daisy_render("cf/tabs.html", tabs=[{"id": "one", "url": "/one"}], slot="C")
     assert 'x-data="cfTabs"' in html
-    assert "setActive('one')" in html
+    # The id reaches Alpine as data, never as expression text (#32).
+    assert 'data-cf-tab="one"' in html
+    assert "setActive($el.dataset.cfTab)" in html
     assert "tab-active" in html

tests/unit/jinja/test_daisy.py

@@ -250,5 +250,7 @@ def test_breadcrumb_uses_daisy_breadcrumbs_class(render):
 def test_tabs_keeps_the_alpine_contract(render):
     html = render("Tabs.jinja", tabs=[{"id": "one", "url": "/one"}], content="C")
     assert 'x-data="cfTabs"' in html
-    assert "setActive('one')" in html
+    # The id reaches Alpine as data, never as expression text (#32).
+    assert 'data-cf-tab="one"' in html
+    assert "setActive($el.dataset.cfTab)" in html
     assert "tab-active" in html