Repository navigation
Validate configuration window entries before writing them - #129
Merged
Merged
Conversation
The configuration window wrote whatever was typed in the address, netmask, gateway, DNS and search fields to rc.conf and resolv.conf. sysrc does not escape values, and rc.conf is run by the shell as root at boot, so a value holding a quote, $(...) or a backtick ran as root. A newline in a DNS or search field also added lines to resolv.conf. Add NetworkMgr/validate.py, which checks interface names, IPv4 and IPv6 addresses, netmasks, prefix lengths and search domains, and returns them normalized. Save now validates every entry first and shows an error dialog naming the bad field, writing nothing. The update code only uses the checked values. set_rc_conf() also refuses any name or value with characters outside a small safe set, as a last guard. An empty IPv4 gateway now removes defaultrouter instead of writing it empty.
Contributor
Reviewer's GuideThe configuration window now validates and normalizes every applicable network field before saving, blocks invalid or unsafe values from reaching rc.conf or resolv.conf, and applies only the validated settings; blank IPv4 gateways now remove the persisted default route. Sequence diagram for validated network configuration savesequenceDiagram
actor User
participant Window as NetCardConfigWindow
participant Validator as NetworkMgr.validate
participant System as rc.conf and resolv.conf
User->>Window: commit_pending_changes()
Window->>Validator: validated_settings()
alt invalid entry
Validator-->>Window: ValueError
Window->>Window: show_invalid_entry(message)
Window-->>User: Error dialog, nothing written
else valid entries
Validator-->>Window: normalized settings
Window->>System: set_rc_conf(name, value)
Window->>System: write resolv.conf
Window->>System: update_system_ipv6(settings)
end
Flow diagram for safe configuration persistenceflowchart TD
A[User selects Save] --> B["validated_settings()"]
B --> C{All applicable fields valid?}
C -- No --> D["show_invalid_entry(message)"]
D --> E[Keep window open]
C -- Yes --> F[Store normalized values in pending]
F --> G["set_rc_conf(name, value)"]
G --> H{Safe rc.conf name and value?}
H -- No --> I[Refuse write]
H -- Yes --> J[Write rc.conf and resolv.conf]
J --> K{IPv4 gateway empty?}
K -- Yes --> L["remove_rc_conf_var(defaultrouter)"]
K -- No --> M[Persist defaultrouter]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="NetworkMgr/configuration.py" line_range="624" />
<code_context>
- inet = self.ip_input_address_entry.get_text()
- netmask = self.ip_input_mask_entry.get_text()
- defaultrouter = self.ip_input_gateway_entry.get_text()
+ settings = self.pending
+ nic = settings['nic']
if self.method == 'Manual':
</code_context>
<issue_to_address>
**DHCP saves stop partway through**
When the IPv4 method is DHCP, `update_system` reaches `start_static_network` with `inet` and `netmask` unassigned, raising `UnboundLocalError` after the DHCP settings are written and the interface is restarted. IPv6 updates and cleanup are skipped, leaving the user with a partial save.
Keep the DHCP path from passing unassigned locals to `start_static_network`; use the appropriate DHCP operation instead.
Also at `NetworkMgr/configuration.py:627-628`.
</issue_to_address>
### Comment 2
<location path="NetworkMgr/configuration.py" line_range="42" />
<code_context>
+RC_CONF_SAFE_REGEX = re.compile(r'[A-Za-z0-9_.:%/ -]*')
+# An rc.conf variable name. A "." from a vlan name such as em0.10 is let
+# through, sysrc refuses it on its own.
+RC_CONF_NAME_REGEX = re.compile(r'[A-Za-z_][A-Za-z0-9_.]*')
</code_context>
<issue_to_address>
**Dotted interface settings are not saved**
When the selected interface name contains a dot and the user saves settings, `RC_CONF_NAME_REGEX` accepts the dotted variable name, but `sysrc` rejects it and `set_rc_conf` ignores the failure with `check=False`. The live setting is applied but is not persisted, so it is lost on reboot.
Ensure dotted interface settings are persisted successfully, or reject them before applying the live change.
Also at `NetworkMgr/validate.py:17`, `NetworkMgr/configuration.py:43`.
</issue_to_address>
### Comment 3
<location path="NetworkMgr/validate.py" line_range="118" />
<code_context>
+ error = ValueError(f'{field}: "{text}" is not a valid IPv6 address.')
+ address, _, scope = value.partition('%')
+ if scope or value.endswith('%'):
+ if not allow_scope or not INTERFACE_REGEX.fullmatch(scope):
+ raise error
+ try:
</code_context>
<issue_to_address>
**Scoped gateway routes fail**
When the IPv6 gateway scope does not identify the selected interface, `ipv6_address` checks only the scope syntax, then `update_system_ipv6` passes the scope through to `route add` and ignores command failure. The gateway is saved without a usable route, so the user sees an unusable IPv6 gateway.
Validate that the gateway scope identifies the selected interface before saving or applying the gateway.
Also at `NetworkMgr/configuration.py:593-594`.
</issue_to_address>
### Comment 4
<location path="NetworkMgr/validate.py" line_range="186-187" />
<code_context>
+ """
+ domains = text.split()
+ for domain in domains:
+ labels = domain.rstrip('.').split('.')
+ if len(domain) > 253 or not all(LABEL_REGEX.fullmatch(label) for label in labels):
+ raise ValueError(f'Search domains: "{domain}" is not a valid domain name.')
+ joined = ' '.join(domains)
</code_context>
<issue_to_address>
**Malformed search suffixes are accepted**
When a search-domain entry has multiple trailing dots, `search_domains` removes every trailing dot for label validation but returns the original token. An entry such as `example.com..` passes validation and is written to `resolv.conf` as a malformed suffix, so lookups using it fail or behave incorrectly.
Reject multiple trailing dots while allowing at most one optional root dot during validation.
</issue_to_address>
### Comment 5
<location path="NetworkMgr/validate.py" line_range="165" />
<code_context>
+ ValueError: when it is not a whole number from 1 to 128.
+ """
+ value = text.strip()
+ if not PREFIX_REGEX.fullmatch(value) or not 1 <= int(value) <= 128:
+ raise ValueError(f'Prefix Length: "{text}" must be a number from 1 to 128.')
+ return str(int(value))
</code_context>
<issue_to_address>
**Valid IPv6 /0 is rejected**
When a user configures a manual IPv6 address with prefix length 0, `ipv6_prefixlen` rejects 0 even though `start_static_ipv6_network` can pass that valid prefix length to `ifconfig`, so Save refuses the configuration.
Accept prefix lengths from 0 through 128 and update the validation message accordingly.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 5 findings to address first, and if the validation or normalization is wrong, the change can write an invalid interface, route, resolver, or shell-backed rc.conf value and immediately cause a network outage or security exposure. Reverting the code will not restore already-written rc.conf, resolv.conf, or running-system state, although the configuration can be repaired and reapplied.
Blocking findings: NetworkMgr/configuration.py:624, NetworkMgr/configuration.py:42, NetworkMgr/validate.py:118, NetworkMgr/validate.py:187, NetworkMgr/validate.py:165
Switching an interface to DHCP raised UnboundLocalError partway through the save, because the interim start_static_network call used locals that only the manual path set. The greyed out address entries are now checked in DHCP mode too, and the call is skipped when they hold no valid address. rc.subr reads the settings of em0.10 from ifconfig_em0_10, and sysrc refuses a name with a dot, so settings for dotted interfaces were silently not saved. Add validate.rc_conf_interface(), which maps ".-/+" to "_" like get_if_var in network.subr, and use it for every ifconfig_* variable written or read. An IPv6 gateway zone must now name the interface being configured. Search domains with more than one trailing dot are refused, and so is a zero IPv4 netmask.
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.
The configuration window wrote whatever was typed in the address, netmask, gateway, DNS and search fields to rc.conf and resolv.conf. sysrc does not escape values, and rc.conf is run by the shell as root at boot, so a value holding a quote, $(...) or a backtick ran as root. A newline in a DNS or search field also added lines to resolv.conf.
Add NetworkMgr/validate.py, which checks interface names, IPv4 and IPv6 addresses, netmasks, prefix lengths and search domains, and returns them normalized. Save now validates every entry first and shows an error dialog naming the bad field, writing nothing. The update code only uses the checked values. set_rc_conf() also refuses any name or value with characters outside a small safe set, as a last guard. An empty IPv4 gateway now removes defaultrouter instead of writing it empty.
Summary by Sourcery
Validate and safely apply all network configuration entries before persisting or activating them.
Bug Fixes:
Enhancements: