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 283d533..b35299c 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/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'); + }); +}); diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php new file mode 100644 index 0000000..6b9bbbc --- /dev/null +++ b/tests/test_prepared_statements.php @@ -0,0 +1,118 @@ +