fix(report): a tally is not a verdict, and "1 device(s)" is not a sentence

netdiff/audit.py

@@ -96,6 +96,22 @@ class Finding:
     verify: str
 
 
+# `4 open port(s)`. A rule cannot know its own count, so the templates carry the
+# marker and this resolves it where the count is finally known. Worth the three
+# lines because the alternative is a report whose whole claim is that the
+# sentences are the product, shipping `1 device(s) are reachable`.
+#
+# Deliberately narrow: a number, then at most four lowercase words, then the
+# marker. Digits cannot appear inside the noun phrase, so `offers 3 deprecated
+# algorithm(s)` binds to the 3 and not to the port number earlier in the line.
+_COUNTED = re.compile(r"(\d+) ((?:[a-z-]+ ){0,3}[a-z-]+)\(s\)")
+
+
+def pluralise(text: str) -> str:
+    """`3 open port(s)` -> `3 open ports`, `1 open port(s)` -> `1 open port`."""
+    return _COUNTED.sub(lambda m: f"{m[1]} {m[2]}" + ("" if m[1] == "1" else "s"), text)
+
+
 def headline(finding) -> str:
     """The title, prefixed with the device when the title does not name it.
 
@@ -544,7 +560,7 @@ def headline(finding) -> str:
     },
     "here-client-isolation-off": {
         "severity": "info",
-        "title": "{count} other device(s) on this network are reachable from here",
+        "title": "{count} other device(s) on this network can be reached from here",
         "why": (
             "Not a problem by itself, and the normal case for a home network - it "
             "is what lets your laptop print. Worth knowing on a network you do not "
@@ -591,7 +607,7 @@ def headline(finding) -> str:
     },
     "here-own-ports-exposed": {
         "severity": "medium",
-        "title": "{count} service(s) on this machine are bound to the network, not loopback",
+        "title": "this machine offers {count} service(s) to the network, not just to itself",
         "why": (
             "These are your ports, not somebody else's. A service bound only to "
             "127.0.0.1 cannot be reached from the network at all; these answered on "
@@ -624,7 +640,7 @@ def headline(finding) -> str:
     },
     "open-ports-noted": {
         "severity": "info",
-        "title": "{count} open port(s) observed, and not reported as problems",
+        "title": "{count} open port(s) observed here, and none reported as a problem",
         "why": (
             "An open port means a service accepted a TCP handshake. That is not a "
             "vulnerability - it is what a working device looks like. Tools that list "
@@ -651,15 +667,18 @@ def finding(rule: str, device: str, evidence: str, **context) -> Finding:
     """
     spec = RULES[rule]
     fields = dict(context, device=device)
+    # The count is known here and nowhere earlier, so this is where `port(s)`
+    # stops being a template and becomes a sentence - including in `evidence`,
+    # which the rules build with the same marker.
     return Finding(
         rule=rule,
         severity=spec["severity"],
         device=device,
-        title=spec["title"].format(**fields),
-        evidence=evidence,
-        why=spec["why"].format(**fields),
-        fix=spec["fix"].format(**fields),
-        verify=spec["verify"].format(**fields),
+        title=pluralise(spec["title"].format(**fields)),
+        evidence=pluralise(evidence),
+        why=pluralise(spec["why"].format(**fields)),
+        fix=pluralise(spec["fix"].format(**fields)),
+        verify=pluralise(spec["verify"].format(**fields)),
     )
 
 
@@ -1101,9 +1120,37 @@ def audit(
 
 
 def summarise(findings) -> str:
-    """'1 critical, 2 high' - info is counted but never leads."""
+    """'1 critical, 2 high' - info is counted but never leads.
+
+    A tally, for the terminal. Whoever typed the command has the context to read
+    one, and density is what a terminal line is for. `verdict` is the other
+    audience.
+    """
     counts = {}
     for item in findings:
         counts[item.severity] = counts.get(item.severity, 0) + 1
     parts = [f"{counts[name]} {name}" for name in SEVERITY_ORDER if counts.get(name)]
     return ", ".join(parts) if parts else "nothing to report"
+
+
+# The HTML report goes to someone who did not run the tool and will not run it -
+# a housemate, a landlord, whoever actually administers the router. A tally is
+# the wrong headline for them: "6 info" is two pieces of jargon and no verdict,
+# and it makes "nothing needs doing" and "act today" look like the same kind of
+# statement. So the page leads with the answer and keeps the tally underneath.
+#
+# Graded by severity rather than by rule, deliberately. Tying the critical line
+# to the one rule that currently produces critical findings would read better
+# today and lie the first time a second such rule is added.
+VERDICTS = {
+    "critical": "Something on this network needs attention today",
+    "high": "Something on this network needs fixing",
+    "medium": "Nothing urgent, but a few things are worth changing",
+    "info": "Nothing on this network needs action",
+}
+
+
+def verdict(findings) -> str:
+    """One sentence, for a reader who may not read the second one."""
+    worst = min((f.severity for f in findings), key=SEVERITY_ORDER.get, default=None)
+    return VERDICTS[worst] if worst else "Nothing to report on this network"

netdiff/cli.py

@@ -110,7 +110,11 @@ def cmd_scan(args) -> int:
             )
         )
     else:
-        print(f"scan {scan_id}: {len(devices)} device(s) on {args.subnet}")
+        print(
+            audit_rules.pluralise(
+                f"scan {scan_id}: {len(devices)} device(s) on {args.subnet}"
+            )
+        )
         for device in devices:
             label = device_label(device.hostname, device.vendor, device.services)
             open_ports = (
@@ -246,7 +250,11 @@ def placeholders(text):
             audit_rules.summarise(findings),
         )
         Path(args.html).write_text(page, encoding="utf-8")
-        print(f"wrote {args.html} - {len(findings)} finding(s), open it in a browser")
+        print(
+            audit_rules.pluralise(
+                f"wrote {args.html} - {len(findings)} finding(s), open it in a browser"
+            )
+        )
         return 0
 
     if args.json:
@@ -340,9 +348,17 @@ def cmd_here(args) -> int:
         return 0
 
     print(f"here: {args.subnet} - {audit_rules.summarise(findings)}")
+    resolvers = list(observed["resolvers"])
     print(
-        f"gateway {observed['gateway'] or 'none'}, "
-        f"resolver(s) {', '.join(observed['resolvers']) or 'none'}\n"
+        audit_rules.pluralise(
+            f"gateway {observed['gateway'] or 'none'}, "
+            + (
+                f"{len(resolvers)} resolver(s): {', '.join(resolvers)}"
+                if resolvers
+                else "no resolvers"
+            )
+            + "\n"
+        )
     )
     severity = ""
     for finding in findings:
@@ -370,7 +386,7 @@ def cmd_inventory(args) -> int:
     if args.json:
         print(json.dumps([r | {"ports": list(r["ports"])} for r in rows], indent=2))
         return 0
-    print(f"{len(rows)} device(s) ever seen\n")
+    print(audit_rules.pluralise(f"{len(rows)} device(s) ever seen\n"))
     for row in rows:
         label = device_label(row["hostname"], row["vendor"], row["services"])
         hint = f"  {row['os_hint']}" if row["os_hint"] else ""

netdiff/report.py

@@ -23,7 +23,7 @@
 
 from html import escape
 
-from .audit import SEVERITY_ORDER, headline
+from .audit import SEVERITY_ORDER, headline, verdict
 
 # Deliberately drab. A report that looks like a security product invites the
 # reader to skim the colours instead of the sentences, and the whole thesis here
@@ -32,8 +32,9 @@
 :root { color-scheme: light dark; }
 body { font: 16px/1.6 system-ui, -apple-system, Segoe UI, sans-serif;
        max-width: 46rem; margin: 3rem auto; padding: 0 1.25rem; }
-h1 { font-size: 1.4rem; margin: 0 0 .25rem; }
-.sub { opacity: .7; font-size: .9rem; margin: 0 0 2rem; }
+h1 { font-size: 1.4rem; margin: 0 0 .4rem; }
+.sub { opacity: .7; font-size: .9rem; margin: 0 0 1.2rem; }
+.lede { margin: 0 0 2rem; }
 details { border-top: 1px solid rgba(128,128,128,.3); padding: .7rem 0; }
 details:last-of-type { border-bottom: 1px solid rgba(128,128,128,.3); }
 summary { cursor: pointer; display: flex; gap: .7rem; align-items: baseline; }
@@ -94,18 +95,23 @@ def render(annotated, subnet: str, scan_id: int, started: str, summary: str) ->
         annotated, key=lambda pair: (SEVERITY_ORDER[pair[0].severity], pair[0].device)
     )
     body = "".join(_finding_html(f, is_new) for f, is_new in findings) or (
-        "<p>Nothing to report - no devices answered, or none had open ports.</p>"
+        "<p>No devices answered, or none had open ports.</p>"
     )
+    # The tally is already the whole of the headline when there is nothing to
+    # report, so printing it again underneath just says it twice.
+    tally = f"{esc(summary)} - " if findings else ""
     return (
         "<!doctype html>\n"
         '<html lang="en"><head><meta charset="utf-8">'
         '<meta name="viewport" content="width=device-width, initial-scale=1">'
         f"<title>netdiff audit {esc(scan_id)} - {esc(subnet)}</title>"
         f"<style>{STYLE}</style></head><body>"
-        f"<h1>{esc(subnet)} - {esc(summary)}</h1>"
-        f'<p class="sub">audit {esc(scan_id)}, {esc(started)}. '
-        "Click any finding for the evidence it rests on, why it matters, how to "
-        "fix it, and a command you can run yourself to confirm it.</p>"
+        f"<h1>{esc(verdict([f for f, _ in findings]))}</h1>"
+        f'<p class="sub">{esc(subnet)} - {tally}'
+        f"audit {esc(scan_id)}, {esc(started)}</p>"
+        '<p class="lede">Click any finding for the evidence it rests on, why it '
+        "matters, how to fix it, and a command you can run yourself to confirm "
+        "it.</p>"
         f"{body}"
         "<footer>An open port is not a vulnerability - it is what a working "
         "device looks like. Only the findings above are claims about this "

tests/test_audit.py

@@ -9,7 +9,16 @@
 important half.
 """
 
-from netdiff.audit import RULES, audit, headline, summarise
+from netdiff.audit import (
+    RULES,
+    SEVERITY_ORDER,
+    VERDICTS,
+    audit,
+    headline,
+    pluralise,
+    summarise,
+    verdict,
+)
 from netdiff.probe import Certificate
 from netdiff.scan import Device
 from netdiff.upnp import Gateway, Mapping
@@ -469,6 +478,43 @@ def test_summarise_counts_by_severity_worst_first():
     assert "info" in text
 
 
+def test_a_count_of_one_reads_as_one_thing():
+    """`1 device(s)` is the one place the prose visibly gives up."""
+    assert pluralise("1 open port(s) observed") == "1 open port observed"
+    assert pluralise("4 open port(s) observed") == "4 open ports observed"
+    assert pluralise("0 device(s) ever seen") == "0 devices ever seen"
+
+
+def test_the_marker_binds_to_its_own_count_and_not_an_earlier_number():
+    """`port 22` and `3 algorithms` are in the same sentence - the 3 wins."""
+    out = pluralise("SSH on port 22 still offers 3 deprecated algorithm(s)")
+    assert out == "SSH on port 22 still offers 3 deprecated algorithms"
+
+
+def test_a_real_finding_never_reaches_a_reader_with_the_marker_in_it():
+    """`finding()` is where the count is known, so it is where this has to hold."""
+    for hit in audit([dev(ports=[80, 443])]):
+        for field in (hit.title, hit.evidence, hit.why, hit.fix, hit.verify):
+            assert "(s)" not in field, hit.rule
+
+
+def test_the_verdict_leads_with_what_to_do_not_with_a_tally():
+    """The HTML headline is read by someone who did not run the tool."""
+    device = dev(ip="192.168.1.23", ports=[23, 8080])
+    serious = audit([device], gw([mapping(client="192.168.1.23", internal=8080)]))
+    assert verdict(serious) == "Something on this network needs attention today"
+
+    quiet = [f for f in audit([dev(ports=[443])]) if f.severity == "info"]
+    assert quiet, "expected a quiet network to still produce info findings"
+    assert verdict(quiet) == "Nothing on this network needs action"
+
+
+def test_the_verdict_has_a_sentence_for_every_severity_a_rule_can_carry():
+    """A new severity would otherwise KeyError on the report, not on a test."""
+    assert set(VERDICTS) == set(SEVERITY_ORDER)
+    assert verdict([]) == "Nothing to report on this network"
+
+
 def test_a_headline_names_the_device_it_is_about():
     """Three devices running lighttpd produce three identical lines otherwise."""
     banners = {("192.168.1.10", 80): "HTTP/1.0 200\r\nServer: lighttpd/1.4.35\r\n"}

tests/test_report.py

@@ -117,6 +117,18 @@ def test_an_empty_audit_says_so_rather_than_rendering_a_blank_page():
     assert "<details>" not in out
 
 
+def test_the_headline_is_a_verdict_and_the_tally_is_demoted():
+    """`192.168.1.0/24 - 6 info` is jargon twice over to whoever this is sent to."""
+    out = page((finding(severity="info"), False), summary="6 info")
+    assert "<h1>Nothing on this network needs action</h1>" in out
+    assert "6 info" in out.split("</h1>")[1], "the tally still exists, one line down"
+
+
+def test_a_serious_finding_changes_the_headline_not_just_a_number():
+    critical = page((finding(severity="critical"), False), summary="1 critical")
+    assert "needs attention today" in critical.split("</h1>")[0]
+
+
 def test_the_footer_repeats_that_an_open_port_is_not_a_vulnerability():
     """The thesis has to survive the trip to whoever the file gets sent to."""
     assert "not a vulnerability" in page((finding(), False))