hardening: migrate quicktree SQL helpers to prepared variants#12
hardening: migrate quicktree SQL helpers to prepared variants#12somethingwithproof wants to merge 3 commits intoCacti:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens the plugin_quicktree plugin by migrating selected SQL read helpers from raw query helpers to prepared-statement variants, and adds a small regression script to verify the migration patterns remain in place.
Changes:
- Convert
quicktree.phpgraph-tree list and max-sequence lookups todb_fetch_*_prepared. - Convert
setup.phpplugin version lookup todb_fetch_cell_prepared. - Add
tests/test_prepared_statements.phpto statically verify prepared-helper usage and absence of raw helper calls in the touched files.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
quicktree.php |
Replaces specific raw read queries with prepared helper equivalents. |
setup.php |
Uses prepared helper for plugin version lookup during upgrade checks. |
tests/test_prepared_statements.php |
Adds regression checks (via regex) to enforce prepared-helper usage in updated files. |
💡 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.
| $quicktree_contents = file_get_contents(__DIR__ . '/../quicktree.php'); | ||
| $setup_contents = file_get_contents(__DIR__ . '/../setup.php'); |
There was a problem hiding this comment.
Good call — fixed in 98b88bf. Added explicit readability assertions for both files and only then run regex checks.
| assert_true( | ||
| 'quicktree.php uses prepared graph tree list query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT g\.id,\s*g\.name\s+FROM graph_tree/s', $quicktree_contents) === 1 | ||
| ); | ||
| assert_true( | ||
| 'quicktree.php uses prepared max sequence query', | ||
| preg_match('/db_fetch_cell_prepared\s*\(\s*\'SELECT MAX\(sequence\)\s+FROM graph_tree/s', $quicktree_contents) === 1 | ||
| ); | ||
| assert_true( | ||
| 'quicktree.php has no raw db_fetch_assoc calls', | ||
| preg_match('/\bdb_fetch_assoc\s*\(/', $quicktree_contents) === 0 | ||
| ); | ||
| 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 | ||
| ); |
There was a problem hiding this comment.
Addressed in 98b88bf: de-brittled SQL assertions to check prepared helper usage plus key query tokens (graph_tree, MAX(sequence), plugin_config, directory = ?) instead of exact SQL text/quoting.
somethingwithproof
changed the title
|
Incorporated follow-up review feedback in |
Summary
plugin_quicktreeSQL helper reads to prepared variantsquicktree.phpto prepared helperssetup.phpto prepared helperTests
php -l quicktree.phpphp -l setup.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpCloses #11