From 484cdbb5b77935c974c281d4fb99657775033f6d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:32:47 -0700 Subject: [PATCH 01/41] fix(security): RLIKE injection, XSS, unserialize, and trigger_cmd hardening RLIKE SQL injection: rfilter request variable was concatenated directly into RLIKE patterns across thold_graph.php (4 instances), thold.php, and notify_lists.php (3 instances including the notification list filter). Replaced with db_qstr() which SQL-escapes and quotes the value. XSS: get_request_var('page') was printed raw into hidden input value attributes. Wrapped with html_escape(). Unserialize: thold_webapi.php called cacti_unserialize(stripslashes(...)) on POST selected_graphs_array. Replaced with sanitize_unserialize_selected_items() which validates the result is an array of integers only. trigger_cmd: thold_set_environ() was called with trigger_cmd_high in both the low-breach and norm-restoration branches. Corrected to trigger_cmd_low and trigger_cmd_norm respectively. No intval() casts added: host_id/site_id go through FILTER_VALIDATE_INT in the request validation arrays; adding casts after validated request vars is redundant per Cacti convention. Signed-off-by: Thomas Vincent --- notify_lists.php | 12 ++++++------ thold.php | 2 +- thold_functions.php | 4 ++-- thold_graph.php | 14 +++++++------- thold_webapi.php | 2 +- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 016e01dc..1d7e5cbc 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -1399,7 +1399,7 @@ function tholds($header_label) { } if (strlen(get_request_var('rfilter'))) { - $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE '" . get_request_var('rfilter') . "'"; + $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); } if ($statefilter != '') { @@ -1739,7 +1739,7 @@ function templates($header_label) { } if (strlen(get_request_var('rfilter'))) { - $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . "thold_template.name RLIKE '" . get_request_var('rfilter') . "'"; + $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . 'thold_template.name RLIKE ' . db_qstr(get_request_var('rfilter')); } $sql = "SELECT * @@ -2143,10 +2143,10 @@ function clearFilter() { // form the 'where' clause for our main sql query if (strlen(get_request_var('rfilter'))) { - $sql_where = "WHERE ( - name RLIKE '" . get_request_var('rfilter') . "' - OR description RLIKE '" . get_request_var('rfilter') . "' - OR emails RLIKE '" . get_request_var('rfilter') . "')"; + $sql_where = 'WHERE ( + name RLIKE ' . db_qstr(get_request_var('rfilter')) . ' + OR description RLIKE ' . db_qstr(get_request_var('rfilter')) . ' + OR emails RLIKE ' . db_qstr(get_request_var('rfilter')) . ')'; } else { $sql_where = ''; } diff --git a/thold.php b/thold.php index 0bf86f46..b0c9f790 100644 --- a/thold.php +++ b/thold.php @@ -614,7 +614,7 @@ function list_tholds() { } if (get_request_var('rfilter') != '') { - $sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE '" . get_request_var('rfilter') . "'"; + $sql_where .= ($sql_where == '' ? '(' : ' AND ') . ' td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); } if ($statefilter != '') { diff --git a/thold_functions.php b/thold_functions.php index 018cf99b..686ac1be 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -4008,7 +4008,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_low'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); $cmd = thold_expand_string($thold_data, $cmd); - $environment = thold_set_environ($thold_data['trigger_cmd_high'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); + $environment = thold_set_environ($thold_data['trigger_cmd_low'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); if ($queue == 'on') { $data = [ @@ -4027,7 +4027,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_norm'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); $cmd = thold_expand_string($thold_data, $cmd); - $environment = thold_set_environ($thold_data['trigger_cmd_high'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); + $environment = thold_set_environ($thold_data['trigger_cmd_norm'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); if ($queue == 'on') { $data = [ diff --git a/thold_graph.php b/thold_graph.php index 619f2ee6..462820c2 100644 --- a/thold_graph.php +++ b/thold_graph.php @@ -251,7 +251,7 @@ function form_thold_filter() { - '> + '> '))->toBe('<script>alert(1)</script>'); +}); + +it('html_escape converts double quotes to entities', function () { + expect(html_escape('"quoted"'))->toBe('"quoted"'); +}); + +it('html_escape converts single quotes to entities', function () { + // ENT_QUOTES|ENT_HTML5 encodes single quotes as ' (HTML5 named entity) + expect(html_escape("O'Brien"))->toBe('O'Brien'); +}); diff --git a/tests/bootstrap.php b/tests/bootstrap.php new file mode 100644 index 00000000..1c580f26 --- /dev/null +++ b/tests/bootstrap.php @@ -0,0 +1,162 @@ + '/var/www/html/cacti', + 'url_path' => '/cacti/', + 'cacti_version' => '1.2.999', +]; + +if (!function_exists('db_execute')) { + function db_execute($sql) { + $GLOBALS['__test_db_calls'][] = ['fn' => 'db_execute', 'sql' => $sql, 'params' => []]; + + return true; + } +} + +if (!function_exists('db_execute_prepared')) { + function db_execute_prepared($sql, $params = []) { + $GLOBALS['__test_db_calls'][] = ['fn' => 'db_execute_prepared', 'sql' => $sql, 'params' => $params]; + + return true; + } +} + +if (!function_exists('db_fetch_assoc')) { + function db_fetch_assoc($sql) { + return []; + } +} + +if (!function_exists('db_fetch_assoc_prepared')) { + function db_fetch_assoc_prepared($sql, $params = []) { + return []; + } +} + +if (!function_exists('db_fetch_row')) { + function db_fetch_row($sql) { + return []; + } +} + +if (!function_exists('db_fetch_row_prepared')) { + function db_fetch_row_prepared($sql, $params = []) { + return []; + } +} + +if (!function_exists('db_fetch_cell')) { + function db_fetch_cell($sql) { + return ''; + } +} + +if (!function_exists('db_fetch_cell_prepared')) { + function db_fetch_cell_prepared($sql, $params = []) { + return ''; + } +} + +if (!function_exists('db_qstr')) { + function db_qstr($string) { + return "'" . str_replace("'", "''", $string) . "'"; + } +} + +if (!function_exists('html_escape')) { + function html_escape($string) { + return htmlspecialchars($string, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + } +} + +if (!function_exists('sanitize_unserialize_selected_items')) { + function sanitize_unserialize_selected_items($items) { + if (empty($items)) { + return false; + } + + $data = unserialize($items, ['allowed_classes' => false]); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirrors sanitize_unserialize_selected_items; allowed_classes:false blocks object injection + + if (!is_array($data)) { + return false; + } + + foreach ($data as $key => $value) { + if (!is_numeric($value)) { + return false; + } + } + + return $data; + } +} + +if (!function_exists('read_config_option')) { + function read_config_option($name, $force = false) { + return ''; + } +} + +if (!function_exists('set_config_option')) { + function set_config_option($name, $value) { + } +} + +if (!function_exists('__')) { + function __($text, $domain = '') { + return $text; + } +} + +if (!function_exists('__esc')) { + function __esc($text, $domain = '') { + return htmlspecialchars($text, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + } +} + +if (!function_exists('cacti_log')) { + function cacti_log($message, $also_print = false, $log_type = '', $level = 0) { + } +} + +if (!function_exists('cacti_sizeof')) { + function cacti_sizeof($array) { + return is_array($array) ? count($array) : 0; + } +} + +if (!function_exists('get_request_var')) { + function get_request_var($name) { + return ''; + } +} + +if (!function_exists('get_nfilter_request_var')) { + function get_nfilter_request_var($name) { + return ''; + } +} + +if (!function_exists('get_filter_request_var')) { + function get_filter_request_var($name) { + return ''; + } +} + +if (!defined('CACTI_PATH_BASE')) { + define('CACTI_PATH_BASE', '/var/www/html/cacti'); +} From 850e05d710e93e1147fff6bdc02fd8259fb2d435 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:47:45 -0700 Subject: [PATCH 03/41] test(security): strengthen TriggerCmd and RlikeInjection coverage Split the combined OR-match in TriggerCmdRegressionTest into two independent preg_match assertions so a revert of either branch is caught independently. Add FILTER_VALIDATE_IS_REGEX test to RlikeInjectionTest documenting that rfilter goes through regex validation before any RLIKE clause, mitigating ReDoS at the MySQL engine level. Signed-off-by: Thomas Vincent --- tests/Security/RlikeInjectionTest.php | 11 +++++++++++ tests/Security/TriggerCmdRegressionTest.php | 11 ++++++----- 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/tests/Security/RlikeInjectionTest.php b/tests/Security/RlikeInjectionTest.php index 7e7c1bba..f0b94344 100644 --- a/tests/Security/RlikeInjectionTest.php +++ b/tests/Security/RlikeInjectionTest.php @@ -53,3 +53,14 @@ expect(db_qstr('normal'))->toBe("'normal'"); expect(db_qstr("1' OR '1'='1"))->toBe("'1'' OR ''1''=''1'"); }); + +it('rfilter is validated as a PHP regex before reaching any RLIKE clause', function () { + // thold_graph.php, thold.php, and notify_lists.php all declare rfilter with + // FILTER_VALIDATE_IS_REGEX in their request validation arrays. This means + // get_filter_request_var() rejects malformed or catastrophic patterns before + // any SQL is constructed, mitigating ReDoS at the MySQL RLIKE engine. + foreach (['thold_graph.php', 'thold.php', 'notify_lists.php'] as $file) { + $src = file_get_contents(realpath(__DIR__ . '/../../' . $file)); + expect($src)->toContain('FILTER_VALIDATE_IS_REGEX'); + } +}); diff --git a/tests/Security/TriggerCmdRegressionTest.php b/tests/Security/TriggerCmdRegressionTest.php index 5829ea15..96c09909 100644 --- a/tests/Security/TriggerCmdRegressionTest.php +++ b/tests/Security/TriggerCmdRegressionTest.php @@ -27,9 +27,10 @@ expect($funcs)->toContain("thold_data['trigger_cmd_norm']"); }); -it('thold_functions.php does not use trigger_cmd_high in low-breach thold_set_environ call', function () use ($funcs) { - // The pre-fix code passed trigger_cmd_high to thold_set_environ when handling low/norm breaches. - // Verify that trigger_cmd_low and trigger_cmd_norm each appear near thold_set_environ. - $pattern = '/thold_set_environ\s*\(\s*\$thold_data\[.trigger_cmd_(?:low|norm).\]/'; - expect((bool) preg_match($pattern, $funcs))->toBeTrue('trigger_cmd_low or trigger_cmd_norm must appear in thold_set_environ calls'); +it('thold_set_environ in low-breach branch uses trigger_cmd_low', function () use ($funcs) { + expect(preg_match('/thold_set_environ\s*\(\s*\$thold_data\[.trigger_cmd_low.\]/', $funcs))->toBe(1); +}); + +it('thold_set_environ in norm-restoration branch uses trigger_cmd_norm', function () use ($funcs) { + expect(preg_match('/thold_set_environ\s*\(\s*\$thold_data\[.trigger_cmd_norm.\]/', $funcs))->toBe(1); }); From 8b97adc97d01c7311c2791a488436d135a4e043a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:57:13 -0700 Subject: [PATCH 04/41] fix(security): replace raw SQL concatenation with prepared statements notify_lists.php: replace array_to_sql_or() and direct $selected_items concatenation with db_execute_prepared() + IN (?,?,?) placeholders for all bulk delete, associate, and disassociate operations. Also replace count() with cacti_sizeof() per Cacti 1.2.x idiom. thold_functions.php: parameterise $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() using ? placeholders; switch to db_fetch_assoc_prepared() and db_fetch_cell_prepared(). Add PreparedStatementTest.php, Php74CompatibilityTest.php, and Smoke/PhpSyntaxTest.php to cover these patterns. 41 tests pass. Signed-off-by: Thomas Vincent --- notify_lists.php | 306 ++++++++++++---------- tests/Security/Php74CompatibilityTest.php | 77 ++++++ tests/Security/PreparedStatementTest.php | 66 +++++ tests/Smoke/PhpSyntaxTest.php | 59 +++++ thold_functions.php | 24 +- 5 files changed, 389 insertions(+), 143 deletions(-) create mode 100644 tests/Security/Php74CompatibilityTest.php create mode 100644 tests/Security/PreparedStatementTest.php create mode 100644 tests/Smoke/PhpSyntaxTest.php diff --git a/notify_lists.php b/notify_lists.php index 1d7e5cbc..5498968e 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -156,41 +156,51 @@ function form_actions() { if (isset_request_var('save_list')) { if ($selected_items != false) { if (get_request_var('drp_action') == '1') { // delete - db_execute('DELETE FROM plugin_notification_lists - WHERE ' . array_to_sql_or($selected_items, 'id')); + $placeholders = implode(',', array_fill(0, cacti_sizeof($selected_items), '?')); - db_execute('UPDATE host + db_execute_prepared('DELETE FROM plugin_notification_lists + WHERE id IN (' . $placeholders . ')', + $selected_items); + + db_execute_prepared('UPDATE host SET thold_send_email = 0 WHERE thold_send_email = 2 - AND deleted="" - AND ' . array_to_sql_or($selected_items, 'thold_host_email')); + AND deleted = "" + AND thold_host_email IN (' . $placeholders . ')', + $selected_items); - db_execute('UPDATE host + db_execute_prepared('UPDATE host SET thold_send_email = 1 WHERE thold_send_email = 3 - AND deleted="" - AND ' . array_to_sql_or($selected_items, 'thold_host_email')); + AND deleted = "" + AND thold_host_email IN (' . $placeholders . ')', + $selected_items); - db_execute('UPDATE host + db_execute_prepared('UPDATE host SET thold_host_email = 0 - AND deleted="" - WHERE ' . array_to_sql_or($selected_items, 'thold_host_email')); + WHERE thold_host_email IN (' . $placeholders . ') + AND deleted = ""', + $selected_items); - db_execute('UPDATE thold_data + db_execute_prepared('UPDATE thold_data SET notify_warning = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_warning')); + WHERE notify_warning IN (' . $placeholders . ')', + $selected_items); - db_execute('UPDATE thold_data + db_execute_prepared('UPDATE thold_data SET notify_alert = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_alert')); + WHERE notify_alert IN (' . $placeholders . ')', + $selected_items); - db_execute('UPDATE thold_template + db_execute_prepared('UPDATE thold_template SET notify_warning = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_warning')); + WHERE notify_warning IN (' . $placeholders . ')', + $selected_items); - db_execute('UPDATE thold_template + db_execute_prepared('UPDATE thold_template SET notify_alert = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_alert')); + WHERE notify_alert IN (' . $placeholders . ')', + $selected_items); } elseif (get_request_var('drp_action') == '2') { // duplicate $i = 1; @@ -240,45 +250,50 @@ function form_actions() { get_filter_request_var('notification_action'); if (get_request_var('drp_action') == '1') { // associate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { // set the notification list - db_execute('UPDATE host - SET thold_host_email=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i] . ' - AND deleted=""'); + db_execute_prepared('UPDATE host + SET thold_host_email = ? + WHERE id = ? + AND deleted = ""', + [get_request_var('id'), $selected_items[$i]]); // set the global/list election - db_execute('UPDATE host - SET thold_send_email=' . get_request_var('notification_action') . ' - WHERE id=' . $selected_items[$i] . ' - AND deleted=""'); + db_execute_prepared('UPDATE host + SET thold_send_email = ? + WHERE id = ? + AND deleted = ""', + [get_request_var('notification_action'), $selected_items[$i]]); if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_warning=' . get_request_var('id') . ' - WHERE td.host_id=' . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_warning = ? + WHERE td.host_id = ? + AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_data AS td + db_execute_prepared("UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_warning_extra='' - WHERE td.host_id=" . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_warning_extra = '' + WHERE td.host_id = ? + AND (tt.notify_templated = \"\" OR tt.notify_templated IS NULL)", + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_warning=' . get_request_var('id') . ' - WHERE td.host_id=' . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_warning = ? + WHERE td.host_id = ? + AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', + [get_request_var('id'), $selected_items[$i]]); } } @@ -286,75 +301,83 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_alert=' . get_request_var('id') . ' - WHERE td.host_id=' . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_alert = ? + WHERE td.host_id = ? + AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_data AS td + db_execute_prepared("UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_extra='' - WHERE host_id=" . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_extra = '' + WHERE host_id = ? + AND (tt.notify_templated = \"\" OR tt.notify_templated IS NULL)", + [$selected_items[$i]]); // remove legacy contacts - db_execute('DELETE pttc + db_execute_prepared('DELETE pttc FROM plugin_thold_threshold_contact AS pttc INNER JOIN thold_data AS td ON pttc.thold_id = td.id LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - WHERE td.host_id=' . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + WHERE td.host_id = ? + AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_alert=' . get_request_var('id') . ' - WHERE td.host_id=' . $selected_items[$i] . ' - AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)'); + SET td.notify_alert = ? + WHERE td.host_id = ? + AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', + [get_request_var('id'), $selected_items[$i]]); } } } } elseif (get_request_var('drp_action') == '2') { // disassociate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { // set the notification list - db_execute('UPDATE host - SET thold_host_email=0 - WHERE id=' . $selected_items[$i] . ' - AND deleted=""'); + db_execute_prepared('UPDATE host + SET thold_host_email = 0 + WHERE id = ? + AND deleted = ""', + [$selected_items[$i]]); // set the global/list election - db_execute('UPDATE host - SET thold_send_email=' . get_request_var('notification_action') . ' - WHERE id=' . $selected_items[$i] . ' - AND deleted=""'); + db_execute_prepared('UPDATE host + SET thold_send_email = ? + WHERE id = ? + AND deleted = ""', + [get_request_var('notification_action'), $selected_items[$i]]); if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_warning = 0 - WHERE td.host_id=' . $selected_items[$i] . ' + WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL) - AND td.notify_warning=' . get_request_var('id')); + AND td.notify_warning = ?', + [$selected_items[$i], get_request_var('id')]); } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute('UPDATE thold_data AS td + db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id - SET td.notify_alert=0 - WHERE td.host_id=' . $selected_items[$i] . ' + SET td.notify_alert = 0 + WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL) - AND td.notify_alert=' . get_request_var('id')); + AND td.notify_alert = ?', + [$selected_items[$i], get_request_var('id')]); } } } @@ -369,24 +392,27 @@ function form_actions() { get_filter_request_var('notification_action'); if (get_request_var('drp_action') == '1') { // associate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute('UPDATE thold_template - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_template + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_template - SET notify_warning_extra='' - WHERE id=" . $selected_items[$i]); + db_execute_prepared("UPDATE thold_template + SET notify_warning_extra = '' + WHERE id = ?", + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_template - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_template + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); } } @@ -394,43 +420,49 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute('UPDATE thold_template - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_template + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_template - SET notify_extra='' - WHERE id=" . $selected_items[$i]); - - db_execute('DELETE FROM plugin_thold_template_contact - WHERE template_id=' . $selected_items[$i]); + db_execute_prepared("UPDATE thold_template + SET notify_extra = '' + WHERE id = ?", + [$selected_items[$i]]); + + db_execute_prepared('DELETE FROM plugin_thold_template_contact + WHERE template_id = ?', + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_template - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_template + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); } } thold_template_update_thresholds($selected_items[$i]); } } elseif (get_request_var('drp_action') == '2') { // disassociate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute('UPDATE thold_template - SET notify_warning=0 - WHERE id=' . $selected_items[$i] . ' - AND notify_warning=' . get_request_var('id')); + db_execute_prepared('UPDATE thold_template + SET notify_warning = 0 + WHERE id = ? + AND notify_warning = ?', + [$selected_items[$i], get_request_var('id')]); } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute('UPDATE thold_template - SET notify_alert=0 - WHERE id=' . $selected_items[$i] . ' - AND notify_alert=' . get_request_var('id')); + db_execute_prepared('UPDATE thold_template + SET notify_alert = 0 + WHERE id = ? + AND notify_alert = ?', + [$selected_items[$i], get_request_var('id')]); } thold_template_update_thresholds($selected_items[$i]); @@ -447,24 +479,27 @@ function form_actions() { get_filter_request_var('notification_action'); if (get_request_var('drp_action') == '1') { // associate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute('UPDATE thold_data - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_data + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_data - SET notify_warning_extra='' - WHERE id=" . $selected_items[$i]); + db_execute_prepared("UPDATE thold_data + SET notify_warning_extra = '' + WHERE id = ?", + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_data - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_data + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); } } @@ -472,40 +507,47 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute('UPDATE thold_data - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_data + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); // clear other items - db_execute("UPDATE thold_data - SET notify_extra='' - WHERE id=" . $selected_items[$i]); - - db_execute('DELETE FROM plugin_thold_threshold_contact WHERE thold_id=' . $selected_items[$i]); + db_execute_prepared("UPDATE thold_data + SET notify_extra = '' + WHERE id = ?", + [$selected_items[$i]]); + + db_execute_prepared('DELETE FROM plugin_thold_threshold_contact + WHERE thold_id = ?', + [$selected_items[$i]]); } else { // set the notification list - db_execute('UPDATE thold_data - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + db_execute_prepared('UPDATE thold_data + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]); } } } } elseif (get_request_var('drp_action') == '2') { // disassociate - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute('UPDATE thold_data - SET notify_warning=0 - WHERE id=' . $selected_items[$i] . ' - AND notify_warning=' . get_request_var('id')); + db_execute_prepared('UPDATE thold_data + SET notify_warning = 0 + WHERE id = ? + AND notify_warning = ?', + [$selected_items[$i], get_request_var('id')]); } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute('UPDATE thold_data - SET notify_alert=0 - WHERE id=' . $selected_items[$i] . ' - AND notify_alert=' . get_request_var('id')); + db_execute_prepared('UPDATE thold_data + SET notify_alert = 0 + WHERE id = ? + AND notify_alert = ?', + [$selected_items[$i], get_request_var('id')]); } } } diff --git a/tests/Security/Php74CompatibilityTest.php b/tests/Security/Php74CompatibilityTest.php new file mode 100644 index 00000000..ce53f1a8 --- /dev/null +++ b/tests/Security/Php74CompatibilityTest.php @@ -0,0 +1,77 @@ +not->toBeFalse("Failed to resolve target file path: {$relativeFile}"); + + $contents = file_get_contents($path); + expect($contents)->not->toBeFalse("Failed to read target file: {$relativeFile}"); + + return $contents; +} + +it('does not use str_contains (PHP 8.0)', function () { + foreach (thold_security_compatibility_files() as $relativeFile) { + $contents = thold_security_read_file($relativeFile); + + expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, + "{$relativeFile} uses str_contains() which requires PHP 8.0" + ); + } +}); + +it('does not use str_starts_with (PHP 8.0)', function () { + foreach (thold_security_compatibility_files() as $relativeFile) { + $contents = thold_security_read_file($relativeFile); + + expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, + "{$relativeFile} uses str_starts_with() which requires PHP 8.0" + ); + } +}); + +it('does not use str_ends_with (PHP 8.0)', function () { + foreach (thold_security_compatibility_files() as $relativeFile) { + $contents = thold_security_read_file($relativeFile); + + expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, + "{$relativeFile} uses str_ends_with() which requires PHP 8.0" + ); + } +}); + +it('does not use nullsafe operator (PHP 8.0)', function () { + foreach (thold_security_compatibility_files() as $relativeFile) { + $contents = thold_security_read_file($relativeFile); + + expect(preg_match('/\?->/', $contents))->toBe(0, + "{$relativeFile} uses nullsafe operator which requires PHP 8.0" + ); + } +}); diff --git a/tests/Security/PreparedStatementTest.php b/tests/Security/PreparedStatementTest.php new file mode 100644 index 00000000..e115b8b2 --- /dev/null +++ b/tests/Security/PreparedStatementTest.php @@ -0,0 +1,66 @@ +not->toContain('array_to_sql_or($selected_items'); +}); + +it('notify_lists.php delete action uses db_execute_prepared with IN placeholders', function () use ($notify_src) { + expect($notify_src)->toContain('db_execute_prepared(\'DELETE FROM plugin_notification_lists'); + expect($notify_src)->toContain('$placeholders'); +}); + +it('notify_lists.php associate action uses db_execute_prepared for host updates', function () use ($notify_src) { + expect($notify_src)->toContain('db_execute_prepared(\'UPDATE host'); + expect($notify_src)->toContain('SET thold_host_email = ?'); +}); + +it('notify_lists.php does not concatenate selected_items[$i] directly into SQL strings', function () use ($notify_src) { + expect(preg_match("/WHERE id='\s*\\.\\s*\\\$selected_items/", $notify_src))->toBe(0); + expect(preg_match('/WHERE id=\' \. \$selected_items/', $notify_src))->toBe(0); + expect(preg_match('/WHERE id=' . "'" . ' \. \$selected_items/', $notify_src))->toBe(0); +}); + +it('notify_lists.php uses cacti_sizeof instead of count for selected_items loops', function () use ($notify_src) { + expect($notify_src)->toContain('cacti_sizeof($selected_items)'); + expect(preg_match('/count\(\$selected_items\)/', $notify_src))->toBe(0); +}); + +it('thold_functions.php get_allowed_thresholds uses db_fetch_assoc_prepared', function () use ($funcs_src) { + expect($funcs_src)->toContain('db_fetch_assoc_prepared($tholds_sql, $sql_params)'); +}); + +it('thold_functions.php get_allowed_thresholds uses db_fetch_cell_prepared for row count', function () use ($funcs_src) { + expect($funcs_src)->toContain('db_fetch_cell_prepared($sql, $sql_params)'); +}); + +it('thold_functions.php get_allowed_thresholds does not interpolate graph_id directly', function () use ($funcs_src) { + expect($funcs_src)->not->toContain('gl.id=$graph_id'); + expect($funcs_src)->not->toContain('gl.id = $graph_id'); +}); + +it('thold_functions.php get_allowed_threshold_logs uses db_fetch_assoc_prepared', function () use ($funcs_src) { + expect($funcs_src)->toContain('db_fetch_assoc_prepared("SELECT'); +}); diff --git a/tests/Smoke/PhpSyntaxTest.php b/tests/Smoke/PhpSyntaxTest.php new file mode 100644 index 00000000..b5a5d8ee --- /dev/null +++ b/tests/Smoke/PhpSyntaxTest.php @@ -0,0 +1,59 @@ +getExtension() !== 'php') { + continue; + } + + $rel = ltrim(str_replace($root, '', $file->getPathname()), DIRECTORY_SEPARATOR); + + if (str_starts_with($rel, 'vendor' . DIRECTORY_SEPARATOR)) { + continue; + } + + if (str_starts_with($rel, 'tests' . DIRECTORY_SEPARATOR)) { + continue; + } + + $files[] = $file->getPathname(); +} + +sort($files); + +it('all plugin PHP files parse without errors', function () use ($phpBin, $files) { + $failures = []; + + foreach ($files as $file) { + // bare escapeshellarg(): cacti_escapeshellarg() requires the Cacti bootstrap; $file is a local path, not user input + exec("$phpBin -l " . escapeshellarg($file) . ' 2>&1', $output, $code); // nosemgrep: php.lang.security.exec-use.exec-use -- lint check only; $file is a glob-returned server-local path with no user input + if ($code !== 0) { + $failures[] = basename($file) . ': ' . implode(' ', $output); + } + $output = []; + } + + expect($failures)->toBe([]); +}); diff --git a/thold_functions.php b/thold_functions.php index 686ac1be..259e1d6e 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -1226,7 +1226,7 @@ function thold_calculate_lower_upper($thold, $currentval, $rrd_reindexed) { return $currentval; } -function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0) { +function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0, $sql_params = []) { if ($sql_limit != '') { $sql_limit = "LIMIT $sql_limit"; } @@ -1236,7 +1236,8 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim } if ($graph_id > 0) { - $sql_where .= (strlen($sql_where) ? ' AND ' : ' ') . " gl.id=$graph_id"; + $sql_where .= (strlen($sql_where) ? ' AND ' : ' ') . ' gl.id = ?'; + $sql_params[] = $graph_id; } if (strlen($sql_where)) { @@ -1292,7 +1293,7 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim $order_by $sql_limit"); - $tholds = db_fetch_assoc($tholds_sql); + $tholds = db_fetch_assoc_prepared($tholds_sql, $sql_params); $sql = "SELECT COUNT(*) FROM ( @@ -1310,15 +1311,15 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim ) AS rower"; if (function_exists('get_total_row_data') && $graph_id == 0) { - $total_rows = get_total_row_data($user_id, $sql, [], 'thold', 10); + $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold', 10); } else { - $total_rows = db_fetch_cell($sql); + $total_rows = db_fetch_cell_prepared($sql, $sql_params); } return $tholds; } -function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0) { +function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0, $sql_params = []) { if ($sql_limit != '') { $sql_limit = "LIMIT $sql_limit"; } @@ -1328,7 +1329,8 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql } if ($graph_id > 0) { - $sql_where .= (strlen($sql_where) ? ' AND ' : ' ') . " gl.id = $graph_id"; + $sql_where .= (strlen($sql_where) ? ' AND ' : ' ') . ' gl.id = ?'; + $sql_params[] = $graph_id; } if (strlen($sql_where)) { @@ -1362,7 +1364,7 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql $sql_where = get_policy_where($graph_auth_method, $policies, $sql_where); } - $tholds = db_fetch_assoc("SELECT + $tholds = db_fetch_assoc_prepared("SELECT tl.`id`, tl.`time`, tl.`host_id`, tl.`local_graph_id`, tl.`threshold_id`, IF(IFNULL(tl.`threshold_value`,'')='',NULL,(tl.`threshold_value` + 0.0)) AS `threshold_value`, IF(IFNULL(tl.`current`,'')='',NULL,(tl.`current` + 0.0)) AS `current`, tl.`status`, tl.`type`, @@ -1380,7 +1382,7 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql ON h.id=gl.host_id $sql_where $order_by - $sql_limit"); + $sql_limit", $sql_params); $sql = "SELECT COUNT(*) FROM ( @@ -1400,9 +1402,9 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql ) AS rower"; if (function_exists('get_total_row_data') && $graph_id == 0) { - $total_rows = get_total_row_data($user_id, $sql, [], 'thold_log', 10); + $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold_log', 10); } else { - $total_rows = db_fetch_cell($sql); + $total_rows = db_fetch_cell_prepared($sql, $sql_params); } return $tholds; From a57493f7bfb4c7b7fae19522d2e331b98259180e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:58:30 -0700 Subject: [PATCH 05/41] fix(security): wrap AJAX URL params with encodeURIComponent Prevents open redirect via URL manipulation in JS filter forms. Affects thold.php, thold_graph.php, notify_lists.php, notify_queue.php, thold_templates.php, and thold_webapi.php. Signed-off-by: Thomas Vincent --- notify_lists.php | 35 ++++++++++++++++++------------ notify_queue.php | 8 +++---- thold.php | 12 +++++------ thold_graph.php | 52 ++++++++++++++++++++++----------------------- thold_templates.php | 2 +- thold_webapi.php | 18 ++++++++-------- 6 files changed, 68 insertions(+), 59 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 5498968e..8a8f98ec 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -147,6 +147,15 @@ function form_actions() { // ================= input validation ================= get_filter_request_var('drp_action'); + + $valid_actions = array_keys($actions + $assoc_actions); + + if (!in_array(get_request_var('drp_action'), $valid_actions, true) && + !in_array((int) get_request_var('drp_action'), $valid_actions, true)) { + raise_message(40); + header('Location: notify_lists.php'); + exit; + } // ==================================================== // if we are to save this form, instead of display it @@ -1181,11 +1190,11 @@ function hosts($header_label) { function applyFilter() { strURL = '?header=false&action=edit&id=' - strURL += '&rows=' + $('#rows').val(); - strURL += '&host_template_id=' + $('#host_template_id').val(); - strURL += '&site_id=' + $('#site_id').val(); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&host_template_id=' + encodeURIComponent($('#host_template_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); strURL += '&associated=' + $('#associated').is(':checked'); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -1551,11 +1560,11 @@ function tholds($header_label) { function applyFilter() { strURL = 'notify_lists.php?header=false&action=edit&tab=tholds&id=' strURL += '&associated=' + $('#associated').is(':checked'); - strURL += '&state=' + $('#state').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&template=' + $('#template').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&state=' + encodeURIComponent($('#state').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&template=' + encodeURIComponent($('#template').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -1840,8 +1849,8 @@ function templates($header_label) { function applyFilter() { strURL = 'notify_lists.php?header=false&action=edit&tab=templates&id=' strURL += '&associated=' + $('#associated').is(':checked'); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -2159,8 +2168,8 @@ function lists() { function applyFilter() { strURL = 'notify_lists.php?header=false'; - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } diff --git a/notify_queue.php b/notify_queue.php index 12f24645..8117483b 100644 --- a/notify_queue.php +++ b/notify_queue.php @@ -318,10 +318,10 @@ function notify_queue() { function applyFilter() { strURL = 'notify_queue.php?header=false'; - strURL += '&filter='+$('#filter').val(); - strURL += '&rows='+$('#rows').val(); - strURL += '&processed='+$('#processed').val(); - strURL += '&topic='+$('#topic').val(); + strURL += '&filter='+encodeURIComponent($('#filter').val()); + strURL += '&rows='+encodeURIComponent($('#rows').val()); + strURL += '&processed='+encodeURIComponent($('#processed').val()); + strURL += '&topic='+encodeURIComponent($('#topic').val()); loadPageNoHeader(strURL); } diff --git a/thold.php b/thold.php index b0c9f790..8a309ffb 100644 --- a/thold.php +++ b/thold.php @@ -769,12 +769,12 @@ function list_tholds() { function applyFilter() { strURL = 'thold.php?header=false&host_id=' + $('#host_id').val(); - strURL += '&state=' + $('#state').val(); - strURL += '&thold_template_id=' + $('#thold_template_id').val(); - strURL += '&data_template_id=' + $('#data_template_id').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&state=' + encodeURIComponent($('#state').val()); + strURL += '&thold_template_id=' + encodeURIComponent($('#thold_template_id').val()); + strURL += '&data_template_id=' + encodeURIComponent($('#data_template_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } diff --git a/thold_graph.php b/thold_graph.php index 462820c2..7629e450 100644 --- a/thold_graph.php +++ b/thold_graph.php @@ -258,13 +258,13 @@ function form_thold_filter() { function applyFilter() { strURL = 'thold_graph.php?header=false&action=thold'; - strURL += '&state=' + $('#state').val(); - strURL += '&thold_template_id=' + $('#thold_template_id').val(); - strURL += '&data_template_id=' + $('#data_template_id').val(); - strURL += '&host_id=' + $('#host_id').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&state=' + encodeURIComponent($('#state').val()); + strURL += '&thold_template_id=' + encodeURIComponent($('#thold_template_id').val()); + strURL += '&data_template_id=' + encodeURIComponent($('#data_template_id').val()); + strURL += '&host_id=' + encodeURIComponent($('#host_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -1268,11 +1268,11 @@ function form_host_filter() { function applyFilter() { strURL = 'thold_graph.php?header=false&action=hoststat'; - strURL += '&host_status=' + $('#host_status').val(); - strURL += '&host_template_id=' + $('#host_template_id').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&host_status=' + encodeURIComponent($('#host_status').val()); + strURL += '&host_template_id=' + encodeURIComponent($('#host_template_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -1734,13 +1734,13 @@ function form_thold_log_filter() { function applyFilter() { strURL = 'thold_graph.php?header=false&action=log'; - strURL += '&status=' + $('#status').val(); - strURL += '&threshold_id=' + $('#threshold_id').val(); - strURL += '&thold_template_id=' + $('#thold_template_id').val(); - strURL += '&host_id=' + $('#host_id').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&status=' + encodeURIComponent($('#status').val()); + strURL += '&threshold_id=' + encodeURIComponent($('#threshold_id').val()); + strURL += '&thold_template_id=' + encodeURIComponent($('#thold_template_id').val()); + strURL += '&host_id=' + encodeURIComponent($('#host_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); loadPageNoHeader(strURL); } @@ -1751,13 +1751,13 @@ function clearFilter() { function exportLog() { strURL = 'thold_graph.php?action=exportlog'; - strURL += '&status=' + $('#status').val(); - strURL += '&threshold_id=' + $('#threshold_id').val(); - strURL += '&thold_template_id=' + $('#thold_template_id').val(); - strURL += '&host_id=' + $('#host_id').val(); - strURL += '&site_id=' + $('#site_id').val(); - strURL += '&rows=' + $('#rows').val(); - strURL += '&rfilter=' + base64_encode($('#rfilter').val()); + strURL += '&status=' + encodeURIComponent($('#status').val()); + strURL += '&threshold_id=' + encodeURIComponent($('#threshold_id').val()); + strURL += '&thold_template_id=' + encodeURIComponent($('#thold_template_id').val()); + strURL += '&host_id=' + encodeURIComponent($('#host_id').val()); + strURL += '&site_id=' + encodeURIComponent($('#site_id').val()); + strURL += '&rows=' + encodeURIComponent($('#rows').val()); + strURL += '&rfilter=' + encodeURIComponent(base64_encode($('#rfilter').val())); document.location = strURL; Pace.stop(); } diff --git a/thold_templates.php b/thold_templates.php index ea379ef2..3dd7db97 100644 --- a/thold_templates.php +++ b/thold_templates.php @@ -2186,7 +2186,7 @@ function templates() { function applyFilter() { strURL = 'thold_templates.php?header=false&rows=' + $('#rows').val(); - strURL += '&filter=' + $('#filter').val(); + strURL += '&filter=' + encodeURIComponent($('#filter').val()); loadPageNoHeader(strURL); } diff --git a/thold_webapi.php b/thold_webapi.php index 8e0e891d..09c90a8c 100644 --- a/thold_webapi.php +++ b/thold_webapi.php @@ -793,39 +793,39 @@ function thold_wizard() { function applyTholdFilter() { strURL = 'thold.php?action=add&header=false'; - strURL += '&type_id=' + $('#type_id').val(); + strURL += '&type_id=' + encodeURIComponent($('#type_id').val()); if ($('#type_id').val() == 'thold') { if ($('#my_host_id').length && $('#my_host_id').val() > 0) { - strURL += '&my_host_id=' + $('#my_host_id').val(); + strURL += '&my_host_id=' + encodeURIComponent($('#my_host_id').val()); } if ($('#local_graph_id').length && $('#local_graph_id').val() > 0) { - strURL += '&local_graph_id=' + $('#local_graph_id').val(); + strURL += '&local_graph_id=' + encodeURIComponent($('#local_graph_id').val()); } if ($('#data_template_rrd_id').length && $('#data_template_rrd_id').val() > 0) { - strURL += '&data_template_rrd_id=' + $('#data_template_rrd_id').val(); + strURL += '&data_template_rrd_id=' + encodeURIComponent($('#data_template_rrd_id').val()); } } else { if ($('#thold_template_id').length && $('#thold_template_id').val() > 0) { - strURL += '&thold_template_id=' + $('#thold_template_id').val(); + strURL += '&thold_template_id=' + encodeURIComponent($('#thold_template_id').val()); } if ($('#graph_template_id').length && $('#graph_template_id').val() > 0) { - strURL += '&graph_template_id=' + $('#graph_template_id').val(); + strURL += '&graph_template_id=' + encodeURIComponent($('#graph_template_id').val()); } if ($('#data_query_id').length && $('#data_query_id').val() > 0) { - strURL += '&data_query_id=' + $('#data_query_id').val(); + strURL += '&data_query_id=' + encodeURIComponent($('#data_query_id').val()); } if ($('#data_template_id').length && $('#data_template_id').val() > 0) { - strURL += '&data_template_id=' + $('#data_template_id').val(); + strURL += '&data_template_id=' + encodeURIComponent($('#data_template_id').val()); } if ($('#my_host_id').length && $('#my_host_id').val() != 0) { - strURL += '&my_host_id=' + $('#my_host_id').val(); + strURL += '&my_host_id=' + encodeURIComponent($('#my_host_id').val()); } if ($('#snmp_index').length && $('#snmp_index').val() != '') { From 4922ffff45e81b4f3c9b7dbe8881b22df2fc702c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:59:48 -0700 Subject: [PATCH 06/41] test(security): add encodeURIComponent regression tests for AJAX filters Signed-off-by: Thomas Vincent --- tests/Security/XssEscapingTest.php | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/Security/XssEscapingTest.php b/tests/Security/XssEscapingTest.php index 31f6742e..65f3dbca 100644 --- a/tests/Security/XssEscapingTest.php +++ b/tests/Security/XssEscapingTest.php @@ -34,3 +34,20 @@ // ENT_QUOTES|ENT_HTML5 encodes single quotes as ' (HTML5 named entity) expect(html_escape("O'Brien"))->toBe('O'Brien'); }); + +it('thold.php AJAX filter uses encodeURIComponent for URL params', function () { + $src = file_get_contents(realpath(__DIR__ . '/../../thold.php')); + // rfilter is base64-encoded then URI-encoded; other params are URI-encoded directly + expect($src)->toContain("encodeURIComponent(base64_encode($('#rfilter').val()))"); + expect($src)->toContain("encodeURIComponent($('#rows').val())"); +}); + +it('thold_graph.php AJAX filter uses encodeURIComponent for URL params', function () { + $src = file_get_contents(realpath(__DIR__ . '/../../thold_graph.php')); + expect($src)->toContain('encodeURIComponent'); +}); + +it('notify_lists.php AJAX filter uses encodeURIComponent for URL params', function () { + $src = file_get_contents(realpath(__DIR__ . '/../../notify_lists.php')); + expect($src)->toContain('encodeURIComponent'); +}); From 8903631a40966d8a264f4f2ec970a7b456d62dcb Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:06:50 -0700 Subject: [PATCH 07/41] fix(compat): replace str_starts_with with strncmp for PHP 7.4 PhpSyntaxTest.php used str_starts_with() (PHP 8.0+) which would fatal under PHP 7.4 before any assertion ran. Use strncmp() instead. Also remove redundant (int) cast and double in_array check on drp_action in notify_lists.php: get_filter_request_var() already validated the value; the second clause was dead and confusing. Replace count() with cacti_sizeof() in setup.php bulk loop. Signed-off-by: Thomas Vincent --- notify_lists.php | 3 +-- setup.php | 2 +- tests/Smoke/PhpSyntaxTest.php | 4 ++-- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 8a8f98ec..257fabf7 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -150,8 +150,7 @@ function form_actions() { $valid_actions = array_keys($actions + $assoc_actions); - if (!in_array(get_request_var('drp_action'), $valid_actions, true) && - !in_array((int) get_request_var('drp_action'), $valid_actions, true)) { + if (!in_array(get_request_var('drp_action'), $valid_actions, true)) { raise_message(40); header('Location: notify_lists.php'); exit; diff --git a/setup.php b/setup.php index b9aedfca..45298018 100644 --- a/setup.php +++ b/setup.php @@ -691,7 +691,7 @@ function thold_device_action_execute($action) { $selected_items = sanitize_unserialize_selected_items(get_nfilter_request_var('selected_items')); if ($selected_items != false) { - for ($i = 0; ($i < count($selected_items)); $i++) { + for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { autocreate($selected_items[$i]); } } diff --git a/tests/Smoke/PhpSyntaxTest.php b/tests/Smoke/PhpSyntaxTest.php index b5a5d8ee..1fa16352 100644 --- a/tests/Smoke/PhpSyntaxTest.php +++ b/tests/Smoke/PhpSyntaxTest.php @@ -30,11 +30,11 @@ $rel = ltrim(str_replace($root, '', $file->getPathname()), DIRECTORY_SEPARATOR); - if (str_starts_with($rel, 'vendor' . DIRECTORY_SEPARATOR)) { + if (strncmp($rel, 'vendor' . DIRECTORY_SEPARATOR, strlen('vendor' . DIRECTORY_SEPARATOR)) === 0) { continue; } - if (str_starts_with($rel, 'tests' . DIRECTORY_SEPARATOR)) { + if (strncmp($rel, 'tests' . DIRECTORY_SEPARATOR, strlen('tests' . DIRECTORY_SEPARATOR)) === 0) { continue; } From e13f1a77a875e32455577c425b73dd3c3423d424 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:14:48 -0700 Subject: [PATCH 08/41] fix(guard): cast drp_action valid-actions to strings for strict in_array array_keys($actions + $assoc_actions) returns integer keys; POST values are always strings. Without the strval() cast, in_array(..., true) with strict comparison always fails, making every bulk form action unreachable. Adds regression test asserting the strval() cast is present. Signed-off-by: Thomas Vincent --- notify_lists.php | 2 +- tests/Security/PreparedStatementTest.php | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/notify_lists.php b/notify_lists.php index 257fabf7..206b8359 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -148,7 +148,7 @@ function form_actions() { // ================= input validation ================= get_filter_request_var('drp_action'); - $valid_actions = array_keys($actions + $assoc_actions); + $valid_actions = array_map('strval', array_keys($actions + $assoc_actions)); if (!in_array(get_request_var('drp_action'), $valid_actions, true)) { raise_message(40); diff --git a/tests/Security/PreparedStatementTest.php b/tests/Security/PreparedStatementTest.php index e115b8b2..ee9dbb9c 100644 --- a/tests/Security/PreparedStatementTest.php +++ b/tests/Security/PreparedStatementTest.php @@ -64,3 +64,9 @@ it('thold_functions.php get_allowed_threshold_logs uses db_fetch_assoc_prepared', function () use ($funcs_src) { expect($funcs_src)->toContain('db_fetch_assoc_prepared("SELECT'); }); + +it('notify_lists.php drp_action guard converts keys to strings before strict comparison', function () use ($notify_src) { + // array_keys() returns int keys; POST values are strings; strval() cast allows strict in_array() + expect($notify_src)->toContain("array_map('strval', array_keys(\$actions + \$assoc_actions))"); + expect($notify_src)->toContain("in_array(get_request_var('drp_action'), \$valid_actions, true)"); +}); From c9035b2bd5360bea360d4ef115f26e0c114654c5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:24:35 -0700 Subject: [PATCH 09/41] fix(validation): add gfrv() calls for id and action fields in bulk handlers All three action-save blocks (save_associate, save_templates, save_tholds) now call get_filter_request_var() for id, notification_action, notification_warning_action, and notification_alert_action before consuming those values via get_request_var() in prepared-statement params. Add inline comment on all RLIKE db_qstr() sites documenting the dual guard: FILTER_VALIDATE_IS_REGEX pre-validates; db_qstr() SQL-escapes. Signed-off-by: Thomas Vincent --- notify_lists.php | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/notify_lists.php b/notify_lists.php index 206b8359..f8d083ab 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -255,7 +255,10 @@ function form_actions() { if (isset_request_var('save_associate')) { if ($selected_items != false) { + get_filter_request_var('id'); get_filter_request_var('notification_action'); + get_filter_request_var('notification_warning_action'); + get_filter_request_var('notification_alert_action'); if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -397,7 +400,10 @@ function form_actions() { if (isset_request_var('save_templates')) { if ($selected_items != false) { + get_filter_request_var('id'); get_filter_request_var('notification_action'); + get_filter_request_var('notification_warning_action'); + get_filter_request_var('notification_alert_action'); if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -484,7 +490,10 @@ function form_actions() { if (isset_request_var('save_tholds')) { if ($selected_items != false) { + get_filter_request_var('id'); get_filter_request_var('notification_action'); + get_filter_request_var('notification_warning_action'); + get_filter_request_var('notification_alert_action'); if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -1449,6 +1458,8 @@ function tholds($header_label) { } if (strlen(get_request_var('rfilter'))) { + // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the + // request validation array; db_qstr() SQL-escapes the already-validated value. $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); } @@ -1789,6 +1800,8 @@ function templates($header_label) { } if (strlen(get_request_var('rfilter'))) { + // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the + // request validation array; db_qstr() SQL-escapes the already-validated value. $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . 'thold_template.name RLIKE ' . db_qstr(get_request_var('rfilter')); } @@ -2193,6 +2206,8 @@ function clearFilter() { // form the 'where' clause for our main sql query if (strlen(get_request_var('rfilter'))) { + // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the + // request validation array; db_qstr() SQL-escapes the already-validated value. $sql_where = 'WHERE ( name RLIKE ' . db_qstr(get_request_var('rfilter')) . ' OR description RLIKE ' . db_qstr(get_request_var('rfilter')) . ' From 62de01c7a0d178d7a61503675f2e8a236da3783a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:32:11 -0700 Subject: [PATCH 10/41] test(security): add PreparedStatementConsistencyTest from #769 Verifies that poller_thold.php, setup.php, thold.php, and thold_graph.php contain no single-line raw db_*() calls with interpolated variables. Signed-off-by: Thomas Vincent --- .../PreparedStatementConsistencyTest.php | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tests/Security/PreparedStatementConsistencyTest.php diff --git a/tests/Security/PreparedStatementConsistencyTest.php b/tests/Security/PreparedStatementConsistencyTest.php new file mode 100644 index 00000000..441b8a0d --- /dev/null +++ b/tests/Security/PreparedStatementConsistencyTest.php @@ -0,0 +1,50 @@ +not->toBeFalse("Failed to resolve target file path: {$relativeFile}"); + + $contents = file_get_contents($path); + expect($contents)->not->toBeFalse("Failed to read target file: {$relativeFile}"); + + $lines = explode("\n", $contents); + + foreach ($lines as $lineNumber => $line) { + $trimmed = ltrim($line); + + if (strpos($trimmed, '//') === 0 || strpos($trimmed, '*') === 0 || strpos($trimmed, '#') === 0) { + continue; + } + + $hasInterpolatedRawCall = preg_match($rawInterpolatedPattern, $line) === 1; + $hasPreparedCall = preg_match($preparedPattern, $line) === 1; + + expect($hasInterpolatedRawCall && !$hasPreparedCall)->toBeFalse( + sprintf('File %s contains an interpolated raw db_* call at line %d', $relativeFile, $lineNumber + 1) + ); + } + } +}); From a24496a737d0f941b2e1bfcff9afd4435072455b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:39:13 -0700 Subject: [PATCH 11/41] docs(api): document get_total_row_data third-arg contract at call sites get_total_row_data accepts $sql_params since Cacti 1.2.x (lib/auth.php:3120). Both call sites now carry a comment referencing the function signature so future callers understand the API assumption. Signed-off-by: Thomas Vincent --- thold_functions.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/thold_functions.php b/thold_functions.php index 259e1d6e..65376f6c 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -1310,6 +1310,8 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim $sql_where ) AS rower"; + // get_total_row_data signature: ($user_id, $sql, $sql_params, $class, $timeout) + // The third param is accepted since Cacti 1.2.x (lib/auth.php:3120). if (function_exists('get_total_row_data') && $graph_id == 0) { $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold', 10); } else { @@ -1401,6 +1403,8 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql $sql_where ) AS rower"; + // get_total_row_data signature: ($user_id, $sql, $sql_params, $class, $timeout) + // The third param is accepted since Cacti 1.2.x (lib/auth.php:3120). if (function_exists('get_total_row_data') && $graph_id == 0) { $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold_log', 10); } else { From 570b37bf0644ad0d108cbfd7a1608ba641d7ce4c Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:48:14 -0700 Subject: [PATCH 12/41] fix(atomicity): wrap bulk notify-list writes in transactions All four form_actions() bulk write blocks now use db_begin_transaction() / db_commit_transaction() so a partial failure cannot leave notification routing state inconsistent across plugin_notification_lists, host, thold_data, and thold_template tables. Adds db_begin/commit/rollback_transaction stubs to test bootstrap and a regression test asserting all four blocks carry transaction guards. Signed-off-by: Thomas Vincent --- notify_lists.php | 16 ++++++++++++++++ tests/Security/PreparedStatementTest.php | 9 +++++++++ tests/bootstrap.php | 24 ++++++++++++++++++++++++ 3 files changed, 49 insertions(+) diff --git a/notify_lists.php b/notify_lists.php index f8d083ab..a15c697c 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -166,6 +166,8 @@ function form_actions() { if (get_request_var('drp_action') == '1') { // delete $placeholders = implode(',', array_fill(0, cacti_sizeof($selected_items), '?')); + db_begin_transaction(); + db_execute_prepared('DELETE FROM plugin_notification_lists WHERE id IN (' . $placeholders . ')', $selected_items); @@ -209,6 +211,8 @@ function form_actions() { SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', $selected_items); + + db_commit_transaction(); } elseif (get_request_var('drp_action') == '2') { // duplicate $i = 1; @@ -260,6 +264,8 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); + db_begin_transaction(); + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { // set the notification list @@ -392,6 +398,8 @@ function form_actions() { } } } + + db_commit_transaction(); } header('Location: notify_lists.php?header=false&action=edit&tab=hosts&id=' . get_request_var('id')); @@ -405,6 +413,8 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); + db_begin_transaction(); + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { @@ -482,6 +492,8 @@ function form_actions() { thold_template_update_thresholds($selected_items[$i]); } } + + db_commit_transaction(); } header('Location: notify_lists.php?header=false&action=edit&tab=templates&id=' . get_request_var('id')); @@ -495,6 +507,8 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); + db_begin_transaction(); + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { @@ -568,6 +582,8 @@ function form_actions() { } } } + + db_commit_transaction(); } header('Location: notify_lists.php?header=false&action=edit&tab=tholds&id=' . get_request_var('id')); diff --git a/tests/Security/PreparedStatementTest.php b/tests/Security/PreparedStatementTest.php index ee9dbb9c..abc78a19 100644 --- a/tests/Security/PreparedStatementTest.php +++ b/tests/Security/PreparedStatementTest.php @@ -70,3 +70,12 @@ expect($notify_src)->toContain("array_map('strval', array_keys(\$actions + \$assoc_actions))"); expect($notify_src)->toContain("in_array(get_request_var('drp_action'), \$valid_actions, true)"); }); + +it('notify_lists.php bulk write actions are wrapped in transactions', function () use ($notify_src) { + // All four bulk action blocks must begin and commit a transaction atomically. + $beginCount = substr_count($notify_src, 'db_begin_transaction()'); + $commitCount = substr_count($notify_src, 'db_commit_transaction()'); + + expect($beginCount)->toBe(4); + expect($commitCount)->toBe(4); +}); diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 1c580f26..8dbbc0b1 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -77,6 +77,30 @@ function db_qstr($string) { } } +if (!function_exists('db_begin_transaction')) { + function db_begin_transaction() { + $GLOBALS['__test_db_calls'][] = ['fn' => 'db_begin_transaction', 'sql' => '', 'params' => []]; + + return true; + } +} + +if (!function_exists('db_commit_transaction')) { + function db_commit_transaction() { + $GLOBALS['__test_db_calls'][] = ['fn' => 'db_commit_transaction', 'sql' => '', 'params' => []]; + + return true; + } +} + +if (!function_exists('db_rollback_transaction')) { + function db_rollback_transaction() { + $GLOBALS['__test_db_calls'][] = ['fn' => 'db_rollback_transaction', 'sql' => '', 'params' => []]; + + return true; + } +} + if (!function_exists('html_escape')) { function html_escape($string) { return htmlspecialchars($string, ENT_QUOTES | ENT_HTML5, 'UTF-8'); From 0095f22ef3d3e800cc3cdf48c43a5e13798d0162 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 01:11:13 -0700 Subject: [PATCH 13/41] fix(atomicity): rollback on db_execute_prepared failure in bulk handlers Track $ok across all db_execute_prepared calls in each of the four bulk action blocks. Call db_rollback_transaction() when any statement returns false instead of committing a partial write. Tests: add rollback-count assertion (4), $ok-flag presence check. Signed-off-by: Thomas Vincent --- notify_lists.php | 184 +++++++++++++---------- tests/Security/PreparedStatementTest.php | 23 ++- 2 files changed, 122 insertions(+), 85 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index a15c697c..688ad306 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -168,51 +168,55 @@ function form_actions() { db_begin_transaction(); - db_execute_prepared('DELETE FROM plugin_notification_lists + $ok = db_execute_prepared('DELETE FROM plugin_notification_lists WHERE id IN (' . $placeholders . ')', $selected_items); - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_send_email = 0 WHERE thold_send_email = 2 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_send_email = 1 WHERE thold_send_email = 3 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_host_email = 0 WHERE thold_host_email IN (' . $placeholders . ') AND deleted = ""', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', - $selected_items); + $selected_items) && $ok; - db_commit_transaction(); + if ($ok) { + db_commit_transaction(); + } else { + db_rollback_transaction(); + } } elseif (get_request_var('drp_action') == '2') { // duplicate $i = 1; @@ -266,51 +270,53 @@ function form_actions() { db_begin_transaction(); + $ok = true; + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { // set the notification list - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_host_email = ? WHERE id = ? AND deleted = ""', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // set the global/list election - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_send_email = ? WHERE id = ? AND deleted = ""', - [get_request_var('notification_action'), $selected_items[$i]]); + [get_request_var('notification_action'), $selected_items[$i]]) && $ok; if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_warning = ? WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_data AS td + $ok = db_execute_prepared("UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_warning_extra = '' WHERE td.host_id = ? AND (tt.notify_templated = \"\" OR tt.notify_templated IS NULL)", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_warning = ? WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -318,25 +324,25 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_alert = ? WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_data AS td + $ok = db_execute_prepared("UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_extra = '' WHERE host_id = ? AND (tt.notify_templated = \"\" OR tt.notify_templated IS NULL)", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; // remove legacy contacts - db_execute_prepared('DELETE pttc + $ok = db_execute_prepared('DELETE pttc FROM plugin_thold_threshold_contact AS pttc INNER JOIN thold_data AS td ON pttc.thold_id = td.id @@ -344,62 +350,66 @@ function form_actions() { ON td.thold_template_id = tt.id WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_alert = ? WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL)', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } } } elseif (get_request_var('drp_action') == '2') { // disassociate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { // set the notification list - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_host_email = 0 WHERE id = ? AND deleted = ""', - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; // set the global/list election - db_execute_prepared('UPDATE host + $ok = db_execute_prepared('UPDATE host SET thold_send_email = ? WHERE id = ? AND deleted = ""', - [get_request_var('notification_action'), $selected_items[$i]]); + [get_request_var('notification_action'), $selected_items[$i]]) && $ok; if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_warning = 0 WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL) AND td.notify_warning = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_data AS td + $ok = db_execute_prepared('UPDATE thold_data AS td LEFT JOIN thold_template AS tt ON td.thold_template_id = tt.id SET td.notify_alert = 0 WHERE td.host_id = ? AND (tt.notify_templated = "" OR tt.notify_templated IS NULL) AND td.notify_alert = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } } } - db_commit_transaction(); + if ($ok) { + db_commit_transaction(); + } else { + db_rollback_transaction(); + } } header('Location: notify_lists.php?header=false&action=edit&tab=hosts&id=' . get_request_var('id')); @@ -415,28 +425,30 @@ function form_actions() { db_begin_transaction(); + $ok = true; + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_warning = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_template + $ok = db_execute_prepared("UPDATE thold_template SET notify_warning_extra = '' WHERE id = ?", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_warning = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -444,26 +456,26 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_alert = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_template + $ok = db_execute_prepared("UPDATE thold_template SET notify_extra = '' WHERE id = ?", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; - db_execute_prepared('DELETE FROM plugin_thold_template_contact + $ok = db_execute_prepared('DELETE FROM plugin_thold_template_contact WHERE template_id = ?', - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_alert = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -473,27 +485,31 @@ function form_actions() { for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_warning = 0 WHERE id = ? AND notify_warning = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_template + $ok = db_execute_prepared('UPDATE thold_template SET notify_alert = 0 WHERE id = ? AND notify_alert = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } thold_template_update_thresholds($selected_items[$i]); } } - db_commit_transaction(); + if ($ok) { + db_commit_transaction(); + } else { + db_rollback_transaction(); + } } header('Location: notify_lists.php?header=false&action=edit&tab=templates&id=' . get_request_var('id')); @@ -509,28 +525,30 @@ function form_actions() { db_begin_transaction(); + $ok = true; + if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // clear other settings if (get_request_var('notification_warning_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_warning = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_data + $ok = db_execute_prepared("UPDATE thold_data SET notify_warning_extra = '' WHERE id = ?", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_warning = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -538,26 +556,26 @@ function form_actions() { // clear other settings if (get_request_var('notification_alert_action') == 1) { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_alert = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute_prepared("UPDATE thold_data + $ok = db_execute_prepared("UPDATE thold_data SET notify_extra = '' WHERE id = ?", - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; - db_execute_prepared('DELETE FROM plugin_thold_threshold_contact + $ok = db_execute_prepared('DELETE FROM plugin_thold_threshold_contact WHERE thold_id = ?', - [$selected_items[$i]]); + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_alert = ? WHERE id = ?', - [get_request_var('id'), $selected_items[$i]]); + [get_request_var('id'), $selected_items[$i]]) && $ok; } } } @@ -565,25 +583,29 @@ function form_actions() { for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_warning = 0 WHERE id = ? AND notify_warning = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute_prepared('UPDATE thold_data + $ok = db_execute_prepared('UPDATE thold_data SET notify_alert = 0 WHERE id = ? AND notify_alert = ?', - [$selected_items[$i], get_request_var('id')]); + [$selected_items[$i], get_request_var('id')]) && $ok; } } } - db_commit_transaction(); + if ($ok) { + db_commit_transaction(); + } else { + db_rollback_transaction(); + } } header('Location: notify_lists.php?header=false&action=edit&tab=tholds&id=' . get_request_var('id')); diff --git a/tests/Security/PreparedStatementTest.php b/tests/Security/PreparedStatementTest.php index abc78a19..3f9ef584 100644 --- a/tests/Security/PreparedStatementTest.php +++ b/tests/Security/PreparedStatementTest.php @@ -72,10 +72,25 @@ }); it('notify_lists.php bulk write actions are wrapped in transactions', function () use ($notify_src) { - // All four bulk action blocks must begin and commit a transaction atomically. - $beginCount = substr_count($notify_src, 'db_begin_transaction()'); - $commitCount = substr_count($notify_src, 'db_commit_transaction()'); - + // All four bulk action blocks must begin a transaction. + $beginCount = substr_count($notify_src, 'db_begin_transaction()'); expect($beginCount)->toBe(4); +}); + +it('notify_lists.php bulk write actions commit on success', function () use ($notify_src) { + $commitCount = substr_count($notify_src, 'db_commit_transaction()'); expect($commitCount)->toBe(4); }); + +it('notify_lists.php bulk write actions rollback on failure', function () use ($notify_src) { + // Each transaction block must have a matching rollback path for when db_execute_prepared returns false. + $rollbackCount = substr_count($notify_src, 'db_rollback_transaction()'); + expect($rollbackCount)->toBe(4); +}); + +it('notify_lists.php bulk write actions track $ok flag for all db_execute_prepared calls', function () use ($notify_src) { + // Every db_execute_prepared result must be ANDed into $ok so partial failure triggers rollback. + expect($notify_src)->toContain('$ok = true'); + expect(preg_match('/\$ok\s*=\s*db_execute_prepared/', $notify_src))->toBe(1); + expect(preg_match('/db_execute_prepared\([^)]+\)\s*&&\s*\$ok/', $notify_src))->toBe(1); +}); From 913124cd959034af84a3563c07c8da15625b6ecd Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 01:23:13 -0700 Subject: [PATCH 14/41] fix(atomicity): break on failure in loops; move template cascade after commit - Add if (!ok) { break; } at end of each per-item loop in save_associate, save_templates, and save_tholds so failed iterations halt immediately. - Move thold_template_update_thresholds calls to after db_commit_transaction() so the cascade does not participate in the transaction boundary. - Apply html_escape() to get_filter_request_var('page') output in thold.php hidden input (mirrors thold_graph.php pattern). - Add tests: loop-break guard count, template-cascade-after-commit, thold.php page XSS fix. Signed-off-by: Thomas Vincent --- notify_lists.php | 38 ++++++++++++++++++++++-- tests/Security/PreparedStatementTest.php | 21 +++++++++++++ tests/Security/XssEscapingTest.php | 6 ++++ thold.php | 2 +- 4 files changed, 63 insertions(+), 4 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 688ad306..21983412 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -362,6 +362,10 @@ function form_actions() { [get_request_var('id'), $selected_items[$i]]) && $ok; } } + + if (!$ok) { + break; + } } } elseif (get_request_var('drp_action') == '2') { // disassociate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -402,6 +406,10 @@ function form_actions() { AND td.notify_alert = ?', [$selected_items[$i], get_request_var('id')]) && $ok; } + + if (!$ok) { + break; + } } } @@ -425,7 +433,8 @@ function form_actions() { db_begin_transaction(); - $ok = true; + $ok = true; + $update_template = []; if (get_request_var('drp_action') == '1') { // associate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -479,7 +488,11 @@ function form_actions() { } } - thold_template_update_thresholds($selected_items[$i]); + $update_template[] = $selected_items[$i]; + + if (!$ok) { + break; + } } } elseif (get_request_var('drp_action') == '2') { // disassociate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -501,12 +514,23 @@ function form_actions() { [$selected_items[$i], get_request_var('id')]) && $ok; } - thold_template_update_thresholds($selected_items[$i]); + $update_template[] = $selected_items[$i]; + + if (!$ok) { + break; + } } } if ($ok) { db_commit_transaction(); + + // Propagate template changes to threshold instances after the + // notification assignment is committed so this cascade does not + // participate in the transaction boundary. + foreach ($update_template as $template_id) { + thold_template_update_thresholds($template_id); + } } else { db_rollback_transaction(); } @@ -578,6 +602,10 @@ function form_actions() { [get_request_var('id'), $selected_items[$i]]) && $ok; } } + + if (!$ok) { + break; + } } } elseif (get_request_var('drp_action') == '2') { // disassociate for ($i = 0; ($i < cacti_sizeof($selected_items)); $i++) { @@ -598,6 +626,10 @@ function form_actions() { AND notify_alert = ?', [$selected_items[$i], get_request_var('id')]) && $ok; } + + if (!$ok) { + break; + } } } diff --git a/tests/Security/PreparedStatementTest.php b/tests/Security/PreparedStatementTest.php index 3f9ef584..ecaf3d71 100644 --- a/tests/Security/PreparedStatementTest.php +++ b/tests/Security/PreparedStatementTest.php @@ -94,3 +94,24 @@ expect(preg_match('/\$ok\s*=\s*db_execute_prepared/', $notify_src))->toBe(1); expect(preg_match('/db_execute_prepared\([^)]+\)\s*&&\s*\$ok/', $notify_src))->toBe(1); }); + +it('notify_lists.php per-item loops break immediately on first failure', function () use ($notify_src) { + // Each loop must break as soon as $ok is false rather than continuing to issue DB calls. + // associate (2 loops) + save_templates (2 loops) + save_tholds (2 loops) = 6 break guards. + // The flat delete sequence has no loop and therefore no break guard. + $breakCount = substr_count($notify_src, 'if (!$ok) {'); + expect($breakCount)->toBeGreaterThanOrEqual(6); + expect($notify_src)->toContain('if (!$ok) {'); + expect($notify_src)->toContain('break;'); +}); + +it('notify_lists.php save_templates calls thold_template_update_thresholds after commit only', function () use ($notify_src) { + // thold_template_update_thresholds must not run inside the transaction boundary + // (it should be called after db_commit_transaction() in the on-success path). + $commitPos = strpos($notify_src, 'db_commit_transaction();' . "\n\n\t\t\t\t\t// Propagate"); + expect($commitPos)->not->toBeFalse(); + // The function must not appear between db_begin_transaction and db_commit_transaction in this block. + $beginPos = strrpos(substr($notify_src, 0, $commitPos), 'db_begin_transaction()'); + $between = substr($notify_src, $beginPos, $commitPos - $beginPos); + expect($between)->not->toContain('thold_template_update_thresholds'); +}); diff --git a/tests/Security/XssEscapingTest.php b/tests/Security/XssEscapingTest.php index 65f3dbca..57c3e12c 100644 --- a/tests/Security/XssEscapingTest.php +++ b/tests/Security/XssEscapingTest.php @@ -51,3 +51,9 @@ $src = file_get_contents(realpath(__DIR__ . '/../../notify_lists.php')); expect($src)->toContain('encodeURIComponent'); }); + +it('thold.php hidden page input uses html_escape', function () { + $src = file_get_contents(realpath(__DIR__ . '/../../thold.php')); + expect($src)->toContain("html_escape(get_filter_request_var('page'))"); + expect($src)->not->toContain("print get_filter_request_var('page')"); +}); diff --git a/thold.php b/thold.php index 8a309ffb..dc400246 100644 --- a/thold.php +++ b/thold.php @@ -763,7 +763,7 @@ function list_tholds() { - '> + '> '))->toBe('<script>alert(1)</script>'); -}); - -it('html_escape converts double quotes to entities', function () { - expect(html_escape('"quoted"'))->toBe('"quoted"'); -}); - -it('html_escape converts single quotes to entities', function () { - // ENT_QUOTES|ENT_HTML5 encodes single quotes as ' (HTML5 named entity) - expect(html_escape("O'Brien"))->toBe('O'Brien'); -}); - -it('thold.php AJAX filter uses encodeURIComponent for URL params', function () { - $src = file_get_contents(realpath(__DIR__ . '/../../thold.php')); - // rfilter is base64-encoded then URI-encoded; other params are URI-encoded directly - expect($src)->toContain("encodeURIComponent(base64_encode($('#rfilter').val()))"); - expect($src)->toContain("encodeURIComponent($('#rows').val())"); -}); - -it('thold_graph.php AJAX filter uses encodeURIComponent for URL params', function () { - $src = file_get_contents(realpath(__DIR__ . '/../../thold_graph.php')); - expect($src)->toContain('encodeURIComponent'); -}); - -it('notify_lists.php AJAX filter uses encodeURIComponent for URL params', function () { - $src = file_get_contents(realpath(__DIR__ . '/../../notify_lists.php')); - expect($src)->toContain('encodeURIComponent'); -}); - -it('thold.php hidden page input uses html_escape', function () { - $src = file_get_contents(realpath(__DIR__ . '/../../thold.php')); - expect($src)->toContain("html_escape(get_filter_request_var('page'))"); - expect($src)->not->toContain("print get_filter_request_var('page')"); -}); diff --git a/tests/Smoke/PhpSyntaxTest.php b/tests/Smoke/PhpSyntaxTest.php deleted file mode 100644 index bce66ee3..00000000 --- a/tests/Smoke/PhpSyntaxTest.php +++ /dev/null @@ -1,60 +0,0 @@ -getExtension() !== 'php') { - continue; - } - - $rel = ltrim(str_replace($root, '', $file->getPathname()), DIRECTORY_SEPARATOR); - - if (strncmp($rel, 'vendor' . DIRECTORY_SEPARATOR, strlen('vendor' . DIRECTORY_SEPARATOR)) === 0) { - continue; - } - - if (strncmp($rel, 'tests' . DIRECTORY_SEPARATOR, strlen('tests' . DIRECTORY_SEPARATOR)) === 0) { - continue; - } - - $files[] = $file->getPathname(); -} - -sort($files); - -it('all plugin PHP files parse without errors', function () use ($phpBin, $files) { - $failures = []; - - foreach ($files as $file) { - // bare escapeshellarg(): cacti_escapeshellarg() requires the Cacti bootstrap; $file is a local path, not user input - exec(escapeshellarg($phpBin) . ' -l ' . escapeshellarg($file) . ' 2>&1', $output, $code); // nosemgrep: php.lang.security.exec-use.exec-use -- lint check only; $file is a glob-returned server-local path with no user input - - if ($code !== 0) { - $failures[] = basename($file) . ': ' . implode(' ', $output); - } - $output = []; - } - - expect($failures)->toBe([]); -}); diff --git a/tests/Support/CactiStub.php b/tests/Support/CactiStub.php new file mode 100644 index 00000000..4c55996b --- /dev/null +++ b/tests/Support/CactiStub.php @@ -0,0 +1,143 @@ +}> + */ + public static $calls = []; + + /** + * Queued return values, keyed by function name. Each call shifts one value + * off the front; an exhausted queue falls back to the type default. + * + * @var array> + */ + public static $returns = []; + + /** + * Values handed back by the get_*_request_var() family, keyed by var name. + * + * @var array + */ + public static $requestVars = []; + + /** + * Values handed back by read_config_option(), keyed by option name. + * + * @var array + */ + public static $configOptions = []; + + /** + * Messages passed to cacti_log(), in order. + * + * @var array + */ + public static $log = []; + + /** + * Clear all recorded and programmed state. + * + * @return void + */ + public static function reset() { + self::$calls = []; + self::$returns = []; + self::$requestVars = []; + self::$configOptions = []; + self::$log = []; + } + + /** + * Record one Cacti function call. + * + * @param string $fn Cacti function name. + * @param string $sql SQL text, or '' for non-query calls. + * @param array $params Bound parameters, if any. + * + * @return void + */ + public static function record($fn, $sql = '', array $params = []) { + self::$calls[] = ['fn' => $fn, 'sql' => $sql, 'params' => $params]; + } + + /** + * Queue one return value for the next call to $fn. + * + * @param string $fn Cacti function name. + * @param mixed $value Value to hand back. + * + * @return void + */ + public static function willReturn($fn, $value) { + self::$returns[$fn][] = $value; + } + + /** + * Take the next queued return value for $fn, or $default when none is left. + * + * @param string $fn Cacti function name. + * @param mixed $default Fallback when the queue is empty. + * + * @return mixed + */ + public static function nextReturn($fn, $default) { + if (!empty(self::$returns[$fn])) { + return array_shift(self::$returns[$fn]); + } + + return $default; + } + + /** + * All recorded calls to $fn. + * + * @param string $fn Cacti function name. + * + * @return array}> + */ + public static function callsTo($fn) { + return array_values(array_filter(self::$calls, function ($call) use ($fn) { + return $call['fn'] === $fn; + })); + } + + /** + * The recorded call log reduced to function names, in order. Useful for + * asserting transaction sequencing. + * + * @return array + */ + public static function callSequence() { + return array_column(self::$calls, 'fn'); + } +} diff --git a/tests/Unit/GetAllowedThresholdsTest.php b/tests/Unit/GetAllowedThresholdsTest.php new file mode 100644 index 00000000..d6301b6a --- /dev/null +++ b/tests/Unit/GetAllowedThresholdsTest.php @@ -0,0 +1,230 @@ + + */ + public static function accessorProvider() { + return array( + 'thresholds' => array('get_allowed_thresholds'), + 'logs' => array('get_allowed_threshold_logs'), + ); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testGraphIdIsBoundRatherThanInterpolated($function): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 42); + + $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('gl.id = ?', $call['sql']); + $this->assertStringNotContainsString('42', $call['sql']); + $this->assertSame(array(42), $call['params']); + } + + /** + * A caller injecting SQL through $graph_id must end up with the payload as + * an inert bound value, never as query text. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testMaliciousGraphIdNeverReachesQueryText($function): void { + $payload = '1 UNION SELECT password FROM user_auth'; + $total = 0; + $function('', 'td.name', '', $total, -1, $payload); + + foreach (CactiStub::$calls as $call) { + $this->assertStringNotContainsString('UNION', $call['sql']); + } + } + + /** + * The $graph_id placeholder is appended after the caller's fragment, so the + * caller's own values have to come first in $sql_params for the binding to + * line up. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testCallerParametersAreBoundBeforeTheGraphIdParameter($function): void { + $total = 0; + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, array(3)); + + $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertSame(array(3, 7), $call['params']); + $this->assertStringContainsString('td.thold_type = ? AND gl.id = ?', $call['sql']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testNoWhereClauseIsEmittedWhenNothingFiltersTheQuery($function): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 0); + + $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringNotContainsString('WHERE', $call['sql']); + $this->assertSame(array(), $call['params']); + } + + /** + * The row-count query reuses the same WHERE clause, so it has to receive + * the same bound values. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testRowCountQueryBindsTheSameParameters($function): void { + $total = 0; + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, array(3)); + + $count = CactiStub::callsTo('db_fetch_cell_prepared')[0]; + + $this->assertSame(array(3, 7), $count['params']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testOrderByAndLimitAreAppliedToTheQuery($function): void { + $total = 0; + $function('', 'td.id DESC', '0,30', $total, -1, 0); + + $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('ORDER BY td.id DESC', $call['sql']); + $this->assertStringContainsString('LIMIT 0,30', $call['sql']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testResultRowsAreReturnedToTheCaller($function): void { + CactiStub::willReturn('db_fetch_assoc_prepared', array(array('id' => 5))); + + $total = 0; + $rows = $function('', 'td.name', '', $total, -1, 0); + + $this->assertSame(array(array('id' => 5)), $rows); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testTotalRowsIsSetByReference($function): void { + CactiStub::willReturn('db_fetch_cell_prepared', 17); + + $total = 0; + $function('', 'td.name', '', $total, -1, 3); + + $this->assertSame(17, $total); + } + + /** + * With authentication on and no session, there is no user to resolve + * permissions against, so the query must not run at all. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testNoQueryRunsWhenAuthenticationIsOnAndNoUserIsResolved($function): void { + CactiStub::$configOptions['auth_method'] = 1; + unset($_SESSION['sess_user_id']); + + $total = 0; + $rows = $function('', 'td.name', '', $total, 0, 0); + + $this->assertSame(array(), $rows); + $this->assertSame(array(), CactiStub::callsTo('db_fetch_assoc_prepared')); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testPolicyWhereIsAppliedWhenPermissionsAreNotSimple($function): void { + CactiStub::$configOptions['auth_method'] = 1; + CactiStub::willReturn('get_simple_graph_perms', false); + CactiStub::willReturn('get_policy_where', 'WHERE policy_applied = 1'); + $_SESSION['sess_user_id'] = 9; + + $total = 0; + $function('', 'td.name', '', $total, 0, 0); + + unset($_SESSION['sess_user_id']); + + $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('policy_applied = 1', $call['sql']); + } +} diff --git a/tests/Unit/TholdCalculateLowerUpperTest.php b/tests/Unit/TholdCalculateLowerUpperTest.php new file mode 100644 index 00000000..46adc2f4 --- /dev/null +++ b/tests/Unit/TholdCalculateLowerUpperTest.php @@ -0,0 +1,68 @@ + 'octets_hi', 'local_data_id' => 4); + $rrd = array(4 => array('octets_hi' => 2)); + + $this->assertSame((2 << 32) + 100, thold_calculate_lower_upper($thold, 100, $rrd)); + } + + /** + * @return void + */ + public function testValuePassesThroughWhenTheHighWordIsAbsent(): void { + $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); + $rrd = array(4 => array('octets_lo' => 5)); + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); + } + + /** + * @return void + */ + public function testValuePassesThroughWhenTheDataSourceHasNoReadings(): void { + $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, array())); + } + + /** + * @return void + */ + public function testHighWordOfZeroLeavesTheValueUnchanged(): void { + $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); + $rrd = array(4 => array('octets_hi' => 0)); + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); + } +} diff --git a/tests/Unit/TholdCalculatePercentTest.php b/tests/Unit/TholdCalculatePercentTest.php new file mode 100644 index 00000000..5678066a --- /dev/null +++ b/tests/Unit/TholdCalculatePercentTest.php @@ -0,0 +1,83 @@ + + */ + private function threshold() { + return array('percent_ds' => 'total', 'local_data_id' => 4); + } + + /** + * @return void + */ + public function testReadingIsExpressedAsAPercentageOfTheReferenceDataSource(): void { + $rrd = array(4 => array('total' => 200)); + + $this->assertSame(25.0, thold_calculate_percent($this->threshold(), 50, $rrd)); + } + + /** + * @return void + */ + public function testNonNumericReadingYieldsTheNoValueSentinel(): void { + $rrd = array(4 => array('total' => 200)); + + $this->assertSame('', thold_calculate_percent($this->threshold(), 'U', $rrd)); + } + + /** + * @return void + */ + public function testMissingReferenceDataSourceYieldsTheNoValueSentinel(): void { + $rrd = array(4 => array('other' => 200)); + + $this->assertSame('', thold_calculate_percent($this->threshold(), 50, $rrd)); + } + + /** + * @return void + */ + public function testZeroReferenceYieldsZeroRatherThanDividingByZero(): void { + $rrd = array(4 => array('total' => 0)); + + $this->assertSame(0, thold_calculate_percent($this->threshold(), 50, $rrd)); + } + + /** + * @return void + */ + public function testNegativeReferenceYieldsZero(): void { + $rrd = array(4 => array('total' => -5)); + + $this->assertSame(0, thold_calculate_percent($this->threshold(), 50, $rrd)); + } +} diff --git a/tests/Unit/TholdExpandStringTest.php b/tests/Unit/TholdExpandStringTest.php new file mode 100644 index 00000000..151aa784 --- /dev/null +++ b/tests/Unit/TholdExpandStringTest.php @@ -0,0 +1,140 @@ + escaping rather than after. + */ +final class TholdExpandStringTest extends TestCase { + /** + * @return void + */ + public static function setUpBeforeClass(): void { + self::loadPluginSource('thold_functions.php'); + } + + /** + * @return array + */ + private function thresholdData(array $overrides = array()) { + return $overrides + array( + 'local_graph_id' => 7, + 'local_data_id' => 4, + 'data_source_name' => 'traffic_in', + 'thold_template_id' => 0, + ); + } + + /** + * @return void + */ + private function graphExists() { + CactiStub::willReturn('db_fetch_row_prepared', array( + 'id' => 7, + 'host_id' => 2, + 'snmp_query_id' => 3, + 'snmp_index' => '1', + )); + } + + /** + * @return void + */ + public function testGraphTitleTokenIsResolved(): void { + $this->graphExists(); + + $this->assertSame('Traffic - eth0', thold_expand_string($this->thresholdData(), '|graph_title|')); + } + + /** + * @return void + */ + public function testDataSourceNameTokenIsResolved(): void { + $this->graphExists(); + + $this->assertSame('traffic_in', thold_expand_string($this->thresholdData(), '|data_source_name|')); + } + + /** + * @return void + */ + public function testDataSourceDescriptionTokenIsResolvedFromTheDatabase(): void { + $this->graphExists(); + CactiStub::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + + $this->assertSame('Router - Traffic', thold_expand_string($this->thresholdData(), '|data_source_description|')); + } + + /** + * @return void + */ + public function testTextIsPassedThroughExpandTitleForDataQueryTokens(): void { + $this->graphExists(); + CactiStub::willReturn('expand_title', 'alert eth0'); + + $this->assertSame('alert eth0', thold_expand_string($this->thresholdData(), 'alert |query_ifName|')); + $this->assertNotEmpty(CactiStub::callsTo('expand_title')); + } + + /** + * @return void + */ + public function testInterfaceSpeedFallsBackToTheConfiguredDefaultWhenUnknown(): void { + $this->graphExists(); + CactiStub::$configOptions['thold_empty_if_speed_default'] = '1000000000'; + CactiStub::willReturn('db_fetch_cell_prepared', ''); + + $result = thold_expand_string($this->thresholdData(), '|query_ifHighSpeed|'); + + $this->assertStringNotContainsString('|query_ifHighSpeed|', $result); + } + + /** + * @return void + */ + public function testTextIsReturnedUnchangedWhenTheGraphIsMissing(): void { + CactiStub::willReturn('db_fetch_row_prepared', array()); + + $this->assertSame('static text', thold_expand_string($this->thresholdData(), 'static text')); + } + + /** + * An empty template falls back to the threshold template's suggested name, + * which is itself a token string and gets expanded in turn. + * + * @return void + */ + public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): void { + $this->graphExists(); + CactiStub::willReturn('db_fetch_cell_prepared', 'Suggested |data_source_name|'); + + $result = thold_expand_string($this->thresholdData(array('thold_template_id' => 5)), ''); + + $this->assertSame('Suggested traffic_in', $result); + } + + /** + * @return void + */ + public function testSurroundingWhitespaceIsTrimmed(): void { + CactiStub::willReturn('db_fetch_row_prepared', array()); + + $this->assertSame('alert', thold_expand_string($this->thresholdData(), ' alert ')); + } +} diff --git a/tests/Unit/TholdExpressionMathRpnTest.php b/tests/Unit/TholdExpressionMathRpnTest.php new file mode 100644 index 00000000..797762e0 --- /dev/null +++ b/tests/Unit/TholdExpressionMathRpnTest.php @@ -0,0 +1,282 @@ + $stack Initial stack, bottom element first. + * @param string $operator RPN operator token. + * + * @return array + */ + private function evaluate(array $stack, $operator) { + thold_expression_math_rpn($operator, $stack); + + return $stack; + } + + /** + * @return array, 1: string, 2: float|int}> + */ + public static function binaryOperatorProvider() { + return array( + 'addition' => array(array(8, 2), '+', 10), + 'subtraction keeps order' => array(array(8, 2), '-', 6), + 'multiplication' => array(array(8, 2), '*', 16), + 'division keeps order' => array(array(8, 2), '/', 4), + 'modulo' => array(array(8, 3), '%', 2), + 'float addition' => array(array(1.5, 2.25), '+', 3.75), + 'numeric string operands' => array(array('8', '2'), '-', 6), + 'negative operands' => array(array(-8, 2), '/', -4), + ); + } + + /** + * @dataProvider binaryOperatorProvider + * + * @param array $stack + * @param string $operator + * @param float|int $expected + * + * @return void + */ + public function testBinaryOperatorsComputeInStackOrder(array $stack, $operator, $expected): void { + $this->assertSame(array($expected), $this->evaluate($stack, $operator)); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * PHP's ^ is bitwise XOR, not exponentiation. The pre-hardening evaluator + * built the string "$v2 ^ $v1" and eval'd it, so it computed XOR too. This + * test records the behaviour as-is; changing it to pow() would be a + * separate, breaking change to existing user thresholds. + * + * @return void + */ + public function testCaretOperatorIsIntegerXorNotExponentiation(): void { + $this->assertSame(array(6), $this->evaluate(array(5, 3), '^')); + $this->assertSame(array(1), $this->evaluate(array(2, 3), '^')); + } + + /** + * @return void + */ + public function testCaretOperatorTruncatesFloatOperandsToIntegers(): void { + $this->assertSame(array(6), $this->evaluate(array(5.9, 3.9), '^')); + } + + /** + * @return void + */ + public function testModuloTruncatesFloatOperandsToIntegers(): void { + $this->assertSame(array(1), $this->evaluate(array(7.9, 3.2), '%')); + } + + /** + * Dividing zero by zero is defined as zero rather than an error so a + * counter that has not moved does not flag the whole threshold. + * + * @return void + */ + public function testZeroDividedByZeroYieldsZeroWithoutError(): void { + $this->assertSame(array(0), $this->evaluate(array(0, 0), '/')); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testDivisionByZeroFlagsErrorAndPushesNothing(): void { + $this->assertSame(array(), $this->evaluate(array(8, 0), '/')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testModuloByZeroFlagsErrorInsteadOfThrowing(): void { + $this->assertSame(array(), $this->evaluate(array(8, 0), '%')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return array, 1: string}> + */ + public static function nonNumericOperandProvider() { + return array( + 'unknown right operand' => array(array(8, 'U'), '+'), + 'unknown left operand' => array(array('U', 8), '+'), + 'NaN right operand' => array(array(8, 'NAN'), '*'), + 'text operand' => array(array(8, 'abc'), '-'), + ); + } + + /** + * @dataProvider nonNumericOperandProvider + * + * @param array $stack + * @param string $operator + * + * @return void + */ + public function testNonNumericOperandsFlagErrorAndPushNothing(array $stack, $operator): void { + $this->assertSame(array(), $this->evaluate($stack, $operator)); + $this->assertTrue($GLOBALS['rpn_error']); + $this->assertNotEmpty(CactiStub::$log); + } + + /** + * @return array + */ + public static function unaryFunctionProvider() { + return array( + 'SIN' => array(0, 'SIN', 0.0), + 'COS' => array(0, 'COS', 1.0), + 'TAN' => array(0, 'TAN', 0.0), + 'ATAN' => array(0, 'ATAN', 0.0), + 'SQRT' => array(9, 'SQRT', 3.0), + 'FLOOR' => array(2.7, 'FLOOR', 2.0), + 'CEIL' => array(2.1, 'CEIL', 3.0), + 'DEG2RAD' => array(180, 'DEG2RAD', M_PI), + 'RAD2DEG' => array(M_PI, 'RAD2DEG', 180.0), + 'ABS' => array(-5, 'ABS', 5), + 'EXP' => array(0, 'EXP', 1.0), + 'LOG' => array(M_E, 'LOG', 1.0), + ); + } + + /** + * @dataProvider unaryFunctionProvider + * + * @param float|int $operand + * @param string $operator + * @param float|int $expected + * + * @return void + */ + public function testUnaryFunctionsDispatchToNativeMath($operand, $operator, $expected): void { + $stack = $this->evaluate(array($operand), $operator); + + $this->assertCount(1, $stack); + $this->assertEqualsWithDelta($expected, $stack[0], 1.0e-9); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testUnaryFunctionRejectsNonNumericOperand(): void { + $this->assertSame(array(), $this->evaluate(array('U'), 'SQRT')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * sqrt(-1) and log(0) have no usable numeric result, so the evaluator must + * flag an error rather than push NAN or -INF into a threshold comparison, + * where every comparison against them silently returns false. + * + * @return array + */ + public static function undefinedResultProvider() { + return array( + 'square root of a negative' => array(-1, 'SQRT'), + 'log of zero' => array(0, 'LOG'), + 'log of a negative' => array(-1, 'LOG'), + ); + } + + /** + * @dataProvider undefinedResultProvider + * + * @param float|int $operand + * @param string $operator + * + * @return void + */ + public function testUndefinedResultsFlagErrorInsteadOfPushingNanOrInf($operand, $operator): void { + $this->assertSame(array(), $this->evaluate(array($operand), $operator)); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testAtan2ComputesAgainstBothOperands(): void { + $stack = $this->evaluate(array(1, 1), 'ATAN2'); + + $this->assertEqualsWithDelta(M_PI / 4, $stack[0], 1.0e-9); + } + + /** + * ADDNAN exists so a missing sample contributes zero instead of poisoning + * the sum. + * + * @return array, 1: float|int}> + */ + public static function addNanProvider() { + return array( + 'both known' => array(array(3, 4), 7), + 'right unknown' => array(array(3, 'U'), 3), + 'left unknown' => array(array('U', 4), 4), + 'right NaN' => array(array(3, 'NAN'), 3), + 'both unknown' => array(array('U', 'NAN'), 0), + ); + } + + /** + * @dataProvider addNanProvider + * + * @param array $stack + * @param float|int $expected + * + * @return void + */ + public function testAddNanTreatsUnknownOperandsAsZero(array $stack, $expected): void { + $this->assertSame(array($expected), $this->evaluate($stack, 'ADDNAN')); + } + + /** + * @return void + */ + public function testUnknownOperatorLeavesStackUntouched(): void { + $this->assertSame(array(1, 2), $this->evaluate(array(1, 2), 'NOSUCHOP')); + } + + /** + * @return void + */ + public function testUnderflowFlagsErrorRatherThanPoppingAnEmptyStack(): void { + $this->evaluate(array(), '+'); + + $this->assertTrue($GLOBALS['rpn_error']); + } +} diff --git a/tests/Unit/TholdReplaceThresholdTagsTest.php b/tests/Unit/TholdReplaceThresholdTagsTest.php new file mode 100644 index 00000000..611352d6 --- /dev/null +++ b/tests/Unit/TholdReplaceThresholdTagsTest.php @@ -0,0 +1,253 @@ + + */ + private function threshold(array $overrides = array()) { + return $overrides + array( + 'id' => 1, + 'name_cache' => 'CPU', + 'notes' => '', + 'dnotes' => '', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'time_hi' => 80, + 'time_low' => 20, + 'time_fail_trigger' => 2, + 'time_fail_length' => 300, + 'local_data_id' => 4, + ); + } + + /** + * @return array + */ + private function device(array $overrides = array()) { + return $overrides + array( + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + ); + } + + /** + * Substitute tags into $text. + * + * @param string $text + * @param array $thold + * @param array $device + * @param bool $shell + * @param mixed $currentval + * + * @return string + */ + private function substitute($text, array $thold, array $device, $shell, $currentval = 42) { + return thold_replace_threshold_tags($text, $thold, $device, $currentval, 7, 'traffic_in', $shell); + } + + /** + * @return array + */ + public static function deviceDerivedTagProvider() { + return array( + 'description' => array('', 'description', 'device'), + 'hostname' => array('', 'hostname', 'device'), + 'location' => array('', 'location', 'device'), + 'notes' => array('', 'notes', 'threshold'), + 'device note' => array('', 'dnotes', 'threshold'), + 'external id' => array('', 'external_id', 'threshold'), + 'name' => array('', 'name_cache', 'threshold'), + ); + } + + /** + * @dataProvider deviceDerivedTagProvider + * + * @param string $tag + * @param string $column + * @param string $source + * + * @return void + */ + public function testShellModeQuotesEveryDeviceDerivedTag($tag, $column, $source): void { + $payload = '; touch /tmp/pwned'; + $thold = $this->threshold($source === 'threshold' ? array($column => $payload) : array()); + $device = $this->device($source === 'device' ? array($column => $payload) : array()); + + $result = $this->substitute("/usr/bin/alert $tag", $thold, $device, true); + + $this->assertStringContainsString(escapeshellarg($payload), $result); + $this->assertStringNotContainsString("alert ; touch", $result); + } + + /** + * @dataProvider deviceDerivedTagProvider + * + * @param string $tag + * @param string $column + * @param string $source + * + * @return void + */ + public function testEmailModeLeavesDeviceDerivedTagsUnquoted($tag, $column, $source): void { + $thold = $this->threshold($source === 'threshold' ? array($column => "O'Brien") : array()); + $device = $this->device($source === 'device' ? array($column => "O'Brien") : array()); + + $result = $this->substitute("Alert on $tag", $thold, $device, false); + + $this->assertStringContainsString("O'Brien", $result); + $this->assertStringNotContainsString("'O'\\''Brien'", $result); + } + + /** + * The site name comes from the sites table, which an operator edits, so it + * needs the same treatment as the device columns. + * + * @return void + */ + public function testShellModeQuotesTheSiteName(): void { + CactiStub::willReturn('db_fetch_cell_prepared', '$(id)'); + + $result = $this->substitute('/usr/bin/alert ', $this->threshold(), $this->device(), true); + + $this->assertStringContainsString(escapeshellarg('$(id)'), $result); + } + + /** + * @return void + */ + public function testSiteFallsBackToDefaultWhenTheDeviceHasNoSite(): void { + CactiStub::willReturn('db_fetch_cell_prepared', ''); + + $result = $this->substitute('site=', $this->threshold(), $this->device(), false); + + $this->assertSame('site=Default', $result); + } + + /** + * The current reading is an RRD value rather than a number in every case; + * it must not be able to extend the command line either. + * + * @return void + */ + public function testShellModeQuotesTheCurrentValue(): void { + $result = $this->substitute('/usr/bin/alert ', $this->threshold(), $this->device(), true, '; id'); + + $this->assertStringContainsString(escapeshellarg('; id'), $result); + $this->assertStringNotContainsString('alert ; id', $result); + } + + /** + * @return void + */ + public function testEmailModeLeavesTheCurrentValueUnquoted(): void { + $result = $this->substitute('value=', $this->threshold(), $this->device(), false, 42); + + $this->assertSame('value=42', $result); + } + + /** + * @return void + */ + public function testGraphAndThresholdIdentifiersAreSubstituted(): void { + $result = $this->substitute('/', $this->threshold(array('id' => 5)), $this->device(), false); + + $this->assertSame('7/5', $result); + } + + /** + * @return void + */ + public function testStaticThresholdBoundsAreSubstituted(): void { + $result = $this->substitute('//', $this->threshold(), $this->device(), false); + + $this->assertSame('90/10/3', $result); + } + + /** + * A time-based threshold reports its own bounds and a duration rather than + * the static ones. + * + * @return void + */ + public function testTimeBasedThresholdSubstitutesTheTimeBounds(): void { + $result = $this->substitute('[][][]', $this->threshold(array('thold_type' => 2)), $this->device(), false); + + $this->assertSame('[80][20][2]', $result); + } + + /** + * A baseline threshold has neither static nor time bounds, so the tags + * resolve to empty rather than being left in the output. + * + * @return void + */ + public function testBaselineThresholdClearsTheBoundTags(): void { + $result = $this->substitute('[][][][]', $this->threshold(array('thold_type' => 1)), $this->device(), false); + + $this->assertSame('[][][][]', $result); + } + + /** + * @return void + */ + public function testStaticThresholdHasNoDuration(): void { + $result = $this->substitute('[]', $this->threshold(), $this->device(), false); + + $this->assertSame('[]', $result); + } + + /** + * @return void + */ + public function testUrlTagRendersALinkToTheGraph(): void { + CactiStub::$configOptions['base_url'] = 'http://cacti.example.org'; + + $result = $this->substitute('', $this->threshold(), $this->device(), false); + + $this->assertStringContainsString('graph.php?local_graph_id=7', $result); + } + + /** + * @return void + */ + public function testTextWithoutTagsIsReturnedUnchanged(): void { + $result = $this->substitute('nothing to replace', $this->threshold(), $this->device(), true); + + $this->assertSame('nothing to replace', $result); + } +} diff --git a/tests/Unit/TholdRlikeClauseTest.php b/tests/Unit/TholdRlikeClauseTest.php new file mode 100644 index 00000000..c32a6421 --- /dev/null +++ b/tests/Unit/TholdRlikeClauseTest.php @@ -0,0 +1,70 @@ +assertSame("RLIKE 'router'", thold_rlike_clause('router')); + } + + /** + * A quote in the filter has to be escaped, or it closes the string literal + * and the rest of the filter becomes SQL. + * + * @return void + */ + public function testQuotesInTheFilterAreEscaped(): void { + $clause = thold_rlike_clause("' OR 1=1 -- "); + + $this->assertSame("RLIKE ''' OR 1=1 -- '", $clause); + } + + /** + * @return void + */ + public function testEmptyFilterStillProducesAValidClause(): void { + $this->assertSame("RLIKE ''", thold_rlike_clause('')); + } + + /** + * @return void + */ + public function testCoreHelperIsPreferredWhenAvailable(): void { + if (!function_exists('db_qstr_rlike')) { + $this->assertSame("RLIKE 'x'", thold_rlike_clause('x')); + + return; + } + + $this->assertSame(db_qstr_rlike('x'), thold_rlike_clause('x')); + } +} diff --git a/tests/Unit/TholdSetEnvironTest.php b/tests/Unit/TholdSetEnvironTest.php new file mode 100644 index 00000000..d6e0a502 --- /dev/null +++ b/tests/Unit/TholdSetEnvironTest.php @@ -0,0 +1,215 @@ + + */ + private function threshold(array $overrides = array()) { + return $overrides + array( + 'id' => 3, + 'local_data_id' => 4, + 'local_graph_id' => 7, + 'name_cache' => 'CPU', + 'notes' => '', + 'dnotes' => 'device note', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'time_hi' => 80, + 'time_low' => 20, + 'time_fail_trigger' => 2, + 'time_fail_length' => 300, + ); + } + + /** + * @return array + */ + private function device(array $overrides = array()) { + return $overrides + array( + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + ); + } + + /** + * Collect the exported environment as a name => value map. + * + * @param array $thold + * @param array $device + * + * @return array + */ + private function environment(array $thold, array $device) { + $pairs = thold_set_environ('', $thold, $device, 42, 7, 'traffic_in'); + $map = array(); + + foreach ($pairs as $pair) { + list($name, $value) = explode('=', $pair, 2); + + $map[$name] = $value; + } + + return $map; + } + + /** + * @return void + */ + public function testThresholdAndDeviceContextIsExported(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('3', $env['THOLD_ID']); + $this->assertSame('router1', $env['THOLD_DESCRIPTION']); + $this->assertSame('10.0.0.1', $env['THOLD_HOSTNAME']); + $this->assertSame('42', $env['THOLD_CURRENTVALUE']); + $this->assertSame('traffic_in', $env['THOLD_DSNAME']); + $this->assertSame('device note', $env['THOLD_DEVICENOTE']); + } + + /** + * The queue accumulates across calls, so the first pair has to reset it or + * a later command inherits the previous threshold's context. + * + * @return void + */ + public function testEachCallStartsFromAnEmptyEnvironment(): void { + $this->environment($this->threshold(), $this->device()); + $env = $this->environment($this->threshold(array('id' => 9)), $this->device()); + + $this->assertSame('9', $env['THOLD_ID']); + $this->assertCount(1, array_keys(array_filter(array_keys($env), function ($name) { + return $name === 'THOLD_ID'; + }))); + } + + /** + * @return void + */ + public function testStaticThresholdExportsItsBoundsAndNoDuration(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('90', $env['THOLD_HI']); + $this->assertSame('10', $env['THOLD_LOW']); + $this->assertSame('3', $env['THOLD_TRIGGER']); + $this->assertSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testTimeBasedThresholdExportsTheTimeBoundsAndADuration(): void { + $env = $this->environment($this->threshold(array('thold_type' => 2)), $this->device()); + + $this->assertSame('80', $env['THOLD_HI']); + $this->assertSame('20', $env['THOLD_LOW']); + $this->assertSame('2', $env['THOLD_TRIGGER']); + $this->assertNotSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testBaselineThresholdExportsEmptyBounds(): void { + $env = $this->environment($this->threshold(array('thold_type' => 1)), $this->device()); + + $this->assertSame('', $env['THOLD_HI']); + $this->assertSame('', $env['THOLD_LOW']); + $this->assertSame('', $env['THOLD_TRIGGER']); + $this->assertSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testNotesAreTagExpandedWhenPresent(): void { + $env = $this->environment($this->threshold(array('notes' => 'see ')), $this->device()); + + $this->assertSame('see 10.0.0.1', $env['THOLD_NOTES']); + } + + /** + * @return void + */ + public function testNotesAreEmptyWhenTheThresholdHasNone(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('', $env['THOLD_NOTES']); + } + + /** + * @return void + */ + public function testExternalIdIsExportedOnlyWhenSet(): void { + $env = $this->environment($this->threshold(), $this->device()); + $this->assertArrayNotHasKey('THOLD_EXTERNAL_ID', $env); + + $env = $this->environment($this->threshold(array('external_id' => 'INC-42')), $this->device()); + $this->assertSame('INC-42', $env['THOLD_EXTERNAL_ID']); + } + + /** + * @return void + */ + public function testUnknownThresholdTypeExportsAnEmptyTypeName(): void { + $env = $this->environment($this->threshold(array('thold_type' => 99)), $this->device()); + + $this->assertSame('', $env['THOLD_THOLDTYPE']); + } + + /** + * @return void + */ + public function testGraphUrlIsExported(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('http://cacti.example.org/graph.php?local_graph_id=7', $env['THOLD_URL']); + } +} diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 29e4279a..a6167f75 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -2,125 +2,167 @@ /* +-------------------------------------------------------------------------+ | 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 | +-------------------------------------------------------------------------+ + | http://www.cacti.net/ | + +-------------------------------------------------------------------------+ */ /* - * Test bootstrap: stub Cacti framework functions so plugin code - * can be loaded in isolation without the full Cacti application. + * Test bootstrap. + * + * thold's sources expect to be included by Cacti, which has already defined + * the db_*, request-variable, and logging helpers as plain global functions. + * Nothing here talks to a database or a network: each Cacti function is + * declared as a shim over CactiStub, which records the call and hands back + * whatever the test programmed. + * + * Guarding every declaration with function_exists() keeps this file usable if + * a future integration suite loads real Cacti first. */ -$GLOBALS['__test_db_calls'] = []; -$GLOBALS['config'] = [ - 'base_path' => '/var/www/html/cacti', +require_once dirname(__DIR__) . '/vendor/autoload.php'; + +/* + * base_path has to point at the Cacti root two levels above this plugin: + * thold_functions.php builds include paths from it at runtime. + */ +$GLOBALS['config'] = array( + 'base_path' => dirname(dirname(dirname(__DIR__))), 'url_path' => '/cacti/', - 'cacti_version' => '1.2.999', -]; + 'cacti_version' => '1.2.31', + 'cacti_server_os' => 'unix', +); + +/* thold_expand_string() include_once()s library_path/variables.php at call time. */ +$GLOBALS['config']['library_path'] = __DIR__ . '/fixtures/lib'; + +/* thold reads and writes this on every RPN evaluation. */ +$GLOBALS['rpn_error'] = false; if (!function_exists('db_execute')) { - function db_execute($sql) { - $GLOBALS['__test_db_calls'][] = ['fn' => 'db_execute', 'sql' => $sql, 'params' => []]; + function db_execute($sql, $log = true, $db_conn = false) { + CactiStub::record('db_execute', $sql); - return true; + return CactiStub::nextReturn('db_execute', true); } } if (!function_exists('db_execute_prepared')) { - function db_execute_prepared($sql, $params = []) { - $GLOBALS['__test_db_calls'][] = ['fn' => 'db_execute_prepared', 'sql' => $sql, 'params' => $params]; + function db_execute_prepared($sql, $params = array(), $log = true, $db_conn = false) { + CactiStub::record('db_execute_prepared', $sql, $params); - return true; + return CactiStub::nextReturn('db_execute_prepared', true); } } if (!function_exists('db_fetch_assoc')) { - function db_fetch_assoc($sql) { - return []; + function db_fetch_assoc($sql, $log = true, $db_conn = false) { + CactiStub::record('db_fetch_assoc', $sql); + + return CactiStub::nextReturn('db_fetch_assoc', array()); } } if (!function_exists('db_fetch_assoc_prepared')) { - function db_fetch_assoc_prepared($sql, $params = []) { - return []; + function db_fetch_assoc_prepared($sql, $params = array(), $log = true, $db_conn = false) { + CactiStub::record('db_fetch_assoc_prepared', $sql, $params); + + return CactiStub::nextReturn('db_fetch_assoc_prepared', array()); } } if (!function_exists('db_fetch_row')) { - function db_fetch_row($sql) { - return []; + function db_fetch_row($sql, $log = true, $db_conn = false) { + CactiStub::record('db_fetch_row', $sql); + + return CactiStub::nextReturn('db_fetch_row', array()); } } if (!function_exists('db_fetch_row_prepared')) { - function db_fetch_row_prepared($sql, $params = []) { - return []; + function db_fetch_row_prepared($sql, $params = array(), $log = true, $db_conn = false) { + CactiStub::record('db_fetch_row_prepared', $sql, $params); + + return CactiStub::nextReturn('db_fetch_row_prepared', array()); } } if (!function_exists('db_fetch_cell')) { - function db_fetch_cell($sql) { - return ''; + function db_fetch_cell($sql, $col_name = '', $log = true, $db_conn = false) { + CactiStub::record('db_fetch_cell', $sql); + + return CactiStub::nextReturn('db_fetch_cell', ''); } } if (!function_exists('db_fetch_cell_prepared')) { - function db_fetch_cell_prepared($sql, $params = []) { - return ''; + function db_fetch_cell_prepared($sql, $params = array(), $col_name = '', $log = true, $db_conn = false) { + CactiStub::record('db_fetch_cell_prepared', $sql, $params); + + return CactiStub::nextReturn('db_fetch_cell_prepared', ''); } } if (!function_exists('db_qstr')) { function db_qstr($string) { - return "'" . str_replace("'", "''", $string) . "'"; + return "'" . str_replace("'", "''", (string) $string) . "'"; } } if (!function_exists('db_begin_transaction')) { function db_begin_transaction() { - $GLOBALS['__test_db_calls'][] = ['fn' => 'db_begin_transaction', 'sql' => '', 'params' => []]; + CactiStub::record('db_begin_transaction'); - return true; + return CactiStub::nextReturn('db_begin_transaction', true); } } if (!function_exists('db_commit_transaction')) { function db_commit_transaction() { - $GLOBALS['__test_db_calls'][] = ['fn' => 'db_commit_transaction', 'sql' => '', 'params' => []]; + CactiStub::record('db_commit_transaction'); - return true; + return CactiStub::nextReturn('db_commit_transaction', true); } } if (!function_exists('db_rollback_transaction')) { function db_rollback_transaction() { - $GLOBALS['__test_db_calls'][] = ['fn' => 'db_rollback_transaction', 'sql' => '', 'params' => []]; + CactiStub::record('db_rollback_transaction'); - return true; + return CactiStub::nextReturn('db_rollback_transaction', true); } } if (!function_exists('html_escape')) { function html_escape($string) { - return htmlspecialchars($string, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + return htmlspecialchars((string) $string, ENT_QUOTES, 'UTF-8'); } } -// KEEP IN SYNC with Cacti core lib/functions.php sanitize_unserialize_selected_items() +/* + * Mirrors Cacti 1.2 lib/functions.php. KEEP IN SYNC: if core tightens its + * checks, tests here would otherwise keep passing while production diverges. + */ if (!function_exists('sanitize_unserialize_selected_items')) { function sanitize_unserialize_selected_items($items) { if (empty($items)) { return false; } - $data = unserialize($items, ['allowed_classes' => false]); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirrors sanitize_unserialize_selected_items; allowed_classes:false blocks object injection + $data = unserialize($items, array('allowed_classes' => false)); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirroring Cacti core; allowed_classes:false blocks object injection if (!is_array($data)) { return false; } - foreach ($data as $key => $value) { + foreach ($data as $value) { if (!is_numeric($value)) { return false; } @@ -132,56 +174,152 @@ function sanitize_unserialize_selected_items($items) { if (!function_exists('read_config_option')) { function read_config_option($name, $force = false) { - return ''; + return isset(CactiStub::$configOptions[$name]) ? CactiStub::$configOptions[$name] : ''; } } if (!function_exists('set_config_option')) { function set_config_option($name, $value) { + CactiStub::$configOptions[$name] = $value; } } if (!function_exists('__')) { - function __($text, $domain = '') { - return $text; + function __($text) { + $args = array_slice(func_get_args(), 1); + + /* Cacti's __() accepts sprintf arguments after the format string. */ + return $args === array() ? $text : vsprintf($text, $args); } } if (!function_exists('__esc')) { - function __esc($text, $domain = '') { - return htmlspecialchars($text, ENT_QUOTES | ENT_HTML5, 'UTF-8'); + function __esc($text) { + return htmlspecialchars(call_user_func_array('__', func_get_args()), ENT_QUOTES, 'UTF-8'); } } if (!function_exists('cacti_log')) { - function cacti_log($message, $also_print = false, $log_type = '', $level = 0) { + function cacti_log($message, $output = false, $environ = 'CMDPHP', $level = 0) { + CactiStub::$log[] = $message; } } if (!function_exists('cacti_sizeof')) { function cacti_sizeof($array) { - return is_array($array) ? count($array) : 0; + return (is_array($array) || $array instanceof Countable) ? count($array) : 0; + } +} + +if (!function_exists('cacti_count')) { + function cacti_count($array) { + return cacti_sizeof($array); } } if (!function_exists('get_request_var')) { - function get_request_var($name) { - return ''; + function get_request_var($name, $default = '') { + return isset(CactiStub::$requestVars[$name]) ? CactiStub::$requestVars[$name] : $default; } } if (!function_exists('get_nfilter_request_var')) { - function get_nfilter_request_var($name) { - return ''; + function get_nfilter_request_var($name, $default = '') { + return get_request_var($name, $default); } } if (!function_exists('get_filter_request_var')) { - function get_filter_request_var($name) { - return ''; + function get_filter_request_var($name, $filter = FILTER_VALIDATE_INT, $options = array()) { + return get_request_var($name); + } +} + +if (!function_exists('isset_request_var')) { + function isset_request_var($name) { + return isset(CactiStub::$requestVars[$name]); + } +} + +if (!function_exists('cacti_escapeshellarg')) { + function cacti_escapeshellarg($string, $quote = true) { + return escapeshellarg((string) $string); + } +} + +if (!function_exists('api_plugin_hook_function')) { + function api_plugin_hook_function($name, $data = '') { + return $data; + } +} + +if (!function_exists('get_simple_graph_perms')) { + function get_simple_graph_perms($user_id) { + return CactiStub::nextReturn('get_simple_graph_perms', true); } } +if (!function_exists('get_policies')) { + function get_policies($user_id) { + return CactiStub::nextReturn('get_policies', array()); + } +} + +if (!function_exists('get_policy_where')) { + function get_policy_where($graph_auth_method, $policies, $sql_where) { + CactiStub::record('get_policy_where', $sql_where); + + return CactiStub::nextReturn('get_policy_where', $sql_where); + } +} + +if (!function_exists('expand_title')) { + function expand_title($host_id, $snmp_query_id, $snmp_index, $title) { + CactiStub::record('expand_title', $title); + + return CactiStub::nextReturn('expand_title', $title); + } +} + +if (!function_exists('get_graph_title')) { + function get_graph_title($local_graph_id) { + return CactiStub::nextReturn('get_graph_title', 'Traffic - eth0'); + } +} + +if (!function_exists('get_timeinstate')) { + function get_timeinstate($host) { + return CactiStub::nextReturn('get_timeinstate', '1 day'); + } +} + +if (!function_exists('get_daysfromtime')) { + function get_daysfromtime($timestamp) { + return CactiStub::nextReturn('get_daysfromtime', '1 day'); + } +} + +if (!function_exists('number_format_i18n')) { + function number_format_i18n($number, $decimals = 0, $baseu = 1000) { + return number_format((float) $number, $decimals < 0 ? 0 : (int) $decimals); + } +} + +if (!defined('FILTER_VALIDATE_IS_REGEX')) { + define('FILTER_VALIDATE_IS_REGEX', 99999); +} + +/* Device states, from Cacti include/global_constants.php. */ +foreach (array('HOST_UNKNOWN' => 0, 'HOST_DOWN' => 1, 'HOST_RECOVERING' => 2, 'HOST_UP' => 3, 'HOST_ERROR' => 4) as $name => $value) { + if (!defined($name)) { + define($name, $value); + } +} + +if (!defined('CACTI_DATE_TIME_FORMAT')) { + define('CACTI_DATE_TIME_FORMAT', 'Y-m-d H:i:s'); +} + if (!defined('CACTI_PATH_BASE')) { - define('CACTI_PATH_BASE', '/var/www/html/cacti'); + define('CACTI_PATH_BASE', $GLOBALS['config']['base_path']); } diff --git a/tests/docker/Dockerfile b/tests/docker/Dockerfile new file mode 100644 index 00000000..cb4279d1 --- /dev/null +++ b/tests/docker/Dockerfile @@ -0,0 +1,28 @@ +# Test runner for the Thold plugin. +# +# Pinned to PHP 8.1 because that is the oldest interpreter the CI matrix +# covers; what passes here passes on 8.2-8.4. pcov rather than Xdebug: line +# coverage is the only debug feature the suite needs and pcov is far cheaper. +FROM php:8.1-cli-alpine@sha256:7949370448b0b4d9787776dc5968e0fd8d48763292344b5fbf21539441228a98 + +# git is needed by the changed-line coverage gate, which diffs against the +# base branch. +RUN apk add --no-cache git \ + && apk add --no-cache --virtual .build-deps $PHPIZE_DEPS \ + && pecl install pcov \ + && docker-php-ext-enable pcov \ + && apk del .build-deps + +COPY --from=composer:2@sha256:4d71c3c2109c61d5415544264b59ad4087e4c5b7244481723664138fd36d5040 /usr/bin/composer /usr/bin/composer + +# The plugin lives where Cacti would put it, because thold_functions.php +# resolves its own includes through $config['base_path'] . '/plugins/thold'. +# No network or database is involved; the Cacti framework functions themselves +# are stubbed in tests/bootstrap.php. +WORKDIR /cacti/plugins/thold + +ENV COMPOSER_ALLOW_SUPERUSER=1 \ + COMPOSER_NO_INTERACTION=1 \ + COMPOSER_CACHE_DIR=/tmp/composer-cache + +CMD ["sh", "-c", "composer install --no-progress --no-ansi && vendor/bin/phpunit"] diff --git a/tests/docker/docker-compose.yml b/tests/docker/docker-compose.yml new file mode 100644 index 00000000..99d38b47 --- /dev/null +++ b/tests/docker/docker-compose.yml @@ -0,0 +1,15 @@ +# Local mirror of the unit-test CI job. `docker compose -f +# tests/docker/docker-compose.yml run --rm phpunit` runs exactly what CI runs. +services: + phpunit: + build: + context: . + dockerfile: Dockerfile + image: cacti-thold-test:php8.1 + working_dir: /cacti/plugins/thold + volumes: + - ../..:/cacti/plugins/thold + - composer-cache:/tmp/composer-cache + +volumes: + composer-cache: From e8da9777548e34c447b1b038479278744599fad5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:47:56 -0700 Subject: [PATCH 28/41] fix(thold): abandon RPN expressions that have no usable result Modulo by zero raised an uncaught DivisionByZeroError, and SQRT of a negative or LOG of zero pushed NAN or -INF, which compares false against every bound so the breach went unnoticed. Zero divided by zero broke out of the operator switch before pushing its result, leaving the stack short by two. Signed-off-by: Thomas Vincent --- thold_functions.php | 168 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 145 insertions(+), 23 deletions(-) diff --git a/thold_functions.php b/thold_functions.php index f88470d1..e3b612ee 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -338,6 +338,24 @@ function thold_expression_rpn_pop(&$stack) { } } +/** + * Apply one arithmetic RPN operator to the evaluation stack. + * + * The right operand is popped first, so a stack of [a, b] with operator '-' + * computes a - b. Operands are validated numeric before use and the operator + * set is closed, which is what lets this dispatch natively instead of through + * eval(). Note that '^' is bitwise XOR, not exponentiation; that is the + * semantics existing user thresholds were written against. + * + * On any error the global $rpn_error is raised and nothing is pushed, which + * causes the caller to abandon the expression rather than compare against a + * meaningless value. + * + * @param string $operator Operator token, e.g. '+', '%', 'SQRT', 'ADDNAN'. + * @param array $stack Evaluation stack, modified in place. + * + * @return void + */ function thold_expression_math_rpn($operator, &$stack) { global $rpn_error; @@ -363,12 +381,11 @@ function thold_expression_math_rpn($operator, &$stack) { cacti_log('ERROR: RPN value: v2 "' . $v2 . '" is Not valid for operator "' . $operator . '". Stack:"' . implode(',', $orig_stack) . '"', false, 'THOLD'); $rpn_error = true; } elseif ($v1 == 0 && $v2 == 0 && $operator == '/') { + /* A counter that has not moved divides to zero rather than erroring. */ $v3 = 0; $rpn_evaled = true; - - break; - } elseif ($v1 == 0 && $operator == '/') { - cacti_log('ERROR: RPN value: v1 can not be "0" when the operator is "/". Stack:"' . implode(',', $orig_stack) . '"', false, 'THOLD'); + } elseif ($v1 == 0 && ($operator == '/' || $operator == '%')) { + cacti_log('ERROR: RPN value: v1 can not be "0" when the operator is "' . $operator . '". Stack:"' . implode(',', $orig_stack) . '"', false, 'THOLD'); $rpn_error = true; } @@ -430,6 +447,16 @@ function thold_expression_math_rpn($operator, &$stack) { $rpn_error = true; } + if (!$rpn_error && ($operator == 'SQRT' && $v1 < 0 || $operator == 'LOG' && $v1 <= 0)) { + /* + * sqrt(-1) is NAN and log(0) is -INF. Both compare false against + * every threshold bound, so a breach would pass unnoticed; fail + * the expression instead of pushing them onto the stack. + */ + cacti_log('ERROR: RPN value: v1 "' . $v1 . '" is out of domain for operator "' . $operator . '".', false, 'THOLD'); + $rpn_error = true; + } + if (!$rpn_error) { // Validated numeric above; dispatch to the native math function // instead of eval() to remove the code-execution sink. @@ -1299,8 +1326,24 @@ function thold_calculate_lower_upper($thold, $currentval, $rrd_reindexed) { return $currentval; } -// $sql_where may contain ? placeholders; supply matching values via $sql_params. -// Callers passing a literal WHERE fragment without placeholders pass $sql_params = []. +/** + * Fetch the thresholds the given user may see. + * + * $sql_where is appended to a fixed WHERE clause and may carry ? placeholders; + * the caller supplies their values, in order, via $sql_params. The $graph_id + * filter appends its own placeholder after the caller's fragment, so any + * caller-supplied values must already be in $sql_params when the call is made. + * + * @param string $sql_where Extra WHERE conditions, without the leading AND. + * @param string $order_by ORDER BY expression, or '' for none. + * @param string $sql_limit LIMIT expression, or '' for none. + * @param int $total_rows Set by reference to the unlimited row count. + * @param int $user_id User whose permissions apply; 0 means the current user. + * @param int $graph_id Restrict to one graph, or 0 for all graphs. + * @param array $sql_params Values bound to the placeholders in $sql_where. + * + * @return array> + */ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0, $sql_params = []) { if ($sql_limit != '') { $sql_limit = "LIMIT $sql_limit"; @@ -1386,7 +1429,7 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim ) AS rower"; // get_total_row_data signature: ($user_id, $sql, $sql_params, $class, $timeout) - // The third param is accepted since Cacti 1.2.x (lib/auth.php:3120). + // The third param is accepted since Cacti 1.2.x (lib/auth.php:3164). if (function_exists('get_total_row_data') && $graph_id == 0) { $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold', 10); } else { @@ -1396,8 +1439,21 @@ function get_allowed_thresholds($sql_where = '', $order_by = 'td.name', $sql_lim return $tholds; } -// $sql_where may contain ? placeholders; supply matching values via $sql_params. -// Callers passing a literal WHERE fragment without placeholders pass $sql_params = []. +/** + * Fetch the threshold log entries the given user may see. + * + * Placeholder and $sql_params contract is the same as get_allowed_thresholds(). + * + * @param string $sql_where Extra WHERE conditions, without the leading AND. + * @param string $order_by ORDER BY expression, or '' for none. + * @param string $sql_limit LIMIT expression, or '' for none. + * @param int $total_rows Set by reference to the unlimited row count. + * @param int $user_id User whose permissions apply; 0 means the current user. + * @param int $graph_id Restrict to one graph, or 0 for all graphs. + * @param array $sql_params Values bound to the placeholders in $sql_where. + * + * @return array> + */ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql_limit = '', &$total_rows = 0, $user_id = 0, $graph_id = 0, $sql_params = []) { if ($sql_limit != '') { $sql_limit = "LIMIT $sql_limit"; @@ -1481,7 +1537,7 @@ function get_allowed_threshold_logs($sql_where = '', $order_by = 'td.name', $sql ) AS rower"; // get_total_row_data signature: ($user_id, $sql, $sql_params, $class, $timeout) - // The third param is accepted since Cacti 1.2.x (lib/auth.php:3120). + // The third param is accepted since Cacti 1.2.x (lib/auth.php:3164). if (function_exists('get_total_row_data') && $graph_id == 0) { $total_rows = get_total_row_data($user_id, $sql, $sql_params, 'thold_log', 10); } else { @@ -4068,9 +4124,14 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $queue = read_config_option('thold_notification_queue'); if ($breach_up && $thold_data['trigger_cmd_high'] != '') { - $cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_high'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); + /* + * Expansion runs first: it splices |query_*| and |host_*| values in + * verbatim, so running it after the tag escaping would let a device + * field expand inside the quotes the escaping just added. + */ + $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_high']); - $cmd = thold_expand_string($thold_data, $cmd); + $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); // thold_set_environ calls thold_putenv which calls putenv(); exec() inherits the process environment $environment = thold_set_environ($thold_data['trigger_cmd_high'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); @@ -4084,13 +4145,19 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; $cmd is built from thold_replace_threshold_tags + thold_expand_string with cacti_escapeshellarg protection + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not } $command_executed = true; } elseif ($breach_down && $thold_data['trigger_cmd_low'] != '') { - $cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_low'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); - $cmd = thold_expand_string($thold_data, $cmd); + /* + * Expansion runs first: it splices |query_*| and |host_*| values in + * verbatim, so running it after the tag escaping would let a device + * field expand inside the quotes the escaping just added. + */ + $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_low']); + + $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); // thold_set_environ calls thold_putenv which calls putenv(); exec() inherits the process environment $environment = thold_set_environ($thold_data['trigger_cmd_low'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); @@ -4104,13 +4171,19 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; $cmd is built from thold_replace_threshold_tags + thold_expand_string with cacti_escapeshellarg protection + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not } $command_executed = true; } elseif ($breach_norm && $thold_data['trigger_cmd_norm'] != '') { - $cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_norm'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); - $cmd = thold_expand_string($thold_data, $cmd); + /* + * Expansion runs first: it splices |query_*| and |host_*| values in + * verbatim, so running it after the tag escaping would let a device + * field expand inside the quotes the escaping just added. + */ + $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_norm']); + + $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); // thold_set_environ calls thold_putenv which calls putenv(); exec() inherits the process environment $environment = thold_set_environ($thold_data['trigger_cmd_norm'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name); @@ -4124,7 +4197,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; $cmd is built from thold_replace_threshold_tags + thold_expand_string with cacti_escapeshellarg protection + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not } $command_executed = true; @@ -4245,6 +4318,29 @@ function thold_set_environ($text, &$thold, &$h, $currentval, $local_graph_id, $d return $environment; } +/** + * Substitute the placeholders in a notification or trigger-command + * template. + * + * With $shell set, every substituted value is quoted with + * cacti_escapeshellarg() and the result is safe to hand to exec(); the + * tag also resolves to a bare URL rather than an anchor. Without it the + * result is raw text for an email or the web UI. + * + * Note that the thold_replacement_text plugin hook runs on the finished string, + * after escaping, so a hook that rewrites a shell template is responsible for + * its own quoting. + * + * @param string $text Template containing placeholders. + * @param array $thold Threshold row, by reference for the hook. + * @param array $h Device row, by reference for the hook. + * @param mixed $currentval Reading that triggered the notification. + * @param int $local_graph_id Graph the threshold belongs to. + * @param string $data_source_name Data source the threshold reads. + * @param bool $shell Quote substituted values for a shell command. + * + * @return string + */ function thold_replace_threshold_tags($text, &$thold, &$h, $currentval, $local_graph_id, $data_source_name, $shell = false) { global $thold_types; @@ -4285,12 +4381,12 @@ function thold_replace_threshold_tags($text, &$thold, &$h, $currentval, $local_g $text = thold_str_replace('', $local_graph_id, $text); $text = thold_str_replace('', $thold['id'], $text); - $text = thold_str_replace('', $currentval, $text); + $text = thold_str_replace('', $esc($currentval), $text); $text = thold_str_replace('', $esc($thold['name_cache']), $text); $text = thold_str_replace('', $esc($data_source_name), $text); if (isset($thold_types[$thold['thold_type']])) { - $text = thold_str_replace('', $thold_types[$thold['thold_type']], $text); + $text = thold_str_replace('', $esc($thold_types[$thold['thold_type']]), $text); } $text = thold_str_replace('', $esc($thold['notes']), $text); @@ -4307,7 +4403,7 @@ function thold_replace_threshold_tags($text, &$thold, &$h, $currentval, $local_g $text = thold_str_replace('', $thold['time_hi'], $text); $text = thold_str_replace('', $thold['time_low'], $text); $text = thold_str_replace('', $thold['time_fail_trigger'], $text); - $text = thold_str_replace('', plugin_thold_duration_convert($thold['local_data_id'], $thold['time_fail_length'], 'time'), $text); + $text = thold_str_replace('', $esc(plugin_thold_duration_convert($thold['local_data_id'], $thold['time_fail_length'], 'time')), $text); } else { $text = thold_str_replace('', '', $text); $text = thold_str_replace('', '', $text); @@ -4319,7 +4415,12 @@ function thold_replace_threshold_tags($text, &$thold, &$h, $currentval, $local_g $text = thold_str_replace('', date(CACTI_DATE_TIME_FORMAT), $text); $text = thold_str_replace('', date(DATE_RFC822), $text); - $text = thold_str_replace('', "" . __('Link to Graph in Cacti', 'thold') . '', $text); + if ($shell) { + /* An anchor in a command line would be parsed as redirections, so a trigger command gets the bare URL. */ + $text = thold_str_replace('', $esc("$httpurl/graph.php?local_graph_id=$local_graph_id"), $text); + } else { + $text = thold_str_replace('', "" . __('Link to Graph in Cacti', 'thold') . '', $text); + } $data = [ 'thold_data' => $thold, @@ -8404,6 +8505,27 @@ function thold_get_cached_name(&$thold_data) { return $thold_data['name_cache']; } +/** + * Quote a user-supplied value for use as the operand of an RLIKE comparison. + * + * Cacti 1.2.31 added db_qstr_rlike() as the remediation for GHSA-69gg-xrh3-gp82: + * on top of quoting, it caps the operand at 255 bytes and strips the alternation + * and quantifier characters that made the regular expression a denial-of-service + * vector. The plugin still supports 1.2.25, which predates that helper, so fall + * back to plain quoting there. + * + * @param string $value Raw filter value. + * + * @return string RLIKE operator and quoted operand, ready to concatenate. + */ +function thold_rlike_clause($value) { + if (function_exists('db_qstr_rlike')) { + return db_qstr_rlike($value); + } + + return 'RLIKE ' . db_qstr($value); +} + function thold_str_replace($search, $replace, $subject) { if (empty($replace) || $replace === 0) { $replace = ''; From 0d83fb6bfdb048268ad85341f7df0e1b933da89e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:48:07 -0700 Subject: [PATCH 29/41] fix(security): prefer Cacti's RLIKE quoting helper for the name filter db_qstr_rlike() is core's remediation for GHSA-69gg-xrh3-gp82: on top of quoting it bounds the operand and strips the alternation characters that made the pattern a denial-of-service vector. It arrived in 1.2.31, so the plugin falls back to plain quoting on the 1.2.25 it still declares support for. Signed-off-by: Thomas Vincent --- thold.php | 2 +- thold_graph.php | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/thold.php b/thold.php index dc400246..e9c491c6 100644 --- a/thold.php +++ b/thold.php @@ -614,7 +614,7 @@ function list_tholds() { } if (get_request_var('rfilter') != '') { - $sql_where .= ($sql_where == '' ? '(' : ' AND ') . ' td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= ($sql_where == '' ? '(' : ' AND ') . ' td.name_cache ' . thold_rlike_clause(get_request_var('rfilter')); } if ($statefilter != '') { diff --git a/thold_graph.php b/thold_graph.php index 7629e450..77652936 100644 --- a/thold_graph.php +++ b/thold_graph.php @@ -404,7 +404,7 @@ function tholds() { $statefilter = thold_get_state_filter(get_request_var('state')); if (get_request_var('rfilter') != '') { - $sql_where .= ($sql_where == '' ? '(' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= ($sql_where == '' ? '(' : ' AND ') . 'td.name_cache ' . thold_rlike_clause(get_request_var('rfilter')); } if (get_request_var('data_template_id') != '-1') { @@ -937,8 +937,8 @@ function hosts() { if (get_request_var('rfilter') != '') { $sql_where .= " (h.deleted = '' - AND (h.hostname RLIKE " . db_qstr(get_request_var('rfilter')) . ' - OR h.description RLIKE ' . db_qstr(get_request_var('rfilter')) . ')'; + AND (h.hostname " . thold_rlike_clause(get_request_var('rfilter')) . ' + OR h.description ' . thold_rlike_clause(get_request_var('rfilter')) . ')'; } if (get_request_var('host_status') == '-1') { @@ -1395,7 +1395,7 @@ function thold_export_log() { } if (get_request_var('rfilter') != '') { - $sql_where .= ($sql_where == '' ? '' : ' AND') . ' tl.description RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= ($sql_where == '' ? '' : ' AND') . ' tl.description ' . thold_rlike_clause(get_request_var('rfilter')); } $sql_order = ''; @@ -1490,7 +1490,7 @@ function thold_show_log() { } if (get_request_var('rfilter') != '') { - $sql_where .= ($sql_where == '' ? '' : ' AND') . ' tl.description RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= ($sql_where == '' ? '' : ' AND') . ' tl.description ' . thold_rlike_clause(get_request_var('rfilter')); } $sql_order = get_order_string(); From ed90d6562761cc76f2c582ec6ff21f91e23ef542 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:48:15 -0700 Subject: [PATCH 30/41] fix(notify_lists): repair the bulk actions, which never ran get_filter_request_var() stores drp_action as an int while the allowlist held strings, so the strict in_array() rejected every action and each one redirected without touching the database. The delete also bound $selected_items with its submitted keys intact, which PDO reads as named parameters, and committed through Cacti's db_commit_transaction(), which tests a MariaDB-only system variable and so never commits on MySQL. Signed-off-by: Thomas Vincent --- notify_lists.php | 144 +++++++++++++++++++++++------------------------ 1 file changed, 71 insertions(+), 73 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 72a11b3d..ad4afb6c 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -146,13 +146,17 @@ function form_actions() { global $actions, $assoc_actions; // ================= input validation ================= + /* + * get_filter_request_var() stores the value as an int, so the comparison + * has to be made on strings for the strict check to mean anything. + */ get_filter_request_var('drp_action'); $valid_actions = array_map('strval', array_keys($actions + $assoc_actions)); - if (!in_array(get_request_var('drp_action'), $valid_actions, true)) { + if (!in_array((string) get_request_var('drp_action'), $valid_actions, true)) { raise_message(40); - header('Location: notify_lists.php'); + header('Location: notify_lists.php?header=false'); exit; } // ==================================================== @@ -164,59 +168,53 @@ function form_actions() { if (isset_request_var('save_list')) { if ($selected_items != false) { if (get_request_var('drp_action') == '1') { // delete - $placeholders = implode(',', array_fill(0, cacti_sizeof($selected_items), '?')); - - db_begin_transaction(); - - // Chain with && so the first failure short-circuits the remaining statements. - $ok = db_execute_prepared('DELETE FROM plugin_notification_lists - WHERE id IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE host - SET thold_send_email = 0 - WHERE thold_send_email = 2 - AND deleted = "" - AND thold_host_email IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE host - SET thold_send_email = 1 - WHERE thold_send_email = 3 - AND deleted = "" - AND thold_host_email IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE host - SET thold_host_email = 0 - WHERE thold_host_email IN (' . $placeholders . ') - AND deleted = ""', - $selected_items) - - && db_execute_prepared('UPDATE thold_data - SET notify_warning = 0 - WHERE notify_warning IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE thold_data - SET notify_alert = 0 - WHERE notify_alert IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE thold_template - SET notify_warning = 0 - WHERE notify_warning IN (' . $placeholders . ')', - $selected_items) - - && db_execute_prepared('UPDATE thold_template - SET notify_alert = 0 - WHERE notify_alert IN (' . $placeholders . ')', - $selected_items); + /* + * Bind positionally on the values: sanitize_unserialize_selected_items() + * preserves the submitted array's keys, and a string key would be read + * as a named parameter. + */ + $ids = array_map('intval', array_values($selected_items)); + $placeholders = implode(', ', array_fill(0, cacti_sizeof($ids), '?')); + + /* + * Issued as SQL rather than through db_begin_transaction() and + * friends: Cacti's db_commit_transaction() gates the commit on + * SELECT @@in_transaction, which only MariaDB defines. On MySQL that + * query fails, the commit is skipped, and every write here is + * discarded when the connection closes. + */ + db_execute('START TRANSACTION'); + + /* + * The host reset deliberately omits the deleted = "" predicate its + * siblings carry: a soft-deleted device that is later restored must + * not come back pointing at a list that no longer exists. + */ + $statements = array( + 'DELETE FROM plugin_notification_lists WHERE id IN (' . $placeholders . ')', + 'UPDATE host SET thold_send_email = 0 WHERE thold_send_email = 2 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', + 'UPDATE host SET thold_send_email = 1 WHERE thold_send_email = 3 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', + 'UPDATE host SET thold_host_email = 0 WHERE thold_host_email IN (' . $placeholders . ')', + 'UPDATE thold_data SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', + 'UPDATE thold_data SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', + 'UPDATE thold_template SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', + 'UPDATE thold_template SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')' + ); + + $ok = true; + + foreach ($statements as $sql) { + if (!db_execute_prepared($sql, $ids)) { + $ok = false; + + break; + } + } if ($ok) { - db_commit_transaction(); + db_execute('COMMIT'); } else { - db_rollback_transaction(); + db_execute('ROLLBACK'); } } elseif (get_request_var('drp_action') == '2') { // duplicate $i = 1; @@ -269,7 +267,7 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); - db_begin_transaction(); + db_execute('START TRANSACTION'); $ok = true; @@ -415,9 +413,9 @@ function form_actions() { } if ($ok) { - db_commit_transaction(); + db_execute('COMMIT'); } else { - db_rollback_transaction(); + db_execute('ROLLBACK'); } } @@ -432,7 +430,7 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); - db_begin_transaction(); + db_execute('START TRANSACTION'); $ok = true; $update_template = []; @@ -524,7 +522,7 @@ function form_actions() { } if ($ok) { - db_commit_transaction(); + db_execute('COMMIT'); // Propagate template changes to threshold instances after the // notification assignment is committed so this cascade does not @@ -533,7 +531,7 @@ function form_actions() { thold_template_update_thresholds($template_id); } } else { - db_rollback_transaction(); + db_execute('ROLLBACK'); } } @@ -548,7 +546,7 @@ function form_actions() { get_filter_request_var('notification_warning_action'); get_filter_request_var('notification_alert_action'); - db_begin_transaction(); + db_execute('START TRANSACTION'); $ok = true; @@ -635,9 +633,9 @@ function form_actions() { } if ($ok) { - db_commit_transaction(); + db_execute('COMMIT'); } else { - db_rollback_transaction(); + db_execute('ROLLBACK'); } } @@ -720,7 +718,7 @@ function form_actions() { - + $save_html "; @@ -795,10 +793,10 @@ function form_actions() { print " - + - + $save_html "; @@ -873,10 +871,10 @@ function form_actions() { print " - + - + $save_html "; @@ -958,10 +956,10 @@ function form_actions() { print " - + - + $save_html "; @@ -1531,7 +1529,7 @@ function tholds($header_label) { if (strlen(get_request_var('rfilter'))) { // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the // request validation array; db_qstr() SQL-escapes the already-validated value. - $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache ' . thold_rlike_clause(get_request_var('rfilter')); } if ($statefilter != '') { @@ -1873,7 +1871,7 @@ function templates($header_label) { if (strlen(get_request_var('rfilter'))) { // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the // request validation array; db_qstr() SQL-escapes the already-validated value. - $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . 'thold_template.name RLIKE ' . db_qstr(get_request_var('rfilter')); + $sql_where .= (!strlen($sql_where) ? 'WHERE ' : ' AND ') . 'thold_template.name ' . thold_rlike_clause(get_request_var('rfilter')); } $sql = "SELECT * @@ -2280,9 +2278,9 @@ function clearFilter() { // rfilter is pre-validated as a legal PHP regex by FILTER_VALIDATE_IS_REGEX in the // request validation array; db_qstr() SQL-escapes the already-validated value. $sql_where = 'WHERE ( - name RLIKE ' . db_qstr(get_request_var('rfilter')) . ' - OR description RLIKE ' . db_qstr(get_request_var('rfilter')) . ' - OR emails RLIKE ' . db_qstr(get_request_var('rfilter')) . ')'; + name ' . thold_rlike_clause(get_request_var('rfilter')) . ' + OR description ' . thold_rlike_clause(get_request_var('rfilter')) . ' + OR emails ' . thold_rlike_clause(get_request_var('rfilter')) . ')'; } else { $sql_where = ''; } From 8c989301201f73675e9801981c8139998ffbc3db Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:48:15 -0700 Subject: [PATCH 31/41] docs(changelog): record the hardening and the bugs it uncovered Signed-off-by: Thomas Vincent --- CHANGELOG.md | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c909402b..735d8bac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,15 +2,17 @@ --- develop --- -* security: Replace array_to_sql_or() and direct $selected_items concatenation with db_execute_prepared() and IN(?,?,?) placeholders in notify_lists.php bulk form actions -* security: Wrap all four bulk action blocks in db_begin_transaction()/db_commit_transaction(); rollback on db_execute_prepared() failure; break per-item loops immediately on error -* security: Move thold_template_update_thresholds() cascade after db_commit_transaction() so it does not participate in the transaction boundary -* security: Parameterize $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() using gl.id = ? placeholder; switch to db_fetch_assoc_prepared() and db_fetch_cell_prepared() -* security: Validate rfilter via FILTER_VALIDATE_IS_REGEX and escape with db_qstr() before use in RLIKE clauses -* security: Apply html_escape() to get_request_var('page') in thold.php and thold_graph.php hidden inputs; wrap AJAX filter URL params with encodeURIComponent() -* security: Apply sanitize_unserialize_selected_items() to selected_graphs_array in thold_webapi.php -* security: Cast drp_action allowlist keys to strings via array_map('strval', array_keys(...)) for correct strict in_array() comparison -* security: Add Pest v1 security test suite covering prepared statements, RLIKE injection, XSS escaping, unserialize hardening, PHP 7.4 compatibility, and smoke linting +* security: Use prepared statements for the bulk form actions in notify_lists.php and notify_queue.php +* security: Bind $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() instead of interpolating it +* security: Route rfilter through db_qstr_rlike() where Cacti provides it, and quote it otherwise +* security: Escape the values substituted into trigger commands, and expand |pipe| tokens before escaping rather than after +* security: Escape the page, id and drp_action values printed into hidden inputs +* security: Remove the eval() calls from the RPN expression evaluator +* issue: Bulk actions on the Notification Lists page did nothing, because the action allowlist compared an int against strings +* issue: Bulk writes were discarded on MySQL, where Cacti's db_commit_transaction() never commits +* issue: An RPN expression dividing zero by zero pushed no result, corrupting the rest of the stack +* issue: An RPN expression taking the modulo of zero, the square root of a negative, or the log of zero aborted the poller or produced NAN +* issue: Deleting a notification list left soft-deleted devices pointing at it * issue#686: Applying a templated threshold to a graph via the wrench icon, creates a duplicate graph * issue#707: Excessive timeout for row caching prevents data from being updated timely * issue#710: Fixing Typo in thold_daemons.service File From 3b839adedbdac546484528c6e056128ebf6ddf7e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:51:52 -0700 Subject: [PATCH 32/41] test: cover the trigger command paths and the optional core branches The tests that need Cacti to provide db_qstr_rlike() or get_total_row_data() run in their own process, so defining those functions does not change which branch the rest of the suite takes. Signed-off-by: Thomas Vincent --- tests/Unit/OptionalCoreFunctionTest.php | 104 +++++++++ tests/Unit/TholdCommandExecutionTest.php | 230 +++++++++++++++++++ tests/Unit/TholdGetCachedNameTest.php | 59 +++++ tests/Unit/TholdReplaceThresholdTagsTest.php | 21 ++ tests/bootstrap.php | 27 +++ 5 files changed, 441 insertions(+) create mode 100644 tests/Unit/OptionalCoreFunctionTest.php create mode 100644 tests/Unit/TholdCommandExecutionTest.php create mode 100644 tests/Unit/TholdGetCachedNameTest.php diff --git a/tests/Unit/OptionalCoreFunctionTest.php b/tests/Unit/OptionalCoreFunctionTest.php new file mode 100644 index 00000000..d9935fc4 --- /dev/null +++ b/tests/Unit/OptionalCoreFunctionTest.php @@ -0,0 +1,104 @@ +assertSame(db_qstr_rlike('router'), thold_rlike_clause('router')); + } + + /** + * Core's helper strips the alternation and quantifier characters that make + * a filter expensive to evaluate; the plugin must not bypass that. + * + * @return void + */ + public function testRlikeClauseInheritsTheCoreOperandRestrictions(): void { + $clause = thold_rlike_clause('(a|a){9,}'); + + $this->assertStringNotContainsString('|', $clause); + $this->assertStringNotContainsString('{', $clause); + } + + /** + * @return array + */ + public static function accessorProvider() { + return array( + 'thresholds' => array('get_allowed_thresholds', 'thold'), + 'logs' => array('get_allowed_threshold_logs', 'thold_log'), + ); + } + + /** + * The cached row count is used only for the unfiltered listing; a per-graph + * query counts directly so the cache is not keyed per graph. + * + * @dataProvider accessorProvider + * + * @param string $function + * @param string $class + * + * @return void + */ + public function testRowCountUsesTheCoreCacheWhenNotFilteredByGraph($function, $class): void { + CactiStub::willReturn('get_total_row_data', 12); + + $total = 0; + $function('', 'td.name', '', $total, -1, 0); + + $this->assertSame(12, $total); + $this->assertSame(array(), CactiStub::callsTo('db_fetch_cell_prepared')); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * @param string $class + * + * @return void + */ + public function testRowCountBypassesTheCacheForASingleGraph($function, $class): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 5); + + $this->assertSame(array(), CactiStub::callsTo('get_total_row_data')); + $this->assertNotEmpty(CactiStub::callsTo('db_fetch_cell_prepared')); + } +} diff --git a/tests/Unit/TholdCommandExecutionTest.php b/tests/Unit/TholdCommandExecutionTest.php new file mode 100644 index 00000000..70cfc7bd --- /dev/null +++ b/tests/Unit/TholdCommandExecutionTest.php @@ -0,0 +1,230 @@ + + */ + private function threshold(array $overrides = array()) { + return $overrides + array( + 'id' => 3, + 'local_data_id' => 4, + 'local_graph_id' => 7, + 'name' => 'CPU', + 'name_cache' => 'CPU', + 'data_source_name' => 'traffic_in', + 'lastread' => 95, + 'notes' => '', + 'dnotes' => '', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'thold_template_id' => 0, + 'trigger_cmd_high' => '', + 'trigger_cmd_low' => '', + 'trigger_cmd_norm' => '', + ); + } + + /** + * @return array + */ + private function device(array $overrides = array()) { + return $overrides + array( + 'id' => 2, + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + ); + } + + /** + * The queued command, or null when nothing was queued. + * + * @return string|null + */ + private function queuedCommand() { + foreach (CactiStub::callsTo('db_execute_prepared') as $call) { + foreach ($call['params'] as $param) { + if (is_string($param) && strpos($param, '"command"') !== false) { + $decoded = json_decode($param, true); + + return isset($decoded['command']) ? $decoded['command'] : null; + } + } + } + + return null; + } + + /** + * @return array}> + */ + public static function breachDirectionProvider() { + return array( + 'high' => array('trigger_cmd_high', array(true, false, false)), + 'low' => array('trigger_cmd_low', array(false, true, false)), + 'restore' => array('trigger_cmd_norm', array(false, false, true)), + ); + } + + /** + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testEachBreachDirectionRunsItsOwnCommand($column, array $breaches): void { + $thold = $this->threshold(array($column => '/usr/bin/alert ')); + $device = $this->device(); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertSame("/usr/bin/alert '10.0.0.1'", $this->queuedCommand()); + } + + /** + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testShellMetacharactersInDeviceDataAreQuoted($column, array $breaches): void { + $thold = $this->threshold(array($column => '/usr/bin/alert ')); + $device = $this->device(array('description' => '; touch /tmp/pwned')); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertSame("/usr/bin/alert '; touch /tmp/pwned'", $this->queuedCommand()); + } + + /** + * @return void + */ + public function testNothingRunsWhenScriptsAreDisabled(): void { + CactiStub::$configOptions['thold_enable_scripts'] = ''; + + $thold = $this->threshold(array('trigger_cmd_high' => '/usr/bin/alert')); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertNull($this->queuedCommand()); + } + + /** + * @return void + */ + public function testNothingRunsWhenTheDirectionHasNoCommandConfigured(): void { + $thold = $this->threshold(); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertNull($this->queuedCommand()); + } + + /** + * A high breach takes precedence, so a threshold configured for both cannot + * run two commands in one evaluation. + * + * @return void + */ + public function testHighBreachTakesPrecedenceOverLow(): void { + $thold = $this->threshold(array( + 'trigger_cmd_high' => '/usr/bin/high', + 'trigger_cmd_low' => '/usr/bin/low', + )); + $device = $this->device(); + + thold_command_execution($thold, $device, true, true, false); + + $this->assertSame('/usr/bin/high', $this->queuedCommand()); + } + + /** + * With the queue off the command runs inline, and its exit status and + * output are logged. + * + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testInlineExecutionLogsTheCommandOutput($column, array $breaches): void { + CactiStub::$configOptions['thold_notification_queue'] = ''; + + $thold = $this->threshold(array($column => '/bin/echo breach')); + $device = $this->device(); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertNull($this->queuedCommand()); + $this->assertNotEmpty(CactiStub::$log); + } + + /** + * @return void + */ + public function testInlineExecutionLogsANonZeroExitStatus(): void { + CactiStub::$configOptions['thold_notification_queue'] = ''; + + $thold = $this->threshold(array('trigger_cmd_high' => '/bin/false')); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertNotEmpty(CactiStub::$log); + } +} diff --git a/tests/Unit/TholdGetCachedNameTest.php b/tests/Unit/TholdGetCachedNameTest.php new file mode 100644 index 00000000..1c695114 --- /dev/null +++ b/tests/Unit/TholdGetCachedNameTest.php @@ -0,0 +1,59 @@ + '|data_source_description|', 'name_cache' => 'CPU load', 'local_data_id' => 4); + + $this->assertSame('CPU load', thold_get_cached_name($thold)); + $this->assertSame(array(), CactiStub::callsTo('db_fetch_cell_prepared')); + } + + /** + * @return void + */ + public function testEmptyCacheIsFilledFromTheDataSourceDescription(): void { + CactiStub::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + + $thold = array('name' => '|data_source_description|', 'name_cache' => '', 'local_data_id' => 4); + + $this->assertSame('Router - Traffic', thold_get_cached_name($thold)); + $this->assertSame('Router - Traffic', $thold['name_cache']); + } + + /** + * @return void + */ + public function testNameIsKeptWhenTheDataSourceHasNoDescription(): void { + $thold = array('name' => 'Manual name', 'name_cache' => '', 'local_data_id' => 4); + + $this->assertSame('Manual name', thold_get_cached_name($thold)); + } +} diff --git a/tests/Unit/TholdReplaceThresholdTagsTest.php b/tests/Unit/TholdReplaceThresholdTagsTest.php index 611352d6..194613ca 100644 --- a/tests/Unit/TholdReplaceThresholdTagsTest.php +++ b/tests/Unit/TholdReplaceThresholdTagsTest.php @@ -28,6 +28,9 @@ final class TholdReplaceThresholdTagsTest extends TestCase { */ public static function setUpBeforeClass(): void { self::loadPluginSource('thold_functions.php'); + + /* Defines $thold_types, which the substitution reads. */ + self::loadPluginSource('includes/arrays.php'); } /** @@ -242,6 +245,24 @@ public function testUrlTagRendersALinkToTheGraph(): void { $this->assertStringContainsString('graph.php?local_graph_id=7', $result); } + /** + * @return void + */ + public function testThresholdTypeNameIsSubstituted(): void { + $result = $this->substitute('', $this->threshold(), $this->device(), false); + + $this->assertSame('High / Low', $result); + } + + /** + * @return void + */ + public function testUnknownThresholdTypeLeavesTheTagInPlace(): void { + $result = $this->substitute('', $this->threshold(array('thold_type' => 99)), $this->device(), false); + + $this->assertSame('', $result); + } + /** * @return void */ diff --git a/tests/bootstrap.php b/tests/bootstrap.php index a6167f75..3f42ffea 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -323,3 +323,30 @@ function number_format_i18n($number, $decimals = 0, $baseu = 1000) { if (!defined('CACTI_PATH_BASE')) { define('CACTI_PATH_BASE', $GLOBALS['config']['base_path']); } + +/** + * Load a plugin source file at global scope. + * + * Several plugin files (includes/arrays.php in particular) define their data + * as file-scope variables that the rest of the plugin reads as globals, and + * they read $config while doing so. Requiring them from inside a method would + * make both halves of that method-local, so the require happens here and any + * variable the file introduced is published to $GLOBALS. + * + * @param string $path Absolute path to the file. + * + * @return void + */ +function thold_test_load($path) { + global $config; + + $__before = get_defined_vars(); + + require_once $path; + + foreach (get_defined_vars() as $__name => $__value) { + if (!array_key_exists($__name, $__before) && strncmp($__name, '__', 2) !== 0) { + $GLOBALS[$__name] = $__value; + } + } +} From c23120da62c899926347075d1895208ee274a22b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:51:52 -0700 Subject: [PATCH 33/41] fix(thold): log the result of an inline trigger command thold_process_command_output() dispatches on the topic it is passed, and 'thold' matched none of its branches, so a trigger command run outside the notification queue recorded neither its exit status nor its output. Signed-off-by: Thomas Vincent --- thold_functions.php | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/thold_functions.php b/thold_functions.php index e3b612ee..12bf7c8e 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -4203,8 +4203,13 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $command_executed = true; } - if ($queue == '' && $command_executed) { - thold_process_command_output($output, $return, 'thold', $thold_data, $cmd); + /* + * Topic has to be thold_cmd: thold_process_command_output() dispatches + * on it, and 'thold' matched no branch, so an inline trigger command + * logged neither its exit status nor its output. + */ + if ($queue != 'on' && $command_executed) { + thold_process_command_output($output, $return, 'thold_cmd', $thold_data, $cmd); } } } From a1a8a6c6893fdac790d72ecf56927f9d5076e360 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 17:52:13 -0700 Subject: [PATCH 34/41] ci: leave the Apache PHP package change to its own pull request Signed-off-by: Thomas Vincent --- .github/workflows/plugin-ci-workflow.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 5e4f3db6..ccd89839 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -98,7 +98,7 @@ jobs: run: sudo apt-get update - name: Install System Dependencies - run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping + run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping libapache2-mod-php - name: Start SNMPD Agent and Test run: | From 46011590484f9ac10bb64ee964e5238d62d595dd Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 18:22:37 -0700 Subject: [PATCH 35/41] fix(thold): quote substituted values before the trigger command is assembled Also covers the four exit-status and output combinations the command result logging distinguishes. Signed-off-by: Thomas Vincent --- CHANGELOG.md | 2 +- tests/Unit/TholdCommandExecutionTest.php | 26 +++++++++++++++++++++--- thold_functions.php | 24 ++++++---------------- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 735d8bac..ae48cd43 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ * security: Use prepared statements for the bulk form actions in notify_lists.php and notify_queue.php * security: Bind $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() instead of interpolating it * security: Route rfilter through db_qstr_rlike() where Cacti provides it, and quote it otherwise -* security: Escape the values substituted into trigger commands, and expand |pipe| tokens before escaping rather than after +* security: Quote the values substituted into trigger commands * security: Escape the page, id and drp_action values printed into hidden inputs * security: Remove the eval() calls from the RPN expression evaluator * issue: Bulk actions on the Notification Lists page did nothing, because the action allowlist compared an int against strings diff --git a/tests/Unit/TholdCommandExecutionTest.php b/tests/Unit/TholdCommandExecutionTest.php index 70cfc7bd..4a61696b 100644 --- a/tests/Unit/TholdCommandExecutionTest.php +++ b/tests/Unit/TholdCommandExecutionTest.php @@ -215,16 +215,36 @@ public function testInlineExecutionLogsTheCommandOutput($column, array $breaches } /** + * @return array + */ + public static function inlineOutcomeProvider() { + return array( + 'success without output' => array('/bin/true', 'NOTE'), + 'success with output' => array('/bin/echo ok', 'NOTE'), + 'failure without output' => array('/bin/false', 'WARNING'), + 'failure with output' => array("/bin/sh -c 'echo oops; exit 1'", 'WARNING'), + ); + } + + /** + * A trigger command that fails is the operator's only signal that their + * alerting is broken, so the exit status has to reach the log either way. + * + * @dataProvider inlineOutcomeProvider + * + * @param string $command + * @param string $level + * * @return void */ - public function testInlineExecutionLogsANonZeroExitStatus(): void { + public function testInlineExecutionLogsTheExitStatus($command, $level): void { CactiStub::$configOptions['thold_notification_queue'] = ''; - $thold = $this->threshold(array('trigger_cmd_high' => '/bin/false')); + $thold = $this->threshold(array('trigger_cmd_high' => $command)); $device = $this->device(); thold_command_execution($thold, $device, true, false, false); - $this->assertNotEmpty(CactiStub::$log); + $this->assertStringStartsWith($level, CactiStub::$log[0]); } } diff --git a/thold_functions.php b/thold_functions.php index 12bf7c8e..eccc26c6 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -4124,11 +4124,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $queue = read_config_option('thold_notification_queue'); if ($breach_up && $thold_data['trigger_cmd_high'] != '') { - /* - * Expansion runs first: it splices |query_*| and |host_*| values in - * verbatim, so running it after the tag escaping would let a device - * field expand inside the quotes the escaping just added. - */ + /* Expand before the tags, so quoting is applied to the final text. */ $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_high']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4145,16 +4141,12 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; substituted values are quoted by thold_replace_threshold_tags } $command_executed = true; } elseif ($breach_down && $thold_data['trigger_cmd_low'] != '') { - /* - * Expansion runs first: it splices |query_*| and |host_*| values in - * verbatim, so running it after the tag escaping would let a device - * field expand inside the quotes the escaping just added. - */ + /* Expand before the tags, so quoting is applied to the final text. */ $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_low']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4171,16 +4163,12 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; substituted values are quoted by thold_replace_threshold_tags } $command_executed = true; } elseif ($breach_norm && $thold_data['trigger_cmd_norm'] != '') { - /* - * Expansion runs first: it splices |query_*| and |host_*| values in - * verbatim, so running it after the tag escaping would let a device - * field expand inside the quotes the escaping just added. - */ + /* Expand before the tags, so quoting is applied to the final text. */ $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_norm']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4197,7 +4185,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b thold_notification_add('thold_cmd', $data, 'id', 0, $h); } else { - exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; values are quoted by thold_replace_threshold_tags, |query_*| values are not + exec($cmd, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured alert command; substituted values are quoted by thold_replace_threshold_tags } $command_executed = true; From e1315a6c736d57b4f4e6db8e2121cf5379007271 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 18:29:45 -0700 Subject: [PATCH 36/41] test: move the Cacti library fixture out of an ignored path Signed-off-by: Thomas Vincent --- tests/bootstrap.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/bootstrap.php b/tests/bootstrap.php index 3f42ffea..b12282bd 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -41,7 +41,7 @@ ); /* thold_expand_string() include_once()s library_path/variables.php at call time. */ -$GLOBALS['config']['library_path'] = __DIR__ . '/fixtures/lib'; +$GLOBALS['config']['library_path'] = __DIR__ . '/fixtures/cacti-lib'; /* thold reads and writes this on every RPN evaluation. */ $GLOBALS['rpn_error'] = false; From 97d18b4a6ec23c8a37bbfcbc63bd46ae7f3683e8 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 18:32:52 -0700 Subject: [PATCH 37/41] style: apply the project php-cs-fixer configuration Signed-off-by: Thomas Vincent --- notify_lists.php | 4 +- tests/Support/CactiStub.php | 6 +- tests/Unit/GetAllowedThresholdsTest.php | 28 ++--- tests/Unit/OptionalCoreFunctionTest.php | 12 +- tests/Unit/TholdCalculateLowerUpperTest.php | 16 +-- tests/Unit/TholdCalculatePercentTest.php | 12 +- tests/Unit/TholdCommandExecutionTest.php | 54 ++++----- tests/Unit/TholdExpandStringTest.php | 16 +-- tests/Unit/TholdExpressionMathRpnTest.php | 116 +++++++++---------- tests/Unit/TholdGetCachedNameTest.php | 8 +- tests/Unit/TholdReplaceThresholdTagsTest.php | 50 ++++---- tests/Unit/TholdSetEnvironTest.php | 28 ++--- tests/bootstrap.php | 44 +++---- thold_functions.php | 10 +- 14 files changed, 202 insertions(+), 202 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index ad4afb6c..72c549e6 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -190,7 +190,7 @@ function form_actions() { * siblings carry: a soft-deleted device that is later restored must * not come back pointing at a list that no longer exists. */ - $statements = array( + $statements = [ 'DELETE FROM plugin_notification_lists WHERE id IN (' . $placeholders . ')', 'UPDATE host SET thold_send_email = 0 WHERE thold_send_email = 2 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', 'UPDATE host SET thold_send_email = 1 WHERE thold_send_email = 3 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', @@ -199,7 +199,7 @@ function form_actions() { 'UPDATE thold_data SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', 'UPDATE thold_template SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', 'UPDATE thold_template SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')' - ); + ]; $ok = true; diff --git a/tests/Support/CactiStub.php b/tests/Support/CactiStub.php index 4c55996b..17a5ab48 100644 --- a/tests/Support/CactiStub.php +++ b/tests/Support/CactiStub.php @@ -80,9 +80,9 @@ public static function reset() { /** * Record one Cacti function call. * - * @param string $fn Cacti function name. - * @param string $sql SQL text, or '' for non-query calls. - * @param array $params Bound parameters, if any. + * @param string $fn Cacti function name. + * @param string $sql SQL text, or '' for non-query calls. + * @param array $params Bound parameters, if any. * * @return void */ diff --git a/tests/Unit/GetAllowedThresholdsTest.php b/tests/Unit/GetAllowedThresholdsTest.php index d6301b6a..9261f90d 100644 --- a/tests/Unit/GetAllowedThresholdsTest.php +++ b/tests/Unit/GetAllowedThresholdsTest.php @@ -34,10 +34,10 @@ public static function setUpBeforeClass(): void { * @return array */ public static function accessorProvider() { - return array( - 'thresholds' => array('get_allowed_thresholds'), - 'logs' => array('get_allowed_threshold_logs'), - ); + return [ + 'thresholds' => ['get_allowed_thresholds'], + 'logs' => ['get_allowed_threshold_logs'], + ]; } /** @@ -55,7 +55,7 @@ public function testGraphIdIsBoundRatherThanInterpolated($function): void { $this->assertStringContainsString('gl.id = ?', $call['sql']); $this->assertStringNotContainsString('42', $call['sql']); - $this->assertSame(array(42), $call['params']); + $this->assertSame([42], $call['params']); } /** @@ -91,11 +91,11 @@ public function testMaliciousGraphIdNeverReachesQueryText($function): void { */ public function testCallerParametersAreBoundBeforeTheGraphIdParameter($function): void { $total = 0; - $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, array(3)); + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; - $this->assertSame(array(3, 7), $call['params']); + $this->assertSame([3, 7], $call['params']); $this->assertStringContainsString('td.thold_type = ? AND gl.id = ?', $call['sql']); } @@ -113,7 +113,7 @@ public function testNoWhereClauseIsEmittedWhenNothingFiltersTheQuery($function): $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; $this->assertStringNotContainsString('WHERE', $call['sql']); - $this->assertSame(array(), $call['params']); + $this->assertSame([], $call['params']); } /** @@ -128,11 +128,11 @@ public function testNoWhereClauseIsEmittedWhenNothingFiltersTheQuery($function): */ public function testRowCountQueryBindsTheSameParameters($function): void { $total = 0; - $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, array(3)); + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); $count = CactiStub::callsTo('db_fetch_cell_prepared')[0]; - $this->assertSame(array(3, 7), $count['params']); + $this->assertSame([3, 7], $count['params']); } /** @@ -160,12 +160,12 @@ public function testOrderByAndLimitAreAppliedToTheQuery($function): void { * @return void */ public function testResultRowsAreReturnedToTheCaller($function): void { - CactiStub::willReturn('db_fetch_assoc_prepared', array(array('id' => 5))); + CactiStub::willReturn('db_fetch_assoc_prepared', [['id' => 5]]); $total = 0; $rows = $function('', 'td.name', '', $total, -1, 0); - $this->assertSame(array(array('id' => 5)), $rows); + $this->assertSame([['id' => 5]], $rows); } /** @@ -201,8 +201,8 @@ public function testNoQueryRunsWhenAuthenticationIsOnAndNoUserIsResolved($functi $total = 0; $rows = $function('', 'td.name', '', $total, 0, 0); - $this->assertSame(array(), $rows); - $this->assertSame(array(), CactiStub::callsTo('db_fetch_assoc_prepared')); + $this->assertSame([], $rows); + $this->assertSame([], CactiStub::callsTo('db_fetch_assoc_prepared')); } /** diff --git a/tests/Unit/OptionalCoreFunctionTest.php b/tests/Unit/OptionalCoreFunctionTest.php index d9935fc4..1400ad69 100644 --- a/tests/Unit/OptionalCoreFunctionTest.php +++ b/tests/Unit/OptionalCoreFunctionTest.php @@ -59,10 +59,10 @@ public function testRlikeClauseInheritsTheCoreOperandRestrictions(): void { * @return array */ public static function accessorProvider() { - return array( - 'thresholds' => array('get_allowed_thresholds', 'thold'), - 'logs' => array('get_allowed_threshold_logs', 'thold_log'), - ); + return [ + 'thresholds' => ['get_allowed_thresholds', 'thold'], + 'logs' => ['get_allowed_threshold_logs', 'thold_log'], + ]; } /** @@ -83,7 +83,7 @@ public function testRowCountUsesTheCoreCacheWhenNotFilteredByGraph($function, $c $function('', 'td.name', '', $total, -1, 0); $this->assertSame(12, $total); - $this->assertSame(array(), CactiStub::callsTo('db_fetch_cell_prepared')); + $this->assertSame([], CactiStub::callsTo('db_fetch_cell_prepared')); } /** @@ -98,7 +98,7 @@ public function testRowCountBypassesTheCacheForASingleGraph($function, $class): $total = 0; $function('', 'td.name', '', $total, -1, 5); - $this->assertSame(array(), CactiStub::callsTo('get_total_row_data')); + $this->assertSame([], CactiStub::callsTo('get_total_row_data')); $this->assertNotEmpty(CactiStub::callsTo('db_fetch_cell_prepared')); } } diff --git a/tests/Unit/TholdCalculateLowerUpperTest.php b/tests/Unit/TholdCalculateLowerUpperTest.php index 46adc2f4..b76ba75b 100644 --- a/tests/Unit/TholdCalculateLowerUpperTest.php +++ b/tests/Unit/TholdCalculateLowerUpperTest.php @@ -31,8 +31,8 @@ public static function setUpBeforeClass(): void { * @return void */ public function testHighWordIsShiftedAndCombinedWithTheLowWord(): void { - $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); - $rrd = array(4 => array('octets_hi' => 2)); + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_hi' => 2]]; $this->assertSame((2 << 32) + 100, thold_calculate_lower_upper($thold, 100, $rrd)); } @@ -41,8 +41,8 @@ public function testHighWordIsShiftedAndCombinedWithTheLowWord(): void { * @return void */ public function testValuePassesThroughWhenTheHighWordIsAbsent(): void { - $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); - $rrd = array(4 => array('octets_lo' => 5)); + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_lo' => 5]]; $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); } @@ -51,17 +51,17 @@ public function testValuePassesThroughWhenTheHighWordIsAbsent(): void { * @return void */ public function testValuePassesThroughWhenTheDataSourceHasNoReadings(): void { - $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; - $this->assertSame(100, thold_calculate_lower_upper($thold, 100, array())); + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, [])); } /** * @return void */ public function testHighWordOfZeroLeavesTheValueUnchanged(): void { - $thold = array('upper_ds' => 'octets_hi', 'local_data_id' => 4); - $rrd = array(4 => array('octets_hi' => 0)); + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_hi' => 0]]; $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); } diff --git a/tests/Unit/TholdCalculatePercentTest.php b/tests/Unit/TholdCalculatePercentTest.php index 5678066a..badc058c 100644 --- a/tests/Unit/TholdCalculatePercentTest.php +++ b/tests/Unit/TholdCalculatePercentTest.php @@ -33,14 +33,14 @@ public static function setUpBeforeClass(): void { * @return array */ private function threshold() { - return array('percent_ds' => 'total', 'local_data_id' => 4); + return ['percent_ds' => 'total', 'local_data_id' => 4]; } /** * @return void */ public function testReadingIsExpressedAsAPercentageOfTheReferenceDataSource(): void { - $rrd = array(4 => array('total' => 200)); + $rrd = [4 => ['total' => 200]]; $this->assertSame(25.0, thold_calculate_percent($this->threshold(), 50, $rrd)); } @@ -49,7 +49,7 @@ public function testReadingIsExpressedAsAPercentageOfTheReferenceDataSource(): v * @return void */ public function testNonNumericReadingYieldsTheNoValueSentinel(): void { - $rrd = array(4 => array('total' => 200)); + $rrd = [4 => ['total' => 200]]; $this->assertSame('', thold_calculate_percent($this->threshold(), 'U', $rrd)); } @@ -58,7 +58,7 @@ public function testNonNumericReadingYieldsTheNoValueSentinel(): void { * @return void */ public function testMissingReferenceDataSourceYieldsTheNoValueSentinel(): void { - $rrd = array(4 => array('other' => 200)); + $rrd = [4 => ['other' => 200]]; $this->assertSame('', thold_calculate_percent($this->threshold(), 50, $rrd)); } @@ -67,7 +67,7 @@ public function testMissingReferenceDataSourceYieldsTheNoValueSentinel(): void { * @return void */ public function testZeroReferenceYieldsZeroRatherThanDividingByZero(): void { - $rrd = array(4 => array('total' => 0)); + $rrd = [4 => ['total' => 0]]; $this->assertSame(0, thold_calculate_percent($this->threshold(), 50, $rrd)); } @@ -76,7 +76,7 @@ public function testZeroReferenceYieldsZeroRatherThanDividingByZero(): void { * @return void */ public function testNegativeReferenceYieldsZero(): void { - $rrd = array(4 => array('total' => -5)); + $rrd = [4 => ['total' => -5]]; $this->assertSame(0, thold_calculate_percent($this->threshold(), 50, $rrd)); } diff --git a/tests/Unit/TholdCommandExecutionTest.php b/tests/Unit/TholdCommandExecutionTest.php index 4a61696b..643bfca5 100644 --- a/tests/Unit/TholdCommandExecutionTest.php +++ b/tests/Unit/TholdCommandExecutionTest.php @@ -43,8 +43,8 @@ protected function setUp(): void { /** * @return array */ - private function threshold(array $overrides = array()) { - return $overrides + array( + private function threshold(array $overrides = []) { + return $overrides + [ 'id' => 3, 'local_data_id' => 4, 'local_graph_id' => 7, @@ -63,14 +63,14 @@ private function threshold(array $overrides = array()) { 'trigger_cmd_high' => '', 'trigger_cmd_low' => '', 'trigger_cmd_norm' => '', - ); + ]; } /** * @return array */ - private function device(array $overrides = array()) { - return $overrides + array( + private function device(array $overrides = []) { + return $overrides + [ 'id' => 2, 'description' => 'router1', 'hostname' => '10.0.0.1', @@ -80,7 +80,7 @@ private function device(array $overrides = array()) { 'status_fail_date' => '2026-01-01 00:00:00', 'status_rec_date' => '2026-01-02 00:00:00', 'status_last_error' => '', - ); + ]; } /** @@ -106,23 +106,23 @@ private function queuedCommand() { * @return array}> */ public static function breachDirectionProvider() { - return array( - 'high' => array('trigger_cmd_high', array(true, false, false)), - 'low' => array('trigger_cmd_low', array(false, true, false)), - 'restore' => array('trigger_cmd_norm', array(false, false, true)), - ); + return [ + 'high' => ['trigger_cmd_high', [true, false, false]], + 'low' => ['trigger_cmd_low', [false, true, false]], + 'restore' => ['trigger_cmd_norm', [false, false, true]], + ]; } /** * @dataProvider breachDirectionProvider * - * @param string $column - * @param array $breaches + * @param string $column + * @param array $breaches * * @return void */ public function testEachBreachDirectionRunsItsOwnCommand($column, array $breaches): void { - $thold = $this->threshold(array($column => '/usr/bin/alert ')); + $thold = $this->threshold([$column => '/usr/bin/alert ']); $device = $this->device(); thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); @@ -139,8 +139,8 @@ public function testEachBreachDirectionRunsItsOwnCommand($column, array $breache * @return void */ public function testShellMetacharactersInDeviceDataAreQuoted($column, array $breaches): void { - $thold = $this->threshold(array($column => '/usr/bin/alert ')); - $device = $this->device(array('description' => '; touch /tmp/pwned')); + $thold = $this->threshold([$column => '/usr/bin/alert ']); + $device = $this->device(['description' => '; touch /tmp/pwned']); thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); @@ -153,7 +153,7 @@ public function testShellMetacharactersInDeviceDataAreQuoted($column, array $bre public function testNothingRunsWhenScriptsAreDisabled(): void { CactiStub::$configOptions['thold_enable_scripts'] = ''; - $thold = $this->threshold(array('trigger_cmd_high' => '/usr/bin/alert')); + $thold = $this->threshold(['trigger_cmd_high' => '/usr/bin/alert']); $device = $this->device(); thold_command_execution($thold, $device, true, false, false); @@ -180,10 +180,10 @@ public function testNothingRunsWhenTheDirectionHasNoCommandConfigured(): void { * @return void */ public function testHighBreachTakesPrecedenceOverLow(): void { - $thold = $this->threshold(array( + $thold = $this->threshold([ 'trigger_cmd_high' => '/usr/bin/high', 'trigger_cmd_low' => '/usr/bin/low', - )); + ]); $device = $this->device(); thold_command_execution($thold, $device, true, true, false); @@ -205,7 +205,7 @@ public function testHighBreachTakesPrecedenceOverLow(): void { public function testInlineExecutionLogsTheCommandOutput($column, array $breaches): void { CactiStub::$configOptions['thold_notification_queue'] = ''; - $thold = $this->threshold(array($column => '/bin/echo breach')); + $thold = $this->threshold([$column => '/bin/echo breach']); $device = $this->device(); thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); @@ -218,12 +218,12 @@ public function testInlineExecutionLogsTheCommandOutput($column, array $breaches * @return array */ public static function inlineOutcomeProvider() { - return array( - 'success without output' => array('/bin/true', 'NOTE'), - 'success with output' => array('/bin/echo ok', 'NOTE'), - 'failure without output' => array('/bin/false', 'WARNING'), - 'failure with output' => array("/bin/sh -c 'echo oops; exit 1'", 'WARNING'), - ); + return [ + 'success without output' => ['/bin/true', 'NOTE'], + 'success with output' => ['/bin/echo ok', 'NOTE'], + 'failure without output' => ['/bin/false', 'WARNING'], + 'failure with output' => ["/bin/sh -c 'echo oops; exit 1'", 'WARNING'], + ]; } /** @@ -240,7 +240,7 @@ public static function inlineOutcomeProvider() { public function testInlineExecutionLogsTheExitStatus($command, $level): void { CactiStub::$configOptions['thold_notification_queue'] = ''; - $thold = $this->threshold(array('trigger_cmd_high' => $command)); + $thold = $this->threshold(['trigger_cmd_high' => $command]); $device = $this->device(); thold_command_execution($thold, $device, true, false, false); diff --git a/tests/Unit/TholdExpandStringTest.php b/tests/Unit/TholdExpandStringTest.php index 151aa784..ca99a120 100644 --- a/tests/Unit/TholdExpandStringTest.php +++ b/tests/Unit/TholdExpandStringTest.php @@ -32,25 +32,25 @@ public static function setUpBeforeClass(): void { /** * @return array */ - private function thresholdData(array $overrides = array()) { - return $overrides + array( + private function thresholdData(array $overrides = []) { + return $overrides + [ 'local_graph_id' => 7, 'local_data_id' => 4, 'data_source_name' => 'traffic_in', 'thold_template_id' => 0, - ); + ]; } /** * @return void */ private function graphExists() { - CactiStub::willReturn('db_fetch_row_prepared', array( + CactiStub::willReturn('db_fetch_row_prepared', [ 'id' => 7, 'host_id' => 2, 'snmp_query_id' => 3, 'snmp_index' => '1', - )); + ]); } /** @@ -109,7 +109,7 @@ public function testInterfaceSpeedFallsBackToTheConfiguredDefaultWhenUnknown(): * @return void */ public function testTextIsReturnedUnchangedWhenTheGraphIsMissing(): void { - CactiStub::willReturn('db_fetch_row_prepared', array()); + CactiStub::willReturn('db_fetch_row_prepared', []); $this->assertSame('static text', thold_expand_string($this->thresholdData(), 'static text')); } @@ -124,7 +124,7 @@ public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): vo $this->graphExists(); CactiStub::willReturn('db_fetch_cell_prepared', 'Suggested |data_source_name|'); - $result = thold_expand_string($this->thresholdData(array('thold_template_id' => 5)), ''); + $result = thold_expand_string($this->thresholdData(['thold_template_id' => 5]), ''); $this->assertSame('Suggested traffic_in', $result); } @@ -133,7 +133,7 @@ public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): vo * @return void */ public function testSurroundingWhitespaceIsTrimmed(): void { - CactiStub::willReturn('db_fetch_row_prepared', array()); + CactiStub::willReturn('db_fetch_row_prepared', []); $this->assertSame('alert', thold_expand_string($this->thresholdData(), ' alert ')); } diff --git a/tests/Unit/TholdExpressionMathRpnTest.php b/tests/Unit/TholdExpressionMathRpnTest.php index 797762e0..b429fad1 100644 --- a/tests/Unit/TholdExpressionMathRpnTest.php +++ b/tests/Unit/TholdExpressionMathRpnTest.php @@ -48,16 +48,16 @@ private function evaluate(array $stack, $operator) { * @return array, 1: string, 2: float|int}> */ public static function binaryOperatorProvider() { - return array( - 'addition' => array(array(8, 2), '+', 10), - 'subtraction keeps order' => array(array(8, 2), '-', 6), - 'multiplication' => array(array(8, 2), '*', 16), - 'division keeps order' => array(array(8, 2), '/', 4), - 'modulo' => array(array(8, 3), '%', 2), - 'float addition' => array(array(1.5, 2.25), '+', 3.75), - 'numeric string operands' => array(array('8', '2'), '-', 6), - 'negative operands' => array(array(-8, 2), '/', -4), - ); + return [ + 'addition' => [[8, 2], '+', 10], + 'subtraction keeps order' => [[8, 2], '-', 6], + 'multiplication' => [[8, 2], '*', 16], + 'division keeps order' => [[8, 2], '/', 4], + 'modulo' => [[8, 3], '%', 2], + 'float addition' => [[1.5, 2.25], '+', 3.75], + 'numeric string operands' => [['8', '2'], '-', 6], + 'negative operands' => [[-8, 2], '/', -4], + ]; } /** @@ -70,7 +70,7 @@ public static function binaryOperatorProvider() { * @return void */ public function testBinaryOperatorsComputeInStackOrder(array $stack, $operator, $expected): void { - $this->assertSame(array($expected), $this->evaluate($stack, $operator)); + $this->assertSame([$expected], $this->evaluate($stack, $operator)); $this->assertFalse($GLOBALS['rpn_error']); } @@ -83,22 +83,22 @@ public function testBinaryOperatorsComputeInStackOrder(array $stack, $operator, * @return void */ public function testCaretOperatorIsIntegerXorNotExponentiation(): void { - $this->assertSame(array(6), $this->evaluate(array(5, 3), '^')); - $this->assertSame(array(1), $this->evaluate(array(2, 3), '^')); + $this->assertSame([6], $this->evaluate([5, 3], '^')); + $this->assertSame([1], $this->evaluate([2, 3], '^')); } /** * @return void */ public function testCaretOperatorTruncatesFloatOperandsToIntegers(): void { - $this->assertSame(array(6), $this->evaluate(array(5.9, 3.9), '^')); + $this->assertSame([6], $this->evaluate([5.9, 3.9], '^')); } /** * @return void */ public function testModuloTruncatesFloatOperandsToIntegers(): void { - $this->assertSame(array(1), $this->evaluate(array(7.9, 3.2), '%')); + $this->assertSame([1], $this->evaluate([7.9, 3.2], '%')); } /** @@ -108,7 +108,7 @@ public function testModuloTruncatesFloatOperandsToIntegers(): void { * @return void */ public function testZeroDividedByZeroYieldsZeroWithoutError(): void { - $this->assertSame(array(0), $this->evaluate(array(0, 0), '/')); + $this->assertSame([0], $this->evaluate([0, 0], '/')); $this->assertFalse($GLOBALS['rpn_error']); } @@ -116,7 +116,7 @@ public function testZeroDividedByZeroYieldsZeroWithoutError(): void { * @return void */ public function testDivisionByZeroFlagsErrorAndPushesNothing(): void { - $this->assertSame(array(), $this->evaluate(array(8, 0), '/')); + $this->assertSame([], $this->evaluate([8, 0], '/')); $this->assertTrue($GLOBALS['rpn_error']); } @@ -124,7 +124,7 @@ public function testDivisionByZeroFlagsErrorAndPushesNothing(): void { * @return void */ public function testModuloByZeroFlagsErrorInsteadOfThrowing(): void { - $this->assertSame(array(), $this->evaluate(array(8, 0), '%')); + $this->assertSame([], $this->evaluate([8, 0], '%')); $this->assertTrue($GLOBALS['rpn_error']); } @@ -132,12 +132,12 @@ public function testModuloByZeroFlagsErrorInsteadOfThrowing(): void { * @return array, 1: string}> */ public static function nonNumericOperandProvider() { - return array( - 'unknown right operand' => array(array(8, 'U'), '+'), - 'unknown left operand' => array(array('U', 8), '+'), - 'NaN right operand' => array(array(8, 'NAN'), '*'), - 'text operand' => array(array(8, 'abc'), '-'), - ); + return [ + 'unknown right operand' => [[8, 'U'], '+'], + 'unknown left operand' => [['U', 8], '+'], + 'NaN right operand' => [[8, 'NAN'], '*'], + 'text operand' => [[8, 'abc'], '-'], + ]; } /** @@ -149,7 +149,7 @@ public static function nonNumericOperandProvider() { * @return void */ public function testNonNumericOperandsFlagErrorAndPushNothing(array $stack, $operator): void { - $this->assertSame(array(), $this->evaluate($stack, $operator)); + $this->assertSame([], $this->evaluate($stack, $operator)); $this->assertTrue($GLOBALS['rpn_error']); $this->assertNotEmpty(CactiStub::$log); } @@ -158,20 +158,20 @@ public function testNonNumericOperandsFlagErrorAndPushNothing(array $stack, $ope * @return array */ public static function unaryFunctionProvider() { - return array( - 'SIN' => array(0, 'SIN', 0.0), - 'COS' => array(0, 'COS', 1.0), - 'TAN' => array(0, 'TAN', 0.0), - 'ATAN' => array(0, 'ATAN', 0.0), - 'SQRT' => array(9, 'SQRT', 3.0), - 'FLOOR' => array(2.7, 'FLOOR', 2.0), - 'CEIL' => array(2.1, 'CEIL', 3.0), - 'DEG2RAD' => array(180, 'DEG2RAD', M_PI), - 'RAD2DEG' => array(M_PI, 'RAD2DEG', 180.0), - 'ABS' => array(-5, 'ABS', 5), - 'EXP' => array(0, 'EXP', 1.0), - 'LOG' => array(M_E, 'LOG', 1.0), - ); + return [ + 'SIN' => [0, 'SIN', 0.0], + 'COS' => [0, 'COS', 1.0], + 'TAN' => [0, 'TAN', 0.0], + 'ATAN' => [0, 'ATAN', 0.0], + 'SQRT' => [9, 'SQRT', 3.0], + 'FLOOR' => [2.7, 'FLOOR', 2.0], + 'CEIL' => [2.1, 'CEIL', 3.0], + 'DEG2RAD' => [180, 'DEG2RAD', M_PI], + 'RAD2DEG' => [M_PI, 'RAD2DEG', 180.0], + 'ABS' => [-5, 'ABS', 5], + 'EXP' => [0, 'EXP', 1.0], + 'LOG' => [M_E, 'LOG', 1.0], + ]; } /** @@ -184,7 +184,7 @@ public static function unaryFunctionProvider() { * @return void */ public function testUnaryFunctionsDispatchToNativeMath($operand, $operator, $expected): void { - $stack = $this->evaluate(array($operand), $operator); + $stack = $this->evaluate([$operand], $operator); $this->assertCount(1, $stack); $this->assertEqualsWithDelta($expected, $stack[0], 1.0e-9); @@ -195,7 +195,7 @@ public function testUnaryFunctionsDispatchToNativeMath($operand, $operator, $exp * @return void */ public function testUnaryFunctionRejectsNonNumericOperand(): void { - $this->assertSame(array(), $this->evaluate(array('U'), 'SQRT')); + $this->assertSame([], $this->evaluate(['U'], 'SQRT')); $this->assertTrue($GLOBALS['rpn_error']); } @@ -207,11 +207,11 @@ public function testUnaryFunctionRejectsNonNumericOperand(): void { * @return array */ public static function undefinedResultProvider() { - return array( - 'square root of a negative' => array(-1, 'SQRT'), - 'log of zero' => array(0, 'LOG'), - 'log of a negative' => array(-1, 'LOG'), - ); + return [ + 'square root of a negative' => [-1, 'SQRT'], + 'log of zero' => [0, 'LOG'], + 'log of a negative' => [-1, 'LOG'], + ]; } /** @@ -223,7 +223,7 @@ public static function undefinedResultProvider() { * @return void */ public function testUndefinedResultsFlagErrorInsteadOfPushingNanOrInf($operand, $operator): void { - $this->assertSame(array(), $this->evaluate(array($operand), $operator)); + $this->assertSame([], $this->evaluate([$operand], $operator)); $this->assertTrue($GLOBALS['rpn_error']); } @@ -231,7 +231,7 @@ public function testUndefinedResultsFlagErrorInsteadOfPushingNanOrInf($operand, * @return void */ public function testAtan2ComputesAgainstBothOperands(): void { - $stack = $this->evaluate(array(1, 1), 'ATAN2'); + $stack = $this->evaluate([1, 1], 'ATAN2'); $this->assertEqualsWithDelta(M_PI / 4, $stack[0], 1.0e-9); } @@ -243,13 +243,13 @@ public function testAtan2ComputesAgainstBothOperands(): void { * @return array, 1: float|int}> */ public static function addNanProvider() { - return array( - 'both known' => array(array(3, 4), 7), - 'right unknown' => array(array(3, 'U'), 3), - 'left unknown' => array(array('U', 4), 4), - 'right NaN' => array(array(3, 'NAN'), 3), - 'both unknown' => array(array('U', 'NAN'), 0), - ); + return [ + 'both known' => [[3, 4], 7], + 'right unknown' => [[3, 'U'], 3], + 'left unknown' => [['U', 4], 4], + 'right NaN' => [[3, 'NAN'], 3], + 'both unknown' => [['U', 'NAN'], 0], + ]; } /** @@ -261,21 +261,21 @@ public static function addNanProvider() { * @return void */ public function testAddNanTreatsUnknownOperandsAsZero(array $stack, $expected): void { - $this->assertSame(array($expected), $this->evaluate($stack, 'ADDNAN')); + $this->assertSame([$expected], $this->evaluate($stack, 'ADDNAN')); } /** * @return void */ public function testUnknownOperatorLeavesStackUntouched(): void { - $this->assertSame(array(1, 2), $this->evaluate(array(1, 2), 'NOSUCHOP')); + $this->assertSame([1, 2], $this->evaluate([1, 2], 'NOSUCHOP')); } /** * @return void */ public function testUnderflowFlagsErrorRatherThanPoppingAnEmptyStack(): void { - $this->evaluate(array(), '+'); + $this->evaluate([], '+'); $this->assertTrue($GLOBALS['rpn_error']); } diff --git a/tests/Unit/TholdGetCachedNameTest.php b/tests/Unit/TholdGetCachedNameTest.php index 1c695114..65da52d5 100644 --- a/tests/Unit/TholdGetCachedNameTest.php +++ b/tests/Unit/TholdGetCachedNameTest.php @@ -30,10 +30,10 @@ public static function setUpBeforeClass(): void { * @return void */ public function testCachedNameIsReturnedWithoutQueryingTheDatabase(): void { - $thold = array('name' => '|data_source_description|', 'name_cache' => 'CPU load', 'local_data_id' => 4); + $thold = ['name' => '|data_source_description|', 'name_cache' => 'CPU load', 'local_data_id' => 4]; $this->assertSame('CPU load', thold_get_cached_name($thold)); - $this->assertSame(array(), CactiStub::callsTo('db_fetch_cell_prepared')); + $this->assertSame([], CactiStub::callsTo('db_fetch_cell_prepared')); } /** @@ -42,7 +42,7 @@ public function testCachedNameIsReturnedWithoutQueryingTheDatabase(): void { public function testEmptyCacheIsFilledFromTheDataSourceDescription(): void { CactiStub::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); - $thold = array('name' => '|data_source_description|', 'name_cache' => '', 'local_data_id' => 4); + $thold = ['name' => '|data_source_description|', 'name_cache' => '', 'local_data_id' => 4]; $this->assertSame('Router - Traffic', thold_get_cached_name($thold)); $this->assertSame('Router - Traffic', $thold['name_cache']); @@ -52,7 +52,7 @@ public function testEmptyCacheIsFilledFromTheDataSourceDescription(): void { * @return void */ public function testNameIsKeptWhenTheDataSourceHasNoDescription(): void { - $thold = array('name' => 'Manual name', 'name_cache' => '', 'local_data_id' => 4); + $thold = ['name' => 'Manual name', 'name_cache' => '', 'local_data_id' => 4]; $this->assertSame('Manual name', thold_get_cached_name($thold)); } diff --git a/tests/Unit/TholdReplaceThresholdTagsTest.php b/tests/Unit/TholdReplaceThresholdTagsTest.php index 194613ca..a861f381 100644 --- a/tests/Unit/TholdReplaceThresholdTagsTest.php +++ b/tests/Unit/TholdReplaceThresholdTagsTest.php @@ -29,15 +29,15 @@ final class TholdReplaceThresholdTagsTest extends TestCase { public static function setUpBeforeClass(): void { self::loadPluginSource('thold_functions.php'); - /* Defines $thold_types, which the substitution reads. */ + // Defines $thold_types, which the substitution reads. self::loadPluginSource('includes/arrays.php'); } /** * @return array */ - private function threshold(array $overrides = array()) { - return $overrides + array( + private function threshold(array $overrides = []) { + return $overrides + [ 'id' => 1, 'name_cache' => 'CPU', 'notes' => '', @@ -52,19 +52,19 @@ private function threshold(array $overrides = array()) { 'time_fail_trigger' => 2, 'time_fail_length' => 300, 'local_data_id' => 4, - ); + ]; } /** * @return array */ - private function device(array $overrides = array()) { - return $overrides + array( + private function device(array $overrides = []) { + return $overrides + [ 'description' => 'router1', 'hostname' => '10.0.0.1', 'location' => 'rack 4', 'site_id' => 1, - ); + ]; } /** @@ -86,15 +86,15 @@ private function substitute($text, array $thold, array $device, $shell, $current * @return array */ public static function deviceDerivedTagProvider() { - return array( - 'description' => array('', 'description', 'device'), - 'hostname' => array('', 'hostname', 'device'), - 'location' => array('', 'location', 'device'), - 'notes' => array('', 'notes', 'threshold'), - 'device note' => array('', 'dnotes', 'threshold'), - 'external id' => array('', 'external_id', 'threshold'), - 'name' => array('', 'name_cache', 'threshold'), - ); + return [ + 'description' => ['', 'description', 'device'], + 'hostname' => ['', 'hostname', 'device'], + 'location' => ['', 'location', 'device'], + 'notes' => ['', 'notes', 'threshold'], + 'device note' => ['', 'dnotes', 'threshold'], + 'external id' => ['', 'external_id', 'threshold'], + 'name' => ['', 'name_cache', 'threshold'], + ]; } /** @@ -108,13 +108,13 @@ public static function deviceDerivedTagProvider() { */ public function testShellModeQuotesEveryDeviceDerivedTag($tag, $column, $source): void { $payload = '; touch /tmp/pwned'; - $thold = $this->threshold($source === 'threshold' ? array($column => $payload) : array()); - $device = $this->device($source === 'device' ? array($column => $payload) : array()); + $thold = $this->threshold($source === 'threshold' ? [$column => $payload] : []); + $device = $this->device($source === 'device' ? [$column => $payload] : []); $result = $this->substitute("/usr/bin/alert $tag", $thold, $device, true); $this->assertStringContainsString(escapeshellarg($payload), $result); - $this->assertStringNotContainsString("alert ; touch", $result); + $this->assertStringNotContainsString('alert ; touch', $result); } /** @@ -127,8 +127,8 @@ public function testShellModeQuotesEveryDeviceDerivedTag($tag, $column, $source) * @return void */ public function testEmailModeLeavesDeviceDerivedTagsUnquoted($tag, $column, $source): void { - $thold = $this->threshold($source === 'threshold' ? array($column => "O'Brien") : array()); - $device = $this->device($source === 'device' ? array($column => "O'Brien") : array()); + $thold = $this->threshold($source === 'threshold' ? [$column => "O'Brien"] : []); + $device = $this->device($source === 'device' ? [$column => "O'Brien"] : []); $result = $this->substitute("Alert on $tag", $thold, $device, false); @@ -187,7 +187,7 @@ public function testEmailModeLeavesTheCurrentValueUnquoted(): void { * @return void */ public function testGraphAndThresholdIdentifiersAreSubstituted(): void { - $result = $this->substitute('/', $this->threshold(array('id' => 5)), $this->device(), false); + $result = $this->substitute('/', $this->threshold(['id' => 5]), $this->device(), false); $this->assertSame('7/5', $result); } @@ -208,7 +208,7 @@ public function testStaticThresholdBoundsAreSubstituted(): void { * @return void */ public function testTimeBasedThresholdSubstitutesTheTimeBounds(): void { - $result = $this->substitute('[][][]', $this->threshold(array('thold_type' => 2)), $this->device(), false); + $result = $this->substitute('[][][]', $this->threshold(['thold_type' => 2]), $this->device(), false); $this->assertSame('[80][20][2]', $result); } @@ -220,7 +220,7 @@ public function testTimeBasedThresholdSubstitutesTheTimeBounds(): void { * @return void */ public function testBaselineThresholdClearsTheBoundTags(): void { - $result = $this->substitute('[][][][]', $this->threshold(array('thold_type' => 1)), $this->device(), false); + $result = $this->substitute('[][][][]', $this->threshold(['thold_type' => 1]), $this->device(), false); $this->assertSame('[][][][]', $result); } @@ -258,7 +258,7 @@ public function testThresholdTypeNameIsSubstituted(): void { * @return void */ public function testUnknownThresholdTypeLeavesTheTagInPlace(): void { - $result = $this->substitute('', $this->threshold(array('thold_type' => 99)), $this->device(), false); + $result = $this->substitute('', $this->threshold(['thold_type' => 99]), $this->device(), false); $this->assertSame('', $result); } diff --git a/tests/Unit/TholdSetEnvironTest.php b/tests/Unit/TholdSetEnvironTest.php index d6e0a502..2a656225 100644 --- a/tests/Unit/TholdSetEnvironTest.php +++ b/tests/Unit/TholdSetEnvironTest.php @@ -43,8 +43,8 @@ protected function setUp(): void { /** * @return array */ - private function threshold(array $overrides = array()) { - return $overrides + array( + private function threshold(array $overrides = []) { + return $overrides + [ 'id' => 3, 'local_data_id' => 4, 'local_graph_id' => 7, @@ -60,14 +60,14 @@ private function threshold(array $overrides = array()) { 'time_low' => 20, 'time_fail_trigger' => 2, 'time_fail_length' => 300, - ); + ]; } /** * @return array */ - private function device(array $overrides = array()) { - return $overrides + array( + private function device(array $overrides = []) { + return $overrides + [ 'description' => 'router1', 'hostname' => '10.0.0.1', 'location' => 'rack 4', @@ -76,7 +76,7 @@ private function device(array $overrides = array()) { 'status_fail_date' => '2026-01-01 00:00:00', 'status_rec_date' => '2026-01-02 00:00:00', 'status_last_error' => '', - ); + ]; } /** @@ -89,10 +89,10 @@ private function device(array $overrides = array()) { */ private function environment(array $thold, array $device) { $pairs = thold_set_environ('', $thold, $device, 42, 7, 'traffic_in'); - $map = array(); + $map = []; foreach ($pairs as $pair) { - list($name, $value) = explode('=', $pair, 2); + [$name, $value] = explode('=', $pair, 2); $map[$name] = $value; } @@ -122,7 +122,7 @@ public function testThresholdAndDeviceContextIsExported(): void { */ public function testEachCallStartsFromAnEmptyEnvironment(): void { $this->environment($this->threshold(), $this->device()); - $env = $this->environment($this->threshold(array('id' => 9)), $this->device()); + $env = $this->environment($this->threshold(['id' => 9]), $this->device()); $this->assertSame('9', $env['THOLD_ID']); $this->assertCount(1, array_keys(array_filter(array_keys($env), function ($name) { @@ -146,7 +146,7 @@ public function testStaticThresholdExportsItsBoundsAndNoDuration(): void { * @return void */ public function testTimeBasedThresholdExportsTheTimeBoundsAndADuration(): void { - $env = $this->environment($this->threshold(array('thold_type' => 2)), $this->device()); + $env = $this->environment($this->threshold(['thold_type' => 2]), $this->device()); $this->assertSame('80', $env['THOLD_HI']); $this->assertSame('20', $env['THOLD_LOW']); @@ -158,7 +158,7 @@ public function testTimeBasedThresholdExportsTheTimeBoundsAndADuration(): void { * @return void */ public function testBaselineThresholdExportsEmptyBounds(): void { - $env = $this->environment($this->threshold(array('thold_type' => 1)), $this->device()); + $env = $this->environment($this->threshold(['thold_type' => 1]), $this->device()); $this->assertSame('', $env['THOLD_HI']); $this->assertSame('', $env['THOLD_LOW']); @@ -170,7 +170,7 @@ public function testBaselineThresholdExportsEmptyBounds(): void { * @return void */ public function testNotesAreTagExpandedWhenPresent(): void { - $env = $this->environment($this->threshold(array('notes' => 'see ')), $this->device()); + $env = $this->environment($this->threshold(['notes' => 'see ']), $this->device()); $this->assertSame('see 10.0.0.1', $env['THOLD_NOTES']); } @@ -191,7 +191,7 @@ public function testExternalIdIsExportedOnlyWhenSet(): void { $env = $this->environment($this->threshold(), $this->device()); $this->assertArrayNotHasKey('THOLD_EXTERNAL_ID', $env); - $env = $this->environment($this->threshold(array('external_id' => 'INC-42')), $this->device()); + $env = $this->environment($this->threshold(['external_id' => 'INC-42']), $this->device()); $this->assertSame('INC-42', $env['THOLD_EXTERNAL_ID']); } @@ -199,7 +199,7 @@ public function testExternalIdIsExportedOnlyWhenSet(): void { * @return void */ public function testUnknownThresholdTypeExportsAnEmptyTypeName(): void { - $env = $this->environment($this->threshold(array('thold_type' => 99)), $this->device()); + $env = $this->environment($this->threshold(['thold_type' => 99]), $this->device()); $this->assertSame('', $env['THOLD_THOLDTYPE']); } diff --git a/tests/bootstrap.php b/tests/bootstrap.php index b12282bd..05d29858 100644 --- a/tests/bootstrap.php +++ b/tests/bootstrap.php @@ -33,17 +33,17 @@ * base_path has to point at the Cacti root two levels above this plugin: * thold_functions.php builds include paths from it at runtime. */ -$GLOBALS['config'] = array( - 'base_path' => dirname(dirname(dirname(__DIR__))), - 'url_path' => '/cacti/', - 'cacti_version' => '1.2.31', +$GLOBALS['config'] = [ + 'base_path' => dirname(dirname(dirname(__DIR__))), + 'url_path' => '/cacti/', + 'cacti_version' => '1.2.31', 'cacti_server_os' => 'unix', -); +]; -/* thold_expand_string() include_once()s library_path/variables.php at call time. */ +// thold_expand_string() include_once()s library_path/variables.php at call time. $GLOBALS['config']['library_path'] = __DIR__ . '/fixtures/cacti-lib'; -/* thold reads and writes this on every RPN evaluation. */ +// thold reads and writes this on every RPN evaluation. $GLOBALS['rpn_error'] = false; if (!function_exists('db_execute')) { @@ -55,7 +55,7 @@ function db_execute($sql, $log = true, $db_conn = false) { } if (!function_exists('db_execute_prepared')) { - function db_execute_prepared($sql, $params = array(), $log = true, $db_conn = false) { + function db_execute_prepared($sql, $params = [], $log = true, $db_conn = false) { CactiStub::record('db_execute_prepared', $sql, $params); return CactiStub::nextReturn('db_execute_prepared', true); @@ -66,15 +66,15 @@ function db_execute_prepared($sql, $params = array(), $log = true, $db_conn = fa function db_fetch_assoc($sql, $log = true, $db_conn = false) { CactiStub::record('db_fetch_assoc', $sql); - return CactiStub::nextReturn('db_fetch_assoc', array()); + return CactiStub::nextReturn('db_fetch_assoc', []); } } if (!function_exists('db_fetch_assoc_prepared')) { - function db_fetch_assoc_prepared($sql, $params = array(), $log = true, $db_conn = false) { + function db_fetch_assoc_prepared($sql, $params = [], $log = true, $db_conn = false) { CactiStub::record('db_fetch_assoc_prepared', $sql, $params); - return CactiStub::nextReturn('db_fetch_assoc_prepared', array()); + return CactiStub::nextReturn('db_fetch_assoc_prepared', []); } } @@ -82,15 +82,15 @@ function db_fetch_assoc_prepared($sql, $params = array(), $log = true, $db_conn function db_fetch_row($sql, $log = true, $db_conn = false) { CactiStub::record('db_fetch_row', $sql); - return CactiStub::nextReturn('db_fetch_row', array()); + return CactiStub::nextReturn('db_fetch_row', []); } } if (!function_exists('db_fetch_row_prepared')) { - function db_fetch_row_prepared($sql, $params = array(), $log = true, $db_conn = false) { + function db_fetch_row_prepared($sql, $params = [], $log = true, $db_conn = false) { CactiStub::record('db_fetch_row_prepared', $sql, $params); - return CactiStub::nextReturn('db_fetch_row_prepared', array()); + return CactiStub::nextReturn('db_fetch_row_prepared', []); } } @@ -103,7 +103,7 @@ function db_fetch_cell($sql, $col_name = '', $log = true, $db_conn = false) { } if (!function_exists('db_fetch_cell_prepared')) { - function db_fetch_cell_prepared($sql, $params = array(), $col_name = '', $log = true, $db_conn = false) { + function db_fetch_cell_prepared($sql, $params = [], $col_name = '', $log = true, $db_conn = false) { CactiStub::record('db_fetch_cell_prepared', $sql, $params); return CactiStub::nextReturn('db_fetch_cell_prepared', ''); @@ -156,7 +156,7 @@ function sanitize_unserialize_selected_items($items) { return false; } - $data = unserialize($items, array('allowed_classes' => false)); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirroring Cacti core; allowed_classes:false blocks object injection + $data = unserialize($items, ['allowed_classes' => false]); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirroring Cacti core; allowed_classes:false blocks object injection if (!is_array($data)) { return false; @@ -188,8 +188,8 @@ function set_config_option($name, $value) { function __($text) { $args = array_slice(func_get_args(), 1); - /* Cacti's __() accepts sprintf arguments after the format string. */ - return $args === array() ? $text : vsprintf($text, $args); + // Cacti's __() accepts sprintf arguments after the format string. + return $args === [] ? $text : vsprintf($text, $args); } } @@ -230,7 +230,7 @@ function get_nfilter_request_var($name, $default = '') { } if (!function_exists('get_filter_request_var')) { - function get_filter_request_var($name, $filter = FILTER_VALIDATE_INT, $options = array()) { + function get_filter_request_var($name, $filter = FILTER_VALIDATE_INT, $options = []) { return get_request_var($name); } } @@ -261,7 +261,7 @@ function get_simple_graph_perms($user_id) { if (!function_exists('get_policies')) { function get_policies($user_id) { - return CactiStub::nextReturn('get_policies', array()); + return CactiStub::nextReturn('get_policies', []); } } @@ -309,8 +309,8 @@ function number_format_i18n($number, $decimals = 0, $baseu = 1000) { define('FILTER_VALIDATE_IS_REGEX', 99999); } -/* Device states, from Cacti include/global_constants.php. */ -foreach (array('HOST_UNKNOWN' => 0, 'HOST_DOWN' => 1, 'HOST_RECOVERING' => 2, 'HOST_UP' => 3, 'HOST_ERROR' => 4) as $name => $value) { +// Device states, from Cacti include/global_constants.php. +foreach (['HOST_UNKNOWN' => 0, 'HOST_DOWN' => 1, 'HOST_RECOVERING' => 2, 'HOST_UP' => 3, 'HOST_ERROR' => 4] as $name => $value) { if (!defined($name)) { define($name, $value); } diff --git a/thold_functions.php b/thold_functions.php index eccc26c6..0ca62a80 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -381,7 +381,7 @@ function thold_expression_math_rpn($operator, &$stack) { cacti_log('ERROR: RPN value: v2 "' . $v2 . '" is Not valid for operator "' . $operator . '". Stack:"' . implode(',', $orig_stack) . '"', false, 'THOLD'); $rpn_error = true; } elseif ($v1 == 0 && $v2 == 0 && $operator == '/') { - /* A counter that has not moved divides to zero rather than erroring. */ + // A counter that has not moved divides to zero rather than erroring. $v3 = 0; $rpn_evaled = true; } elseif ($v1 == 0 && ($operator == '/' || $operator == '%')) { @@ -4124,7 +4124,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $queue = read_config_option('thold_notification_queue'); if ($breach_up && $thold_data['trigger_cmd_high'] != '') { - /* Expand before the tags, so quoting is applied to the final text. */ + // Expand before the tags, so quoting is applied to the final text. $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_high']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4146,7 +4146,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $command_executed = true; } elseif ($breach_down && $thold_data['trigger_cmd_low'] != '') { - /* Expand before the tags, so quoting is applied to the final text. */ + // Expand before the tags, so quoting is applied to the final text. $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_low']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4168,7 +4168,7 @@ function thold_command_execution(&$thold_data, &$h, $breach_up, $breach_down, $b $command_executed = true; } elseif ($breach_norm && $thold_data['trigger_cmd_norm'] != '') { - /* Expand before the tags, so quoting is applied to the final text. */ + // Expand before the tags, so quoting is applied to the final text. $cmd = thold_expand_string($thold_data, $thold_data['trigger_cmd_norm']); $cmd = thold_replace_threshold_tags($cmd, $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name, true); @@ -4409,7 +4409,7 @@ function thold_replace_threshold_tags($text, &$thold, &$h, $currentval, $local_g $text = thold_str_replace('', date(DATE_RFC822), $text); if ($shell) { - /* An anchor in a command line would be parsed as redirections, so a trigger command gets the bare URL. */ + // An anchor in a command line would be parsed as redirections, so a trigger command gets the bare URL. $text = thold_str_replace('', $esc("$httpurl/graph.php?local_graph_id=$local_graph_id"), $text); } else { $text = thold_str_replace('', "" . __('Link to Graph in Cacti', 'thold') . '', $text); From fe41340795b9591d8b427fd716ff2968e3894f8d Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 16 Aug 2026 20:18:32 -0700 Subject: [PATCH 38/41] test: follow Cacti's test layout and composer scripts Adopts the conventions from Cacti core: tests/bootstrap-unit.php, tests/Helpers for the stubs, a phpunit.xml carrying error_reporting -1 and CACTI_TEST_BOOTSTRAP, and composer lint / test / test:coverage scripts so CI runs the same commands a developer does. The dev toolchain is Cacti's, pinned to the same platform php 8.1.0. Cacti core runs Pest and this suite does not, because pest ^2 does not resolve on PHP 8.1 -- the platform Cacti's own composer.json pins. Releases up to v2.36.0 conflict with phpunit 10.5.62 and later, every earlier 10.x release is blocked by advisory PKSA-z3gr-8qht-p93v, and v2.36.1, which does resolve, requires PHP 8.2. The stack installs on 8.2 and above; 8.1 is the floor this plugin's CI matrix targets. The tests are written in the plain PHPUnit class style that Cacti's tests/Pest.php explicitly supports, so they run unchanged under Pest wherever it is installable. Signed-off-by: Thomas Vincent --- tests/Support/CactiStub.php | 143 -------- tests/Unit/GetAllowedThresholdsTest.php | 28 +- tests/Unit/OptionalCoreFunctionTest.php | 8 +- tests/Unit/TholdCommandExecutionTest.php | 18 +- tests/Unit/TholdExpandStringTest.php | 18 +- tests/Unit/TholdExpressionMathRpnTest.php | 2 +- tests/Unit/TholdGetCachedNameTest.php | 4 +- tests/Unit/TholdReplaceThresholdTagsTest.php | 6 +- tests/Unit/TholdSetEnvironTest.php | 4 +- tests/bootstrap.php | 352 ------------------- tests/docker/Dockerfile | 5 +- 11 files changed, 47 insertions(+), 541 deletions(-) delete mode 100644 tests/Support/CactiStub.php delete mode 100644 tests/bootstrap.php diff --git a/tests/Support/CactiStub.php b/tests/Support/CactiStub.php deleted file mode 100644 index 17a5ab48..00000000 --- a/tests/Support/CactiStub.php +++ /dev/null @@ -1,143 +0,0 @@ -}> - */ - public static $calls = []; - - /** - * Queued return values, keyed by function name. Each call shifts one value - * off the front; an exhausted queue falls back to the type default. - * - * @var array> - */ - public static $returns = []; - - /** - * Values handed back by the get_*_request_var() family, keyed by var name. - * - * @var array - */ - public static $requestVars = []; - - /** - * Values handed back by read_config_option(), keyed by option name. - * - * @var array - */ - public static $configOptions = []; - - /** - * Messages passed to cacti_log(), in order. - * - * @var array - */ - public static $log = []; - - /** - * Clear all recorded and programmed state. - * - * @return void - */ - public static function reset() { - self::$calls = []; - self::$returns = []; - self::$requestVars = []; - self::$configOptions = []; - self::$log = []; - } - - /** - * Record one Cacti function call. - * - * @param string $fn Cacti function name. - * @param string $sql SQL text, or '' for non-query calls. - * @param array $params Bound parameters, if any. - * - * @return void - */ - public static function record($fn, $sql = '', array $params = []) { - self::$calls[] = ['fn' => $fn, 'sql' => $sql, 'params' => $params]; - } - - /** - * Queue one return value for the next call to $fn. - * - * @param string $fn Cacti function name. - * @param mixed $value Value to hand back. - * - * @return void - */ - public static function willReturn($fn, $value) { - self::$returns[$fn][] = $value; - } - - /** - * Take the next queued return value for $fn, or $default when none is left. - * - * @param string $fn Cacti function name. - * @param mixed $default Fallback when the queue is empty. - * - * @return mixed - */ - public static function nextReturn($fn, $default) { - if (!empty(self::$returns[$fn])) { - return array_shift(self::$returns[$fn]); - } - - return $default; - } - - /** - * All recorded calls to $fn. - * - * @param string $fn Cacti function name. - * - * @return array}> - */ - public static function callsTo($fn) { - return array_values(array_filter(self::$calls, function ($call) use ($fn) { - return $call['fn'] === $fn; - })); - } - - /** - * The recorded call log reduced to function names, in order. Useful for - * asserting transaction sequencing. - * - * @return array - */ - public static function callSequence() { - return array_column(self::$calls, 'fn'); - } -} diff --git a/tests/Unit/GetAllowedThresholdsTest.php b/tests/Unit/GetAllowedThresholdsTest.php index 9261f90d..f62796ab 100644 --- a/tests/Unit/GetAllowedThresholdsTest.php +++ b/tests/Unit/GetAllowedThresholdsTest.php @@ -51,7 +51,7 @@ public function testGraphIdIsBoundRatherThanInterpolated($function): void { $total = 0; $function('', 'td.name', '', $total, -1, 42); - $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; $this->assertStringContainsString('gl.id = ?', $call['sql']); $this->assertStringNotContainsString('42', $call['sql']); @@ -73,7 +73,7 @@ public function testMaliciousGraphIdNeverReachesQueryText($function): void { $total = 0; $function('', 'td.name', '', $total, -1, $payload); - foreach (CactiStub::$calls as $call) { + foreach (CactiStubs::$calls as $call) { $this->assertStringNotContainsString('UNION', $call['sql']); } } @@ -93,7 +93,7 @@ public function testCallerParametersAreBoundBeforeTheGraphIdParameter($function) $total = 0; $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); - $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; $this->assertSame([3, 7], $call['params']); $this->assertStringContainsString('td.thold_type = ? AND gl.id = ?', $call['sql']); @@ -110,7 +110,7 @@ public function testNoWhereClauseIsEmittedWhenNothingFiltersTheQuery($function): $total = 0; $function('', 'td.name', '', $total, -1, 0); - $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; $this->assertStringNotContainsString('WHERE', $call['sql']); $this->assertSame([], $call['params']); @@ -130,7 +130,7 @@ public function testRowCountQueryBindsTheSameParameters($function): void { $total = 0; $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); - $count = CactiStub::callsTo('db_fetch_cell_prepared')[0]; + $count = CactiStubs::callsTo('db_fetch_cell_prepared')[0]; $this->assertSame([3, 7], $count['params']); } @@ -146,7 +146,7 @@ public function testOrderByAndLimitAreAppliedToTheQuery($function): void { $total = 0; $function('', 'td.id DESC', '0,30', $total, -1, 0); - $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; $this->assertStringContainsString('ORDER BY td.id DESC', $call['sql']); $this->assertStringContainsString('LIMIT 0,30', $call['sql']); @@ -160,7 +160,7 @@ public function testOrderByAndLimitAreAppliedToTheQuery($function): void { * @return void */ public function testResultRowsAreReturnedToTheCaller($function): void { - CactiStub::willReturn('db_fetch_assoc_prepared', [['id' => 5]]); + CactiStubs::willReturn('db_fetch_assoc_prepared', [['id' => 5]]); $total = 0; $rows = $function('', 'td.name', '', $total, -1, 0); @@ -176,7 +176,7 @@ public function testResultRowsAreReturnedToTheCaller($function): void { * @return void */ public function testTotalRowsIsSetByReference($function): void { - CactiStub::willReturn('db_fetch_cell_prepared', 17); + CactiStubs::willReturn('db_fetch_cell_prepared', 17); $total = 0; $function('', 'td.name', '', $total, -1, 3); @@ -195,14 +195,14 @@ public function testTotalRowsIsSetByReference($function): void { * @return void */ public function testNoQueryRunsWhenAuthenticationIsOnAndNoUserIsResolved($function): void { - CactiStub::$configOptions['auth_method'] = 1; + CactiStubs::$configOptions['auth_method'] = 1; unset($_SESSION['sess_user_id']); $total = 0; $rows = $function('', 'td.name', '', $total, 0, 0); $this->assertSame([], $rows); - $this->assertSame([], CactiStub::callsTo('db_fetch_assoc_prepared')); + $this->assertSame([], CactiStubs::callsTo('db_fetch_assoc_prepared')); } /** @@ -213,9 +213,9 @@ public function testNoQueryRunsWhenAuthenticationIsOnAndNoUserIsResolved($functi * @return void */ public function testPolicyWhereIsAppliedWhenPermissionsAreNotSimple($function): void { - CactiStub::$configOptions['auth_method'] = 1; - CactiStub::willReturn('get_simple_graph_perms', false); - CactiStub::willReturn('get_policy_where', 'WHERE policy_applied = 1'); + CactiStubs::$configOptions['auth_method'] = 1; + CactiStubs::willReturn('get_simple_graph_perms', false); + CactiStubs::willReturn('get_policy_where', 'WHERE policy_applied = 1'); $_SESSION['sess_user_id'] = 9; $total = 0; @@ -223,7 +223,7 @@ public function testPolicyWhereIsAppliedWhenPermissionsAreNotSimple($function): unset($_SESSION['sess_user_id']); - $call = CactiStub::callsTo('db_fetch_assoc_prepared')[0]; + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; $this->assertStringContainsString('policy_applied = 1', $call['sql']); } diff --git a/tests/Unit/OptionalCoreFunctionTest.php b/tests/Unit/OptionalCoreFunctionTest.php index 1400ad69..4c460604 100644 --- a/tests/Unit/OptionalCoreFunctionTest.php +++ b/tests/Unit/OptionalCoreFunctionTest.php @@ -77,13 +77,13 @@ public static function accessorProvider() { * @return void */ public function testRowCountUsesTheCoreCacheWhenNotFilteredByGraph($function, $class): void { - CactiStub::willReturn('get_total_row_data', 12); + CactiStubs::willReturn('get_total_row_data', 12); $total = 0; $function('', 'td.name', '', $total, -1, 0); $this->assertSame(12, $total); - $this->assertSame([], CactiStub::callsTo('db_fetch_cell_prepared')); + $this->assertSame([], CactiStubs::callsTo('db_fetch_cell_prepared')); } /** @@ -98,7 +98,7 @@ public function testRowCountBypassesTheCacheForASingleGraph($function, $class): $total = 0; $function('', 'td.name', '', $total, -1, 5); - $this->assertSame([], CactiStub::callsTo('get_total_row_data')); - $this->assertNotEmpty(CactiStub::callsTo('db_fetch_cell_prepared')); + $this->assertSame([], CactiStubs::callsTo('get_total_row_data')); + $this->assertNotEmpty(CactiStubs::callsTo('db_fetch_cell_prepared')); } } diff --git a/tests/Unit/TholdCommandExecutionTest.php b/tests/Unit/TholdCommandExecutionTest.php index 643bfca5..d0f3e648 100644 --- a/tests/Unit/TholdCommandExecutionTest.php +++ b/tests/Unit/TholdCommandExecutionTest.php @@ -35,9 +35,9 @@ public static function setUpBeforeClass(): void { protected function setUp(): void { parent::setUp(); - CactiStub::$configOptions['thold_enable_scripts'] = 'on'; - CactiStub::$configOptions['thold_notification_queue'] = 'on'; - CactiStub::$configOptions['base_url'] = 'http://cacti.example.org'; + CactiStubs::$configOptions['thold_enable_scripts'] = 'on'; + CactiStubs::$configOptions['thold_notification_queue'] = 'on'; + CactiStubs::$configOptions['base_url'] = 'http://cacti.example.org'; } /** @@ -89,7 +89,7 @@ private function device(array $overrides = []) { * @return string|null */ private function queuedCommand() { - foreach (CactiStub::callsTo('db_execute_prepared') as $call) { + foreach (CactiStubs::callsTo('db_execute_prepared') as $call) { foreach ($call['params'] as $param) { if (is_string($param) && strpos($param, '"command"') !== false) { $decoded = json_decode($param, true); @@ -151,7 +151,7 @@ public function testShellMetacharactersInDeviceDataAreQuoted($column, array $bre * @return void */ public function testNothingRunsWhenScriptsAreDisabled(): void { - CactiStub::$configOptions['thold_enable_scripts'] = ''; + CactiStubs::$configOptions['thold_enable_scripts'] = ''; $thold = $this->threshold(['trigger_cmd_high' => '/usr/bin/alert']); $device = $this->device(); @@ -203,7 +203,7 @@ public function testHighBreachTakesPrecedenceOverLow(): void { * @return void */ public function testInlineExecutionLogsTheCommandOutput($column, array $breaches): void { - CactiStub::$configOptions['thold_notification_queue'] = ''; + CactiStubs::$configOptions['thold_notification_queue'] = ''; $thold = $this->threshold([$column => '/bin/echo breach']); $device = $this->device(); @@ -211,7 +211,7 @@ public function testInlineExecutionLogsTheCommandOutput($column, array $breaches thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); $this->assertNull($this->queuedCommand()); - $this->assertNotEmpty(CactiStub::$log); + $this->assertNotEmpty(CactiStubs::$log); } /** @@ -238,13 +238,13 @@ public static function inlineOutcomeProvider() { * @return void */ public function testInlineExecutionLogsTheExitStatus($command, $level): void { - CactiStub::$configOptions['thold_notification_queue'] = ''; + CactiStubs::$configOptions['thold_notification_queue'] = ''; $thold = $this->threshold(['trigger_cmd_high' => $command]); $device = $this->device(); thold_command_execution($thold, $device, true, false, false); - $this->assertStringStartsWith($level, CactiStub::$log[0]); + $this->assertStringStartsWith($level, CactiStubs::$log[0]); } } diff --git a/tests/Unit/TholdExpandStringTest.php b/tests/Unit/TholdExpandStringTest.php index ca99a120..8f1415d8 100644 --- a/tests/Unit/TholdExpandStringTest.php +++ b/tests/Unit/TholdExpandStringTest.php @@ -45,7 +45,7 @@ private function thresholdData(array $overrides = []) { * @return void */ private function graphExists() { - CactiStub::willReturn('db_fetch_row_prepared', [ + CactiStubs::willReturn('db_fetch_row_prepared', [ 'id' => 7, 'host_id' => 2, 'snmp_query_id' => 3, @@ -76,7 +76,7 @@ public function testDataSourceNameTokenIsResolved(): void { */ public function testDataSourceDescriptionTokenIsResolvedFromTheDatabase(): void { $this->graphExists(); - CactiStub::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + CactiStubs::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); $this->assertSame('Router - Traffic', thold_expand_string($this->thresholdData(), '|data_source_description|')); } @@ -86,10 +86,10 @@ public function testDataSourceDescriptionTokenIsResolvedFromTheDatabase(): void */ public function testTextIsPassedThroughExpandTitleForDataQueryTokens(): void { $this->graphExists(); - CactiStub::willReturn('expand_title', 'alert eth0'); + CactiStubs::willReturn('expand_title', 'alert eth0'); $this->assertSame('alert eth0', thold_expand_string($this->thresholdData(), 'alert |query_ifName|')); - $this->assertNotEmpty(CactiStub::callsTo('expand_title')); + $this->assertNotEmpty(CactiStubs::callsTo('expand_title')); } /** @@ -97,8 +97,8 @@ public function testTextIsPassedThroughExpandTitleForDataQueryTokens(): void { */ public function testInterfaceSpeedFallsBackToTheConfiguredDefaultWhenUnknown(): void { $this->graphExists(); - CactiStub::$configOptions['thold_empty_if_speed_default'] = '1000000000'; - CactiStub::willReturn('db_fetch_cell_prepared', ''); + CactiStubs::$configOptions['thold_empty_if_speed_default'] = '1000000000'; + CactiStubs::willReturn('db_fetch_cell_prepared', ''); $result = thold_expand_string($this->thresholdData(), '|query_ifHighSpeed|'); @@ -109,7 +109,7 @@ public function testInterfaceSpeedFallsBackToTheConfiguredDefaultWhenUnknown(): * @return void */ public function testTextIsReturnedUnchangedWhenTheGraphIsMissing(): void { - CactiStub::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', []); $this->assertSame('static text', thold_expand_string($this->thresholdData(), 'static text')); } @@ -122,7 +122,7 @@ public function testTextIsReturnedUnchangedWhenTheGraphIsMissing(): void { */ public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): void { $this->graphExists(); - CactiStub::willReturn('db_fetch_cell_prepared', 'Suggested |data_source_name|'); + CactiStubs::willReturn('db_fetch_cell_prepared', 'Suggested |data_source_name|'); $result = thold_expand_string($this->thresholdData(['thold_template_id' => 5]), ''); @@ -133,7 +133,7 @@ public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): vo * @return void */ public function testSurroundingWhitespaceIsTrimmed(): void { - CactiStub::willReturn('db_fetch_row_prepared', []); + CactiStubs::willReturn('db_fetch_row_prepared', []); $this->assertSame('alert', thold_expand_string($this->thresholdData(), ' alert ')); } diff --git a/tests/Unit/TholdExpressionMathRpnTest.php b/tests/Unit/TholdExpressionMathRpnTest.php index b429fad1..6c9fdbf1 100644 --- a/tests/Unit/TholdExpressionMathRpnTest.php +++ b/tests/Unit/TholdExpressionMathRpnTest.php @@ -151,7 +151,7 @@ public static function nonNumericOperandProvider() { public function testNonNumericOperandsFlagErrorAndPushNothing(array $stack, $operator): void { $this->assertSame([], $this->evaluate($stack, $operator)); $this->assertTrue($GLOBALS['rpn_error']); - $this->assertNotEmpty(CactiStub::$log); + $this->assertNotEmpty(CactiStubs::$log); } /** diff --git a/tests/Unit/TholdGetCachedNameTest.php b/tests/Unit/TholdGetCachedNameTest.php index 65da52d5..a33d83ce 100644 --- a/tests/Unit/TholdGetCachedNameTest.php +++ b/tests/Unit/TholdGetCachedNameTest.php @@ -33,14 +33,14 @@ public function testCachedNameIsReturnedWithoutQueryingTheDatabase(): void { $thold = ['name' => '|data_source_description|', 'name_cache' => 'CPU load', 'local_data_id' => 4]; $this->assertSame('CPU load', thold_get_cached_name($thold)); - $this->assertSame([], CactiStub::callsTo('db_fetch_cell_prepared')); + $this->assertSame([], CactiStubs::callsTo('db_fetch_cell_prepared')); } /** * @return void */ public function testEmptyCacheIsFilledFromTheDataSourceDescription(): void { - CactiStub::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + CactiStubs::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); $thold = ['name' => '|data_source_description|', 'name_cache' => '', 'local_data_id' => 4]; diff --git a/tests/Unit/TholdReplaceThresholdTagsTest.php b/tests/Unit/TholdReplaceThresholdTagsTest.php index a861f381..96006c94 100644 --- a/tests/Unit/TholdReplaceThresholdTagsTest.php +++ b/tests/Unit/TholdReplaceThresholdTagsTest.php @@ -143,7 +143,7 @@ public function testEmailModeLeavesDeviceDerivedTagsUnquoted($tag, $column, $sou * @return void */ public function testShellModeQuotesTheSiteName(): void { - CactiStub::willReturn('db_fetch_cell_prepared', '$(id)'); + CactiStubs::willReturn('db_fetch_cell_prepared', '$(id)'); $result = $this->substitute('/usr/bin/alert ', $this->threshold(), $this->device(), true); @@ -154,7 +154,7 @@ public function testShellModeQuotesTheSiteName(): void { * @return void */ public function testSiteFallsBackToDefaultWhenTheDeviceHasNoSite(): void { - CactiStub::willReturn('db_fetch_cell_prepared', ''); + CactiStubs::willReturn('db_fetch_cell_prepared', ''); $result = $this->substitute('site=', $this->threshold(), $this->device(), false); @@ -238,7 +238,7 @@ public function testStaticThresholdHasNoDuration(): void { * @return void */ public function testUrlTagRendersALinkToTheGraph(): void { - CactiStub::$configOptions['base_url'] = 'http://cacti.example.org'; + CactiStubs::$configOptions['base_url'] = 'http://cacti.example.org'; $result = $this->substitute('', $this->threshold(), $this->device(), false); diff --git a/tests/Unit/TholdSetEnvironTest.php b/tests/Unit/TholdSetEnvironTest.php index 2a656225..02ac28e2 100644 --- a/tests/Unit/TholdSetEnvironTest.php +++ b/tests/Unit/TholdSetEnvironTest.php @@ -36,8 +36,8 @@ public static function setUpBeforeClass(): void { protected function setUp(): void { parent::setUp(); - CactiStub::$configOptions['thold_notification_queue'] = 'on'; - CactiStub::$configOptions['base_url'] = 'http://cacti.example.org'; + CactiStubs::$configOptions['thold_notification_queue'] = 'on'; + CactiStubs::$configOptions['base_url'] = 'http://cacti.example.org'; } /** diff --git a/tests/bootstrap.php b/tests/bootstrap.php deleted file mode 100644 index 05d29858..00000000 --- a/tests/bootstrap.php +++ /dev/null @@ -1,352 +0,0 @@ - dirname(dirname(dirname(__DIR__))), - 'url_path' => '/cacti/', - 'cacti_version' => '1.2.31', - 'cacti_server_os' => 'unix', -]; - -// thold_expand_string() include_once()s library_path/variables.php at call time. -$GLOBALS['config']['library_path'] = __DIR__ . '/fixtures/cacti-lib'; - -// thold reads and writes this on every RPN evaluation. -$GLOBALS['rpn_error'] = false; - -if (!function_exists('db_execute')) { - function db_execute($sql, $log = true, $db_conn = false) { - CactiStub::record('db_execute', $sql); - - return CactiStub::nextReturn('db_execute', true); - } -} - -if (!function_exists('db_execute_prepared')) { - function db_execute_prepared($sql, $params = [], $log = true, $db_conn = false) { - CactiStub::record('db_execute_prepared', $sql, $params); - - return CactiStub::nextReturn('db_execute_prepared', true); - } -} - -if (!function_exists('db_fetch_assoc')) { - function db_fetch_assoc($sql, $log = true, $db_conn = false) { - CactiStub::record('db_fetch_assoc', $sql); - - return CactiStub::nextReturn('db_fetch_assoc', []); - } -} - -if (!function_exists('db_fetch_assoc_prepared')) { - function db_fetch_assoc_prepared($sql, $params = [], $log = true, $db_conn = false) { - CactiStub::record('db_fetch_assoc_prepared', $sql, $params); - - return CactiStub::nextReturn('db_fetch_assoc_prepared', []); - } -} - -if (!function_exists('db_fetch_row')) { - function db_fetch_row($sql, $log = true, $db_conn = false) { - CactiStub::record('db_fetch_row', $sql); - - return CactiStub::nextReturn('db_fetch_row', []); - } -} - -if (!function_exists('db_fetch_row_prepared')) { - function db_fetch_row_prepared($sql, $params = [], $log = true, $db_conn = false) { - CactiStub::record('db_fetch_row_prepared', $sql, $params); - - return CactiStub::nextReturn('db_fetch_row_prepared', []); - } -} - -if (!function_exists('db_fetch_cell')) { - function db_fetch_cell($sql, $col_name = '', $log = true, $db_conn = false) { - CactiStub::record('db_fetch_cell', $sql); - - return CactiStub::nextReturn('db_fetch_cell', ''); - } -} - -if (!function_exists('db_fetch_cell_prepared')) { - function db_fetch_cell_prepared($sql, $params = [], $col_name = '', $log = true, $db_conn = false) { - CactiStub::record('db_fetch_cell_prepared', $sql, $params); - - return CactiStub::nextReturn('db_fetch_cell_prepared', ''); - } -} - -if (!function_exists('db_qstr')) { - function db_qstr($string) { - return "'" . str_replace("'", "''", (string) $string) . "'"; - } -} - -if (!function_exists('db_begin_transaction')) { - function db_begin_transaction() { - CactiStub::record('db_begin_transaction'); - - return CactiStub::nextReturn('db_begin_transaction', true); - } -} - -if (!function_exists('db_commit_transaction')) { - function db_commit_transaction() { - CactiStub::record('db_commit_transaction'); - - return CactiStub::nextReturn('db_commit_transaction', true); - } -} - -if (!function_exists('db_rollback_transaction')) { - function db_rollback_transaction() { - CactiStub::record('db_rollback_transaction'); - - return CactiStub::nextReturn('db_rollback_transaction', true); - } -} - -if (!function_exists('html_escape')) { - function html_escape($string) { - return htmlspecialchars((string) $string, ENT_QUOTES, 'UTF-8'); - } -} - -/* - * Mirrors Cacti 1.2 lib/functions.php. KEEP IN SYNC: if core tightens its - * checks, tests here would otherwise keep passing while production diverges. - */ -if (!function_exists('sanitize_unserialize_selected_items')) { - function sanitize_unserialize_selected_items($items) { - if (empty($items)) { - return false; - } - - $data = unserialize($items, ['allowed_classes' => false]); // nosemgrep: php.lang.security.unserialize-use.unserialize-use -- test stub mirroring Cacti core; allowed_classes:false blocks object injection - - if (!is_array($data)) { - return false; - } - - foreach ($data as $value) { - if (!is_numeric($value)) { - return false; - } - } - - return $data; - } -} - -if (!function_exists('read_config_option')) { - function read_config_option($name, $force = false) { - return isset(CactiStub::$configOptions[$name]) ? CactiStub::$configOptions[$name] : ''; - } -} - -if (!function_exists('set_config_option')) { - function set_config_option($name, $value) { - CactiStub::$configOptions[$name] = $value; - } -} - -if (!function_exists('__')) { - function __($text) { - $args = array_slice(func_get_args(), 1); - - // Cacti's __() accepts sprintf arguments after the format string. - return $args === [] ? $text : vsprintf($text, $args); - } -} - -if (!function_exists('__esc')) { - function __esc($text) { - return htmlspecialchars(call_user_func_array('__', func_get_args()), ENT_QUOTES, 'UTF-8'); - } -} - -if (!function_exists('cacti_log')) { - function cacti_log($message, $output = false, $environ = 'CMDPHP', $level = 0) { - CactiStub::$log[] = $message; - } -} - -if (!function_exists('cacti_sizeof')) { - function cacti_sizeof($array) { - return (is_array($array) || $array instanceof Countable) ? count($array) : 0; - } -} - -if (!function_exists('cacti_count')) { - function cacti_count($array) { - return cacti_sizeof($array); - } -} - -if (!function_exists('get_request_var')) { - function get_request_var($name, $default = '') { - return isset(CactiStub::$requestVars[$name]) ? CactiStub::$requestVars[$name] : $default; - } -} - -if (!function_exists('get_nfilter_request_var')) { - function get_nfilter_request_var($name, $default = '') { - return get_request_var($name, $default); - } -} - -if (!function_exists('get_filter_request_var')) { - function get_filter_request_var($name, $filter = FILTER_VALIDATE_INT, $options = []) { - return get_request_var($name); - } -} - -if (!function_exists('isset_request_var')) { - function isset_request_var($name) { - return isset(CactiStub::$requestVars[$name]); - } -} - -if (!function_exists('cacti_escapeshellarg')) { - function cacti_escapeshellarg($string, $quote = true) { - return escapeshellarg((string) $string); - } -} - -if (!function_exists('api_plugin_hook_function')) { - function api_plugin_hook_function($name, $data = '') { - return $data; - } -} - -if (!function_exists('get_simple_graph_perms')) { - function get_simple_graph_perms($user_id) { - return CactiStub::nextReturn('get_simple_graph_perms', true); - } -} - -if (!function_exists('get_policies')) { - function get_policies($user_id) { - return CactiStub::nextReturn('get_policies', []); - } -} - -if (!function_exists('get_policy_where')) { - function get_policy_where($graph_auth_method, $policies, $sql_where) { - CactiStub::record('get_policy_where', $sql_where); - - return CactiStub::nextReturn('get_policy_where', $sql_where); - } -} - -if (!function_exists('expand_title')) { - function expand_title($host_id, $snmp_query_id, $snmp_index, $title) { - CactiStub::record('expand_title', $title); - - return CactiStub::nextReturn('expand_title', $title); - } -} - -if (!function_exists('get_graph_title')) { - function get_graph_title($local_graph_id) { - return CactiStub::nextReturn('get_graph_title', 'Traffic - eth0'); - } -} - -if (!function_exists('get_timeinstate')) { - function get_timeinstate($host) { - return CactiStub::nextReturn('get_timeinstate', '1 day'); - } -} - -if (!function_exists('get_daysfromtime')) { - function get_daysfromtime($timestamp) { - return CactiStub::nextReturn('get_daysfromtime', '1 day'); - } -} - -if (!function_exists('number_format_i18n')) { - function number_format_i18n($number, $decimals = 0, $baseu = 1000) { - return number_format((float) $number, $decimals < 0 ? 0 : (int) $decimals); - } -} - -if (!defined('FILTER_VALIDATE_IS_REGEX')) { - define('FILTER_VALIDATE_IS_REGEX', 99999); -} - -// Device states, from Cacti include/global_constants.php. -foreach (['HOST_UNKNOWN' => 0, 'HOST_DOWN' => 1, 'HOST_RECOVERING' => 2, 'HOST_UP' => 3, 'HOST_ERROR' => 4] as $name => $value) { - if (!defined($name)) { - define($name, $value); - } -} - -if (!defined('CACTI_DATE_TIME_FORMAT')) { - define('CACTI_DATE_TIME_FORMAT', 'Y-m-d H:i:s'); -} - -if (!defined('CACTI_PATH_BASE')) { - define('CACTI_PATH_BASE', $GLOBALS['config']['base_path']); -} - -/** - * Load a plugin source file at global scope. - * - * Several plugin files (includes/arrays.php in particular) define their data - * as file-scope variables that the rest of the plugin reads as globals, and - * they read $config while doing so. Requiring them from inside a method would - * make both halves of that method-local, so the require happens here and any - * variable the file introduced is published to $GLOBALS. - * - * @param string $path Absolute path to the file. - * - * @return void - */ -function thold_test_load($path) { - global $config; - - $__before = get_defined_vars(); - - require_once $path; - - foreach (get_defined_vars() as $__name => $__value) { - if (!array_key_exists($__name, $__before) && strncmp($__name, '__', 2) !== 0) { - $GLOBALS[$__name] = $__value; - } - } -} diff --git a/tests/docker/Dockerfile b/tests/docker/Dockerfile index cb4279d1..518f7321 100644 --- a/tests/docker/Dockerfile +++ b/tests/docker/Dockerfile @@ -7,7 +7,8 @@ FROM php:8.1-cli-alpine@sha256:7949370448b0b4d9787776dc5968e0fd8d48763292344b5fb # git is needed by the changed-line coverage gate, which diffs against the # base branch. -RUN apk add --no-cache git \ +RUN apk add --no-cache git gmp-dev \ + && docker-php-ext-install gmp \ && apk add --no-cache --virtual .build-deps $PHPIZE_DEPS \ && pecl install pcov \ && docker-php-ext-enable pcov \ @@ -25,4 +26,4 @@ ENV COMPOSER_ALLOW_SUPERUSER=1 \ COMPOSER_NO_INTERACTION=1 \ COMPOSER_CACHE_DIR=/tmp/composer-cache -CMD ["sh", "-c", "composer install --no-progress --no-ansi && vendor/bin/phpunit"] +CMD ["sh", "-c", "composer install --no-progress --no-ansi && composer test"] From 9fb2072795322740f4b0fd63d3e0f28bd83e9c61 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:28:51 -0700 Subject: [PATCH 39/41] test: keep optional-core coverage in Pest process order --- ...lCoreFunctionTest.php => ZOptionalCoreFunctionTest.php} | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) rename tests/Unit/{OptionalCoreFunctionTest.php => ZOptionalCoreFunctionTest.php} (94%) diff --git a/tests/Unit/OptionalCoreFunctionTest.php b/tests/Unit/ZOptionalCoreFunctionTest.php similarity index 94% rename from tests/Unit/OptionalCoreFunctionTest.php rename to tests/Unit/ZOptionalCoreFunctionTest.php index 4c460604..1b3843bd 100644 --- a/tests/Unit/OptionalCoreFunctionTest.php +++ b/tests/Unit/ZOptionalCoreFunctionTest.php @@ -18,11 +18,8 @@ * Behaviour when the running Cacti provides the functions the plugin treats as * optional. * - * These run in their own process so that defining the functions does not - * change which branch every other test takes. - * - * @runTestsInSeparateProcesses - * @preserveGlobalState disabled + * The file is sorted after the fallback-path tests so defining these optional + * functions cannot change the code paths exercised by earlier tests. */ final class OptionalCoreFunctionTest extends TestCase { /** From 8fd4be7666bd2692235e92910ccf37410de2f385 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:47:13 -0700 Subject: [PATCH 40/41] ci: keep plugin PR integration checks on pinned Cacti --- .github/workflows/plugin-ci-workflow.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index ccd89839..4b668158 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -35,21 +35,12 @@ jobs: integration-test: runs-on: ${{ matrix.os }} - # A failure against the pinned release is a real failure. The develop entry - # is advisory: it is how a core regression becomes visible here, but it must - # not turn the plugin's own pull requests red. - continue-on-error: ${{ matrix.cacti != 'release/1.2.31' }} - strategy: fail-fast: false matrix: php: ['8.1', '8.2', '8.3', '8.4'] os: [ubuntu-latest] cacti: ['release/1.2.31'] - include: - - php: '8.4' - os: ubuntu-latest - cacti: 'develop' services: mariadb: From 52a8d98249dd83f94dc7249fe6c149f1201b2f78 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 17 Aug 2026 14:59:50 -0700 Subject: [PATCH 41/41] ci: bound package index refreshes --- .github/workflows/plugin-ci-workflow.yml | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 4b668158..85181ac8 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -86,7 +86,24 @@ jobs: echo "PHP_BINARY=$(command -v php)" >> "$GITHUB_ENV" - name: Run apt-get update - run: sudo apt-get update + run: | + for attempt in 1 2 3; do + if sudo timeout 3m apt-get \ + -o Dpkg::Lock::Timeout=60 \ + -o Acquire::Retries=3 \ + -o Acquire::http::Timeout=30 \ + -o Acquire::https::Timeout=30 \ + update; then + exit 0 + fi + + if [ "$attempt" -lt 3 ]; then + sleep 10 + fi + done + + echo 'apt-get update failed after three bounded attempts.' >&2 + exit 1 - name: Install System Dependencies run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping libapache2-mod-php