Skip to content

hardening: migrate gexport SQL helpers to prepared variants #67

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 gexport SQL helpers to prepared variants #67

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
2fb549b
security: migrate gexport SQL helpers to prepared variants
somethingwithproof Mar 15, 2026
2361e7e
test: guard gexport prepared regression file reads
somethingwithproof Mar 16, 2026
3f6b043
test: add grok follow-up guard checks for gexport deletes
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
59 changes: 46 additions & 13 deletions gexport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,17 +181,22 @@ function export_form_actions() {
if (get_nfilter_request_var('drp_action') === '1') { /* delete */
/* do a referential integrity check */
if (sizeof($selected_items)) {
$export_ids = array();

foreach($selected_items as $export_id) {
/* ================= input validation ================= */
input_validate_input_number($export_id);
/* ==================================================== */

$export_ids[] = $export_id;
$export_ids[] = (int)$export_id;
}
}

if (isset($export_ids)) {
db_execute('DELETE FROM graph_exports WHERE ' . array_to_sql_or($export_ids, 'id'));
if (isset($export_ids) && cacti_sizeof($export_ids)) {
$placeholders = implode(',', array_fill(0, cacti_sizeof($export_ids), '?'));
db_execute_prepared("DELETE FROM graph_exports
WHERE id IN ($placeholders)",
$export_ids);
}
} elseif (get_nfilter_request_var('drp_action') === '2') { /* enable */
for ($i=0;($i<count($selected_items));$i++) {
Expand Down Expand Up @@ -663,31 +668,40 @@ function clearFilter() {

function get_export_records(&$total_rows, &$rows) {
/* form the 'where' clause for our main sql query */
$sql_params = array();

if (get_request_var('filter') != '') {
$sql_where = 'WHERE (name LIKE ' . db_qstr('%' . get_request_var('filter') . '%') . ')';
$sql_where = 'WHERE (name LIKE ?)';
$sql_params[] = '%' . get_request_var('filter') . '%';
} else {
$sql_where = '';
}

$total_rows = db_fetch_cell("SELECT COUNT(*) FROM graph_exports $sql_where");
$total_rows = db_fetch_cell_prepared("SELECT COUNT(*)
FROM graph_exports
$sql_where",
$sql_params);

$sql_order = get_order_string();
$sql_limit = ' LIMIT ' . ($rows*(get_request_var('page')-1)) . ',' . $rows;
$offset = ((int)$rows * ((int)get_request_var('page') - 1));
$sql_limit = ' LIMIT ' . $offset . ',' . (int)$rows;

return db_fetch_assoc("SELECT *
return db_fetch_assoc_prepared("SELECT *
FROM graph_exports
$sql_where
$sql_order
$sql_limit");
$sql_limit",
$sql_params);
}

function gexport() {
global $export_actions;

$running = db_fetch_cell('SELECT COUNT(*)
$running = db_fetch_cell_prepared('SELECT COUNT(*)
FROM graph_exports
WHERE export_pid > 0
AND status > 0');
AND status > 0',
array());

if ($running == 0) {
set_request_var('refresh', 99999999);
Expand Down Expand Up @@ -881,7 +895,17 @@ function gexport() {
form_selectable_cell(__('All Sites', 'gexport'), $export['id'], '', 'text-align:right');
} else {
if ($export['graph_site'] != '') {
$sites = db_fetch_cell('SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ", ") FROM sites WHERE id IN(' . $export['graph_site'] . ')');
$site_ids = array_values(array_filter(array_map('intval', preg_split('/\s*,\s*/', $export['graph_site'], -1, PREG_SPLIT_NO_EMPTY))));

if (cacti_sizeof($site_ids)) {
$placeholders = implode(',', array_fill(0, cacti_sizeof($site_ids), '?'));
$sites = db_fetch_cell_prepared("SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ', ')
FROM sites
WHERE id IN ($placeholders)",
$site_ids);
} else {
$sites = '';
}
} else {
$sites = '';
}
Expand All @@ -892,7 +916,17 @@ function gexport() {
form_selectable_cell(__('All Trees', 'gexport'), $export['id'], '', 'text-align:right');
} else {
if ($export['graph_tree'] != '') {
$trees = db_fetch_cell('SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ", ") FROM graph_tree WHERE id IN(' . $export['graph_tree'] . ')');
$tree_ids = array_values(array_filter(array_map('intval', preg_split('/\s*,\s*/', $export['graph_tree'], -1, PREG_SPLIT_NO_EMPTY))));

if (cacti_sizeof($tree_ids)) {
$placeholders = implode(',', array_fill(0, cacti_sizeof($tree_ids), '?'));
$trees = db_fetch_cell_prepared("SELECT GROUP_CONCAT(name ORDER BY name SEPARATOR ', ')
FROM graph_tree
WHERE id IN ($placeholders)",
$tree_ids);
} else {
$trees = '';
}
} else {
$trees = '';
}
Expand Down Expand Up @@ -937,4 +971,3 @@ function gexport() {

form_end();
}

11 changes: 8 additions & 3 deletions setup.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,10 @@ function gexport_poller_bottom() {

/* graph export */
if ($config['poller_id'] == 1) {
$exports = db_fetch_assoc('SELECT * FROM graph_exports WHERE enabled="on"');
$exports = db_fetch_assoc_prepared('SELECT *
FROM graph_exports
WHERE enabled = ?',
array('on'));
if (sizeof($exports)) {
$command_string = read_config_option('path_php_binary');
$extra_args = '-q "' . $config['base_path'] . '/plugins/gexport/poller_export.php"';
Expand All @@ -78,7 +81,10 @@ function gexport_check_upgrade() {

$info = plugin_gexport_version ();
$current = $info['version'];
$old = db_fetch_cell("SELECT version FROM plugin_config WHERE directory='gexport'");
$old = db_fetch_cell_prepared('SELECT version
FROM plugin_config
WHERE directory = ?',
array('gexport'));

if (cacti_version_compare($old,$current,'<')) {
if (api_plugin_is_enabled('gexport')) {
Expand Down Expand Up @@ -610,4 +616,3 @@ function gexport_draw_navigation_text($nav) {

return $nav;
}

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

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 2361e7e: added explicit readability assertions for both source files and only then run regex/substring assertions.

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

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

assert_true(
'setup.php uses prepared enabled exports query',
preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT \*\s+FROM graph_exports\s+WHERE enabled = \?/s', $setup_contents) === 1
);
assert_true(
'setup.php uses prepared plugin version lookup',
preg_match('/db_fetch_cell_prepared\s*\(\s*\'SELECT version\s+FROM plugin_config\s+WHERE directory = \?/s', $setup_contents) === 1
);
assert_true(
'gexport.php uses prepared bulk delete',
preg_match('/db_execute_prepared\s*\(\s*"DELETE FROM graph_exports\s+WHERE id IN \(/s', $gexport_contents) === 1
);
assert_true(
'gexport.php guards bulk delete on non-empty export id lists',
preg_match('/if\s*\(isset\(\$export_ids\)\s*&&\s*cacti_sizeof\(\$export_ids\)\s*\)/', $gexport_contents) === 1
);
assert_true(
'gexport.php builds export-id placeholders from item count',
strpos($gexport_contents, '$placeholders = implode(\',\', array_fill(0, cacti_sizeof($export_ids), \'?\'));') !== false
);
assert_true(
'gexport.php no longer uses array_to_sql_or for export delete',
strpos($gexport_contents, "array_to_sql_or(\$export_ids, 'id')") === false
);
assert_true(
'gexport.php uses prepared count and record fetch for list query',
preg_match_all('/db_fetch_(?:cell|assoc)_prepared\s*\(/', $gexport_contents) >= 4
);
assert_true(
'gexport.php parameterizes site/tree group concat lookups',
preg_match('/GROUP_CONCAT\(name ORDER BY name SEPARATOR \', \'\)\s+FROM sites\s+WHERE id IN \(\$placeholders\)/s', $gexport_contents) === 1 &&
preg_match('/GROUP_CONCAT\(name ORDER BY name SEPARATOR \', \'\)\s+FROM graph_tree\s+WHERE id IN \(\$placeholders\)/s', $gexport_contents) === 1
);

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

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