Ban SafeText-as-child: only Safe nodes render unescaped

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 <noreply@anthropic.com>
2026-06-13 18:35:43 +02:00
parent 544da26a9d
commit 0c6c536d07
10 changed files with 86 additions and 54 deletions
@@ -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)
@@ -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 ``"<b>"`` and an unsafe ``"<b>"`` 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 ``"<b>"`` and an unsafe ``"<b>"`` 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)


@@ -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"<template x-if=\"status == '{value}'\">"
@@ -228,7 +227,7 @@ def GameStatusSelector(game, game_statuses, csrf_token: str) -> SafeText:
        for value, label in game_statuses
    )

-    return mark_safe(f"""
+    return Safe(f"""
 <div class="flex gap-2 items-center"
     x-data="{{
         status: '{game.status}',
@@ -261,7 +260,7 @@ def GameStatusSelector(game, game_statuses, csrf_token: str) -> SafeText:
 """)


-def SessionDeviceSelector(session, session_devices, csrf_token: str) -> SafeText:
+def SessionDeviceSelector(session, session_devices, csrf_token: str) -> Node:
    """Alpine.js dropdown to change a session's device."""
    device_id = session.device_id or "null"
    device_name = (session.device.name if session.device else "Unknown").replace(
@@ -276,7 +275,7 @@ def SessionDeviceSelector(session, session_devices, csrf_token: str) -> SafeText
        for d in session_devices
    )

-    return mark_safe(f"""
+    return Safe(f"""
 <div class="flex gap-2 items-center"
     x-data="{{
         originalDeviceId: {device_id},
@@ -15,6 +15,7 @@ from django.utils.safestring import SafeText, mark_safe

 from common.components.core import (
    Attributes,
+    Child,
    Children,
    Element,
    Fragment,
@@ -80,7 +81,7 @@ Th = _html_element("th")

 def _popover_html(
    id: str,
-    popover_content: str,
+    popover_content: Child,
    wrapped_content: str = "",
    wrapped_classes: str = "",
    slot: "Node | str" = "",
@@ -130,14 +131,14 @@ def _popover_html(


 def Popover(
-    popover_content: str,
+    popover_content: Child,
    wrapped_content: str = "",
    wrapped_classes: str = "",
    children: Children = None,
    attributes: Attributes | None = None,
    id: str = "",
 ) -> Node:
-    children = children or []
+    children = as_children(children)
    if not wrapped_content and not children:
        raise ValueError("One of wrapped_content or children is required.")
    if not id:
@@ -155,7 +156,7 @@ def Popover(

 def PopoverTruncated(
    input_string: str,
-    popover_content: str = "",
+    popover_content: Child = "",
    popover_if_not_truncated: bool = False,
    length: int = 30,
    ellipsis: str = "…",
@@ -507,9 +508,12 @@ def Pill(
    return Span(attributes=pill_attrs, children=children)


-def CsrfInput(request) -> SafeText:
-    """Hidden CSRF input, equivalent to the `{% csrf_token %}` template tag."""
-    return mark_safe(
+def CsrfInput(request) -> Node:
+    """Hidden CSRF input, equivalent to the `{% csrf_token %}` template tag.
+
+    Returns a ``Safe`` node (not a safe string): it is always used as a tree
+    child, and only nodes render unescaped now."""
+    return Safe(
        f'<input type="hidden" name="csrfmiddlewaretoken" value="{get_token(request)}">'
    )

@@ -958,7 +962,7 @@ def _pagination_nav(page_obj, elided_page_range, request) -> str:
 def SimpleTable(
    columns: list[str] | None = None,
    rows: list | None = None,
-    header_action: SafeText | str | None = None,
+    header_action: Child | None = None,
    page_obj=None,
    elided_page_range=None,
    request=None,
@@ -3,9 +3,8 @@ registration/login.html)."""

 from django.contrib.auth import views as auth_views
 from django.http import HttpResponse
-from django.utils.safestring import mark_safe

-from common.components import CsrfInput, Div, Element, Input, Node
+from common.components import CsrfInput, Div, Element, Input, Node, Safe
 from common.components.primitives import Td, Tr
 from common.layout import render_page

@@ -15,7 +14,7 @@ def _login_content(form, request) -> Node:
        "table",
        children=[
            CsrfInput(request),
-            mark_safe(str(form.as_table())),
+            Safe(str(form.as_table())),
            Tr(
                children=[
                    Td(),
@@ -28,9 +28,11 @@ from common.components import (
    Modal,
    ModuleScript,
    NameWithIcon,
+    Node,
    Popover,
    PopoverTruncated,
    PurchasePrice,
+    Safe,
    SearchField,
    SimpleTable,
    Ul,
@@ -386,7 +388,7 @@ _PLAYED_ROW_TEMPLATE = """<div class="flex gap-2 items-center" x-data="{ open: f
 </div>"""


-def _played_row(game: Game, request: HttpRequest) -> SafeText:
+def _played_row(game: Game, request: HttpRequest) -> Node:
    """The 'Played N times' control with its Alpine.js dropdown."""
    replacements = {
        "@@PLAYED_COUNT@@": str(game.playevents.count()),
@@ -400,7 +402,7 @@ def _played_row(game: Game, request: HttpRequest) -> SafeText:
    html = _PLAYED_ROW_TEMPLATE
    for token, value in replacements.items():
        html = html.replace(token, value)
-    return mark_safe(html)
+    return Safe(html)


 def _stat_popover(popover_id: str, tooltip: str, svg_key: str, value: str) -> SafeText:
@@ -408,14 +410,12 @@ def _stat_popover(popover_id: str, tooltip: str, svg_key: str, value: str) -> Sa
        popover_content=tooltip,
        wrapped_classes="flex gap-2 items-center",
        id=popover_id,
-        children=[mark_safe(_STAT_SVGS[svg_key]), str(value)],
+        children=[Safe(_STAT_SVGS[svg_key]), str(value)],
    )


-def _meta_row(
-    label: str, value: SafeText | str, extra: SafeText | str = ""
-) -> SafeText:
-    children: list[SafeText | str] = [
+def _meta_row(label: str, value: Node | str, extra: Node | str = "") -> Node:
+    children: list[Node | str] = [
        Span(attributes=[("class", "uppercase")], children=[label]),
        value,
    ]
@@ -565,7 +565,7 @@ def _game_header(game: Game, request: HttpRequest, metrics: dict[str, Any]) -> S
        ]
        + (
            [
-                mark_safe("&nbsp;"),
+                Safe("&nbsp;"),
                Popover(
                    popover_content="Original release year",
                    wrapped_classes="text-slate-500 text-2xl",
@@ -22,6 +22,7 @@ from common.components import (
    NameWithIcon,
    Node,
    Popover,
+    Safe,
    SearchField,
    SessionDeviceSelector,
    paginated_table_content,
@@ -199,9 +200,9 @@ def _session_fields(form) -> Fragment:
    """
    rows: list[Node] = []
    for field in form:
-        children: list[SafeText | str] = [
-            mark_safe(str(field.label_tag())),
-            mark_safe(str(field)),
+        children: list[Node | str] = [
+            Safe(str(field.label_tag())),
+            Safe(str(field)),
        ]
        if field.name in ("timestamp_start", "timestamp_end"):
            this_side = "start" if field.name == "timestamp_start" else "end"
@@ -9,9 +9,18 @@ from django.template.defaultfilters import date as date_filter
 from django.template.defaultfilters import floatformat
 from django.urls import reverse
 from django.utils.html import conditional_escape
-from django.utils.safestring import mark_safe
-
-from common.components import A, Div, Element, GameLink, Node, Td, Th, Tr, YearPicker
+from common.components import (
+    A,
+    Div,
+    Element,
+    GameLink,
+    Node,
+    Safe,
+    Td,
+    Th,
+    Tr,
+    YearPicker,
+)
 from common.time import durationformat, format_duration

 _CELL = "px-2 sm:px-4 md:px-6 md:py-2"
@@ -70,7 +79,7 @@ def _purchase_name(purchase) -> Node:
        name = game_name or purchase.name
        link = GameLink(first_game.id, name)
        suffix = f" ({first_game.name} {purchase.get_type_display()})"
-        return mark_safe(str(link) + conditional_escape(suffix))
+        return Safe(str(link) + conditional_escape(suffix))
    name = game_name or first_game.name
    return GameLink(first_game.id, name)

@@ -55,10 +55,10 @@ class ComponentCacheTest(unittest.TestCase):
        self.assertEqual(components._render_element.cache_parameters()["maxsize"], 4096)

    def test_safe_and_unsafe_children_do_not_collide(self):
-        """A SafeText "<b>" and a plain "<b>" are equal as strings but must
-        render differently — the cache key must keep them distinct."""
+        """A Safe-node ``<b>`` and a plain-string ``<b>`` render differently —
+        the cache key must keep them distinct."""
        safe = str(
-            components.Element(tag_name="span", children=[mark_safe("<b>x</b>")])
+            components.Element(tag_name="span", children=[components.Safe("<b>x</b>")])
        )
        unsafe = str(components.Element(tag_name="span", children=["<b>x</b>"]))
        self.assertIn("<b>x</b>", safe)
@@ -350,13 +350,23 @@ class ComponentEdgeCasesTest(unittest.TestCase):
        self.assertNotIn("<script>", result)
        self.assertIn("&lt;script&gt;", result)

-    def test_mark_safe_children_pass_through(self):
+    def test_safe_node_children_pass_through(self):
+        result = str(
+            components.Element(
+                tag_name="div", children=[components.Safe("<span>safe</span>")]
+            )
+        )
+        self.assertIn("<span>safe</span>", result)
+
+    def test_mark_safe_string_children_are_escaped(self):
+        # Trusted markup must be a Safe node; a mark_safe string is still a
+        # string, so it is escaped like any other text child.
        result = str(
            components.Element(
                tag_name="div", children=[mark_safe("<span>safe</span>")]
            )
        )
-        self.assertIn("<span>safe</span>", result)
+        self.assertIn("&lt;span&gt;safe&lt;/span&gt;", result)

    def test_attribute_values_are_escaped(self):
        result = str(
@@ -840,14 +850,12 @@ class SimpleTableRenderingTest(unittest.TestCase):

    def test_simple_table_header_action_as_caption(self):
        """Verify header_action renders inside <caption>."""
-        from django.utils.safestring import mark_safe
-
        result = str(
            str(
                components.SimpleTable(
                    columns=["Game", "Started"],
                    rows=[["Game1", "2025-01-01"]],
-                    header_action=mark_safe('<a href="/add">Add</a>'),
+                    header_action=components.Safe('<a href="/add">Add</a>'),
                )
            )
        )
@@ -29,10 +29,18 @@ class ElementRenderTest(unittest.TestCase):
            render(Element("span", children=["<b>"])), "<span>&lt;b&gt;</span>"
        )

-    def test_safe_children_pass_through(self):
+    def test_safe_node_child_passes_through(self):
+        self.assertEqual(
+            render(Element("span", children=[Safe("<b>x</b>")])),
+            "<span><b>x</b></span>",
+        )
+
+    def test_safetext_child_is_escaped(self):
+        # A string child is always escaped — even a mark_safe/SafeText one.
+        # Trusted markup must be a Safe node, not a safe string.
        self.assertEqual(
            render(Element("span", children=[mark_safe("<b>x</b>")])),
-            "<span><b>x</b></span>",
+            "<span>&lt;b&gt;x&lt;/b&gt;</span>",
        )

    def test_node_children_render_safely(self):