hardening: migrate monitor SQL helpers to prepared variants#208
hardening: migrate monitor SQL helpers to prepared variants#208somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Hardens the monitor plugin by migrating a few remaining raw SQL helper call sites in setup.php to prepared-statement variants, plus adds a lightweight regression script intended to prevent reintroducing those raw patterns.
Changes:
- Switched plugin version lookup in
monitor_check_upgrade()todb_fetch_cell_prepared()with a bound placeholder. - Switched uninstall table drops and
monitor_device_remove()cleanup deletes todb_execute_prepared(), using generatedIN (?, ?, ...)placeholders for multi-device removal. - Added a standalone PHP regression script under
tests/that string-checks for prepared helper usage insetup.php.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| setup.php | Replaces selected raw SQL calls with prepared helper variants; adds placeholder-based IN (...) deletes. |
| tests/test_prepared_statements.php | Adds a standalone regression script that asserts prepared-helper usage patterns in setup.php. |
You can also share your feedback on Copilot code review. Take the survey.
| $setup = file_get_contents(__DIR__ . '/../setup.php'); | ||
| if ($setup === false) { | ||
| fwrite(STDERR, "Unable to read setup.php\n"); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
Addressed in 0e8344b: added a CI step in plugin-ci-workflow.yml to execute tests/test_*.php when present.
setup.php
Outdated
| db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_notify_history'); | ||
| db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_reboot_history'); | ||
| db_execute_prepared('DROP TABLE IF EXISTS plugin_monitor_uptime'); |
There was a problem hiding this comment.
Good catch — updated in 0e8344b to pass explicit empty bindings ([]) for those prepared uninstall calls.
tests/test_prepared_statements.php
Outdated
| "db_fetch_cell_prepared(\n\t\t'SELECT version", | ||
| 'Expected plugin version lookup to use db_fetch_cell_prepared().' |
There was a problem hiding this comment.
Updated in 0e8344b to use less brittle assertions (regex/tolerant matching) rather than whitespace-sensitive exact strings.
| 'db_execute(\'DELETE FROM plugin_monitor_notify_history WHERE host_id IN(\' . implode(\',\', $devices) . \')\');', | ||
| 'Raw notify-history delete should not remain.' | ||
| ); | ||
|
|
There was a problem hiding this comment.
Addressed in 0e8344b: added negative assertions for reboot and uptime raw-delete patterns as well.
|
Incorporated follow-up feedback in |
Summary
Implements issue #207 by migrating selected raw monitor SQL helpers to prepared variants in
setup.php.Changes
monitor_check_upgrade()todb_fetch_cell_prepared()plugin_monitor_uninstall()todb_execute_prepared()monitor_device_remove()deletes to placeholder-based prepared statements usingIN (...)tests/test_prepared_statements.phpValidation
php -l setup.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpIssue
Closes #207