fix: address TheWitness review feedback on PR Cacti#283

- Remove $uniqueID filter from syslog_remove query (incorrectly
  filtered removal rules by random batch marker)
- Reorder CI workflow: lint/PHPStan before integration tests
- Switch echo to print in syslog_batch_transfer.php

Signed-off-by: Thomas Vincent <[email protected]>

Author: somethingwithproof

.github/workflows/plugin-ci-workflow.yml

@@ -187,6 +187,38 @@ jobs:
           exit 1
         fi
 
+    - name: Create PHPStan config
+      run: |
+        cd ${{ github.workspace }}/cacti/plugins/syslog
+        cat > phpstan.neon << 'EOF'
+        parameters:
+          level: 5
+          paths:
+            - .
+          excludePaths:
+            - vendor/
+            - locales/
+          ignoreErrors:
+            - '#has invalid return type the\.#'
+          bootstrapFiles:
+            - ../../include/global.php
+        EOF
+
+    - name: Install PHPStan
+      run: |
+        cd ${{ github.workspace }}/cacti/plugins/syslog
+        composer require --dev phpstan/phpstan --with-all-dependencies || composer global require phpstan/phpstan
+
+    - name: Run PHPStan Analysis
+      run: |
+        cd ${{ github.workspace }}/cacti/plugins/syslog
+        if [ -f vendor/bin/phpstan ]; then
+          vendor/bin/phpstan analyse --no-progress --error-format=github || true
+        else
+          phpstan analyse --no-progress --error-format=github || true
+        fi
+      continue-on-error: true
+
     - name: Run Plugin Regression Tests
       run: |
         cd ${{ github.workspace }}/cacti/plugins/syslog
@@ -196,8 +228,7 @@ jobs:
             php "$test"
           done
         fi
-    
-    
+
     - name: Run Cacti Poller
       run: |
         cd ${{ github.workspace }}/cacti
@@ -207,14 +238,13 @@ jobs:
           cat log/cacti.log
           exit 1
         fi
-    
+
     - name: Populate Syslog Test Data
       run: |
         sudo chmod +x ${{ github.workspace }}/cacti/plugins/syslog/.github/workflows/populate_syslog_incoming.sh
         cd ${{ github.workspace }}/cacti/plugins/syslog/.github/workflows
         sudo ./populate_syslog_incoming.sh
 
-
     - name: force Syslog Plugin Poller
       run: |
         cd ${{ github.workspace }}/cacti
@@ -225,7 +255,6 @@ jobs:
           exit 1
         fi
 
-    
     - name: View Cacti Logs
       if: always()
       run: |
@@ -235,36 +264,3 @@ jobs:
         fi
 
 
-    - name: Create PHPStan config
-      run: |
-        cd ${{ github.workspace }}/cacti/plugins/syslog
-        cat > phpstan.neon << 'EOF'
-        parameters:
-          level: 5
-          paths:
-            - .
-          excludePaths:
-            - vendor/
-            - locales/
-          ignoreErrors:
-            - '#has invalid return type the\.#'
-          bootstrapFiles:
-            - ../../include/global.php
-        EOF
-    
-    - name: Install PHPStan
-      run: |
-        cd ${{ github.workspace }}/cacti/plugins/syslog
-        composer require --dev phpstan/phpstan --with-all-dependencies || composer global require phpstan/phpstan
-    
-    - name: Run PHPStan Analysis
-      run: |
-        cd ${{ github.workspace }}/cacti/plugins/syslog
-        if [ -f vendor/bin/phpstan ]; then
-          vendor/bin/phpstan analyse --no-progress --error-format=github || true
-        else
-          phpstan analyse --no-progress --error-format=github || true
-        fi
-      continue-on-error: true
-
-

functions.php

@@ -232,32 +232,63 @@ function syslog_partition_manage() {
 	return $syslog_deleted;
 }
 
+/**
+ * syslog_partition_table_allowed - validate that the table being partitioned
+ *   is in our approved list.
+ *
+ * @param  (string)  The table name
+ *
+ * @return (bool)    True if allowed, False otherwise
+ */
+function syslog_partition_table_allowed($table) {
+	if (in_array($table, array('syslog', 'syslog_removed'), true)) {
+		return (bool)preg_match('/^[a-z_]+$/', $table);
+	}
+
+	return false;
+}
+
 /**
  * This function will create a new partition for the specified table.
  */
 function syslog_partition_create($table) {
 	global $syslogdb_default;
 
+	if (!syslog_partition_table_allowed($table)) {
+		return false;
+	}
+
 	/* determine the format of the table name */
 	$time    = time();
 	$cformat = 'd' . date('Ymd', $time);
 	$lnow    = date('Y-m-d', $time+86400);
 
-	$exists = syslog_db_fetch_row("SELECT *
+	$exists = syslog_db_fetch_row_prepared("SELECT *
 		FROM `information_schema`.`partitions`
-        WHERE table_schema='" . $syslogdb_default . "'
-		AND partition_name='" . $cformat . "'
-		AND table_name='syslog'
-        ORDER BY partition_ordinal_position");
+		WHERE table_schema = ?
+		AND partition_name = ?
+		AND table_name = ?
+		ORDER BY partition_ordinal_position",
+		array($syslogdb_default, $cformat, $table)
+	);
 
 	if (!cacti_sizeof($exists)) {
-		cacti_log("SYSLOG: Creating new partition '$cformat'", false, 'SYSTEM');
+		$lock_name = hash('sha256', $syslogdb_default . 'syslog_partition_create.' . $table);
+
+		try {
+			syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
+
+			cacti_log("SYSLOG: Creating new partition '$cformat' for table '$table'", false, 'SYSTEM');
 
-		syslog_debug("Creating new partition '$cformat'");
+			syslog_debug("Creating new partition '$cformat' for table '$table'");
 
-		syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` REORGANIZE PARTITION dMaxValue INTO (
-			PARTITION $cformat VALUES LESS THAN (TO_DAYS('$lnow')),
-			PARTITION dMaxValue VALUES LESS THAN MAXVALUE)");
+			/* MySQL does not support parameter binding for DDL statements */
+			syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` REORGANIZE PARTITION dMaxValue INTO (
+				PARTITION $cformat VALUES LESS THAN (TO_DAYS('$lnow')),
+				PARTITION dMaxValue VALUES LESS THAN MAXVALUE)");
+		} finally {
+			syslog_db_fetch_cell_prepared('SELECT RELEASE_LOCK(?)', array($lock_name));
+		}
 	}
 }
 
@@ -267,32 +298,48 @@ function syslog_partition_create($table) {
 function syslog_partition_remove($table) {
 	global $syslogdb_default;
 
+	if (!syslog_partition_table_allowed($table)) {
+		cacti_log("SYSLOG ERROR: Attempt to remove partitions from disallowed table '$table'", false, 'SYSTEM');
+		return 0;
+	}
+
 	$syslog_deleted = 0;
-	$number_of_partitions = syslog_db_fetch_assoc("SELECT *
+	$number_of_partitions = syslog_db_fetch_assoc_prepared("SELECT *
 		FROM `information_schema`.`partitions`
-		WHERE table_schema='" . $syslogdb_default . "' AND table_name='syslog'
-		ORDER BY partition_ordinal_position");
+		WHERE table_schema = ?
+		AND table_name = ?
+		ORDER BY partition_ordinal_position",
+		array($syslogdb_default, $table)
+	);
 
 	$days = read_config_option('syslog_retention');
 
-	syslog_debug("There are currently '" . sizeof($number_of_partitions) . "' Syslog Partitions, We will keep '$days' of them.");
+	syslog_debug("There are currently '" . sizeof($number_of_partitions) . "' Syslog Partitions for '$table', We will keep '$days' of them.");
 
 	if ($days > 0) {
 		$user_partitions = sizeof($number_of_partitions) - 1;
 		if ($user_partitions >= $days) {
-			$i = 0;
-			while ($user_partitions > $days) {
-				$oldest = $number_of_partitions[$i];
+			$lock_name = hash('sha256', $syslogdb_default . 'syslog_partition_remove.' . $table);
 
-				cacti_log("SYSLOG: Removing old partition '" . $oldest['PARTITION_NAME'] . "'", false, 'SYSTEM');
+			try {
+				syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
 
-				syslog_debug("Removing partition '" . $oldest['PARTITION_NAME'] . "'");
+				$i = 0;
+				while ($user_partitions > $days) {
+					$oldest = $number_of_partitions[$i];
 
-				syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` DROP PARTITION " . $oldest['PARTITION_NAME']);
+					cacti_log("SYSLOG: Removing old partition '" . $oldest['PARTITION_NAME'] . "' from table '$table'", false, 'SYSTEM');
 
-				$i++;
-				$user_partitions--;
-				$syslog_deleted++;
+					syslog_debug("Removing partition '" . $oldest['PARTITION_NAME'] . "' from table '$table'");
+
+					syslog_db_execute("ALTER TABLE `" . $syslogdb_default . "`.`$table` DROP PARTITION " . $oldest['PARTITION_NAME']);
+
+					$i++;
+					$user_partitions--;
+					$syslog_deleted++;
+				}
+			} finally {
+				syslog_db_fetch_cell_prepared('SELECT RELEASE_LOCK(?)', array($lock_name));
 			}
 		}
 	}
@@ -303,21 +350,29 @@ function syslog_partition_remove($table) {
 function syslog_partition_check($table) {
 	global $syslogdb_default;
 
+	if (!syslog_partition_table_allowed($table)) {
+		return false;
+	}
+
 	if (defined('SYSLOG_CONFIG')) {
 		include(SYSLOG_CONFIG);
 	}
 
 	/* find date of last partition */
-	$last_part = syslog_db_fetch_cell("SELECT PARTITION_NAME
+	$last_part = syslog_db_fetch_cell_prepared("SELECT PARTITION_NAME
 		FROM `information_schema`.`partitions`
-		WHERE table_schema='" . $syslogdb_default . "' AND table_name='syslog'
+		WHERE table_schema = ?
+		AND table_name = ?
 		ORDER BY partition_ordinal_position DESC
-		LIMIT 1,1;");
+		LIMIT 1,1",
+		array($syslogdb_default, $table)
+	);
 
 	$lformat   = str_replace('d', '', $last_part);
 	$cformat   = date('Ymd');
 
 	if ($cformat > $lformat) {
+
 		return true;
 	} else {
 		return false;
@@ -339,16 +394,9 @@ function syslog_remove_items($table, $uniqueID) {
 	syslog_debug('-------------------------------------------------------------------------------------');
 	syslog_debug('Processing Removal Rules...');
 
-	if ($table == 'syslog') {
-		$rows = syslog_db_fetch_assoc("SELECT *
-			FROM `" . $syslogdb_default . "`.`syslog_remove`
-			WHERE enabled = 'on'
-			AND id = $uniqueID");
-	} else {
-		$rows = syslog_db_fetch_assoc('SELECT *
-			FROM `' . $syslogdb_default . '`.`syslog_remove`
-			WHERE enabled="on"');
-	}
+	$rows = syslog_db_fetch_assoc('SELECT *
+		FROM `' . $syslogdb_default . '`.`syslog_remove`
+		WHERE enabled="on"');
 
 	syslog_debug(sprintf('Found   %5s - Removal Rule(s) to process', cacti_sizeof($rows)));
 

syslog_batch_transfer.php

@@ -70,7 +70,7 @@
 				display_help();
 				exit(0);
 			default:
-				echo "ERROR: Invalid Argument: ($arg)\n\n";
+				print "ERROR: Invalid Argument: ($arg)\n\n";
 				display_help();
 				exit(1);
 		}
@@ -140,15 +140,15 @@ function display_version() {
 	}
 
 	$info = plugin_syslog_version();
-	echo "Syslog Batch Process, Version " . $info['version'] . ", " . COPYRIGHT_YEARS . "\n";
+	print "Syslog Batch Process, Version " . $info['version'] . ", " . COPYRIGHT_YEARS . "\n";
 }
 
 function display_help() {
 	display_version();
 
-	echo "\nusage: syslog_batch_transfer.php [--debug|-d]\n\n";
-	echo "The Syslog batch process script for Cacti Syslogging.\n";
-	echo "This script removes old messages from main view.\n";
+	print "\nusage: syslog_batch_transfer.php [--debug|-d]\n\n";
+	print "The Syslog batch process script for Cacti Syslogging.\n";
+	print "This script removes old messages from main view.\n";
 }
 
 

tests/Integration/SyslogPartitioningIntegrationTest.php

@@ -0,0 +1,32 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * Integration tests for syslog partitioning logic.
+ *
+ * This test verifies the allowlist and creation logic for partitions.
+ */
+
+// Load stubs
+require_once __DIR__ . '/../Helpers/GlobalStubs.php';
+
+// Load the logic
+require_once __DIR__ . '/../../functions.php';
+
+describe('Syslog Partitioning Integration', function () {
+    test('syslog_partition_table_allowed() only allows known tables', function () {
+        expect(syslog_partition_table_allowed('syslog'))->toBeTrue();
+        expect(syslog_partition_table_allowed('syslog_removed'))->toBeTrue();
+        
+        expect(syslog_partition_table_allowed('users'))->toBeFalse();
+        expect(syslog_partition_table_allowed('syslog; DROP TABLE syslog'))->toBeFalse();
+        expect(syslog_partition_table_allowed(''))->toBeFalse();
+    });
+
+    test('syslog_partition_create() returns false for disallowed tables', function () {
+        // This should return early without doing anything
+        $result = syslog_partition_create('invalid_table');
+        expect($result)->toBeFalse();
+    });
+});