hardening: prepared statements, PHP 7.4 idioms, and security fixes#51
hardening: prepared statements, PHP 7.4 idioms, and security fixes#51somethingwithproof wants to merge 3 commits intoCacti:developfrom
Conversation
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens the Cacti Audit plugin by reducing injection/XSS risk and modernizing a small PHP idiom, aligning with the plugin’s security posture around database access and UI output.
Changes:
- Convert the audit retention purge query to a prepared statement.
- Escape audit log “getdata” output fields to prevent HTML/JS injection in the UI.
- Modernize session user ID fallback using the null coalescing operator.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
setup.php |
Uses db_execute_prepared() for the retention-based purge query. |
audit.php |
Applies html_escape() to multiple fields when rendering audit record details. |
audit_functions.php |
Replaces isset() ternary with ?? for session user ID fallback. |
| $output .= '<span><b>' . __('Page:', 'audit') . '</b> <i>' . html_escape($data['page']) . '</i></span>'; | ||
| $output .= '<br><span><b>' . __('User:', 'audit') . '</b> <i>' . html_escape($data['user_agent']) . '</i></span>'; | ||
| $output .= '<br><span><b>' . __('IP Address:', 'audit') . '</b> <i>' . html_escape($data['ip_address']) . '</i></span>'; | ||
| $output .= '<br><span><b>' . __('Date:', 'audit') . '</b> <i>' . html_escape($data['event_time']) . '</i></span>'; | ||
| $output .= '<br><span><b>' . __('Action:', 'audit') . '</b> <i>' . html_escape($data['action']) . '</i></span>'; | ||
| $output .= '<hr>'; | ||
| $output .= '<span><b>' . __('Script:', 'audit') . '</b> <i>' . $data['post'] . '</i></span>'; | ||
| $output .= '<span><b>' . __('Script:', 'audit') . '</b> <i>' . html_escape($data['post']) . '</i></span>'; |
There was a problem hiding this comment.
PR description states the changes have "zero behavioral impact", but escaping values with html_escape() changes the rendered output (e.g., special characters will display escaped) and is a functional/security behavior change. Please update the PR description (or note this as an intentional behavior change) so reviewers/operators aren't surprised.
There was a problem hiding this comment.
Correct observation. The ?? operator preserves behavior when the value is set. The only behavioral difference is when the value is null, which previously would have triggered an undefined variable notice.
somethingwithproof
force-pushed
the
hardening/comprehensive
branch
from
8da31f9 to
59bbe87
Compare
Consolidated hardening PR:
3 files changed, 13 insertions(+), 13 deletions(-)
All changes are mechanical transforms with zero behavioral impact. PHP 7.4+ compatible.