refactor: safe PHP 7.4 modernization by somethingwithproof · Pull Request #218 · Cacti/plugin_weathermap

somethingwithproof · 2026-04-09T21:16:32Z

This PR adds strict typing, short array syntax, and null coalescing operators across the plugin. Standalone infrastructure files were removed per architectural mandate.

Copilot

Pull request overview

This PR attempts a broad PHP “modernization” across the Weathermap Cacti plugin by adding declare(strict_types=1); to many entrypoints/classes, converting some legacy array syntax in comments, and introducing security documentation plus initial test/static-analysis scaffolding.

Changes:

Added declare(strict_types=1); across many plugin PHP files.
Introduced security documentation (SECURITY.md, SECURITY-AUDIT.md, BACKLOG.md) and new security-focused tests under tests/Security/.
Added tool configuration files (phpunit.xml, phpstan.neon, infection.json) to support testing and static analysis.

Reviewed changes

Copilot reviewed 66 out of 66 changed files in this pull request and generated 27 comments.

Show a summary per file

File	Description
weathermap-cacti-rebuild.php	Adds strict types declaration.
weathermap-cacti-plugin.php	Adds strict types declaration; minor comment-only array syntax update.
weathermap-cacti-plugin-mgmt.php	Adds strict types declaration; introduces invalid `is_[...]` syntax in runtime logic.
weathermap-cacti-plugin-editor.php	Adds strict types declaration; introduces invalid `in_[...]` syntax in runtime logic.
setup.php	Adds strict types declaration; introduces invalid `is_[...]` syntax in poller output logic.
lib/WeatherMap.class.php	Adds strict types declaration; introduces invalid `in_[...]`/`is_[...]` syntax in core engine logic.
lib/WeatherMap.functions.php	Adds strict types declaration; introduces invalid `is_[...]` syntax in logging helpers.
lib/poller-common.php	Adds strict types declaration; introduces invalid `is_[...]` syntax in poller map iteration.
lib/datasources/WeatherMapDataSource_rrd.php	Adds strict types declaration; introduces invalid `in_[...]` syntax in datasource logic.
lib/datasources/WeatherMapDataSource_dsstats.php	Adds strict types declaration; introduces invalid `in_[...]` syntax in datasource logic.
lib/datasources/WeatherMapDataSource_cactithold.php	Adds strict types declaration; introduces invalid `in_[...]`/`is_[...]` syntax in datasource logic.
lib/WMVector.class.php	Adds strict types declaration.
lib/WMPoint.class.php	Adds strict types declaration.
lib/WMLine.class.php	Adds strict types declaration.
lib/WeatherMapNode.class.php	Adds strict types declaration; comment-only array syntax updates.
lib/WeatherMapLink.class.php	Adds strict types declaration; comment-only array syntax update.
lib/WeatherMap.keywords.inc.php	Adds strict types declaration.
lib/pre/WeatherMapPreProcessorExample.php	Adds strict types declaration.
lib/pre/index.php	Adds strict types declaration.
lib/post/WeatherMapPostProcessorExample.php	Adds strict types declaration.
lib/post/index.php	Adds strict types declaration.
lib/index.php	Adds strict types declaration.
lib/HTML_ImageMap.class.php	Adds strict types declaration.
lib/geometry.php	Adds strict types declaration.
lib/editor.inc.php	Adds strict types declaration.
lib/editor.actions.php	Adds strict types declaration.
lib/ds-common.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_wmdata.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_time.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_tabfile.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_static.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_snmp3.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_snmp2c.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_snmp.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_mrtg.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_fping.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_cactihost.php	Adds strict types declaration.
lib/datasources/WeatherMapDataSource_cacti.php	Adds strict types declaration.
lib/datasources/index.php	Adds strict types declaration.
index.php	Adds strict types declaration.
check.php	Adds strict types declaration.
cli/index.php	Adds strict types declaration.
cli/upgrade_configs.php	Adds strict types declaration.
cli/map-tidyup.php	Adds strict types declaration.
cli/convert-to-dsstats.php	Adds strict types declaration.
cli/cacti-mapper.php	Adds strict types declaration.
cli/cacti-integrate.php	Adds strict types declaration.
cli/bristle.php	Adds strict types declaration.
configs/index.php	Adds strict types declaration to redirect stub.
docs/index.php	Adds strict types declaration to redirect stub.
output/index.php	Adds strict types declaration to redirect stub.
images/index.php	Adds strict types declaration.
images/backgrounds/index.php	Adds strict types declaration.
images/icons/index.php	Adds strict types declaration.
images/objects/index.php	Adds strict types declaration.
locales/index.php	Adds strict types declaration.
locales/LC_MESSAGES/index.php	Adds strict types declaration.
locales/po/index.php	Adds strict types declaration.
SECURITY.md	Adds security policy document.
SECURITY-AUDIT.md	Adds security audit report document.
BACKLOG.md	Adds backlog document derived from security audit.
tests/Security/PathTraversalTest.php	Adds new “surface” tests (currently stubbed).
tests/Security/CommandInjectionTest.php	Adds new “surface” tests (currently not exercising production code).
phpunit.xml	Adds PHPUnit config (currently points to missing bootstrap/vendor paths).
phpstan.neon	Adds PHPStan config (currently references missing vendor rules and missing `src/` path; PHP version mismatch).
infection.json	Adds Infection config (currently targets missing `src/`).

Comments suppressed due to low confidence (1)

lib/datasources/WeatherMapDataSource_cactithold.php:108

in_['thold_data', $tables, true] is invalid PHP syntax and will cause a parse error. Replace with in_array('thold_data', $tables, true).

Copilot · 2026-04-09T21:21:43Z

+	if (is_[$maps]) {
 		foreach ($maps as $map) {


is_[$maps] is invalid PHP syntax and will cause a parse error. This should be the standard array check (e.g., is_array($maps)) before iterating.

Copilot · 2026-04-09T21:21:44Z

+	if (is_[$maps]) {
 		foreach ($maps as $map) {


is_[$maps] is invalid PHP syntax and will cause a parse error. Replace with an actual array check (e.g., is_array($maps)) before the foreach.

Copilot · 2026-04-09T21:21:44Z

+		if (is_[$result] && sizeof($result) > 0) {
 			$name  = $result[0]['optname'];


is_[$result] is invalid PHP syntax and will cause a parse error. This needs to be an is_array($result) (or equivalent) check before sizeof($result) is used.

Copilot · 2026-04-09T21:21:44Z

 foreach ($map->used_images as $im) {
-	if (!in_array($im, $image_list, true)) {
+	if (!in_[$im, $image_list, true]) {
 		$image_list[] = $im;


in_[$im, $image_list, true] is invalid PHP syntax and will cause a parse error. This should be in_array($im, $image_list, true) to correctly check for membership.

Copilot · 2026-04-09T21:21:45Z

 	foreach (array_keys($rrd_update_array) as $key) {
-		if (isset($rrd_update_array[$key]['times']) && is_array($rrd_update_array[$key]['times'])) {
+		if (isset($rrd_update_array[$key]['times']) && is_[$rrd_update_array[$key]['times']]) {
 			if ($debug) {


is_[$rrd_update_array[$key]['times']] is invalid PHP syntax and will cause a parse error. This needs to be a real array check (e.g., is_array(...)) before indexing into it.

Copilot · 2026-04-09T21:21:49Z

+<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+         xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd"
+         bootstrap="tests/Helpers/GlobalStubs.php"


xsi:noNamespaceSchemaLocation points into vendor/phpunit/..., but this repo does not include a vendor/ directory or composer metadata to install it. If PHPUnit is expected to run in this repo, add the dependency management (e.g., composer) or reference a schema path that will exist in CI/dev setups.

Copilot · 2026-04-09T21:21:50Z

@@ -0,0 +1,8 @@
+{
+    "timeout": 10,
+    "source": {"directories": ["src"]},


Infection is configured to mutate src/, but there is no src/ directory in this repo. Update the source.directories to the actual code locations or remove this config until the src/ layout exists.

Suggested change

"source": {"directories": ["src"]},

Copilot · 2026-04-09T21:21:50Z

+ *     3. Must not contain '\'  (Windows path traversal defense-in-depth — added fix)
+ *     4. Passes through wm_editor_sanitize_uri (strips dangerous URI chars)
+ *
+ *   PHP 8.4 is required, so null-byte injection risk (PHP < 8.0) is absent.
+ */


This test file states “PHP 8.4 is required”, but the repo documentation does not establish PHP 8.4 as a requirement (and the PR is framed as PHP 7.4 modernization). Please align this comment with the actual minimum supported PHP version to avoid misleading security guidance.

Copilot · 2026-04-09T21:21:50Z

+    /**
+     * Reimplementation of wm_editor_sanitize_conffile logic for isolated testing.
+     * Mirrors the production function including the backslash fix from WM-PATH-01.
+     */
+    function sanitize_conffile_stub(string $filename): string
+    {
+        // Must end in .conf
+        if (substr($filename, -5, 5) !== '.conf') {
+            return '';
+        }


These tests reimplement the sanitizer logic in a stub instead of calling the production wm_editor_sanitize_conffile function. That means the test can stay green even if the real implementation regresses. Prefer including the production file and testing the real function (with minimal stubbing of dependencies if needed).

Copilot · 2026-04-09T21:21:50Z

+describe('Weathermap fping command injection (WM-CMD-01)', function (): void {
+    it('demonstrates that $target from map config flows into popen without escaping (pre-fix behaviour)', function (): void {
+        // WeatherMapDataSource_fping.php:103 (pre-fix):
+        //   $command = $this->fping_cmd . " -t100 -r1 -p20 -u -C $ping_count -i10 -q $target 2>&1";
+        //   $pipe = popen($command, 'r');
+        //
+        // A crafted target value with shell metacharacters produces an injectable command.
+        $fping_cmd  = '/usr/local/sbin/fping';
+        $ping_count = 5;
+        $target     = '192.0.2.1; id > /tmp/wm_pwned';
+
+        $command = $fping_cmd . " -t100 -r1 -p20 -u -C $ping_count -i10 -q $target 2>&1";
+
+        expect($command)->toContain('; id > /tmp/wm_pwned');
+
+        // Fixed form: validate then escapeshellarg.
+        $safe_command = $fping_cmd
+            . ' -t100 -r1 -p20 -u -C ' . (int) $ping_count
+            . ' -i10 -q ' . escapeshellarg($target)
+            . ' 2>&1';


These “command injection” tests only validate string-building in the test itself and do not exercise the actual production command-building code paths in WeatherMapDataSource_fping.php / WeatherMapDataSource_rrd.php. Consider refactoring command assembly into a callable helper (or seam) and asserting against that, so the test detects real regressions.

Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof · 2026-04-11T00:10:50Z

Converted to draft to serialize the stack in this repo. Blocked by #215; will un-draft after that merges to avoid cross-PR merge conflicts.

somethingwithproof · 2026-05-17T10:07:21Z

Mass strict-typing refactor is a separate concern from the canonical bug-fix (#226) and hardening (#227) PRs. Closing to consolidate.

somethingwithproof added 2 commits April 9, 2026 13:59

refactor: add strict typing and clean up standalone infra

78b6aa1

refactor: safe PHP 7.4 modernization (arrays, null coalescing)

2d851b3

Copilot AI review requested due to automatic review settings April 9, 2026 21:16

Copilot started reviewing on behalf of somethingwithproof April 9, 2026 21:17 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

somethingwithproof added 3 commits April 9, 2026 22:36

fix: restore corrupted function calls from refactor tool

fc4086b

Revert bulk array()->[] rewrite damage affecting: - is_array, in_array, xml2array - call_user_func_array, filter_var_array - Function declarations with _array suffix Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

fix: restore corrupted array access and is_array calls

cc9bf96

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

style: remove trailing double semicolons

90f2f62

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof marked this pull request as draft April 11, 2026 00:10

fix(security): normalize editor actions at entrypoint

83e7cf6

somethingwithproof closed this May 17, 2026

		if (is_[$result] && sizeof($result) > 0) {
		$name = $result[0]['optname'];

Conversation

somethingwithproof commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

somethingwithproof commented Apr 11, 2026

Uh oh!

somethingwithproof commented May 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants