fix(templates): read every continuation of a comment holding template tags

docs/usage.md

@@ -212,8 +212,12 @@ checked, a quoted value stays quoted until its quote closes, and a variable in a
 ends there and the browser would never run it. A template including one that ends inside
 such a block, a tag or an end tag is not established either: the markup after the
 include continues it. A template extending another ends where that one ends. A comment
-hides what it holds only up to its first template tag, since an include there may close
-it and a condition around its `<!--` may leave it out.
+holding template tags may end at any of them, since an include there may close it and a
+condition around its `<!--` may leave it out, or run to its close when they render
+nothing (#278). The markup is read on from its first tag, and that reading must be in
+HTML data right after each later tag and right after the close, where the others
+resume; where it is not, the template is not established. Such a comment left open does
+not end in HTML data, since it may hide what follows.
 
 Each page is also put together as Django renders it, and read once more (#273): the
 templates it extends, each block taking the definition of the most derived one and `{{

src/coretrace_python/taint/templates.py

@@ -19,8 +19,10 @@
   construct of another file is read there (#273);
 - no Python file of the project turning autoescaping off.
 
-A comment hides what it holds only up to its first template tag: an include there may
-close it, and a condition around its ``<!--`` may leave it out.
+A comment holding template tags may end at any of them, an include there closing it or a
+condition around its ``<!--`` leaving it out, or run to its close when they render
+nothing: the markup after its first tag is read, and must be in HTML data wherever the
+other readings resume (#278).
 
 Templates are found under the project's ``templates`` directories, by their path below
 one. A template the engine cannot find — named by an expression, under a ``DIRS`` entry
@@ -49,6 +51,8 @@
 from __future__ import annotations
 
 import re
+from bisect import bisect_left
+from collections import deque
 from collections.abc import Container, Iterable, Iterator, Mapping, Sequence
 from dataclasses import dataclass, field
 from pathlib import Path
@@ -775,17 +779,51 @@ def _read_markup(html: str) -> _Reading:
     hides the rest of the text, and hands nothing on: what follows its include stays
     inside it.
 
+    A comment holding template tags may also end at a later one of them, or run to its
+    close when they render nothing (#278). Each of these readings resumes, right after
+    that tag or that close, where this one must then be in HTML data: there, they read
+    what it reads. Where it is inside another construct, the text is refused. Such a
+    comment left open does not end in data: its tags may render nothing, and it then
+    hides what follows.
+
     An inserted markup (``_INSERTED``) may hold anything a file of its name holds: it is
     accepted only in HTML data, never in a comment, a script or style block, a tag or
     an end tag, nor after what it could complete into one. In data it is as if the
-    markup were there, provided that markup is accepted and ends in data (#273)."""
+    markup were there, provided that markup is accepted and ends in data (#273).
+
+    The reading stays linear, but for looking up inserted markups by position: it only
+    moves forward, and every part of the text is looked at once. The points where other
+    readings resume come in two sequences that never go back, the later tags of comments
+    as they are listed and the closes of comments as these are read, so a queue of each
+    holds them."""
 
     position = 0
     open_end = False
     # The first comment close after the comment being read, searched once for all the
     # comments that open before it; and the position after which none closes.
     close: re.Match[str] | None = None
     unclosed_from = len(html) + 1
+    # Where other readings of comments holding tags resume, which this one must reach in
+    # HTML data; how far the tags inside comments have been listed, a comment read inside
+    # another sharing the rest of its region; whether such a comment is left open; and
+    # where inserted markups stand, for a comment to look for one inside it at once.
+    after_tags: deque[int] = deque()
+    after_closes: deque[int] = deque()
+    tags_listed_to = 0
+    left_open = False
+    inserted = [match.start() for match in re.finditer(_INSERTED, html)] if _INSERTED in html else []
+
+    def crossed(start: int, end: int) -> bool:
+        """Whether a point of resumption lies inside the construct read from ``start``
+        to ``end``: those before it are reached in data, and leave the queues."""
+
+        if not after_tags and not after_closes:
+            return False
+        for points in (after_tags, after_closes):
+            while points and points[0] <= start:
+                points.popleft()
+        return any(points and points[0] < end for points in (after_tags, after_closes))
+
     while (start := html.find("<", position)) != -1:
         if html.startswith("<!--", start):
             if start >= unclosed_from:
@@ -795,32 +833,52 @@ def _read_markup(html: str) -> _Reading:
                 if close is None:
                     unclosed_from = start
             hidden_to = len(html) if close is None else close.start()
-            first_tag = html.find(_TAG, start + 4, hidden_to)
-            if html.find(_INSERTED, start + 4, hidden_to if first_tag == -1 else first_tag) != -1:
+            if inserted and (index := bisect_left(inserted, start + 4)) < len(inserted) and inserted[index] < hidden_to:
                 return _NOT_READ
+            first_tag = html.find(_TAG, start + 4, hidden_to)
             if first_tag != -1:
+                if crossed(start, first_tag + 1):
+                    return _NOT_READ
+                tag = html.find(_TAG, max(first_tag + 1, tags_listed_to), hidden_to)
+                while tag != -1:
+                    after_tags.append(tag + 1)
+                    tag = html.find(_TAG, tag + 1, hidden_to)
+                tags_listed_to = max(tags_listed_to, hidden_to)
+                if close is None:
+                    left_open = True
+                else:
+                    after_closes.append(close.end())
                 position, open_end = first_tag + 1, False
             elif close is not None:
+                if crossed(start, close.end()):
+                    return _NOT_READ
                 position, open_end = close.end(), False
             else:
+                if crossed(start, len(html)):
+                    return _NOT_READ
                 return _Reading(escapes=True, open_end=False)
         elif raw := _RAW_TEXT.match(html, start):
             if _OUTPUT in raw.group(2) or _INSERTED in raw.group(0) or not _attributes_escape(html[start : raw.start(2)]):
                 return _NOT_READ
+            if crossed(start, raw.end()):
+                return _NOT_READ
             position, open_end = raw.end(), not raw.group(3)
-        elif tag := _START_TAG.match(html, start):
-            if _OUTPUT in tag.group(1) or _INSERTED in tag.group(0) or not _attributes_escape(tag.group(2)):
+        elif tag_match := _START_TAG.match(html, start):
+            if _OUTPUT in tag_match.group(1) or _INSERTED in tag_match.group(0) or not _attributes_escape(tag_match.group(2)):
+                return _NOT_READ
+            if crossed(start, tag_match.end()):
                 return _NOT_READ
-            position, open_end = tag.end(), tag.group(3) is None
+            position, open_end = tag_match.end(), tag_match.group(3) is None
         elif end := _END_TAG.match(html, start):
-            if _OUTPUT in end.group(0) or _INSERTED in end.group(0):
+            if _OUTPUT in end.group(0) or _INSERTED in end.group(0) or crossed(start, end.end()):
                 return _NOT_READ
             position, open_end = end.end(), end.group(1) is None
         elif _NAMED_BY_OUTPUT.match(html, start) or _OPENED_BY_INSERTION.match(html, start):
             return _NOT_READ
         else:
             position = start + 1
-    return _Reading(escapes=True, open_end=open_end, settled=not open_end and not _ENDING_OPENER.search(html))
+    settled = not open_end and not left_open and not _ENDING_OPENER.search(html)
+    return _Reading(escapes=True, open_end=open_end, settled=settled)
 
 
 def _attributes_escape(attributes: str) -> bool:

tests/test_template_comment_continuations.py

@@ -0,0 +1,221 @@
+"""Acceptance tests for issue #278: every continuation of a comment holding template tags.
+
+Rendered, such a comment may end at any of its tags, an include closing it or a condition
+leaving out its opener, or run to its close when its tags render nothing. The reading
+resumed after its first tag (#272) is kept. At every other point where a reading may
+resume, right after each later tag of the comment and right after its close, that reading
+must be in HTML data, where every other continuation reads what it reads; elsewhere, the
+template is refused. Each inspection and analysis runs in a subprocess with a timeout, so
+that one that does not end fails instead of hanging the suite.
+"""
+
+from __future__ import annotations
+
+import itertools
+import json
+import random
+import subprocess
+import sys
+from functools import cache
+from pathlib import Path
+
+import pytest
+
+TIMEOUT = 60
+# Far beyond what a quadratic reading reads within the timeout; a linear one takes about a
+# second.
+SIZE = 3_000_000
+
+ESCAPED_TEMPLATES = (
+    "import json, sys\n"
+    "from pathlib import Path\n"
+    "from coretrace_python.taint.templates import escaped_templates\n"
+    "print(json.dumps(sorted(escaped_templates(Path(sys.argv[1])))))\n"
+)
+
+VIEW = (
+    "from django.http import HttpRequest, HttpResponse\n"
+    "from django.template.loader import render_to_string\n\n"
+    "def page(request: HttpRequest):\n"
+    "    return HttpResponse(render_to_string('page.html', {'name': request.GET['name']}))\n"
+)
+
+
+def run(command: list[str]) -> subprocess.CompletedProcess[str]:
+    try:
+        return subprocess.run(command, capture_output=True, text=True, check=False, timeout=TIMEOUT)
+    except subprocess.TimeoutExpired:
+        pytest.fail(f"{command[1:3]} did not end within {TIMEOUT} s")
+
+
+def project(tmp_path: Path, templates: dict[str, str]) -> Path:
+    """A project rendering ``page.html`` with the request's ``name``; a template given by
+    its name lies under ``app/templates``, one given by a path under that path."""
+
+    root = tmp_path / "project"
+    (root / "app" / "templates").mkdir(parents=True, exist_ok=True)
+    (root / "app" / "views.py").write_text(VIEW, encoding="utf-8")
+    for name, content in templates.items():
+        path = root / name if "/templates/" in name else root / "app" / "templates" / name
+        path.parent.mkdir(parents=True, exist_ok=True)
+        path.write_text(content, encoding="utf-8")
+    return root
+
+
+def escaped(tmp_path: Path, templates: dict[str, str]) -> list[str]:
+    completed = run([sys.executable, "-c", ESCAPED_TEMPLATES, str(project(tmp_path, templates))])
+    assert completed.returncode == 0, completed.stderr
+    names: list[str] = json.loads(completed.stdout)
+    return names
+
+
+def xss(tmp_path: Path, templates: dict[str, str]) -> list[tuple[str, int]]:
+    root = project(tmp_path, templates)
+    completed = run([sys.executable, "-m", "coretrace_python", "--check", str(root), "--format", "json"])
+    findings = json.loads(completed.stdout)["findings"]
+    return [(f["location"]["path"], f["location"]["line"]) for f in findings if f["rule_id"] == "xss"]
+
+
+# The reproduction of #278, rendered with Django 5.1.15 and run in Chrome 155: the comment
+# runs to the "-->" in the title, and the variable after it names a tag.
+def test_a_comment_closed_inside_an_attribute_after_its_tags_is_cross_site_scripting(tmp_path: Path) -> None:
+    templates = {"page.html": '<!--{% if debug %}{% endif %}<a title="--><{{ name }}">x</a>\n'}
+
+    assert xss(tmp_path, templates) == [("app/views.py", 5)]
+
+
+@pytest.mark.parametrize(
+    "template",
+    [
+        '<!--{% if debug %}{% endif %}<a title="--><{{ name }}">x</a>\n',
+        '<!-- {% if a %}x <a title="y {% endif %} z">{{ name }} -->\n',
+    ],
+    ids=["close-inside-a-title", "later-tag-inside-a-title"],
+)
+def test_a_comment_resuming_elsewhere_than_in_html_data_is_not_established(tmp_path: Path, template: str) -> None:
+    assert escaped(tmp_path, {"page.html": template}) == []
+
+
+@pytest.mark.parametrize(
+    "template",
+    [
+        "<!-- {% if a %} note {% endif %} -->\n",
+        "<!-- {% if a %} note {% endif %} --><p>{{ name }}</p>\n",
+        '<!-- <div class="{% if a %}x{% endif %}"> -->\n',
+        '<p>hello</p><!-- {{ name }} <a onclick="{{ name }}"> -->\n',
+    ],
+    ids=["tags-around-text", "variable-after-the-comment", "tags-in-a-class", "no-tag"],
+)
+def test_a_comment_resuming_in_html_data_everywhere_stays_established(tmp_path: Path, template: str) -> None:
+    assert escaped(tmp_path, {"page.html": template}) == ["page.html"]
+
+
+def test_an_include_of_a_name_two_files_have_anywhere_in_a_comment_is_refused(tmp_path: Path) -> None:
+    # The tag before it may render nothing: the include then stands inside the comment.
+    templates = {
+        "part.html": "x\n",
+        "other/templates/part.html": "y\n",
+        "page.html": "<!-- {% if a %} {% include 'part.html' %} -->\n",
+    }
+
+    assert escaped(tmp_path, templates) == ["part.html"]
+
+
+def test_a_comment_holding_tags_left_open_does_not_end_in_html_data(tmp_path: Path) -> None:
+    # Its tags may render nothing: the comment then hides what follows the include.
+    templates = {
+        "part.html": "x\n",
+        "other/templates/part.html": "y <!-- {% if a %}{% endif %}",
+        "page.html": "<div>{% include 'part.html' %}</div>\n",
+    }
+
+    assert escaped(tmp_path, templates) == ["part.html"]
+
+
+@pytest.mark.parametrize(
+    "shape",
+    [
+        "tags-in-one-open-comment",
+        "tags-in-one-closed-comment",
+        "tag-then-open-start-tag",
+        "tag-then-open-script",
+        "comments-with-tags-in-titles",
+    ],
+)
+def test_the_points_of_resumption_keep_the_reading_linear(tmp_path: Path, shape: str) -> None:
+    template = {
+        "tags-in-one-open-comment": "<!--" + "{% if x %}" * (SIZE // 10),
+        "tags-in-one-closed-comment": "<!--" + "{% if x %}x" * (SIZE // 11) + "-->",
+        "tag-then-open-start-tag": "<!--{% if x %}<a " * (SIZE // 17),
+        "tag-then-open-script": "<!--{% if x %}<script>" * (SIZE // 22),
+        "comments-with-tags-in-titles": '<!--{% if x %}<a title="{% if y %}">-->' * (SIZE // 38),
+    }[shape]
+
+    escaped(tmp_path, {"page.html": template})
+
+
+# Pieces of a template's markup as the reader sees it: markup, a variable, a template tag
+# and an inserted markup (#273).
+OUTPUT, TAG, INSERTED = "\x00", "\x01", "\x02"
+PIECES = ["<!--", "-->", "<!-->", "--!>", TAG, OUTPUT, '<a title="', '<a onclick="', '"', ">", "<script>", "</script>", "x", " ", "<", "</a", INSERTED]
+
+
+def every_continuation_accepted(html: str) -> bool:
+    """The reading of ``html`` that follows every continuation of each comment holding
+    template tags: from each of its tags, and from its close. Its cost is not linear;
+    it is the reference the reading of the engine must never be more lenient than."""
+
+    from coretrace_python.taint import templates
+
+    @cache
+    def from_position(position: int) -> bool:
+        while (start := html.find("<", position)) != -1:
+            if html.startswith("<!--", start):
+                close = templates._COMMENT_END.search(html, start + 2)
+                hidden_to = len(html) if close is None else close.start()
+                if INSERTED in html[start + 4 : hidden_to]:
+                    return False
+                tags = [index for index in range(start + 4, hidden_to) if html[index] == TAG]
+                if not all(from_position(tag + 1) for tag in tags):
+                    return False
+                if close is None:
+                    return True
+                position = close.end()
+            elif raw := templates._RAW_TEXT.match(html, start):
+                if OUTPUT in raw.group(2) or INSERTED in raw.group(0) or not templates._attributes_escape(html[start : raw.start(2)]):
+                    return False
+                position = raw.end()
+            elif tag := templates._START_TAG.match(html, start):
+                if OUTPUT in tag.group(1) or INSERTED in tag.group(0) or not templates._attributes_escape(tag.group(2)):
+                    return False
+                position = tag.end()
+            elif end := templates._END_TAG.match(html, start):
+                if OUTPUT in end.group(0) or INSERTED in end.group(0):
+                    return False
+                position = end.end()
+            elif templates._NAMED_BY_OUTPUT.match(html, start) or templates._OPENED_BY_INSERTION.match(html, start):
+                return False
+            else:
+                position = start + 1
+        return True
+
+    return from_position(0)
+
+
+def test_the_reading_accepts_no_text_a_reading_of_every_continuation_refuses() -> None:
+    """Checked on every text of up to four pieces, and on random longer ones."""
+
+    from coretrace_python.taint import templates
+
+    rng = random.Random(278)
+    texts = [
+        *("".join(pieces) for length in range(5) for pieces in itertools.product(PIECES, repeat=length)),
+        *("".join(rng.choice(PIECES) for _ in range(rng.randint(5, 14))) for _ in range(30_000)),
+    ]
+    refused_by_the_reference = 0
+    for text in texts:
+        accepted = templates._read_markup(text).escapes
+        reference = every_continuation_accepted(text)
+        refused_by_the_reference += not reference
+        assert reference or not accepted, text
+    assert refused_by_the_reference