fix(xml): disable external entity loader in xml2array paths by somethingwithproof · Pull Request #7067 · Cacti/cacti

somethingwithproof · 2026-04-24T21:14:33Z

Blocks XXE in the two xml2array() entry points in lib/xml.php.

xml_parse_into_struct expands external entities and DTD references by default on PHP 7.4. A crafted <!DOCTYPE> in an imported template, package, or data query XML can read local files or exfiltrate via out-of-band channels. Wrap the parse with libxml_disable_entity_loader(true) and restore the prior state in a finally-equivalent pattern.

PHP 8.0 deprecates and 8.1+ removes the call; the function_exists guard handles that. PHP 8+ disables entity loading by default for libxml consumers, so the shim is a no-op on develop's runtime but keeps the existing guarantee explicit.

Atomic re-file from the closed batch PR #7036. Further pieces from that batch will land as their own small PRs on request.

Files

lib/xml.php — 16 lines added at two call sites
tests/Unit/XmlXxeHardeningTest.php — 36-line Pest test

Verification

php -l lib/xml.php clean
Test file loads lib/xml.php, invokes xml2array() against a sample payload, asserts output shape and that the disable-loader call is present in the source

…(batch 2) (#1) * fix: correct grammar and possessive errors in English source strings (batch 2) Seven errors found via cacti.pot audit: - automation_networks.php: "senders name" → "sender\'s name" (line 839) - automation_networks.php: "senders Email" → "sender\'s Email" (line 848) - automation_templates.php: "devices sysDescr" → "device\'s sysDescr" (line 1060) - automation_templates.php: "devices sysName" → "device\'s sysName" (line 1067) - automation_templates.php: "devices sysOid" → "device\'s sysOid" (line 1074) - data_templates.php: garbled "it either it value...critical data Data Query" → "its value...Data Query" (line 903) - package_repos.php: "Package Repositorites" → "Package Repositories" (line 436) All changes are to UI description strings inside __() / __esc() calls. cacti.pot should be regenerated via build_gettext.sh after merge. * fix: correct grammar and possessive errors in English source strings (batch 2) Seven errors found via cacti.pot audit: - automation_networks.php: "senders name" → "sender\'s name" (line 839) - automation_networks.php: "senders Email" → "sender\'s Email" (line 848) - automation_templates.php: "devices sysDescr" → "device\'s sysDescr" (line 1060) - automation_templates.php: "devices sysName" → "device\'s sysName" (line 1067) - automation_templates.php: "devices sysOid" → "device\'s sysOid" (line 1074) - data_templates.php: garbled string → "its value...Data Query" (line 903) - package_repos.php: "Package Repositorites" → "Package Repositories" (line 436) All changes are to UI description strings inside __() / __esc() calls. cacti.pot should be regenerated via build_gettext.sh after merge. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* Improve IPv6 support in RRDtool proxy and ping utilities - Fix RRDtool proxy to dynamically detect IPv6 addresses and use AF_INET6 sockets instead of hardcoded AF_INET (IPv4-only), enabling connections to IPv6 RRDtool proxy servers including backup servers - Strip brackets from IPv6 addresses before passing to socket_connect - Properly close failed sockets before creating new ones for failover - Replace fragile str_contains(':') IPv6 detection in ping with filter_var(FILTER_FLAG_IPV6) for more robust address validation https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Fix JS code quality: implicit globals, deprecated APIs, loose equality - Add var declarations to prevent implicit globals in install.js (element, enabled, button, buttonCheck) - Remove console.log debug output left in production (install.js) - Replace deprecated jQuery .unbind() with .off() (layout.js) - Fix "depreciated" typo to "deprecated" in deprecation warnings - Convert == / != to === / !== for null, boolean, string, typeof, and numeric comparisons across install.js and realtime.js https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Replace deprecated jQuery shorthand event methods in layout.js Replace .click(), .keyup(), .keydown(), .mousedown(), .mouseenter(), .mouseleave(), .submit(), .resize() with .on() equivalents. Replace .focus(), .change() trigger calls with .trigger(). These shorthands were deprecated in jQuery 3.5. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Replace deprecated jQuery methods in realtime.js Replace .bind() with .on() and .change() trigger calls with .trigger('change'). .bind() was deprecated in jQuery 3.0 and shorthand triggers in jQuery 3.5. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Replace deprecated jQuery methods in install.js and fix ===/!== errors Replace .click(), .change(), .focus() with .on()/.trigger() equivalents. Also fix !=== and ==== operators that were incorrectly introduced by a prior replace-all of == to === within existing !== and === expressions. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Restore == null where needed to catch both null and undefined In JavaScript, == null matches both null and undefined, which is an intentional idiom. The prior === null conversion broke cases where values come from jQuery .val(), .data(), $.urlParam(), or object property access that may return undefined rather than null. Revert those specific cases while keeping === null where variables are explicitly initialized to null. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Replace deprecated jQuery methods in all theme main.js files Replace .unbind().click() with .off('click').on('click'), convert .hover() to .on('mouseenter').on('mouseleave'), replace .change(), .scroll(), .click() shorthands with .on() equivalents, and .blur() with .trigger('blur') across all 10 theme files. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Convert string concatenation to template literals Replace 'str' + var + 'str' patterns with ES6 template literals in realtime.js and install.js. Improves readability especially for URL construction and HTML building. Also replace $.parseJSON() with native JSON.parse() in realtime.js. https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo * Convert var to const for single-assignment variables Replace var with const where the variable is assigned once and never reassigned within its scope, in install.js and realtime.js. Keeps var for variables that are conditionally reassigned (e.g. size, url). https://claude.ai/code/session_01SbuDigvAkYvPKvdcougsdo --------- Co-authored-by: Claude <noreply@anthropic.com>

xml_parse_into_struct resolves entities by default, including external DTD references. Wrap both xml2array entry points in libxml_disable_entity_loader(true) and restore the prior state after the parse. PHP 8+ disables entity loading by default and deprecates the call, so the shim is guarded on function_exists. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot

Pull request overview

This PR aims to harden XML parsing against XXE by modifying Cacti’s XML-to-array helper functions, and adds a unit test for the new behavior. The diff also includes a broad set of unrelated updates (JS event-binding refactors across installer/themes/layout/realtime, RRDtool proxy IPv6/socket handling changes, copy/typo fixes, and new GitHub automation configs).

Changes:

Add libxml entity-loader toggling around the two XML parsing entry points in lib/xml.php, plus a new unit test.
Update RRDtool proxy socket creation/connect logic to better support IPv6 and load-balancing failover.
Refactor numerous JS event bindings (unbind/click/change → off/on) and add Dependabot/CodeQL workflow configs.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`lib/xml.php`	Adds libxml entity-loader toggling around `xml_parse_into_struct` calls in `xml2array()` and `rrdxport2array()`.
`tests/Unit/XmlXxeHardeningTest.php`	Adds a Pest test asserting the hardening call exists and `xml2array()` basic behavior.
`lib/rrd.php`	Updates RRDtool proxy socket initialization for IPv6 and improves failover handling.
`lib/ping.php`	Replaces IPv6 detection heuristic (`str_contains(':')`) with `filter_var(...FILTER_FLAG_IPV6)` in multiple paths.
`install/install.js`	Large event-binding/templating refactor and some const/strictness adjustments in installer JS.
`include/layout.js`	Switches various bindings to `.off()`/`.on()` and adjusts some trigger/focus patterns; fixes “deprecated” spelling in console errors.
`include/realtime.js`	Refactors URL building and event bindings; updates JSON parsing and trigger usage.
`include/themes/sunrise/main.js`	Replaces `unbind()/hover()/change()/click()` with `off()/on()` patterns.
`include/themes/raspberry/main.js`	Replaces `unbind()/hover()` with `off()/on()` patterns.
`include/themes/paw/main.js`	Replaces `unbind()/hover()` with `off()/on()` patterns.
`include/themes/paper-plane/main.js`	Replaces `unbind()/hover()/change()` with `off()/on()` patterns.
`include/themes/modern/main.js`	Replaces `change()/unbind().click()` with `on('change')/off('click').on('click')`.
`include/themes/midwinter/main.js`	Replaces `click()/hover()/change()` with `on(...)` and some trigger adjustments.
`include/themes/hollyberry/main.js`	Replaces `unbind()/hover()` with `off()/on()` patterns.
`include/themes/dark/main.js`	Replaces `change()/hover()` with `on(...)` patterns.
`include/themes/carrot/main.js`	Replaces `unbind()/hover()` with `off()/on()` patterns.
`include/themes/cacti/main.js`	Replaces `unbind()/hover()` with `off()/on()` patterns.
`package_repos.php`	Fixes UI string typo (“Repositorites” → “Repositories”).
`data_templates.php`	Improves wording of a user-facing message.
`automation_templates.php`	Fixes grammar in device rule descriptions (“device’s ...”).
`automation_networks.php`	Fixes grammar (“sender’s ...”) in notification field descriptions.
`.github/workflows/codeql.yml`	Adds a CodeQL workflow for JS/TS and Python.
`.github/dependabot.yml`	Adds Dependabot config for npm and GitHub Actions.

+	if (function_exists('libxml_disable_entity_loader')) {
+		$loader = libxml_disable_entity_loader(true);
+	}
+
 	$p     = xml_parser_create();


+	if (isset($loader)) {
+		libxml_disable_entity_loader($loader);
+	}


+	// Detect address family based on server address for IPv6 support
+	$socket_family = filter_var(trim($server, '[]'), FILTER_VALIDATE_IP, FILTER_FLAG_IPV6) ? AF_INET6 : AF_INET;
+
+	$rrdp_socket = @socket_create($socket_family, SOCK_STREAM, SOL_TCP);
+


+	if (isset($loader)) {
+		libxml_disable_entity_loader($loader);
+	}



 	if ($('#form_graph_view').length) {
-		$('#form_graph_view').filter('input, select').not('#date1, #date2').click(function () {
+		$('#form_graph_view').filter('input, select').not('#date1, #date2').on('click', function () {


+test('xml2array uses libxml_disable_entity_loader', function () {
+	$xmlPath = __DIR__ . '/../../lib/xml.php';
+	$contents = file_get_contents($xmlPath);
+
+	expect($contents)->toContain('libxml_disable_entity_loader(true)');
+});
+


+			// Close the failed socket before creating a new one for the backup server
+			@socket_close($rrdp_socket);
+
 			$rrdp_id = ($rrdp_id + 1) % 2;


 				if (fieldData.hasOwnProperty(propName)) {
 					propValue = fieldData[propName];
 					if (propValue !== undefined) {
-						element = $('#' + propName);
-						if (element != null && element.length > 0) {
+						element = $(`#${propName}`);
+						if (element !== null && element.length > 0) {


 function disableButton(buttonName) {
-	button = $('#button'+buttonName);
-	if (button != null) {
+	button = $(`#button${buttonName}`);
+	if (button !== null) {
 		button.button();


+  - package-ecosystem: "npm"
+    directory: "/"
+    schedule:
+      interval: "weekly"
+    open-pull-requests-limit: 10


somethingwithproof · 2026-04-24T21:38:30Z

Closing. Copilot's first review comment is correct on substance:

xml2array() uses the Expat xml_parser_* APIs, not libxml2.
libxml_disable_entity_loader() only affects libxml2 consumers (DOM/SimpleXML/XMLReader).
Expat does not resolve external entities by default and does not honor the libxml toggle.

So this PR's change is a no-op on the code path it tries to harden. The real XXE surface on Cacti is the DOM/SimpleXML paths elsewhere in the codebase, which should be addressed in their own PR with a behavior-level test that feeds an external-entity payload and asserts it was not resolved.

The extra files that appeared in the diff (dependabot/codeql, themes/*, rrd.php, ping.php, layout.js) were artifacts of my fork's develop being behind upstream when I branched. They were not intended and will not reappear in future re-files; I've now registered upstream and will branch off upstream/develop going forward.

No salvageable change here. Closing rather than rewriting.

somethingwithproof and others added 7 commits March 9, 2026 13:04

ci: add dependabot config with npm and github-actions

eae0c5e

ci: add CodeQL (JS/TS, excludes PHP) with workflow_dispatch

636703d

ci: update CodeQL for javascript-typescript, python

d315e95

fix(ci): use specific runner labels, fix codeql concurrency

c7de355

Copilot AI review requested due to automatic review settings April 24, 2026 21:14

Copilot started reviewing on behalf of somethingwithproof April 24, 2026 21:15 View session

somethingwithproof mentioned this pull request Apr 24, 2026

feat(security): add cacti_* validation helper module (lib/security_validation.php) #7068

Closed

Copilot AI reviewed Apr 24, 2026

View reviewed changes

somethingwithproof closed this Apr 24, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(xml): disable external entity loader in xml2array paths#7067

fix(xml): disable external entity loader in xml2array paths#7067
somethingwithproof wants to merge 7 commits into
Cacti:developfrom
somethingwithproof:fix/xml-xxe-hardening-develop

somethingwithproof commented Apr 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

somethingwithproof commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

somethingwithproof commented Apr 24, 2026

Files

Verification

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

somethingwithproof commented Apr 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants