hardening: prepared statements, PHP 7.4 idioms, and security fixes by somethingwithproof · Pull Request #766 · Cacti/plugin_thold

somethingwithproof · 2026-04-09T06:03:59Z

Consolidated hardening PR:

Migrate isset() ternary to null coalescing (??) operator
Migrate !isset() assign patterns to ??= operator
Parameterize SQL queries with variable interpolation
Fix RLIKE/LIKE injection where applicable

5 files changed, 146 insertions(+), 150 deletions(-)

All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot

Pull request overview

This PR aims to harden and modernize the thold plugin’s PHP by mechanically replacing isset() ternaries and !isset() assignment patterns with PHP 7.4+ null coalescing operators (??, ??=), primarily in UI form field defaults and a few helper paths.

Changes:

Replaced many isset(...) ? ... : ... patterns with ?? across threshold and template edit forms.
Replaced a couple of if (!isset(...)) { ... } patterns with ??=.
Minor refactors in notification list form handling to use ?? defaults.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
thold.php	Refactors form field default/value assignments to use `??` (notably notification/metadata fields).
thold_templates.php	Similar `??` refactors for template edit form fields; includes several default/value paths.
thold_functions.php	Updates log/notification helper string composition to use `??` and uses `??=` for `$value`.
notify_lists.php	Uses `??` in a couple of places when building saved state / injected form variables.
includes/arrays.php	Uses `??=` to default `$step` from config.

Copilot · 2026-04-09T06:08:14Z

thold_functions.php

+					$desc .= ' Trigger[' . plugin_thold_duration_convert($thold['data_template_id'], ($message['thold_fail_trigger'] ?? ''), 'alert', 'data_template_id') . ']';
+					$desc .= ' WarnHigh[' . ($message['thold_warning_hi'] ?? '') . ']';
+					$desc .= ' WarnLow[' . ($message['thold_warning_low'] ?? '') . ']';
 					$desc .= ' WarnTrigger[' . plugin_thold_duration_convert($thold['data_template_id'], (isset($message['thold_warning_fail_trigger']) ? $message['thold_fail_trigger'] : ''), 'alert', 'data_template_id') . ']';


In this WarnTrigger log entry, the conditional checks for thold_warning_fail_trigger but the value passed is thold_fail_trigger. This produces incorrect logging output for warning trigger duration. Use thold_warning_fail_trigger (and consider using ?? '' here for consistency with the rest of the hunk).

Suggested change

$desc .= ' WarnTrigger[' . plugin_thold_duration_convert($thold['data_template_id'], (isset($message['thold_warning_fail_trigger']) ? $message['thold_fail_trigger'] : ''), 'alert', 'data_template_id') . ']';

$desc .= ' WarnTrigger[' . plugin_thold_duration_convert($thold['data_template_id'], ($message['thold_warning_fail_trigger'] ?? ''), 'alert', 'data_template_id') . ']';

Copilot · 2026-04-09T06:08:14Z

thold_templates.php

 			'method'        => 'drop_array',
 			'array'         => $repeatarray,
 			'default'       => read_config_option('alert_repeat'),
 			'description'   => __('Repeat alert after this amount of time has pasted since the last alert.', 'thold'),


Typo in user-facing string: "has pasted since the last alert" should be "has passed since the last alert".

Suggested change

'description' => __('Repeat alert after this amount of time has pasted since the last alert.', 'thold'),

'description' => __('Repeat alert after this amount of time has passed since the last alert.', 'thold'),

Copilot · 2026-04-09T06:08:15Z

thold.php

@@ -2272,7 +2272,7 @@ function thold_edit() {
 			'friendly_name' => __('External ID', 'thold'),
 			'method'        => 'textbox',
 			'description'   => __('Enter an Eternal ID for this Threshold.', 'thold'),


Typo in user-facing string: "Enter an Eternal ID" should be "Enter an External ID" (matches the field label).

Suggested change

'description' => __('Enter an Eternal ID for this Threshold.', 'thold'),

'description' => __('Enter an External ID for this Threshold.', 'thold'),

Copilot · 2026-04-09T06:08:15Z

thold_templates.php

@@ -1797,7 +1797,7 @@ function template_edit() {
 			'friendly_name' => __('External ID', 'thold'),
 			'method'        => 'textbox',
 			'description'   => __('Enter an Eternal ID for this Thold Template.', 'thold'),


Typo in user-facing string: "Enter an Eternal ID" should be "Enter an External ID" (matches the field label).

Suggested change

'description' => __('Enter an Eternal ID for this Thold Template.', 'thold'),

'description' => __('Enter an External ID for this Thold Template.', 'thold'),

Copilot · 2026-04-09T06:08:15Z

thold_templates.php

 			'method'        => 'textbox',
 			'max_length'    => 3,
 			'size'          => 15,
 			'description'   => __('Number of consecutive times the Data Source must be in a breached condition for an alert to be raised.<br>Leave empty to use default value (Default: %s cycles', read_config_option('alert_bl_trigger'), 'thold'),


This description string appears to be missing a closing parenthesis (and/or closing markup) after "Default: %s cycles" which will render oddly in the UI.

Suggested change

'description' => __('Number of consecutive times the Data Source must be in a breached condition for an alert to be raised.<br>Leave empty to use default value (Default: %s cycles', read_config_option('alert_bl_trigger'), 'thold'),

'description' => __('Number of consecutive times the Data Source must be in a breached condition for an alert to be raised.<br>Leave empty to use default value (Default: %s cycles)', read_config_option('alert_bl_trigger'), 'thold'),

Copilot · 2026-04-09T06:08:15Z

thold_templates.php

 			'default'       => read_config_option('thold_warning_time_fail_trigger'),
 			'description'   => __('The number of times the Data Source must be in breach condition prior to issuing a warning.', 'thold'),
-			'value'         => isset($thold_data['time_warning_fail_trigger']) ? $thold_data['time_warning_fail_trigger'] : read_config_option('alert_trigger')
+			'value'         => $thold_data['time_warning_fail_trigger'] ?? read_config_option('alert_trigger')


The time_warning_fail_trigger field sets its default from thold_warning_time_fail_trigger, but when the value is absent it falls back to alert_trigger. This makes the displayed/defaulted value inconsistent with the configured default for this field; align the fallback with the default option.

Suggested change

'value' => $thold_data['time_warning_fail_trigger'] ?? read_config_option('alert_trigger')

'value' => $thold_data['time_warning_fail_trigger'] ?? read_config_option('thold_warning_time_fail_trigger')

Copilot · 2026-04-09T06:08:16Z

thold_templates.php

 			'array'         => $timearray,
 			'description'   => __('The amount of time in the past to check for Threshold breaches.', 'thold'),
-			'value'         => isset($thold_data['time_warning_fail_length']) ? $thold_data['time_warning_fail_length'] : (read_config_option('thold_time_fail_length') > 0 ? read_config_option('thold_warning_time_fail_length') : 1)
+			'value'         => $thold_data['time_warning_fail_length'] ?? (read_config_option('thold_time_fail_length') > 0 ? read_config_option('thold_warning_time_fail_length') : 1)


time_warning_fail_length falls back using a condition on thold_time_fail_length, but returns thold_warning_time_fail_length. This looks like a copy/paste error and can cause the warning default to depend on the alert setting; the condition should likely check the warning option as well.

Suggested change

'value' => $thold_data['time_warning_fail_length'] ?? (read_config_option('thold_time_fail_length') > 0 ? read_config_option('thold_warning_time_fail_length') : 1)

'value' => $thold_data['time_warning_fail_length'] ?? (read_config_option('thold_warning_time_fail_length') > 0 ? read_config_option('thold_warning_time_fail_length') : 1)

Copilot · 2026-04-09T06:08:16Z

includes/arrays.php

-if (!isset($step)) {
-	$step = read_config_option('poller_interval');
-}
+$step ??= read_config_option('poller_interval');


PR description mentions prepared statements / SQL parameterization and RLIKE/LIKE injection fixes, but the changes in this PR appear limited to ?? / ??= mechanical refactors in the touched files. Either update the PR description to match the actual changes, or include the intended SQL hardening changes in this PR.

Copilot · 2026-04-09T06:08:16Z

thold.php

 			'max_length'    => 128,
 			'description'   => __('This is the Warning Email subject that will be displayed in the Email after change to Warning (128 Char MAX).  Leave blank for the default.  There are several common replacement tags that may be used in include:<br>&#060PHASE&#062 &#060THRESHOLDVALUE&#062 &#060CURRENTVALUE&#062 &#060THRESHOLDNAME&#062 &#060DSNAME&#062 &#060BREACHUP&#062 &#060REALERT&#062 &#60HOSTNAME&#62 &#60LOCATION&#62 &#60SITE&#62', 'thold'),
-			'value'         => isset($thold_data['email_subject']) ? $thold_data['email_subject'] : ''
+			'value'         => $thold_data['email_subject'] ?? ''


The email_subject_warn form field is populated from $thold_data['email_subject'] instead of $thold_data['email_subject_warn']. This causes the Warning Subject UI to display (and potentially save) the Alert Subject value, overwriting the real warning subject.

Suggested change

'value' => $thold_data['email_subject'] ?? ''

'value' => $thold_data['email_subject_warn'] ?? ''

Copilot · 2026-04-09T06:08:16Z

thold_templates.php

 			'max_length'    => 128,
 			'description'   => __('This is the Warning subject that will be displayed in the Email (128 Char MAX).  Leave blank for the default.  There are several common replacement tags that may be used in include:<br>&#060PHASE&#062 &#060THRESHOLDVALUE&#062 &#060CURRENTVALUE&#062 &#060THRESHOLDNAME&#062 &#060DSNAME&#062 &#060BREACHUP&#062 &#060REALERT&#062 &#60HOSTNAME&#62', 'thold'),
-			'value'         => isset($thold_data['email_subject']) ? $thold_data['email_subject'] : ''
+			'value'         => $thold_data['email_subject'] ?? ''


The email_subject_warn form field is populated from $thold_data['email_subject'] instead of $thold_data['email_subject_warn']. This causes the Warning Subject UI to display (and potentially save) the Alert Subject value, overwriting the real warning subject.

Suggested change

'value' => $thold_data['email_subject'] ?? ''

'value' => $thold_data['email_subject_warn'] ?? ''

…rplate Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

refactor(php74): use null coalescing (??) and ??= operators

8727ab9

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

Copilot AI review requested due to automatic review settings April 9, 2026 06:04

Copilot started reviewing on behalf of somethingwithproof April 9, 2026 06:04 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

refactor: extract plugin_get_rows_per_page helper to DRY filter boile…

2f29161

…rplate Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

somethingwithproof force-pushed the hardening/comprehensive branch from e265860 to 2f29161 Compare April 9, 2026 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hardening: prepared statements, PHP 7.4 idioms, and security fixes#766

hardening: prepared statements, PHP 7.4 idioms, and security fixes#766
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:hardening/comprehensive

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!

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

	$desc .= ' WarnTrigger[' . plugin_thold_duration_convert($thold['data_template_id'], (isset($message['thold_warning_fail_trigger']) ? $message['thold_fail_trigger'] : ''), 'alert', 'data_template_id') . ']';
	$desc .= ' WarnTrigger[' . plugin_thold_duration_convert($thold['data_template_id'], ($message['thold_warning_fail_trigger'] ?? ''), 'alert', 'data_template_id') . ']';

	'description' => __('Repeat alert after this amount of time has pasted since the last alert.', 'thold'),
	'description' => __('Repeat alert after this amount of time has passed since the last alert.', 'thold'),

	'description' => __('Enter an Eternal ID for this Threshold.', 'thold'),
	'description' => __('Enter an External ID for this Threshold.', 'thold'),

	'description' => __('Number of consecutive times the Data Source must be in a breached condition for an alert to be raised.<br>Leave empty to use default value (Default: %s cycles', read_config_option('alert_bl_trigger'), 'thold'),
	'description' => __('Number of consecutive times the Data Source must be in a breached condition for an alert to be raised.<br>Leave empty to use default value (Default: %s cycles)', read_config_option('alert_bl_trigger'), 'thold'),

	'value' => $thold_data['time_warning_fail_trigger'] ?? read_config_option('alert_trigger')
	'value' => $thold_data['time_warning_fail_trigger'] ?? read_config_option('thold_warning_time_fail_trigger')

	'value' => $thold_data['email_subject'] ?? ''
	'value' => $thold_data['email_subject_warn'] ?? ''

Conversation

somethingwithproof commented Apr 9, 2026

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!

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