Fix copy_dict_to_element() stale children causing alias misassignment#237
Open
djlongy wants to merge 1 commit intopfsensible:masterfrom
Open
Fix copy_dict_to_element() stale children causing alias misassignment#237djlongy wants to merge 1 commit intopfsensible:masterfrom
djlongy wants to merge 1 commit intopfsensible:masterfrom
Conversation
When copy_dict_to_element() processes a list (e.g. DNS resolver hosts), it pairs input items to XML elements by position. If a host with aliases (child <item> elements under <aliases>) swaps position with a host that has no aliases, the scalar assignment branch sets the element text but does not remove the stale child elements. The alias-less host inherits the previous occupant's aliases. Add a guard in the scalar assignment branch: before setting .text, check if the element has child elements left over from a previous dict/list value and remove them. ET.Element.clear() is intentionally avoided because it also resets .tail, which would corrupt pretty-print whitespace. Add a regression test with a dedicated fixture containing two hosts (one with aliases, one without). The test provides hosts in reversed order and asserts that aliases remain with the correct parent after the module rewrites the XML.
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
Fix
copy_dict_to_element()inpfsense.pyto remove stale child elements when an XML element transitions from holding a dict/list (children) to a scalar value. Without this fix, DNS resolver host override aliases bleed into the wrong host record when the host list order changes.Root Cause
copy_dict_to_element()pairs input list items to XML elements by position. When position N inconfig.xmlheld a host with aliases (child<item>elements under<aliases>), and the module receives a host list where position N is now a host without aliases, the scalar assignment branch setsthis_elt.textbut does not remove the stale<item>children. The alias-less host inherits the previous occupant's aliases.How to Reproduce
Setup: Configure DNS resolver with two hosts — "server" (with aliases) at position 0, "other" (no aliases) at position 1.
Trigger: Re-run the module with hosts in reversed order (other first, server second):
Result (before fix): In
config.xml, host "other" at position 0 now has alias1 and alias2 as stale<item>children — they bled from "server" which previously occupied position 0.Result (after fix): Host "other" has no alias items. Host "server" retains its 2 aliases. Correct.
This was reproduced and confirmed on a live pfSense instance.
Fix
In the scalar assignment branch of
copy_dict_to_element(), before setting.text, check if the element has child elements left over from a previous dict/list value and remove them:Test plan
test_dns_resolver_hosts_reorder_aliases_stay— provides hosts in reversed order and asserts aliases stay with the correct parentThis patch was developed with AI coding assistance (Claude Code). All changes were reviewed, tested on a live pfSense instance, and verified by a human maintainer before submission.