Cacti · somethingwithproof · Mar 7, 2026 · Mar 7, 2026 · Mar 7, 2026 · Mar 8, 2026
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 --- develop ---
 
+* issue#278: Extract alert command execution into shared helper in functions.php; command tokenization now uses preg_split (handles tabs and consecutive spaces); /bin/sh fallback for non-executable command templates removed (use absolute paths with execute bit set)
 * issue: Making changes to support Cacti 1.3
 * issue: Don't use MyISAM for non-analytical tables
 * issue: The install advisor for Syslog was broken in current Cacti releases

diff --git a/functions.php b/functions.php
@@ -1081,6 +1081,93 @@ function syslog_array2xml($array, $tag = 'template') {
 	return $xml;
 }
 
+/**
+ * syslog_execute_ticket_command - run the configured ticketing command for an alert
+ *
+ * @param  array  $alert          The alert row from syslog_alert table
+ * @param  array  $hostlist       Hostnames matched by the alert
+ * @param  string $context        Short label for the log entry (e.g. 'Ticket Command', 'Command')
+ *
+ * @return void
+ */
+function syslog_execute_ticket_command($alert, $hostlist, $context) {
+	$command = read_config_option('syslog_ticket_command');
+
+	if ($command != '') {
+		$command = trim($command);
+	}
+
+	if ($alert['open_ticket'] == 'on' && $command != '') {
+		/* trim surrounding quotes so paths like "/usr/bin/cmd" resolve correctly */
+		$cparts     = preg_split('/\s+/', trim($command));
+		$executable = trim($cparts[0], '"\'');
+
+		if (is_executable($executable)) {
+			$command = $command .
+				' --alert-name=' . cacti_escapeshellarg(clean_up_name($alert['name'])) .
+				' --severity='   . cacti_escapeshellarg($alert['severity']) .
+				' --hostlist='   . cacti_escapeshellarg(implode(',', $hostlist)) .
+				' --message='    . cacti_escapeshellarg($alert['message']);
+
+			$output = array();
+			$return = 0;
+
+			exec($command, $output, $return);
+
+			if ($return !== 0) {
+				cacti_log(sprintf('ERROR: %s Failed.  Alert:%s, Exit:%s, Output:%s', $context, $alert['name'], $return, implode(', ', $output)), false, 'SYSLOG');
+			}
+		} else {
+			$reason = (strpos($executable, DIRECTORY_SEPARATOR) === false)
+				? 'PATH-based lookups are not supported; use an absolute path'
+				: 'file not found or not marked executable';
+			cacti_log("SYSLOG ERROR: $context is not executable: '$command' -- $reason", false, 'SYSTEM');
+		}
+	}
+}
+
+/**
+ * syslog_execute_alert_command - run the per-alert shell command for a matched result
+ *
+ * @param  array  $alert     The alert row from syslog_alert table
+ * @param  array  $results   The matched syslog result row
+ * @param  string $hostname  Resolved hostname for the source device
+ *
+ * @return void
+ */
+function syslog_execute_alert_command($alert, $results, $hostname) {
+	/* alert_replace_variables() escapes each substituted token (<ALERTID>,
+	 * <HOSTNAME>, <PRIORITY>, <FACILITY>, <MESSAGE>, <SEVERITY>) with
+	 * cacti_escapeshellarg(). The command template itself comes from admin
+	 * configuration ($alert['command']) and is trusted at that boundary.
+	 * Do not introduce additional substitution paths that bypass this escaping. */
+	$command = alert_replace_variables($alert, $results, $hostname);
+
+	/* trim surrounding quotes so paths like "/usr/bin/cmd" resolve correctly */
+	$cparts     = preg_split('/\s+/', trim($command));
+	$executable = trim($cparts[0], '"\'');
+
+	$output = array();
+	$return = 0;
+
+	if (is_executable($executable)) {
+		exec($command, $output, $return);
+
+		if ($return !== 0 && !empty($output)) {
+			cacti_log('SYSLOG NOTICE: Alert command output: ' . implode(', ', $output), true, 'SYSTEM');
+		}
+
+		if ($return !== 0) {
+			cacti_log(sprintf('ERROR: Alert command failed.  Alert:%s, Exit:%s, Output:%s', $alert['name'], $return, implode(', ', $output)), false, 'SYSLOG');
+		}
+	} else {
+		$reason = (strpos($executable, DIRECTORY_SEPARATOR) === false)
+			? 'PATH-based lookups are not supported; use an absolute path'
+			: 'file not found or not marked executable';
+		cacti_log("SYSLOG ERROR: Alert command is not executable: '$command' -- $reason", false, 'SYSTEM');
+	}
+}
+
 /**
  * syslog_process_alerts - Process each of the Syslog Alerts
  *
@@ -1495,49 +1582,10 @@ function syslog_process_alert($alert, $sql, $params, $count, $hostname = '') {
 					/**
 					 * Open a ticket if this options have been selected.
 					 */
-					 $command = read_config_option('syslog_ticket_command');
-
-					if ($command != '') {
-						$command = trim($command);
-					}
-
-					if ($alert['open_ticket'] == 'on' && $command != '') {
-						if (is_executable($command)) {
-							$command = $command .
-								' --alert-name=' . cacti_escapeshellarg(clean_up_name($alert['name'])) .
-								' --severity='   . cacti_escapeshellarg($alert['severity']) .
-								' --hostlist='   . cacti_escapeshellarg(implode(',',$hostlist)) .
-								' --message='    . cacti_escapeshellarg($alert['message']);
-
-							$output = array();
-							$return = 0;
-
-							exec($command, $output, $return);
-
-							if ($return != 0) {
-								cacti_log(sprintf('ERROR: Ticket Command Failed.  Alert:%s, Exit:%s, Output:%s', $alert['name'], $return, implode(', ', $output)), false, 'SYSLOG');
-							}
-						}
-					}
+					syslog_execute_ticket_command($alert, $hostlist, 'Ticket Command');
 
 					if (trim($alert['command']) != '' && !$found) {
-						$command = alert_replace_variables($alert, $results, $hostname);
-
-						$logMessage = "SYSLOG NOTICE: Executing '$command'";
-
-						$cparts = explode(' ', $command);
-
-						if (is_executable($cparts[0])) {
-							exec($command, $output, $returnCode);
-						} else {
-							exec('/bin/sh ' . $command, $output, $returnCode);
-						}
-
-						// Append the return code to the log message without the dot
-						$logMessage .= " Command return code: $returnCode";
-
-						// Log the combined message
-						cacti_log($logMessage, true, 'SYSTEM');
+						syslog_execute_alert_command($alert, $results, $hostname);
 					}
 
 				}
@@ -1555,49 +1603,10 @@ function syslog_process_alert($alert, $sql, $params, $count, $hostname = '') {
 
 					alert_setup_environment($alert, $results, $hostlist, $hostname);
 
-					$command = read_config_option('syslog_ticket_command');
-
-					if ($command != '') {
-						$command = trim($command);
-					}
-
-					if ($alert['open_ticket'] == 'on' && $command != '') {
-						if (is_executable($command)) {
-							$command = $command .
-								' --alert-name=' . cacti_escapeshellarg(clean_up_name($alert['name'])) .
-								' --severity='   . cacti_escapeshellarg($alert['severity']) .
-								' --hostlist='   . cacti_escapeshellarg(implode(',',$hostlist)) .
-								' --message='    . cacti_escapeshellarg($alert['message']);
-
-							$output = array();
-							$return = 0;
-
-							exec($command, $output, $return);
-
-							if ($return != 0) {
-								cacti_log(sprintf('ERROR: Command Failed.  Alert:%s, Exit:%s, Output:%s', $alert['name'], $return, implode(', ', $output)), false, 'SYSLOG');
-							}
-						}
-					}
+					syslog_execute_ticket_command($alert, $hostlist, 'Command');
 
 					if (trim($alert['command']) != '' && !$found) {
-						$command = alert_replace_variables($alert, $results, $hostname);
-
-						$logMessage = "SYSLOG NOTICE: Executing '$command'";
-
-						$cparts = explode(' ', $command);
-
-						if (is_executable($cparts[0])) {
-							exec($command, $output, $returnCode);
-						} else {
-							exec('/bin/sh ' . $command, $output, $returnCode);
-						}
-
-						// Append the return code to the log message without the dot
-						$logMessage .= " Command return code: $returnCode";
-
-						// Log the combined message
-						cacti_log($logMessage, true, 'SYSTEM');
+						syslog_execute_alert_command($alert, $results, $hostname);
 					}
 				}
 			}
@@ -2421,4 +2430,3 @@ function alert_replace_variables($alert, $results, $hostname = '') {
 
 	return $command;
 }
-
diff --git a/tests/regression/issue278_command_execution_refactor_test.php b/tests/regression/issue278_command_execution_refactor_test.php
@@ -0,0 +1,118 @@
+<?php
+
+$functions = file_get_contents(dirname(__DIR__, 2) . '/functions.php');
+
+if ($functions === false) {
+	fwrite(STDERR, "Failed to load functions.php\n");
+	exit(1);
+}
+
+if (strpos($functions, 'function syslog_execute_ticket_command(') === false) {
+	fwrite(STDERR, "Ticket command execution helper is missing.\n");
+	exit(1);
+}
+
+if (strpos($functions, 'function syslog_execute_alert_command(') === false) {
+	fwrite(STDERR, "Alert command execution helper is missing.\n");
+	exit(1);
+}
+
+/* Match only call sites (third arg is a string literal), not the function
+ * definition whose third param is $context.  Two call sites exist: one for
+ * method==0 ('Ticket Command') and one for method==1 ('Command'). */
+if (substr_count($functions, "syslog_execute_ticket_command(\$alert, \$hostlist, '") < 2) {
+	fwrite(STDERR, "syslog_process_alerts() is not consistently using the ticket command helper.\n");
+	exit(1);
+}
+
+if (substr_count($functions, 'syslog_execute_alert_command($alert, $results, $hostname);') < 2) {
+	fwrite(STDERR, "syslog_process_alerts() is not consistently using the alert command helper.\n");
+	exit(1);
+}
+
+if (strpos($functions, "exec('/bin/sh '") !== false) {
+	fwrite(STDERR, "Shell fallback execution path must not appear in shared helpers.\n");
+	exit(1);
+}
+
+/* open_ticket guard: function is a no-op unless open_ticket == 'on' */
+if (strpos($functions, "\$alert['open_ticket'] == 'on'") === false) {
+	fwrite(STDERR, "syslog_execute_ticket_command() must guard on open_ticket == 'on'.\n");
+	exit(1);
+}
+
+/* empty-command guard: function is a no-op when command trims to '' */
+if (strpos($functions, "\$command != ''") === false) {
+	fwrite(STDERR, "syslog_execute_ticket_command() must guard on non-empty command.\n");
+	exit(1);
+}
+
+/* is_executable must be called on the stripped executable, not the raw command string.
+ * Old code checked is_executable($command) which broke quoted absolute paths. */
+if (strpos($functions, 'is_executable($executable)') === false) {
+	fwrite(STDERR, "is_executable() must be called on stripped \$executable, not raw \$command.\n");
+	exit(1);
+}
+
+if (strpos($functions, 'is_executable($command)') !== false) {
+	fwrite(STDERR, "is_executable() must not be called on raw \$command string.\n");
+	exit(1);
+}
+
+/* PATH-lookup detection: both helpers must produce distinct error messages via
+ * DIRECTORY_SEPARATOR to distinguish "no path separator" from "not executable". */
+if (substr_count($functions, 'strpos($executable, DIRECTORY_SEPARATOR)') < 2) {
+	fwrite(STDERR, "Both helpers must detect PATH-based lookups via DIRECTORY_SEPARATOR.\n");
+	exit(1);
+}
+
+/* syslog_execute_alert_command must delegate variable substitution to
+ * alert_replace_variables(); an empty result falls through to is_executable('')
+ * which returns false and logs the error path. */
+if (strpos($functions, 'alert_replace_variables(') === false) {
+	fwrite(STDERR, "syslog_execute_alert_command() must call alert_replace_variables().\n");
+	exit(1);
+}
+
+/* ticket command must only log on failure ($return != 0), not unconditionally */
+if (preg_match('/exec\(\$command,.*?\$return\);\s*\n\s*cacti_log\(sprintf\(\'SYSLOG NOTICE:/s', $functions)) {
+	fwrite(STDERR, "syslog_execute_ticket_command() must not unconditionally log success after exec().\n");
+	exit(1);
+}
+
+/* syslog_execute_alert_command must not have dead assignment $returnCode = 126 */
+if (preg_match('/\$returnCode\s*=\s*126/', $functions)) {
+	fwrite(STDERR, "syslog_execute_alert_command() must not contain dead assignment \$returnCode = 126.\n");
+	exit(1);
+}
+
+/* quote-stripping: executable extraction must trim surrounding quotes */
+if (strpos($functions, "trim(\$cparts[0], '\"\\'')") === false) {
+	fwrite(STDERR, "Executable extraction must strip surrounding quotes from command path.\n");
+	exit(1);
+}
+
+/* preg_split for whitespace tokenization (handles tabs and consecutive spaces) */
+if (substr_count($functions, "preg_split('/\\s+/', trim(\$command))") < 1) {
+	fwrite(STDERR, "Command tokenization must use preg_split for whitespace splitting.\n");
+	exit(1);
+}
+
+/* non-executable error path must log SYSLOG ERROR in both helpers */
+if (substr_count($functions, 'SYSLOG ERROR:') < 2) {
+	fwrite(STDERR, "Both helpers must log SYSLOG ERROR when executable is missing.\n");
+	exit(1);
+}
+
+/* cacti_escapeshellarg must wrap all four --arg values in ticket command */
+$ticket_fn_match = array();
+if (preg_match('/function syslog_execute_ticket_command\b.*?^}/ms', $functions, $ticket_fn_match)) {
+	$ticket_body = $ticket_fn_match[0];
+	$esc_count   = substr_count($ticket_body, 'cacti_escapeshellarg(');
+	if ($esc_count < 4) {
+		fwrite(STDERR, "syslog_execute_ticket_command() must call cacti_escapeshellarg() for all 4 --arg values (found $esc_count).\n");
+		exit(1);
+	}
+}
+
+echo "issue278_command_execution_refactor_test passed\n";