test: add Pest v1 security test infrastructure#53
Draft
somethingwithproof wants to merge 5 commits intoCacti:developfrom
Draft
test: add Pest v1 security test infrastructure#53somethingwithproof wants to merge 5 commits intoCacti:developfrom
somethingwithproof wants to merge 5 commits intoCacti:developfrom
Conversation
Add source-scan tests verifying security patterns (prepared statements, output escaping, auth guards, PHP 7.4 compatibility) remain in place across refactors. Tests run with Pest v1 (PHP 7.3+) and stub the Cacti framework so plugins can be tested in isolation. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
There was a problem hiding this comment.
Pull request overview
Adds a Pest v1-based test scaffold for the Cacti audit plugin, focused on source-scanning security checks and basic structural/compatibility validation.
Changes:
- Introduces Pest bootstrap/config plus initial security/compatibility tests under
tests/Security/. - Adds a Composer dev-dependency on Pest to support running the new test suite.
- Adds GitHub metadata workflows/configs (CodeQL + Dependabot).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
tests/Security/SetupStructureTest.php |
Source-level assertions for required setup.php functions/keys. |
tests/Security/PreparedStatementConsistencyTest.php |
Source-scan intended to enforce prepared DB helper usage. |
tests/Security/Php74CompatibilityTest.php |
Source-scan for selected PHP 8+ symbols/operators to preserve PHP 7.4 compatibility. |
tests/Pest.php |
Pest entrypoint requiring the test bootstrap. |
tests/bootstrap.php |
Adds Cacti function stubs to allow tests to run without full Cacti. |
composer.json |
Adds Pest as a dev dependency and dev autoload bootstrap. |
.github/workflows/codeql.yml |
Adds CodeQL workflow (currently JS/TS only; PHP ignored). |
.github/dependabot.yml |
Adds Dependabot updates for npm and GitHub Actions. |
Comment on lines
+15
to
+16
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | ||
|
|
There was a problem hiding this comment.
$source is read via file_get_contents(realpath(...)) without checking that realpath() succeeded. If setup.php is missing or the path resolves to false, this will raise warnings/TypeErrors instead of producing a clear test failure; assert the resolved path exists/is readable before reading.
Suggested change
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | |
| $setup_path = realpath(__DIR__ . '/../../setup.php'); | |
| if ($setup_path === false || !is_readable($setup_path)) { | |
| throw new RuntimeException('Unable to read setup.php for structure test.'); | |
| } | |
| $source = file_get_contents($setup_path); | |
| if ($source === false) { | |
| throw new RuntimeException('Failed to load setup.php contents for structure test.'); | |
| } |
Comment on lines
+17
to
+25
| $targetFiles = array( | ||
| 'audit.php', | ||
| 'audit_functions.php', | ||
| 'setup.php', | ||
| ); | ||
|
|
||
| $rawPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)\s*\(/'; | ||
| $preparedPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)_prepared\s*\(/'; | ||
|
|
There was a problem hiding this comment.
This test currently treats any db_fetch_cell()/db_fetch_assoc()/db_fetch_row()/db_execute() as a failure unless it ends with _prepared. The plugin source currently contains such calls (including constant queries), so this will fail immediately; either migrate the targeted files in the same PR or narrow/allowlist the raw helpers that are acceptable (e.g., db_fetch_cell for constant queries).
Comment on lines
+29
to
+37
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
If realpath()/file_get_contents() fails, the test silently continues and can pass without checking anything. Consider failing the test when a target file can't be found/read so it can't mask packaging/path issues.
Suggested change
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect($path)->not->toBeFalse( | |
| "Unable to resolve test target file {$relativeFile}" | |
| ); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse( | |
| "Unable to read test target file {$relativeFile}" | |
| ); |
Comment on lines
+39
to
+53
| $lines = explode("\n", $contents); | ||
| $rawCallsOutsideComments = 0; | ||
|
|
||
| foreach ($lines as $line) { | ||
| $trimmed = ltrim($line); | ||
|
|
||
| if (strpos($trimmed, '//') === 0 || strpos($trimmed, '*') === 0 || strpos($trimmed, '#') === 0) { | ||
| continue; | ||
| } | ||
|
|
||
| if (preg_match($rawPattern, $line) && !preg_match($preparedPattern, $line)) { | ||
| $rawCallsOutsideComments++; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The comment skipping logic only ignores lines starting with //, , or #. It does not handle block comment starts like / ... / (or inline comments after code), which can cause false positives/negatives. Consider a simple block-comment state machine or skipping lines containing // */ appropriately.
Suggested change
| $lines = explode("\n", $contents); | |
| $rawCallsOutsideComments = 0; | |
| foreach ($lines as $line) { | |
| $trimmed = ltrim($line); | |
| if (strpos($trimmed, '//') === 0 || strpos($trimmed, '*') === 0 || strpos($trimmed, '#') === 0) { | |
| continue; | |
| } | |
| if (preg_match($rawPattern, $line) && !preg_match($preparedPattern, $line)) { | |
| $rawCallsOutsideComments++; | |
| } | |
| } | |
| $tokens = token_get_all($contents); | |
| $codeWithoutComments = ''; | |
| foreach ($tokens as $token) { | |
| if (is_array($token)) { | |
| if ($token[0] === T_COMMENT || $token[0] === T_DOC_COMMENT) { | |
| continue; | |
| } | |
| $codeWithoutComments .= $token[1]; | |
| } else { | |
| $codeWithoutComments .= $token; | |
| } | |
| } | |
| $rawCallsOutsideComments = preg_match_all($rawPattern, $codeWithoutComments, $rawMatches); | |
| $preparedCallsOutsideComments = preg_match_all($preparedPattern, $codeWithoutComments, $preparedMatches); | |
| if ($rawCallsOutsideComments === false) { | |
| $rawCallsOutsideComments = 0; | |
| } | |
| if ($preparedCallsOutsideComments === false) { | |
| $preparedCallsOutsideComments = 0; | |
| } | |
| $rawCallsOutsideComments -= $preparedCallsOutsideComments; |
Comment on lines
+22
to
+95
| it('does not use str_contains (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use str_starts_with (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use str_ends_with (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | ||
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files) { | ||
| foreach ($files as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
Each check silently continues when a file can't be resolved/read, which can make the suite pass while verifying nothing. Prefer asserting the file exists/is readable (and possibly de-duplicating the repeated path/contents loading into a helper).
Suggested change
| it('does not use str_contains (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_starts_with (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_ends_with (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files) { | |
| foreach ($files as $relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| $getFileContents = function (string $relativeFile): string { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| expect($path)->not->toBeFalse("Unable to resolve file path for {$relativeFile}"); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse("Unable to read file contents for {$relativeFile}"); | |
| return $contents; | |
| }; | |
| it('does not use str_contains (PHP 8.0)', function () use ($files, $getFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $getFileContents($relativeFile); | |
| expect(preg_match('/\bstr_contains\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_contains() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_starts_with (PHP 8.0)', function () use ($files, $getFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $getFileContents($relativeFile); | |
| expect(preg_match('/\bstr_starts_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_starts_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use str_ends_with (PHP 8.0)', function () use ($files, $getFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $getFileContents($relativeFile); | |
| expect(preg_match('/\bstr_ends_with\s*\(/', $contents))->toBe(0, | |
| "{$relativeFile} uses str_ends_with() which requires PHP 8.0" | |
| ); | |
| } | |
| }); | |
| it('does not use nullsafe operator (PHP 8.0)', function () use ($files, $getFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $getFileContents($relativeFile); |
Comment on lines
+18
to
+62
| function db_execute($sql) { | ||
| $GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute', 'sql' => $sql, 'params' => array()); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_execute_prepared')) { | ||
| function db_execute_prepared($sql, $params = array()) { | ||
| $GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute_prepared', 'sql' => $sql, 'params' => $params); | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_assoc')) { | ||
| function db_fetch_assoc($sql) { | ||
| return array(); | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_assoc_prepared')) { | ||
| function db_fetch_assoc_prepared($sql, $params = array()) { | ||
| return array(); | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_row')) { | ||
| function db_fetch_row($sql) { | ||
| return array(); | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_row_prepared')) { | ||
| function db_fetch_row_prepared($sql, $params = array()) { | ||
| return array(); | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_cell')) { | ||
| function db_fetch_cell($sql) { | ||
| return ''; | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('db_fetch_cell_prepared')) { | ||
| function db_fetch_cell_prepared($sql, $params = array()) { |
There was a problem hiding this comment.
These DB stubs don't match the call signatures used by the plugin (e.g., setup.php calls db_execute($sql, $transaction, $rcnn_id)). If future tests include/execute plugin code, this will fatal with argument count errors. Make these stubs accept optional/variadic parameters to be compatible with Cacti's helpers.
Suggested change
| function db_execute($sql) { | |
| $GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute', 'sql' => $sql, 'params' => array()); | |
| return true; | |
| } | |
| } | |
| if (!function_exists('db_execute_prepared')) { | |
| function db_execute_prepared($sql, $params = array()) { | |
| $GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute_prepared', 'sql' => $sql, 'params' => $params); | |
| return true; | |
| } | |
| } | |
| if (!function_exists('db_fetch_assoc')) { | |
| function db_fetch_assoc($sql) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_assoc_prepared')) { | |
| function db_fetch_assoc_prepared($sql, $params = array()) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_row')) { | |
| function db_fetch_row($sql) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_row_prepared')) { | |
| function db_fetch_row_prepared($sql, $params = array()) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_cell')) { | |
| function db_fetch_cell($sql) { | |
| return ''; | |
| } | |
| } | |
| if (!function_exists('db_fetch_cell_prepared')) { | |
| function db_fetch_cell_prepared($sql, $params = array()) { | |
| function db_execute($sql, ...$args) { | |
| $GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute', 'sql' => $sql, 'params' => $args); | |
| return true; | |
| } | |
| } | |
| if (!function_exists('db_execute_prepared')) { | |
| function db_execute_prepared($sql, $params = array(), ...$args) { | |
| $GLOBALS['__test_db_calls'][] = array( | |
| 'fn' => 'db_execute_prepared', | |
| 'sql' => $sql, | |
| 'params' => $params, | |
| 'extra_args' => $args | |
| ); | |
| return true; | |
| } | |
| } | |
| if (!function_exists('db_fetch_assoc')) { | |
| function db_fetch_assoc($sql, ...$args) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_assoc_prepared')) { | |
| function db_fetch_assoc_prepared($sql, $params = array(), ...$args) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_row')) { | |
| function db_fetch_row($sql, ...$args) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_row_prepared')) { | |
| function db_fetch_row_prepared($sql, $params = array(), ...$args) { | |
| return array(); | |
| } | |
| } | |
| if (!function_exists('db_fetch_cell')) { | |
| function db_fetch_cell($sql, ...$args) { | |
| return ''; | |
| } | |
| } | |
| if (!function_exists('db_fetch_cell_prepared')) { | |
| function db_fetch_cell_prepared($sql, $params = array(), ...$args) { |
| "autoload-dev": { | ||
| "files": [ | ||
| "tests/bootstrap.php" | ||
| ] |
There was a problem hiding this comment.
Adding composer.json with Pest is useful, but the existing CI workflow only runs composer install in the Cacti root, not in the plugin directory, so vendor/bin/pest won't be executed in CI. Consider adding a CI step (or a composer script) that installs dev deps and runs Pest from the plugin directory to prevent test drift.
Suggested change
| ] | |
| ] | |
| }, | |
| "scripts": { | |
| "test": "vendor/bin/pest", | |
| "pest": "vendor/bin/pest" |
| directory: "/" | ||
| schedule: | ||
| interval: "weekly" | ||
| open-pull-requests-limit: 10 |
There was a problem hiding this comment.
Dependabot is configured for npm and GitHub Actions, but this PR introduces composer-managed dependencies. Add a package-ecosystem: "composer" entry for / so Pest (and other PHP tooling) gets security/version updates automatically.
Suggested change
| open-pull-requests-limit: 10 | |
| open-pull-requests-limit: 10 | |
| - package-ecosystem: "composer" | |
| directory: "/" | |
| schedule: | |
| interval: "weekly" | |
| open-pull-requests-limit: 10 |
Comment on lines
+4
to
+14
| push: | ||
| branches: [main, master, develop] | ||
| paths-ignore: | ||
| - "**/*.php" | ||
| - "**/*.md" | ||
| pull_request: | ||
| branches: [main, master, develop] | ||
| paths-ignore: | ||
| - "**/*.php" | ||
| - "**/*.md" | ||
| schedule: |
There was a problem hiding this comment.
This CodeQL workflow is configured only for JavaScript/TypeScript and ignores all *.php changes. Since this repository is primarily PHP, consider adding php to the analyzed languages (and removing the PHP paths-ignore) if the intent is to get security scanning coverage for the plugin code.
…dabot - Throw RuntimeException when realpath/file_get_contents fails (previously silent continue hid unscanned files) - Fix Dependabot ecosystem from npm to composer - Remove committed .omc session artifacts, add .omc/ to .gitignore Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Author
|
Converted to draft to serialize the stack in this repo. Blocked by #51; will un-draft after that merges to avoid cross-PR merge conflicts. |
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
Test plan
composer install && vendor/bin/pestpasses