Skip to content

hardening: migrate apcupsd SQL helpers to prepared variants #18

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 apcupsd SQL helpers to prepared variants #18

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
503209f
security: migrate apcupsd SQL helpers to prepared variants
somethingwithproof Mar 15, 2026
a077706
fix: harden apcupsd prepared test checks
somethingwithproof Mar 16, 2026
9ebe2a7
test: add grok follow-up checks for apcupsd migration
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
14 changes: 10 additions & 4 deletions setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,10 @@ function apcupsd_check_upgrade() {

$info = plugin_apcupsd_version();
$current = $info['version'];
$old = db_fetch_cell("SELECT version FROM plugin_config WHERE directory='apcupsd'");
$old = db_fetch_cell_prepared('SELECT version
FROM plugin_config
WHERE directory = ?',
array('apcupsd'));
if ($current != $old) {
if (api_plugin_is_enabled('apcupsd')) {
# may sound ridiculous, but enables new hooks
Expand Down Expand Up @@ -322,11 +325,15 @@ function apcupsd_replicate_out($data) {

include_once($config['base_path'] . '/lib/poller.php');

$upsdata = db_fetch_assoc('SELECT * FROM apcupsd_ups');
$upsdata = db_fetch_assoc_prepared('SELECT *
FROM apcupsd_ups',
array());

replicate_out_table($data['rcnn_id'], $upsdata, 'apcupsd_ups', $data['remote_poller_id']);

$upsdata = db_fetch_assoc('SELECT * FROM apcupsd_ups_stats');
$upsdata = db_fetch_assoc_prepared('SELECT *
FROM apcupsd_ups_stats',
array());

replicate_out_table($data['rcnn_id'], $upsdata, 'apcupsd_ups_stats', $data['remote_poller_id']);

Expand All @@ -343,4 +350,3 @@ function apcupsd_draw_navigation_text($nav) {

return $nav;
}

80 changes: 80 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,80 @@
<?php
/*
+-------------------------------------------------------------------------+
| Copyright (C) 2004-2026 The Cacti Group |
| |
| Regression checks for prepared DB helper migration in apcupsd 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_file = __DIR__ . '/../setup.php';
$upses_file = __DIR__ . '/../upses.php';

$setup_contents = file_get_contents($setup_file);
$upses_contents = file_get_contents($upses_file);
Comment on lines +30 to +31
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 follow-up commit a077706.


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

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

assert_true(
'setup.php uses prepared plugin_config version lookup',
preg_match('/db_fetch_cell_prepared\s*\(\s*[\'"]SELECT\s+version/s', $setup_contents) === 1
);
assert_true(
'setup.php has no raw db_fetch_cell calls',
preg_match('/\bdb_fetch_cell\s*\(/', $setup_contents) === 0
);
assert_true(
'setup.php uses prepared replicate_out reads',
preg_match_all('/\bdb_fetch_assoc_prepared\s*\(/', $setup_contents, $setup_prepared_matches) >= 2
);

assert_true(
'upses.php has no raw db_fetch_assoc calls',
preg_match('/\bdb_fetch_assoc\s*\(/', $upses_contents) === 0
);
assert_true(
'upses.php uses prepared action updates/deletes',
preg_match_all('/\bdb_execute_prepared\s*\(/', $upses_contents, $upses_prepared_matches) >= 2
);
assert_true(
'upses.php no longer builds SQL with array_to_sql_or',
strpos($upses_contents, 'array_to_sql_or(') === false
);
assert_true(
'upses.php guards bulk actions on non-empty selected items',
strpos($upses_contents, '$selected_items != false && cacti_sizeof($selected_items)') !== false
);
assert_true(
'upses.php derives IN-clause placeholders from selected item count',
strpos($upses_contents, '$selected_placeholders = implode(\',\', array_fill(0, cacti_sizeof($selected_items), \'?\'));') !== false
);
assert_true(
'upses.php has no remaining raw db helpers',
preg_match('/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)\s*\(/', $upses_contents) === 0
);

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

exit($fail > 0 ? 1 : 0);
24 changes: 17 additions & 7 deletions upses.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,13 +417,21 @@ function form_actions() {
if (isset_request_var('selected_items')) {
$selected_items = sanitize_unserialize_selected_items(get_nfilter_request_var('selected_items'));

if ($selected_items != false) {
if ($selected_items != false && cacti_sizeof($selected_items)) {
$selected_items = array_values($selected_items);
$selected_placeholders = implode(',', array_fill(0, cacti_sizeof($selected_items), '?'));

if (get_nfilter_request_var('drp_action') == '1') { /* delete */
db_execute('DELETE FROM apcupsd_ups WHERE ' . array_to_sql_or($selected_items, 'id'));
db_execute_prepared("DELETE FROM apcupsd_ups
WHERE id IN ($selected_placeholders)",
$selected_items);
} elseif (get_nfilter_request_var('drp_action') == '2') { /* Duplicate */
duplicate_ups($selected_items, get_nfilter_request_var('ups_name'));
} elseif (get_nfilter_request_var('drp_action') == '3') { /* Reset Detection */
db_execute('UPDATE apcupsd_ups SET snmp_skipped = "" WHERE ' . array_to_sql_or($selected_items, 'id'));
db_execute_prepared("UPDATE apcupsd_ups
SET snmp_skipped = ''
WHERE id IN ($selected_placeholders)",
$selected_items);
}
}

Expand Down Expand Up @@ -680,11 +688,12 @@ function upses() {
<option value='-2'<?php print (get_request_var('site_id') == '-2' ? ' selected>':'>') . __('None', 'apcupsd');?></option>
<?php
$sites = array_rekey(
db_fetch_assoc('SELECT s.id, s.name
db_fetch_assoc_prepared('SELECT s.id, s.name
FROM sites AS s
INNER JOIN apcupsd_ups AS u
ON s.id = u.site_id
ORDER BY name'),
ORDER BY s.name',
array()),
'id', 'name'
);

Expand All @@ -705,11 +714,12 @@ function upses() {
<option value='-2'<?php print (get_request_var('location') == '-2' ? ' selected>':'>') . __('None', 'apcupsd');?></option>
<?php
$locations = array_rekey(
db_fetch_assoc('SELECT DISTINCT h.location AS id, h.location AS name
db_fetch_assoc_prepared('SELECT DISTINCT h.location AS id, h.location AS name
FROM host AS h
INNER JOIN apcupsd_ups AS u
ON h.id = u.host_id
ORDER BY h.location'),
ORDER BY h.location',
array()),
'id', 'name'
);

Expand Down