From 503209fccacb9c21064e3f83af6359fcb45c63cf Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 15:49:22 -0700 Subject: [PATCH 1/3] security: migrate apcupsd SQL helpers to prepared variants --- setup.php | 14 +++++-- tests/test_prepared_statements.php | 62 ++++++++++++++++++++++++++++++ upses.php | 24 ++++++++---- 3 files changed, 89 insertions(+), 11 deletions(-) create mode 100644 tests/test_prepared_statements.php diff --git a/setup.php b/setup.php index aa1e43b..dfc134b 100644 --- a/setup.php +++ b/setup.php @@ -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 @@ -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']); @@ -343,4 +350,3 @@ function apcupsd_draw_navigation_text($nav) { return $nav; } - diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php new file mode 100644 index 0000000..2071fd5 --- /dev/null +++ b/tests/test_prepared_statements.php @@ -0,0 +1,62 @@ += 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) >= 2 +); +assert_true( + 'upses.php no longer builds SQL with array_to_sql_or', + strpos($upses_contents, 'array_to_sql_or(') === false +); + +echo "\n"; +echo "Results: $pass passed, $fail failed\n"; + +exit($fail > 0 ? 1 : 0); diff --git a/upses.php b/upses.php index 95bde24..c8d1db9 100644 --- a/upses.php +++ b/upses.php @@ -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); } } @@ -680,11 +688,12 @@ function upses() { ':'>') . __('None', 'apcupsd');?> Date: Sun, 15 Mar 2026 17:09:00 -0700 Subject: [PATCH 2/3] fix: harden apcupsd prepared test checks --- tests/test_prepared_statements.php | 12 +++++++++--- upses.php | 2 +- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 2071fd5..477e2bc 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -30,9 +30,15 @@ function assert_true($label, $value) { $setup_contents = file_get_contents($setup_file); $upses_contents = file_get_contents($upses_file); +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 version/s', $setup_contents) === 1 + 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', @@ -40,7 +46,7 @@ function assert_true($label, $value) { ); assert_true( 'setup.php uses prepared replicate_out reads', - preg_match_all('/\bdb_fetch_assoc_prepared\s*\(/', $setup_contents) >= 2 + preg_match_all('/\bdb_fetch_assoc_prepared\s*\(/', $setup_contents, $setup_prepared_matches) >= 2 ); assert_true( @@ -49,7 +55,7 @@ function assert_true($label, $value) { ); assert_true( 'upses.php uses prepared action updates/deletes', - preg_match_all('/\bdb_execute_prepared\s*\(/', $upses_contents) >= 2 + 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', diff --git a/upses.php b/upses.php index c8d1db9..67ede44 100644 --- a/upses.php +++ b/upses.php @@ -692,7 +692,7 @@ function upses() { 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' ); From 9ebe2a708d5273112eed13d57513e9c76d21a9c3 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 19:16:34 -0700 Subject: [PATCH 3/3] test: add grok follow-up checks for apcupsd migration --- tests/test_prepared_statements.php | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 477e2bc..f69a706 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -61,6 +61,18 @@ function assert_true($label, $value) { '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";