hardening: run the test suite, and add CodeQL - #229
Open
somethingwithproof wants to merge 12 commits into
Open
Conversation
Pin actions/checkout (v5), shivammathur/setup-php (v2) and actions/upload-artifact (v4) to full commit SHAs with a trailing # vX comment. Mutable tags let a compromised or force-pushed tag run arbitrary code in CI; pinning to an immutable SHA is the supply-chain-hardening best practice and matches the SHA-pinned checkout used in the core security workflow.
Keep 1.2.31 pin while covering mid-stream PHP releases. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
The plugin shipped three test files that nothing ran: the workflow had no reference to tests/ at all, so a suite covering request-variable output escaping sat unexecuted. Adopts the Pest-through-composer pattern from thold and mactrack, with a bootstrap that stubs Cacti so plugin code loads without a full install. Two of the three existing files were meaningful grep guards against raw request reuse and are kept as Pest datasets; the third only asserted that htmlspecialchars() works. Pest 2 rather than mactrack's Pest 1, which no longer runs on current PHP. The PHP 8.0 runtime floor is enforced by the syntax-floor job rather than a composer platform pin, since the dev tooling needs 8.1. Stacked on Cacti#225, so the SHA pinning and permissions block are inherited rather than duplicated. Dependabot is left to Cacti#226. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the plugin’s CI posture by introducing an executable PHP test suite (via Pest + a lightweight Cacti bootstrap stub), adding a PHP 8.0 syntax-floor job to enforce the declared minimum runtime compatibility, and enabling CodeQL scanning for the plugin’s JavaScript/TypeScript.
Changes:
- Replace the previously standalone/CLI-style test scripts under
tests/with a Pest-based suite and atests/bootstrap.phpCacti stub so plugin code can load without a full Cacti install. - Extend the existing CI workflow to run
composer testand add a separate PHP 8.0 syntax lint job. - Add a CodeQL workflow (JS/TS only) for scheduled and PR scanning.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_request_output_escaping.php | Removes the old standalone escaping check script (superseded by Pest coverage). |
| tests/Pest.php | Adds global Pest hooks to reset request/settings/sql test globals before each test. |
| tests/Integration/test_monitor_request_output_wiring.php | Removes old grep-guard style script (replaced by Pest datasets). |
| tests/Integration/OutputEscapingTest.php | Adds Pest datasets asserting request output escaping and preventing raw request concatenation regressions. |
| tests/e2e/test_monitor_no_raw_request_reuse.php | Removes old standalone “raw request reuse” script (replaced by Pest datasets). |
| tests/bootstrap.php | Adds Cacti framework stubs for test execution without a full Cacti install. |
| phpunit.xml.dist | Adds PHPUnit/Pest bootstrap configuration targeting the Integration test directory. |
| composer.json | Introduces Composer metadata and Pest as a dev dependency with composer test scripts. |
| .gitignore | Ignores composer artifacts/caches related to the new test tooling. |
| .github/workflows/plugin-ci-workflow.yml | Adds a unit-test job and a PHP 8.0 syntax-floor job; refactors parts of integration job setup. |
| .github/workflows/codeql.yml | Adds CodeQL analysis workflow for JavaScript/TypeScript. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+45
to
+53
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: monitor-ci-${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| permissions: | ||
| contents: read |
Comment on lines
109
to
112
| services: | ||
| mariadb: | ||
| image: mariadb:10.6 | ||
| image: mariadb:10.11 | ||
| env: |
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.
Stacked on #225, which supplies the workflow modernisation, the SHA pinning and the
permissions:block. This adds only what is missing from it.The gap this closes
The plugin shipped three test files that nothing ran. The workflow contained no reference to
tests/at all, so a suite covering request-variable output escaping sat unexecuted since it was written.Testing
Adopts the pattern the other Cacti plugins already use: Pest through composer, as in
plugin_tholdandplugin_mactrack, with atests/bootstrap.phpthat stubs the Cacti framework so plugin code loads without a full install.Two of the three existing files were meaningful grep guards against raw request reuse and are kept as Pest datasets. The third only asserted that
htmlspecialchars()works and is dropped.Pest 2 rather than mactrack's Pest 1, which no longer runs on current PHP: it fails on 8.4 with implicit-nullable deprecations.
There is no
config.platformpin. It would hold the dev tooling to the plugin's PHP 8.0 runtime floor, which Pest 2 cannot meet. The floor is enforced by the newsyntax-floorjob instead, which is where it belongs.composer.lockis not committed, matching thold.What this adds to #225
unit-test, runningcomposer test; ci: pin integration tests to Cacti 1.2.31 #225 has no test jobsyntax-floor, linting every file at PHP 8.0, the floorcompat = 1.2.15implies. The existing matrix starts at 8.1 and cannot catch syntax the declared minimum rejectscodeql.yml, matchingplugin_auditandplugin_syslog. It analysesjavascript-typescriptonly, which is correct rather than an oversight: CodeQL has no PHP analyser, so the PHP is covered by the syntax and integration jobscomposer install --no-plugins --no-scripts, since the tree carries no lockfileDependabot is deliberately absent: #226 covers it.
What these tests do not do
The escaping tests match source text rather than running the pages. They detect an
html_escapecall being deleted; they prove nothing about runtime behaviour. The pageschdir()to the Cacti root and includeauth.php, so exercising them needs a live install. That limitation is stated in the file rather than left for a reader to find.Verification
PHPStan reports 209 errors on the two files this touches, both before and after; no new findings are introduced.