From 19511e0a8f17cdebed511318c70ed3c0cdca1519 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 15:59:34 -0700 Subject: [PATCH 1/3] security: migrate cycle SQL helpers to prepared variants --- cycle.php | 14 +++++-- setup.php | 34 +++++++++++----- tests/test_prepared_statements.php | 65 ++++++++++++++++++++++++++++++ 3 files changed, 99 insertions(+), 14 deletions(-) create mode 100644 tests/test_prepared_statements.php diff --git a/cycle.php b/cycle.php index fc996d1..79d9601 100644 --- a/cycle.php +++ b/cycle.php @@ -73,7 +73,13 @@ function cycle_graphs() { $width = get_request_var('width'); $height = get_request_var('height'); - if (empty($tree_id)) $tree_id = db_fetch_cell('SELECT id FROM graph_tree ORDER BY name LIMIT 1'); + if (empty($tree_id)) { + $tree_id = db_fetch_cell_prepared('SELECT id + FROM graph_tree + ORDER BY name + LIMIT 1', + array()); + } if (empty($id)) $id = -1; /* get the start and end times for the graph */ @@ -165,10 +171,11 @@ function cycle() { $height = get_request_var('height'); if (empty($tree_id)) { - $tree_id = db_fetch_cell('SELECT id + $tree_id = db_fetch_cell_prepared('SELECT id FROM graph_tree ORDER BY name - LIMIT 1'); + LIMIT 1', + array()); } if (empty($id)) { @@ -401,4 +408,3 @@ function cycle() { bottom_footer(); } - diff --git a/setup.php b/setup.php index 46b0b14..8b99972 100644 --- a/setup.php +++ b/setup.php @@ -62,7 +62,10 @@ function cycle_check_upgrade () { $info = plugin_cycle_version(); $current = $info['version']; - $old = db_fetch_row("SELECT * FROM plugin_config WHERE directory='cycle'"); + $old = db_fetch_row_prepared('SELECT * + FROM plugin_config + WHERE directory = ?', + array('cycle')); if (cacti_sizeof($old) && $current != $old['version']) { /* if the plugin is installed and/or active */ @@ -78,22 +81,33 @@ function cycle_check_upgrade () { api_plugin_register_realm('cycle', 'cycle.php,cycle_ajax.php', 'Plugin -> Cycle Graphs', 1); /* get the realm id's and change from old to new */ - $user = db_fetch_cell("SELECT id FROM plugin_realms WHERE file='cycle.php'")+100; - $users = db_fetch_assoc('SELECT user_id FROM user_auth_realm WHERE realm_id=42'); + $user = db_fetch_cell_prepared('SELECT id + FROM plugin_realms + WHERE file = ?', + array('cycle.php')) + 100; + $users = db_fetch_assoc_prepared('SELECT user_id + FROM user_auth_realm + WHERE realm_id = ?', + array(42)); if (sizeof($users)) { foreach($users as $u) { - db_execute('INSERT INTO user_auth_realm - (realm_id, user_id) VALUES (' . $user . ', ' . $u['user_id'] . ') - ON DUPLICATE KEY UPDATE realm_id=VALUES(realm_id)'); - db_execute('DELETE FROM user_auth_realm - WHERE user_id=' . $u['user_id'] . ' - AND realm_id=' . $user); + db_execute_prepared('INSERT INTO user_auth_realm + (realm_id, user_id) VALUES (?, ?) + ON DUPLICATE KEY UPDATE realm_id=VALUES(realm_id)', + array($user, $u['user_id'])); + db_execute_prepared('DELETE FROM user_auth_realm + WHERE user_id = ? + AND realm_id = ?', + array($u['user_id'], $user)); } } } /* update the plugin information */ - $id = db_fetch_cell("SELECT id FROM plugin_config WHERE directory='cycle'"); + $id = db_fetch_cell_prepared('SELECT id + FROM plugin_config + WHERE directory = ?', + array('cycle')); /* remove legacy hook */ db_execute('DELETE FROM plugin_hooks WHERE name="cycle" AND hook="config_form"'); diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php new file mode 100644 index 0000000..98b461d --- /dev/null +++ b/tests/test_prepared_statements.php @@ -0,0 +1,65 @@ += 2 +); +assert_true( + 'cycle.php no longer uses raw tree-id db_fetch_cell lookup', + strpos($cycle_contents, "db_fetch_cell('SELECT id FROM graph_tree ORDER BY name LIMIT 1')") === false +); +assert_true( + 'setup.php uses prepared plugin_config row lookup', + preg_match('/db_fetch_row_prepared\s*\(\s*\'SELECT \*\s+FROM plugin_config/s', $setup_contents) === 1 +); +assert_true( + 'setup.php uses prepared plugin_realm lookup', + preg_match('/db_fetch_cell_prepared\s*\(\s*\'SELECT id\s+FROM plugin_realms/s', $setup_contents) === 1 +); +assert_true( + 'setup.php uses prepared user realm list lookup', + preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT user_id\s+FROM user_auth_realm/s', $setup_contents) === 1 +); +assert_true( + 'setup.php uses prepared realm insert/delete updates', + preg_match_all('/db_execute_prepared\s*\(/', $setup_contents) >= 3 +); +assert_true( + 'setup.php no longer concatenates user_id in SQL strings', + strpos($setup_contents, "WHERE user_id=' . \$u['user_id'] . '") === false +); + +echo "\n"; +echo "Results: $pass passed, $fail failed\n"; + +exit($fail > 0 ? 1 : 0); From ecb390d64978dd13535837410db189825db8944f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 17:11:40 -0700 Subject: [PATCH 2/3] fix: preserve cycle realm migration semantics and harden tests --- setup.php | 5 +++-- tests/test_prepared_statements.php | 12 +++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/setup.php b/setup.php index 8b99972..7246d81 100644 --- a/setup.php +++ b/setup.php @@ -85,10 +85,11 @@ function cycle_check_upgrade () { FROM plugin_realms WHERE file = ?', array('cycle.php')) + 100; + $legacy_realm_id = 42; $users = db_fetch_assoc_prepared('SELECT user_id FROM user_auth_realm WHERE realm_id = ?', - array(42)); + array($legacy_realm_id)); if (sizeof($users)) { foreach($users as $u) { db_execute_prepared('INSERT INTO user_auth_realm @@ -98,7 +99,7 @@ function cycle_check_upgrade () { db_execute_prepared('DELETE FROM user_auth_realm WHERE user_id = ? AND realm_id = ?', - array($u['user_id'], $user)); + array($u['user_id'], $legacy_realm_id)); } } } diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 98b461d..48df8b8 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -30,13 +30,19 @@ function assert_true($label, $value) { $cycle_contents = file_get_contents($cycle_file); $setup_contents = file_get_contents($setup_file); +assert_true('cycle.php is readable', $cycle_contents !== false); +assert_true('setup.php is readable', $setup_contents !== false); + +$cycle_contents = ($cycle_contents === false ? '' : $cycle_contents); +$setup_contents = ($setup_contents === false ? '' : $setup_contents); + assert_true( 'cycle.php uses prepared tree-id lookup', - preg_match_all('/db_fetch_cell_prepared\s*\(\s*\'SELECT id\s+FROM graph_tree/s', $cycle_contents) >= 2 + preg_match_all('/db_fetch_cell_prepared\s*\(\s*\'SELECT id\s+FROM graph_tree/s', $cycle_contents, $cycle_matches) >= 2 ); assert_true( 'cycle.php no longer uses raw tree-id db_fetch_cell lookup', - strpos($cycle_contents, "db_fetch_cell('SELECT id FROM graph_tree ORDER BY name LIMIT 1')") === false + preg_match('/db_fetch_cell\s*\(\s*[\'"]SELECT\s+id\s+FROM\s+graph_tree\s+ORDER\s+BY\s+name\s+LIMIT\s+1[\'"]/i', $cycle_contents) === 0 ); assert_true( 'setup.php uses prepared plugin_config row lookup', @@ -52,7 +58,7 @@ function assert_true($label, $value) { ); assert_true( 'setup.php uses prepared realm insert/delete updates', - preg_match_all('/db_execute_prepared\s*\(/', $setup_contents) >= 3 + preg_match_all('/db_execute_prepared\s*\(/', $setup_contents, $setup_execute_matches) >= 3 ); assert_true( 'setup.php no longer concatenates user_id in SQL strings', From 564c686315340793d694aaa74b34278d1493ae8e Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 15 Mar 2026 19:16:36 -0700 Subject: [PATCH 3/3] test: add grok follow-up migration parity checks for cycle --- tests/test_prepared_statements.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/test_prepared_statements.php b/tests/test_prepared_statements.php index 48df8b8..bf24bea 100644 --- a/tests/test_prepared_statements.php +++ b/tests/test_prepared_statements.php @@ -60,6 +60,13 @@ function assert_true($label, $value) { 'setup.php uses prepared realm insert/delete updates', preg_match_all('/db_execute_prepared\s*\(/', $setup_contents, $setup_execute_matches) >= 3 ); +assert_true( + 'setup.php preserves legacy realm migration semantics', + preg_match('/\$legacy_realm_id\s*=\s*42\s*;/', $setup_contents) === 1 + && preg_match('/INSERT INTO user_auth_realm\s*\(realm_id,\s*user_id\)\s*VALUES\s*\(\?,\s*\?\)/s', $setup_contents) === 1 + && preg_match('/DELETE FROM user_auth_realm[\s\S]*WHERE user_id = \?[\s\S]*AND realm_id = \?/s', $setup_contents) === 1 + && preg_match('/array\(\$u\[\'user_id\'\],\s*\$legacy_realm_id\)/', $setup_contents) === 1 +); assert_true( 'setup.php no longer concatenates user_id in SQL strings', strpos($setup_contents, "WHERE user_id=' . \$u['user_id'] . '") === false