diff --git a/strix/report/state.py b/strix/report/state.py index a665adea..e043f088 100644 --- a/strix/report/state.py +++ b/strix/report/state.py @@ -1,5 +1,6 @@ import json import logging +import re import subprocess import threading from collections.abc import Callable @@ -32,6 +33,8 @@ logger = logging.getLogger(__name__) _global_report_state: Optional["ReportState"] = None +_CONTROL_CHARS = re.compile(r"[\x00-\x1f\x7f]+") + def _strix_version() -> str | None: """Best-effort package version for the SARIF tool.driver.version field.""" @@ -41,6 +44,17 @@ def _strix_version() -> str | None: return None +def _clean_title(title: str) -> str: + """Return a single-line finding title. + + A title quotes text from the scanned target, so it can carry newlines, tabs or + other control characters. Those break every artifact that renders the title on + one line, such as the markdown heading, the CSV cell and the TUI list. Control + characters become spaces and runs of whitespace collapse to one space. + """ + return " ".join(_CONTROL_CHARS.sub(" ", title).split()) + + def _number(value: Any) -> int | float: try: return float(value or 0) @@ -266,7 +280,7 @@ class ReportState: report: dict[str, Any] = { "id": report_id, - "title": title.strip(), + "title": _clean_title(title), "severity": severity.lower().strip(), "timestamp": datetime.now(UTC).strftime("%Y-%m-%d %H:%M:%S UTC"), } diff --git a/strix/report/writer.py b/strix/report/writer.py index b5f65b45..47184d39 100644 --- a/strix/report/writer.py +++ b/strix/report/writer.py @@ -42,8 +42,10 @@ def csv_safe(value: object) -> str: scanned target, which is exactly the attacker-influenced input this guards against. - Prefixing with an apostrophe is the standard mitigation: spreadsheets treat - the rest of the cell as literal text and hide the apostrophe on display. + Prefixing with an apostrophe is the standard mitigation (OWASP): the rest of + the cell is kept as literal text instead of being evaluated. Excel shows the + apostrophe when it opens a ``.csv`` directly, which is cosmetic — the point is + that nothing runs. """ text = str(value) if text.startswith(_CSV_FORMULA_PREFIXES): @@ -197,11 +199,17 @@ def write_vulnerabilities( def atomic_write_text(path: Path, payload: str) -> None: - """Write *payload* to *path* via a sibling temp file and an atomic rename.""" + """Write *payload* to *path* via a sibling temp file and an atomic rename. + + ``newline=""`` disables newline translation so *payload* lands byte-for-byte: + the CSV index carries its own ``\\r\\n`` terminators, which text mode would turn + into ``\\r\\r\\n`` on Windows. + """ path.parent.mkdir(parents=True, exist_ok=True) with tempfile.NamedTemporaryFile( mode="w", encoding="utf-8", + newline="", dir=str(path.parent), prefix=f".{path.name}.", suffix=".tmp", diff --git a/tests/test_list_reports.py b/tests/test_list_reports.py index 95025ac3..8fb90f9d 100644 --- a/tests/test_list_reports.py +++ b/tests/test_list_reports.py @@ -27,6 +27,20 @@ def report_state(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> ReportState return state +def test_add_vulnerability_report_strips_control_chars_from_title( + report_state: ReportState, +) -> None: + # A title quotes text from the scanned target, so it can carry newlines or + # tabs that break the markdown heading, the CSV cell and the TUI list. + report_id = report_state.add_vulnerability_report( + title="\tXSS in\r\n search\x00 form ", + severity="medium", + target="https://app.example.com", + ) + report = next(r for r in report_state.vulnerability_reports if r["id"] == report_id) + assert report["title"] == "XSS in search form" + + def _seed(state: ReportState) -> None: state.add_vulnerability_report( title="Reflected XSS in search", diff --git a/tests/test_report_writer.py b/tests/test_report_writer.py index 25ad3de6..9b849010 100644 --- a/tests/test_report_writer.py +++ b/tests/test_report_writer.py @@ -9,6 +9,7 @@ from typing import TYPE_CHECKING, Any import pytest from strix.report.writer import ( + atomic_write_text, read_run_record, render_vulnerability_md, write_executive_report, @@ -210,6 +211,17 @@ def test_write_vulnerabilities_csv_leaves_benign_titles_unchanged(tmp_path: Path assert csv_rows[0]["title"] == "SQL Injection in /login" +def test_atomic_write_text_keeps_payload_byte_for_byte(tmp_path: Path) -> None: + # The CSV index carries its own \r\n terminators, so newline translation would + # turn every row ending into \r\r\n on Windows. + payload = "a,b\r\nc,d\r\n" + path = tmp_path / "index.csv" + + atomic_write_text(path, payload) + + assert path.read_bytes() == payload.encode("utf-8") + + def test_write_vulnerabilities_skips_already_saved_ids(tmp_path: Path) -> None: reports = [_sample_report(id="vuln-0001")] saved: set[str] = {"vuln-0001"}