hardening: migrate uninstall drop statements to prepared - #25
Closed
somethingwithproof wants to merge 6 commits into
Closed
somethingwithproof wants to merge 6 commits into
somethingwithproof wants to merge 6 commits into
Conversation
somethingwithproof
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the plugin uninstall routine to use prepared-statement execution for table drops and adds a lightweight regression test to ensure raw db_execute() DROP TABLE statements don’t return.
Changes:
- Switched DROP TABLE calls in
plugin_evidence_uninstall()fromdb_execute()todb_execute_prepared(). - Added a PHP test script that inspects
setup.phpto enforce the prepared-statement usage.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
setup.php |
Replaces raw DROP TABLE executions with db_execute_prepared() in the uninstall routine. |
tests/test_prepared_statements.php |
Adds a simple string-based regression test to validate uninstall DROP statements use db_execute_prepared(). |
💡 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.
Member
Author
|
Incorporated follow-up feedback in |
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>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
marked this pull request as draft
April 11, 2026 00:09
Member
Author
|
Converted to draft to serialize the stack in this repo. Blocked by #23; will un-draft after that merges to avoid cross-PR merge conflicts. |
xmacan
approved these changes
Sep 11, 2026
TheWitness
added a commit
to TheWitness/plugin_evidence
that referenced
this pull request
Sep 11, 2026
Supersedes Cacti#25 ("hardening: migrate uninstall drop statements to prepared" by @somethingwithproof) — this branch carries forward that PR's prepared-statement hardening and test coverage, then updates the test/CI plumbing to match the model used by Cacti/plugin_thold, mirroring the same work already applied in PR Cacti#29: - Remove composer.json (no composer.lock was present): Pest/PHPUnit no longer come from a vendor tree local to this plugin. - Add tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php (adapted from thold). bootstrap-unit.php verifies the Cacti checkout in CI matches tests/.cacti-version, requires Cacti's own Composer vendor autoloader, and stubs the Cacti global functions plugin source expects. - Replace tests/bootstrap.php with tests/bootstrap-unit.php and update tests/Pest.php's comment to match. - Add phpunit.xml (none existed before) bootstrapping from tests/bootstrap-unit.php and covering tests/Security, tests/Unit, tests/Integration, and tests/E2E. - Add .github/copilot-instructions.md adapted from thold's for the Evidence plugin. - Add .github/workflows/php-unit-tests.yml modeled on thold's workflow: checks out this plugin plus a pinned Cacti runtime and test toolchain, builds a Docker test image, lints, and runs Pest with coverage.
Merged
1 task
Member
|
Superseded by #30. |
TheWitness
added a commit
that referenced
this pull request
Sep 12, 2026
… framework with Thold (#30) * security: migrate uninstall drop statements to prepared * fix: complete uninstall drop coverage and harden tests * fix: harden evidence uninstall drop path with IF EXISTS * test: expand security test coverage for hardening changes 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> * fix: escape quotes in Pest regex patterns Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): escape evidence filter output * test: align Evidence test framework with Thold model Supersedes #25 ("hardening: migrate uninstall drop statements to prepared" by @somethingwithproof) — this branch carries forward that PR's prepared-statement hardening and test coverage, then updates the test/CI plumbing to match the model used by Cacti/plugin_thold, mirroring the same work already applied in PR #29: - Remove composer.json (no composer.lock was present): Pest/PHPUnit no longer come from a vendor tree local to this plugin. - Add tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php (adapted from thold). bootstrap-unit.php verifies the Cacti checkout in CI matches tests/.cacti-version, requires Cacti's own Composer vendor autoloader, and stubs the Cacti global functions plugin source expects. - Replace tests/bootstrap.php with tests/bootstrap-unit.php and update tests/Pest.php's comment to match. - Add phpunit.xml (none existed before) bootstrapping from tests/bootstrap-unit.php and covering tests/Security, tests/Unit, tests/Integration, and tests/E2E. - Add .github/copilot-instructions.md adapted from thold's for the Evidence plugin. - Add .github/workflows/php-unit-tests.yml modeled on thold's workflow: checks out this plugin plus a pinned Cacti runtime and test toolchain, builds a Docker test image, lints, and runs Pest with coverage. * fix(tests): correct pre-existing test bugs surfaced by new CI run Adding phpunit.xml means these tests actually execute in CI for the first time; two of them had latent bugs that were never caught before: - AuthGuardTest.php checked tests/test_prepared_statements.php (a test helper script) instead of the plugin's real UI entry points (evidence.php, evidence_tab.php), causing a false-positive failure and a risky (zero-assertion) test once no matching line existed. - SetupStructureTest.php regexed setup.php's raw source for `'name' =>` / `'version' =>`, but those keys live in the INFO ini file that setup.php parses at runtime, not in its literal source text. Assert against the parsed INFO file instead, matching PR #29's approach. * chore: normalize line endings to LF and enforce via .gitattributes .github/copilot-instructions.md, .github/workflows/php-unit-tests.yml, tests/.cacti-version, tests/TestCase.php, and tests/bootstrap-unit.php were committed with CRLF line endings. Normalize them to LF to match the rest of the repo, and add .gitattributes (`* text=auto eol=lf`) so this doesn't regress. * fix(tests): address Copilot review feedback on PR #30 - PreparedStatementConsistencyTest.php: drop the non-existent tests/test_prepared_statements.php target and narrow the description/ scope to the setup.php uninstall/drop path it actually verifies, instead of implying (and failing to enforce) coverage of every plugin file's DB calls. - RedirectSafetyTest.php: scan the real redirect entry points (index.php, images/index.php, data/index.php) instead of setup.php, which has no header(Location) call at all. - index.php, images/index.php, data/index.php: follow the header("Location: ...") redirect with exit, so the test above passes for a real reason and the scripts can't fall through if code is later added after the redirect. - Php74CompatibilityTest.php: drop the dangling reference to the removed tests/test_prepared_statements.php. - Convert tests/test_prepared_statements.php (a standalone script never discovered by phpunit.xml or Pest's *Test.php convention, so its assertions never ran in CI) into tests/Security/UninstallPreparedStatementsTest.php, a proper Pest test with the same assertions. * chore: remove php-unit-tests.yml workflow from this PR * docs: merge copilot-instructions.md with main's structure * docs: bring in CHANGELOG.md and updated README.md from main * fix(tests): align phpunit.xml schema path and cacti version check with main * docs: clarify renaming plugin_evidence directory to evidence on install * ci: add plugin-ci-workflow.yml from main and run full test suite; docs: raise PHP floor to 8.2 * ci: run composer install without sudo to avoid root-owned vendor tree breaking Pest cache writes --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: Thomas Vincent <thomasvincent@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements issue #24 by migrating plugin uninstall drop-table statements to prepared DB helpers.
Changes
plugin_evidence_uninstall()insetup.php:db_execute("DROP TABLE ...")calls todb_execute_prepared('DROP TABLE ...')tests/test_prepared_statements.phpValidation
php -l setup.phpphp -l tests/test_prepared_statements.phpphp tests/test_prepared_statements.phpIssue
Closes #24