Skip to content

hardening: migrate maint SQL helpers to prepared variants #48

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
somethingwithproof wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

hardening: migrate maint SQL helpers to prepared variants #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7ad6390
security: migrate maint SQL helpers to prepared variants
somethingwithproof Mar 15, 2026
80dd247
fix: address prepared migration review feedback
somethingwithproof Mar 16, 2026
3f8f963
test: add grok follow-up coverage checks for maint prepared lookups
somethingwithproof Mar 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions maint.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,9 +924,10 @@ function changemaintType () {
function schedules(): void {
global $actions, $maint_types, $maint_intervals, $yesno;

$schedules = db_fetch_assoc('SELECT *
$schedules = db_fetch_assoc_prepared('SELECT *
FROM plugin_maint_schedules
ORDER BY name');
ORDER BY name',
[]);

form_start('maint.php', 'chk');

Expand Down Expand Up @@ -1134,10 +1135,11 @@ function clearFilter() {
<option value='<?php print MAINT_HOST_FILTER_ANY ?>' <?php if (get_request_var('site_id') == MAINT_HOST_FILTER_ANY) {?> selected<?php }?>><?php print __('Any', 'maint'); ?></option>
<option value='<?php print MAINT_HOST_FILTER_NONE ?>' <?php if (get_request_var('site_id') == MAINT_HOST_FILTER_NONE) {?> selected<?php }?>><?php print __('None', 'maint'); ?></option>
<?php
$sites = db_fetch_assoc('SELECT id, name
$sites = db_fetch_assoc_prepared('SELECT id, name
FROM sites
WHERE id IN (SELECT site_id FROM host)
ORDER BY name');
ORDER BY name',
[]);

if (cacti_sizeof($sites)) {
foreach ($sites as $site) {
Expand All @@ -1158,9 +1160,10 @@ function clearFilter() {
<select id='poller_id'>
<option value='<?php print MAINT_HOST_FILTER_ANY ?>' <?php if (get_request_var('poller_id') == MAINT_HOST_FILTER_ANY) {?> selected<?php }?>><?php print __('Any', 'maint'); ?></option>
<?php
$pollers = db_fetch_assoc('SELECT id, name
$pollers = db_fetch_assoc_prepared('SELECT id, name
FROM poller
ORDER BY name');
ORDER BY name',
[]);

if (cacti_sizeof($pollers)) {
foreach ($pollers as $poller) {
Expand Down
5 changes: 3 additions & 2 deletions setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,9 +298,10 @@ function maint_device_action_prepare(array $save): array {
}

// Load schedules to choose from
$schedules = db_fetch_assoc('SELECT id, name, enabled, mtype, stime, etime, minterval
$schedules = db_fetch_assoc_prepared('SELECT id, name, enabled, mtype, stime, etime, minterval
FROM plugin_maint_schedules
ORDER BY name');
ORDER BY name',
[]);

$select = "<select name='maint_schedule_id' style='min-width:360px'>";

Expand Down
76 changes: 76 additions & 0 deletions tests/test_prepared_statements.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
<?php
/*
+-------------------------------------------------------------------------+
| Copyright (C) 2004-2026 The Cacti Group |
| |
| Regression checks for prepared DB helper migration in maint plugin |
| |
| Run: php tests/test_prepared_statements.php |
+-------------------------------------------------------------------------+
*/

$pass = 0;
$fail = 0;

function assert_true($label, $value) {
global $pass, $fail;

if ($value) {
echo "PASS $label\n";
$pass++;
} else {
echo "FAIL $label\n";
$fail++;
}
}

$setup_contents = file_get_contents(__DIR__ . '/../setup.php');
$maint_contents = file_get_contents(__DIR__ . '/../maint.php');

assert_true('setup.php is readable', $setup_contents !== false);
assert_true('maint.php is readable', $maint_contents !== false);

$setup_contents = ($setup_contents === false ? '' : $setup_contents);
$maint_contents = ($maint_contents === false ? '' : $maint_contents);

assert_true(
'setup.php uses prepared schedules query',
preg_match('/db_fetch_assoc_prepared\s*\(/', $setup_contents) === 1
&& preg_match('/\bplugin_maint_schedules\b/', $setup_contents) === 1
);
assert_true(
'setup.php uses prepared host description lookup',
preg_match('/db_fetch_row_prepared\s*\(\s*\'SELECT description FROM host WHERE id = \?/s', $setup_contents) === 1
);
Comment on lines +27 to +40
Copy link
Author

@somethingwithproof somethingwithproof Mar 16, 2026

Choose a reason for hiding this comment

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

Addressed in 80dd247: added explicit readability assertions for setup.php and maint.php before regex checks.

assert_true(
'setup.php has no raw db_fetch_assoc calls',
preg_match('/\bdb_fetch_assoc\s*\(/', $setup_contents) === 0
);
assert_true(
'setup.php has no raw db_fetch_row calls',
preg_match('/\bdb_fetch_row\s*\(/', $setup_contents) === 0
);
assert_true(
'maint.php uses prepared schedule list query',
preg_match('/db_fetch_assoc_prepared\s*\(/', $maint_contents) === 1
&& preg_match('/\bplugin_maint_schedules\b/', $maint_contents) === 1
);
assert_true(
'maint.php uses prepared site list query',
preg_match('/db_fetch_assoc_prepared\s*\(/', $maint_contents) === 1
&& preg_match('/\bFROM\s+sites\b/i', $maint_contents) === 1
);
assert_true(
'maint.php uses prepared poller list query',
preg_match('/db_fetch_assoc_prepared\s*\(/', $maint_contents) === 1
&& preg_match('/\bFROM\s+poller\b/i', $maint_contents) === 1
);
Comment on lines +36 to +67
Copy link
Author

@somethingwithproof somethingwithproof Mar 16, 2026

Choose a reason for hiding this comment

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

Updated in 80dd247: de-brittled prepared-statement tests to assert helper usage + key table tokens instead of exact SQL formatting/quoting.

assert_true(
'maint.php has no raw db_fetch_assoc calls',
preg_match('/\bdb_fetch_assoc\s*\(/', $maint_contents) === 0
);

echo "\n";
echo "Results: $pass passed, $fail failed\n";

exit($fail > 0 ? 1 : 0);