test: add Pest v1 security test infrastructure#328
test: add Pest v1 security test infrastructure#328somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a minimal Pest-based security test scaffold for the mactrack plugin, aiming to enforce baseline structural checks, prepared-statement usage consistency, and PHP 7.4 compatibility.
Changes:
- Introduces Pest bootstrap/config files under
tests/to run plugin-focused checks without loading full Cacti. - Adds a setup.php structure test to confirm required plugin hook functions and version keys exist.
- Adds “security gate” tests for prepared-statement-only DB helper usage and for avoiding select PHP 8.0+ syntax.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Security/SetupStructureTest.php | Scans setup.php source to assert required functions/keys exist. |
| tests/Security/PreparedStatementConsistencyTest.php | Scans selected plugin files to ensure raw db_* calls are not used. |
| tests/Security/Php74CompatibilityTest.php | Scans selected plugin files to forbid a few PHP 8.0+ constructs. |
| tests/Pest.php | Pest entry/config file requiring the test bootstrap. |
| tests/bootstrap.php | Provides stub implementations/constants to support isolated test execution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | ||
|
|
There was a problem hiding this comment.
realpath() can return false; passing that into file_get_contents() will raise a warning and leave $source as false, which then makes the expectations behave unpredictably. Consider asserting the file exists/is readable and failing the test with a clear message if it cannot be loaded.
| $source = file_get_contents(realpath(__DIR__ . '/../../setup.php')); | |
| $setupFile = __DIR__ . '/../../setup.php'; | |
| $setupPath = realpath($setupFile); | |
| expect($setupPath) | |
| ->not->toBeFalse("Failed to resolve setup.php path: {$setupFile}"); | |
| expect(is_readable($setupPath)) | |
| ->toBeTrue("setup.php is not readable: {$setupPath}"); | |
| $source = file_get_contents($setupPath); | |
| expect($source) | |
| ->not->toBeFalse("Failed to read setup.php contents: {$setupPath}"); |
| if ($path === false) { | ||
| continue; | ||
| } | ||
|
|
||
| $contents = file_get_contents($path); | ||
|
|
||
| if ($contents === false) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This test currently continues when a target file path cannot be resolved (or later when file_get_contents() fails), which means the test can pass while silently skipping files. For a consistency/security gate, it should fail when a listed file is missing or unreadable so regressions can’t be masked.
| if ($path === false) { | |
| continue; | |
| } | |
| $contents = file_get_contents($path); | |
| if ($contents === false) { | |
| continue; | |
| } | |
| expect($path)->not->toBeFalse( | |
| "Expected target file {$relativeFile} to exist and resolve to a real path" | |
| ); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse( | |
| "Expected target file {$relativeFile} to be readable" | |
| ); |
| $targetFiles = array( | ||
| 'mactrack_devices.php', | ||
| 'mactrack_device_types.php', | ||
| 'mactrack_interfaces.php', | ||
| 'mactrack_sites.php', | ||
| 'mactrack_snmp.php', | ||
| 'mactrack_utilities.php', | ||
| 'mactrack_view_arp.php', | ||
| 'mactrack_view_macs.php', | ||
| 'mactrack_view_sites.php', | ||
| 'setup.php', | ||
| ); |
There was a problem hiding this comment.
As written, this will fail on the current codebase: several of the listed files contain raw db_execute/db_fetch_* calls (e.g., mactrack_devices.php uses db_execute( and db_fetch_row(, and setup.php uses db_execute(). Either migrate those files before enabling this assertion, or narrow the target list / temporarily skip the test until the migration PR lands.
| 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.
These checks continue when a file is missing or unreadable, which allows the compatibility tests to pass while skipping files. It would be more reliable to fail with an explicit message when a file in the list can’t be loaded.
| 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; | |
| } | |
| $loadFileContents = function ($relativeFile) { | |
| $path = realpath(__DIR__ . '/../../' . $relativeFile); | |
| expect($path)->not->toBeFalse("Failed to resolve required file: {$relativeFile}"); | |
| $contents = file_get_contents($path); | |
| expect($contents)->not->toBeFalse("Failed to read required file: {$relativeFile}"); | |
| return $contents; | |
| }; | |
| it('does not use str_contains (PHP 8.0)', function () use ($files, $loadFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $loadFileContents($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, $loadFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $loadFileContents($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, $loadFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $loadFileContents($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, $loadFileContents) { | |
| foreach ($files as $relativeFile) { | |
| $contents = $loadFileContents($relativeFile); |
Separates the Pest security test infrastructure into its own PR as requested.