test: add Pest v1 security test infrastructure#55
test: add Pest v1 security test infrastructure#55somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds initial Pest v1-based “security” test scaffolding for the Cacti audit plugin, aiming to catch structural regressions and certain unsafe patterns via lightweight static checks.
Changes:
- Added a Pest bootstrap that stubs key Cacti framework functions/constants for isolated test execution.
- Added security-oriented Pest tests for
setup.phpstructure, prepared-statement usage, and PHP 7.4 syntax compatibility. - Added
tests/Pest.phpto load the bootstrap.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/bootstrap.php | Provides stubbed Cacti functions/constants to allow tests to run without a full Cacti install. |
| tests/Pest.php | Pest entrypoint that loads the test bootstrap. |
| tests/Security/SetupStructureTest.php | Verifies setup.php contains required plugin lifecycle functions and version keys. |
| tests/Security/PreparedStatementConsistencyTest.php | Attempts to enforce exclusive usage of prepared DB helpers in core plugin files. |
| tests/Security/Php74CompatibilityTest.php | Scans for common PHP 8.0+ syntax/features in core plugin files. |
| function __($text, $domain = '') { | ||
| return $text; | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('__esc')) { | ||
| function __esc($text, $domain = '') { |
There was a problem hiding this comment.
The test stub for __() only accepts 2 parameters, but this plugin calls __() with 3 parameters for pluralization (e.g., __('%d Months', 2, 'audit') in setup.php). If any tests start including/executing plugin code that reaches those calls, this will fatal with an argument count error. Update the stub signature to be compatible with Cacti's usage (e.g., accept a count + domain or be variadic).
| function __($text, $domain = '') { | |
| return $text; | |
| } | |
| } | |
| if (!function_exists('__esc')) { | |
| function __esc($text, $domain = '') { | |
| function __($text, ...$args) { | |
| return $text; | |
| } | |
| } | |
| if (!function_exists('__esc')) { | |
| function __esc($text, ...$args) { |
| function __($text, $domain = '') { | ||
| return $text; | ||
| } | ||
| } | ||
|
|
||
| if (!function_exists('__esc')) { | ||
| function __esc($text, $domain = '') { |
There was a problem hiding this comment.
__esc() is stubbed with only 2 parameters, but in Cacti it is typically used similarly to __() (including pluralization/count variants). Keeping the stub signature aligned (e.g., accept optional count/domain or be variadic) will prevent future tests from failing due to argument count errors when plugin code is loaded.
| function __($text, $domain = '') { | |
| return $text; | |
| } | |
| } | |
| if (!function_exists('__esc')) { | |
| function __esc($text, $domain = '') { | |
| function __($text, ...$args) { | |
| return $text; | |
| } | |
| } | |
| if (!function_exists('__esc')) { | |
| function __esc($text, ...$args) { |
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | ||
|
|
There was a problem hiding this comment.
realpath() can return false; passing that directly to file_get_contents() will emit warnings and $source may become false, which then causes confusing failures in later expectations. Consider asserting that the resolved path/content is not false (and failing the test with a clear message) before running the toContain/toMatch checks.
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | |
| $setup_path = realpath(__DIR__ . '/../../setup.php'); | |
| if ($setup_path === false) { | |
| throw new RuntimeException('Failed to resolve setup.php path for structure test.'); | |
| } | |
| $source = file_get_contents($setup_path); | |
| if ($source === false) { | |
| throw new RuntimeException('Failed to read setup.php contents for structure test.'); | |
| } |
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This test silently continues when a target file can't be resolved/read. That makes the assertion meaningless in those cases (the test will pass even though nothing was checked). Since these are required plugin files, it would be better to fail fast with a clear expectation when $path/$contents is false.
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect($path)->not->toBeFalse( | |
| "Required plugin file {$relativeFile} could not be resolved" | |
| ); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse( | |
| "Required plugin file {$relativeFile} could not be read" | |
| ); |
| $rawPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)\s*\(/'; | ||
| $preparedPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)_prepared\s*\(/'; | ||
|
|
||
| foreach ($targetFiles as $relativeFile) { | ||
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | ||
|
|
||
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $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++; | ||
| } | ||
| } | ||
|
|
||
| expect($rawCallsOutsideComments)->toBe(0, | ||
| "File {$relativeFile} contains raw (unprepared) DB calls" | ||
| ); |
There was a problem hiding this comment.
The assertion expects zero raw db_execute/db_fetch_* calls in audit.php, audit_functions.php, and setup.php, but the current plugin code contains raw calls (e.g., audit.php uses db_execute('TRUNCATE TABLE audit_log') and setup.php has multiple db_execute(...)/db_fetch_cell(...)). As written, this test will fail as soon as Pest is run. Either narrow the rule to what the codebase actually enforces (e.g., only forbid raw calls when user input is involved) or update the plugin code in the same PR to satisfy this requirement.
| 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; | ||
| } | ||
|
|
||
| expect(preg_match('/\?->/', $contents))->toBe(0, | ||
| "{$relativeFile} uses nullsafe operator which requires PHP 8.0" | ||
| ); | ||
| } |
There was a problem hiding this comment.
These compatibility checks silently skip files that can't be resolved/read (continue on $path === false / $contents === false). That can lead to false positives where the test suite passes without actually checking anything (and the same pattern repeats in each it(...) block). Prefer failing fast if a required file isn't readable, and consider extracting the repeated loop into a helper to avoid duplicated logic across the four tests.
| 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; | |
| } | |
| expect(preg_match('/\?->/', $contents))->toBe(0, | |
| "{$relativeFile} uses nullsafe operator which requires PHP 8.0" | |
| ); | |
| } | |
| $readFileContents = function (string $relativeFile): string { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| if ($path === false) { | |
| throw new RuntimeException("Unable to resolve required file: {$relativeFile}"); | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| throw new RuntimeException("Unable to read required file: {$relativeFile}"); | |
| } | |
| return $contents; | |
| }; | |
| $assertPatternAbsent = function (string $pattern, string $message) use ($files, $readFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $readFileContents($relativeFile); | |
| expect(preg_match($pattern, $contents))->toBe(0, sprintf($message, $relativeFile)); | |
| } | |
| }; | |
| it('does not use str_contains (PHP 8.0)', function () use ($assertPatternAbsent) { | |
| $assertPatternAbsent('/\bstr_contains\s*\(/', '%s uses str_contains() which requires PHP 8.0'); | |
| }); | |
| it('does not use str_starts_with (PHP 8.0)', function () use ($assertPatternAbsent) { | |
| $assertPatternAbsent('/\bstr_starts_with\s*\(/', '%s uses str_starts_with() which requires PHP 8.0'); | |
| }); | |
| it('does not use str_ends_with (PHP 8.0)', function () use ($assertPatternAbsent) { | |
| $assertPatternAbsent('/\bstr_ends_with\s*\(/', '%s uses str_ends_with() which requires PHP 8.0'); | |
| }); | |
| it('does not use nullsafe operator (PHP 8.0)', function () use ($assertPatternAbsent) { | |
| $assertPatternAbsent('/\?->/', '%s uses nullsafe operator which requires PHP 8.0'); |
Separates the Pest security test infrastructure into its own PR as requested.