refactor(pkg): audit converters and ConversionWarning API — NATS-145 - #580
Merged
Conversation
Harden the public ConversionWarning surface and lock in converter invariants that downstream consumers (opnConfigGenerator, opndossier-pro) depend on. Audit uncovered one real production bug (fixed) and six test-quality gaps (also fixed in the same review pass). Production fix - pkg/parser/opnsense/converter.go: add IsValid() guards and warnings to convertOutboundNATRules (line 373) and convertInboundNATRules (line 429). Both sites previously did bare `common.IPProtocol(r.IPProtocol)` casts with no validation — exactly the GOTCHAS §5.2 anti-pattern. The pfSense equivalents already had the guard; OPNsense was asymmetric and silently passed invalid values through the pipeline. Godoc - pkg/model/warning.go: strengthen Severity constants with concrete examples of when each level applies; add a producer/consumer usage block on ConversionWarning; note that SeverityCritical is reserved for future use. Regression tests (all new) - pkg/model/warning_test.go: Severity table tests + runnable ExampleConversionWarning. - pkg/parser/opnsense/converter_enum_cast_test.go: 8 enum-cast cases (firewall Type/Direction/IPProtocol, NAT Outbound Mode, NAT Outbound and Inbound rule IPProtocol, LAGG Protocol, VIP Mode) with specific severity assertions. Empty-string exemption and multi-invalid accumulation both covered. - pkg/parser/opnsense/converter_kea_test.go: 5 GOTCHAS §18 regression tests — happy path, multi-pool first-wins with warning, orphan reservation, subnet missing UUID, reservation missing parent subnet. - pkg/parser/opnsense/converter_testhelpers_test.go: shared findKeaScope and findWarning helpers for the opnsense_test package. - pkg/parser/pfsense/converter_enum_cast_test.go: 6 enum-cast cases covering both InboundRule and OutboundRule IPProtocol sites, empty- string exemption, and multi-invalid accumulation. - pkg/parser/pfsense/converter_ipsec_test.go: GOTCHAS §16 table-driven test locking in the Phase-1-gated IPsec Enabled contract — Phase 2 or mobile client without Phase 1 warns as orphan and leaves Enabled=false. Documentation - GOTCHAS.md §5.2: reference the new regression tests and record the NATS-145 audit history (the two OPNsense NAT IPProtocol gaps). - GOTCHAS.md §16.1: rewrite to match the actual Phase-1-gated implementation. The previous wording implied any of Phase1, Phase2, or mobile client could enable IPsec — that has never been true. Verification - pkg/model 98.2% coverage, pkg/parser/opnsense 94.9%, pkg/parser/pfsense 94.1%. - just ci-check passes clean. - go test -race ./pkg/model/... ./pkg/parser/... passes. - Zero internal/ imports in production converter code. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
dosubot
Bot
added
the
size:L
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Require conventional commit format per https://www.conventionalcommits.org/en/v1.0.0/. Skipped for dependabot and dosubot.
🟢 Full CI must passWonderful, this rule succeeded.All CI checks must pass. Activates for non-bot authors, or dependabot when files exist outside .github/workflows/.
🟢 Do not merge outdated PRsWonderful, this rule succeeded.Make sure PRs are within 10 commits of the base branch before merging
|
dosubot
Bot
added
bug
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded guarded enum validation for IPProtocol in OPNsense NAT converters that emits low-severity warnings for non-empty invalid values; tightened pfSense IPsec Enabled semantics to require Phase1 and emit medium-severity orphan warnings for Phase2/Client without Phase1; expanded godoc for Severity/ConversionWarning; and added regression/unit tests and test helpers for enum casts, Kea DHCPv4, and pfSense IPsec. Several docs/comments updated; no public API signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant XML as OpnSense/pfSense XML
participant Parser as pkg/parser
participant Converter as converter logic
participant Model as pkg/model (ConversionWarning)
participant Output as CommonDevice
XML->>Parser: parse into device-specific Document
Parser->>Converter: ConvertDocument(document)
Converter->>Converter: map fields, perform enum casts (e.g., IPProtocol)
alt enum value invalid and non-empty
Converter->>Model: add low-severity ConversionWarning(Field, Value, Message)
end
alt pfSense IPsec Phase1 absent but Phase2/Client present
Converter->>Model: add medium-severity orphan IPsec warnings
Converter->>Converter: ensure VPN.IPsec.Enabled = false
else Phase1 present
Converter->>Converter: set VPN.IPsec.Enabled = true
end
Converter->>Output: return CommonDevice + []ConversionWarning
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
coderabbitai
Bot
added
go
Contributor
There was a problem hiding this comment.
Pull request overview
Audit-and-hardening pass over the public ConversionWarning API and the OPNsense/pfSense converters to ensure enum-cast validation is consistently guarded and regression-tested for downstream consumer imports.
Changes:
- Fixed OPNsense NAT rule
IPProtocolenum-cast handling by adding the missingIsValid()guard + warning emission in both outbound and inbound NAT conversions. - Expanded
pkg/modelwarning API documentation (Severity guidance + producer/consumer usage) and added focused unit tests around severity validation andValidSeverities(). - Added/updated regression tests covering enum-cast warning invariants, pfSense IPsec Phase-1 gating behavior, and OPNsense Kea DHCP4 pool/reservation semantics; updated GOTCHAS references accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/opnsense/converter.go | Adds IPProtocol enum validity guarding + warning emission for NAT rules (production bug fix). |
| pkg/model/warning.go | Strengthens public API docs for Severity and ConversionWarning usage/expectations. |
| pkg/model/warning_test.go | Adds unit tests for Severity.String, IsValidSeverity, and ValidSeverities() copy/coverage invariants. |
| pkg/parser/opnsense/converter_enum_cast_test.go | Regression tests locking in enum-cast warning behavior (including NAT IPProtocol cases). |
| pkg/parser/pfsense/converter_enum_cast_test.go | pfSense-specific enum-cast regression coverage mirroring converter callsites. |
| pkg/parser/pfsense/converter_ipsec_test.go | Regression test enforcing the Phase-1 gate invariant for pfSense IPsec Enabled. |
| pkg/parser/opnsense/converter_kea_test.go | Regression tests for Kea DHCP4 pool parsing + reservation/subnet UUID orphan handling. |
| pkg/parser/opnsense/converter_testhelpers_test.go | Shared test helpers for warning/scope lookup used by new OPNsense converter tests. |
| GOTCHAS.md | Updates GOTCHAS §5.2/§16 guidance with references to new regression tests and corrected IPsec gating wording. |
![coderabbitai[bot]](https://webproxy.poorya-velaei-d67.workers.dev/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/parser/opnsense/converter_kea_test.go`:
- Around line 20-161: These five near-duplicate tests
(TestConverter_KeaDHCP4_Gotchas18_HappyPath, _MultiPoolFirstWins,
_OrphanReservation, _SubnetMissingUUID, _ReservationMissingSubnet) should be
consolidated into a single table-driven test that iterates over a slice of cases
(each case containing a name, a prebuilt schema.NewOpnSenseDocument() or a
builder func, and a per-case assertion callback). Implement a cases
[]struct{name string; doc *schema.OpnSenseDocument or build func()
*schema.OpnSenseDocument; assert func(t *testing.T, device opnsense.Device,
warnings []common.Warning)} then run for each with t.Run(case.name, func(t
*testing.T){ t.Parallel(); device, warnings, err :=
opnsense.ConvertDocument(case.doc); require.NoError(t, err); case.assert(t,
device, warnings) }), keeping the existing checks (findKeaScope, findWarning,
assertions about ranges, static leases, and warning messages) inside each case's
assert to preserve behavior and warnings.
- Around line 108-110: Tests in pkg/parser/opnsense/converter_kea_test.go only
check for existence and message of warning entries (e.g., orphanWarning from
findWarning(warnings, "kea.dhcp4.reservations", "subnet-missing")) but do not
assert the warning Severity; update each warning-path test (the cases using
findWarning at the three locations) to include an explicit severity assertion
(e.g., require.Equal or assert.Equal on orphanWarning.Severity) so the expected
severity is enforced and accidental severity changes will fail CI. Locate the
occurrences that call findWarning(warnings, "...", "...") and add an assertion
that the returned warning.Severity equals the intended level for that test.
- Around line 117-135: The test documents that a subnet without a UUID must
still produce a scope but currently only asserts a warning; update
TestConverter_KeaDHCP4_Gotchas18_SubnetMissingUUID to also assert that
opnsense.ConvertDocument's first return (scopes) contains a scope for the
"10.40.0.0/24" subnet (or an equivalent scope object) so scope creation is
locked in; use the returned scopes from ConvertDocument and a helper or simple
loop to find a scope matching subnet "10.40.0.0/24" and require it is present
alongside the existing warning checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9d5be21a-1ed2-4136-b36b-a0d771d189c3
📒 Files selected for processing (9)
GOTCHAS.mdpkg/model/warning.gopkg/model/warning_test.gopkg/parser/opnsense/converter.gopkg/parser/opnsense/converter_enum_cast_test.gopkg/parser/opnsense/converter_kea_test.gopkg/parser/opnsense/converter_testhelpers_test.gopkg/parser/pfsense/converter_enum_cast_test.gopkg/parser/pfsense/converter_ipsec_test.go
- pkg/parser/opnsense/converter_kea_test.go: add SeverityMedium assertions to the three Kea orphan/UUID warning tests (CodeRabbit review thread PRRT_kwDOLTC0_M57-Ifh); add scope-creation assertion in the UUID-missing subnet test so scope regression can't hide behind warning-only checks (CodeRabbit thread PRRT_kwDOLTC0_M57-Ifi). - pkg/parser/pfsense/converter_ipsec_test.go: assert orphan-warning Value format; add TestConverter_IPsecPhase1/Phase2 FieldValidators and TestConverter_IPsecPhase1_PreSharedKeyExclusion to cover the previously- untested Phase 1/2 enum validators and the PSK security-exclusion warning. - pkg/model/warning_test.go: rewrite ExampleConversionWarning with real severities and field paths that match actual converter output; add TestIsValidSeverity_MatchesValidSeverities cross-check. - pkg/model/warning.go: clarify that ConversionWarning.Value sometimes stores raw input rather than a sibling identifier. - pkg/parser/opnsense/converter_enum_cast_test.go: disambiguate godoc to clarify this file is OPNsense-only. - Tighten assert.NotNil -> require.NotNil in MultipleInvalidsAccumulate tests for consistency with the main enum-cast test. CodeRabbit thread PRRT_kwDOLTC0_M57-Ifg (table-driven consolidation) is declined in favor of per-test functions that preserve the GOTCHAS §18.x narrative; reply on the thread. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
audit
- pkg/model/warning.go: IsValidSeverity now uses slices.Contains against
validSeverities, eliminating the dual-maintenance boundary that a
cross-check test could only half-cover (ce:review COR-001 / M-08).
Adding a new Severity constant now requires touching exactly one place.
- pkg/model/warning_test.go: drop the redundant TestIsValidSeverity_
MatchesValidSeverities (trivially true after the slices.Contains
refactor) and the redundant valid-case rows of TestIsValidSeverity
(already covered by TestValidSeverities_Coverage). Keep the
negative-case rows — those are the real tripwires for case-sensitivity
and whitespace handling.
- pkg/parser/pfsense/converter_ipsec_test.go: switch PSK exclusion
assertion from fmt.Sprintf("%+v", tunnel) to json.Marshal (security
reviewer SEC-001) so pointer fields cannot hide PSK leaks; consolidate
three parallel warning-search loops to findPfSenseWarning
(maintainability M-06); add TestConverter_IPsecPhase2FieldValidators_
EmptyDoesNotWarn to lock in the Phase 2 empty-string exemption
(testing G-002).
- pkg/parser/opnsense/converter_kea_test.go: move the explanatory comment
to directly precede the `_ = findKeaScope(...)` discard so the intent
is visible at a glance (maintainability M-07).
- internal/converter/enrichment.go: correct the misleading SECURITY NOTE
that claimed schema.IPsec.PreSharedKeys is already excluded from
CommonDevice. It is mapped, but currently holds UUID references
rather than raw key material. Comment now reflects reality and notes
the invariant that must be preserved (security reviewer SEC-002).
Also cross-references the pfSense PSK exclusion test.
SEC-003 (sanitizer pinning test for the pfSense <pre-shared-key> element
name) deferred to todo 100.
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Contributor
Merge Queue Status🛑 Queue command has been cancelled |
39 tasks
Document the OPNsense NAT rule IPProtocol bare-cast bug that the NATS-145 audit uncovered and fixed, so the institutional knowledge compounds instead of rediscovering the same pattern the next time. Includes: - The exact before/after at both callsites - Why prose-only GOTCHAS §5.2 wasn't a sufficient tripwire - The "asymmetry detection" review technique that found the bug (when one parallel converter has a guard and the other doesn't, that's the signal) - Cross-references to the table-driven regression test that now acts as the durable tripwire - Session-history context: NATS-144's audit stopped at the top-level signatures, NATS-145's initial planning prematurely declared "all guarded", and ce:review correctness reviewer at 0.88 confidence surfaced the gap. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
coderabbitai
Bot
added
documentation
Contributor
There was a problem hiding this comment.
Warning
CodeRabbit couldn't request changes on this pull request because it doesn't have sufficient GitHub permissions.
Please grant CodeRabbit Pull requests: Read and write permission and re-run the review.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@docs/solutions/logic-errors/opnsense-nat-ipprotocol-enum-cast-missing-guard.md`:
- Around line 40-43: The document contains internal provenance/meta phrases like
"(session history)", "0.88 confidence", and "(auto memory [claude])" that must
be removed or replaced with repository-verifiable references; edit the narrative
in this file so any mention of provenance in the paragraphs that reference the
NATS-144/NATS-145 analysis (including the passages discussing
convertOutboundNATRules and convertInboundNATRules and the later mentions around
lines 115-117) either links to a concrete commit/PR/test identifier or is
deleted, ensuring the text only contains operator-facing, verifiable evidence
(commit/PR links, test names, or code references) and no internal notes or
confidence tags.
- Line 100: Replace the brittle hard-coded line numbers with stable symbol-based
references: mention the file pkg/parser/pfsense/converter_security.go and refer
to the specific symbols/sections handling the firewall rule IPProtocol, outbound
NAT rule IPProtocol, and inbound NAT rule IPProtocol (e.g., the
functions/handlers that define those fields) instead of "line 72/165/237";
update the doc under docs/**/*.md to cite those symbol names (and optionally the
relevant PR or test IDs) so the cross-file references remain accurate as the
code moves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4cad562c-60e7-4dcc-ad01-475ce42c646a
📒 Files selected for processing (1)
docs/solutions/logic-errors/opnsense-nat-ipprotocol-enum-cast-missing-guard.md
…e refs (#580) CodeRabbit feedback on the NATS-145 solution doc: the narrative contained internal provenance tags ("(session history)", "0.88 confidence", "(auto memory [claude])") that aren't user-verifiable, and a pfSense cross-reference used hard-coded line numbers that will drift. - Replaced the provenance tags with repo-verifiable links (PR #575 for NATS-144, the NATS-145 plan document). - Rewrote the pfSense cross-reference to use function names (convertFirewallRules, convertNAT, convertOutboundNATRules, convertInboundNATRules) instead of line numbers. - Dropped the "0.88 confidence" framing; the finding was surfaced during code review and the value-add is in the technique, not the score. - Quoted the AGENTS.md Code Quality Policy verbatim instead of the auto-memory paraphrase, so the source is the project file. Resolves CodeRabbit threads PRRT_...2iE (major) and PRRT_...2iF (nitpick). Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
Member
Author
|
@Mergifyio queue |
Contributor
Merge Queue Status
Required conditions to enter a queue
|
Contributor
Merge Queue Status
This pull request spent 5 minutes 59 seconds in the queue, including 5 minutes 45 seconds running CI. Required conditions to merge
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audit-and-harden pass on the converter packages that downstream consumers (opnConfigGenerator, opndossier-pro) will import. Scoped to
pkg/model/warning.go(publicConversionWarningAPI) andpkg/parser/{opnsense,pfsense}/converter*.go. Audit uncovered one real production bug (fixed) and six test-quality gaps (all fixed in the same pass).pkg/parser/opnsense/converter.gohad two barecommon.IPProtocol(r.IPProtocol)casts inconvertOutboundNATRules(line 373) andconvertInboundNATRules(line 429) with noIsValid()guard — the exact GOTCHAS §5.2 anti-pattern. pfSense already had the guard; OPNsense was asymmetric and silently passed invalid values through the pipeline. Fixed by adding the canonicalif field != \"\" && !cast.IsValid() { addWarning(...) }pattern to both callsites.ConversionWarning; notedSeverityCriticalis reserved for future use.IPProtocolgaps).Blocked by: NATS-144 (merged as
33ccf82). Unblocks: NATS-146 (cross-repo integration).Test Plan
pkg/model/coverage ≥ 80% (actual: 98.2%)pkg/parser/opnsense/coverage ≥ 80% (actual: 94.9%)pkg/parser/pfsense/coverage ≥ 80% (actual: 94.1%)just ci-checkpasses cleango test -race ./pkg/model/... ./pkg/parser/...passesinternal/imports in production converter code (verified viagit grep)IsValid()guard on OPNsense outbound NAT IPProtocol causesTestConverter_EnumCast_EmitsWarning/nat_outbound_rule_ip_protocolto fail (tripwire check)len(ipsec.Phase1) == 0early return) causesTestConverter_IPsecEnabled_Gotchas16/phase2_without_phase1_is_orphan_and_disabledto fail (tripwire check)Linked work
docs/plans/2026-04-18-003-refactor-audit-converters-conversion-warning-plan.md(local, gitignored)33ccf82)