Skip to content

hardening: migrate audit SQL helpers to prepared variants#49

Open
somethingwithproof wants to merge 3 commits intoCacti:developfrom
somethingwithproof:fix/prepared-sql-48
Open

hardening: migrate audit SQL helpers to prepared variants#49
somethingwithproof wants to merge 3 commits intoCacti:developfrom
somethingwithproof:fix/prepared-sql-48

Conversation

@somethingwithproof
Copy link

Summary

  • migrate selected plugin_audit SQL helper call sites to prepared variants
  • convert audit filter list reads and export/list record queries in audit.php to prepared helper usage
  • parameterize dynamic filter clauses with shared audit_build_filter_sql() where/param builder
  • convert plugin version/config update and retention purge queries in setup.php to prepared variants

Tests

  • php -l audit.php
  • php -l setup.php
  • php -l tests/test_prepared_statements.php
  • php tests/test_prepared_statements.php

Closes #48

Copilot AI review requested due to automatic review settings March 15, 2026 23:21
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens the audit plugin by migrating remaining dynamic SQL helper call sites in runtime and setup paths to prepared-statement variants, reducing SQL injection risk while keeping existing UI behavior intact.

Changes:

  • Convert setup.php plugin-config reads/updates and retention purge delete to prepared DB helpers.
  • Convert audit.php filter/list/export queries to prepared DB helpers using a shared audit_build_filter_sql() where/params builder.
  • Add a lightweight regression script to assert the prepared-helper migrations remain in place.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
audit.php Reworks filter SQL building and converts log list/export queries to prepared DB helpers.
setup.php Converts version/config updates and retention purge delete to prepared DB helpers.
tests/test_prepared_statements.php Adds regex-based regression checks to guard against reintroducing raw DB helpers.

You can also share your feedback on Copilot code review. Take the survey.

Comment on lines 113 to 117
if ($class == 'all') {
if (!db_table_exists('alert_log', false, $rcnn_id)) {
$create = db_fetch_cell('SHOW CREATE TABLE autid_log');
$create = db_fetch_cell_prepared('SHOW CREATE TABLE autid_log', array());

db_execute($create, false, $rcnn_id);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great catch. Fixed in 0a07152: corrected the dependency check to audit_log and aligned SHOW CREATE TABLE audit_log to the actual plugin schema. Also added regression coverage in tests/test_prepared_statements.php.

@somethingwithproof somethingwithproof changed the title security: migrate audit SQL helpers to prepared variants hardening: migrate audit SQL helpers to prepared variants Mar 15, 2026
@somethingwithproof
Copy link
Author

somethingwithproof commented Mar 16, 2026

Incorporated follow-up review feedback in 9b48b03. Added follow-up checks for file readability and explicit parameter binding validation on version and retention prepared calls.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hardening: migrate audit SQL helpers to prepared variants

2 participants