Skip to content

test: add Pest v1 security test infrastructure#769

Open
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:test/add-security-test-infrastructure
Open

test: add Pest v1 security test infrastructure#769
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:test/add-security-test-infrastructure

Conversation

@somethingwithproof
Copy link
Copy Markdown

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 thold plugin security/hardening expectations by scanning source for setup structure, prepared-statement usage, and PHP version compatibility constraints.

Changes:

  • Introduces Pest bootstrap/config plus Cacti function stubs under tests/.
  • Adds source-scan tests for setup.php structure and prepared-statement consistency.
  • Adds a PHP-compatibility source scan test (currently framed as PHP 7.4 compatibility).

Reviewed changes

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

Show a summary per file
File Description
composer.json Adds Pest as a dev dependency and configures test bootstrap autoloading.
tests/Pest.php Pest configuration entrypoint loading the bootstrap.
tests/bootstrap.php Provides framework stubs/constants for isolated test execution.
tests/Security/SetupStructureTest.php Verifies setup.php includes expected plugin functions/version keys via source scan.
tests/Security/PreparedStatementConsistencyTest.php Scans a set of files for raw db_* calls to enforce prepared helper usage.
tests/Security/Php74CompatibilityTest.php Scans selected files for PHP 8-only functions/operators, asserting PHP 7.4 compatibility.

Comment on lines +16 to +27
it('uses prepared DB helpers in all plugin files', function () {
$targetFiles = array(
'includes/database.php',
'includes/polling.php',
'includes/settings.php',
'notify_lists.php',
'notify_queue.php',
'poller_thold.php',
'setup.php',
'thold.php',
'thold_graph.php',
);
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 because several files in $targetFiles contain non-*_prepared DB helper calls (e.g., notify_lists.php contains db_fetch_assoc(...) at line ~1078). Either narrow $targetFiles to only the migrated/hardened files, or change the assertion to enforce prepared statements only for queries that include dynamic/user-supplied values (instead of banning all db_fetch_* calls).

Copilot uses AI. Check for mistakes.
Comment on lines +35 to +43
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 test silently continues when a file path can't be resolved or read. That can make the suite pass while skipping checks (e.g., if a file gets renamed/removed). Consider failing the test when realpath() or file_get_contents() returns false so missing/unreadable target files are surfaced as failures.

Suggested change
if ($path === false) {
continue;
}
$contents = file_get_contents($path);
if ($contents === false) {
continue;
}
expect($path)->not->toBeFalse("Failed to resolve target file path: {$relativeFile}");
$contents = file_get_contents($path);
expect($contents)->not->toBeFalse("Failed to read target file: {$relativeFile}");

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +13
/*
* Verify plugin source files do not use PHP 8.0+ syntax.
* Cacti 1.2.x plugins must remain compatible with PHP 7.4.
*/
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 hard-codes a requirement that the plugin must remain compatible with PHP 7.4, but the repo docs/changelog indicate ongoing PHP 8.1+ support (e.g., .github/copilot-instructions.md:10 and CHANGELOG entries about PHP 8.1.2). If the supported minimum PHP version is 8.1+, this test will block legitimate use of PHP 8 features; please align the minimum-version assertions and the file/comment naming with the project's actual support policy.

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

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

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

it('defines plugin_thold_uninstall function', function () use ($source) {
expect($source)->toContain('function plugin_thold_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.

$source = file_get_contents(realpath(...)) will throw a TypeError on PHP 8+ if realpath() returns false (e.g., unexpected directory layout), causing a hard error instead of a clean test failure. Consider asserting that the path resolves and the file contents are readable before running the toContain/toMatch assertions.

Suggested change
$source = file_get_contents(realpath(__DIR__ . '/../../setup.php'));
it('defines plugin_thold_install function', function () use ($source) {
expect($source)->toContain('function plugin_thold_install');
});
it('defines plugin_thold_version function', function () use ($source) {
expect($source)->toContain('function plugin_thold_version');
});
it('defines plugin_thold_uninstall function', function () use ($source) {
expect($source)->toContain('function plugin_thold_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) {
$read_setup_source = function (): string {
$setup_path = realpath(__DIR__ . '/../../setup.php');
expect($setup_path)->not->toBeFalse();
if ($setup_path === false) {
return '';
}
expect(is_readable($setup_path))->toBeTrue();
$source = file_get_contents($setup_path);
expect($source)->not->toBeFalse();
return ($source === false) ? '' : $source;
};
it('defines plugin_thold_install function', function () use ($read_setup_source) {
$source = $read_setup_source();
expect($source)->toContain('function plugin_thold_install');
});
it('defines plugin_thold_version function', function () use ($read_setup_source) {
$source = $read_setup_source();
expect($source)->toContain('function plugin_thold_version');
});
it('defines plugin_thold_uninstall function', function () use ($read_setup_source) {
$source = $read_setup_source();
expect($source)->toContain('function plugin_thold_uninstall');
});
it('returns version array with name key', function () use ($read_setup_source) {
$source = $read_setup_source();
expect($source)->toMatch('/[\'\""]name[\'\""]\s*=>/');
});
it('returns version array with version key', function () use ($read_setup_source) {
$source = $read_setup_source();

Copilot uses AI. Check for mistakes.
Comment on lines +10 to +20
/*
* Test bootstrap: stub Cacti framework functions so plugin code
* can be loaded in isolation without the full Cacti application.
*/

$GLOBALS['__test_db_calls'] = array();

if (!function_exists('db_execute')) {
function db_execute($sql) {
$GLOBALS['__test_db_calls'][] = array('fn' => 'db_execute', 'sql' => $sql, 'params' => array());
return true;
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 says the bootstrap stubs Cacti framework functions so plugin code can be loaded in isolation, but it doesn't define the global $config array that core plugin entry points rely on (e.g., setup.php uses $config['cacti_version'] / $config['base_path']). If future tests include plugin files, this will fatal; consider initializing a minimal $config (base_path, url_path, cacti_version, etc.) to match what the plugin expects.

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>
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