Skip to content

Commit 058c56c

Browse files
somethingwithproofsomethingwithproof
committed
fix: refine command execution and standardize CI tests
- Implement robust executable extraction in command helpers - Standardize regression test output using print instead of echo - Enforce PHPStan and add PHP-CS-Fixer check to CI workflow
1 parent a6088d9 commit 058c56c

10 files changed

Lines changed: 122 additions & 38 deletions

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

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,11 +213,24 @@ jobs:
213213
run: |
214214
cd ${{ github.workspace }}/cacti/plugins/syslog
215215
if [ -f vendor/bin/phpstan ]; then
216-
vendor/bin/phpstan analyse --no-progress --error-format=github || true
216+
vendor/bin/phpstan analyse --no-progress --error-format=github
217217
else
218-
phpstan analyse --no-progress --error-format=github || true
218+
phpstan analyse --no-progress --error-format=github
219+
fi
220+
221+
- name: Install PHP-CS-Fixer
222+
run: |
223+
cd ${{ github.workspace }}/cacti/plugins/syslog
224+
composer require --dev friendsofphp/php-cs-fixer --with-all-dependencies || composer global require friendsofphp/php-cs-fixer
225+
226+
- name: Run PHP-CS-Fixer Check
227+
run: |
228+
cd ${{ github.workspace }}/cacti/plugins/syslog
229+
if [ -f vendor/bin/php-cs-fixer ]; then
230+
vendor/bin/php-cs-fixer fix --dry-run --diff --format=github
231+
else
232+
php-cs-fixer fix --dry-run --diff --format=github
219233
fi
220-
continue-on-error: true
221234
222235
- name: Run Plugin Regression Tests
223236
run: |

‎functions.php‎

Lines changed: 98 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,12 @@ function syslog_partition_create($table) {
295295
$cformat = 'd' . date('Ymd', $time);
296296
$lnow = date('Y-m-d', $time+86400);
297297

298+
/* validate DDL interpolation values to prevent injection */
299+
if (!preg_match('/^d\d{8}$/', $cformat) || !preg_match('/^\d{4}-\d{2}-\d{2}$/', $lnow)) {
300+
cacti_log('SYSLOG ERROR: Invalid partition format values detected', false, 'SYSTEM');
301+
return false;
302+
}
303+
298304
$exists = syslog_db_fetch_row_prepared("SELECT *
299305
FROM `information_schema`.`partitions`
300306
WHERE table_schema = ?
@@ -308,13 +314,19 @@ function syslog_partition_create($table) {
308314
$lock_name = hash('sha256', $syslogdb_default . 'syslog_partition_create.' . $table);
309315

310316
try {
311-
syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
317+
$locked = syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
318+
319+
if ((int)$locked !== 1) {
320+
cacti_log("SYSLOG WARNING: Failed to acquire partition create lock for '$table'", false, 'SYSTEM');
321+
return false;
322+
}
312323

313324
cacti_log("SYSLOG: Creating new partition '$cformat' for table '$table'", false, 'SYSTEM');
314325

315326
syslog_debug("Creating new partition '$cformat' for table '$table'");
316327

317-
/* MySQL does not support parameter binding for DDL statements */
328+
/* MySQL does not support parameter binding for DDL statements;
329+
$cformat and $lnow are validated above via regex */
318330
syslog_db_execute_prepared("ALTER TABLE `" . $syslogdb_default . "`.`$table` REORGANIZE PARTITION dMaxValue INTO (
319331
PARTITION $cformat VALUES LESS THAN (TO_DAYS('$lnow')),
320332
PARTITION dMaxValue VALUES LESS THAN MAXVALUE)");
@@ -354,7 +366,12 @@ function syslog_partition_remove($table) {
354366
$lock_name = hash('sha256', $syslogdb_default . 'syslog_partition_remove.' . $table);
355367

356368
try {
357-
syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
369+
$locked = syslog_db_fetch_cell_prepared('SELECT GET_LOCK(?, 10)', array($lock_name));
370+
371+
if ((int)$locked !== 1) {
372+
cacti_log("SYSLOG WARNING: Failed to acquire partition remove lock for '$table'", false, 'SYSTEM');
373+
return $syslog_deleted;
374+
}
358375

359376
$i = 0;
360377
while ($user_partitions > $days) {
@@ -445,8 +462,10 @@ function syslog_remove_items($table, $uniqueID) {
445462

446463
if (cacti_sizeof($rows)) {
447464
foreach($rows as $remove) {
448-
$sql = '';
449-
$sql1 = '';
465+
$sql = '';
466+
$sql1 = '';
467+
$params = array();
468+
$params1 = array();
450469

451470
if ($remove['type'] == 'facility') {
452471
if ($table == 'syslog_incoming') {
@@ -1326,10 +1345,33 @@ function syslog_execute_ticket_command($alert, $hostlist, $context) {
13261345
$command = trim(read_config_option('syslog_ticket_command'));
13271346

13281347
if ($command != '') {
1329-
$cparts = preg_split('/\s+/', trim($command));
1330-
$executable = trim($cparts[0], '"\'');
1348+
/*
1349+
* Extract the executable portion from the configured command.
1350+
* This allows for quoted paths and additional arguments in the
1351+
* configuration while still validating the executable itself.
1352+
*/
1353+
$executable = $command;
1354+
$firstChar = substr($executable, 0, 1);
13311355

1332-
if (is_executable($executable)) {
1356+
if ($firstChar === '"' || $firstChar === "'") {
1357+
$quoteChar = $firstChar;
1358+
$closing = strpos($executable, $quoteChar, 1);
1359+
1360+
if ($closing !== false) {
1361+
$executable = substr($executable, 1, $closing - 1);
1362+
} else {
1363+
// Unbalanced quotes; fall back to trimming quotes/whitespace.
1364+
$executable = trim($executable, " \t\n\r\0\x0B\"'");
1365+
}
1366+
} else {
1367+
$parts = preg_split('/\s+/', $executable);
1368+
if (is_array($parts) && isset($parts[0])) {
1369+
$executable = $parts[0];
1370+
}
1371+
$executable = trim($executable, " \t\n\r\0\x0B\"'");
1372+
}
1373+
1374+
if ($executable !== '' && is_executable($executable)) {
13331375
$command = $command .
13341376
' --alert-name=' . cacti_escapeshellarg(clean_up_name($alert['name'])) .
13351377
' --severity=' . cacti_escapeshellarg($alert['severity']) .
@@ -1356,31 +1398,51 @@ function syslog_execute_ticket_command($alert, $hostlist, $context) {
13561398
}
13571399

13581400
function syslog_execute_alert_command($alert, $results, $hostname) {
1359-
if ($alert['execute_command'] != 'on') {
1360-
return;
1361-
}
1401+
if ($alert['execute_command'] == 'on') {
1402+
$command = trim($alert['command']);
13621403

1363-
$command = trim($alert['command']);
1404+
if ($command != '') {
1405+
$command = alert_replace_variables($alert, $results, $hostname);
13641406

1365-
if ($command != '') {
1366-
$command = alert_replace_variables($alert, $results, $hostname);
1407+
/*
1408+
* Extract the executable portion from the command string.
1409+
* This allows for quoted paths and additional arguments.
1410+
*/
1411+
$executable = $command;
1412+
$firstChar = substr($executable, 0, 1);
13671413

1368-
$cparts = preg_split('/\s+/', trim($command));
1369-
$executable = trim($cparts[0], '"\'');
1414+
if ($firstChar === '"' || $firstChar === "'") {
1415+
$quoteChar = $firstChar;
1416+
$closing = strpos($executable, $quoteChar, 1);
13701417

1371-
if (is_executable($executable)) {
1372-
$output = array();
1373-
$returnCode = 0;
1418+
if ($closing !== false) {
1419+
$executable = substr($executable, 1, $closing - 1);
1420+
} else {
1421+
// Unbalanced quotes; fall back to trimming quotes/whitespace.
1422+
$executable = trim($executable, " \t\n\r\0\x0B\"'");
1423+
}
1424+
} else {
1425+
$parts = preg_split('/\s+/', $executable);
1426+
if (is_array($parts) && isset($parts[0])) {
1427+
$executable = $parts[0];
1428+
}
1429+
$executable = trim($executable, " \t\n\r\0\x0B\"'");
1430+
}
13741431

1375-
exec($command, $output, $returnCode);
1432+
if ($executable !== '' && is_executable($executable)) {
1433+
$output = array();
1434+
$returnCode = 0;
13761435

1377-
$logMessage = "SYSLOG NOTICE: Executing '$command' Command return code: $returnCode";
1378-
cacti_log($logMessage, true, 'SYSTEM');
1379-
} else {
1380-
if (strpos($executable, DIRECTORY_SEPARATOR) === false) {
1381-
cacti_log(sprintf('SYSLOG ERROR: Alert Command path \'%s\' is missing absolute path separator', $executable), false, 'SYSTEM');
1436+
exec($command, $output, $returnCode);
1437+
1438+
$logMessage = "SYSLOG NOTICE: Executing '$command' Command return code: $returnCode";
1439+
cacti_log($logMessage, true, 'SYSTEM');
13821440
} else {
1383-
cacti_log(sprintf('SYSLOG ERROR: Alert Command path \'%s\' is not executable', $executable), false, 'SYSTEM');
1441+
if (strpos($executable, DIRECTORY_SEPARATOR) === false) {
1442+
cacti_log(sprintf('SYSLOG ERROR: Alert Command path \'%s\' is missing absolute path separator', $executable), false, 'SYSTEM');
1443+
} else {
1444+
cacti_log(sprintf('SYSLOG ERROR: Alert Command path \'%s\' is not executable', $executable), false, 'SYSTEM');
1445+
}
13841446
}
13851447
}
13861448
}
@@ -1822,6 +1884,8 @@ function syslog_get_alert_sql(&$alert, $uniqueID) {
18221884
function syslog_preprocess_incoming_records() {
18231885
global $syslogdb_default;
18241886

1887+
$attempts = 0;
1888+
18251889
while (1) {
18261890
$uniqueID = rand(1, 127);
18271891

@@ -1833,6 +1897,13 @@ function syslog_preprocess_incoming_records() {
18331897
if ($count == 0) {
18341898
break;
18351899
}
1900+
1901+
$attempts++;
1902+
1903+
if ($attempts >= 256) {
1904+
cacti_log('SYSLOG ERROR: Unable to find unused uniqueID after 256 attempts', false, 'SYSTEM');
1905+
return array('uniqueID' => 0, 'incoming' => 0);
1906+
}
18361907
}
18371908

18381909
/* flag all records with the uniqueID prior to moving */
@@ -2088,7 +2159,7 @@ function syslog_incoming_to_syslog($uniqueID) {
20882159

20892160
syslog_debug(sprintf('Moved %5s - Message(s) to the syslog table', $moved));
20902161

2091-
syslog_db_execute_prepared('DELETE FROM `' . $syslogdb_default . '`.`syslog_incoming` WHERE status=' . $uniqueID);
2162+
syslog_db_execute_prepared('DELETE FROM `' . $syslogdb_default . '`.`syslog_incoming` WHERE status = ?', array($uniqueID));
20922163

20932164
syslog_debug(sprintf('Deleted %5s - Already Processed Message(s) from incoming', db_affected_rows($syslog_cnn)));
20942165

‎tests/regression/issue252_xss_output_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,4 @@
4242
exit(1);
4343
}
4444

45-
echo "issue252_xss_output_test passed\n";
45+
print "issue252_xss_output_test passed\n";

‎tests/regression/issue259_csrf_purge_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -103,4 +103,4 @@
103103
exit(1);
104104
}
105105

106-
echo "issue259_csrf_purge_test passed\n";
106+
print "issue259_csrf_purge_test passed\n";

‎tests/regression/issue260_remove_eval_callback_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,4 +42,4 @@
4242
exit(1);
4343
}
4444

45-
echo "issue260_remove_eval_callback_test passed\n";
45+
print "issue260_remove_eval_callback_test passed\n";

‎tests/regression/issue261_domain_strip_parameterized_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,4 +24,4 @@
2424
exit(1);
2525
}
2626

27-
echo "issue261_domain_strip_parameterized_test passed\n";
27+
print "issue261_domain_strip_parameterized_test passed\n";

‎tests/regression/issue269_import_text_branch_logic_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,4 @@
4747
exit(1);
4848
}
4949

50-
echo "issue269_import_text_branch_logic_test passed\n";
50+
print "issue269_import_text_branch_logic_test passed\n";

‎tests/regression/issue269_import_text_trim_check_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,5 +23,5 @@
2323
exit(1);
2424
}
2525

26-
echo "issue269_import_text_trim_check_test passed\n";
26+
print "issue269_import_text_trim_check_test passed\n";
2727

‎tests/regression/issue_csrf_purge_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,4 +13,4 @@
1313
exit(1);
1414
}
1515

16-
echo "issue_csrf_purge_test passed\n";
16+
print "issue_csrf_purge_test passed\n";

‎tests/regression/issue_removal_parameterization_test.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,4 +33,4 @@
3333
exit(1);
3434
}
3535

36-
echo "issue_removal_parameterization_test passed\n";
36+
print "issue_removal_parameterization_test passed\n";

0 commit comments

Comments
 (0)