Skip to content

test: add Pest v1 security test infrastructure - #328

Closed
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:test/security-test-infrastructure
Closed

test: add Pest v1 security test infrastructure#328
somethingwithproof wants to merge 1 commit into
Cacti:developfrom
somethingwithproof:test/security-test-infrastructure

Conversation

@somethingwithproof

Copy link
Copy Markdown
Member

Separates the Pest security test infrastructure into its own PR as requested.

Copilot AI review requested due to automatic review settings April 11, 2026 23:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

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

Comment on lines +15 to +16
$source = file_get_contents(realpath(__DIR__ . '/../../setup.php'));

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
$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}");

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

$contents = file_get_contents($path);

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

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

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

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.

Suggested change
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"
);

Copilot uses AI. Check for mistakes.
Comment on lines +17 to +28
$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',
);

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

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

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.

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

Copilot AI Apr 11, 2026

Copy link

Choose a reason for hiding this comment

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

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.

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;
}
$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);

Copilot uses AI. Check for mistakes.
somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
@somethingwithproof

Copy link
Copy Markdown
Member Author

Test infrastructure cherry-picked into both #333 (fix/mactrack-logic-bugs) and #334 (hardening/mactrack). Closing in favour of those consolidated branches.

somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request May 17, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof added a commit to somethingwithproof/plugin_mactrack that referenced this pull request Aug 31, 2026
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
TheWitness pushed a commit that referenced this pull request Aug 31, 2026
…ocess dispatch (#334)

* harden(mactrack): escape extra_args tokens and capture passthru exit code

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* harden(mactrack): validate request vars before AJAX shell dispatch; remove global clobber

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* hardening(mactrack): cherry-pick prepared stmts and XSS fixes from #327

- db_fetch_assoc_prepared for interfaces query in mactrack_functions.php
- db_fetch_row_prepared for host/mt_device lookups in mactrack_actions.php
- html_escape device_name/hostname in mactrack_devices.php
- unserialize allowed_classes guard + html_escape in mactrack_view_macs.php

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* test: add Pest v1 security test scaffold from #328

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix(mactrack): address Copilot review on site_scan --web flag and error surfacing

Make --web conditional on $web param in mactrack_site_scan (was always added).
Append subprocess exit-code errors to data['content'] so the AJAX client
shows failures rather than an empty dialog.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* hardening(mactrack): escape drp_action output and parameterize row_id IN() deletes

mactrack_view_macs.php: wrap the drp_action hidden-input value with html_escape() in both form_actions() and form_aggregated_actions() (defense in depth; drp_action is already int-validated via get_filter_request_var), and escape the aggregated serialize($row_array) hidden input for parity with the MAC form.

Parameterize the two interpolated 'row_id IN (...)' statements (aggregated delete and the row lookup) with db_execute_prepared()/db_fetch_assoc_prepared() and ?-placeholders. The bound values were already numeric-validated (sanitize_unserialize_selected_items / input_validate_input_number), so this is defense in depth and brings the file in line with the PR's PreparedStatementConsistencyTest.

Co-Authored-By: Oz <oz-agent@warp.dev>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* hardening(mactrack): fix stored XSS and second-order SQLi in mactrack_devices.php

form_mactrack_actions(): html_escape() the device_name/hostname printed into the confirmation <li> list. These values originate from SNMP discovery (e.g. sysName) and were emitted unescaped (stored XSS). Mirrors the escaping already applied to mactrack_device_edit().

Copy SNMP Settings (drp_action==7): the UPDATE built its SET clause by interpolating host-table values with bare single quotes, allowing second-order SQL injection when a value contains a quote. Values are now bound via db_execute_prepared() placeholders; the column name ($field_name) remains interpolated as it is a trusted key from the static $fields_mactrack_snmp_item definition.

Co-Authored-By: Oz <oz-agent@warp.dev>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* Apply project PHP formatting

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* fix: escape aggregated MAC confirm list fields

Device name, IP, MAC, and port values are echoed into HTML lists.

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

* harden(mactrack): sanitize IP/MAC/port filter inputs on view pages

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>

---------

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Co-authored-by: Thomas Vincent <thomas@vincent.dev>
Co-authored-by: Oz <oz-agent@warp.dev>
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