From 0c6c536d07637ceba9839d21d4ca13e7e35cf96f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luk=C3=A1=C5=A1=20Kucharczyk?= Date: Sat, 13 Jun 2026 18:35:43 +0200 Subject: [PATCH] Ban SafeText-as-child: only Safe nodes render unescaped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tightens the child model so the type is honest end to end. Previously a ``SafeText``/``mark_safe`` string passed as a child rendered unescaped — a trusted-HTML-as-string backdoor that ``Child = Node | str`` couldn't express (every ``SafeText`` is a ``str``). Now ``_child_key`` escapes *every* string child; the only way to put trusted pre-rendered HTML into the tree is a ``Safe`` node. So a ``str`` child is always untrusted text — which is exactly what the renderer escapes. Converted the trusted-HTML children that relied on the old passthrough: - ``CsrfInput`` and the Alpine selectors (``GameStatusSelector`` / ``SessionDeviceSelector``) now return ``Safe`` nodes instead of ``mark_safe`` strings — they are always tree children. - ``popover_content`` is now a ``Child`` (it is rendered as a child); the one HTML caller (``LinkedPurchase``) passes ``Safe(...)``. - View-side children that were ``mark_safe`` strings → ``Safe(...)``: ``_played_row`` (game detail), the stat SVGs and `` `` spacer (game), the login table (auth), the manual session-form field/label markup (session), and ``_purchase_name`` (stats). - ``SimpleTable.header_action`` typed ``Child``. The script-tag string helpers (``ModuleScript`` / ``StaticScript`` / ``ExternalScript``) stay ``SafeText`` strings: they are only ever joined into the ``scripts=`` string, never used as tree children. ``Children`` regains a bare ``Node`` member (a single node child is valid); the one ``*children`` site (``Popover``) normalises via ``as_children`` first. Tests that asserted the old SafeText-passthrough now assert the new rule (mark_safe child escaped; ``Safe`` node passes through). Full suite green (445; +2 new escaping tests). Co-Authored-By: Claude Opus 4.8 --- CLAUDE.md | 2 +- common/components/core.py | 24 ++++++++++++++---------- common/components/domain.py | 13 ++++++------- common/components/primitives.py | 20 ++++++++++++-------- games/views/auth.py | 5 ++--- games/views/game.py | 16 ++++++++-------- games/views/session.py | 7 ++++--- games/views/stats_content.py | 17 +++++++++++++---- tests/test_components.py | 24 ++++++++++++++++-------- tests/test_node_tree.py | 12 ++++++++++-- 10 files changed, 86 insertions(+), 54 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 4a9046a..6456fa5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,7 +62,7 @@ docs/ — Additional documentation **Component system** (`common/components/`): a FastHTML-style **lazy node tree**. Components are `Node` objects that render to HTML only when asked (`str(node)` / `Page()`), so `Page()` can walk a finished tree and collect each component's JS. Split into submodules re-exported via `common/components/__init__.py`: -- **`core.py`** — the node layer. `Node` (base; `__html__`/`__str__` return a `SafeString`), `Element` (the single class for *any* HTML element), `Safe` (wraps pre-rendered/trusted HTML), `Fragment` (ordered children, no wrapper tag — use instead of `str(a)+str(b)`), `BaseComponent` (base for higher-level components: implement `render()`, declare `media`), and `Media` (declarative JS deps with order-preserving dedup merge; `collect_media()` sums them over a tree, `node.with_media(...)` attaches them). `_render_element()` is `@lru_cache`-memoized (4096). Attribute values are always escaped; children escaped unless `SafeText`/`Node`. `randomid()` generates stable hash-based IDs. +- **`core.py`** — the node layer. `Node` (base; `__html__`/`__str__` return a `SafeString`), `Element` (the single class for *any* HTML element), `Safe` (wraps pre-rendered/trusted HTML), `Fragment` (ordered children, no wrapper tag — use instead of `str(a)+str(b)`), `BaseComponent` (base for higher-level components: implement `render()`, declare `media`), and `Media` (declarative JS deps with order-preserving dedup merge; `collect_media()` sums them over a tree, `node.with_media(...)` attaches them). `_render_element()` is `@lru_cache`-memoized (4096). Attribute values are always escaped. **Children: every string child is escaped — `SafeText`/`mark_safe` included; only `Node` children (so `Safe`) render unescaped.** Trusted pre-rendered HTML must be wrapped in `Safe(...)`, never passed as a safe string. `randomid()` generates stable hash-based IDs. - **`primitives.py`** — Generic HTML. Plain leaf builders (`Div`, `Span`, `P`, `Ul`, `Li`, `Strong`, `Label`, `Template`, `Td`, `Tr`, `Th`) are **generated from a whitelist** via the `_html_element(tag)` factory over `Element` — not hand-written per tag. Builders that add classes/behaviour are written out: `A()`, `Button()`, `ButtonGroup()`, `Input()`, `Checkbox()`, `Radio()`, `Pill()`, `Icon()`, `Popover()`, `PopoverTruncated()`, `SearchField()`, `H1()`, `Modal()`, `SimpleTable()`, `TableRow()`, `TableTd()`, `TableHeader()`, `paginated_table_content()`, `AddForm()`, `YearPicker()` (declares datepicker media), `CsrfInput()`/`ModuleScript()`/`StaticScript()` (script-tag string helpers used by `Page()`). - **`domain.py`** — Domain-specific: `GameLink()`, `GameStatus()` (colored dot + label), `GameStatusSelector()` (Alpine.js PATCH dropdown), `SessionDeviceSelector()` (Alpine.js PATCH dropdown), `LinkedPurchase()`, `NameWithIcon()`, `PriceConverted()`, `PurchasePrice()` - **`filters.py`** — Filter UI: `FilterBar()`, `SessionFilterBar()`, `PurchaseFilterBar()` (built from `FilterSelect` widgets) diff --git a/common/components/core.py b/common/components/core.py index 25c119f..0e518e5 100644 --- a/common/components/core.py +++ b/common/components/core.py @@ -134,18 +134,20 @@ class Node: return mark_safe(self._render()) -# A renderable child is a node or a string (plain strings are escaped, SafeText -# and nodes pass through). ``Children`` is the type for a builder's ``children`` +# A renderable child is a node or a string. Strings are ALWAYS escaped (a string +# is untrusted text — ``SafeText``/``mark_safe`` is escaped too); trusted +# pre-rendered HTML must be a ``Safe`` node. ``Children`` is the type for a +# builder's ``children`` # parameter: a sequence of child nodes/strings, a bare string, or nothing. The # sequence is a covariant ``Sequence`` so ``list[Element]`` / ``list[Node]`` are # accepted (a plain ``list[str]`` would be invariant and reject them). A single # bare ``Node`` is accepted only by ``Element`` itself (which wraps it); the # higher-level builders take ``Children``. Child = Node | str -Children = Sequence[Child] | str | None +Children = Sequence[Child] | Node | str | None -def as_children(children: "Children | Node") -> list[Child]: +def as_children(children: Children) -> list[Child]: """Normalise a builder's ``children`` argument to a flat list. Accepts ``None`` (→ empty), a single node/string (→ one-element list), or a @@ -172,16 +174,18 @@ def as_attributes(attributes: "Attributes | None") -> list[HTMLAttribute]: def _child_key(child: object) -> tuple[str, bool]: """Normalise a child to a ``(text, is_safe)`` pair. - Nodes render to safe HTML; ``SafeText`` (and anything exposing ``__html__``) - is already safe; plain strings are escaped. ``is_safe`` is part of the - render cache key so a safe ``""`` and an unsafe ``""`` never collide. + Only :class:`Node` children render unescaped — that includes :class:`Safe`, + the one sanctioned way to put trusted pre-rendered HTML into the tree. Every + *string* child is escaped, ``SafeText``/``mark_safe`` included: a string is + always treated as untrusted text, so trusted markup must be wrapped in + ``Safe(...)`` rather than smuggled in as a safe string. ``is_safe`` is part + of the render cache key so a safe ``""`` and an unsafe ``""`` never + collide. """ if isinstance(child, Node): return (child._render(), True) if isinstance(child, str): - return (child, isinstance(child, SafeText)) - if hasattr(child, "__html__"): - return (child.__html__(), True) + return (child, False) return (str(child), False) diff --git a/common/components/domain.py b/common/components/domain.py index b77a9a2..f4534a3 100644 --- a/common/components/domain.py +++ b/common/components/domain.py @@ -4,9 +4,8 @@ from typing import Any from django.template.defaultfilters import floatformat from django.urls import reverse -from django.utils.safestring import SafeText, mark_safe -from common.components.core import Children, Node, as_children +from common.components.core import Children, Node, Safe, as_children from common.components.primitives import ( A, Div, @@ -130,7 +129,7 @@ def LinkedPurchase(purchase: Purchase) -> Node: ), PopoverTruncated( input_string=link_content, - popover_content=mark_safe(popover_content), + popover_content=Safe(popover_content), popover_if_not_truncated=popover_if_not_truncated, ), ], @@ -210,7 +209,7 @@ def PurchasePrice(purchase) -> Node: ) -def GameStatusSelector(game, game_statuses, csrf_token: str) -> SafeText: +def GameStatusSelector(game, game_statuses, csrf_token: str) -> Node: """Alpine.js dropdown to change a game's status.""" options_html = "\n".join( f"