hardening: prepared statements, PHP 7.4 idioms, and security fixes#49
hardening: prepared statements, PHP 7.4 idioms, and security fixes#49somethingwithproof wants to merge 2 commits into
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the maint plugin’s UI and runtime schedule evaluation by modernizing a few PHP idioms, escaping user-controlled output in the schedules list, and adding a guard to prevent pathological recurring-schedule recalculation.
Changes:
- Replace
isset(...) ? ... : ...with??in the schedule edit form field values. - Escape schedule names passed into
form_checkbox_cell()in the schedules list. - Prevent an infinite loop in recurring schedule rollover when
minterval <= 0by returning early.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| maint.php | Uses ?? for form values and escapes schedule name in the schedules table checkbox label. |
| functions.php | Adds a minterval guard to prevent infinite looping when rolling recurring schedules forward. |
| // minterval=0 would produce a zero-duration DateInterval and loop forever (FIND-004) | ||
| if ($sc['minterval'] <= 0) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The PR description claims these are purely mechanical/“zero behavioral impact” changes and mentions prepared statements / LIKE hardening, but this new early return changes runtime behavior for recurring schedules with invalid minterval (<=0) and the diff doesn’t show any SQL parameterization changes. Please update the PR description (or include the missing changes) so it accurately reflects what’s being merged.
| // past, calculate next | ||
| if ($sc['etime'] < $t) { | ||
| // minterval=0 would produce a zero-duration DateInterval and loop forever (FIND-004) | ||
| if ($sc['minterval'] <= 0) { |
There was a problem hiding this comment.
Returning false on minterval <= 0 prevents an infinite loop (good), but it fails silently and leaves the schedule in an expired state with no indication to admins why recurring maintenance stopped. Consider logging a warning (and/or auto-disabling the schedule) when this misconfiguration is detected so it’s observable and actionable.
| if ($sc['minterval'] <= 0) { | |
| if ($sc['minterval'] <= 0) { | |
| cacti_log('WARNING: Maintenance schedule "' . $sc['name'] . '" (ID ' . $schedule . ') has invalid recurring interval "' . $sc['minterval'] | |
| . '" and cannot be advanced. Recurring maintenance will remain inactive until this schedule is corrected.', false, 'MAINT'); |
Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Consolidated hardening PR:
2 files changed, 9 insertions(+), 4 deletions(-)
All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.