From 0aeeaa9d4ec2d1c4b2d2e8aa06a6588259d47984 Mon Sep 17 00:00:00 2001 From: ARSH871-bot Date: Thu, 8 Oct 2026 17:33:03 +1300 Subject: [PATCH] The CSV report says what conversion left out, like the other surfaces (#308) Measured on main: a PF Sense upload whose DHCP WAN rule could not be converted showed the skip on the dashboard (X-Netwise-Conversion-Gap-Count) and in the HTML report, while the CSV of the same scan had no trace of it. Every row read as a complete account of a firewall with a rule left out, in the export most likely to be forwarded. render_csv() now writes an excluded_during_conversion column on every row, from the same coverage.summarise() the HTML uses: the skip notes, UNKNOWN when the record cannot be read, or empty when nothing was excluded. It is kept out of CSV_COLUMNS because other renderers build their header from that tuple and their rows by hand. tests/test_coverage_on_every_surface.py is #308's item 2: one upload through the real converter, three surfaces, three states (excluded, nothing excluded, unreadable). Its docstring says the surfaces are enumerated, not discovered, as asked on the issue. --- analysis/report.py | 35 +++++- tests/test_coverage_on_every_surface.py | 140 ++++++++++++++++++++++++ tests/test_report_export.py | 4 +- web/main.py | 4 +- 4 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 tests/test_coverage_on_every_surface.py diff --git a/analysis/report.py b/analysis/report.py index c56c335..0abc45d 100644 --- a/analysis/report.py +++ b/analysis/report.py @@ -64,6 +64,15 @@ "summary", "detail", "source", "nothing_to_check", "report_subject") +#: Columns about the WHOLE REPORT rather than one finding, written by +#: `render_csv()` after CSV_COLUMNS on every row (#308). +#: +#: KEPT OUT OF CSV_COLUMNS ON PURPOSE. Other renderers build their header from +#: CSV_COLUMNS and their rows by hand (#298's comparison CSV does), so adding +#: a column there would leave their header one wider than their rows -- a +#: misalignment no merge conflict would show. +CSV_REPORT_COLUMNS = ("excluded_during_conversion",) + #: The `device` value a check uses when a finding is ABOUT THE CHECK rather #: than about any device. Today exactly one producer emits it -- #: `analysis/checks/policy_compliance.py`, for "your policy has no rules for @@ -142,7 +151,8 @@ def _now() -> str: def render_csv(findings: Sequence[Dict[str, Any]], - source: Optional[str] = None) -> str: + source: Optional[str] = None, + conversion_gaps: Optional[Sequence[str]] = ()) -> str: """Every finding, one row each, including the ones that could not run. A `status` COLUMN rather than only the rows that found something -- an @@ -172,10 +182,30 @@ def render_csv(findings: Sequence[Dict[str, Any]], Optional, defaulting to None, so every existing caller keeps working; when it is absent the column is present and empty, which is the honest rendering of "this report does not say". + + `excluded_during_conversion` -- WHAT A CONVERTER LEFT OUT (#308) + Measured on main before this existed: a PF Sense upload whose DHCP + WAN rule could not be converted showed the skip on the dashboard and + in the HTML report, and the CSV of the same scan said nothing. Every + row read as complete. Same three states as `render_html()`, from the + same `coverage.summarise()`, so the two cannot disagree: + + the skip notes, joined something was excluded + "UNKNOWN -- ..." the record could not be read + empty nothing was excluded + + Repeated on every row for the reason `report_subject` is. """ buffer = io.StringIO() writer = csv.writer(buffer, lineterminator="\n") - writer.writerow(CSV_COLUMNS) + writer.writerow(CSV_COLUMNS + CSV_REPORT_COLUMNS) + + summary = coverage.summarise(findings, conversion_gaps) + if summary["conversion_gaps_unreadable"]: + excluded = ("UNKNOWN -- the record of what conversion excluded could " + "not be read, so whether anything was left out is not known") + else: + excluded = "; ".join(summary["conversion_gaps"]) ordered = (_sections(findings)["blind"] + _sections(findings)["problems"] @@ -199,6 +229,7 @@ def render_csv(findings: Sequence[Dict[str, Any]], # spreadsheet, which is the point of having it at all. "yes" if is_nothing_to_check(finding) else "", source or "", + excluded, ]) return buffer.getvalue() diff --git a/tests/test_coverage_on_every_surface.py b/tests/test_coverage_on_every_surface.py new file mode 100644 index 0000000..36b69f3 --- /dev/null +++ b/tests/test_coverage_on_every_surface.py @@ -0,0 +1,140 @@ +"""A coverage statement must reach every surface that shows findings (#308). + +THE RULE, AS AGREED ON #308 + "A coverage statement must appear on every surface that renders + findings. A surface that cannot carry it must say so, not omit it." + (@SamikaPerera's wording, which builds F-4's asymmetry in: the silent + surface is the one that looks complete.) + +WHAT WAS MEASURED BEFORE THIS TEST EXISTED, on main (`fa568f7`), 8 October + A PF Sense upload whose DHCP WAN rule could not be converted: + + dashboard X-Netwise-Conversion-Gap-Count: 1 says so + HTML "Excluded during conversion ... wan" says so + CSV three could-not-check rows, nothing SILENT + + The CSV is the export most likely to be forwarded, and it read as a + complete account of a firewall that had a rule left out. + +THE SURFACES ARE ENUMERATED, NOT DISCOVERED + This test knows three surfaces: `/api/findings` (whose conversion facts + travel in response headers that `app.js` renders, pinned by + tests/js/conversion_gaps_notice_harness.js), the HTML report and the + CSV report. A fourth surface added later will not be caught here unless + someone adds it to SURFACES. @SamikaPerera asked for that to be said + rather than implied, and it is the honest limit of this test. + +NO BATFISH + The upload, the real PF Sense converter and the real skip record are all + used. Only `pipeline._analyse` is replaced, with canned findings, because + what is under test is whether a fact about coverage reaches each surface + -- not what the checks find. +""" + +import html + +import pytest +from fastapi.testclient import TestClient + +from analysis import findings, pipeline +from web import main + +# One modellable interface (lan) and one that is not (wan, DHCP): the +# client's real shape, and the converter skips exactly one rule for it. +DHCP_WAN_XML = b""" + + skip-device + + em0 + em110.0.0.124 + + + passwantcp + + passlantcp + + + +""" + +CISCO = b"hostname rtr-us5\n!\ninterface GigabitEthernet0/0\n ip address 10.10.10.1 255.255.255.0\n" + +#: Every check ran clean: the case where a silent surface does the most harm, +#: because nothing else on it hints that anything was left out. +CLEAN = [ + findings.make_finding(check=check, severity="low", device="skip-device", + summary=f"No issues found by {check}", detail="ran clean", + source="test", status="none", number=0) + for check in ("access_control", "policy_compliance", "routing") +] + + +@pytest.fixture +def client(tmp_path, monkeypatch): + monkeypatch.setattr(main, "SESSIONS_ROOT", tmp_path / "sessions") + monkeypatch.setattr(pipeline, "_analyse", lambda *a, **k: [dict(f) for f in CLEAN]) + return TestClient(main.app) + + +def _dashboard(client): + response = client.get("/api/findings") + assert response.status_code == 200 + return { + "count": response.headers.get("x-netwise-conversion-gap-count"), + "unreadable": response.headers.get("x-netwise-conversion-gaps-unreadable"), + } + + +def _html(client): + return client.get("/api/report?format=html").text + + +def _csv_column(client): + import csv + import io + + rows = list(csv.DictReader(io.StringIO(client.get("/api/report?format=csv").text))) + assert rows, "the CSV has no rows to carry anything" + assert "excluded_during_conversion" in rows[0], "the CSV has no column for it" + values = {row["excluded_during_conversion"] for row in rows} + assert len(values) == 1, f"rows disagree about what was excluded: {values}" + return values.pop() + + +SURFACES = ("dashboard", "html", "csv") + + +def test_a_skipped_rule_is_named_on_every_surface(client): + upload = client.post("/api/upload", files={"file": ("config.xml", DHCP_WAN_XML, "application/xml")}) + (note,) = upload.json()["skipped"] + assert "wan" in note + + assert _dashboard(client) == {"count": "1", "unreadable": "false"} + assert html.escape(note) in _html(client) # the report escapes every value + assert _csv_column(client) == note + + +def test_nothing_excluded_is_claimed_on_no_surface(client): + """The other direction: a plain upload must not acquire a gap anywhere.""" + client.post("/api/upload", files={"file": ("rtr.cfg", CISCO, "text/plain")}) + + assert _dashboard(client) == {"count": "0", "unreadable": "false"} + assert "Excluded during conversion" not in _html(client) + assert _csv_column(client) == "" + + +def test_an_unreadable_record_is_unknown_on_every_surface(client): + """A record that exists and cannot be read is "we cannot tell", never + "nothing was excluded" (#302 review). Every surface must say the same.""" + client.post("/api/upload", files={"file": ("config.xml", DHCP_WAN_XML, "application/xml")}) + main.pfsense_skips_path().write_text("{truncated", encoding="utf-8") + + assert _dashboard(client)["unreadable"] == "true" + assert "could not be read" in _html(client) + assert _csv_column(client).startswith("UNKNOWN") + + +def test_every_surface_is_listed(): + """A deliberately dumb guard: if someone removes a surface from this file, + the docstring's claim of three stops being true without anything failing.""" + assert SURFACES == ("dashboard", "html", "csv") diff --git a/tests/test_report_export.py b/tests/test_report_export.py index 35d9a6d..f7ede00 100644 --- a/tests/test_report_export.py +++ b/tests/test_report_export.py @@ -577,4 +577,6 @@ def test_no_dashboard_only_text_leaks_into_either_format(monkeypatch): "_attach_remediation() for why that is still an open decision, " "not one this test should silently start allowing" ) - assert set(_rows(csv_body)[0]) == set(report.CSV_COLUMNS) + # The documented header and nothing else: per-finding columns plus the + # report-level ones (#308's excluded_during_conversion). + assert set(_rows(csv_body)[0]) == set(report.CSV_COLUMNS + report.CSV_REPORT_COLUMNS) diff --git a/web/main.py b/web/main.py index 084d4d1..88ce858 100644 --- a/web/main.py +++ b/web/main.py @@ -1175,7 +1175,9 @@ def download_report(format: str = "html") -> Response: results, source=subject, conversion_gaps=_staged_pfsense_skips() ) if format == "html" - else report.render_csv(results, source=subject) + else report.render_csv( + results, source=subject, conversion_gaps=_staged_pfsense_skips() + ) ) stamp = datetime.now(timezone.utc).strftime("%Y%m%d-%H%M") return Response(