From 834fbcaca1aaf904b65001465ec2ddcd9f090e6a Mon Sep 17 00:00:00 2001 From: patelankeet2 Date: Sat, 5 Sep 2026 20:17:00 +1200 Subject: [PATCH 1/4] pfsense: one bad rule no longer sinks the whole file (#78 item 4) _rule_to_acl_line() failures (missing/unsupported , unsupported value, an unresolved alias in address or port, an out-of-range port, an unresolvable network reference) used to abort conversion of the entire file, not just the interface or rule they were on. At the client's real scale (1,998 XML elements against this project's 54-element fixture), one anomaly anywhere meant zero analysis of anything. Re-scoped the same way #104 already re-scoped an unmodellable interface: the one failing rule is excluded and named in `skipped` with its precise real reason, and everything else -- every other rule on the same interface, every other interface -- converts exactly as if it were never there. If every rule on a role fails, the role's ACL ends up empty, which already produces the existing fail-closed `deny ip any any`; that required fixing convert()'s own deny-all fallback, which checked `role in acl_lines_by_role` (membership) rather than `acl_lines_by_role.get(role)` (truthiness) -- before this change a role was never a key with an empty value, so the distinction never mattered until now. Left unfixed it would have emitted an ACL with no lines in it at all, not even a fail-closed deny -- fail OPEN instead of closed. Missing gets its own precise REFUSALS message, split from "present but invalid" the same way #104 split the DHCP-WAN and undeclared-VPN interface messages -- both used to produce the identical confusing "...is not pass, block or reject: is None" string. This does NOT touch what any field is interpreted to mean. Every REFUSALS message still names the same cause it always did; only the blast radius shrinks from "the whole file" to "this one rule". In particular 's existing default-to-"any" behaviour is untouched -- that is issue #80's own open question, explicitly blocked on the client ("do not fix this before that comes back"), and not this change's to answer. Updated the 6 existing tests whose expectation flips from "raises" to "skips and continues" (each fixture has exactly one rule on one interface, so with nothing else to say the interface now gets a fail-closed deny-all instead of the whole conversion aborting). Added a test for missing specifically (zero coverage before this), a test proving two rules failing for different reasons get separate notes rather than one merged one, and a combined end-to-end test matching the client's actual shape (DHCP WAN + undeclared VPN + NAT + missing type, all in one file, all named independently, the one good rule still converting) -- no such combined fixture existed before. Mutation-tested both new guards by hand: reverted the per-rule try/except to a bare list comprehension and confirmed the 8 relevant tests fail by raising again; reverted the deny-all fallback to membership-based and confirmed the same 8 tests fail by the ACL body coming out empty instead of fail-closed. Restored both, full suite green: 1257 passed, 8 skipped. ruff clean. Co-Authored-By: Claude Sonnet 5 --- analysis/pfsense_convert.py | 74 +++++++++++++-- tests/test_pfsense_convert.py | 164 ++++++++++++++++++++++++++++------ 2 files changed, 201 insertions(+), 37 deletions(-) diff --git a/analysis/pfsense_convert.py b/analysis/pfsense_convert.py index f42f2a8..ff5184d 100644 --- a/analysis/pfsense_convert.py +++ b/analysis/pfsense_convert.py @@ -195,6 +195,11 @@ "likely a named alias group" ), "unsupported_rule_type": "a rule's is not pass, block or reject", + "missing_rule_type": ( + "a rule's element is absent -- cannot tell whether it is " + "meant to pass, block or reject, and guessing either direction " + "would misstate what the rule does" + ), "unsupported_protocol": ( "a rule's is not tcp, udp, icmp or any -- combined forms " "like 'tcp/udp' need two Cisco ACL lines and are not handled" @@ -730,7 +735,13 @@ def _check_rule_order_is_unambiguous( def _rule_to_acl_line(rule_el: ET.Element, interfaces: Dict[str, Dict[str, str]]) -> str: """Convert one PF Sense element into one Cisco extended-ACL line.""" pf_type = _text(rule_el, "type") - action = _PFSENSE_TO_CISCO_ACTION.get(pf_type or "") + # Absent gets its own message, distinct from present-but-invalid (#78 + # item 4) -- " is None" was the same confusing string either way, + # and the two are different questions: one is a value this module does + # not recognise, the other is a value that was never written at all. + if pf_type is None: + raise PfSenseConversionError(REFUSALS["missing_rule_type"]) + action = _PFSENSE_TO_CISCO_ACTION.get(pf_type, None) if action is None: raise PfSenseConversionError( f"{REFUSALS['unsupported_rule_type']}: is {pf_type!r}" @@ -974,15 +985,52 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult: # module still refuses rather than guesses, because the general case # -- some rules quick, some not, in no particular pattern -- does not # reduce to a single reordering the way the all-or-nothing cases do. + # SKIPPED, NOT REFUSED (#78 item 4, re-scoped the same way item 1/2 were + # in #104): a rule that _rule_to_acl_line() cannot convert -- missing or + # unsupported , an unsupported value, a named alias + # where an address or port is expected, an out-of-range port, an + # unresolvable network reference -- used to abort THIS ENTIRE FILE, not + # just the interface it was on or the rule itself. At the client's real + # scale (1,998 XML elements against this project's 54-element fixture, + # per CLAUDE.md section 7), one anomaly anywhere meant zero analysis of + # anything. That is the same shape #104 fixed for an unmodellable + # interface, just one level deeper -- so it gets the same fix: exclude + # the one rule, name the real reason in `skipped`, convert everything + # else exactly as if it were never there. + # + # This changes NOTHING about what any field is interpreted to mean -- + # every REFUSALS message below still names the same cause it always did. + # It only shrinks the blast radius of that cause from "the whole file" + # to "this one rule". In particular this does NOT touch 's + # existing default-to-"any" behaviour, which is issue #80's own open + # question (blocked on the client) and is not this change's to answer. acl_lines_by_role: Dict[str, List[str]] = {} for role, rules in rules_by_role.items(): - if not any(_is_quick(r) for r in rules): - rules = list(reversed(rules)) - acl_lines_by_role[role] = [_rule_to_acl_line(r, interfaces) for r in rules] - continue - lines_for_role = [_rule_to_acl_line(r, interfaces) for r in rules] - _check_rule_order_is_unambiguous(rules, interfaces, lines_for_role) - acl_lines_by_role[role] = lines_for_role + no_quick_rules_at_all = not any(_is_quick(r) for r in rules) + # Position in a skip note is counted from ORIGINAL document order, + # before any reversal -- so the number matches what a human reading + # the XML top to bottom would count, not this function's internal + # processing order. + original_order = rules + ordered = list(reversed(rules)) if no_quick_rules_at_all else rules + + kept_rules: List[ET.Element] = [] + kept_lines: List[str] = [] + for r in ordered: + try: + kept_lines.append(_rule_to_acl_line(r, interfaces)) + except PfSenseConversionError as error: + position = original_order.index(r) + 1 + skipped.append( + f"rule {position} of {len(original_order)} on {role!r} " + f"could not be converted: {error} -- skipped, not converted" + ) + continue + kept_rules.append(r) + + if not no_quick_rules_at_all: + _check_rule_order_is_unambiguous(kept_rules, interfaces, kept_lines) + acl_lines_by_role[role] = kept_lines lines: List[str] = [f"hostname {hostname}", "!"] for role, info in interfaces.items(): @@ -1004,7 +1052,15 @@ def convert(xml_path: Union[str, Path]) -> ConversionResult: for role in interfaces: acl_name = _acl_name(role, rule_bearing_roles=modelled_roles) lines.append(f"ip access-list extended {acl_name}") - if role in acl_lines_by_role: + # TRUTHINESS, NOT MEMBERSHIP (#78 item 4). A role can now be a key in + # acl_lines_by_role with an EMPTY list -- every one of its rules + # failed to convert and was skipped -- which used to be impossible: + # before the per-rule skip above, a role either had every rule + # convert or the whole file aborted, so a present key always meant + # real content. `role in acl_lines_by_role` alone would then emit an + # ACL with no lines in it at all, not even a fail-closed deny -- the + # exact fail-OPEN inversion the deny-all fallback exists to prevent. + if acl_lines_by_role.get(role): lines.extend(f" {line}" for line in acl_lines_by_role[role]) else: lines.append(" deny ip any any") diff --git a/tests/test_pfsense_convert.py b/tests/test_pfsense_convert.py index 0802429..ba5a003 100644 --- a/tests/test_pfsense_convert.py +++ b/tests/test_pfsense_convert.py @@ -364,6 +364,43 @@ def test_a_nat_generated_rule_alongside_an_unmodellable_interface_gets_its_own_n assert "NAT-generated" in joined or "associated-rule-id" in joined +def test_the_clients_actual_combined_shape_converts_with_every_anomaly_named(): + """#78's real shape, all at once, in one file -- a DHCP WAN, an + undeclared VPN role, a NAT-generated rule, and a rule missing , + alongside a genuinely good LAN rule. No fixture combining all of these + existed before this test (confirmed: every prior test above builds one + or two anomalies at a time). This is the "both halves tested, the join + between them is not" gap CLAUDE.md section 11 warns about, closed for + this specific combination -- four independent reasons for four + independent exclusions, the good rule still converts, and one + anomaly's message never mentions another's cause.""" + xml_text = _minimal_xml( + interfaces_xml=_dhcp_wan_and_lan_interfaces(), + rules_xml=( + _rule_on("wan") + + _rule_on("WireGuard") + + _nat_generated_rule_on("lan") + + "lantcp" + "" + + _ONE_BENIGN_LAN_RULE + ), + ) + result = _convert_string_full(xml_text) + + assert len(result.skipped) == 4 + joined = " | ".join(result.skipped) + assert "wan" in joined and "no static address configured" in joined + assert "WireGuard" in joined and "not declared under " in joined + assert "NAT-generated" in joined or "associated-rule-id" in joined + assert "absent" in joined # the missing + + # The good rule still converts, and nothing anomalous leaks into it. + assert "permit tcp any any" in result.text + assert "WireGuard" not in result.text + assert "em0" not in result.text + assert result.text.count("ip access-list extended") == 1 # only lan + + def test_when_every_rule_is_nat_generated_it_refuses_with_the_specific_reason(): """A distinct refusal from an empty (no rules ever existed) and from every rule missing (no_interface_named) -- the file had @@ -474,22 +511,81 @@ def test_protocol_any_becomes_cisco_ip(): assert "permit ip any any" in _convert_string(xml_text) -def test_unsupported_rule_type_raises(): +def test_unsupported_rule_type_is_skipped_and_named_the_real_reason(): + """Used to abort the WHOLE FILE for one bad (#78 item 4, + re-scoped the same way item 1/2 were in #104): the rule is excluded and + named in `skipped` instead, and everything else still converts. Since + this rule is the only one on `lan`, the interface still gets a bound ACL + -- fail-closed with `deny ip any any`, same as any interface with no + convertible rules.""" xml_text = _minimal_xml(rules_xml=""" matchlantcp """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "lan" in result.skipped[0] + assert "not pass, block or reject" in result.skipped[0] + assert "deny ip any any" in result.text + + +def test_a_rule_with_no_type_element_at_all_gets_its_own_precise_message(): + """The other half of #80's inconsistency, and the reason #78 item 4 was + unresolved: a MISSING and an INVALID used to produce the + identical confusing string ("...is not pass, block or reject: is + None"), conflating "this module does not recognise this value" with + "this field was never written". Split the same way #104 split the + DHCP-WAN and undeclared-VPN interface messages. Still refuses -- there is + no safe default for pass/block/reject the way "any" is one for protocol + -- but names the real, distinct reason and does not touch the whole + file's other rules.""" + xml_text = _minimal_xml(rules_xml=""" + lantcp + + """) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "absent" in result.skipped[0] + assert "not pass, block or reject" not in result.skipped[0], ( + "a missing must not reuse the present-but-invalid message" + ) + assert "deny ip any any" in result.text -def test_combined_tcp_udp_protocol_raises_rather_than_guessing(): +def test_two_rules_on_the_same_role_failing_for_different_reasons_get_separate_notes(): + """The heterogeneous counterpart to + test_two_nat_generated_rules_are_counted_together_not_one_note_each: + NAT-generated rules are merged into one note because they all share the + exact same cause. Rule-level conversion failures do not -- one rule can + be missing while another has an out-of-range port -- so each gets + its own note naming its own real reason, never blended into a single + generic one.""" + xml_text = _minimal_xml(rules_xml=""" + lantcp + + passlantcp +
10.0.0.5
0 + """) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 2 + reasons = " ".join(result.skipped) + assert "absent" in reasons + assert " is '0'" in reasons or "0" in reasons + assert "deny ip any any" in result.text + + +def test_combined_tcp_udp_protocol_is_skipped_not_guessed_and_not_fatal(): + """Still refuses to guess at 'tcp/udp' -- it still needs two Cisco ACL + lines this module does not build -- but that refusal no longer takes the + whole file down with it.""" xml_text = _minimal_xml(rules_xml=""" passlantcp/udp """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "tcp/udp" in result.skipped[0] + assert "deny ip any any" in result.text # --- Rules: source/destination resolution --------------------------------------- @@ -530,25 +626,27 @@ def test_port_is_not_emitted_for_protocol_any(): assert "eq" not in output -def test_a_named_alias_in_address_raises_rather_than_being_emitted_as_a_host(): +def test_a_named_alias_in_address_is_skipped_not_emitted_as_a_host(): """Regression coverage for the review finding: PF Sense's
can hold a named alias (e.g. "TRUSTED_HOSTS") instead of a literal IP. Without validation this became "host TRUSTED_HOSTS" -- not valid Cisco syntax, and measured to NOT make Batfish reject the file outright: fileParseStatus reports PARTIALLY_UNRECOGNIZED and Batfish silently drops - just that line. Must raise here, at the layer that knows what went - wrong, rather than depend on analysis.pipeline's parse strictness (which - is itself under proposal to relax for real-world configs) as the only - thing catching it.""" + just that line. Still refused at the layer that knows what went wrong, + rather than depending on analysis.pipeline's parse strictness -- but the + refusal now excludes this one rule (#78 item 4) instead of the whole + file, matching every other rule-level cause below.""" xml_text = _minimal_xml(rules_xml=""" passlanudp
TRUSTED_HOSTS
 """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "TRUSTED_HOSTS" not in result.text + assert "deny ip any any" in result.text -def test_a_named_alias_in_port_raises_rather_than_being_emitted_literally(): +def test_a_named_alias_in_port_is_skipped_not_emitted_literally(): """Same class of gap as the
case above, found by auditing the rest of the module for the identical pattern once the review pointed out the first instance: PF Sense's can hold a named alias (e.g. @@ -558,32 +656,41 @@ def test_a_named_alias_in_port_raises_rather_than_being_emitted_literally(): passlantcp
10.0.0.5
HTTPS_ALT """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "HTTPS_ALT" not in result.text + assert "deny ip any any" in result.text -def test_port_zero_and_out_of_range_also_raise(): +def test_port_zero_and_out_of_range_are_skipped_not_fatal(): """Boundary check for the same validation -- a port must be in the - valid 1-65535 range, not merely 'looks like digits'.""" + valid 1-65535 range, not merely 'looks like digits'. Each bad port is + its own file here (rather than sharing one, as the old raise-based + version did) so each is independently confirmed to skip cleanly rather + than only the first one being exercised.""" for bad_port in ("0", "65536", "999999"): xml_text = _minimal_xml(rules_xml=f""" passlantcp
10.0.0.5
{bad_port} """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1, bad_port + assert "deny ip any any" in result.text -def test_unresolvable_network_reference_raises(): +def test_unresolvable_network_reference_is_skipped_not_guessed(): """A network alias this converter does not resolve (e.g. PF Sense's 'lanip', meaning the interface's own address rather than its subnet) - must raise, not silently fall back to something plausible-looking.""" + is still refused rather than falling back to something plausible- + looking -- just scoped to this one rule now, not the whole file.""" xml_text = _minimal_xml(rules_xml=""" passlantcp lanip """) - with pytest.raises(PfSenseConversionError): - _convert_string(xml_text) + result = _convert_string_full(xml_text) + assert len(result.skipped) == 1 + assert "lanip" in result.skipped[0] + assert "deny ip any any" in result.text # --- Rules spanning more than one interface -------------------------------------- @@ -1185,11 +1292,12 @@ def test_refusal_summary_lists_every_category(): def test_every_raise_site_actually_uses_a_refusals_entry(): """Guards against the exact drift #155 was filed about: a message that duplicates REFUSALS wording instead of reading it, which can then drift - without anything noticing. Every one of the 18 refusal call sites in + without anything noticing. Every one of the 19 refusal call sites in analysis/pfsense_convert.py was audited by hand to confirm it builds its - message from REFUSALS; this only re-checks the mechanical half -- that - every entry is actually referenced somewhere in the module, so a key - nothing raises for is caught rather than silently accumulating. + message from REFUSALS (18 at #155, plus `missing_rule_type` added for + #78 item 4); this only re-checks the mechanical half -- that every entry + is actually referenced somewhere in the module, so a key nothing raises + for is caught rather than silently accumulating. """ import inspect From e073ba3ba039f6a0a9e7eef23676535deb3ea0bf Mon Sep 17 00:00:00 2001 From: patelankeet2 Date: Sat, 5 Sep 2026 20:19:26 +1200 Subject: [PATCH 2/4] docs: record what #78 item 4 actually resolved, and reaffirm NAT stays untouched CLAUDE.md section 7's 23 August update said the NAT decision and #80's missing type/protocol question were "unaffected and remain open" -- still true of the actual semantic questions, but stale about the blast-radius problem fixed alongside them today. Appended a new UPDATE block rather than editing the old one in place, matching this section's own established practice of leaving prior updates as the historical record of what was true when they were written. docs/design/pfsense-nat-support.md gets a new closing section recording that NAT was checked against today's "proceed without the client" instruction specifically, and does not clear the bar the other three #78 items did: there is no version of implementing NAT that does not require guessing at translation semantics never observed in any real or synthetic shape. Detection and exclusion (#264) stays exactly as it was. This is not new analysis -- it restates the document's own existing conclusion -- but records that the decision was revisited and reaffirmed today rather than simply left stale. No functional change. Docs only. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 39 ++++++++++++++++++++++++++++++ docs/design/pfsense-nat-support.md | 26 ++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/CLAUDE.md b/CLAUDE.md index 144b6e4..096f83b 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -236,6 +236,45 @@ Batfish runs in Docker container `batfish` (image `batfish/allinone`), exposing > missing ``/`` question (#80) are unaffected by this and > remain open. + > **UPDATE, 5 September.** The rest of #78 was picked up without waiting + > for Senaka's answers to the four questions now written out in Discussion + > #294, on the reasoning that a decision genuinely blocked on him should + > stay blocked, but nothing else should sit idle in the meantime. That + > split the remaining work into two very different piles. + > + > **The blast radius, fixed.** A single rule failing to convert for ANY + > reason (missing or unsupported ``, an unsupported `` + > value, an unresolved alias, a bad port, an unresolvable network + > reference) used to abort the entire file, not just the interface or rule + > it was on. At the client's real scale — 1,998 XML elements against this + > project's 54-element fixture — one anomaly anywhere meant zero analysis + > of anything. Re-scoped the same way the DHCP-WAN/undeclared-VPN case was: + > the one failing rule is excluded and named, everything else converts + > exactly as if it were never there. This required no guess about what any + > field means — every refusal still names the same cause it always did, + > only its blast radius shrank from "the whole file" to "this one rule". + > + > **``'s semantic question, still untouched.** #80 itself says + > "do not fix this before the client answers" — that instruction stands, + > unconditionally. What DID move: missing `` now gets its own precise + > message, split from present-but-invalid the same way the two interface + > causes were split earlier, because there is no safe default for + > pass/block/reject the way "any" is one for protocol. ``'s + > existing default-to-`"any"` behaviour was not touched at all. + > + > **NAT, reaffirmed rather than left stale.** Checked against the same + > "no guessing" bar the blast-radius fix cleared: it does not clear it. + > There is no version of "convert the NAT rule" that does not invent + > translation semantics this module has never observed. Detection and + > exclusion (#264) is unchanged, and `docs/design/pfsense-nat-support.md` + > records that this was revisited today, not simply never revisited. + > + > A separate idea — disclosing an inferred `` with a comment, + > #80's own preferred Option C, which changes no behaviour, only makes the + > existing silent default visible — was drafted as a question for the team + > rather than built solo, precisely because it touches the one issue + > marked blocked on the client. Not yet decided. + ## 7a. The finding format (F-1) — the one contract **`docs/finding-format.md` is authoritative.** It was agreed by all four team diff --git a/docs/design/pfsense-nat-support.md b/docs/design/pfsense-nat-support.md index d680b86..0d79ed5 100644 --- a/docs/design/pfsense-nat-support.md +++ b/docs/design/pfsense-nat-support.md @@ -91,3 +91,29 @@ Not a decision. Not started. Filed so the choice is visible rather than re-derived from scratch, the same reason `remediation-phase-b.md` and `config-change-and-pushback.md` exist as separate documents from the features they scoped. + +## Revisited without waiting for the client, and reaffirmed (#78) + +The rest of #78 was picked up without waiting for Senaka's answers to the +four questions in Discussion #294, on the reasoning that a decision genuinely +blocked on him should stay blocked, but nothing else should sit idle in the +meantime. NAT was checked against that reasoning specifically, and it does +not clear the bar the other three items did. + +The WAN-address and undeclared-VPN-interface problems, and the whole-file +blast radius fixed alongside missing `` (see `CLAUDE.md` section 7), +were all fixable without any guess about network behaviour — they are +questions about how far one bad input's damage should spread, not about +what any field means. NAT is a different shape of problem: there is no +version of "convert the NAT rule" that does not require inventing +translation semantics (source, destination, or port rewriting) this module +has never observed in any real or synthetic `` element. Option 1 above +was already rejected for exactly that reason before this revisit, and +nothing about proceeding without the client changes the argument — it is +not a decision waiting on permission, it is a guess waiting on evidence +that still does not exist. + +So: unchanged. Detection and exclusion (Option 2, already shipped as #264) +remains the only NAT-related behaviour in this module. The three +information-gathering steps above are still the right next steps, in the +same order, whenever someone picks this up. From 67c7c8097033e58210dd1465a9ee7bbf84e87cbd Mon Sep 17 00:00:00 2001 From: patelankeet2 Date: Sun, 6 Sep 2026 15:28:45 +1200 Subject: [PATCH 3/4] carry a pfSense conversion's skip notes through to the downloaded report (#302 review) Samika's review found a real gap: ConversionResult.skipped reached the user exactly once, in the upload response, and was gone by the time a report was downloaded later. A config with every check running clean could produce a report claiming "Every reported check ran. Nothing was skipped." while real rules from the uploaded file were never analysed -- not false about the checks, but read as one claim, and the wrong one on a report about a partially-converted config. She also found the direction that makes this genuinely dangerous, not just imprecise: skipping a `pass` rule makes the emitted ACL stricter than the real device, which can make a policy check that should report a violation report `none` instead -- F-4's exact failure, reached through the converter rather than a check. Recorded in CLAUDE.md section 7 (6 September update) regardless of which fix shipped, per her request. Two occurrences of the same false claim existed, worded differently (analysis/coverage.py's coverage statement, and render_html()'s own "Could not check" section fallback text) -- both fixed, since finding one by testing does not prove the other was caught. analysis/coverage.py: summarise() takes an optional conversion_gaps parameter, additive and backward compatible -- every existing caller that omits it gets the exact prior output. When non-empty, `complete` is False and the statement says a part of the file was excluded, never merged with check-level gaps (a check that could not run and a rule excluded before any check ran are different claims). analysis/report.py: render_html() threads conversion_gaps through to _coverage_html(), rendered as its own "Excluded during conversion, before any check ran" list, separate from the check-gap list. web/main.py: a new pfsense-skipped.json staged beside policy.json and business-context.json, following their exact pattern -- written on a PF Sense upload that skipped anything, cleared on every new upload (same reasoning as _discard_staged_policy(): a skip note names a rule from the PREVIOUS config), left untouched by a refused conversion (same validate-then-stage discipline the staged config file itself follows). /api/report reads it back and passes it through. Filed #306 for the more complete fix Samika's review also named -- carrying skips into F-1 findings directly, visible on the dashboard itself -- as its own properly scoped follow-up rather than folding it into this one. Tested: 6 new report-rendering tests (the false claim in both locations, plural phrasing, separation from check-gaps, backward compatibility, the conversion_gaps key always present) and 8 new web-layer tests (persistence, clearing on new upload, refusal leaves the previous upload's notes untouched, the report actually names a skip, a clean conversion's report is unaffected). Mutation-tested all three new guards by hand: reverted the coverage.py branch (2 tests catch it), reverted the report.py blind-section text (1 test catches it), reverted the discard-before-write ordering in web/main.py (1 test catches it, and for the exact reason -- a stale file survives). Full suite: 1271 passed, 8 skipped. ruff clean. No em dashes. Co-Authored-By: Claude Sonnet 5 --- CLAUDE.md | 40 ++++++++++++ analysis/coverage.py | 51 ++++++++++++++- analysis/report.py | 59 +++++++++++++---- tests/test_report_export.py | 80 +++++++++++++++++++++++ tests/test_web_config_upload.py | 111 ++++++++++++++++++++++++++++++-- web/main.py | 79 ++++++++++++++++++++++- 6 files changed, 400 insertions(+), 20 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 096f83b..9d4aafc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -275,6 +275,46 @@ Batfish runs in Docker container `batfish` (image `batfish/allinone`), exposing > rather than built solo, precisely because it touches the one issue > marked blocked on the client. Not yet decided. + > **UPDATE, 6 September — the blast-radius fix has a direction, and one + > direction is dangerous.** @SamikaPerera's review on #302 found what the + > entry above did not name: excluding a rule changes what the emitted ACL + > decides, and the two directions are not equally safe. + > + > **Skipping a `block` rule makes the emitted ACL MORE permissive than the + > real device.** Netwise then reports traffic as reachable that the real + > firewall denies — noisy, but safe, because the false finding gets + > investigated and dismissed. + > + > **Skipping a `pass` rule makes the emitted ACL STRICTER than the real + > device**, and this is the direction that matters. A policy rule asserting + > *"this traffic must be denied"* can then report `none` (clean) against + > the emitted ACL, while the real firewall actually permits that traffic — + > a real policy violation, reported as a pass. That is F-4's exact failure, + > reached through the converter instead of through a check: the report says + > "we checked and it is fine" about something nobody actually verified. + > + > Not new IN KIND — #104's interface-level skip made the identical trade, + > accepted at the time. What changed is conspicuousness: a whole excluded + > interface is visible in the skip list as a named gap; a single dropped + > `pass` rule inside an otherwise-normal ACL is not, unless someone reads + > the skip notes and reasons through what they mean for a specific policy + > rule. + > + > **Fixed, not just documented**: `analysis/coverage.py` and + > `analysis/report.py` (PR #302) now carry the skip list through to the + > downloaded report as its own "excluded during conversion" section, and + > the report's coverage statement stops claiming "nothing was skipped" + > when a conversion excluded a rule, even when every check ran clean. That + > closes the disclosure gap Samika also found — before this, the skip list + > reached the user exactly once, at upload, and was gone by the time a + > report was generated. It does not close the underlying modelling gap + > above: a skipped `pass` rule is still excluded, not modelled, and a + > policy check can still report `none` where the real firewall would not. + > Carrying skips into F-1 findings directly (an `error` card per skip, + > visible on the dashboard itself, not only in a downloaded report) was + > raised as the more complete fix and deliberately left for a separate, + > properly scoped change rather than folded into this one -- see #306. + ## 7a. The finding format (F-1) — the one contract **`docs/finding-format.md` is authoritative.** It was agreed by all four team diff --git a/analysis/coverage.py b/analysis/coverage.py index 074a895..a8b8751 100644 --- a/analysis/coverage.py +++ b/analysis/coverage.py @@ -11,11 +11,32 @@ * status="error" means Netwise could not check something. That row must remain visible even when the main finding sections are skimmed, exported, or sorted elsewhere. + +CONVERSION GAPS ARE A SEPARATE THING FROM A CHECK GAP, AND THEY ARE PASSED IN +SEPARATELY RATHER THAN INFERRED (#302 review, @SamikaPerera) + A finding-based gap means a CHECK could not run. A conversion gap means + part of the SOURCE FILE was never turned into anything a check could see + at all -- today, a pfSense rule `analysis/pfsense_convert.py` could not + convert (#78). Neither this module nor its caller can derive that from + the findings list, because a converter-excluded rule leaves no finding + behind to derive it from -- that is exactly the gap. + + Before this parameter existed, `summarise()` had no way to know a + conversion had dropped anything, so a config with every CHECK running + cleanly reported "Every reported check ran. Nothing was skipped." even + when real rules from the uploaded file were never analysed. Not false + about the checks -- every one of them did run -- but read by a person as + a single claim, and the wrong one: skipping a `pass` rule makes the + emitted config STRICTER than the real device, which can make a policy + check that should report a violation report `none` instead. See + `CLAUDE.md` section 7 for the direction analysis. A reader who trusted + "nothing was skipped" would have no way to know that risk exists for + this specific report. """ from __future__ import annotations -from typing import Any, Dict, Iterable, List, Tuple +from typing import Any, Dict, Iterable, List, Sequence, Tuple def _label(finding: Dict[str, Any]) -> Tuple[str, str]: @@ -25,9 +46,19 @@ def _label(finding: Dict[str, Any]) -> Tuple[str, str]: ) -def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: +def summarise( + findings: Iterable[Dict[str, Any]], + conversion_gaps: Sequence[str] = (), +) -> Dict[str, Any]: """Return the coverage/certainty view implied by a findings list. + `conversion_gaps` is optional and additive -- omitting it reproduces this + function's exact prior behaviour, so every existing caller keeps working + unchanged. Pass it when the findings came from a converted config and the + converter excluded anything, so the statement below can stop claiming + nothing was skipped when something genuinely was -- just not something a + check could have reported on. + The result is intentionally plain dicts, matching the rest of the analysis layer's F-1 data. It is derived, not cached; if the findings change, the coverage statement changes with them. @@ -59,6 +90,7 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: {"check": check, "device": device} for check, device in sorted(checked) ] + conversion_gap_rows = [str(note) for note in conversion_gaps] if not checked and not gaps: # THE THIRD STATE, AND IT IS NOT COMPLETENESS. @@ -80,6 +112,20 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: f"Findings may be incomplete." ) complete = False + elif conversion_gap_rows: + # THE CASE #302's REVIEW FOUND. Every check ran and none of them + # reported a blind spot -- but that is a claim about the checks, + # not about the file. Complete is False here on purpose: this + # report cannot say analysis was exhaustive when part of the + # source file was excluded before any check ever saw it. + one = len(conversion_gap_rows) == 1 + statement = ( + f"Every reported check ran, but {len(conversion_gap_rows)} part" + f"{'' if one else 's'} of the uploaded configuration could not " + f"be converted and {'was' if one else 'were'} not included in " + f"this analysis." + ) + complete = False else: statement = "Every reported check ran. Nothing was skipped." complete = True @@ -89,4 +135,5 @@ def summarise(findings: Iterable[Dict[str, Any]]) -> Dict[str, Any]: "statement": statement, "checked": checked_rows, "gaps": gaps, + "conversion_gaps": conversion_gap_rows, } diff --git a/analysis/report.py b/analysis/report.py index 35b6195..60a88a7 100644 --- a/analysis/report.py +++ b/analysis/report.py @@ -212,15 +212,23 @@ def _section_html(title: str, lede: str, items: List[Dict], css_class: str, return f'{head}

{html.escape(lede)}

{body}' -def _coverage_html(findings: Sequence[Dict[str, Any]]) -> str: +def _coverage_html( + findings: Sequence[Dict[str, Any]], + conversion_gaps: Sequence[str] = (), +) -> str: """The first-class coverage statement (#235), derived from status values. It is not a fourth finding section. It is the reading aid that says whether the finding sections are complete, and if not, exactly which blind spots remain. Removing the gap list is the mutation #235 names: the report would still have findings, but no longer name what was not checked. + + `conversion_gaps` (#302 review) is rendered as its OWN list, never merged + into `gaps` -- a check that could not run and a rule a converter excluded + before any check ran are different claims, and merging them would lose + which one happened. """ - summary = coverage.summarise(findings) + summary = coverage.summarise(findings, conversion_gaps) css = "complete" if summary["complete"] else "incomplete" esc = html.escape @@ -263,14 +271,30 @@ def _coverage_html(findings: Sequence[Dict[str, Any]]) -> str: f'

Checked: {esc(names)}.

' ) + if summary["conversion_gaps"]: + parts.append( + '

Excluded during conversion, ' + "before any check ran:

" + ) + parts.append("
    ") + for note in summary["conversion_gaps"]: + parts.append(f"
  • {esc(note)}
    • ") + parts.append("
") + parts.append("") return "".join(parts) def render_html(findings: Sequence[Dict[str, Any]], source: Optional[str] = None, - generated_at: Optional[str] = None) -> str: - """A complete, self-contained HTML report. No external files, no scripts.""" + generated_at: Optional[str] = None, + conversion_gaps: Sequence[str] = ()) -> str: + """A complete, self-contained HTML report. No external files, no scripts. + + `conversion_gaps` (#302 review) names anything a converter (pfSense's, + today) excluded before this findings list ever existed -- optional, and + omitting it reproduces this function's exact prior output. + """ s = _sections(findings) when = generated_at or _now() subject = source or "an uploaded configuration" @@ -287,17 +311,30 @@ def render_html(findings: Sequence[Dict[str, Any]], ) # "Nothing was skipped" is a COVERAGE claim, and it is only true when - # something was actually reported. With an empty findings list it says - # nobody-looked in the words of everything-ran -- the same F-4 confusion - # the coverage box above was fixed for, in a sentence that predates it. - # One claim, two places; both now read from the same condition. + # something was actually reported AND nothing was excluded before any + # check ran (#302 review -- conversion_gaps is exactly that second + # condition, which this sentence did not know about before). With an + # empty findings list it says nobody-looked in the words of + # everything-ran -- the same F-4 confusion the coverage box above was + # fixed for, in a sentence that predates it. One claim, two places; both + # now read from the same conditions. + if conversion_gaps: + blind_empty_text = ( + "Every check that ran found nothing, but part of the uploaded " + "configuration was excluded before any check saw it -- see " + "Coverage and certainty above." + ) + elif findings: + blind_empty_text = "Every check ran. Nothing was skipped." + else: + blind_empty_text = "No check reported a result, so nothing is known either way." + blind = _section_html( "Could not check", "These checks did not run. Nothing is known about what they cover. " "This is not a pass.", s["blind"], "blind", - ("Every check ran. Nothing was skipped." if findings - else "No check reported a result, so nothing is known either way."), + blind_empty_text, warn=True) problems = _section_html( @@ -320,7 +357,7 @@ def render_html(findings: Sequence[Dict[str, Any]], f"

Netwise analysis report

" f'

{html.escape(subject)} · generated ' f'{html.escape(when)}

' - f"{counts}{_coverage_html(findings)}{blind}{problems}{clean}" + f"{counts}{_coverage_html(findings, conversion_gaps)}{blind}{problems}{clean}" '