fix(pfsense): resolve interfaces and DHCP scopes always reporting as disabled (#461) - #473
Merged
Conversation
Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…r rule engine behavior Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…g for presence-based enablement Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…forks in pfSense Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…shaling for presence-based enablement Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…d types for improved handling Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…lag for presence-based enablement Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…se-specific types for interfaces and DHCP Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…enhance clarity on supported configurations Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…configurations Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…lFlag for presence-based enablement Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…d comprehensive XML marshaling/unmarshaling tests Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…r Interfaces and Dhcpd, and enhance XML unmarshaling for Interface Enable Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…aceLAN in EnablePresenceXML tests Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…nterfaces and Dhcpd with BoolFlag support Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
… isPfSenseValueTrue for accurate boolean evaluation Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
… Interfaces and Dhcpd in pfSense document validation Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
…d pfSense configurations Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
9 tasks
dosubot
Bot
added
the
size:XXL
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughForked pfSense schema types for Interfaces and DHCP to use Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…dhcp-scopes-always-report-as-disabled-fork-interfacedhcpdinterface-to-use-boolflag-for-presence-based-enable
Contributor
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟠 Full CI must passWaiting checks:
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
|
Related Documentation 2 document(s) may need updating based on files changed in this PR: opnDossier Technical Deep-Dive: Architecture, Internals & RoadmapView Suggested Changes@@ -120,7 +120,13 @@
**OPNsense** (via `internal/cfgparser/`): Streaming XML parser reads `config.xml` into `schema.OpnSenseDocument` DTOs with switch-case routing for top-level elements.
-**pfSense** (via `pkg/parser/pfsense/`): Self-contained XML decoding using `parser.NewSecureXMLDecoder()` directly, reads into `pfsense.Document` without cfgparser dependency.
+**pfSense** (via `pkg/parser/pfsense/`): Three-layer XML parsing approach for presence-based boolean handling:
+
+- <!-- §nt:aa111111-2222-3333-4444-555555555555 --> **Layer 1: XML decode** → `decodeDocument` (intermediate type with `pfsense.Interface` and `pfsense.DhcpdInterface` using `opnsense.BoolFlag` for `Enable` fields)
+- <!-- §nt:bb222222-3333-4444-5555-666666666666 --> **Layer 2: Conversion** → `toDocument()` converts `BoolFlag` to backward-compatible `opnsense.Interfaces`/`opnsense.Dhcpd` with `Enable = "1"` (enabled) or `""` (disabled)
+- <!-- §nt:cc333333-4444-5555-6666-777777777777 --> **Layer 3: Standard flow** → Parser converts to `CommonDevice` as before
+
+This decode-layer approach correctly distinguishes `<enable/>` (present = enabled) from absent (disabled) while keeping `pfsense.Document`'s exported API backward-compatible. See GOTCHAS.md §15 for BoolFlag marshaling requirements.
**Shared Security Infrastructure** (via `pkg/parser/xmlutil.go`):
- <!-- §nt:1b4309a8-87d3-4a16-9d12-9de23072e2d7 --> **Security protections**: XXE prevention, XML bomb protection
@@ -237,11 +243,15 @@
| Domain Model | `pkg/model/` | Platform-agnostic `CommonDevice` with JSON/YAML tags. ~87 types. No XML tags. |
| Parser | `pkg/parser/opnsense/`, `pkg/parser/pfsense/` | Converts device-specific XML DTOs to `CommonDevice`. Returns conversion warnings. |
-**pfSense Schema Reuse Pattern** (copy-on-write): The `pkg/schema/pfsense/` package imports and reuses `pkg/schema/opnsense/` types for structurally identical XML sections (interfaces, DHCP, OpenVPN, certificates, VLANs, etc.) and only defines pfSense-specific types at divergence points:
-- NAT inbound rules: pfSense uses `<target>` field, OPNsense uses `<internalip>`
-- User authentication: pfSense stores bcrypt hashes in `<bcrypt-hash>`, OPNsense uses SHA-based `<password>`
-- Filter rules: pfSense adds fields not present in OPNsense (`id`, `tag`, `tagged`, `os`, `associated-rule-id`)
-- DHCPv6: pfSense includes `RAMode` and `RAPriority` configuration
+**pfSense Schema Reuse Pattern** (copy-on-write): The `pkg/schema/pfsense/` package imports and reuses `pkg/schema/opnsense/` types for structurally identical XML sections (OpenVPN, certificates, VLANs, static routes, etc.) and only defines pfSense-specific types at divergence points:
+
+- <!-- §nt:aa888888-9999-aaaa-bbbb-cccccccccccc --> **Interfaces and DHCP scopes**: pfSense uses presence-based `<enable/>` elements; forked `pfsense.Interface` and `pfsense.DhcpdInterface` with `Enable opnsense.BoolFlag` (see `pkg/schema/pfsense/interfaces.go`, `pkg/schema/pfsense/dhcp.go`, and `pkg/parser/pfsense/decode_types.go` for BoolFlag→string conversion)
+- <!-- §nt:bb999999-aaaa-bbbb-cccc-dddddddddddd --> **NAT inbound rules**: pfSense uses `<target>` field, OPNsense uses `<internalip>`
+- <!-- §nt:cc000000-bbbb-cccc-dddd-eeeeeeeeeeee --> **User authentication**: pfSense stores bcrypt hashes in `<bcrypt-hash>`, OPNsense uses SHA-based `<password>`
+- <!-- §nt:dd111111-cccc-dddd-eeee-ffffffffffff --> **Filter rules**: pfSense adds fields not present in OPNsense (`id`, `tag`, `tagged`, `os`, `associated-rule-id`)
+- <!-- §nt:ee222222-dddd-eeee-ffff-000000000000 --> **DHCPv6**: pfSense includes `RAMode` and `RAPriority` configuration
+
+**Validator cascade on type forks** (GOTCHAS.md §9.2): When a `Document` field type changes from `opnsense.*` to `pfsense.*`, the validator function must adapt each item back to the opnsense type before calling shared validators.
<!-- §nt:022ad0b9-47a4-4bbd-829d-c4112e80c8e8 -->
## CommonDevice Structure
@@ -305,6 +315,8 @@
| Value-based boolean (`== "1"` in PHP) | `string` | `<enable>1</enable>`, `<blockpriv>1</blockpriv>` |
| Presence with value access | `*string` | `<any/>` in Source/Destination |
| Container + children | `[]ChildType` in container struct | `VLANs` container with `[]VLAN` slice |
+
+**pfSense BoolFlag Marshaling Pattern** (GOTCHAS.md §15): When forking an opnsense struct to use `BoolFlag` fields, add a private type alias and a pointer-receiver `MarshalXML` method that delegates via `e.EncodeElement((*alias)(&value), start)` to ensure `(*BoolFlag).MarshalXML` is invoked for proper `<enable/>` element serialization. See `pkg/schema/pfsense/interfaces.go` (`interfaceAlias`) and `pkg/schema/pfsense/dhcp.go` (`dhcpdInterfaceAlias`) for implementation examples.
<!-- §nt:2f7ccda1-2240-485f-a691-7544c7c99048 -->
---✅ Accepted xml-structure-research
|
coderabbitai
Bot
added
documentation
coderabbitai
Bot
added
priority:normal
…olFlag usage Replace the intermediate decodeDocument type with direct use of pfsense.Interfaces and pfsense.Dhcpd in Document. Converters now call iface.Enable.Bool() directly instead of isPfSenseValueTrue on pre-converted string values. Validators use pfSense-specific wrappers (validatePfSenseInterfaces, validatePfSenseDhcpd) that adapt to shared field-level validators. Deletes 178 lines of field-by-field copy code in decode_types.go. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
![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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 438-440: The docs currently mix two forms for reading BoolFlag
(e.g., "iface.Enable.Bool()" vs "bool(iface.Enable)"); update the AGENTS.md
wording to use the canonical form used in code—use bool(iface.Enable) and
bool(d.Enable) everywhere; also confirm the sentence about pfsense.Interface
having Enable of type opnsense.BoolFlag matches the implemented API and remove
references to isPfSenseValueTrue() so consumers see only the canonical bool(...)
usage (refer to symbols pfsense.Interface, iface.Enable, d.Enable,
opnsense.BoolFlag, and the validator adapters validatePfSenseInterfaces /
validatePfSenseDhcpd as context).
In `@pkg/schema/pfsense/dhcp.go`:
- Around line 99-123: The UnmarshalXML method for Dhcpd currently wraps any
decoder.Token() error (including io.EOF) as "failed to read Dhcpd token"; update
Dhcpd.UnmarshalXML to check for io.EOF after calling decoder.Token() and return
a clearer error (e.g., "unexpected end of XML while unmarshalling Dhcpd")
instead of the generic wrap, while preserving the existing fmt.Errorf wrapping
for other errors; look for the function Dhcpd.UnmarshalXML, the decoder.Token()
call, and the start xml.StartElement/end element handling to implement this
distinction.
In `@pkg/schema/pfsense/interfaces_test.go`:
- Around line 66-105: Add a brief inline guard comment to
TestInterfacesMarshalXML_EnableProducesPresenceElement explaining that the
test's substring extraction relies on the alphabetical ordering guaranteed by
Interfaces.MarshalXML (which sorts map keys when marshaling the Items map), e.g.
noting that "lan" will appear before "wan" because MarshalXML sorts keys; keep
the existing require.Greater checks but document the ordering guarantee so
future maintainers understand why lanIdx < wanIdx is expected.
In `@pkg/schema/pfsense/interfaces.go`:
- Line 45: The Range field (type opnsense.DhcpRange) currently has a JSON tag
json:"range" but YAML tag yaml:"range,omitempty"; update the struct field tag
for Range to include omitempty in the JSON tag as well (e.g.,
json:"range,omitempty") so JSON serialization matches other optional fields and
avoids emitting zero-value ranges.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55bb608e-ae51-4fc9-905f-72367b67f19f
📒 Files selected for processing (16)
AGENTS.mdGOTCHAS.mddocs/about.mddocs/index.mdinternal/validator/pfsense.gollms.txtmkdocs.ymlpkg/parser/pfsense/converter_network.gopkg/parser/pfsense/converter_services.gopkg/parser/pfsense/decode_types.gopkg/parser/pfsense/parser.gopkg/parser/pfsense/parser_test.gopkg/schema/pfsense/dhcp.gopkg/schema/pfsense/dhcp_test.gopkg/schema/pfsense/interfaces.gopkg/schema/pfsense/interfaces_test.go
coderabbitai
Bot
added
priority:high
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: 1
🤖 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/pfsense/parser_test.go`:
- Around line 984-1061: Extend TestParser_EnablePresenceXML to also include a
DHCP scope in the XML input that lacks the self-closing <enable/> (e.g. add a
<wan> DHCP block without <enable/>), then locate that scope after parsing
(similar to how lanDHCP is found) and add an assertion that its Enabled field is
false; update the test inside TestParser_EnablePresenceXML (use the same device,
device.DHCP iteration logic and variable naming like wanDHCP) to require.NotNil
for the new scope and assert.False(wanDHCP.Enabled).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e37eb68-9d92-41a0-adce-a63a3f07adfe
📒 Files selected for processing (9)
internal/validator/pfsense.gointernal/validator/pfsense_test.gopkg/parser/pfsense/converter_network.gopkg/parser/pfsense/converter_services.gopkg/parser/pfsense/parser.gopkg/parser/pfsense/parser_test.gopkg/schema/pfsense/dhcp_test.gopkg/schema/pfsense/document.gopkg/schema/pfsense/interfaces_test.go
… test Add opt1 DHCP scope without <enable/> to verify absent element correctly produces Enabled: false, locking both sides of the BoolFlag contract. Addresses CodeRabbit review feedback. Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
unclesp1d3r
deleted the
461-fixpfsenseschema-pfsense-interfaces-and-dhcp-scopes-always-report-as-disabled-fork-interfacedhcpdinterface-to-use-boolflag-for-presence-based-enable
branch
March 23, 2026 07:01
coderabbitai
Bot
added
documentation
This was referenced Mar 23, 2026
mergify Bot
pushed a commit
that referenced
this pull request
Apr 11, 2026
Update documentation for #473 This document provides comprehensive research on OPNsense and pfSense config.xml structure, including boolean patterns, source/destination complexity, and firewall rule fields. It documents schema gaps that have been resolved across five implementation phases and establishes the rationale for type decisions (BoolFlag vs string) across the parser, enabling more accurate config parsing and identifying remaining low-priority fields for future work. _Generated by [Dosu](https://dosu.dev)_ Co-authored-by: dosubot[bot] <131922026+dosubot[bot]@users.noreply.github.com>
Closed
This was referenced Apr 17, 2026
Merged
8 tasks
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
1 participant
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
pfsense.Interfaceandpfsense.DhcpdInterfacewithEnable opnsense.BoolFlagto correctly decode pfSense presence-based<enable/>elements (self-closing = enabled, absent = disabled)Document.InterfacesandDocument.Dhcpdnow use the forked pfsense types directly — no intermediate decode layeriface.Enable.Bool()/d.Enable.Bool()directly instead ofisPfSenseValueTruevalidatePfSenseInterfaces,validatePfSenseDhcpd) that adapt to shared field-level validatorsdecode_types.go— 178 lines of field-by-field copy code eliminatedArchitecture
Documentusespfsense.Interfacesandpfsense.DhcpdwithBoolFlagfields. XML decodes directly intoDocument. No intermediate representation.Test Plan
just ci-checkpasses (0 lint issues, all 35 packages green)pfsense.Interface—<enable/>→true, absent →falsepfsense.DhcpdInterface— same cases<enable>elements (not<enable>true</enable>)Interfaces/Dhcpdmaps<enable/>on WAN + DHCP, absent on LANEnabled: trueon interfaces and DHCP scopesCloses #461