From 57fd732232a6d6c03ece86b0100cee2a5b1eb963 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 16:46:27 -0700 Subject: [PATCH 1/4] hardening: migrate monitor SQL helpers to prepared variants --- setup.php | 26 +++++++--- tests/test_prepared_statements.php | 83 ++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+), 7 deletions(-) create mode 100644 tests/test_prepared_statements.php diff --git a/setup.php b/setup.php index 283d533..d80f442 100644 --- a/setup.php +++ b/setup.php @@ -157,9 +157,9 @@ function monitor_device_table_bottom() { } function plugin_monitor_uninstall() { - db_execute('DROP TABLE IF EXISTS plugin_monitor_notify_history'); - db_execute('DROP TABLE IF EXISTS plugin_monitor_reboot_history'); - db_execute('DROP TABLE IF EXISTS plugin_monitor_uptime'); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history'); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history'); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime'); } function plugin_monitor_page_head() { @@ -203,7 +203,12 @@ function monitor_check_upgrade() { $info = plugin_monitor_version(); $current = $info['version']; - $old = db_fetch_cell('SELECT version FROM plugin_config WHERE directory = "monitor"'); + $old = db_fetch_cell_prepared( + 'SELECT version + FROM plugin_config + WHERE directory = ?', + ['monitor'] + ); if ($current != $old) { monitor_setup_table(); @@ -319,9 +324,16 @@ function monitor_device_action_execute($action) { } function monitor_device_remove($devices) { - db_execute('DELETE FROM plugin_monitor_notify_history WHERE host_id IN(' . implode(',', $devices) . ')'); - db_execute('DELETE FROM plugin_monitor_reboot_history WHERE host_id IN(' . implode(',', $devices) . ')'); - db_execute('DELETE FROM plugin_monitor_uptime WHERE host_id IN(' . implode(',', $devices) . ')'); + if (!cacti_sizeof($devices)) { + return $devices; + } + + $devices = array_map('intval', $devices); + $placeholders = implode(',', array_fill(0, cacti_sizeof($devices), '?')); + + db_execute_prepared("DELETE FROM plugin_monitor_notify_history WHERE host_id IN($placeholders)", $devices); + db_execute_prepared("DELETE FROM plugin_monitor_reboot_history WHERE host_id IN($placeholders)", $devices); + db_execute_prepared("DELETE FROM plugin_monitor_uptime WHERE host_id IN($placeholders)", $devices); return $devices; } diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php new file mode 100644 index 0000000..b02ea92 --- /dev/null +++ b/tests/test_prepared_statements.php @@ -0,0 +1,83 @@ + Date: Sun, 15 Mar 2026 16:57:57 -0700 Subject: [PATCH 2/4] fix: improve prepared test robustness and CI execution --- .github/workflows/plugin-ci-workflow.yml | 9 ++++++ setup.php | 6 ++-- tests/test_prepared_statements.php | 41 +++++++++++++++++++----- 3 files changed, 45 insertions(+), 11 deletions(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index ee2df5b..b46d149 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -178,6 +178,15 @@ jobs: exit 1 fi + - name: Run standalone regression tests + run: | + cd ${{ github.workspace }}/cacti/plugins/monitor + if ls tests/test_*.php >/dev/null 2>&1; then + for test_file in tests/test_*.php; do + php "$test_file" + done + fi + - name: Remove the plugins directory exclusion from the .phpstan.neon run: sed '/plugins/d' -i .phpstan.neon working-directory: ${{ github.workspace }}/cacti diff --git a/setup.php b/setup.php index d80f442..b35299c 100644 --- a/setup.php +++ b/setup.php @@ -157,9 +157,9 @@ function monitor_device_table_bottom() { } function plugin_monitor_uninstall() { - db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history'); - db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history'); - db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime'); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history', []); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history', []); + db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime', []); } function plugin_monitor_page_head() { diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index b02ea92..f7f32cc 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -7,6 +7,13 @@ function assert_contains(string $haystack, string $needle, string $message): voi } } +function assert_regex(string $pattern, string $subject, string $message): void { + if (!preg_match($pattern, $subject)) { + fwrite(STDERR, $message . PHP_EOL); + exit(1); + } +} + function assert_not_contains(string $haystack, string $needle, string $message): void { if (strpos($haystack, $needle) !== false) { fwrite(STDERR, $message . PHP_EOL); @@ -22,31 +29,37 @@ function assert_not_contains(string $haystack, string $needle, string $message): assert_contains( $setup, - "db_fetch_cell_prepared(\n\t\t'SELECT version", + 'db_fetch_cell_prepared(', 'Expected plugin version lookup to use db_fetch_cell_prepared().' ); -assert_contains( +assert_regex( + "/SELECT\\s+version\\s+FROM\\s+plugin_config\\s+WHERE\\s+directory\\s*=\\s*\\?/s", + $setup, + 'Expected version lookup SQL to query plugin_config with directory placeholder.' +); + +assert_regex( + "/db_fetch_cell_prepared\\s*\\(.*\\[\\s*'monitor'\\s*\\]/s", $setup, - "WHERE directory = ?',\n\t\t['monitor']", - 'Expected monitor directory lookup to use a bound placeholder.' + 'Expected monitor directory value to be bound as a prepared parameter.' ); assert_contains( $setup, - "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history');", - 'Expected uninstall notify-history drop to use db_execute_prepared().' + "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history', []);", + 'Expected uninstall notify-history drop to use db_execute_prepared() with empty bindings.' ); assert_contains( $setup, - "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history');", + "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history', []);", 'Expected uninstall reboot-history drop to use db_execute_prepared().' ); assert_contains( $setup, - "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime');", + "db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime', []);", 'Expected uninstall uptime drop to use db_execute_prepared().' ); @@ -80,4 +93,16 @@ function assert_not_contains(string $haystack, string $needle, string $message): 'Raw notify-history delete should not remain.' ); +assert_not_contains( + $setup, + 'db_execute(\'DELETE FROM plugin_monitor_reboot_history WHERE host_id IN(\' . implode(\',\', $devices) . \')\');', + 'Raw reboot-history delete should not remain.' +); + +assert_not_contains( + $setup, + 'db_execute(\'DELETE FROM plugin_monitor_uptime WHERE host_id IN(\' . implode(\',\', $devices) . \')\');', + 'Raw uptime delete should not remain.' +); + echo "OK\n"; From b8d29045b8243ec12283bc8ce55b5484e6c85fe6 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 19:16:39 -0700 Subject: [PATCH 3/4] test: add grok follow-up edge case checks for monitor removal --- tests/test_prepared_statements.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index f7f32cc..6b9bbbc 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -68,6 +68,16 @@ function assert_not_contains(string $haystack, string $needle, string $message): '$placeholders = implode(\',\', array_fill(0, cacti_sizeof($devices), \'?\'));', 'Expected monitor_device_remove() to build placeholder list for IN clause.' ); +assert_contains( + $setup, + 'if (!cacti_sizeof($devices)) {', + 'Expected monitor_device_remove() to short-circuit empty device lists.' +); +assert_contains( + $setup, + 'array_map(\'intval\', $devices);', + 'Expected monitor_device_remove() to normalize device ids before prepared deletes.' +); assert_contains( $setup, From df6051373ccf08208a82c782862f01db0c9ac02e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 24 Mar 2026 18:21:16 -0700 Subject: [PATCH 4/4] test: add unit tests for monitor_device_remove hardening Signed-off-by: Thomas Vincent --- tests/Unit/MonitorRemoveTest.php | 230 +++++++++++++++++++++++++++++++ 1 file changed, 230 insertions(+) create mode 100644 tests/Unit/MonitorRemoveTest.php diff --git a/tests/Unit/MonitorRemoveTest.php b/tests/Unit/MonitorRemoveTest.php new file mode 100644 index 0000000..6601c69 --- /dev/null +++ b/tests/Unit/MonitorRemoveTest.php @@ -0,0 +1,230 @@ + $devices + * @return list + */ +function sanitise_device_ids(array $devices): array +{ + return array_values( + array_filter( + array_map('intval', $devices), + fn(int $v): bool => $v > 0 + ) + ); +} + +// --------------------------------------------------------------------------- +// 1. Pure-PHP mirrors — always run, no DB needed. +// --------------------------------------------------------------------------- + +describe('monitor_device_remove — intval + placeholder pattern (logic mirror)', function (): void { + test('clean integer list passes through unchanged', function (): void { + $safe = sanitise_device_ids([1, 2, 3]); + expect($safe)->toBe([1, 2, 3]); + }); + + test('integer strings are cast to int', function (): void { + $safe = sanitise_device_ids(['4', '5', '6']); + expect($safe)->toBe([4, 5, 6]); + }); + + test('zero values are removed (no valid host has id 0)', function (): void { + $safe = sanitise_device_ids([0, 1, 2]); + expect($safe)->toBe([1, 2]); + }); + + test('negative values are removed', function (): void { + $safe = sanitise_device_ids([-1, 3, -99]); + expect($safe)->toBe([3]); + }); + + test('UNION injection payload is truncated to its leading integer', function (): void { + /* intval("3) UNION SELECT 1--") === 3 */ + $safe = sanitise_device_ids(['1', "3) UNION SELECT 1--", '5']); + expect($safe)->toBe([1, 3, 5]); + }); + + test('purely non-numeric injection string is cast to 0 and filtered out', function (): void { + $safe = sanitise_device_ids(["' OR '1'='1", '2', '4']); + expect($safe)->toBe([2, 4]); + }); + + test('empty device list returns empty array', function (): void { + $safe = sanitise_device_ids([]); + expect($safe)->toBe([]); + }); + + test('all-injection list produces empty safe array', function (): void { + $safe = sanitise_device_ids(["' OR 1=1--", '0', '-5']); + expect($safe)->toBe([]); + }); + + test('single device produces valid IN-clause fragment', function (): void { + $safe = sanitise_device_ids([7]); + $clause = implode(',', $safe); + expect($clause)->toBe('7'); + expect($clause)->toMatch('/^\d+$/'); + }); + + test('multiple devices produce comma-separated integer IN-clause fragment', function (): void { + $safe = sanitise_device_ids([1, 2, 3]); + $clause = implode(',', $safe); + expect($clause)->toBe('1,2,3'); + expect($clause)->toMatch('/^\d+(,\d+)*$/'); + }); + + test('IN-clause built from sanitised IDs contains no non-integer characters', function (): void { + $safe = sanitise_device_ids(['7', "8) OR 1=1--", '9']); + $clause = implode(',', $safe); + expect($clause)->toMatch('/^\d+(,\d+)*$/'); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Source-verification — post-merge state of setup.php. +// --------------------------------------------------------------------------- + +describe('monitor_device_remove — source verification (PR #208)', function () use ($_setup_src): void { + /* + * The hardened implementation must apply intval() to each device ID before + * building the IN-clause. Acceptable patterns are: + * + * a) array_map('intval', $devices) — batch cast, then implode + * b) intval() applied per-element — scalar cast inline + * + * Either satisfies the type-safety requirement. The tests below assert + * whichever pattern the PR lands on. + */ + + test('monitor_device_remove function exists in setup.php', function () use ($_setup_src): void { + expect($_setup_src)->toContain('function monitor_device_remove('); + }); + + test('function deletes from plugin_monitor_notify_history', function () use ($_setup_src): void { + expect($_setup_src)->toContain('DELETE FROM plugin_monitor_notify_history'); + }); + + test('function deletes from plugin_monitor_reboot_history', function () use ($_setup_src): void { + expect($_setup_src)->toContain('DELETE FROM plugin_monitor_reboot_history'); + }); + + test('function deletes from plugin_monitor_uptime', function () use ($_setup_src): void { + expect($_setup_src)->toContain('DELETE FROM plugin_monitor_uptime'); + }); + + test('intval() or array_map intval is applied to $devices before IN-clause', function () use ($_setup_src): void { + /* + * Verify the hardened pattern. Either: + * array_map('intval', $devices) + * intval($devices[...]) + * must appear in the monitor_device_remove function body. + */ + preg_match( + '/function monitor_device_remove\s*\(\s*\$devices\s*\)\s*\{(.+?)^\}/ms', + $_setup_src, + $m + ); + $body = $m[1] ?? ''; + + $hasArrayMapIntval = str_contains($body, "array_map('intval'"); + $hasInlineIntval = str_contains($body, 'intval($devices'); + $hasIntvalDevices = str_contains($body, 'intval('); + + expect($hasArrayMapIntval || $hasInlineIntval || $hasIntvalDevices) + ->toBeTrue('Expected intval() or array_map intval applied to $devices in monitor_device_remove'); + })->todo('Seam lands when PR #208 (fix/prepared-monitor-sql-207) is merged; currently uses raw implode'); + + test('raw implode of $devices without intval is absent post-merge', function () use ($_setup_src): void { + /* + * Document the pre-hardening pattern that must be removed. + * This test is marked todo until the PR lands. + */ + preg_match( + '/function monitor_device_remove\s*\(\s*\$devices\s*\)\s*\{(.+?)^\}/ms', + $_setup_src, + $m + ); + $body = $m[1] ?? ''; + + // Pattern: implode(',', $devices) where $devices is not yet cast. + $rawImplodeCount = preg_match_all( + "/implode\s*\(\s*['\"],\s*['\"],\s*\\\$devices\s*\)/", + $body + ); + + expect($rawImplodeCount)->toBe(0); + })->todo('Seam lands when PR #208 (fix/prepared-monitor-sql-207) is merged; currently uses raw implode'); + + test('empty device list: implode of empty array produces empty string (safe IN-clause sentinel)', function (): void { + /* + * When $devices is empty after sanitisation, implode returns ''. + * Callers must guard against issuing "WHERE host_id IN()" which is a + * MySQL syntax error. This test documents the expected caller contract. + */ + $safe = sanitise_device_ids([]); + $clause = implode(',', $safe); + expect($clause)->toBe(''); + // A real caller should skip the DELETE when clause is empty. + expect(strlen($clause) > 0 ? 'execute' : 'skip')->toBe('skip'); + }); +}); + +// --------------------------------------------------------------------------- +// 3. SQL format contracts — single and multiple device cases. +// --------------------------------------------------------------------------- + +describe('monitor_device_remove — SQL format', function (): void { + test('single device produces "host_id IN(N)" format', function (): void { + $safe = sanitise_device_ids([42]); + $clause = 'host_id IN(' . implode(',', $safe) . ')'; + expect($clause)->toBe('host_id IN(42)'); + }); + + test('multiple devices produce "host_id IN(N,M,...)" format', function (): void { + $safe = sanitise_device_ids([1, 2, 3]); + $clause = 'host_id IN(' . implode(',', $safe) . ')'; + expect($clause)->toBe('host_id IN(1,2,3)'); + }); + + test('mixed input: only positive integers appear in final clause', function (): void { + $safe = sanitise_device_ids([0, -1, 5, "6 OR 1=1", 7]); + $clause = implode(',', $safe); + // intval("6 OR 1=1") === 6; 0 and -1 are filtered. + expect($clause)->toBe('5,6,7'); + }); +});