diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 73630b5..1f325b6 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -106,7 +106,7 @@ jobs: exit 1 - name: Install System Dependencies - run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping + run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping libapache2-mod-php - name: Start SNMPD Agent and Test run: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b7db7d..ae48cd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,17 @@ --- develop --- +* security: Use prepared statements for the bulk form actions in notify_lists.php and notify_queue.php +* security: Bind $graph_id in get_allowed_thresholds() and get_allowed_threshold_logs() instead of interpolating it +* security: Route rfilter through db_qstr_rlike() where Cacti provides it, and quote it otherwise +* security: Quote the values substituted into trigger commands +* security: Escape the page, id and drp_action values printed into hidden inputs +* security: Remove the eval() calls from the RPN expression evaluator +* issue: Bulk actions on the Notification Lists page did nothing, because the action allowlist compared an int against strings +* issue: Bulk writes were discarded on MySQL, where Cacti's db_commit_transaction() never commits +* issue: An RPN expression dividing zero by zero pushed no result, corrupting the rest of the stack +* issue: An RPN expression taking the modulo of zero, the square root of a negative, or the log of zero aborted the poller or produced NAN +* issue: Deleting a notification list left soft-deleted devices pointing at it * issue#686: Applying a templated threshold to a graph via the wrench icon, creates a duplicate graph * issue#707: Excessive timeout for row caching prevents data from being updated timely * issue#710: Fixing Typo in thold_daemons.service File diff --git a/notify_lists.php b/notify_lists.php index 016e01d..72c549e 100644 --- a/notify_lists.php +++ b/notify_lists.php @@ -146,7 +146,19 @@ function form_actions() { global $actions, $assoc_actions; // ================= input validation ================= + /* + * get_filter_request_var() stores the value as an int, so the comparison + * has to be made on strings for the strict check to mean anything. + */ get_filter_request_var('drp_action'); + + $valid_actions = array_map('strval', array_keys($actions + $assoc_actions)); + + if (!in_array((string) get_request_var('drp_action'), $valid_actions, true)) { + raise_message(40); + header('Location: notify_lists.php?header=false'); + exit; + } // ==================================================== // if we are to save this form, instead of display it @@ -156,41 +168,54 @@ 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')); - - db_execute('UPDATE host - SET thold_send_email = 0 - WHERE thold_send_email = 2 - AND deleted="" - AND ' . array_to_sql_or($selected_items, 'thold_host_email')); - - db_execute('UPDATE host - SET thold_send_email = 1 - WHERE thold_send_email = 3 - AND deleted="" - AND ' . array_to_sql_or($selected_items, 'thold_host_email')); - - db_execute('UPDATE host - SET thold_host_email = 0 - AND deleted="" - WHERE ' . array_to_sql_or($selected_items, 'thold_host_email')); - - db_execute('UPDATE thold_data - SET notify_warning = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_warning')); - - db_execute('UPDATE thold_data - SET notify_alert = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_alert')); - - db_execute('UPDATE thold_template - SET notify_warning = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_warning')); - - db_execute('UPDATE thold_template - SET notify_alert = 0 - WHERE ' . array_to_sql_or($selected_items, 'notify_alert')); + /* + * Bind positionally on the values: sanitize_unserialize_selected_items() + * preserves the submitted array's keys, and a string key would be read + * as a named parameter. + */ + $ids = array_map('intval', array_values($selected_items)); + $placeholders = implode(', ', array_fill(0, cacti_sizeof($ids), '?')); + + /* + * Issued as SQL rather than through db_begin_transaction() and + * friends: Cacti's db_commit_transaction() gates the commit on + * SELECT @@in_transaction, which only MariaDB defines. On MySQL that + * query fails, the commit is skipped, and every write here is + * discarded when the connection closes. + */ + db_execute('START TRANSACTION'); + + /* + * The host reset deliberately omits the deleted = "" predicate its + * siblings carry: a soft-deleted device that is later restored must + * not come back pointing at a list that no longer exists. + */ + $statements = [ + 'DELETE FROM plugin_notification_lists WHERE id IN (' . $placeholders . ')', + 'UPDATE host SET thold_send_email = 0 WHERE thold_send_email = 2 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', + 'UPDATE host SET thold_send_email = 1 WHERE thold_send_email = 3 AND deleted = "" AND thold_host_email IN (' . $placeholders . ')', + 'UPDATE host SET thold_host_email = 0 WHERE thold_host_email IN (' . $placeholders . ')', + 'UPDATE thold_data SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', + 'UPDATE thold_data SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')', + 'UPDATE thold_template SET notify_warning = 0 WHERE notify_warning IN (' . $placeholders . ')', + 'UPDATE thold_template SET notify_alert = 0 WHERE notify_alert IN (' . $placeholders . ')' + ]; + + $ok = true; + + foreach ($statements as $sql) { + if (!db_execute_prepared($sql, $ids)) { + $ok = false; + + break; + } + } + + if ($ok) { + db_execute('COMMIT'); + } else { + db_execute('ROLLBACK'); + } } elseif (get_request_var('drp_action') == '2') { // duplicate $i = 1; @@ -237,48 +262,60 @@ 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'); + + db_execute('START TRANSACTION'); + + $ok = true; 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=""'); + $ok = db_execute_prepared('UPDATE host + SET thold_host_email = ? + WHERE id = ? + AND deleted = ""', + [get_request_var('id'), $selected_items[$i]]) && $ok; // 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=""'); + $ok = db_execute_prepared('UPDATE host + SET thold_send_email = ? + WHERE id = ? + AND deleted = ""', + [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('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=' . 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]]) && $ok; // clear other items - db_execute("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=" . $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]]) && $ok; } else { // set the notification list - db_execute('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=' . 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]]) && $ok; } } @@ -286,78 +323,100 @@ 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 + $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=' . 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]]) && $ok; // clear other items - db_execute("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=" . $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]]) && $ok; // remove legacy contacts - db_execute('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 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]]) && $ok; } else { // set the notification list - db_execute('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=' . 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]]) && $ok; } } + + if (!$ok) { + break; + } } } 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=""'); + $ok = db_execute_prepared('UPDATE host + SET thold_host_email = 0 + WHERE id = ? + AND deleted = ""', + [$selected_items[$i]]) && $ok; // 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=""'); + $ok = db_execute_prepared('UPDATE host + SET thold_send_email = ? + WHERE id = ? + AND deleted = ""', + [get_request_var('notification_action'), $selected_items[$i]]) && $ok; if (get_request_var('notification_warning_action') > 0) { // set the notification list - db_execute('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=' . $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')]) && $ok; } if (get_request_var('notification_alert_action') > 0) { // set the notification list - db_execute('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=' . $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')]) && $ok; + } + + if (!$ok) { + break; } } } + + if ($ok) { + db_execute('COMMIT'); + } else { + db_execute('ROLLBACK'); + } } header('Location: notify_lists.php?header=false&action=edit&tab=hosts&id=' . get_request_var('id')); @@ -366,27 +425,38 @@ 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'); + + db_execute('START TRANSACTION'); + + $ok = true; + $update_template = []; 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]); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute("UPDATE thold_template - SET notify_warning_extra='' - WHERE id=" . $selected_items[$i]); + $ok = db_execute_prepared("UPDATE thold_template + SET notify_warning_extra = '' + WHERE id = ?", + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute('UPDATE thold_template - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -394,47 +464,74 @@ 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]); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; // 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]); + $ok = db_execute_prepared("UPDATE thold_template + SET notify_extra = '' + WHERE id = ?", + [$selected_items[$i]]) && $ok; + + $ok = db_execute_prepared('DELETE FROM plugin_thold_template_contact + WHERE template_id = ?', + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute('UPDATE thold_template - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; } } - 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 < 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')); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_warning = 0 + WHERE id = ? + AND notify_warning = ?', + [$selected_items[$i], get_request_var('id')]) && $ok; } 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')); + $ok = db_execute_prepared('UPDATE thold_template + SET notify_alert = 0 + WHERE id = ? + AND notify_alert = ?', + [$selected_items[$i], get_request_var('id')]) && $ok; + } + + $update_template[] = $selected_items[$i]; + + if (!$ok) { + break; } + } + } + + if ($ok) { + db_execute('COMMIT'); - thold_template_update_thresholds($selected_items[$i]); + // 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_execute('ROLLBACK'); } } @@ -444,27 +541,37 @@ 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'); + + db_execute('START TRANSACTION'); + + $ok = true; 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]); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; // clear other items - db_execute("UPDATE thold_data - SET notify_warning_extra='' - WHERE id=" . $selected_items[$i]); + $ok = db_execute_prepared("UPDATE thold_data + SET notify_warning_extra = '' + WHERE id = ?", + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute('UPDATE thold_data - SET notify_warning=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_warning = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; } } @@ -472,43 +579,64 @@ 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]); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; // 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]); + $ok = db_execute_prepared("UPDATE thold_data + SET notify_extra = '' + WHERE id = ?", + [$selected_items[$i]]) && $ok; + + $ok = db_execute_prepared('DELETE FROM plugin_thold_threshold_contact + WHERE thold_id = ?', + [$selected_items[$i]]) && $ok; } else { // set the notification list - db_execute('UPDATE thold_data - SET notify_alert=' . get_request_var('id') . ' - WHERE id=' . $selected_items[$i]); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_alert = ? + WHERE id = ?', + [get_request_var('id'), $selected_items[$i]]) && $ok; } } + + if (!$ok) { + break; + } } } 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')); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_warning = 0 + WHERE id = ? + AND notify_warning = ?', + [$selected_items[$i], get_request_var('id')]) && $ok; } 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')); + $ok = db_execute_prepared('UPDATE thold_data + SET notify_alert = 0 + WHERE id = ? + AND notify_alert = ?', + [$selected_items[$i], get_request_var('id')]) && $ok; + } + + if (!$ok) { + break; } } } + + if ($ok) { + db_execute('COMMIT'); + } else { + db_execute('ROLLBACK'); + } } header('Location: notify_lists.php?header=false&action=edit&tab=tholds&id=' . get_request_var('id')); @@ -590,7 +718,7 @@ function form_actions() { - + $save_html "; @@ -665,10 +793,10 @@ function form_actions() { print " - + - + $save_html "; @@ -743,10 +871,10 @@ function form_actions() { print " - + - + $save_html "; @@ -828,10 +956,10 @@ function form_actions() { print " - + - + $save_html "; @@ -1139,11 +1267,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); } @@ -1399,7 +1527,9 @@ function tholds($header_label) { } if (strlen(get_request_var('rfilter'))) { - $sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE '" . 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 ' . thold_rlike_clause(get_request_var('rfilter')); } if ($statefilter != '') { @@ -1509,11 +1639,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); } @@ -1739,7 +1869,9 @@ 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') . "'"; + // 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 ' . thold_rlike_clause(get_request_var('rfilter')); } $sql = "SELECT * @@ -1798,8 +1930,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); } @@ -2117,8 +2249,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); } @@ -2143,10 +2275,12 @@ 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') . "')"; + // 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 ' . thold_rlike_clause(get_request_var('rfilter')) . ' + OR description ' . thold_rlike_clause(get_request_var('rfilter')) . ' + OR emails ' . thold_rlike_clause(get_request_var('rfilter')) . ')'; } else { $sql_where = ''; } diff --git a/notify_queue.php b/notify_queue.php index 12f2464..51a6a4e 100644 --- a/notify_queue.php +++ b/notify_queue.php @@ -110,7 +110,13 @@ function form_actions() { if ($selected_items != false) { if (get_nfilter_request_var('drp_action') == '1') { // delete - db_execute('DELETE FROM notification_queue WHERE ' . array_to_sql_or($selected_items, 'id')); + $placeholders = implode(', ', array_fill(0, cacti_sizeof($selected_items), '?')); + $params = array_map('intval', array_values($selected_items)); + + db_execute_prepared( + "DELETE FROM notification_queue WHERE id IN ($placeholders)", + $params + ); } } @@ -318,10 +324,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/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/Unit/GetAllowedThresholdsTest.php b/tests/Unit/GetAllowedThresholdsTest.php new file mode 100644 index 0000000..f62796a --- /dev/null +++ b/tests/Unit/GetAllowedThresholdsTest.php @@ -0,0 +1,230 @@ + + */ + public static function accessorProvider() { + return [ + 'thresholds' => ['get_allowed_thresholds'], + 'logs' => ['get_allowed_threshold_logs'], + ]; + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testGraphIdIsBoundRatherThanInterpolated($function): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 42); + + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('gl.id = ?', $call['sql']); + $this->assertStringNotContainsString('42', $call['sql']); + $this->assertSame([42], $call['params']); + } + + /** + * A caller injecting SQL through $graph_id must end up with the payload as + * an inert bound value, never as query text. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testMaliciousGraphIdNeverReachesQueryText($function): void { + $payload = '1 UNION SELECT password FROM user_auth'; + $total = 0; + $function('', 'td.name', '', $total, -1, $payload); + + foreach (CactiStubs::$calls as $call) { + $this->assertStringNotContainsString('UNION', $call['sql']); + } + } + + /** + * The $graph_id placeholder is appended after the caller's fragment, so the + * caller's own values have to come first in $sql_params for the binding to + * line up. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testCallerParametersAreBoundBeforeTheGraphIdParameter($function): void { + $total = 0; + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); + + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertSame([3, 7], $call['params']); + $this->assertStringContainsString('td.thold_type = ? AND gl.id = ?', $call['sql']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testNoWhereClauseIsEmittedWhenNothingFiltersTheQuery($function): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 0); + + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringNotContainsString('WHERE', $call['sql']); + $this->assertSame([], $call['params']); + } + + /** + * The row-count query reuses the same WHERE clause, so it has to receive + * the same bound values. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testRowCountQueryBindsTheSameParameters($function): void { + $total = 0; + $function('td.thold_type = ?', 'td.name', '', $total, -1, 7, [3]); + + $count = CactiStubs::callsTo('db_fetch_cell_prepared')[0]; + + $this->assertSame([3, 7], $count['params']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testOrderByAndLimitAreAppliedToTheQuery($function): void { + $total = 0; + $function('', 'td.id DESC', '0,30', $total, -1, 0); + + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('ORDER BY td.id DESC', $call['sql']); + $this->assertStringContainsString('LIMIT 0,30', $call['sql']); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testResultRowsAreReturnedToTheCaller($function): void { + CactiStubs::willReturn('db_fetch_assoc_prepared', [['id' => 5]]); + + $total = 0; + $rows = $function('', 'td.name', '', $total, -1, 0); + + $this->assertSame([['id' => 5]], $rows); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testTotalRowsIsSetByReference($function): void { + CactiStubs::willReturn('db_fetch_cell_prepared', 17); + + $total = 0; + $function('', 'td.name', '', $total, -1, 3); + + $this->assertSame(17, $total); + } + + /** + * With authentication on and no session, there is no user to resolve + * permissions against, so the query must not run at all. + * + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testNoQueryRunsWhenAuthenticationIsOnAndNoUserIsResolved($function): void { + CactiStubs::$configOptions['auth_method'] = 1; + unset($_SESSION['sess_user_id']); + + $total = 0; + $rows = $function('', 'td.name', '', $total, 0, 0); + + $this->assertSame([], $rows); + $this->assertSame([], CactiStubs::callsTo('db_fetch_assoc_prepared')); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * + * @return void + */ + public function testPolicyWhereIsAppliedWhenPermissionsAreNotSimple($function): void { + CactiStubs::$configOptions['auth_method'] = 1; + CactiStubs::willReturn('get_simple_graph_perms', false); + CactiStubs::willReturn('get_policy_where', 'WHERE policy_applied = 1'); + $_SESSION['sess_user_id'] = 9; + + $total = 0; + $function('', 'td.name', '', $total, 0, 0); + + unset($_SESSION['sess_user_id']); + + $call = CactiStubs::callsTo('db_fetch_assoc_prepared')[0]; + + $this->assertStringContainsString('policy_applied = 1', $call['sql']); + } +} diff --git a/tests/Unit/TholdCalculateLowerUpperTest.php b/tests/Unit/TholdCalculateLowerUpperTest.php new file mode 100644 index 0000000..b76ba75 --- /dev/null +++ b/tests/Unit/TholdCalculateLowerUpperTest.php @@ -0,0 +1,68 @@ + 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_hi' => 2]]; + + $this->assertSame((2 << 32) + 100, thold_calculate_lower_upper($thold, 100, $rrd)); + } + + /** + * @return void + */ + public function testValuePassesThroughWhenTheHighWordIsAbsent(): void { + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_lo' => 5]]; + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); + } + + /** + * @return void + */ + public function testValuePassesThroughWhenTheDataSourceHasNoReadings(): void { + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, [])); + } + + /** + * @return void + */ + public function testHighWordOfZeroLeavesTheValueUnchanged(): void { + $thold = ['upper_ds' => 'octets_hi', 'local_data_id' => 4]; + $rrd = [4 => ['octets_hi' => 0]]; + + $this->assertSame(100, thold_calculate_lower_upper($thold, 100, $rrd)); + } +} diff --git a/tests/Unit/TholdCommandExecutionTest.php b/tests/Unit/TholdCommandExecutionTest.php new file mode 100644 index 0000000..d0f3e64 --- /dev/null +++ b/tests/Unit/TholdCommandExecutionTest.php @@ -0,0 +1,250 @@ + + */ + private function threshold(array $overrides = []) { + return $overrides + [ + 'id' => 3, + 'local_data_id' => 4, + 'local_graph_id' => 7, + 'name' => 'CPU', + 'name_cache' => 'CPU', + 'data_source_name' => 'traffic_in', + 'lastread' => 95, + 'notes' => '', + 'dnotes' => '', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'thold_template_id' => 0, + 'trigger_cmd_high' => '', + 'trigger_cmd_low' => '', + 'trigger_cmd_norm' => '', + ]; + } + + /** + * @return array + */ + private function device(array $overrides = []) { + return $overrides + [ + 'id' => 2, + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + ]; + } + + /** + * The queued command, or null when nothing was queued. + * + * @return string|null + */ + private function queuedCommand() { + foreach (CactiStubs::callsTo('db_execute_prepared') as $call) { + foreach ($call['params'] as $param) { + if (is_string($param) && strpos($param, '"command"') !== false) { + $decoded = json_decode($param, true); + + return isset($decoded['command']) ? $decoded['command'] : null; + } + } + } + + return null; + } + + /** + * @return array}> + */ + public static function breachDirectionProvider() { + return [ + 'high' => ['trigger_cmd_high', [true, false, false]], + 'low' => ['trigger_cmd_low', [false, true, false]], + 'restore' => ['trigger_cmd_norm', [false, false, true]], + ]; + } + + /** + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testEachBreachDirectionRunsItsOwnCommand($column, array $breaches): void { + $thold = $this->threshold([$column => '/usr/bin/alert ']); + $device = $this->device(); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertSame("/usr/bin/alert '10.0.0.1'", $this->queuedCommand()); + } + + /** + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testShellMetacharactersInDeviceDataAreQuoted($column, array $breaches): void { + $thold = $this->threshold([$column => '/usr/bin/alert ']); + $device = $this->device(['description' => '; touch /tmp/pwned']); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertSame("/usr/bin/alert '; touch /tmp/pwned'", $this->queuedCommand()); + } + + /** + * @return void + */ + public function testNothingRunsWhenScriptsAreDisabled(): void { + CactiStubs::$configOptions['thold_enable_scripts'] = ''; + + $thold = $this->threshold(['trigger_cmd_high' => '/usr/bin/alert']); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertNull($this->queuedCommand()); + } + + /** + * @return void + */ + public function testNothingRunsWhenTheDirectionHasNoCommandConfigured(): void { + $thold = $this->threshold(); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertNull($this->queuedCommand()); + } + + /** + * A high breach takes precedence, so a threshold configured for both cannot + * run two commands in one evaluation. + * + * @return void + */ + public function testHighBreachTakesPrecedenceOverLow(): void { + $thold = $this->threshold([ + 'trigger_cmd_high' => '/usr/bin/high', + 'trigger_cmd_low' => '/usr/bin/low', + ]); + $device = $this->device(); + + thold_command_execution($thold, $device, true, true, false); + + $this->assertSame('/usr/bin/high', $this->queuedCommand()); + } + + /** + * With the queue off the command runs inline, and its exit status and + * output are logged. + * + * @dataProvider breachDirectionProvider + * + * @param string $column + * @param array $breaches + * + * @return void + */ + public function testInlineExecutionLogsTheCommandOutput($column, array $breaches): void { + CactiStubs::$configOptions['thold_notification_queue'] = ''; + + $thold = $this->threshold([$column => '/bin/echo breach']); + $device = $this->device(); + + thold_command_execution($thold, $device, $breaches[0], $breaches[1], $breaches[2]); + + $this->assertNull($this->queuedCommand()); + $this->assertNotEmpty(CactiStubs::$log); + } + + /** + * @return array + */ + public static function inlineOutcomeProvider() { + return [ + 'success without output' => ['/bin/true', 'NOTE'], + 'success with output' => ['/bin/echo ok', 'NOTE'], + 'failure without output' => ['/bin/false', 'WARNING'], + 'failure with output' => ["/bin/sh -c 'echo oops; exit 1'", 'WARNING'], + ]; + } + + /** + * A trigger command that fails is the operator's only signal that their + * alerting is broken, so the exit status has to reach the log either way. + * + * @dataProvider inlineOutcomeProvider + * + * @param string $command + * @param string $level + * + * @return void + */ + public function testInlineExecutionLogsTheExitStatus($command, $level): void { + CactiStubs::$configOptions['thold_notification_queue'] = ''; + + $thold = $this->threshold(['trigger_cmd_high' => $command]); + $device = $this->device(); + + thold_command_execution($thold, $device, true, false, false); + + $this->assertStringStartsWith($level, CactiStubs::$log[0]); + } +} diff --git a/tests/Unit/TholdExpandStringTest.php b/tests/Unit/TholdExpandStringTest.php new file mode 100644 index 0000000..8f1415d --- /dev/null +++ b/tests/Unit/TholdExpandStringTest.php @@ -0,0 +1,140 @@ + escaping rather than after. + */ +final class TholdExpandStringTest extends TestCase { + /** + * @return void + */ + public static function setUpBeforeClass(): void { + self::loadPluginSource('thold_functions.php'); + } + + /** + * @return array + */ + private function thresholdData(array $overrides = []) { + return $overrides + [ + 'local_graph_id' => 7, + 'local_data_id' => 4, + 'data_source_name' => 'traffic_in', + 'thold_template_id' => 0, + ]; + } + + /** + * @return void + */ + private function graphExists() { + CactiStubs::willReturn('db_fetch_row_prepared', [ + 'id' => 7, + 'host_id' => 2, + 'snmp_query_id' => 3, + 'snmp_index' => '1', + ]); + } + + /** + * @return void + */ + public function testGraphTitleTokenIsResolved(): void { + $this->graphExists(); + + $this->assertSame('Traffic - eth0', thold_expand_string($this->thresholdData(), '|graph_title|')); + } + + /** + * @return void + */ + public function testDataSourceNameTokenIsResolved(): void { + $this->graphExists(); + + $this->assertSame('traffic_in', thold_expand_string($this->thresholdData(), '|data_source_name|')); + } + + /** + * @return void + */ + public function testDataSourceDescriptionTokenIsResolvedFromTheDatabase(): void { + $this->graphExists(); + CactiStubs::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + + $this->assertSame('Router - Traffic', thold_expand_string($this->thresholdData(), '|data_source_description|')); + } + + /** + * @return void + */ + public function testTextIsPassedThroughExpandTitleForDataQueryTokens(): void { + $this->graphExists(); + CactiStubs::willReturn('expand_title', 'alert eth0'); + + $this->assertSame('alert eth0', thold_expand_string($this->thresholdData(), 'alert |query_ifName|')); + $this->assertNotEmpty(CactiStubs::callsTo('expand_title')); + } + + /** + * @return void + */ + public function testInterfaceSpeedFallsBackToTheConfiguredDefaultWhenUnknown(): void { + $this->graphExists(); + CactiStubs::$configOptions['thold_empty_if_speed_default'] = '1000000000'; + CactiStubs::willReturn('db_fetch_cell_prepared', ''); + + $result = thold_expand_string($this->thresholdData(), '|query_ifHighSpeed|'); + + $this->assertStringNotContainsString('|query_ifHighSpeed|', $result); + } + + /** + * @return void + */ + public function testTextIsReturnedUnchangedWhenTheGraphIsMissing(): void { + CactiStubs::willReturn('db_fetch_row_prepared', []); + + $this->assertSame('static text', thold_expand_string($this->thresholdData(), 'static text')); + } + + /** + * An empty template falls back to the threshold template's suggested name, + * which is itself a token string and gets expanded in turn. + * + * @return void + */ + public function testEmptyStringFallsBackToTheExpandedTemplateSuggestedName(): void { + $this->graphExists(); + CactiStubs::willReturn('db_fetch_cell_prepared', 'Suggested |data_source_name|'); + + $result = thold_expand_string($this->thresholdData(['thold_template_id' => 5]), ''); + + $this->assertSame('Suggested traffic_in', $result); + } + + /** + * @return void + */ + public function testSurroundingWhitespaceIsTrimmed(): void { + CactiStubs::willReturn('db_fetch_row_prepared', []); + + $this->assertSame('alert', thold_expand_string($this->thresholdData(), ' alert ')); + } +} diff --git a/tests/Unit/TholdExpressionMathRpnTest.php b/tests/Unit/TholdExpressionMathRpnTest.php new file mode 100644 index 0000000..6c9fdbf --- /dev/null +++ b/tests/Unit/TholdExpressionMathRpnTest.php @@ -0,0 +1,282 @@ + $stack Initial stack, bottom element first. + * @param string $operator RPN operator token. + * + * @return array + */ + private function evaluate(array $stack, $operator) { + thold_expression_math_rpn($operator, $stack); + + return $stack; + } + + /** + * @return array, 1: string, 2: float|int}> + */ + public static function binaryOperatorProvider() { + return [ + 'addition' => [[8, 2], '+', 10], + 'subtraction keeps order' => [[8, 2], '-', 6], + 'multiplication' => [[8, 2], '*', 16], + 'division keeps order' => [[8, 2], '/', 4], + 'modulo' => [[8, 3], '%', 2], + 'float addition' => [[1.5, 2.25], '+', 3.75], + 'numeric string operands' => [['8', '2'], '-', 6], + 'negative operands' => [[-8, 2], '/', -4], + ]; + } + + /** + * @dataProvider binaryOperatorProvider + * + * @param array $stack + * @param string $operator + * @param float|int $expected + * + * @return void + */ + public function testBinaryOperatorsComputeInStackOrder(array $stack, $operator, $expected): void { + $this->assertSame([$expected], $this->evaluate($stack, $operator)); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * PHP's ^ is bitwise XOR, not exponentiation. The pre-hardening evaluator + * built the string "$v2 ^ $v1" and eval'd it, so it computed XOR too. This + * test records the behaviour as-is; changing it to pow() would be a + * separate, breaking change to existing user thresholds. + * + * @return void + */ + public function testCaretOperatorIsIntegerXorNotExponentiation(): void { + $this->assertSame([6], $this->evaluate([5, 3], '^')); + $this->assertSame([1], $this->evaluate([2, 3], '^')); + } + + /** + * @return void + */ + public function testCaretOperatorTruncatesFloatOperandsToIntegers(): void { + $this->assertSame([6], $this->evaluate([5.9, 3.9], '^')); + } + + /** + * @return void + */ + public function testModuloTruncatesFloatOperandsToIntegers(): void { + $this->assertSame([1], $this->evaluate([7.9, 3.2], '%')); + } + + /** + * Dividing zero by zero is defined as zero rather than an error so a + * counter that has not moved does not flag the whole threshold. + * + * @return void + */ + public function testZeroDividedByZeroYieldsZeroWithoutError(): void { + $this->assertSame([0], $this->evaluate([0, 0], '/')); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testDivisionByZeroFlagsErrorAndPushesNothing(): void { + $this->assertSame([], $this->evaluate([8, 0], '/')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testModuloByZeroFlagsErrorInsteadOfThrowing(): void { + $this->assertSame([], $this->evaluate([8, 0], '%')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return array, 1: string}> + */ + public static function nonNumericOperandProvider() { + return [ + 'unknown right operand' => [[8, 'U'], '+'], + 'unknown left operand' => [['U', 8], '+'], + 'NaN right operand' => [[8, 'NAN'], '*'], + 'text operand' => [[8, 'abc'], '-'], + ]; + } + + /** + * @dataProvider nonNumericOperandProvider + * + * @param array $stack + * @param string $operator + * + * @return void + */ + public function testNonNumericOperandsFlagErrorAndPushNothing(array $stack, $operator): void { + $this->assertSame([], $this->evaluate($stack, $operator)); + $this->assertTrue($GLOBALS['rpn_error']); + $this->assertNotEmpty(CactiStubs::$log); + } + + /** + * @return array + */ + public static function unaryFunctionProvider() { + return [ + 'SIN' => [0, 'SIN', 0.0], + 'COS' => [0, 'COS', 1.0], + 'TAN' => [0, 'TAN', 0.0], + 'ATAN' => [0, 'ATAN', 0.0], + 'SQRT' => [9, 'SQRT', 3.0], + 'FLOOR' => [2.7, 'FLOOR', 2.0], + 'CEIL' => [2.1, 'CEIL', 3.0], + 'DEG2RAD' => [180, 'DEG2RAD', M_PI], + 'RAD2DEG' => [M_PI, 'RAD2DEG', 180.0], + 'ABS' => [-5, 'ABS', 5], + 'EXP' => [0, 'EXP', 1.0], + 'LOG' => [M_E, 'LOG', 1.0], + ]; + } + + /** + * @dataProvider unaryFunctionProvider + * + * @param float|int $operand + * @param string $operator + * @param float|int $expected + * + * @return void + */ + public function testUnaryFunctionsDispatchToNativeMath($operand, $operator, $expected): void { + $stack = $this->evaluate([$operand], $operator); + + $this->assertCount(1, $stack); + $this->assertEqualsWithDelta($expected, $stack[0], 1.0e-9); + $this->assertFalse($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testUnaryFunctionRejectsNonNumericOperand(): void { + $this->assertSame([], $this->evaluate(['U'], 'SQRT')); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * sqrt(-1) and log(0) have no usable numeric result, so the evaluator must + * flag an error rather than push NAN or -INF into a threshold comparison, + * where every comparison against them silently returns false. + * + * @return array + */ + public static function undefinedResultProvider() { + return [ + 'square root of a negative' => [-1, 'SQRT'], + 'log of zero' => [0, 'LOG'], + 'log of a negative' => [-1, 'LOG'], + ]; + } + + /** + * @dataProvider undefinedResultProvider + * + * @param float|int $operand + * @param string $operator + * + * @return void + */ + public function testUndefinedResultsFlagErrorInsteadOfPushingNanOrInf($operand, $operator): void { + $this->assertSame([], $this->evaluate([$operand], $operator)); + $this->assertTrue($GLOBALS['rpn_error']); + } + + /** + * @return void + */ + public function testAtan2ComputesAgainstBothOperands(): void { + $stack = $this->evaluate([1, 1], 'ATAN2'); + + $this->assertEqualsWithDelta(M_PI / 4, $stack[0], 1.0e-9); + } + + /** + * ADDNAN exists so a missing sample contributes zero instead of poisoning + * the sum. + * + * @return array, 1: float|int}> + */ + public static function addNanProvider() { + return [ + 'both known' => [[3, 4], 7], + 'right unknown' => [[3, 'U'], 3], + 'left unknown' => [['U', 4], 4], + 'right NaN' => [[3, 'NAN'], 3], + 'both unknown' => [['U', 'NAN'], 0], + ]; + } + + /** + * @dataProvider addNanProvider + * + * @param array $stack + * @param float|int $expected + * + * @return void + */ + public function testAddNanTreatsUnknownOperandsAsZero(array $stack, $expected): void { + $this->assertSame([$expected], $this->evaluate($stack, 'ADDNAN')); + } + + /** + * @return void + */ + public function testUnknownOperatorLeavesStackUntouched(): void { + $this->assertSame([1, 2], $this->evaluate([1, 2], 'NOSUCHOP')); + } + + /** + * @return void + */ + public function testUnderflowFlagsErrorRatherThanPoppingAnEmptyStack(): void { + $this->evaluate([], '+'); + + $this->assertTrue($GLOBALS['rpn_error']); + } +} diff --git a/tests/Unit/TholdGetCachedNameTest.php b/tests/Unit/TholdGetCachedNameTest.php new file mode 100644 index 0000000..a33d83c --- /dev/null +++ b/tests/Unit/TholdGetCachedNameTest.php @@ -0,0 +1,59 @@ + '|data_source_description|', 'name_cache' => 'CPU load', 'local_data_id' => 4]; + + $this->assertSame('CPU load', thold_get_cached_name($thold)); + $this->assertSame([], CactiStubs::callsTo('db_fetch_cell_prepared')); + } + + /** + * @return void + */ + public function testEmptyCacheIsFilledFromTheDataSourceDescription(): void { + CactiStubs::willReturn('db_fetch_cell_prepared', 'Router - Traffic'); + + $thold = ['name' => '|data_source_description|', 'name_cache' => '', 'local_data_id' => 4]; + + $this->assertSame('Router - Traffic', thold_get_cached_name($thold)); + $this->assertSame('Router - Traffic', $thold['name_cache']); + } + + /** + * @return void + */ + public function testNameIsKeptWhenTheDataSourceHasNoDescription(): void { + $thold = ['name' => 'Manual name', 'name_cache' => '', 'local_data_id' => 4]; + + $this->assertSame('Manual name', thold_get_cached_name($thold)); + } +} diff --git a/tests/Unit/TholdReplaceThresholdTagsTest.php b/tests/Unit/TholdReplaceThresholdTagsTest.php new file mode 100644 index 0000000..96006c94 --- /dev/null +++ b/tests/Unit/TholdReplaceThresholdTagsTest.php @@ -0,0 +1,274 @@ + substitution reads. + self::loadPluginSource('includes/arrays.php'); + } + + /** + * @return array + */ + private function threshold(array $overrides = []) { + return $overrides + [ + 'id' => 1, + 'name_cache' => 'CPU', + 'notes' => '', + 'dnotes' => '', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'time_hi' => 80, + 'time_low' => 20, + 'time_fail_trigger' => 2, + 'time_fail_length' => 300, + 'local_data_id' => 4, + ]; + } + + /** + * @return array + */ + private function device(array $overrides = []) { + return $overrides + [ + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + ]; + } + + /** + * Substitute tags into $text. + * + * @param string $text + * @param array $thold + * @param array $device + * @param bool $shell + * @param mixed $currentval + * + * @return string + */ + private function substitute($text, array $thold, array $device, $shell, $currentval = 42) { + return thold_replace_threshold_tags($text, $thold, $device, $currentval, 7, 'traffic_in', $shell); + } + + /** + * @return array + */ + public static function deviceDerivedTagProvider() { + return [ + 'description' => ['', 'description', 'device'], + 'hostname' => ['', 'hostname', 'device'], + 'location' => ['', 'location', 'device'], + 'notes' => ['', 'notes', 'threshold'], + 'device note' => ['', 'dnotes', 'threshold'], + 'external id' => ['', 'external_id', 'threshold'], + 'name' => ['', 'name_cache', 'threshold'], + ]; + } + + /** + * @dataProvider deviceDerivedTagProvider + * + * @param string $tag + * @param string $column + * @param string $source + * + * @return void + */ + public function testShellModeQuotesEveryDeviceDerivedTag($tag, $column, $source): void { + $payload = '; touch /tmp/pwned'; + $thold = $this->threshold($source === 'threshold' ? [$column => $payload] : []); + $device = $this->device($source === 'device' ? [$column => $payload] : []); + + $result = $this->substitute("/usr/bin/alert $tag", $thold, $device, true); + + $this->assertStringContainsString(escapeshellarg($payload), $result); + $this->assertStringNotContainsString('alert ; touch', $result); + } + + /** + * @dataProvider deviceDerivedTagProvider + * + * @param string $tag + * @param string $column + * @param string $source + * + * @return void + */ + public function testEmailModeLeavesDeviceDerivedTagsUnquoted($tag, $column, $source): void { + $thold = $this->threshold($source === 'threshold' ? [$column => "O'Brien"] : []); + $device = $this->device($source === 'device' ? [$column => "O'Brien"] : []); + + $result = $this->substitute("Alert on $tag", $thold, $device, false); + + $this->assertStringContainsString("O'Brien", $result); + $this->assertStringNotContainsString("'O'\\''Brien'", $result); + } + + /** + * The site name comes from the sites table, which an operator edits, so it + * needs the same treatment as the device columns. + * + * @return void + */ + public function testShellModeQuotesTheSiteName(): void { + CactiStubs::willReturn('db_fetch_cell_prepared', '$(id)'); + + $result = $this->substitute('/usr/bin/alert ', $this->threshold(), $this->device(), true); + + $this->assertStringContainsString(escapeshellarg('$(id)'), $result); + } + + /** + * @return void + */ + public function testSiteFallsBackToDefaultWhenTheDeviceHasNoSite(): void { + CactiStubs::willReturn('db_fetch_cell_prepared', ''); + + $result = $this->substitute('site=', $this->threshold(), $this->device(), false); + + $this->assertSame('site=Default', $result); + } + + /** + * The current reading is an RRD value rather than a number in every case; + * it must not be able to extend the command line either. + * + * @return void + */ + public function testShellModeQuotesTheCurrentValue(): void { + $result = $this->substitute('/usr/bin/alert ', $this->threshold(), $this->device(), true, '; id'); + + $this->assertStringContainsString(escapeshellarg('; id'), $result); + $this->assertStringNotContainsString('alert ; id', $result); + } + + /** + * @return void + */ + public function testEmailModeLeavesTheCurrentValueUnquoted(): void { + $result = $this->substitute('value=', $this->threshold(), $this->device(), false, 42); + + $this->assertSame('value=42', $result); + } + + /** + * @return void + */ + public function testGraphAndThresholdIdentifiersAreSubstituted(): void { + $result = $this->substitute('/', $this->threshold(['id' => 5]), $this->device(), false); + + $this->assertSame('7/5', $result); + } + + /** + * @return void + */ + public function testStaticThresholdBoundsAreSubstituted(): void { + $result = $this->substitute('//', $this->threshold(), $this->device(), false); + + $this->assertSame('90/10/3', $result); + } + + /** + * A time-based threshold reports its own bounds and a duration rather than + * the static ones. + * + * @return void + */ + public function testTimeBasedThresholdSubstitutesTheTimeBounds(): void { + $result = $this->substitute('[][][]', $this->threshold(['thold_type' => 2]), $this->device(), false); + + $this->assertSame('[80][20][2]', $result); + } + + /** + * A baseline threshold has neither static nor time bounds, so the tags + * resolve to empty rather than being left in the output. + * + * @return void + */ + public function testBaselineThresholdClearsTheBoundTags(): void { + $result = $this->substitute('[][][][]', $this->threshold(['thold_type' => 1]), $this->device(), false); + + $this->assertSame('[][][][]', $result); + } + + /** + * @return void + */ + public function testStaticThresholdHasNoDuration(): void { + $result = $this->substitute('[]', $this->threshold(), $this->device(), false); + + $this->assertSame('[]', $result); + } + + /** + * @return void + */ + public function testUrlTagRendersALinkToTheGraph(): void { + CactiStubs::$configOptions['base_url'] = 'http://cacti.example.org'; + + $result = $this->substitute('', $this->threshold(), $this->device(), false); + + $this->assertStringContainsString('graph.php?local_graph_id=7', $result); + } + + /** + * @return void + */ + public function testThresholdTypeNameIsSubstituted(): void { + $result = $this->substitute('', $this->threshold(), $this->device(), false); + + $this->assertSame('High / Low', $result); + } + + /** + * @return void + */ + public function testUnknownThresholdTypeLeavesTheTagInPlace(): void { + $result = $this->substitute('', $this->threshold(['thold_type' => 99]), $this->device(), false); + + $this->assertSame('', $result); + } + + /** + * @return void + */ + public function testTextWithoutTagsIsReturnedUnchanged(): void { + $result = $this->substitute('nothing to replace', $this->threshold(), $this->device(), true); + + $this->assertSame('nothing to replace', $result); + } +} diff --git a/tests/Unit/TholdRlikeClauseTest.php b/tests/Unit/TholdRlikeClauseTest.php new file mode 100644 index 0000000..c32a642 --- /dev/null +++ b/tests/Unit/TholdRlikeClauseTest.php @@ -0,0 +1,70 @@ +assertSame("RLIKE 'router'", thold_rlike_clause('router')); + } + + /** + * A quote in the filter has to be escaped, or it closes the string literal + * and the rest of the filter becomes SQL. + * + * @return void + */ + public function testQuotesInTheFilterAreEscaped(): void { + $clause = thold_rlike_clause("' OR 1=1 -- "); + + $this->assertSame("RLIKE ''' OR 1=1 -- '", $clause); + } + + /** + * @return void + */ + public function testEmptyFilterStillProducesAValidClause(): void { + $this->assertSame("RLIKE ''", thold_rlike_clause('')); + } + + /** + * @return void + */ + public function testCoreHelperIsPreferredWhenAvailable(): void { + if (!function_exists('db_qstr_rlike')) { + $this->assertSame("RLIKE 'x'", thold_rlike_clause('x')); + + return; + } + + $this->assertSame(db_qstr_rlike('x'), thold_rlike_clause('x')); + } +} diff --git a/tests/Unit/TholdSetEnvironTest.php b/tests/Unit/TholdSetEnvironTest.php new file mode 100644 index 0000000..02ac28e --- /dev/null +++ b/tests/Unit/TholdSetEnvironTest.php @@ -0,0 +1,215 @@ + + */ + private function threshold(array $overrides = []) { + return $overrides + [ + 'id' => 3, + 'local_data_id' => 4, + 'local_graph_id' => 7, + 'name_cache' => 'CPU', + 'notes' => '', + 'dnotes' => 'device note', + 'external_id' => '', + 'thold_type' => 0, + 'thold_hi' => 90, + 'thold_low' => 10, + 'thold_fail_trigger' => 3, + 'time_hi' => 80, + 'time_low' => 20, + 'time_fail_trigger' => 2, + 'time_fail_length' => 300, + ]; + } + + /** + * @return array + */ + private function device(array $overrides = []) { + return $overrides + [ + 'description' => 'router1', + 'hostname' => '10.0.0.1', + 'location' => 'rack 4', + 'site_id' => 1, + 'status' => 3, + 'status_fail_date' => '2026-01-01 00:00:00', + 'status_rec_date' => '2026-01-02 00:00:00', + 'status_last_error' => '', + ]; + } + + /** + * Collect the exported environment as a name => value map. + * + * @param array $thold + * @param array $device + * + * @return array + */ + private function environment(array $thold, array $device) { + $pairs = thold_set_environ('', $thold, $device, 42, 7, 'traffic_in'); + $map = []; + + foreach ($pairs as $pair) { + [$name, $value] = explode('=', $pair, 2); + + $map[$name] = $value; + } + + return $map; + } + + /** + * @return void + */ + public function testThresholdAndDeviceContextIsExported(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('3', $env['THOLD_ID']); + $this->assertSame('router1', $env['THOLD_DESCRIPTION']); + $this->assertSame('10.0.0.1', $env['THOLD_HOSTNAME']); + $this->assertSame('42', $env['THOLD_CURRENTVALUE']); + $this->assertSame('traffic_in', $env['THOLD_DSNAME']); + $this->assertSame('device note', $env['THOLD_DEVICENOTE']); + } + + /** + * The queue accumulates across calls, so the first pair has to reset it or + * a later command inherits the previous threshold's context. + * + * @return void + */ + public function testEachCallStartsFromAnEmptyEnvironment(): void { + $this->environment($this->threshold(), $this->device()); + $env = $this->environment($this->threshold(['id' => 9]), $this->device()); + + $this->assertSame('9', $env['THOLD_ID']); + $this->assertCount(1, array_keys(array_filter(array_keys($env), function ($name) { + return $name === 'THOLD_ID'; + }))); + } + + /** + * @return void + */ + public function testStaticThresholdExportsItsBoundsAndNoDuration(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('90', $env['THOLD_HI']); + $this->assertSame('10', $env['THOLD_LOW']); + $this->assertSame('3', $env['THOLD_TRIGGER']); + $this->assertSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testTimeBasedThresholdExportsTheTimeBoundsAndADuration(): void { + $env = $this->environment($this->threshold(['thold_type' => 2]), $this->device()); + + $this->assertSame('80', $env['THOLD_HI']); + $this->assertSame('20', $env['THOLD_LOW']); + $this->assertSame('2', $env['THOLD_TRIGGER']); + $this->assertNotSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testBaselineThresholdExportsEmptyBounds(): void { + $env = $this->environment($this->threshold(['thold_type' => 1]), $this->device()); + + $this->assertSame('', $env['THOLD_HI']); + $this->assertSame('', $env['THOLD_LOW']); + $this->assertSame('', $env['THOLD_TRIGGER']); + $this->assertSame('', $env['THOLD_DURATION']); + } + + /** + * @return void + */ + public function testNotesAreTagExpandedWhenPresent(): void { + $env = $this->environment($this->threshold(['notes' => 'see ']), $this->device()); + + $this->assertSame('see 10.0.0.1', $env['THOLD_NOTES']); + } + + /** + * @return void + */ + public function testNotesAreEmptyWhenTheThresholdHasNone(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('', $env['THOLD_NOTES']); + } + + /** + * @return void + */ + public function testExternalIdIsExportedOnlyWhenSet(): void { + $env = $this->environment($this->threshold(), $this->device()); + $this->assertArrayNotHasKey('THOLD_EXTERNAL_ID', $env); + + $env = $this->environment($this->threshold(['external_id' => 'INC-42']), $this->device()); + $this->assertSame('INC-42', $env['THOLD_EXTERNAL_ID']); + } + + /** + * @return void + */ + public function testUnknownThresholdTypeExportsAnEmptyTypeName(): void { + $env = $this->environment($this->threshold(['thold_type' => 99]), $this->device()); + + $this->assertSame('', $env['THOLD_THOLDTYPE']); + } + + /** + * @return void + */ + public function testGraphUrlIsExported(): void { + $env = $this->environment($this->threshold(), $this->device()); + + $this->assertSame('http://cacti.example.org/graph.php?local_graph_id=7', $env['THOLD_URL']); + } +} diff --git a/tests/Unit/ZOptionalCoreFunctionTest.php b/tests/Unit/ZOptionalCoreFunctionTest.php new file mode 100644 index 0000000..1b3843b --- /dev/null +++ b/tests/Unit/ZOptionalCoreFunctionTest.php @@ -0,0 +1,101 @@ +assertSame(db_qstr_rlike('router'), thold_rlike_clause('router')); + } + + /** + * Core's helper strips the alternation and quantifier characters that make + * a filter expensive to evaluate; the plugin must not bypass that. + * + * @return void + */ + public function testRlikeClauseInheritsTheCoreOperandRestrictions(): void { + $clause = thold_rlike_clause('(a|a){9,}'); + + $this->assertStringNotContainsString('|', $clause); + $this->assertStringNotContainsString('{', $clause); + } + + /** + * @return array + */ + public static function accessorProvider() { + return [ + 'thresholds' => ['get_allowed_thresholds', 'thold'], + 'logs' => ['get_allowed_threshold_logs', 'thold_log'], + ]; + } + + /** + * The cached row count is used only for the unfiltered listing; a per-graph + * query counts directly so the cache is not keyed per graph. + * + * @dataProvider accessorProvider + * + * @param string $function + * @param string $class + * + * @return void + */ + public function testRowCountUsesTheCoreCacheWhenNotFilteredByGraph($function, $class): void { + CactiStubs::willReturn('get_total_row_data', 12); + + $total = 0; + $function('', 'td.name', '', $total, -1, 0); + + $this->assertSame(12, $total); + $this->assertSame([], CactiStubs::callsTo('db_fetch_cell_prepared')); + } + + /** + * @dataProvider accessorProvider + * + * @param string $function + * @param string $class + * + * @return void + */ + public function testRowCountBypassesTheCacheForASingleGraph($function, $class): void { + $total = 0; + $function('', 'td.name', '', $total, -1, 5); + + $this->assertSame([], CactiStubs::callsTo('get_total_row_data')); + $this->assertNotEmpty(CactiStubs::callsTo('db_fetch_cell_prepared')); + } +} diff --git a/thold.php b/thold.php index 0bf86f4..e9c491c 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 ' . thold_rlike_clause(get_request_var('rfilter')); } if ($statefilter != '') { @@ -763,18 +763,18 @@ function list_tholds() { - '> + '>