Skip to content

hardening: migrate gexport SQL helpers to prepared variants#67

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

hardening: migrate gexport SQL helpers to prepared variants#67
somethingwithproof wants to merge 3 commits intoCacti:developfrom
somethingwithproof:fix/prepared-sql-66

Conversation

@somethingwithproof
Copy link

Summary

  • migrate selected plugin_gexport SQL helper call sites to prepared variants
  • convert export deletion action to parameterized IN (...) prepared delete
  • convert list record/count and running-state queries to prepared fetch helpers
  • convert setup poller and plugin version lookups to prepared helpers
  • parameterize site/tree name aggregation queries from validated ID lists

Tests

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

Closes #66

Copilot AI review requested due to automatic review settings March 15, 2026 23:15
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 plugin_gexport database access paths by migrating remaining raw SQL helper call sites (db_fetch_*, db_execute) to prepared/parameterized variants to reduce SQL injection risk and standardize query patterns.

Changes:

  • Updated setup.php poller + upgrade checks to use db_fetch_*_prepared helpers.
  • Converted gexport.php export deletion to a parameterized IN (...) prepared delete and migrated list/count + running-state queries to prepared fetch helpers.
  • Added a lightweight regression script to verify prepared helper usage in touched files.

Reviewed changes

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

File Description
tests/test_prepared_statements.php Adds regression checks to ensure prepared helper usage is present at key call sites.
setup.php Replaces raw exports query and plugin version lookup with prepared variants.
gexport.php Parameterizes bulk delete, count/list queries, running-state query, and site/tree name aggregation queries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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


$setup_contents = file_get_contents(__DIR__ . '/../setup.php');
$gexport_contents = file_get_contents(__DIR__ . '/../gexport.php');

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in 2361e7e: added explicit readability assertions for both source files and only then run regex/substring assertions.

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

somethingwithproof commented Mar 16, 2026

Incorporated follow-up feedback in 3f6b043. Added follow-up checks for guarded bulk-delete behavior and placeholder construction from export-id count.

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 gexport SQL helpers to prepared variants

2 participants