fix: bind SQL values in setup.php as prepared statements

CHANGELOG.md

@@ -3,6 +3,7 @@
 --- 3.0 ---
 
 * security: Require POST for the save, remove, mute and card-order actions so csrf-magic covers them, and only let a dashboard's owner save over it
+* security: Bind the plugin version lookup and the device_remove cleanup deletes as prepared statements instead of building them from the device list
 * bug: Restore alert audio for down and triggered devices; getMonitorSound() looked for sounds in includes/sounds/ after the controller moved, so the audio element was never rendered
 * feature: Replace the single Status column/filter with a multi-select Events filter (additive) and an Events column that shows a short pill per active event (Down, Recovering, Triggered, Down/Triggered M/A, Warn/Alert Ping, Failed Svc), 'None' when clean; the Status scope dropdown now only selects All Monitored / All Devices / Not Monitored / Without Thresholds
 * bug: Centre the tile hostname under the glyph and keep it clickable in the Tiles & Time view

setup.php

@@ -328,7 +328,7 @@ 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 = ?', array('monitor'));
 
 	if ($current != $old) {
 		monitor_setup_table();
@@ -484,9 +484,15 @@ function monitor_device_action_execute($action) {
  * @return array The unmodified $devices array.
  */
 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) . ')');
+	$ids = array_map('intval', array_values($devices));
+
+	if (cacti_sizeof($ids)) {
+		$in = implode(',', array_fill(0, count($ids), '?'));
+
+		db_execute_prepared("DELETE FROM plugin_monitor_notify_history WHERE host_id IN ($in)", $ids);
+		db_execute_prepared("DELETE FROM plugin_monitor_reboot_history WHERE host_id IN ($in)", $ids);
+		db_execute_prepared("DELETE FROM plugin_monitor_uptime WHERE host_id IN ($in)", $ids);
+	}
 
 	return $devices;
 }

tests/Unit/MonitorDeviceHooksTest.php

@@ -59,10 +59,28 @@
 	expect($result)->toBe(array(3, 4));
 
 	foreach ($GLOBALS['__test_db_calls'] as $call) {
-		expect($call['sql'])->toContain('IN(3,4)');
+		expect($call['fn'])->toBe('db_execute_prepared');
+		expect($call['sql'])->toContain('IN (?,?)');
+		expect($call['params'])->toBe(array(3, 4));
 	}
 });
 
+it('binds device ids as integers on device_remove', function () {
+	monitor_device_remove(array('5', '6) OR 1=1 --'));
+
+	foreach ($GLOBALS['__test_db_calls'] as $call) {
+		expect($call['sql'])->not->toContain('OR 1=1');
+		expect($call['params'])->toBe(array(5, 6));
+	}
+});
+
+it('issues no delete on device_remove when no devices were removed', function () {
+	$result = monitor_device_remove(array());
+
+	expect($GLOBALS['__test_db_calls'])->toBeEmpty();
+	expect($result)->toBe(array());
+});
+
 it('adds the monitor breadcrumb entry without disturbing existing ones', function () {
 	$nav = monitor_draw_navigation_text(array('other.php:' => array('title' => 'Other')));
 

tests/Unit/MonitorLifecycleTest.php

@@ -97,7 +97,7 @@
 });
 
 it('updates the stored plugin_config version when it drifts', function () {
-	monitor_test_mock_db('db_fetch_cell', 'plugin_config', '0.0.0');
+	monitor_test_mock_db('db_fetch_cell_prepared', 'plugin_config', '0.0.0');
 
 	monitor_check_upgrade();