Escape untrusted values rendered into issue/PR bodies

vulnhunter-agent/agent/issues_render.py

@@ -101,6 +101,35 @@ def render_title(f: Finding) -> str:
 _MD_ESCAPE_CHARS = ("\\", "`", "[", "]")
 
 
+# Machine-readable footer markers (``<!-- vulnhunt-finding-id: ... -->`` etc.)
+# are intentionally NOT html.escaped so the marker regexes stay exact. But the
+# values (``f.id``, ``report.results_dir_name``) are attacker-influenced
+# LLM/scan-README data, so a value containing ``-->`` would close the comment
+# and inject arbitrary markdown/HTML (CWE-79 / CWE-116). Restrict marker values
+# to a strict identifier charset — the same class the downstream parsers accept
+# (``VULN-\d{3}`` / ``[A-Za-z0-9._-]+_VULNHUNT_RESULTS_...``) — dropping any
+# other char. Removing '<' and '>' makes a comment breakout impossible while
+# keeping legitimate ids/dir-names intact.
+_MARKER_UNSAFE_RE = re.compile(r"[^A-Za-z0-9._-]")
+
+
+def _sanitize_marker_value(value: str, marker: str = "marker") -> str:
+    """Restrict an HTML-comment marker value to a safe identifier charset so it
+    cannot break out of the surrounding ``<!-- ... -->`` (CWE-116).
+
+    Stripping can turn a value into a *different* identifier (e.g. a
+    results-dir name that now points at another directory), so any change is
+    logged as a warning rather than happening silently."""
+    raw = value or ""
+    cleaned = _MARKER_UNSAFE_RE.sub("", raw)
+    if cleaned != raw:
+        logger.warning(
+            "Stripped out-of-charset characters from %s marker value: %r -> %r",
+            marker, raw, cleaned,
+        )
+    return cleaned
+
+
 def _sanitize_for_issue_body(value: str) -> str:
     """Neutralize attacker-influenced Finding text before it lands in a
     GitHub issue body (CWE-79).
@@ -118,6 +147,40 @@ def _sanitize_for_issue_body(value: str) -> str:
     return s
 
 
+def _sanitize_inline(value: str) -> str:
+    """``_sanitize_for_issue_body`` for fields rendered on a single line —
+    the ``### Finding`` table cells and the H2 title / Priority line.
+
+    Additionally escapes ``|`` as ``\\|`` (GFM's in-cell pipe escape; it
+    renders as a plain ``|`` outside a table too) so a value cannot add or
+    forge table cells, and collapses CR/LF runs to one space so it cannot end
+    the row/heading and inject block markdown (PR #9 review, CWE-116).
+    Multi-line free-form sections keep using ``_sanitize_for_issue_body``.
+    """
+    s = _sanitize_for_issue_body(value).replace("|", "\\|")
+    return re.sub(r"[\r\n]+", " ", s)
+
+
+_PLACEHOLDER_RE = re.compile(r"\{([A-Z_]+)\}")
+
+
+def _substitute(template: str, fields: dict[str, str]) -> tuple[str, set[str]]:
+    """Fill ``{NAME}`` placeholders in ONE pass over the template.
+
+    Field-by-field ``str.replace`` re-scanned already-substituted values, so
+    attacker text containing e.g. ``{IDEMPOTENCY_KEY}`` was expanded by a
+    later pass. Returns (body, template placeholders with no field)."""
+    missing: set[str] = set()
+
+    def repl(m: re.Match[str]) -> str:
+        if m.group(1) in fields:
+            return fields[m.group(1)]
+        missing.add(m.group(1))
+        return m.group(0)
+
+    return _PLACEHOLDER_RE.sub(repl, template), missing
+
+
 def render_body(
     f: Finding,
     *,
@@ -132,14 +195,17 @@ def render_body(
     anchor = _github_anchor(f"{f.id}: {f.title}") if f.id else ""
     deep_link = f"{report_url}#{anchor}" if anchor else report_url
     fields = {
-        # Attacker-influenced Finding fields — sanitized (CWE-79).
-        "TITLE": _sanitize_for_issue_body(f.title or "(untitled)"),
-        "CWE": _sanitize_for_issue_body(f.cwe or "Unknown CWE"),
-        "CWE_NAME": _sanitize_for_issue_body(f.cwe_name or "(unspecified)"),
-        "SEVERITY": _sanitize_for_issue_body(f.severity or "Unspecified"),
-        "LOCATION": _sanitize_for_issue_body(f.location or "(not specified)"),
-        "ROOT_CAUSE": _sanitize_for_issue_body(f.root_cause or "(not specified)"),
-        "DATA_FLOW": _sanitize_for_issue_body(f.data_flow or "(not specified)"),
+        # Attacker-influenced Finding fields — sanitized (CWE-79). Fields that
+        # render on a single line (``### Finding`` table cells, the H2 title,
+        # the Priority line) also get '|' escaped and CR/LF collapsed.
+        "TITLE": _sanitize_inline(f.title or "(untitled)"),
+        "CWE": _sanitize_inline(f.cwe or "Unknown CWE"),
+        "CWE_NAME": _sanitize_inline(f.cwe_name or "(unspecified)"),
+        "SEVERITY": _sanitize_inline(f.severity or "Unspecified"),
+        "LOCATION": _sanitize_inline(f.location or "(not specified)"),
+        "ROOT_CAUSE": _sanitize_inline(f.root_cause or "(not specified)"),
+        "DATA_FLOW": _sanitize_inline(f.data_flow or "(not specified)"),
+        # Free-form multi-line sections — newlines are legitimate here.
         "EXPLOIT_DESCRIPTION": _sanitize_for_issue_body(
             f.exploit_description or "(not specified in report)"
         ),
@@ -155,25 +221,23 @@ def render_body(
         "SCAN_DATE": report.scan_date,
         "REPORT_URL": deep_link,
         "REPORT_ACCESS_MESSAGE": _report_access_message(),
-        "IDEMPOTENCY_KEY": f.vulnfix_key,
-        "VULN_ID": f.id,
-        "RESULTS_DIR_NAME": report.results_dir_name,
+        # Marker values are NOT html.escaped (keeps the marker regexes exact)
+        # but ARE restricted to a safe identifier charset so an attacker-
+        # influenced value cannot break out of the HTML comment (CWE-116).
+        "IDEMPOTENCY_KEY": _sanitize_marker_value(f.vulnfix_key, "vulnfix-key"),
+        "VULN_ID": _sanitize_marker_value(f.id, "vulnhunt-finding-id"),
+        "RESULTS_DIR_NAME": _sanitize_marker_value(
+            report.results_dir_name, "vulnhunt-results-dir"
+        ),
     }
-    body = template
-    for key, value in fields.items():
-        body = body.replace("{" + key + "}", value)
-    leftovers = _find_placeholders(body)
+    body, leftovers = _substitute(template, fields)
     if leftovers:
         logger.warning(
             "Template placeholders unfilled in rendered body: %s", sorted(leftovers)
         )
     return body
 
 
-def _find_placeholders(body: str) -> set[str]:
-    return set(re.findall(r"\{([A-Z_]+)\}", body))
-
-
 @dataclass(frozen=True)
 class CleanScanContext:
     """Values needed to render a clean-scan issue or comment.
@@ -251,10 +315,7 @@ def _render_clean_scan(ctx: CleanScanContext, template_path: Path) -> str:
         "SKILL_VERSION": ctx.skill_version or "unknown",
         "REPORT_URL_LINE": report_line,
     }
-    body = template
-    for key, value in fields.items():
-        body = body.replace("{" + key + "}", value)
-    leftovers = _find_placeholders(body)
+    body, leftovers = _substitute(template, fields)
     if leftovers:
         logger.warning(
             "Clean-scan template placeholders unfilled: %s", sorted(leftovers)

vulnhunter-agent/tests/test_issues_render.py

@@ -309,3 +309,172 @@ def test_compact_shape(self) -> None:
         assert "abc1234" in comment
         assert "757 seconds" in comment
         assert "2026-07-06T18:42:51Z" in comment
+
+
+class TestMarkerCommentBreakout:
+    """CANON-21: attacker-influenced marker fields must not break out of the
+    HTML-comment footer markers (CWE-79 / CWE-116).
+
+    ``f.id`` and ``report.results_dir_name`` are stamped verbatim into
+    ``<!-- vulnhunt-finding-id: {VULN_ID} -->`` /
+    ``<!-- vulnhunt-results-dir: {RESULTS_DIR_NAME} -->``. Both originate from
+    LLM/scan-README-derived data, so a value containing ``-->`` would close the
+    comment early and inject arbitrary markdown/HTML into the rendered issue.
+    """
+
+    def test_finding_id_cannot_break_out_of_comment(self) -> None:
+        body = render_body(
+            _finding(id="VULN-001 --><script>injected</script><!--"),
+            report=_report(),
+            report_url="https://example/README.md",
+        )
+        # The injected breakout + payload must not appear un-neutralized.
+        assert "--><script>injected</script>" not in body
+        assert "<script>injected</script>" not in body
+        # The finding-id marker line stays a single well-formed comment: exactly
+        # one closing '-->' on that line and no injected content after it.
+        marker_line = next(
+            l for l in body.splitlines() if "vulnhunt-finding-id" in l
+        )
+        assert marker_line.count("-->") == 1
+        assert marker_line.rstrip().endswith("-->")
+
+    def test_results_dir_cannot_break_out_of_comment(self) -> None:
+        body = render_body(
+            _finding(),
+            report=_report(
+                results_dir_name="r_VULNHUNT_RESULTS_x --><b>x</b><!--"
+            ),
+            report_url="https://example/README.md",
+        )
+        assert "--><b>x</b>" not in body
+        assert "<b>x</b>" not in body
+        marker_line = next(
+            l for l in body.splitlines() if "vulnhunt-results-dir" in l
+        )
+        assert marker_line.count("-->") == 1
+        assert marker_line.rstrip().endswith("-->")
+
+    def test_benign_finding_id_still_survives(self) -> None:
+        # Neutralization must not corrupt a legitimate VULN-NNN id so the
+        # downstream ``vulnhunt-finding-id: (VULN-\d{3})`` parser still matches.
+        body = render_body(
+            _finding(id="VULN-042"),
+            report=_report(),
+            report_url="https://example/README.md",
+        )
+        assert "<!-- vulnhunt-finding-id: VULN-042 -->" in body
+
+
+_UNESCAPED_PIPE = re.compile(r"(?<!\\)\|")
+
+
+def _finding_table_rows(body: str) -> list[list[str]]:
+    """Return the cells of each data row of the '### Finding' table."""
+    lines = body.splitlines()
+    start = lines.index("| Field | Value |")
+    rows = []
+    for line in lines[start + 2:]:
+        if not line.strip():
+            break
+        rows.append([c.strip() for c in _UNESCAPED_PIPE.split(line.strip()[1:-1])])
+    return rows
+
+
+class TestFindingTableCellInjection:
+    """PR #9 review: Finding fields rendered inside the '### Finding' table
+    (CWE, CWE name, severity, location, root cause, data flow) must not add
+    cells via '|' or break out of the table / inject headings via CR/LF."""
+
+    @pytest.mark.parametrize("field", [
+        "cwe", "cwe_name", "severity", "location", "root_cause", "data_flow",
+    ])
+    def test_pipe_cannot_forge_cells(self, field: str) -> None:
+        body = render_body(
+            _finding(**{field: "a.py:1 | **Severity** | Low"}),
+            report=_report(),
+            report_url="https://example/README.md",
+        )
+        rows = _finding_table_rows(body)
+        assert len(rows) == 5
+        assert all(len(r) == 2 for r in rows), rows
+
+    @pytest.mark.parametrize("field", [
+        "cwe", "cwe_name", "severity", "location", "root_cause", "data_flow", "title",
+    ])
+    def test_newline_cannot_inject_heading(self, field: str) -> None:
+        body = render_body(
+            _finding(**{field: "x\n\n## Fixed — close this issue\r\n| a | b |"}),
+            report=_report(),
+            report_url="https://example/README.md",
+        )
+        assert not any(l.startswith("## Fixed") for l in body.splitlines())
+        rows = _finding_table_rows(body)
+        assert len(rows) == 5
+        assert all(len(r) == 2 for r in rows), rows
+
+    def test_multiline_free_form_sections_keep_newlines(self) -> None:
+        body = render_body(
+            _finding(
+                exploit_description="Step one.\n\nStep two.",
+                fix_strategy="1. Parameterize.\n2. Add a test.",
+            ),
+            report=_report(),
+            report_url="https://example/README.md",
+        )
+        assert "Step one.\n\nStep two." in body
+        assert "1. Parameterize.\n2. Add a test." in body
+
+    def test_legit_location_unchanged(self) -> None:
+        body = render_body(_finding(), report=_report(),
+                           report_url="https://example/README.md")
+        assert "| **Location** | src/db.py:42 |" in body
+
+
+class TestSinglePassSubstitution:
+    """PR #9 review nit: placeholders were replaced field-by-field, so a value
+    containing '{IDEMPOTENCY_KEY}' etc. was expanded by a later pass."""
+
+    def test_placeholder_text_in_value_is_not_expanded(self) -> None:
+        finding = _finding(
+            root_cause="see {IDEMPOTENCY_KEY} and {REPORT_URL}",
+            title="{VULN_ID}",
+        )
+        body = render_body(finding, report=_report(),
+                           report_url="https://example/README.md")
+        assert "see {IDEMPOTENCY_KEY} and {REPORT_URL}" in body
+        assert body.count(finding.vulnfix_key) == 1  # only the footer marker
+        assert "## Security Finding: CWE-89 — {VULN_ID}" in body
+
+    def test_placeholder_text_in_value_does_not_warn(self, caplog) -> None:
+        with caplog.at_level("WARNING", logger="agent.issues_render"):
+            render_body(_finding(root_cause="{NOT_A_FIELD}"), report=_report(),
+                        report_url="https://example/README.md")
+        assert "unfilled" not in caplog.text
+
+    def test_clean_scan_value_is_not_expanded(self) -> None:
+        body = render_clean_scan_body(_clean_scan_ctx(repo_slug="o/{SKILL_VERSION}"))
+        assert "o/{SKILL_VERSION}" in body
+        assert "o/3500d0c-clean" not in body  # never expanded by a later pass
+
+
+class TestMarkerSanitizeWarns:
+    """PR #9 review: _sanitize_marker_value silently stripped out-of-charset
+    chars, which can yield a marker pointing at a different results dir."""
+
+    def test_changed_marker_value_logs_warning(self, caplog) -> None:
+        with caplog.at_level("WARNING", logger="agent.issues_render"):
+            body = render_body(
+                _finding(),
+                report=_report(results_dir_name="r_VULNHUNT_RESULTS_a b/../c"),
+                report_url="https://example/README.md",
+            )
+        assert "<!-- vulnhunt-results-dir: r_VULNHUNT_RESULTS_ab..c -->" in body
+        assert "vulnhunt-results-dir" in caplog.text
+        assert "r_VULNHUNT_RESULTS_a b/../c" in caplog.text
+
+    def test_clean_marker_values_do_not_warn(self, caplog) -> None:
+        with caplog.at_level("WARNING", logger="agent.issues_render"):
+            render_body(_finding(), report=_report(),
+                        report_url="https://example/README.md")
+        assert caplog.text == ""

vulnhunter-fix/references/verification-table-rules.md

@@ -18,6 +18,12 @@ via `render_verification_table`), workers MUST emit these exact 9 headers
 in this order — any drift (renaming, dropping `#`, adding a `Sink`
 column) is rejected at delivery time by `scripts/validate-verification.py`.
 
+Every row MUST have exactly nine cells. A literal `|` inside a cell is
+written `\|` (GFM's in-cell escape) and a cell never contains a line break;
+`render_verification_table` does both via `_escape_table_cell`. The
+validator splits only on unescaped pipes and rejects (exit 1, naming the row)
+any row whose cell count differs from the header — rows are never skipped.
+
 ## Cell values (REQ-GRA-012)
 
 Each cell is one of the following literal strings: