hardening: migrate maint SQL helpers to prepared variants#48
hardening: migrate maint SQL helpers to prepared variants#48somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR reduces SQL injection risk in the maint plugin by migrating remaining read-only db_fetch_assoc call sites in setup.php/maint.php to prepared-statement helper variants, and adds a small regression script to ensure those raw helper calls don’t reappear.
Changes:
- Convert the schedules query in
setup.phptodb_fetch_assoc_prepared. - Convert schedules/site/poller list queries in
maint.phptodb_fetch_assoc_prepared. - Add
tests/test_prepared_statements.phpto regression-check the prepared helper usage and absence of rawdb_fetch_assoc.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| tests/test_prepared_statements.php | Adds a regression script to detect raw db_fetch_assoc usage and confirm prepared helper usage in touched files. |
| setup.php | Migrates the schedules list query used by device actions to db_fetch_assoc_prepared. |
| maint.php | Migrates schedule and filter list queries to db_fetch_assoc_prepared. |
You can also share your feedback on Copilot code review. Take the survey.
| assert_true( | ||
| 'setup.php uses prepared schedules query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT id,\s*name,\s*enabled,\s*mtype,\s*stime,\s*etime,\s*minterval/s', $setup_contents) === 1 | ||
| ); | ||
| assert_true( | ||
| 'setup.php has no raw db_fetch_assoc calls', | ||
| preg_match('/\bdb_fetch_assoc\s*\(/', $setup_contents) === 0 | ||
| ); | ||
| assert_true( | ||
| 'maint.php uses prepared schedule list query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT \*\s+FROM plugin_maint_schedules/s', $maint_contents) === 1 | ||
| ); | ||
| assert_true( | ||
| 'maint.php uses prepared site list query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT id,\s*name\s+FROM sites/s', $maint_contents) === 1 | ||
| ); | ||
| assert_true( | ||
| 'maint.php uses prepared poller list query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT id,\s*name\s+FROM poller/s', $maint_contents) === 1 | ||
| ); |
There was a problem hiding this comment.
Updated in 80dd247: de-brittled prepared-statement tests to assert helper usage + key table tokens instead of exact SQL formatting/quoting.
setup.php
Outdated
| FROM plugin_maint_schedules | ||
| ORDER BY name'); | ||
| ORDER BY name', | ||
| array()); |
There was a problem hiding this comment.
Fixed in 80dd247: replaced the new empty array() params with short [] for consistency with file style.
maint.php
Outdated
| FROM plugin_maint_schedules | ||
| ORDER BY name'); | ||
| ORDER BY name', | ||
| array()); |
There was a problem hiding this comment.
Fixed in 80dd247: replaced the new empty array() params with short [] for consistency with file style.
maint.php
Outdated
| WHERE id IN (SELECT site_id FROM host) | ||
| ORDER BY name'); | ||
| ORDER BY name', | ||
| array()); |
There was a problem hiding this comment.
Fixed in 80dd247: replaced the new empty array() params with short [] for consistency with file style.
maint.php
Outdated
| $pollers = db_fetch_assoc_prepared('SELECT id, name | ||
| FROM poller | ||
| ORDER BY name'); | ||
| ORDER BY name', | ||
| array()); |
There was a problem hiding this comment.
Fixed in 80dd247: replaced the new empty array() params with short [] for consistency with file style.
| $setup_contents = file_get_contents(__DIR__ . '/../setup.php'); | ||
| $maint_contents = file_get_contents(__DIR__ . '/../maint.php'); | ||
|
|
||
| assert_true( | ||
| 'setup.php uses prepared schedules query', | ||
| preg_match('/db_fetch_assoc_prepared\s*\(\s*\'SELECT id,\s*name,\s*enabled,\s*mtype,\s*stime,\s*etime,\s*minterval/s', $setup_contents) === 1 | ||
| ); |
There was a problem hiding this comment.
Addressed in 80dd247: added explicit readability assertions for setup.php and maint.php before regex checks.
somethingwithproof
changed the title
|
Incorporated follow-up review feedback in |
Summary
plugin_maintread query call sites to prepared helper variantssetup.phptodb_fetch_assoc_preparedmaint.phptodb_fetch_assoc_prepareddb_fetch_assocusage in these filesTests
php -l setup.phpphp -l maint.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpCloses #47