Skip to content

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

Open
somethingwithproof wants to merge 4 commits intoCacti:developfrom
somethingwithproof:hardening/comprehensive
Open

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

Conversation

@somethingwithproof
Copy link
Copy Markdown

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, 13 insertions(+), 14 deletions(-)

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

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Copilot AI review requested due to automatic review settings April 9, 2026 06:03
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR consolidates several PHP 7.4+ mechanical hardening changes (null coalescing operators, some shell escaping, and minor query hardening) across the HMIB plugin and SNMP helper code.

Changes:

  • Replace isset() ternaries / !isset() assignments with ?? / ??= in multiple files.
  • Add additional shell-escaping / numeric casting for CLI invocations.
  • Tighten one SQL update by casting an ID to integer.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
snmp.php Uses ??= for global defaults and adds numeric casting before invoking snmpget.
poller_hmib.php Replaces isset() ternary with null coalescing for date parsing.
poller_graphs.php Escapes configured PHP binary path and base path used in CLI commands.
hmib.php Uses ??= for session-cached graph template/host lists.
hmib_types.php Casts host_type update ID to int and uses ?? when injecting form variables.
Comments suppressed due to low confidence (1)

hmib_types.php:396

  • This UPDATE query still concatenates $known['sysObjectID'] and $known['sysDescrMatch'] directly into LIKE/RLIKE expressions without quoting/escaping, which leaves a SQL injection vector if those fields contain quotes (these values are user-controlled via the Host Types UI/import). Use db_execute_prepared with placeholders (building the LIKE patterns in parameters) and ensure the RLIKE patterns are passed as parameters/escaped appropriately.
					db_execute('UPDATE plugin_hmib_hrSystem SET host_type=' . (int) $known['id'] . "
						WHERE host_type=0 AND (sysObjectID LIKE '%" . $known['sysObjectID'] . "%' AND
						sysDescr LIKE '%" . $known['sysDescrMatch'] . "%')
						OR (sysObjectID RLIKE '" . $known['sysObjectID'] . "' AND
						sysDescr RLIKE '" . $known['sysDescrMatch'] . "')");

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread snmp.php
Comment on lines +133 to +135
$version = (int) $version;
$timeout = (int) $timeout;
$retries = (int) $retries;
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Casting $version to int here breaks the net-snmp binary invocation for v2 because earlier code intentionally sets $version = '2c'. After the cast, '-v 2c' becomes '-v 2', which can change behavior or fail depending on the snmpget implementation. Consider leaving $version as the validated/whitelisted string (e.g., '1','2c','3') and only casting $timeout/$retries (and possibly $port) to int for injection hardening.

Suggested change
$version = (int) $version;
$timeout = (int) $timeout;
$retries = (int) $retries;
$timeout = (int) $timeout;
$retries = (int) $retries;
$port = (int) $port;

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Valid. The int cast is from pre-existing code, not introduced by this PR. Will investigate the v2c handling separately.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants