From f9f54b163dee9c522ade3153949123aece90068e Mon Sep 17 00:00:00 2001 From: Francis Secada Date: Wed, 29 Jul 2026 22:28:24 -0400 Subject: [PATCH 1/2] fix(security): stop tab ids reaching Alpine as expression source (#32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request-controlled `tab.id` executed as JavaScript on page load. Every 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. The surrounding expression still parsed, so Alpine logged nothing and no interaction was required. HTML escaping does not mitigate this and cannot: the parser decodes `'` back to `'` while building the DOM, and Alpine reads the decoded attribute. The escaping is gone before the evaluator sees the value. The value has to stop being source, so every binding now reads `$el.dataset.cfTab`. `data-cf-tab` was already on each tab for the roving tabindex; the only new plumbing is on the row wrapper that carries the active class in Bulma. `cf_ui_alpine.js` already stated this rule in `initTabs()` and already followed it for `data-cf-active` — it was never carried one level down. Sixteen sites across the two shipped themes. The three open theme branches each copied the pattern, which is why this lands before them: two themes to patch now instead of five later, the same reasoning that put #21 ahead of the expansion epic. Tests: - `tests/unit/test_alpine_expression_safety.py` guards the rule over every template in the package, not per theme, so a sixth theme cannot reintroduce it by copying a fifth. Verified non-vacuous both ways: it failed on exactly the four tabs templates before the fix, and dropping `data-cf-tab` from the Bulma wrapper still fails the pairing test. - `tests/e2e/test_alpine_injection.py` proves the payload is inert in a real browser, which is the only tier that can — the decode-then- evaluate sequence needs a real parser and a real Alpine. It asserts both that the payload did not run and that the bindings did evaluate; a fix that made Alpine throw would satisfy the first alone. - The two `test_tabs_keeps_the_alpine_contract` cases asserted the old interpolated form and now assert the data-attribute one. 625 unit + integration pass, 83 E2E pass, 65 node pass, ruff and prek clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NhqNRBg83czKfr8L6FF5xf --- CHANGELOG.md | 26 +++ docs/accessibility.md | 54 ++++- src/cf_ui/static/cf_ui/cf_ui_alpine.js | 4 + .../templates/cotton/_themes/bulma/tabs.html | 9 +- .../templates/cotton/_themes/daisy/tabs.html | 8 +- src/cf_ui/templates/jinja/bulma/Tabs.jinja | 9 +- src/cf_ui/templates/jinja/daisy/Tabs.jinja | 8 +- tests/e2e/test_alpine_injection.py | 100 +++++++++ tests/unit/cotton/test_daisy.py | 4 +- tests/unit/jinja/test_daisy.py | 4 +- tests/unit/test_alpine_expression_safety.py | 202 ++++++++++++++++++ 11 files changed, 403 insertions(+), 25 deletions(-) create mode 100644 tests/e2e/test_alpine_injection.py create mode 100644 tests/unit/test_alpine_expression_safety.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fb5593..228d374 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,32 @@ ## [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 the two shipped themes; the three open + theme branches had each copied the pattern, which is why this landed before + them rather than after. + + 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. + + `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 — real Tailwind build in CI (#17) - **A CI job that builds the vendored plugin through the actual Tailwind CLI.** diff --git a/docs/accessibility.md b/docs/accessibility.md index 1a42d59..4f975e4 100644 --- a/docs/accessibility.md +++ b/docs/accessibility.md @@ -144,14 +144,44 @@ 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 }}')" +``` + +**Escaping is not the fix, and cannot be.** 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 +`'` 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 because its value is never parsed as source. + +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 +192,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. diff --git a/src/cf_ui/static/cf_ui/cf_ui_alpine.js b/src/cf_ui/static/cf_ui/cf_ui_alpine.js index 7fd2053..5847496 100644 --- a/src/cf_ui/static/cf_ui/cf_ui_alpine.js +++ b/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; }, diff --git a/src/cf_ui/templates/cotton/_themes/bulma/tabs.html b/src/cf_ui/templates/cotton/_themes/bulma/tabs.html index dc9f4e0..4e5461b 100644 --- a/src/cf_ui/templates/cotton/_themes/bulma/tabs.html +++ b/src/cf_ui/templates/cotton/_themes/bulma/tabs.html @@ -7,15 +7,16 @@ {% for tab in tabs %} diff --git a/src/cf_ui/templates/cotton/_themes/daisy/tabs.html b/src/cf_ui/templates/cotton/_themes/daisy/tabs.html index 142bcb5..acda8b5 100644 --- a/src/cf_ui/templates/cotton/_themes/daisy/tabs.html +++ b/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 }} {% endfor %} diff --git a/src/cf_ui/templates/jinja/bulma/Tabs.jinja b/src/cf_ui/templates/jinja/bulma/Tabs.jinja index 29eaf88..607087d 100644 --- a/src/cf_ui/templates/jinja/bulma/Tabs.jinja +++ b/src/cf_ui/templates/jinja/bulma/Tabs.jinja @@ -12,15 +12,16 @@ {% for tab in tabs %} diff --git a/src/cf_ui/templates/jinja/daisy/Tabs.jinja b/src/cf_ui/templates/jinja/daisy/Tabs.jinja index a7a7773..9e4c05c 100644 --- a/src/cf_ui/templates/jinja/daisy/Tabs.jinja +++ b/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 }} {% endfor %} diff --git a/tests/e2e/test_alpine_injection.py b/tests/e2e/test_alpine_injection.py new file mode 100644 index 0000000..93e8fb7 --- /dev/null +++ b/tests/e2e/test_alpine_injection.py @@ -0,0 +1,100 @@ +"""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 + +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" + +#: Kept in step with `_DEFAULTS["alpinejs"]` in templatetags/cf_ui.py and the +#: `cf_ui_body` macro — testing against a different Alpine than the package +#: ships would be testing the wrong evaluator. +ALPINE_CDN = "https://cdn.jsdelivr.net/npm/alpinejs@3.14.1/dist/cdn.min.js" + +THEMES = ["bulma", "daisy"] + +#: 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 = """ + + + + + + +{component} + + +""" + + +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" + ) diff --git a/tests/unit/cotton/test_daisy.py b/tests/unit/cotton/test_daisy.py index 5a48001..0034236 100644 --- a/tests/unit/cotton/test_daisy.py +++ b/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 diff --git a/tests/unit/jinja/test_daisy.py b/tests/unit/jinja/test_daisy.py index 970787a..5bca540 100644 --- a/tests/unit/jinja/test_daisy.py +++ b/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 diff --git a/tests/unit/test_alpine_expression_safety.py b/tests/unit/test_alpine_expression_safety.py new file mode 100644 index 0000000..bead6c0 --- /dev/null +++ b/tests/unit/test_alpine_expression_safety.py @@ -0,0 +1,202 @@ +"""Request-controlled values must never reach Alpine as expression text (#32). + +Alpine evaluates the *value* of `:attr`, `@event` and `x-*` attributes as +JavaScript source. Interpolating a template variable into one of those is not +an escaping problem that a template engine can solve: the HTML parser decodes +`'` back to `'` while building the DOM, and Alpine reads the decoded +attribute. By the time the string reaches Alpine's evaluator the escaping is +gone, so a quote-bearing value breaks out of its string literal and runs. + +`cf_ui_alpine.js` already states the rule in `initTabs()` and already follows +it for the seed value — `data-cf-active` crosses the boundary as data, and +`x-data="cfTabs('{{ active }}')"` is deliberately not used. This module is the +enforcement for the same rule one level down, plus a tree-wide guard so a +sixth theme cannot reintroduce the pattern by copying a fifth. + +The proof that the fix works lives in `tests/e2e/test_alpine_injection.py` — +only a real browser can show the payload does not execute, because only a real +browser runs the HTML parser and Alpine's evaluator. What is asserted *here* +is the property that makes that possible: the rendered attribute holds no +splice point at all. +""" + +import re +from collections.abc import Callable +from pathlib import Path + +import pytest +from jinja2 import Environment, FileSystemLoader, StrictUndefined, select_autoescape + +TEMPLATES_DIR = Path(__file__).parent.parent.parent / "src" / "cf_ui" / "templates" +JINJA_DIR = TEMPLATES_DIR / "jinja" +COTTON_DIR = TEMPLATES_DIR / "cotton" + +THEMES = ["bulma", "daisy"] + +#: A tab id that closes the string literal it would be spliced into, runs, and +#: reopens it so the surrounding expression still parses. An expression that +#: merely *fails* to parse is not the interesting case — Alpine logs and moves +#: on. This one succeeds. +HOSTILE_ID = "');window.cfPwned=true;('" + +TABS = [{"id": "one", "url": "/one/"}, {"id": "two", "url": "/two/"}] + + +# ── 1. Tree-wide guard ──────────────────────────────────────────────────── +# +# Every template, every theme, every engine. This is the test that keeps the +# fix from being a one-time cleanup: it fails on the next template that gets +# the pattern wrong, wherever it lands. + +#: Attributes Alpine evaluates. The lookbehind matters — without it, `x-` +#: matches inside `hx-get`, and HTMX attributes are values (a URL, a selector), +#: not source text, so interpolating into them is correct and expected. +ALPINE_ATTR = re.compile( + r"""(? list[Path]: + return sorted([p for p in JINJA_DIR.rglob("*.jinja")] + [p for p in COTTON_DIR.rglob("*.html")]) + + +def _rel(path: Path) -> str: + return str(path.relative_to(TEMPLATES_DIR)).replace("\\", "/") + + +@pytest.mark.parametrize("template", _all_templates(), ids=_rel) +def test_no_alpine_expression_attribute_interpolates_template_output(template: Path): + source = template.read_text(encoding="utf-8") + offenders = [ + match.group(0) + for match in ALPINE_ATTR.finditer(source) + if INTERPOLATION.search(match.group(1)) + ] + assert not offenders, ( + f"{_rel(template)} splices template output into an Alpine expression:\n" + + "\n".join(f" {o}" for o in offenders) + + "\n\nAlpine evaluates these values as JavaScript. Pass the value through a " + "`data-` attribute and read it back with `$el.dataset.*` instead — see " + "docs/accessibility.md." + ) + + +# ── 2. The tab bindings specifically ────────────────────────────────────── + + +@pytest.fixture(params=THEMES) +def theme(request) -> str: + return request.param + + +@pytest.fixture +def jinja_render(theme: str) -> Callable[..., str]: + env = Environment( + loader=FileSystemLoader(JINJA_DIR / theme), + autoescape=select_autoescape(["html", "jinja"]), + undefined=StrictUndefined, + ) + + def _render(template_name: str, **ctx: object) -> str: + return env.get_template(template_name).render(**ctx) + + return _render + + +@pytest.fixture +def cotton_render(settings, theme: str) -> Callable[..., str]: + from django.template.loader import render_to_string + + settings.CF_UI_THEME = theme + + def _render(stem: str, **props: object) -> str: + return render_to_string(f"cotton/cf/{stem}.html", props) + + return _render + + +def _alpine_values(html: str) -> list[str]: + return [match.group(1) for match in ALPINE_ATTR.finditer(html)] + + +def test_jinja_tab_bindings_read_the_tab_id_from_a_data_attribute(jinja_render): + html = jinja_render( + "Tabs.jinja", tabs=TABS, hx_target="tc", active="two", content="", extra_class="" + ) + assert "$el.dataset.cfTab" in html + assert "tabIndexFor('" not in html + assert "setActive('" not in html + + +def test_cotton_tab_bindings_read_the_tab_id_from_a_data_attribute(cotton_render): + html = cotton_render("tabs", tabs=TABS, hx_target="tc", active="two", **{"class": ""}) + assert "$el.dataset.cfTab" in html + assert "tabIndexFor('" not in html + assert "setActive('" not in html + + +def test_jinja_every_element_bound_to_the_tab_id_carries_it_as_data(jinja_render): + """`$el.dataset.cfTab` only resolves on an element that has the attribute. + + The active-class binding sits on the row wrapper in some themes and on the + anchor in others, so both have to carry `data-cf-tab` — reaching across + the DOM from one to the other would put theme structure back into + `cf_ui_alpine.js`. + """ + html = jinja_render( + "Tabs.jinja", tabs=TABS, hx_target="tc", active="two", content="", extra_class="" + ) + for tag in re.findall(r"<[a-zA-Z][^>]*>", html, re.DOTALL): + if "$el.dataset.cfTab" in tag: + assert "data-cf-tab=" in tag, ( + f"reads $el.dataset.cfTab but carries no data-cf-tab:\n {tag}" + ) + + +def test_cotton_every_element_bound_to_the_tab_id_carries_it_as_data(cotton_render): + html = cotton_render("tabs", tabs=TABS, hx_target="tc", active="two", **{"class": ""}) + for tag in re.findall(r"<[a-zA-Z][^>]*>", html, re.DOTALL): + if "$el.dataset.cfTab" in tag: + assert "data-cf-tab=" in tag, ( + f"reads $el.dataset.cfTab but carries no data-cf-tab:\n {tag}" + ) + + +# ── 3. A hostile id, rendered ───────────────────────────────────────────── + + +def test_jinja_tabs_leave_a_hostile_tab_id_out_of_every_expression(jinja_render): + html = jinja_render( + "Tabs.jinja", + tabs=[{"id": HOSTILE_ID, "url": "/x/"}], + hx_target="tc", + active=HOSTILE_ID, + content="", + extra_class="", + ) + assert "cfPwned" not in "".join(_alpine_values(html)), ( + "the payload reached an attribute Alpine evaluates as JavaScript" + ) + assert "cfPwned" in html, "the hostile id never rendered at all — this test proved nothing" + + +def test_cotton_tabs_leave_a_hostile_tab_id_out_of_every_expression(cotton_render): + html = cotton_render( + "tabs", + tabs=[{"id": HOSTILE_ID, "url": "/x/"}], + hx_target="tc", + active=HOSTILE_ID, + **{"class": ""}, + ) + assert "cfPwned" not in "".join(_alpine_values(html)) + assert "cfPwned" in html, "the hostile id never rendered at all" From 64a937a8a99de5a125a2499a6b053e77c98b0249 Mon Sep 17 00:00:00 2001 From: Francis Secada Date: Wed, 29 Jul 2026 22:36:19 -0400 Subject: [PATCH 2/2] fix(security): narrow the escaping claim and harden the guard (#32) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of PR #35 found a second exposure and two soft spots in the new guard. `install_cf_ui` leaves JinjaX's `autoescape` off — `jinjax.Catalog()` builds its environment with autoescape disabled and adopts it only from a caller-supplied `jinja_env`. So on the FastAPI/Litestar path `data-cf-tab="{{ tab.id }}"` is unescaped, and a double-quote-bearing id breaks out of the attribute and installs a live event handler. Reproduced through the real installer. Django/cotton is unaffected. That is not a regression — the old code was exposed the same way, with the expression injection on top — but it means "a `data-` attribute has no such seam" was a stronger claim than the code supports. Filed as #36, because switching autoescape on changes behaviour for existing consumers and is a release-shaped decision, not a line folded into a security patch. `docs/accessibility.md` now states the exposure and the interim workaround, and the CHANGELOG points at #36 rather than implying attribute safety cf-ui does not yet provide. Worth recording why the suite could not see it: every Jinja fixture in `tests/` hand-builds an environment with autoescape enabled, which is friendlier than what `install_cf_ui` produces. The escaping assertions were proving a property of the harness — the proxy-test shape #19 exists to close. Guard hardening, all in code this PR introduced: - `ALPINE_ATTR` matched only double-quoted values, so `:class='{ "is-active": … }'` and bare values slipped through. Single quotes are exactly what an author reaches for when the expression needs a double quote, and this guard is the only thing standing between a sixth theme and reintroducing #32. All three quoting forms now match. - Added `test_the_guard_flags_what_it_claims_to`: thirteen cases pinning what the regex catches and what it correctly ignores (`hx-*`, `data-*`, interpolation-free expressions). A lint that quietly stops matching reports clean forever. - Both new files took `THEMES` from `cf_ui.themes` instead of a literal, so the per-theme cases and the E2E proof pick up bootstrap, foundation and fomantic the moment those land — they are the reason this PR exists, and would otherwise have been silently uncovered. - The E2E Alpine URL comes from `_ALPINE_CDN`/`_DEFAULTS` rather than a fourth hardcoded copy of `3.14.1`. 638 unit + integration pass, ruff and prek clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NhqNRBg83czKfr8L6FF5xf --- CHANGELOG.md | 7 +++ docs/accessibility.md | 24 +++++--- tests/e2e/test_alpine_injection.py | 14 +++-- tests/unit/test_alpine_expression_safety.py | 67 ++++++++++++++++++--- 4 files changed, 92 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 228d374..6ca376e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,13 @@ 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 diff --git a/docs/accessibility.md b/docs/accessibility.md index 4f975e4..f7c76c1 100644 --- a/docs/accessibility.md +++ b/docs/accessibility.md @@ -168,13 +168,23 @@ not: :tabindex="tabIndexFor('{{ tab.id }}')" ``` -**Escaping is not the fix, and cannot be.** 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 -`'` 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 because its value is never parsed as source. +**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 `'` 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 diff --git a/tests/e2e/test_alpine_injection.py b/tests/e2e/test_alpine_injection.py index 93e8fb7..3da463a 100644 --- a/tests/e2e/test_alpine_injection.py +++ b/tests/e2e/test_alpine_injection.py @@ -23,16 +23,20 @@ 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" -#: Kept in step with `_DEFAULTS["alpinejs"]` in templatetags/cf_ui.py and the -#: `cf_ui_body` macro — testing against a different Alpine than the package -#: ships would be testing the wrong evaluator. -ALPINE_CDN = "https://cdn.jsdelivr.net/npm/alpinejs@3.14.1/dist/cdn.min.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"]) -THEMES = ["bulma", "daisy"] +#: 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 diff --git a/tests/unit/test_alpine_expression_safety.py b/tests/unit/test_alpine_expression_safety.py index bead6c0..94e6ee2 100644 --- a/tests/unit/test_alpine_expression_safety.py +++ b/tests/unit/test_alpine_expression_safety.py @@ -27,11 +27,15 @@ import pytest from jinja2 import Environment, FileSystemLoader, StrictUndefined, select_autoescape +from cf_ui import themes as cf_ui_themes + TEMPLATES_DIR = Path(__file__).parent.parent.parent / "src" / "cf_ui" / "templates" JINJA_DIR = TEMPLATES_DIR / "jinja" COTTON_DIR = TEMPLATES_DIR / "cotton" -THEMES = ["bulma", "daisy"] +#: From the registry, not a literal, so the per-theme cases below start +#: covering a new theme the moment `resolve_theme` starts accepting it. +THEMES = list(cf_ui_themes.THEMES) #: A tab id that closes the string literal it would be spliced into, runs, and #: reopens it so the surrounding expression still parses. An expression that @@ -48,16 +52,27 @@ # fix from being a one-time cleanup: it fails on the next template that gets # the pattern wrong, wherever it lands. -#: Attributes Alpine evaluates. The lookbehind matters — without it, `x-` -#: matches inside `hx-get`, and HTMX attributes are values (a URL, a selector), -#: not source text, so interpolating into them is correct and expected. +#: Attributes Alpine evaluates. Two details carry weight: +#: +#: * The lookbehind — without it, `x-` matches inside `hx-get`, and HTMX +#: attributes are values (a URL, a selector), not source text, so +#: interpolating into them is correct and expected. +#: * All three quoting forms. Single-quoted delimiters are not hypothetical: +#: they are what an author reaches for when the expression itself needs a +#: double quote (`:class='{ "is-active": … }'`). This guard is the only thing +#: standing between a new theme and reintroducing #32, so a form it cannot +#: see is a form that gets reported clean forever. ALPINE_ATTR = re.compile( - r"""(?'"]+) # bare + ) """, re.VERBOSE, ) @@ -66,6 +81,42 @@ INTERPOLATION = re.compile(r"\{\{|\{%") +def _value(match: re.Match) -> str: + """The matched attribute's value, whichever quoting form it used.""" + return next(group for group in match.groups() if group is not None) + + +# The guard's own tests. A lint that silently stops matching reports clean +# forever, which is worse than not having it — so what it does and does not +# catch is pinned here rather than left to the regex being read correctly. +GUARD_CASES = [ + (True, ':tabindex="tabIndexFor({{ tab.id }})"'), + (True, ":tabindex='tabIndexFor({{ tab.id }})'"), + (True, ":tabindex={{ tab.id }}"), + (True, 'x-on:click="go({{ id }})"'), + (True, "x-data=\"cfTabs('{{ active }}')\""), + (True, "@click.prevent=\"setActive('{{ tab.id }}')\""), + (True, ':class="{% if x %}a{% endif %}"'), + # HTMX values are a URL and a selector, not source. Interpolating is right. + (False, 'hx-get="{{ tab.url }}"'), + (False, 'hx-target="#{{ hx_target }}"'), + # data-* is the sanctioned route, however hostile the value. + (False, 'data-cf-tab="{{ tab.id }}"'), + (False, 'data-x-thing="{{ v }}"'), + # An Alpine expression with no interpolation at all. + (False, ":class=\"{ 'is-active': active === $el.dataset.cfTab }\""), + (False, '@keydown="onKeydown($event)"'), +] + + +@pytest.mark.parametrize(("flagged", "attribute"), GUARD_CASES, ids=lambda v: str(v)[:48]) +def test_the_guard_flags_what_it_claims_to(flagged: bool, attribute: str): + hits = [m for m in ALPINE_ATTR.finditer(attribute) if INTERPOLATION.search(_value(m))] + assert bool(hits) is flagged, ( + f"guard {'missed' if flagged else 'false-positived on'}: {attribute}" + ) + + def _all_templates() -> list[Path]: return sorted([p for p in JINJA_DIR.rglob("*.jinja")] + [p for p in COTTON_DIR.rglob("*.html")]) @@ -80,7 +131,7 @@ def test_no_alpine_expression_attribute_interpolates_template_output(template: P offenders = [ match.group(0) for match in ALPINE_ATTR.finditer(source) - if INTERPOLATION.search(match.group(1)) + if INTERPOLATION.search(_value(match)) ] assert not offenders, ( f"{_rel(template)} splices template output into an Alpine expression:\n" @@ -126,7 +177,7 @@ def _render(stem: str, **props: object) -> str: def _alpine_values(html: str) -> list[str]: - return [match.group(1) for match in ALPINE_ATTR.finditer(html)] + return [_value(match) for match in ALPINE_ATTR.finditer(html)] def test_jinja_tab_bindings_read_the_tab_id_from_a_data_attribute(jinja_render):