Skip to content

test: add Pest v1 security test infrastructure#217

Closed
somethingwithproof wants to merge 3 commits into
Cacti:developfrom
somethingwithproof:test/add-security-test-infrastructure
Closed

test: add Pest v1 security test infrastructure#217
somethingwithproof wants to merge 3 commits into
Cacti:developfrom
somethingwithproof:test/add-security-test-infrastructure

Conversation

@somethingwithproof
Copy link
Copy Markdown
Contributor

Summary

  • Add Pest v1 test scaffold with Cacti framework stubs
  • Source-scan tests for prepared statement consistency
  • PHP 7.4 compatibility verification tests
  • Plugin setup.php structure validation

Test plan

  • composer install && vendor/bin/pest passes
  • Tests verify security patterns match hardening PRs

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>
Copilot AI review requested due to automatic review settings April 9, 2026 06:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Pest v1-based test scaffold intended to validate security hardening patterns and basic plugin structure via lightweight source-scanning tests.

Changes:

  • Introduces a composer.json with Pest as a dev dependency for running the new test suite.
  • Adds a Pest bootstrap with Cacti framework function stubs to support isolated test execution.
  • Adds security-oriented tests for setup.php structure, DB prepared-statement usage consistency, and avoidance of a few PHP 8-only APIs.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
composer.json Adds Pest v1 dev dependency and test bootstrap autoloading.
tests/bootstrap.php Stubs key Cacti DB/helpers/constants for test execution outside Cacti.
tests/Pest.php Loads the test bootstrap for Pest runs.
tests/Security/SetupStructureTest.php Verifies setup.php declares expected plugin hook functions and version keys.
tests/Security/PreparedStatementConsistencyTest.php Scans selected files for raw db_* calls vs prepared helpers.
tests/Security/Php74CompatibilityTest.php Scans selected files for a few PHP 8.0+ APIs/operators.

Comment on lines +15 to +23
describe('prepared statement consistency in weathermap', function () {
it('uses prepared DB helpers in all plugin files', function () {
$targetFiles = array(
'check.php',
'setup.php',
);

$rawPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)\s*\(/';
$preparedPattern = '/\bdb_(?:execute|fetch_row|fetch_assoc|fetch_cell)_prepared\s*\(/';
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will currently fail against the existing repo: setup.php contains multiple raw db_execute(...) / db_fetch_*( calls (e.g., setup.php:72-76, setup.php:137, setup.php:593). Either migrate those calls to the prepared helpers, or narrow/adjust this test (e.g., exclude DDL/static SQL sections or limit the scan to files that have actually been migrated).

Copilot uses AI. Check for mistakes.
Comment on lines +28 to +36
if ($path === false) {
continue;
}

$contents = file_get_contents($path);

if ($contents === false) {
continue;
}
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The continue branches when realpath()/file_get_contents() fails cause the test to silently skip files and still pass, reducing its value as a regression check. Since check.php/setup.php are expected to exist, consider failing the test with a clear message if a target file can't be located or read.

Suggested change
if ($path === false) {
continue;
}
$contents = file_get_contents($path);
if ($contents === false) {
continue;
}
expect($path)->not->toBeFalse("Expected test target file {$relativeFile} to exist");
$contents = file_get_contents($path);
expect($contents)->not->toBeFalse("Expected test target file {$relativeFile} to be readable");

Copilot uses AI. Check for mistakes.
Comment on lines +38 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++;
}
}

expect($rawCallsOutsideComments)->toBe(0,
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment-skipping logic only ignores lines starting with //, *, or #. It will still count patterns inside /* ... */ lines (trimmed starts with /*) and also inside inline comments after code, which can create false positives. Consider using token_get_all() to ignore PHP comments/strings, or at least expand the filter to handle /* and inline comment segments.

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++;
}
}
expect($rawCallsOutsideComments)->toBe(0,
$tokens = token_get_all($contents);
$searchableCode = '';
foreach ($tokens as $token) {
if (is_array($token)) {
if (
$token[0] === T_COMMENT ||
$token[0] === T_DOC_COMMENT ||
$token[0] === T_CONSTANT_ENCAPSED_STRING ||
$token[0] === T_INLINE_HTML
) {
continue;
}
$searchableCode .= $token[1];
} else {
$searchableCode .= $token;
}
}
$hasRawCalls = preg_match($rawPattern, $searchableCode) && !preg_match($preparedPattern, $searchableCode);
expect($hasRawCalls)->toBeFalse(

Copilot uses AI. Check for mistakes.
Comment on lines +21 to +94
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;
}

Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests continue when realpath() or file_get_contents() fails, which can make the suite pass even if the target source files are missing/unreadable. Since these files are required for the guarantee you're testing, consider failing with a clear assertion when a target file can't be found/read.

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;
}
$get_file_contents = function (string $relativeFile): string {
$path = realpath(__DIR__ . '/../../' . $relativeFile);
expect($path)->not->toBeFalse("Unable to resolve required source file: {$relativeFile}");
$contents = file_get_contents($path);
expect($contents)->not->toBeFalse("Unable to read required source file: {$relativeFile}");
return $contents;
};
it('does not use str_contains (PHP 8.0)', function () use ($files, $get_file_contents) {
foreach ($files as $relativeFile) {
$contents = $get_file_contents($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, $get_file_contents) {
foreach ($files as $relativeFile) {
$contents = $get_file_contents($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, $get_file_contents) {
foreach ($files as $relativeFile) {
$contents = $get_file_contents($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, $get_file_contents) {
foreach ($files as $relativeFile) {
$contents = $get_file_contents($relativeFile);

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +15
* Verify plugin source files do not use PHP 8.0+ syntax.
* Cacti 1.2.x plugins must remain compatible with PHP 7.4.
*/

describe('PHP 7.4 compatibility in weathermap', function () {
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The header comment asserts the plugin must remain compatible with PHP 7.4, but this repo’s GitHub Actions workflow tests PHP 8.1–8.4 only (.github/workflows/plugin-ci-workflow.yml:41). If the project no longer targets PHP 7.4, update the comment (and potentially rename the test) to match the supported PHP range so the intent is accurate.

Suggested change
* Verify plugin source files do not use PHP 8.0+ syntax.
* Cacti 1.2.x plugins must remain compatible with PHP 7.4.
*/
describe('PHP 7.4 compatibility in weathermap', function () {
* Verify key plugin entry-point source files do not use PHP 8.0-only syntax.
* This is a narrow syntax guard for these files and does not define overall
* plugin runtime support beyond the repository's current PHP test matrix.
*/
describe('PHP syntax guardrails in weathermap', function () {

Copilot uses AI. Check for mistakes.
Comment on lines +15 to +33
$source = file_get_contents(realpath(__DIR__ . '/../../setup.php'));

it('defines plugin_weathermap_install function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_install');
});

it('defines plugin_weathermap_version function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_version');
});

it('defines plugin_weathermap_uninstall function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_uninstall');
});

it('returns version array with name key', function () use ($source) {
expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/');
});

it('returns version array with version key', function () use ($source) {
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

file_get_contents(realpath(...)) is executed at describe evaluation time; if the path resolution/read fails, you’ll get warnings and the whole file may error before any assertions run. Consider asserting the path/content are valid (or loading in a beforeAll/inside each it) so failures are reported cleanly.

Suggested change
$source = file_get_contents(realpath(__DIR__ . '/../../setup.php'));
it('defines plugin_weathermap_install function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_install');
});
it('defines plugin_weathermap_version function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_version');
});
it('defines plugin_weathermap_uninstall function', function () use ($source) {
expect($source)->toContain('function plugin_weathermap_uninstall');
});
it('returns version array with name key', function () use ($source) {
expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/');
});
it('returns version array with version key', function () use ($source) {
$get_source = function () {
$setup_path = realpath(__DIR__ . '/../../setup.php');
expect($setup_path)->not->toBeFalse();
$source = file_get_contents($setup_path);
expect($source)->not->toBeFalse();
return $source;
};
it('defines plugin_weathermap_install function', function () use ($get_source) {
$source = $get_source();
expect($source)->toContain('function plugin_weathermap_install');
});
it('defines plugin_weathermap_version function', function () use ($get_source) {
$source = $get_source();
expect($source)->toContain('function plugin_weathermap_version');
});
it('defines plugin_weathermap_uninstall function', function () use ($get_source) {
$source = $get_source();
expect($source)->toContain('function plugin_weathermap_uninstall');
});
it('returns version array with name key', function () use ($get_source) {
$source = $get_source();
expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/');
});
it('returns version array with version key', function () use ($get_source) {
$source = $get_source();

Copilot uses AI. Check for mistakes.
Comment thread composer.json
{
"name": "cacti/plugin_weathermap",
"description": "plugin_weathermap plugin for Cacti",
"license": "GPL-2.0-or-later",
Copy link

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an explicit PHP version constraint (e.g., in require or config.platform) aligned with the versions this repo supports/tests. Without it, composer install behavior can vary by developer machine PHP version and fail with less actionable solver errors.

Suggested change
"license": "GPL-2.0-or-later",
"license": "GPL-2.0-or-later",
"require": {
"php": "^8.1"
},

Copilot uses AI. Check for mistakes.
…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>
@somethingwithproof somethingwithproof marked this pull request as draft April 11, 2026 00:10
@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Converted to draft to serialize the stack in this repo. Blocked by #215; will un-draft after that merges to avoid cross-PR merge conflicts.

@somethingwithproof
Copy link
Copy Markdown
Contributor Author

Test scaffold superseded by the test suite in #225 which is being consolidated into #226/#227.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants