hardening: migrate cycle SQL helpers to prepared variants#26
hardening: migrate cycle SQL helpers to prepared variants#26somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the cycle plugin’s DB usage by migrating several SQL helper calls to prepared-statement variants and adds a small regression script to ensure the migration sticks.
Changes:
- Replace raw
db_fetch_*/db_executecalls with prepared variants insetup.phpupgrade paths. - Convert default
graph_treelookups incycle.phptodb_fetch_cell_prepared. - Add
tests/test_prepared_statements.phpto assert prepared-helper usage via simple source-code checks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
cycle.php |
Switch default tree lookups to db_fetch_cell_prepared. |
setup.php |
Convert plugin/realm/user-realm queries to prepared helpers during upgrade/migration. |
tests/test_prepared_statements.php |
Add regression checks to detect reintroduction of raw DB helpers in key sites. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
setup.php
Outdated
| 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)); |
There was a problem hiding this comment.
Addressed in ecb390d: delete now targets the legacy realm id (42) after insert/update into the new realm, preserving intended migration behavior.
|
|
||
| $cycle_contents = file_get_contents($cycle_file); | ||
| $setup_contents = file_get_contents($setup_file); | ||
|
|
There was a problem hiding this comment.
Addressed in ecb390d: added explicit readability checks and safe fallbacks before regex assertions.
tests/test_prepared_statements.php
Outdated
| ); | ||
| 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 |
There was a problem hiding this comment.
Addressed in ecb390d: replaced the exact raw-tree string check with a regex pattern that tolerates formatting differences.
somethingwithproof
changed the title
|
Incorporated follow-up feedback in |
Summary
plugin_cycleSQL helper calls to prepared variantscycle.phpto prepared fetchessetup.phpto prepared helpersTests
php -l cycle.phpphp -l setup.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpCloses #25