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

[fsecada01]

Closes #32. Part of #34. Unblocks #29, #30, #31 — each of those branches needs to merge master and adopt the fixed pattern before merge.

What was wrong

Every tabs template interpolated the request-controlled tab.id into four attributes Alpine evaluates as JavaScript source:

:class="{ 'is-active': active === '{{ tab.id }}' }"
:aria-selected="active === '{{ tab.id }}'"
:tabindex="tabIndexFor('{{ tab.id }}')"
@click.prevent="setActive('{{ tab.id }}')"

An id containing an apostrophe closes its string literal, runs, and reopens it — so the surrounding expression still parses, Alpine logs nothing, and no user interaction is required. Confirmed with a working payload before the ticket was filed, and the E2E test in this PR reproduces it: with the fix reverted it fails on the "did not execute" assertion, having already passed the "bindings evaluated" one.

HTML escaping is not the fix and cannot be. 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. A template engine escapes an attribute correctly; it has no idea it is writing JavaScript source.

The fix

The value stops being source. Every binding reads it back from a data- attribute:

:aria-selected="active === $el.dataset.cfTab"
:tabindex="tabIndexFor($el.dataset.cfTab)"
@click.prevent="setActive($el.dataset.cfTab)"

data-cf-tab="{{ tab.id }}" was already on every <a role="tab"> — tabIndexFor already read it for the roving tabindex — so the only new plumbing is on the <li> wrapper that carries the active class under Bulma. $el.dataset.* resolves against the element the directive sits on, so reaching from the <li> into its child would have put theme-specific DOM structure back into cf_ui_alpine.js.

cf_ui_alpine.js already stated this exact rule in initTabs() and already followed it for data-cf-active. It was never carried one level down; that is the whole bug.

Scope

Sixteen sites, four files, both shipped themes.

The three open theme PRs each copied the pattern into their own theme, so this lands ahead of them — two themes to patch now instead of five later, which is the same reasoning that put #21 ahead of the theme-expansion epic. A sweep of every :attr / @event / x-* attribute across all five themes found no other interpolation into an Alpine expression. hx-get / hx-target interpolation is untouched: HTMX treats those as a URL and a selector, not as source.

Tests

tests/unit/test_alpine_expression_safety.py — the rule as a guard over every template in the package rather than a per-theme check, so a sixth theme cannot reintroduce it by copying a fifth. Verified non-vacuous in both directions: before the fix it failed on exactly the four tabs templates and passed on the other 66; and dropping data-cf-tab from the Bulma wrapper still fails the pairing test that says an element reading $el.dataset.cfTab must carry the attribute itself.

tests/e2e/test_alpine_injection.py — the tier that actually proves the fix, because the mechanism depends on two things pytest does not have: a parser that decodes entities while building the DOM, and Alpine's evaluator reading the decoded value back. It asserts the payload did not run and that the bindings did evaluate — a fix that made Alpine throw on the expression would satisfy the first assertion while leaving the widget dead. The page is assembled in the test from the real templates, the real cf_ui_alpine.js, and the pinned Alpine build rather than served by the demo app, which should not grow a route whose job is to render a payload.

Two existing cases (test_tabs_keeps_the_alpine_contract, Jinja and Cotton) asserted the old interpolated form and now assert the data-attribute one.

Gates

Gate	Result
pytest tests/unit tests/integration	625 passed
pytest tests/e2e --browser chromium	83 passed, 13 skipped
node --test tests/js/*.test.mjs	65 passed
ruff check src tests	clean
ruff format --check src tests	clean
prek (run twice)	clean, no hook fight

Behaviour is unchanged and still asserted: roving tabindex still puts exactly one tabindex="0" in the widget including the no-active first-tab case, keyboard nav and manual activation still pass, and the js_off assertions still pass.

Follow-ups, not in this diff

• just test-js silently runs zero tests and exits 0 on Windows — the quoted glob reaches node literally under cmd.exe. Pre-existing, Windows-only, CI unaffected (it invokes node directly). Same root cause as the just test-tailwind quoting fixed in CI: build the Tailwind plugin against real Tailwind, and assert the bad build fails #17.
• .claude/worktrees/ holds three stale agent worktrees from the parallel theme work, untracked and not in .gitignore. Harmless until someone runs git add ., but ruff format --check . picks them up.

[fsecada01]

Adversarial review — PR #35

+403 / -25, 11 files. Reviewed against the diff only.

Overview

Moves tab.id out of four Alpine expression attributes and into data-cf-tab, read back as $el.dataset.cfTab. Adds a tree-wide source guard, a browser-level proof, and documentation. The diagnosis is right, the mechanism is the correct one, and both new test files were checked for non-vacuity rather than assumed. Behaviour is preserved and still asserted — the roving-tabindex edge case, keyboard nav, and the js_off tier all still pass.

Four findings, one of them significant.

---

1. HIGH — the data- attribute is not escaped on the JinjaX path, so the payload just moves

This PR's central claim is that a data- attribute "has no such seam because its value is never parsed as source". That is true of expression injection. It silently assumes the attribute itself is escaped — and on the package's own documented FastAPI/Litestar path, it is not.

jinjax.Catalog() constructs its environment with jinja2.Environment(undefined=StrictUndefined) and adopts autoescape only from a caller-supplied jinja_env (jinjax/catalog.py:186-194). install_cf_ui never touches it. Verified against the real installer:

c = Catalog()
install_cf_ui(c, theme="bulma")
c.render("Cf:Tabs", tabs=[{"id": '" onmouseover="window.cfPwned=true" x="', "url": "/x/"}],
         active="", hx_target="tc")

autoescape = False

data-cf-tab="" onmouseover="window.cfPwned=true" x=""
data-cf-tab="" onmouseover="window.cfPwned=true" x=""

double quote escaped anywhere? False
live onmouseover attribute?     True

A double-quote-bearing tab.id closes data-cf-tab and installs a live event handler on both the <li> and the <a>. Same class of bug, different quote character.

Three things worth separating:

• This is not a regression. The pre-existing code was exposed the same way and had the expression injection on top. The PR strictly improves matters.
• The Cotton/Django path is safe. Django autoescapes by default, which is why the Cotton unit tests pass honestly.
• The tests cannot see it. Every Jinja fixture in the new module — and in test_accessibility.py before it — builds Environment(autoescape=select_autoescape([...])) by hand. That is a strictly friendlier environment than install_cf_ui produces, so the suite proves the property under a configuration the package does not ship. This is precisely the proxy-test failure mode epic Epic: axis + component hardening — close deferred review findings from #5, #6, #7 #19 was opened to close, and it now sits directly underneath a security claim.

autoescape=False package-wide was already on the follow-up list, so this is known — but it should not stay a background item while docs/accessibility.md says the data-attribute route "has no such seam". Either:

• set catalog.jinja_env.autoescape = True in install_cf_ui (and in the Litestar installer), assert it in a test, and keep the claim; or
• merge as-is and narrow the wording to what is actually true today — the value is no longer executed, and attribute-level escaping is tracked separately — with the autoescape fix as a named blocker before the next release.

The first is a few lines and would let the sentence stand. Whichever way, one Jinja fixture built the way install_cf_ui builds it would keep this from being re-discovered later.

2. MEDIUM — the tree guard only matches double-quoted attribute values

ALPINE_ATTR ends in \s*=\s*"([^"]*)". Probed directly:

CAUGHT  :tabindex="tabIndexFor({{ tab.id }})"
MISSED  :tabindex='tabIndexFor({{ tab.id }})'
MISSED  :tabindex={{ tab.id }}
MISSED  hx-get="{{ url }}"          <-- correct, HTMX values are not source

Single-quoted delimiters are not hypothetical: they are the natural move for an author who needs a double quote inside the expression — :class='{ "is-active": … }'. No template uses them today (checked), so nothing is currently slipping through. But this guard is the only thing standing between a sixth theme and reintroducing the exact bug being fixed, which raises the bar for it: ["'] with a backreference, plus an unquoted branch, closes both.

Worth adding a self-test for the guard itself — feed it the strings above and assert which are caught. A regex that quietly stops matching is a lint that reports clean forever.

3. LOW — THEMES is hardcoded in both new files, so the per-theme cases will not pick up the incoming themes

THEMES = ["bulma", "daisy"] in test_alpine_expression_safety.py and test_alpine_injection.py. cf_ui.themes.THEMES is importable and is the registry resolve_theme already gates on.

The tree guard (section 1 of the module) rglobs and does auto-scale, which is the important half. But sections 2 and 3, and the entire E2E proof, will silently keep covering two themes after #29/#30/#31 land — and those three PRs are the reason this one exists. test_accessibility.py hardcodes the same list, so this matches existing convention; the convention is the thing worth changing, in a repo whose CLAUDE.md advertises that adding a theme to THEMES makes the parametrized tests pick it up.

4. LOW — Alpine 3.14.1 is now in four places with no drift guard

assets.jinja:47, templatetags/cf_ui.py:26, tests/integration/jinja_app/main.py:139, and now test_alpine_injection.py:33. The new copy carries a comment asking the reader to keep it in step, which is the same arrangement axes.py deliberately refused for its generated artifacts — those get a drift test. Importing _DEFAULTS["alpinejs"] costs nothing here and removes the fourth copy; asserting assets.jinja agrees with _DEFAULTS would close the other two. Pre-existing, and this PR only widens it by one.

---

Checked and clear

• $el in a :class/@click binding resolves to the element carrying the directive, so the <li>-level binding is correct and does not reach into its child — the pairing test in section 2 enforces exactly that.
• data-cf-tab now appears on both the <li> and the <a> in Bulma. Nothing selects [data-cf-tab] bare: _tabs() filters on [role="tab"] and every other read is document.activeElement.dataset.cfTab, so the duplicate is inert.
• The lookbehind correctly excludes hx-get/hx-target, which are a URL and a selector, not source.
• The E2E test's second assertion (aria-selected === 'true' before checking the payload) is genuinely load-bearing — without it a fix that made Alpine throw would pass.
• Assembling the E2E page in the test rather than adding a hostile route to the demo app is the right call.
• Both test_tabs_keeps_the_alpine_contract updates assert the new form and data-cf-tab="one", so they did not just get loosened.

Verdict

Merge-worthy on its own terms; it removes a live, no-interaction code-execution path and leaves an enforcement mechanism behind. Finding 1 is the one that needs a decision before merge, because it determines whether docs/accessibility.md ships a claim that is stronger than the code — not whether this diff should land.

[fsecada01]

Addressed the review. Second commit (64a937a):

Finding 1 — split out as #36 rather than folded in. Switching JinjaX's autoescape on changes behaviour for existing consumers, so it is a release-shaped decision, not a line in a security patch. What changed here instead: docs/accessibility.md states the exposure plainly with an interim workaround, and the CHANGELOG points at #36 rather than implying attribute safety cf-ui does not yet provide. #36 carries the reproduction, the three implementation options, and — as its first acceptance criterion — an integration-tier fixture built the way install_cf_ui builds one, since that omission is why the suite could not see this.

Findings 2, 3, 4 — fixed, all in code this PR introduced. ALPINE_ATTR now matches single-quoted and bare values as well as double-quoted; test_the_guard_flags_what_it_claims_to pins thirteen cases of what the guard catches and what it correctly ignores; both new files take THEMES from cf_ui.themes so the per-theme cases and the E2E proof pick up the three incoming themes automatically; and the E2E Alpine URL resolves from _ALPINE_CDN/_DEFAULTS instead of a fourth hardcoded 3.14.1.

638 unit + integration pass (up from 625 — the guard's self-tests), ruff and prek clean.