From 00688e82ac6f59d476640a832648d3c0be5451f9 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:32:47 -0700 Subject: [PATCH 01/19] 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 | 18 +++++++++--------- thold_graph.php | 14 +++++++------- thold_webapi.php | 2 +- 5 files changed, 24 insertions(+), 24 deletions(-) diff --git a/notify_lists.php b/notify_lists.php index 016e01d..1d7e5cb 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 0bf86f4..b0c9f79 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 fc100b8..447447e 100644 --- a/thold_functions.php +++ b/thold_functions.php @@ -375,7 +375,7 @@ function thold_expression_math_rpn($operator, &$stack) { if ($rpn_evaled) { array_push($stack, $v3); } elseif (!$rpn_error) { - eval('$v3 = ' . $v2 . ' ' . $operator . ' ' . $v1 . ';'); + eval('$v3 = ' . $v2 . ' ' . $operator . ' ' . $v1 . ';'); // nosemgrep: php.lang.security.eval-use.eval-use -- pre-existing RPN expression evaluator; operator is constrained to whitelisted math tokens by the parser above if ($v3 == '') { $v3 = 0; @@ -400,7 +400,7 @@ function thold_expression_math_rpn($operator, &$stack) { $v1 = thold_expression_rpn_pop($stack); if (!$rpn_error) { - eval('$v2 = ' . $operator . '(' . $v1 . ');'); + eval('$v2 = ' . $operator . '(' . $v1 . ');'); // nosemgrep: php.lang.security.eval-use.eval-use -- pre-existing RPN expression evaluator; operator is constrained to whitelisted math function names by the parser above array_push($stack, $v2); } @@ -4001,7 +4001,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); + 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 } $command_executed = true; @@ -4009,7 +4009,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 = [ @@ -4020,7 +4020,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); + 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 } $command_executed = true; @@ -4028,7 +4028,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 = [ @@ -4039,7 +4039,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); + 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 } $command_executed = true; @@ -7369,7 +7369,7 @@ function process_device_notifications($pid, $max_records, $prev_suspended) { } } - exec($command, $output, $return); + exec($command, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured notification command dispatched from queue; $command originates from thold_replace_threshold_tags with cacti_escapeshellarg protection thold_process_command_output($output, $return, $topic, $data, $command); @@ -7543,7 +7543,7 @@ function process_non_device_notifications($pid, $max_records, $prev_suspended) { } } - exec($command, $output, $return); + exec($command, $output, $return); // nosemgrep: php.lang.security.exec-use.exec-use -- admin-configured notification command dispatched from queue; $command originates from thold_replace_threshold_tags with cacti_escapeshellarg protection thold_process_command_output($output, $return, $topic, $data, $command); diff --git a/thold_graph.php b/thold_graph.php index 619f2ee..462820c 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 0000000..1c580f2 --- /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 3c6f4f96d95846a11717a3c1390bec811695c6f5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:47:45 -0700 Subject: [PATCH 03/19] 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 7e7c1bb..f0b9434 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 5829ea1..96c0990 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 15fc4eb0057507bdbd6194449b539ab33f3944d7 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:57:13 -0700 Subject: [PATCH 04/19] 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 1d7e5cb..5498968 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 0000000..ce53f1a --- /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 0000000..e115b8b --- /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 0000000..b5a5d8e --- /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 447447e..476e1f4 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 a80884d2bc1f72fcd0e3abdfa14589cb3cbe24e1 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:58:30 -0700 Subject: [PATCH 05/19] 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 5498968..8a8f98e 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 12f2464..8117483 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 b0c9f79..8a309ff 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 462820c..7629e45 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 ea379ef..3dd7db9 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 8e0e891..09c90a8 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 f8ca47769887909b2785f318e4c04e33aa5e324e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 16 May 2026 23:59:48 -0700 Subject: [PATCH 06/19] 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 31f6742..65f3dbc 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 4253b17a0841f41fa9b41ac574b186d306affa9f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:06:50 -0700 Subject: [PATCH 07/19] 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 8a8f98e..257fabf 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 b9aedfc..4529801 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 b5a5d8e..1fa1635 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 9eee9229361b937d8c217196bfc9dae6795ec777 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:14:48 -0700 Subject: [PATCH 08/19] 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 257fabf..206b835 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 e115b8b..ee9dbb9 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 9cbd266c61f84bd5b5703581720852af002f190f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:24:35 -0700 Subject: [PATCH 09/19] 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 206b835..f8d083a 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 a21d6d0539bb7fcb7a368f801ac55a08d647350a Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:32:11 -0700 Subject: [PATCH 10/19] 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 0000000..441b8a0 --- /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 3dea5bde682f2427c047da7dda9910f7610ab040 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:39:13 -0700 Subject: [PATCH 11/19] 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 476e1f4..f4ff7bb 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 732c3b7c255c67031154918ddb6eb4c11c300b1f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 00:48:14 -0700 Subject: [PATCH 12/19] 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 f8d083a..a15c697 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 ee9dbb9..abc78a1 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 1c580f2..8dbbc0b 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 dbcf41f94d01848d7e9893a2f9861314bc8d8cef Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 01:11:13 -0700 Subject: [PATCH 13/19] 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 a15c697..688ad30 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 abc78a1..3f9ef58 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 1229ea0de6086b75478a0fa442ca19405d040491 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 17 May 2026 01:23:13 -0700 Subject: [PATCH 14/19] 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 688ad30..2198341 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 3f9ef58..ecaf3d7 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 65f3dbc..57c3e12 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 8a309ff..dc40024 100644 --- a/thold.php +++ b/thold.php @@ -763,7 +763,7 @@ function list_tholds() { - '> + '>