fix: ship Net_DNS2 with the plugin again - #345
Conversation
Cacti plugins install by unpacking a tarball, so a runtime Composer dependency is never present on a user's system. vendor/ was not committed and mactrack_resolver.php required its autoloader unconditionally, which made the resolver fatal on every install that had not run composer by hand. Net_DNS2 1.5.5 is bundled under Net/, the version composer.lock already pinned, matching how plugin_flowview ships the same library. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The function came from a 1.2.x security backport and has no equivalent on develop, so mactrack_view_interfaces.php is fatal there. Removing the call would put the RLIKE injection back, so the fallback mirrors core's body. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The plugin already matched core's php-cs-fixer rules without carrying the config, so nothing but the new fallback needed reformatting. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The ARP export called format_mac_address(), which is defined nowhere; the identical line in mactrack_view_macs.php calls mactrack_format_mac(). The ajax graph-settings action lost its handler in 75b4d2b back in 2016 and nothing requests it. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Every branch of mactrack_format_mac() returned early on a known format and the function fell off the end otherwise, so an unset setting rendered null in place of each address. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The resolver CLI entrypoint still require_once()s the bundled Net_DNS2 file without an is_file() guard, so a missing file would fatal before the intended diagnostic can run.
Pull request overview
This PR restores a tarball-friendly dependency model by bundling Net_DNS2 back into the plugin (removing the runtime Composer requirement) and adds compatibility shims/fixes to prevent fatals on newer Cacti core variants.
Changes:
- Bundle Net_DNS2 v1.5.5 under
Net/and update resolver/setup/tests/docs to loadNet/DNS2.phpinstead ofvendor/autoload.php. - Add a guarded fallback implementation of
db_qstr_rlike()for cores that don’t provide it, preserving safe interface filtering. - Remove calls to non-existent/dead handlers (ARP export MAC formatting, AJAX graph-settings action) and add a php-cs-fixer ruleset plus CI job.
File summaries
| File | Description |
|---|---|
| .github/workflows/code-quality.yml | Add php-cs-fixer standards check job and keep lint/phpstan flows consistent with bundled dependency model |
| .gitignore | Ignore php-cs-fixer cache file |
| .php-cs-fixer.php | Add Cacti-core-aligned php-cs-fixer configuration (excluding Net/, tests/, etc.) |
| CHANGELOG.md | Document new fixes/compatibility changes |
| README.md | Remove Composer install step and adjust documented PHP floor |
| composer.json | Remove Composer manifest (no runtime Composer dependency) |
| lib/mactrack_functions.php | Add db_qstr_rlike() fallback and ensure MAC formatting fallback returns the raw MAC |
| mactrack_ajax.php | Remove dead save_graph_settings action dispatch |
| mactrack_resolver.php | Load bundled Net_DNS2 via Net/DNS2.php |
| mactrack_view_arp.php | Use mactrack_format_mac() instead of non-existent format_mac_address() |
| setup.php | Guard and load bundled Net_DNS2 during plugin enablement checks |
| tests/Unit/test_device_type_sql_safety.php | Update assertions to validate bundled Net_DNS2 wiring |
| tests/e2e/bootstrap-mactrack.sh | Update install check to require bundled Net_DNS2 entrypoint |
| tests/e2e/mactrack_smoke.php | Update smoke test to load bundled Net_DNS2 entrypoint |
| Net/LICENSE | Add bundled Net_DNS2 license text |
| Net/DNS2.php | Bundled Net_DNS2 v1.5.5 entrypoint (autoload registration) |
| Net/DNS2/BitMap.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Cache.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Cache/File.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Cache/Shm.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Exception.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Header.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Lookups.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Names.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Notifier.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Packet.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Packet/Request.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Packet/Response.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/PrivateKey.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Question.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/RR.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/RR/A.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/AAAA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/AFSDB.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/AMTRELAY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/ANY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/APL.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/ATMA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/AVC.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CAA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CDNSKEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CDS.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CERT.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CNAME.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/CSYNC.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/DHCID.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/DLV.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/DNAME.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/DNSKEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/DS.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/EID.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/EUI48.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/EUI64.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/HINFO.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/HIP.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/IPSECKEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/ISDN.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/KEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/KX.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/L32.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/L64.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/LOC.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/LP.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/MX.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NAPTR.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NID.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NIMLOC.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NS.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NSAP.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NSEC.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NSEC3.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/NSEC3PARAM.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/OPENPGPKEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/OPT.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/PTR.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/PX.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/RP.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/RRSIG.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/RT.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SIG.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SMIMEA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SOA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SPF.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SRV.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/SSHFP.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TALINK.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TKEY.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TLSA.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TSIG.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TXT.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/TYPE65534.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/URI.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/WKS.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/X25.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/RR/ZONEMD.php | Bundled Net_DNS2 v1.5.5 RR implementation |
| Net/DNS2/Resolver.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Socket.php | Bundled Net_DNS2 v1.5.5 library file |
| Net/DNS2/Updater.php | Bundled Net_DNS2 v1.5.5 library file |
Review details
- Files reviewed: 90/100 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
|
Fixed in 84197f0 — the resolver now guards the load with |
There was a problem hiding this comment.
🟡 Changes recommended
There are unresolved security/documentation issues (notably unsafe unserialize() usage in vendored cache paths and a PHP version floor mismatch in README) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
README.md:33
- README prerequisites state PHP 8.1+, but the repo has a PHP 7.4 compatibility contract (tests/Security/Php74CompatibilityTest.php) and the PR description asserts runtime files parse on 7.4. This mismatch can mislead users about the supported runtime floor.
Net/DNS2/Cache/File.php:176
- Unserialize is used to reload and merge the cache metadata array without restricting classes. Use allowed_classes => false here as well to prevent object instantiation from a tampered cache file.
Net/DNS2/Cache/Shm.php:217 - Unserialize is used to reload and merge the cache metadata array without restricting classes. Use allowed_classes => false to prevent object instantiation if the shared segment is tampered with.
- Files reviewed: 92/105 changed files
- Comments generated: 3
- Review effort level: Lite
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
🔵 Needs a closer look
It introduces a large vendored dependency tree plus multiple runtime/CI behavior changes, so a final human pass is warranted even though the targeted regressions appear addressed.
Review details
- Files reviewed: 93/107 changed files
- Comments generated: 1
- Review effort level: Lite
Three regressions on current
develop, all introduced by #336.1. Missing Composer autoloader. Cacti plugins install by unpacking a tarball, so a runtime Composer dependency is never present on a user's system.
vendor/was not committed, andmactrack_resolver.php:45required its autoloader unconditionally.require_onceon a missing file is fatal, so the friendly "run composer install" message two lines below was unreachable and the resolver died on every install that had not run Composer by hand.setup.phpguarded its own load correctly; the resolver did not. Reported on #342, where Cacti caught the fatal and disabled the plugin.2.
db_qstr_rlike()does not exist before Cacti 1.2.31.mactrack_view_interfaces.php:48calls it. Checked every tag: absent inrelease/1.2.14throughrelease/1.2.30, present only inrelease/1.2.31, and absent on Cactidevelop.INFOdeclarescompat = 1.2.14, so the Interfaces view is an undefined-function fatal on 17 of the 18 supported 1.2 releases and on 1.3. Filed as #347.3. The same call strips the Ports to Ignore regex.
db_qstr_rlike()removes|,{and}, so the shipped default(Vlan|Loopback|Null)becomes(VlanLoopbackNull). Verified against MariaDB:'Vlan10' NOT RLIKE '(Vlan|Loopback|Null)'is 0,'Vlan10' NOT RLIKE '(VlanLoopbackNull)'is 1. Every Vlan, Loopback and Null interface leaks into the Issues views and "Ignored Interfaces" returns nothing. Filed as #347.4. The plugin never activates.
plugin_mactrack_check_config()gates enablement on loading Net_DNS2, whose autoloader uses relative includes and cannot resolve from Cacti's working directory, so a clean install parks at status 2, "needs configuration". The gate did not exist before #336 and flowview does not have one. Filed as #348.Net_DNS2 now ships under
Net/again, at 1.5.5, the versioncomposer.lockalready pinned. The library code is therefore unchanged rather than reverting to the 1.5.0 the old bundle carried.plugin_flowviewships the same library the same way.What the branch does:
Ship Net_DNS2 with the plugin. Bundles 1.5.5, drops
composer.jsonandcomposer.lock, points both load sites atNet/DNS2.php, which self-registers its autoloader. Three tests asserted on the Composer wiring and were updated with it.Quote the Ports to Ignore pattern with
db_qstr()instead ofdb_qstr_rlike(). That keeps the injection fix from fix: harden Mactrack release boundaries #336, leaves the regex intact, and drops the dependency on a helper that only exists in 1.2.31. An earlier commit on this branch added afunction_existsshim for that helper; it is removed again, since not calling it at all is the better answer.To be explicit about what this gives up:
db_qstr_rlike()does two things, escaping and stripping|,{,}to bound regex backtracking. This keeps the escaping and drops the stripping. That is deliberate.mt_ignorePortsis an admin-only setting whose whole purpose is to hold a regular expression, so mangling its metacharacters does not harden it, it disables it. The pre-fix: harden Mactrack release boundaries #336 code interpolated the value with no quoting at all, which was the actual injection;db_qstr()closes that. Binding the value as a query parameter would keep both properties, but$sql_wherehere is a concatenated string consumed bydb_fetch_assoc()in three places, so that is a separate refactor rather than something to fold into a regression fix.Stop gating enablement on the DNS library.
plugin_mactrack_check_config()goes back to its pre-fix: harden Mactrack release boundaries #336 body. The resolver adds the plugin directory toinclude_pathso the bundled autoloader no longer depends onchdir(), and guards itsrequire_oncewithis_file()so the diagnostic below it is reachable.Check coding standards against Cacti's ruleset. Adds
.php-cs-fixer.phpusing core's rules plus a CI job. The plugin already matched them without carrying the config; only the new code needed reformatting.Stop calling functions that do not exist. The ARP export called
format_mac_address(), which is defined nowhere in the plugin or core; the identical line inmactrack_view_macs.php:461callsmactrack_format_mac(). The ajax graph-settings action lost its handler in75b4d2bin 2016 and nothing requests it.Verification:
Disposable Cacti 1.2.31 + MariaDB 10.11 install, driven by
cli/install_cacti.phpand
cli/plugin_manage.php --install --enable:The RLIKE change checked against the running MariaDB:
Also, for the record on the PHP floor: all 67 runtime files parse on 7.4, a
tokenizer sweep of all 428 call sites finds no PHP 8 only builtin, and the
bundled Net_DNS2 1.5.5 instantiates on 7.4. Net_DNS2 aside, nothing here needs
more than 7.4, whatever floor the project settles on.
One note for review. 86 of the 105 changed files are the vendored library. Reviewing only what was written by hand is 19 files and 326 added lines:
Net/README.mdreturns with a provenance record: upstream repository, tagv1.5.5, commitea39ef5a97d5c2b9893a8c35af7b5fd5b0e40bc9, and what changed between it and the 1.5.0 the old bundle carried.README.mdalso drops thecomposer installstep and corrects its stated PHP floor from 8.2 to 8.1, which is what Cacti 1.2 requires and what this plugin's CI already tests.Closes #346, #347, #348.
Closes #347
Closes #348