Develop

[TheWitness]

No description provided.

[Copilot]

🟡 Changes recommended

Unresolved credential exposure and CI, compatibility, reproducibility, and test-execution issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request hardens the Evidence plugin’s uninstall, redirect, and HTML output handling while adding automated security, integration, and CI coverage.

Changes:

• Adds prepared uninstall statements and safe redirect termination.
• Escapes rendered output and centralizes tab-page rendering.
• Adds Pest tests, CI integration, documentation, and repository guidance.

File summaries

File	Summary
tests/Unit/FilterOutputEscapingTest.php	Tests HTML escaping.
tests/TestCase.php	Adds a reusable test base class.
tests/test_tab_wrapper.php	Tests tab-wrapper behavior; nit (2 votes): this check is not executed by CI.
tests/Security/UninstallPreparedStatementsTest.php	Tests prepared uninstall statements.
tests/Security/SetupStructureTest.php	Adds setup-structure security checks.
tests/Security/RedirectSafetyTest.php	Tests redirect safety.
tests/Security/PreparedStatementConsistencyTest.php	Checks prepared-statement consistency.
tests/Security/Php74CompatibilityTest.php	Adds PHP compatibility checks; moderate (1 vote): the no-op assertion always passes.
tests/Security/OutputEscapingTest.php	Tests output escaping.
tests/Security/AuthGuardTest.php	Tests authentication guards.
tests/Pest.php	Adds Pest configuration.
tests/Integration/EvidenceTabEscapingTest.php	Tests escaped tab output.
tests/E2E/EvidenceFilterXssRegressionTest.php	Adds filter XSS regression coverage.
tests/bootstrap-unit.php	Bootstraps Cacti/Pest tests.
tests/.cacti-version	Records the expected Cacti version.
setup.php	Uses prepared table drops during uninstall.
README.md	Updates plugin documentation.
phpunit.xml	Configures suites and coverage; nit (1 vote): the standalone tab-wrapper test is not discovered.
index.php	Terminates after redirects.
include/ui_helpers.php	Adds shared tab rendering.
images/index.php	Terminates after redirects.
evidence_tab.php	Uses shared rendering and escapes output.
data/index.php	Terminates after redirects.
CHANGELOG.md	Updates changelog history; nit (1 vote): current changes are not documented.
.github/workflows/plugin-ci-workflow.yml	Adds CI integration; critical (2 votes): removes credential-printing diagnostics. Moderate findings (1 vote each) cover missing coverage-driver setup, the PHP 8.2/Pest 3 mismatch, and deleting the generated lockfile.
.github/copilot-instructions.md	Adds development guidance.
.gitattributes	Normalizes line endings.

Review details

Suppressed comments (7)

.github/workflows/plugin-ci-workflow.yml:114

• MYSQL_AUTH_USR stores ~ inside an environment variable. Bash does not perform tilde expansion after parameter expansion, so mysql $MYSQL_AUTH_USR passes the literal --defaults-file=~/.my.cnf; the database-initialization step can fail before the schema is loaded. Invoke mysql with --defaults-file="$HOME/.my.cnf" directly or provide an absolute path.

      env:
        MYSQL_AUTH_USR: '--defaults-file=~/.my.cnf'

.github/workflows/plugin-ci-workflow.yml:83

• The workflow requests a Clover coverage report below, but this PHP setup does not enable Xdebug or PCOV. PHPUnit will fail with no available coverage driver on these jobs; add a coverage driver (for example coverage: pcov) or remove the coverage option from the Pest command.

        extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring, snmp
        ini-values: "post_max_size=256M, max_execution_time=60, date.timezone=America/New_York"

.github/workflows/plugin-ci-workflow.yml:138

• Pest 3 requires PHP 8.3+, but this matrix still runs PHP 8.2 (line 44), so dependency resolution fails for that job before the tests run. Use a Pest release compatible with PHP 8.2 or remove/update the 8.2 matrix entry.

          composer require --dev --no-progress --no-interaction "pestphp/pest: ^3" "pestphp/pest-plugin-drift: ^3.0"

.github/workflows/plugin-ci-workflow.yml:139

• Deleting composer.lock immediately after composer require makes every CI run resolve the full dependency graph from moving package metadata instead of using a lockfile. A future transitive release can break all PHP jobs without any repository change; keep the lockfile produced by composer require or pin a reproducible dependency set.

          rm -f composer.lock

CHANGELOG.md:10

• This PR changes uninstall behavior, output escaping, redirects, and the test/CI setup, but the changelog only contains the historical 0.1–0.3 entries. The release notes therefore omit the current changes; add a current entry (and align the plugin version if this is intended as a release).

--- 0.3 ---
* Add generic snmp info

--- 0.2 ---
* Better data display

--- 0.1 ---
* Beginning

phpunit.xml:20

• tests/test_tab_wrapper.php is neither under these configured directories nor named with the standard *Test.php suffix. The CI invocation therefore will not execute this standalone wrapper check, leaving the new rendering helper without the intended regression test; convert it to a discovered Pest test or explicitly run it in CI.

            <directory>tests/Security</directory>
            <directory>tests/Unit</directory>
            <directory>tests/Integration</directory>
            <directory>tests/E2E</directory>

tests/Security/Php74CompatibilityTest.php:107

• This branch intentionally contains no assertion, and the only assertion after the loop is unconditionally true, so the test passes regardless of whether mixed array syntax is present. It provides false confidence under the stated test name; either enforce the condition or remove this no-op test.

			if ($hasArrayFunc && $hasShortArray) {
				// Allow mixed if the file existed before our changes
				// This is informational, not a hard fail
			}

• Files reviewed: 27/27 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.