Skip to content

A rule with no <protocol> silently widens to 'ip'; a rule with no <type> refuses #80

Closed

Description

@ARSH871-bot

Found while checking what the converter does with the two field-omission anomalies in the client's export (#78). Synthetic inputs only — no client data was read, and none is in this issue.

The inconsistency

Two rule fields can be missing. The converter treats them oppositely:

no <type>       -> refused cleanly:  "unsupported rule <type>: None"
no <protocol>   -> CONVERTED:        permit ip any any

analysis/pfsense_convert.py:425 and :474:

pf_protocol = _text(rule_el, "protocol", default="any")

"any" maps to Cisco ip, so a rule with no <protocol> silently becomes the
widest possible protocol match. One missing field refuses; the other
silently widens the rule.

Why this matters now rather than in the abstract

Four of the client's seven rules carry no <protocol> (#78). So this path
is not hypothetical — it is most of his rule set, and it is the path that
broadens rather than narrows.

What I am NOT claiming

pfSense may genuinely treat an absent <protocol> as "any". If so, ip is
the correct translation and the behaviour is right. I have not confirmed that,
and I am not asserting it is wrong.

The problem is narrower and survives either answer: the converter cannot
distinguish "pfSense omitted a default" from "the field was stripped", and it
resolves that ambiguity silently, in the permissive direction.
Everywhere else
this module meets an input it cannot be certain about, it raises.

That is the discipline CLAUDE.md §7 describes and #53/#54/#58 all implement.
This one line is the exception, and it is exercised four times by the first real
file we have ever seen.

Options

A — leave it, and document it. If pfSense's default really is "any", this is
correct and only the silence is wrong. Cheapest, and it needs the client answer
first.

B — require <protocol> explicitly, refusing when absent. Consistent with
<type>, and safe. Costs: refuses configs that are legitimately using the
default, which may be many.

C — keep the default but disclose it — emit the ACL line with a comment
recording that no protocol was specified, so a reader can see the widening was
inferred rather than written. Mirrors what ai/query.py does for a substituted
CIDR: make the translation visible rather than silent.

My preference is C, then A once the client answers. B is safest and probably
too strict for real files.

Blocked on a client answer

Question already sent: is the omission how pfSense stores it, or did the
anonymisation drop it?
Do not fix this before that comes back — the right
behaviour depends on it, and guessing here would be the same mistake the issue
is about.

Reproducing

# a rule with <type> and <source>/<destination> but no <protocol>
convert(path)   # -> permit ip any any

Full probe in the comments of #78's investigation; four synthetic cases, no
client file involved.

Activity

  1. added this to the Sprint 5 milestone on Aug 23, 2026
  2. modified the milestones: Sprint 5, Sprint 6 on Aug 31, 2026
  3. patelankeet2 commented on Sep 5, 2026

    @patelankeet2
    Collaborator

    Confirming this stays exactly where it was: open, blocked on the client, untouched.

    While working through the rest of #78 without waiting on Senaka (PR #302), I checked this issue specifically before doing anything near it, because its own text says "do not fix this before that comes back -- the right behaviour depends on it, and guessing here would be the same mistake the issue is about." That instruction held. PR #302 does not change <protocol>'s default-to-any behaviour at all, and does not choose between Options A/B/C.

    What #302 does fix is adjacent but genuinely different: a rule with no <type> used to abort conversion of the ENTIRE file, not just itself -- that's a blast-radius problem (how far one bad rule's damage spreads), not the semantic question this issue is about (what an absent field means). Fixed by excluding just that one rule and naming it, the same way an unmodellable interface already was. Missing <type> also now gets its own precise message, split from "present but invalid" -- but the split is about wording clarity, not a decision about correctness.

    One thing worth raising here rather than deciding alone: your own Option C (keep the default, disclose it with a comment) doesn't require guessing at anything either -- same ip/any output as today, just visible instead of silent. That's a real candidate for closing the "silently" half of your complaint even before Senaka answers the "is it even wrong" half. Drafted as a message for the team rather than built into #302, since it's your issue and your call whether that's worth doing now or waiting for A. Let me know either way.

  4. ARSH871-bot commented on Sep 27, 2026

    @ARSH871-bot
    OwnerAuthor

    Answered from pfSense's own source rather than the client, and fixed in #367 (stacked on #302).

    pfSense saves "any" by unsetting <protocol> (firewall_rules_edit.php:1195 at 480a1c0626), so ip is right for protocol-less rules, including the client's four. Option B would have refused valid rules. Two neighbouring cases did widen silently, and #367 translates them exactly: no <protocol> + a port (pfSense means TCP, filter.inc:4497), and tcp/udp source ports (enforced by pfSense, previously dropped). Verified in Batfish; the table is in #367.

    @patelankeet2, on your Option C question from 5 September: sorry it sat unanswered. With every absent-<protocol> case now matching pfSense's documented behaviour, there's no silent default left to disclose, so C is superseded.

  5. added a commit that references this issue on Sep 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    blocked: clientWaiting on an answer from SenakabugSomething isn't working

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions