fix: three regressions from #342 and #334 - #354
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🟡 Changes recommended
xform_mac_address() still uses default trim() which strips NUL bytes and can corrupt binary SNMP MACs ending in 00, and the test suite should cover that edge case.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Fixes three regressions introduced by recent mactrack changes and observed on a real Cacti 1.2.31 install: (1) reverse-DNS “NODATA” results causing infinite re-query loops, (2) empty hardware addresses being stored as a placeholder string instead of remaining empty, and (3) port-name filtering breaking on punctuation and URL query-string characters.
Changes:
- Make PTR resolution robust to NOERROR/NODATA responses and ensure unresolved IPs are stored in a way that prevents infinite re-processing.
- Restore historical behavior for empty MAC addresses (store as empty string) and add/adjust tests around
xform_mac_address(). - Preserve punctuation in
port_name_filterand URL-encode it in the JS filter application logic.
File summaries
| File | Description |
|---|---|
tests/Pest/Unit/XformMacAddressTest.php |
Switch to testing the real xform_mac_address() and update expectations for empty-MAC behavior. |
mactrack_view_macs.php |
Stop sanitizing punctuation out of port_name_filter and URL-encode the value in filter requests. |
mactrack_view_dot1x.php |
Same port_name_filter punctuation preservation and URL-encoding as the MACs view. |
mactrack_resolver.php |
Guard PTR response dereference and normalize unresolved outcomes to avoid endless retries. |
lib/mactrack_h3c_3com.php |
Avoid indexing missing MAC entries and skip DB lookups when MAC is empty. |
lib/mactrack_functions.php |
Restore empty-MAC transformation to '' (instead of a placeholder). |
CHANGELOG.md |
Document the three regression fixes. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| $mac_address = trim((string) $mac_address); | ||
|
|
||
| // An interface with no hardware address stores an empty string, not the | ||
| // placeholder the dead branch below used to build and throw away. | ||
| if ($mac_address === '') { |
| test('binary hex bytes convert to an uppercase hex string', function () { | ||
| expect(xform_mac_address(hex2bin('aabbccddeeff')))->toBe('AABBCCDDEEFF'); | ||
| }); |
Three regressions found while verifying #345 against a real Cacti 1.2.31 install. All three are fallout from recent merges, and each is filed separately with its evidence.
Merge order: this goes after #345. That PR restores
Net/, without which the resolver cannot load at all, and it touches a different part ofmactrack_resolver.php. The two merge cleanly but this branch is pointless before it.Addresses with no PTR record are re-queried forever, #353
mactrack_resolver.phpdid$resp->answer[0]->ptrdnamewith no guard. A NOERROR response with an empty answer section is the normal reply for an address with no PTR record, and it is not an exception, so thecatch (Net_DNS2_Exception $e)never ran. Against a live resolver with the bundled 1.5.5:The row was then stored with an empty
dns_hostname, which the selection query eleven lines above picks straight back up, so the same address was queried on every pass. Internal ranges without reverse DNS are the common case.Newly reachable: before #342 the
$use_resolverbranches were inverted, so anyone with nameservers configured fell through togethostbyaddr()and never entered this path.The same block had a second fault. The
catchbranch assignedgethostbyaddr()directly and, unlike theelsebranch below it, never fell back to the address when that returnedfalse, so those rows looped too. Both now fall back the same way.An empty hardware address is stored as the string
NOT USER, #350xform_mac_address()returned''for empty input for twenty years, because the trailingstr_replace()overwrote the placeholder the dead branch built. #342 removed the overwrite and the placeholder became live.mac_track_interfaces.ifPhysAddressis a MAC column, and a VLAN SVI, loopback or tunnel reports an empty address, so the Interfaces view and its CSV export showNOT USERwhere the cell used to be blank.The port name filter drops punctuation, #349
#334 put
port_name_filterthroughsanitize_search_string, which deletes( )and replaces, + [ ] { }and others with spaces:The value feeds
mtp.port_name = db_qstr(...)and aLIKEprefix, so an exact match on a description containing punctuation could never succeed. All ten consumers acrossmactrack_view_macs.phpandmactrack_view_dot1x.phpwere already bound throughdb_qstr()before #334 (commitf02d27d), so the sanitiser added no protection, only breakage. Reverted toFILTER_DEFAULTfor this field;ip_filterandmac_filterkeep theirs, since:,-,.and digits survive intact.Verification
Disposable Cacti 1.2.31 + MariaDB 10.11, installed with
cli/install_cacti.phpandcli/plugin_manage.php:The new test was checked against a reverted fix before being trusted: