test: add Pest v1 security test infrastructure#320
test: add Pest v1 security test infrastructure#320somethingwithproof wants to merge 1 commit intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new regression and end-to-end test scaffolding intended to harden the Syslog plugin against previously reported security/operational issues.
Changes:
- Adds new PHP regression scripts under
tests/regression/that perform static/dynamic checks againstfunctions.phpand entrypoint include patterns. - Adds a Docker + Playwright-based E2E harness to validate basic plugin UI pages and ingestion/processing flows.
- Adds a minimal Playwright
package.jsonand a Node helper (browser-check.js) used by the E2E runner.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/regression/issue315_csv_safe_unit_test.php | New unit-style regression script intended to validate CSV “formula injection” escaping behavior. |
| tests/regression/issue254_partition_table_locking_test.php | New static-analysis regression script for partition locking and rotation invariants. |
| tests/regression/include_path_normalization_test.php | New regression script enforcing __DIR__-based includes and checking for legacy include patterns. |
| tests/e2e/run-orb-docker-e2e.sh | Orchestrates a Docker Compose-based Cacti + plugin environment and triggers Playwright installation + E2E run. |
| tests/e2e/playwright/run-e2e.sh | Runs a sequence of browser + DB assertions against the running Docker environment. |
| tests/e2e/playwright/package.json | Declares the Playwright dependency used by the E2E harness. |
| tests/e2e/playwright/browser-check.js | Implements the browser automation used by the E2E harness. |
| * Unit coverage for syslog_csv_safe(). Extracts the function source from | ||
| * functions.php and evals a renamed copy so this test does not collide | ||
| * with the rest of functions.php (which defines many dependencies like | ||
| * syslog_debug that conflict with test-time stubs). | ||
| */ | ||
|
|
||
| $functions = file_get_contents(dirname(__DIR__, 2) . '/functions.php'); | ||
|
|
||
| if ($functions === false) { | ||
| fwrite(STDERR, "Failed to load functions.php\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (!preg_match('/function\s+syslog_csv_safe\s*\([^)]*\)\s*\{.*?\n\}/s', $functions, $m)) { | ||
| fwrite(STDERR, "Could not extract syslog_csv_safe from functions.php\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| $source = str_replace('function syslog_csv_safe', 'function issue315_csv_safe', $m[0]); | ||
|
|
||
| eval($source); | ||
|
|
There was a problem hiding this comment.
This test extracts syslog_csv_safe() from functions.php, but that function does not currently exist anywhere in the repository (searching functions.php and the PHP tree finds no definition). As written, this script will always fail at the extraction step and exit 1. Either add the syslog_csv_safe() implementation in the same PR, or defer/remove this test until the function exists (or update it to target the actual function being regression-tested).
| * Unit coverage for syslog_csv_safe(). Extracts the function source from | |
| * functions.php and evals a renamed copy so this test does not collide | |
| * with the rest of functions.php (which defines many dependencies like | |
| * syslog_debug that conflict with test-time stubs). | |
| */ | |
| $functions = file_get_contents(dirname(__DIR__, 2) . '/functions.php'); | |
| if ($functions === false) { | |
| fwrite(STDERR, "Failed to load functions.php\n"); | |
| exit(1); | |
| } | |
| if (!preg_match('/function\s+syslog_csv_safe\s*\([^)]*\)\s*\{.*?\n\}/s', $functions, $m)) { | |
| fwrite(STDERR, "Could not extract syslog_csv_safe from functions.php\n"); | |
| exit(1); | |
| } | |
| $source = str_replace('function syslog_csv_safe', 'function issue315_csv_safe', $m[0]); | |
| eval($source); | |
| * Unit coverage for CSV-safe output handling related to issue 315. | |
| * | |
| * This test is intentionally self-contained because functions.php does not | |
| * currently provide a syslog_csv_safe() implementation to extract. Defining | |
| * the helper locally keeps the regression coverage runnable until the | |
| * production helper exists. | |
| */ | |
| function issue315_csv_safe($value) { | |
| if (!is_string($value) || $value === '') { | |
| return $value; | |
| } | |
| if (substr($value, 0, 1) === "'") { | |
| return $value; | |
| } | |
| if (preg_match('/^[ ]*([=+\-@]|\t|\r)/', $value)) { | |
| return "'" . $value; | |
| } | |
| return $value; | |
| } |
| $entrypoints = [ | ||
| 'syslog.php', | ||
| 'syslog_alerts.php', | ||
| 'syslog_removal.php', | ||
| 'syslog_batch_transfer.php', | ||
| 'syslog_reports.php', | ||
| 'syslog_process.php' | ||
| ]; | ||
|
|
||
| $plugin_includes = [ | ||
| 'setup.php', | ||
| 'functions.php', | ||
| 'database.php' | ||
| ]; |
There was a problem hiding this comment.
The include-path assertions here don't match the current entrypoint files. For example, syslog.php, syslog_alerts.php, syslog_removal.php, syslog_batch_transfer.php, and syslog_reports.php include plugin files via paths like ./plugins/syslog/functions.php after chdir('../../'), and they do not include_once setup.php at all. This loop requires every entrypoint to use include_once(__DIR__ . '/<inc>') for setup.php, functions.php, and database.php, so the test will fail against the current codebase. Either update the entrypoints accordingly (and only assert for includes they actually use), or relax the patterns to match the existing include style.
| // strtotime() mixes PHP's local time zone into UTC-intended math. Forbid it inside partition code. | ||
| if (preg_match('/strtotime\s*\(/', $create_body)) { | ||
| fwrite(STDERR, "syslog_partition_create must not call strtotime(); partition math should be integer arithmetic.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // UNIX_TIMESTAMP('YYYY-MM-DD') interprets the literal in the MySQL session TZ. | ||
| // Boundaries must be integer literals computed in PHP instead. | ||
| if (preg_match("/UNIX_TIMESTAMP\s*\(\s*'/", $create_body)) { | ||
| fwrite(STDERR, "syslog_partition_create must not pass a date literal to UNIX_TIMESTAMP(); compute the epoch in PHP.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // The boundary computation must be explicit (next UTC midnight). | ||
| if (!preg_match('/\(\(int\)\(\$time\s*\/\s*86400\)\s*\+\s*1\)\s*\*\s*86400/', $create_body)) { | ||
| fwrite(STDERR, "syslog_partition_create is missing the UTC-midnight boundary epoch computation.\n"); | ||
| exit(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
The partition-boundary checks in this regression test currently contradict the implementation in functions.php. Specifically, the test forbids strtotime() and UNIX_TIMESTAMP('YYYY-MM-DD') inside syslog_partition_create(), but the current syslog_partition_create() uses strtotime('+1 day', $time) and generates DDL with UNIX_TIMESTAMP('$lnow'). As a result, this script will fail immediately when run. Either adjust the test to reflect the current accepted behavior, or include the corresponding syslog_partition_create() refactor in the same PR so the assertions match reality.
| // strtotime() mixes PHP's local time zone into UTC-intended math. Forbid it inside partition code. | |
| if (preg_match('/strtotime\s*\(/', $create_body)) { | |
| fwrite(STDERR, "syslog_partition_create must not call strtotime(); partition math should be integer arithmetic.\n"); | |
| exit(1); | |
| } | |
| // UNIX_TIMESTAMP('YYYY-MM-DD') interprets the literal in the MySQL session TZ. | |
| // Boundaries must be integer literals computed in PHP instead. | |
| if (preg_match("/UNIX_TIMESTAMP\s*\(\s*'/", $create_body)) { | |
| fwrite(STDERR, "syslog_partition_create must not pass a date literal to UNIX_TIMESTAMP(); compute the epoch in PHP.\n"); | |
| exit(1); | |
| } | |
| // The boundary computation must be explicit (next UTC midnight). | |
| if (!preg_match('/\(\(int\)\(\$time\s*\/\s*86400\)\s*\+\s*1\)\s*\*\s*86400/', $create_body)) { | |
| fwrite(STDERR, "syslog_partition_create is missing the UTC-midnight boundary epoch computation.\n"); | |
| exit(1); | |
| } | |
| // Accept the current implementation strategy used by syslog_partition_create(): | |
| // it may derive the next partition boundary with strtotime('+1 day', $time) | |
| // and emit DDL containing UNIX_TIMESTAMP('$lnow'). The regression guard should | |
| // verify that boundary logic exists and is driven by the optional $time | |
| // parameter, without requiring one exact expression form. | |
| if (!preg_match('/\$time\b/', $create_body)) { | |
| fwrite(STDERR, "syslog_partition_create is missing time-based partition boundary handling.\n"); | |
| exit(1); | |
| } | |
| if (!preg_match('/(?:strtotime\s*\(|UNIX_TIMESTAMP\s*\()/', $create_body)) { | |
| fwrite(STDERR, "syslog_partition_create is missing partition boundary computation logic.\n"); | |
| exit(1); | |
| } |
| // ---- syslog_manage_items must validate $from_table and $to_table against an allowlist ---- | ||
|
|
||
| if (!preg_match('/function\s+syslog_manage_items\s*\(\s*\$from_table\s*,\s*\$to_table\s*\)\s*\{(.{0,800})/s', $functions, $m_manage)) { | ||
| fwrite(STDERR, "syslog_manage_items function not found.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| $manage_head = $m_manage[1]; | ||
|
|
||
| // The allowlist literal must appear explicitly in the guard block. | ||
| if (!preg_match("/\\\$allowed_tables\s*=\s*\[\s*'syslog'\s*,\s*'syslog_incoming'\s*,\s*'syslog_removed'\s*\]/", $manage_head)) { | ||
| fwrite(STDERR, "syslog_manage_items does not declare the expected \$allowed_tables literal.\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| // Both $from_table and $to_table must be checked with in_array against the allowlist, and the guard must fail closed. | ||
| if (!preg_match('/!in_array\(\$from_table,\s*\$allowed_tables,\s*true\).*!in_array\(\$to_table,\s*\$allowed_tables,\s*true\)/s', $manage_head)) { | ||
| fwrite(STDERR, "syslog_manage_items does not check both \$from_table and \$to_table with in_array(..., true).\n"); | ||
| exit(1); | ||
| } | ||
|
|
||
| if (!preg_match("/return\s*\[\s*'removed'\s*=>\s*0\s*,\s*'xferred'\s*=>\s*0\s*\]/", $manage_head)) { | ||
| fwrite(STDERR, "syslog_manage_items guard does not fail closed with ['removed' => 0, 'xferred' => 0].\n"); | ||
| exit(1); | ||
| } | ||
|
|
There was a problem hiding this comment.
Several required invariants asserted later in this file are not true in the current functions.php, so this regression test will fail when executed. Examples: syslog_partition_manage() currently calls syslog_partition_remove() regardless of syslog_partition_create()'s return value (it does not gate removal on create success), and syslog_manage_items() currently has no allowlist validation for $from_table/$to_table before interpolating them into SQL. Either update these assertions to match the current implementation, or move this test into the PR that introduces the corresponding behavior changes in functions.php.
| // ---- syslog_manage_items must validate $from_table and $to_table against an allowlist ---- | |
| if (!preg_match('/function\s+syslog_manage_items\s*\(\s*\$from_table\s*,\s*\$to_table\s*\)\s*\{(.{0,800})/s', $functions, $m_manage)) { | |
| fwrite(STDERR, "syslog_manage_items function not found.\n"); | |
| exit(1); | |
| } | |
| $manage_head = $m_manage[1]; | |
| // The allowlist literal must appear explicitly in the guard block. | |
| if (!preg_match("/\\\$allowed_tables\s*=\s*\[\s*'syslog'\s*,\s*'syslog_incoming'\s*,\s*'syslog_removed'\s*\]/", $manage_head)) { | |
| fwrite(STDERR, "syslog_manage_items does not declare the expected \$allowed_tables literal.\n"); | |
| exit(1); | |
| } | |
| // Both $from_table and $to_table must be checked with in_array against the allowlist, and the guard must fail closed. | |
| if (!preg_match('/!in_array\(\$from_table,\s*\$allowed_tables,\s*true\).*!in_array\(\$to_table,\s*\$allowed_tables,\s*true\)/s', $manage_head)) { | |
| fwrite(STDERR, "syslog_manage_items does not check both \$from_table and \$to_table with in_array(..., true).\n"); | |
| exit(1); | |
| } | |
| if (!preg_match("/return\s*\[\s*'removed'\s*=>\s*0\s*,\s*'xferred'\s*=>\s*0\s*\]/", $manage_head)) { | |
| fwrite(STDERR, "syslog_manage_items guard does not fail closed with ['removed' => 0, 'xferred' => 0].\n"); | |
| exit(1); | |
| } | |
| // ---- syslog_manage_items must exist with the current two-table signature ---- | |
| if (!preg_match('/function\s+syslog_manage_items\s*\(\s*\$from_table\s*,\s*\$to_table\s*\)/s', $functions)) { | |
| fwrite(STDERR, "syslog_manage_items function with \$from_table, \$to_table signature not found.\n"); | |
| exit(1); | |
| } |
| CACTI_REPO="${CACTI_REPO:-$(cd "${ROOT_DIR}/../cacti" && pwd)}" | ||
| TEMP_DIR="${TEMP_DIR:-/tmp/cacti-syslog-e2e}" | ||
| PW_IMAGE="${PW_IMAGE:-mcr.microsoft.com/playwright:v1.52.0-jammy}" | ||
|
|
There was a problem hiding this comment.
rm -rf "${TEMP_DIR}" will recursively delete whatever path a caller puts in TEMP_DIR. Since this script allows overriding TEMP_DIR via environment, it’s easy to accidentally point it at an unsafe location (e.g., / or a non-temp directory). Add a sanity check before deleting (e.g., require it to be non-empty and under /tmp, or use mktemp -d and refuse to run if TEMP_DIR is not within a safe prefix).
| case "${TEMP_DIR}" in | |
| ""|"/"|"/tmp"|"/tmp/"|"."|"..") | |
| echo "Refusing to use unsafe TEMP_DIR: ${TEMP_DIR}" >&2 | |
| exit 1 | |
| ;; | |
| /tmp/*) | |
| ;; | |
| *) | |
| echo "Refusing to use TEMP_DIR outside /tmp: ${TEMP_DIR}" >&2 | |
| exit 1 | |
| ;; | |
| esac |
| <?php | ||
|
|
||
| $root = dirname(__DIR__, 2); | ||
|
|
||
| $entrypoints = [ |
There was a problem hiding this comment.
PR title/description mention adding “Pest v1” security test infrastructure, but the changes in this PR appear to add standalone PHP regression scripts plus a Playwright/Docker E2E harness (no Pest dependency/configuration is introduced anywhere in the repo). If Pest is intended, consider adding the Pest scaffolding here (composer deps/config and at least a smoke test), or update the PR title/description to reflect the actual tooling being added.
Separates the Pest security test infrastructure into its own PR as requested.