fix: guard api_plugin_moveup/movedown against NULL prior/next id (1.2.x backport)

lib/plugins.php

@@ -987,32 +987,42 @@ function api_plugin_moveup($plugin) {
 		WHERE directory = ?',
 		array($plugin));
 
-	if (!empty($id)) {
-		$temp_id = db_fetch_cell('SELECT MAX(id) FROM plugin_config')+1;
-
+	if ($id !== false) {
 		$prior_id = db_fetch_cell_prepared('SELECT MAX(id)
 			FROM plugin_config
 			WHERE id < ?',
 			array($id));
 
-		/* update the above plugin to the prior temp id */
-		db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($temp_id, $prior_id));
-		db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($prior_id, $id));
-		db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($id, $temp_id));
+		/* MAX() on an empty set returns NULL; without a guard, the UPDATE
+		 * below would set id = NULL on the current plugin, which non-strict
+		 * MariaDB/MySQL silently stores as 0, corrupting the primary key. */
+		if ($prior_id !== null) {
+			$temp_id = db_fetch_cell('SELECT MAX(id) FROM plugin_config') + 1;
+
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($temp_id, $prior_id));
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($prior_id, $id));
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($id, $temp_id));
+		}
 	}
 
 	api_plugin_replicate_config();
 }
 
 function api_plugin_movedown($plugin) {
-	$id      = db_fetch_cell_prepared('SELECT id FROM plugin_config WHERE directory = ?', array($plugin));
-	$temp_id = db_fetch_cell('SELECT MAX(id) FROM plugin_config')+1;
-	$next_id = db_fetch_cell_prepared('SELECT MIN(id) FROM plugin_config WHERE id > ?', array($id));
-
-	/* update the above plugin to the prior temp id */
-	db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($temp_id, $next_id));
-	db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($next_id, $id));
-	db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($id, $temp_id));
+	$id = db_fetch_cell_prepared('SELECT id FROM plugin_config WHERE directory = ?', array($plugin));
+
+	if ($id !== false) {
+		$next_id = db_fetch_cell_prepared('SELECT MIN(id) FROM plugin_config WHERE id > ?', array($id));
+
+		/* MIN() on an empty set returns NULL; same NULL->0 corruption risk as moveup. */
+		if ($next_id !== null) {
+			$temp_id = db_fetch_cell('SELECT MAX(id) FROM plugin_config') + 1;
+
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($temp_id, $next_id));
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($next_id, $id));
+			db_execute_prepared('UPDATE plugin_config SET id = ? WHERE id = ?', array($id, $temp_id));
+		}
+	}
 
 	api_plugin_replicate_config();
 }

plugins.php

@@ -235,7 +235,19 @@ function plugins_load_temp_table() {
 
 			db_execute("CREATE TEMPORARY TABLE IF NOT EXISTS $table LIKE plugin_config");
 			db_execute("TRUNCATE $table");
+
+			/* Cacti strips NO_AUTO_VALUE_ON_ZERO on connect (database.php). Without it,
+			 * a row with id=0 in plugin_config (e.g. from a plugin upgrade script) is
+			 * reassigned by AUTO_INCREMENT to the next sequence value, causing a 1062
+			 * collision when another row already holds that id. */
+			$orig_sql_mode = db_fetch_cell('SELECT @@SESSION.sql_mode');
+			$modes = array_filter(array_map('trim', explode(',', (string)$orig_sql_mode)));
+			if (!in_array('NO_AUTO_VALUE_ON_ZERO', $modes, true)) {
+				$modes[] = 'NO_AUTO_VALUE_ON_ZERO';
+			}
+			db_execute_prepared('SET SESSION sql_mode = ?', [implode(',', $modes)]);
 			db_execute("INSERT INTO $table SELECT * FROM plugin_config");
+			db_execute_prepared('SET SESSION sql_mode = ?', array($orig_sql_mode));
 
 			break;
 		} else {

tests/Unit/PluginMoveorderBugTest.php

@@ -0,0 +1,195 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ |                                                                         |
+ | This program is free software; you can redistribute it and/or           |
+ | modify it under the terms of the GNU General Public License             |
+ | as published by the Free Software Foundation; either version 2          |
+ | of the License, or (at your option) any later version.                  |
+ +-------------------------------------------------------------------------+
+ | Cacti: The Complete RRDtool-based Graphing Solution                     |
+ +-------------------------------------------------------------------------+
+*/
+
+/*
+ * Regression tests for two bugs in lib/plugins.php that corrupt plugin_config:
+ *
+ * BUG 1 (moveup/movedown NULL->0 corruption):
+ *   api_plugin_moveup() and api_plugin_movedown() swap plugin order by
+ *   renaming rows via a three-step id rotation (current->temp, prior->current,
+ *   temp->prior). When called at the boundary (first plugin moved up, last
+ *   plugin moved down), the subquery MAX/MIN returns NULL. Cacti strips
+ *   STRICT_TRANS_TABLES from the session SQL mode on every connection; under
+ *   non-strict mode, UPDATE SET id = NULL on a NOT NULL column silently stores
+ *   0, leaving the plugin with a corrupted primary key.
+ *
+ * BUG 2 (plugins_load_temp_table 1062 on id=0 row):
+ *   plugin_config.id is AUTO_INCREMENT. Cacti also strips NO_AUTO_VALUE_ON_ZERO
+ *   on connect. When an id=0 row exists in plugin_config (caused by bug 1 or a
+ *   plugin upgrade script), the bulk INSERT INTO temp SELECT * FROM plugin_config
+ *   reassigns the 0 to the next AUTO_INCREMENT sequence value, colliding with
+ *   whatever row already holds that id and producing ERROR 1062.
+ */
+
+$libPluginsPath = __DIR__ . '/../../lib/plugins.php';
+$pluginsPath    = __DIR__ . '/../../plugins.php';
+
+// ---------------------------------------------------------------------------
+// api_plugin_moveup
+// ---------------------------------------------------------------------------
+
+test('api_plugin_moveup has $prior_id !== null guard around the three-step swap', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_moveup(');
+	$guard_pos = strpos($source, 'if ($prior_id !== null)', $fn_pos);
+
+	expect($fn_pos)->not->toBeFalse('api_plugin_moveup not found');
+	expect($guard_pos)->not->toBeFalse('$prior_id !== null guard missing from api_plugin_moveup');
+});
+
+test('api_plugin_moveup swap executes only after the prior_id guard, not before', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_moveup(');
+	$guard_pos = strpos($source, 'if ($prior_id !== null)', $fn_pos);
+	$swap_pos  = strpos($source, 'UPDATE plugin_config SET id = ? WHERE id = ?', $fn_pos);
+
+	expect($guard_pos)->not->toBeFalse();
+	expect($swap_pos)->not->toBeFalse();
+	// The first UPDATE must come after the null guard.
+	expect($swap_pos)->toBeGreaterThan($guard_pos);
+});
+
+test('api_plugin_moveup temp_id computation is inside the prior_id guard', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos      = strpos($source, 'function api_plugin_moveup(');
+	$guard_pos   = strpos($source, 'if ($prior_id !== null)', $fn_pos);
+	$temp_id_pos = strpos($source, '$temp_id = db_fetch_cell(\'SELECT MAX(id) FROM plugin_config\')', $fn_pos);
+
+	expect($guard_pos)->not->toBeFalse();
+	expect($temp_id_pos)->not->toBeFalse('$temp_id not found in moveup');
+	// temp_id must be computed after the guard, not before the NULL check.
+	expect($temp_id_pos)->toBeGreaterThan($guard_pos);
+});
+
+test('api_plugin_moveup does not assign $prior_id to any id column before the null guard', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_moveup(');
+	$guard_pos = strpos($source, 'if ($prior_id !== null)', $fn_pos);
+
+	// Extract the slice between function start and the guard; it must contain
+	// no UPDATE statement, because any UPDATE before the guard would set id
+	// to a potentially NULL $prior_id value.
+	$pre_guard = substr($source, $fn_pos, $guard_pos - $fn_pos);
+	expect($pre_guard)->not->toContain('UPDATE plugin_config SET id');
+});
+
+// ---------------------------------------------------------------------------
+// api_plugin_movedown
+// ---------------------------------------------------------------------------
+
+test('api_plugin_movedown has outer $id !== false guard', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos   = strpos($source, 'function api_plugin_movedown(');
+	$id_guard = strpos($source, 'if ($id !== false)', $fn_pos);
+
+	expect($fn_pos)->not->toBeFalse();
+	expect($id_guard)->not->toBeFalse('$id !== false guard missing from api_plugin_movedown');
+});
+
+test('api_plugin_movedown has $next_id !== null guard around the three-step swap', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_movedown(');
+	$guard_pos = strpos($source, 'if ($next_id !== null)', $fn_pos);
+
+	expect($fn_pos)->not->toBeFalse();
+	expect($guard_pos)->not->toBeFalse('$next_id !== null guard missing from api_plugin_movedown');
+});
+
+test('api_plugin_movedown swap executes only after the next_id guard, not before', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_movedown(');
+	$guard_pos = strpos($source, 'if ($next_id !== null)', $fn_pos);
+	$swap_pos  = strpos($source, 'UPDATE plugin_config SET id = ? WHERE id = ?', $fn_pos);
+
+	expect($guard_pos)->not->toBeFalse();
+	expect($swap_pos)->not->toBeFalse();
+	expect($swap_pos)->toBeGreaterThan($guard_pos);
+});
+
+test('api_plugin_movedown does not assign $next_id to any id column before the null guard', function () use ($libPluginsPath) {
+	$source = file_get_contents($libPluginsPath);
+
+	$fn_pos    = strpos($source, 'function api_plugin_movedown(');
+	$guard_pos = strpos($source, 'if ($next_id !== null)', $fn_pos);
+
+	$pre_guard = substr($source, $fn_pos, $guard_pos - $fn_pos);
+	expect($pre_guard)->not->toContain('UPDATE plugin_config SET id');
+});
+
+// ---------------------------------------------------------------------------
+// plugins_load_temp_table sql_mode save/restore
+// ---------------------------------------------------------------------------
+
+test('plugins_load_temp_table saves @@SESSION.sql_mode before adding NO_AUTO_VALUE_ON_ZERO', function () use ($pluginsPath) {
+	$source = file_get_contents($pluginsPath);
+
+	$fn_pos   = strpos($source, 'function plugins_load_temp_table()');
+	$save_pos = strpos($source, '$orig_sql_mode = db_fetch_cell(\'SELECT @@SESSION.sql_mode\')', $fn_pos);
+	// Anchor on the PHP-side mode append, not the SET SESSION statement, so
+	// the order check is not confused by the restore call that also contains
+	// 'SET SESSION sql_mode'.
+	$set_pos  = strpos($source, "\$modes[] = 'NO_AUTO_VALUE_ON_ZERO'", $fn_pos);
+
+	expect($fn_pos)->not->toBeFalse();
+	expect($save_pos)->not->toBeFalse('$orig_sql_mode save not found in plugins_load_temp_table');
+	expect($set_pos)->not->toBeFalse('NO_AUTO_VALUE_ON_ZERO mode append not found in plugins_load_temp_table');
+	// Save must come before the mode append.
+	expect($save_pos)->toBeLessThan($set_pos);
+});
+
+test('plugins_load_temp_table inserts into temp table while NO_AUTO_VALUE_ON_ZERO is active', function () use ($pluginsPath) {
+	$source = file_get_contents($pluginsPath);
+
+	$fn_pos      = strpos($source, 'function plugins_load_temp_table()');
+	$nav_pos     = strpos($source, 'NO_AUTO_VALUE_ON_ZERO', $fn_pos);
+	$insert_pos  = strpos($source, 'INSERT INTO $table SELECT * FROM plugin_config', $fn_pos);
+
+	expect($nav_pos)->not->toBeFalse();
+	expect($insert_pos)->not->toBeFalse('bulk INSERT not found in plugins_load_temp_table');
+	// INSERT must come after the mode is set.
+	expect($insert_pos)->toBeGreaterThan($nav_pos);
+});
+
+test('plugins_load_temp_table restores original sql_mode after the bulk INSERT', function () use ($pluginsPath) {
+	$source = file_get_contents($pluginsPath);
+
+	$fn_pos       = strpos($source, 'function plugins_load_temp_table()');
+	$insert_pos   = strpos($source, 'INSERT INTO $table SELECT * FROM plugin_config', $fn_pos);
+	$restore_pos  = strpos($source, 'SET SESSION sql_mode = ?', $fn_pos);
+
+	expect($insert_pos)->not->toBeFalse();
+	expect($restore_pos)->not->toBeFalse('sql_mode restore not found in plugins_load_temp_table');
+	// Restore must come after the INSERT.
+	expect($restore_pos)->toBeGreaterThan($insert_pos);
+});
+
+test('plugins_load_temp_table restore uses db_execute_prepared with $orig_sql_mode', function () use ($pluginsPath) {
+	$source = file_get_contents($pluginsPath);
+
+	$fn_pos = strpos($source, 'function plugins_load_temp_table()');
+
+	// The restore call must bind $orig_sql_mode, not a hardcoded string, so
+	// the session mode is returned to exactly what it was before the copy.
+	$restore_slice_start = strpos($source, 'SET SESSION sql_mode = ?', $fn_pos);
+	$restore_slice       = substr($source, $restore_slice_start, 120);
+
+	expect($restore_slice)->toContain('$orig_sql_mode');
+});