refactor(pkg): audit converters and ConversionWarning API — NATS-145

AGENTS.md

@@ -53,10 +53,11 @@ When rules conflict, follow the higher precedence rule.
 ### Rules of Engagement
 
 - **TERM=dumb Support**: Ensure terminal output respects `TERM="dumb"` for CI/automation
-- **No Auto-commits**: Never commit without explicit permission
+- **No Merging**: Never merge without a passing CI check and code review approval on a PR. This must be performed by a human maintainer, not an AI assistant.
+- **Security-First**: All changes must maintain least privilege and undergo security review.
 - **Focus on Value**: Enhance the project's unique value as an OPNsense auditing tool
-- **No Destructive Actions**: No major refactors without explicit permission
 - **Stay Focused**: Avoid scope creep
+- **AI Disclosure**: Always disclose AI usage in PR descriptions, following the [AI Usage Policy](AI_POLICY.md). Be transparent, but brief — no need to list every prompt, just the tools used (e.g., "Used Claude Code (`Claude Opus 4.7 (1M Context)`) for initial draft of detection engine refactor. All code reviewed and tested.").
 
 ### Issue Resolution
 

GOTCHAS.md

@@ -102,6 +102,8 @@ When converting XML schema `string` fields to typed enums (e.g., `common.Firewal
 
 - **Symptom:** Invalid enum values (e.g., `FirewallRuleType("match")`) pass through the pipeline without error, failing silently in downstream `switch` statements.
 - **Prevention:** Call `IsValid()` after every XML-to-enum cast. For `NATOutboundMode`, `LAGGProtocol`, and `VIPMode` there is no downstream validation — the converter cast is the only defense.
+- **Regression tests:** `TestConverter_EnumCast_EmitsWarning` in `pkg/parser/opnsense/converter_enum_cast_test.go` and `pkg/parser/pfsense/converter_enum_cast_test.go` cover every known callsite. When adding a new enum cast, add a row to the table-driven test in the same PR — otherwise the §5.2 defense is invisible.
+- **History:** The NATS-145 audit (2026-04-18) discovered two unguarded `IPProtocol` casts in OPNsense `convertOutboundNATRules` and `convertInboundNATRules` that had been silently passing invalid values through for months. Both were fixed in the same audit with the canonical `if field != "" && !cast.IsValid() { addWarning }` pattern.
 
 ### 5.3 PreRunE Test Commands Must Bind to Real Globals
 
@@ -276,12 +278,15 @@ Both OPNsense and pfSense emit the same liberal truthy vocabulary (`1|on|yes|tru
 
 ## 16. pfSense IPsec Enabled Flag
 
-### 16.1 Converter Must Set Enabled Explicitly
+### 16.1 Phase 1 Is the Gate
 
-`convertIPsec()` must set `common.IPsecConfig.Enabled = true` when Phase1/Phase2 tunnels exist or the mobile client is enabled. Downstream consumers (e.g., `builder_vpn.go`) short-circuit to "No IPsec configuration present" when `Enabled` is false, effectively hiding valid converted tunnel data.
+`convertIPsec()` in `pkg/parser/pfsense/converter_services.go` sets `common.IPsecConfig.Enabled = true` **only when `len(ipsec.Phase1) > 0`**. Phase 2 tunnels and the mobile client configuration hang off Phase 1 in pfSense — without a Phase 1 entry they are functionally inactive, so the converter treats them as orphans: `Enabled` stays `false` and a medium-severity `ConversionWarning` is emitted for each orphan kind (`IPsec.Phase2`, `IPsec.Client`).
 
-- **Symptom:** IPsec tunnels are converted but reports say "No IPsec configuration present."
-- **Fix:** Ensure the converter sets `Enabled: true` whenever meaningful IPsec data exists.
+Downstream consumers (e.g., `builder_vpn.go`) short-circuit to "No IPsec configuration present" when `Enabled` is false — this is the correct behavior for orphan-only data, but breaks silently if the Phase 1 gate is ever weakened.
+
+- **Symptom:** Valid Phase 1 tunnels show as "No IPsec configuration present" in reports (gate broken, `Enabled` stuck at `false`).
+- **Detection:** `TestConverter_IPsecEnabled_Gotchas16` in `pkg/parser/pfsense/converter_ipsec_test.go` is the canonical regression. If that test fails, the gate has drifted.
+- **Fix:** Keep the Phase 1 guard in `convertIPsec` intact. Phase 2 or mobile client without Phase 1 must stay orphan-warned, not implicitly promoted to `Enabled`.
 
 ## 17. HybridGenerator Interface Coupling
 

docs/solutions/logic-errors/opnsense-nat-ipprotocol-enum-cast-missing-guard.md

@@ -0,0 +1,128 @@
+---
+title: OPNsense NAT rule IPProtocol bare enum casts silently passed invalid values
+category: logic-errors
+date: 2026-04-18
+tags: [enum-validation, xml-to-go, converters, nat, asymmetry-drift, gotchas, audit, regression-tests]
+component: pkg/parser/opnsense, pkg/parser/pfsense, pkg/model
+module: pkg/parser/opnsense
+problem_type: logic_error
+severity: high
+symptoms:
+  - Invalid IPProtocol values from OPNsense NAT XML passed silently through the conversion pipeline
+  - No ConversionWarning emitted for unrecognized IPProtocol strings in outbound or inbound NAT rules
+  - Downstream consumers (builders, audit plugins) received invalid enum values without any signal
+root_cause: missing_validation
+resolution_type: code_fix
+issue: NATS-145
+pr: 580
+related_docs:
+  - docs/solutions/logic-errors/cli-flag-wiring-silent-ignore.md
+  - docs/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md
+---
+
+# OPNsense NAT rule `IPProtocol` bare enum casts silently passed invalid values
+
+## Problem
+
+`convertOutboundNATRules` and `convertInboundNATRules` in `pkg/parser/opnsense/converter.go` performed bare `common.IPProtocol(r.IPProtocol)` casts without the mandatory `IsValid()` guard, silently propagating unrecognized IP protocol family strings through the entire conversion pipeline into the `CommonDevice` model. The pattern this violates was already documented in `GOTCHAS.md §5.2` ("Enum Type Casts from XML") — the pfSense equivalents had the guard — but these two OPNsense callsites were never enumerated in the gotcha text and had no regression test, so the gap persisted undetected for months.
+
+## Symptoms
+
+- An OPNsense `config.xml` containing an unrecognized `<ipprotocol>` value on an outbound or inbound NAT rule (future OPNsense extension, hand-edited config, malformed import) produced no warning and no error.
+- The invalid `common.IPProtocol` value propagated silently into `CommonDevice.NAT.OutboundRules[*].IPProtocol` and `.InboundRules[*].IPProtocol`.
+- Downstream `switch` statements on `IPProtocol` fell through to their default branch, producing wrong output with no diagnostic.
+- `ConvertDocument` returned a `nil` error, giving callers no signal that the data was semantically invalid.
+
+## What Didn't Work
+
+**Prose-only GOTCHAS documentation was not a sufficient tripwire.** `GOTCHAS.md §5.2` correctly described the required `IsValid()` pattern since before NATS-145 (the pfSense converter had adopted it; the `DeviceType` enum had adopted it). It explicitly named `NATOutboundMode`, `LAGGProtocol`, and `VIPMode` as callsites requiring the guard. But it did not enumerate the NAT rule `IPProtocol` paths, and there was no regression test that would fail when a new cast site was added without a guard. The documentation told contributors the rule; it did not enforce it.
+
+**NATS-144's parser audit ([PR #575](https://github.com/EvilBit-Labs/opnDossier/pull/575)) did not look inside converter rule-loops.** The NATS-144 pass was scoped to the registry, factory, and per-device parser entry points — godoc completeness, `internal/` import boundary, coverage thresholds. It stopped at the top-level `ConvertDocument` signature and did not descend into the inner `convertOutboundNATRules` / `convertInboundNATRules` loops where the bare casts lived.
+
+**NATS-145's initial planning phase declared the problem solved prematurely.** The planning document ([`docs/plans/2026-04-18-003-refactor-audit-converters-conversion-warning-plan.md`](../../plans/2026-04-18-003-refactor-audit-converters-conversion-warning-plan.md)) checked the six most visible enum-cast sites in the OPNsense converter and reported "all 6 casts already have `IsValid()` guards" — accurate for the sites it checked, but the search targeted top-level switch/convert patterns, not inner per-rule mapping loops. The two NAT rule paths (in `convertOutboundNATRules` and `convertInboundNATRules`) were missed. The PR was scoped as "audit-only: godoc completeness" on that basis.
+
+## Solution
+
+Added the canonical `IsValid()` + warning guard at both OPNsense NAT rule callsites, mirroring the pfSense implementation that was already correct.
+
+### Before (pre-NATS-145, commit `0225387`)
+
+In `convertOutboundNATRules`:
+
+```go
+result = append(result, common.NATRule{
+    ...
+    IPProtocol: common.IPProtocol(r.IPProtocol), // bare cast, no guard
+    ...
+})
+```
+
+In `convertInboundNATRules`:
+
+```go
+result = append(result, common.InboundNATRule{
+    ...
+    IPProtocol: common.IPProtocol(r.IPProtocol), // bare cast, no guard
+    ...
+})
+```
+
+### After (PR #580, commit `a6dc558`)
+
+In `convertOutboundNATRules`:
+
+```go
+ipProto := common.IPProtocol(r.IPProtocol)
+if r.IPProtocol != "" && !ipProto.IsValid() {
+    c.addWarning(
+        fmt.Sprintf("NAT.OutboundRules[%d].IPProtocol", i),
+        r.IPProtocol,
+        "unrecognized IP protocol family",
+        common.SeverityLow,
+    )
+}
+```
+
+In `convertInboundNATRules`:
+
+```go
+ipProto := common.IPProtocol(r.IPProtocol)
+if r.IPProtocol != "" && !ipProto.IsValid() {
+    c.addWarning(
+        fmt.Sprintf("NAT.InboundRules[%d].IPProtocol", i),
+        r.IPProtocol,
+        "unrecognized IP protocol family",
+        common.SeverityLow,
+    )
+}
+```
+
+The canonical pfSense reference is `pkg/parser/pfsense/converter_security.go` — `convertFirewallRules` (firewall rule `IPProtocol`), `convertNAT` (outbound mode + rule `IPProtocol` via `convertOutboundNATRules`), and `convertInboundNATRules` (inbound rule `IPProtocol`) all had the `IsValid()` guard before NATS-145 opened.
+
+## Why This Works
+
+The two-step pattern provides two invariants simultaneously:
+
+- The `r.IPProtocol != ""` check exempts legitimately absent XML elements. When the schema omits the `<ipprotocol>` element entirely, the decoder produces an empty string — a valid "unspecified" state that must not generate noise.
+- The `!ipProto.IsValid()` check catches every string that is non-empty but does not match any enum member. This makes the converter the last line of defense before invalid data enters `CommonDevice`.
+
+`addWarning` accumulates into the `ConversionWarning` slice that `ConvertDocument` returns as its second return value, giving callers actionable diagnostics without forcing a hard error that would abort report generation. Consumers can filter by severity, log, or surface in UI at their discretion.
+
+## Prevention
+
+**Regression test as tripwire.** `TestConverter_EnumCast_EmitsWarning` in `pkg/parser/opnsense/converter_enum_cast_test.go` is a table-driven test with one row per guarded enum callsite. The two new rows — `"nat outbound rule ip protocol"` and `"nat inbound rule ip protocol"` — exercise the fixed code directly. Any future bare cast added to the OPNsense converter that omits the guard has no corresponding row, making the omission visible at PR review time. `TestConverter_EnumCast_EmptyStringDoesNotWarn` protects the empty-string exemption from accidental removal. Equivalent tests exist in the pfSense package.
+
+**Audit technique: asymmetry detection between parallel implementations.** The bug surfaced during code review when guard coverage was compared across the OPNsense and pfSense converters. Both implement the same contract (XML schema → `CommonDevice`) and expose the same `CommonDevice.NAT.*` fields. When one side has a guard and the other does not, that asymmetry is a red flag. When reviewing any new converter code, enumerate every `T(xmlString)` cast and verify its partner in the sibling converter has equivalent coverage.
+
+**Scope decision: fix in-audit rather than defer.** The audit was nominally "audit-only," but the `AGENTS.md` "Code Quality Policy" rule — "Zero tolerance for tech debt. Never dismiss warnings, lint failures, or CI errors as 'pre-existing' or 'not from our changes'" — applied. Filing a follow-up todo would have left the bug in production while documentation accumulated; fixing in-scope closed the gap immediately. The fix landed in the same PR that surfaced it.
+
+**`GOTCHAS.md §5.2` updated with regression-test cross-references and the NATS-145 discovery history.** Future contributors who add a new enum cast are now instructed to add a corresponding row to the table-driven test in the same PR. The history note prevents the institutional-knowledge loss that caused the original gap.
+
+## Related Issues
+
+- [NATS-145 Jira ticket](https://evilbitlabs.atlassian.net/browse/NATS-145) — converter audit epic
+- PR #575 (NATS-144) — sibling audit that established the methodology but did not check inner converter loops
+- PR #577 — liberal boolean parsing; touched `pkg/parser/opnsense/converter.go` on a parallel track, did not surface this bug
+- [`docs/solutions/logic-errors/cli-flag-wiring-silent-ignore.md`](cli-flag-wiring-silent-ignore.md) — same class of silent-ignore defect in a different layer (CLI flag wiring)
+- [`docs/solutions/runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md`](../runtime-errors/liberal-boolean-xml-parsing-opnsense-pfsense.md) — sibling XML-to-Go correctness work on the same converter file
+- `GOTCHAS.md §5.2` — canonical pattern documentation (updated in PR #580 to cross-reference the regression tests)

internal/converter/enrichment.go

@@ -77,10 +77,19 @@ func prepareForExport(data *common.CommonDevice, redact bool) *common.CommonDevi
 // the caller's data. Slice fields that contain sensitive data are deep-copied
 // before redaction.
 //
-// SECURITY NOTE: The following sensitive fields are already excluded by the converter's
-// field mapping and never appear in CommonDevice:
+// SECURITY NOTE: The following sensitive field mappings are vetted:
 //   - OpenVPN TLS keys (schema.OpenVPNServer.TLS, schema.OpenVPNSystem.StaticKeys)
-//   - IPsec pre-shared keys (schema.IPsec.PreSharedKeys)
+//     — excluded by the converter's field mapping and never appear in CommonDevice.
+//   - IPsec pre-shared keys — schema.IPsec.PreSharedKeys IS mapped to
+//     common.IPsecConfig.PreSharedKeys. In the current OPNsense MVC model this
+//     field stores UUID references to the Ipsec/KeyPairs/PreSharedKey MVC model
+//     (not raw key material), so no credential leaks today. If a future schema
+//     revision ever starts storing raw keys in this field, redaction logic
+//     must be added below.
+//   - pfSense IPsecPhase1.PreSharedKey is a scalar raw key but is intentionally
+//     not mapped into common.IPsecPhase1Tunnel — see
+//     pkg/parser/pfsense/converter_services.go convertIPsecPhase1Tunnels and
+//     the TestConverter_IPsecPhase1_PreSharedKeyExclusion regression test.
 //   - WireGuard private keys (only public keys are mapped; PSKs are mapped but redacted below)
 //
 // If new secret fields are added to common.*, they MUST be added here.