fix(thold): correct trigger_cmd variable references for low/normal states by somethingwithproof · Pull Request #765 · Cacti/plugin_thold

somethingwithproof · 2026-04-09T00:48:03Z

Summary

Fix trigger command environment variable bug where the breach_down and breach_norm code paths used trigger_cmd_high instead of trigger_cmd_low and trigger_cmd_norm respectively.

This caused low and normal threshold trigger commands to receive the high threshold command's environment variables, producing incorrect command execution context.

Test plan

Configure a threshold with separate high/low/normal trigger commands
Trigger each state and verify the correct command environment is used

Closes #760

Wrap all get_request_var('rfilter') values with db_qstr() in RLIKE SQL clauses across thold.php, thold_graph.php, and notify_lists.php. 10 sites total. RLIKE accepts regex from rfilter which passes FILTER_VALIDATE_IS_REGEX but is not SQL-safe. A single quote breaks the SQL string context. Closes Cacti#761, Cacti#763 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

…ates thold_set_environ() for the breach_down path used trigger_cmd_high instead of trigger_cmd_low, and the breach_norm path also used trigger_cmd_high instead of trigger_cmd_norm. This caused low and normal threshold trigger commands to receive the high threshold command's environment variables. Closes Cacti#760 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot

Pull request overview

This PR aims to fix incorrect trigger-command environment handling in thold_command_execution() and also hardens several RLIKE SQL filters against injection by quoting/preparing the rfilter input.

Changes:

Update thold_command_execution() to pass trigger_cmd_low / trigger_cmd_norm into thold_set_environ() for breach-down and breach-normal paths.
Quote rfilter values in multiple RLIKE SQL fragments using db_qstr().
Convert some notification list queries to prepared statements (db_fetch_*_prepared) with RLIKE ? placeholders.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.

File	Description
`thold.php`	Quotes `rfilter` in `RLIKE` filter to reduce SQL injection risk.
`thold_graph.php`	Quotes `rfilter` in multiple `RLIKE` filters (thresholds, hosts, log views/exports).
`thold_functions.php`	Changes which trigger command string is passed to `thold_set_environ()` for low/normal paths.
`notify_lists.php`	Quotes `rfilter` in a `RLIKE` filter; switches some list/template queries to prepared statements.

Copilot · 2026-04-09T00:51:58Z

thold_functions.php

 		} elseif ($breach_down && $thold_data['trigger_cmd_low'] != '') {
 			$cmd = thold_replace_threshold_tags($thold_data['trigger_cmd_low'], $thold_data, $h, $thold_data['lastread'], $thold_data['local_graph_id'], $data_source_name);
 			$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);



thold_set_environ() currently does not reference its $text argument (it sets a fixed set of THOLD_* vars), so switching the call sites from trigger_cmd_high to trigger_cmd_low/trigger_cmd_norm does not change the generated environment. If the intent is for the environment to be derived from the specific trigger command, the logic needs to be implemented inside thold_set_environ() (or the unused parameter removed and the PR/issue description adjusted accordingly).

Copilot · 2026-04-09T00:51:59Z

thold.php


 	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')) . "";


This concatenation ends with . "", which is a no-op and makes the SQL assembly harder to read. Consider removing the trailing empty-string concatenation.

Suggested change

$sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . "";

$sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter'));

Copilot · 2026-04-09T00:51:59Z

thold_graph.php


 	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')) . "";


This concatenation ends with . "", which is a no-op and makes the SQL assembly harder to read. Consider removing the trailing empty-string concatenation.

Suggested change

$sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . "";

$sql_where .= ($sql_where == '' ? '(' : ' AND ') . ' td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter'));

Copilot · 2026-04-09T00:51:59Z

thold_graph.php


 	if (get_request_var('rfilter') != '') {
-		$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE '" . get_request_var('rfilter') . "'";
+		$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . "";


This concatenation ends with . "", which is a no-op and makes the SQL assembly harder to read. Consider removing the trailing empty-string concatenation.

Suggested change

$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . "";

$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter'));

Copilot · 2026-04-09T00:52:00Z

thold_graph.php


 	if (get_request_var('rfilter') != '') {
-		$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE '" . get_request_var('rfilter') . "'";
+		$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . "";


This concatenation ends with . "", which is a no-op and makes the SQL assembly harder to read. Consider removing the trailing empty-string concatenation.

Suggested change

$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . "";

$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter'));

Copilot · 2026-04-09T00:52:00Z

notify_lists.php


 	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')) . "";


This concatenation ends with . "", which is a no-op and makes the SQL assembly harder to read. Consider removing the trailing empty-string concatenation.

Suggested change

$sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . "";

$sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter'));

somethingwithproof added 2 commits April 8, 2026 17:32

Copilot AI review requested due to automatic review settings April 9, 2026 00:48

Copilot started reviewing on behalf of somethingwithproof April 9, 2026 00:48 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

somethingwithproof closed this Apr 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(thold): correct trigger_cmd variable references for low/normal states#765

fix(thold): correct trigger_cmd variable references for low/normal states#765
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:fix/bug-batch

somethingwithproof commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Copilot AI Apr 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	$sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . "";
	$sql_where .= ($sql_where == '' ? '(' : ' AND ') . " td.name_cache RLIKE " . db_qstr(get_request_var('rfilter'));

	$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter')) . "";
	$sql_where .= ($sql_where == '' ? '' : ' AND') . " tl.description RLIKE " . db_qstr(get_request_var('rfilter'));

	$sql_where .= (!strlen($sql_where) ? '' : ' AND ') . "td.name_cache RLIKE " . db_qstr(get_request_var('rfilter')) . "";
	$sql_where .= (!strlen($sql_where) ? '' : ' AND ') . 'td.name_cache RLIKE ' . db_qstr(get_request_var('rfilter'));

Conversation

somethingwithproof commented Apr 9, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants