Repository navigation
test: rework FlowView test harness to match plugin_evidence's Cacti-based CI - #260
Merged
Merged
Conversation
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>
…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>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
…ased CI - drop the local composer.json/composer.lock dependency and the custom tests.yml workflow in favor of plugin_evidence's plugin-ci-workflow.yml, which checks out a real Cacti install and reuses its vendor Pest/PHPUnit - adopt plugin_evidence's tests/.cacti-version, tests/Pest.php, tests/TestCase.php and tests/bootstrap-unit.php, adapted for FlowView - drop the PDO-backed Contract/Integration suites and php-syntax.sh script; PHP syntax and a live MariaDB poller run are now covered by the CI workflow itself, matching plugin_evidence - broaden the PHP 7.4 compatibility check to scan the plugin tree recursively like plugin_evidence instead of a fixed file list - point phpunit.xml at the new bootstrap and Cacti's own vendor tree - update tests/README.md to describe testing against a working Cacti install instead of a local composer install Attribution: builds on the FlowView test harness proposed in #251 by @somethingwithproof.
7 of 8 tasks
bmfmancini
approved these changes
Sep 15, 2026
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved CI setup and validation issues prevent reliable approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Reworks FlowView’s test harness to run against a Cacti checkout with shared dependencies, CI integration, unit/security coverage, and related production fixes.
Changes:
- Adds Cacti-based Pest configuration, bootstrapping, fixtures, and tests.
- Adds CI coverage for MariaDB, syntax checks, and poller validation.
- Updates helper/database logic and testing documentation.
- Removes plugin-local Composer setup and superseded test harnesses.
File summaries
| File | Summary |
|---|---|
tests/Unit/QueryBuilderTest.php |
Covers query and filter SQL generation. |
tests/Unit/DatabaseAdapterTest.php |
Covers database adapter behavior. |
tests/Unit/CoreFunctionsTest.php |
Covers core helper functions. |
tests/TestCase.php |
Provides the test fixture base class. |
tests/Security/SetupStructureTest.php |
Validates plugin structure. |
tests/Security/Php74CompatibilityTest.php |
Checks recursive PHP 7.4 compatibility. |
tests/README.md |
Documents Cacti-based testing. |
tests/Pest.php |
Configures and documents Pest usage. |
tests/bootstrap-unit.php |
Provides Cacti stubs and loads plugin sources. Moderate (1 vote): unconditional source loading affects security-only runs. |
tests/.cacti-version |
Defines the expected Cacti branch. |
phpunit.xml |
Configures Pest/PHPUnit suites and coverage. |
functions.php |
Updates filtering, partition dates, whitespace, and domain handling. |
database.php |
Validates table identifiers and escapes patterns. Moderate (2 votes): unmatched backticks remain accepted. |
.gitignore |
Ignores test artifacts and tracks workflow files. |
.github/workflows/plugin-ci-workflow.yml |
Adds Cacti/MariaDB CI and poller validation. Critical (3 votes): no config file is created before plugin installation. Moderate (3 votes): coverage is requested without a coverage driver. Moderate (2 votes): the MySQL defaults-file path uses ineffective tilde expansion. |
Review details
Suppressed comments (1)
tests/bootstrap-unit.php:424
- This bootstrap is used for the entire configured suite, so these unconditional requires execute plugin source even when only
tests/Securityis selected. That makes the PHP 7.4 compatibility test parser-dependent and contradicts the README's claim that the security checks do not execute plugin source; use a separate security bootstrap or load these files only for unit tests.
require_once dirname(__DIR__) . '/functions.php';
require_once dirname(__DIR__) . '/database.php';
- Files reviewed: 14/15 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+163
to
+166
| - name: Install flowview Plugin | ||
| run: | | ||
| cd ${{ github.workspace }}/cacti | ||
| sudo php cli/plugin_manage.php --plugin=flowview --install --enable |
Comment on lines
+70
to
+71
| extensions: intl, mysql, gd, ldap, gmp, xml, curl, json, mbstring, snmp | ||
| ini-values: "post_max_size=256M, max_execution_time=60, date.timezone=America/New_York" |
|
|
||
| - name: Initialize Cacti Database | ||
| env: | ||
| MYSQL_AUTH_USR: '--defaults-file=~/.my.cnf' |
Comment on lines
+244
to
+246
| $matched = preg_match('/^(?:`?(?<database>[A-Za-z0-9_]+)`?\.)?`?(?<table>[A-Za-z0-9_]+)`?$/D', $table, $matches); | ||
| if ($matched === 1) { | ||
| $table_pattern = addcslashes($matches['table'], '\\%_'); |
TheWitness
added a commit
to somethingwithproof/plugin_flowview
that referenced
this pull request
Sep 15, 2026
- drop composer.json and tests/bootstrap.php; Pest and the Cacti API stubs now come from tests/bootstrap-unit.php via phpunit.xml, same as every other suite in this repo - move the E2E and Integration regression tests into tests/Security, since they are static source-scan checks like the rest of that suite and phpunit.xml only scans tests/Security and tests/Unit
TheWitness
added a commit
to somethingwithproof/plugin_flowview
that referenced
this pull request
Sep 15, 2026
…harness Verified with a sandbox Cacti checkout (Pest 3 + phpunit.xml from Cacti#260): - AuthGuardTest/OutputEscapingTest/RedirectSafetyTest/PreparedStatementConsistencyTest scanned tests/test_prepared_statements.php itself as if it were a UI entry point, which isn't meaningful for a test script; drop it from those scan lists. - PreparedStatementConsistencyTest flagged setup.php's two db_execute() calls that create the core Cacti reports_log/reports_queued tables. Those are static CREATE TABLE DDL with no bound values, so there is nothing to parameterize; exclude DDL lines from the raw-call check. All 61 tests in tests/Security and tests/Unit now pass.
TheWitness
added a commit
that referenced
this pull request
Sep 15, 2026
* security: migrate flowview process/version lookups to prepared * fix: reduce brittleness in prepared regression checks * test: add grok follow-up parameter binding checks for flowview * test: expand security test coverage for hardening changes Add targeted tests for prepared statement migration, output escaping, auth guard presence, CSRF token validation, redirect safety, and PHP 7.4 compatibility. Tests use source-scan patterns that verify security invariants without requiring the Cacti database. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix: escape quotes in Pest regex patterns Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> * fix(security): harden listener port command handling * test: align with the shared Cacti-checkout Pest framework from #260 - drop composer.json and tests/bootstrap.php; Pest and the Cacti API stubs now come from tests/bootstrap-unit.php via phpunit.xml, same as every other suite in this repo - move the E2E and Integration regression tests into tests/Security, since they are static source-scan checks like the rest of that suite and phpunit.xml only scans tests/Security and tests/Unit * fix(security): use a portable range comparison for IP/CIDR filters (#239) get_ip_filter() masked VARBINARY addresses with a bitwise AND, which MySQL 8.0 evaluates correctly but MariaDB casts both sides to BIGINT and always matches, silently returning every row. Replace it with a LENGTH()-guarded BETWEEN range, which is portable across both servers and keeps a 4-byte IPv4 range from reaching 16-byte IPv6 rows. Reported and diagnosed by @somethingwithproof in #239, whose suggested fix this applies verbatim. Closes #239 * fix(tests): make the PR 248 security suite pass under develop's Pest harness Verified with a sandbox Cacti checkout (Pest 3 + phpunit.xml from #260): - AuthGuardTest/OutputEscapingTest/RedirectSafetyTest/PreparedStatementConsistencyTest scanned tests/test_prepared_statements.php itself as if it were a UI entry point, which isn't meaningful for a test script; drop it from those scan lists. - PreparedStatementConsistencyTest flagged setup.php's two db_execute() calls that create the core Cacti reports_log/reports_queued tables. Those are static CREATE TABLE DDL with no bound values, so there is nothing to parameterize; exclude DDL lines from the raw-call check. All 61 tests in tests/Security and tests/Unit now pass. --------- Signed-off-by: Thomas Vincent <thomasvincent@gmail.com> Co-authored-by: TheWitness <thewitness@cacti.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reworks the FlowView test harness proposed in #251 to follow the same
Cacti-checkout-based CI pattern used by
plugin_evidence, instead of aplugin-local Composer install.
composer.json/composer.lockand the custom.github/workflows/tests.yml.github/workflows/plugin-ci-workflow.yml,phpunit.xml, andtests/.cacti-version,tests/Pest.php,tests/TestCase.php,tests/bootstrap-unit.php, adapted fromplugin_evidence, so Pest runsagainst a real Cacti checkout's own vendor tree in CI
tests/Contractandtests/Integrationsuites andtests/scripts/php-syntax.sh; PHP syntax validation and a live MariaDBpoller run are now covered by the CI workflow itself, matching
plugin_evidencerecursively, like
plugin_evidence, instead of a fixed file listtests/README.mdto describe testing against a working Cactiinstall rather than a local Composer install
Attribution
This builds directly on the FlowView unit/contract test coverage and
correctness fixes to
functions.php/database.phpproposed by@somethingwithproof in #251. Closing #251 in favor of this PR, which keeps
those fixes and tests but restructures the harness and CI workflow to match
plugin_evidence.Validation
php -lpasses on all changed/added PHP files