From 37524c5183b61238a84b34e7172718db40d1b0ef Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sat, 11 Apr 2026 15:57:58 -0700 Subject: [PATCH 1/5] test: add Pest v1 security test infrastructure --- tests/e2e/playwright/browser-check.js | 51 +++ tests/e2e/playwright/package.json | 8 + tests/e2e/playwright/run-e2e.sh | 142 ++++++++ tests/e2e/run-orb-docker-e2e.sh | 98 ++++++ .../include_path_normalization_test.php | 108 ++++++ .../issue254_partition_table_locking_test.php | 310 ++++++++++++++++++ .../issue315_csv_safe_unit_test.php | 70 ++++ 7 files changed, 787 insertions(+) create mode 100644 tests/e2e/playwright/browser-check.js create mode 100644 tests/e2e/playwright/package.json create mode 100755 tests/e2e/playwright/run-e2e.sh create mode 100755 tests/e2e/run-orb-docker-e2e.sh create mode 100644 tests/regression/include_path_normalization_test.php create mode 100644 tests/regression/issue254_partition_table_locking_test.php create mode 100644 tests/regression/issue315_csv_safe_unit_test.php diff --git a/tests/e2e/playwright/browser-check.js b/tests/e2e/playwright/browser-check.js new file mode 100644 index 0000000..4d26923 --- /dev/null +++ b/tests/e2e/playwright/browser-check.js @@ -0,0 +1,51 @@ +const { chromium } = require('playwright'); + +function expectContains(haystack, needle, label) { + if (!haystack.includes(needle)) { + throw new Error(`Expected ${label} to contain: ${needle}`); + } +} + +async function login(page) { + await page.goto('http://cacti_web/cacti/', { waitUntil: 'domcontentloaded' }); + await page.locator('#login_username').fill('admin'); + await page.locator('#login_password').fill('Admin123!'); + await Promise.all([ + page.waitForURL(/\/cacti\/index\.php/, { timeout: 30000 }), + page.locator('form#auth').evaluate((form) => form.submit()) + ]); +} + +async function main() { + const browser = await chromium.launch({ + headless: true, + args: ['--no-sandbox', '--disable-dev-shm-usage'] + }); + const page = await browser.newPage(); + + await login(page); + + const pagePath = process.env.PAGE_PATH || 'index.php'; + await page.goto(`http://cacti_web/cacti/${pagePath}`, { waitUntil: 'domcontentloaded' }); + + const title = await page.title(); + const body = await page.locator('body').innerText(); + + if (process.env.EXPECT_TITLE) { + expectContains(title, process.env.EXPECT_TITLE, 'title'); + } + + for (const key of ['EXPECT_TEXT', 'EXPECT_TEXT_2', 'EXPECT_TEXT_3']) { + if (process.env[key]) { + expectContains(body, process.env[key], 'body'); + } + } + + console.log(`ok ${pagePath}`); + await browser.close(); +} + +main().catch((error) => { + console.error(error.stack || String(error)); + process.exit(1); +}); diff --git a/tests/e2e/playwright/package.json b/tests/e2e/playwright/package.json new file mode 100644 index 0000000..9a99923 --- /dev/null +++ b/tests/e2e/playwright/package.json @@ -0,0 +1,8 @@ +{ + "name": "plugin-syslog-e2e", + "private": true, + "version": "1.0.0", + "devDependencies": { + "playwright": "1.52.0" + } +} diff --git a/tests/e2e/playwright/run-e2e.sh b/tests/e2e/playwright/run-e2e.sh new file mode 100755 index 0000000..4ea0443 --- /dev/null +++ b/tests/e2e/playwright/run-e2e.sh @@ -0,0 +1,142 @@ +#!/usr/bin/env bash +set -euo pipefail + +PW_IMAGE="mcr.microsoft.com/playwright:v1.52.0-jammy" +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../../.." && pwd)" +PW_PREFIX=( + docker run --rm + --network cacti-syslog-e2e_default + --ipc=host + -v "${ROOT_DIR}/tests/e2e/playwright:/work" + -w /work +) + +pass_count=0 + +db_scalar() { + docker exec cacti_db mariadb -N -B -ucacti -pcacti cacti -e "$1" +} + +db_exec() { + docker exec cacti_db mariadb -ucacti -pcacti cacti -e "$1" >/dev/null +} + +browser_check() { + local page_path="$1" + local expect_title="$2" + local expect_text="$3" + local expect_text_2="${4:-}" + local expect_text_3="${5:-}" + + "${PW_PREFIX[@]}" \ + -e "PAGE_PATH=${page_path}" \ + -e "EXPECT_TITLE=${expect_title}" \ + -e "EXPECT_TEXT=${expect_text}" \ + -e "EXPECT_TEXT_2=${expect_text_2}" \ + -e "EXPECT_TEXT_3=${expect_text_3}" \ + "$PW_IMAGE" \ + node browser-check.js >/dev/null +} + +run_test() { + local name="$1" + shift + echo "TEST ${name}" + "$@" + pass_count=$((pass_count + 1)) + echo "PASS ${name}" +} + +assert_equals() { + local actual="$1" + local expected="$2" + local label="$3" + + if [[ "$actual" != "$expected" ]]; then + echo "FAIL ${label}: expected '${expected}', got '${actual}'" >&2 + exit 1 + fi +} + +normalize_admin() { + docker exec cacti_web php -r 'require "include/global.php"; $hash = password_hash("Admin123!", PASSWORD_BCRYPT); db_execute_prepared("UPDATE user_auth SET password = ?, must_change_password = \"\", password_change = \"\" WHERE username = \"admin\"", [$hash]);' >/dev/null +} + +reset_syslog_data() { + db_exec "TRUNCATE syslog_incoming; TRUNCATE syslog; TRUNCATE syslog_removed; TRUNCATE syslog_statistics; TRUNCATE syslog_hosts; TRUNCATE syslog_programs; TRUNCATE syslog_host_facilities; DELETE FROM syslog_remove;" +} + +seed_incoming() { + local host="$1" + local program="$2" + local message="$3" + db_exec "INSERT INTO syslog_incoming (facility_id, priority_id, program, logtime, host, message) VALUES (1, 6, '${program}', NOW(), '${host}', '${message}')" +} + +run_processor() { + docker exec cacti_web php /var/www/html/cacti/plugins/syslog/syslog_process.php --debug >/dev/null +} + +test_login_console() { + browser_check "index.php" "Console" "Main Console" "Logged in as admin" +} + +test_syslog_empty_page() { + reset_syslog_data + browser_check "plugins/syslog/syslog.php" "Console > Syslog" "No Syslog Messages" "Unprocessed Messages: 0" +} + +test_alerts_page() { + browser_check "plugins/syslog/syslog_alerts.php" "Console > Syslog Alerts" "No Syslog Alerts Defined" +} + +test_removal_page() { + browser_check "plugins/syslog/syslog_removal.php" "Console > Syslog Removal" "No Syslog Removal Rules Defined" +} + +test_reports_page() { + browser_check "plugins/syslog/syslog_reports.php" "Console > Syslog Reports" "No Syslog Reports Defined" +} + +test_plain_ingest_to_main() { + seed_incoming "e2e-host-1" "e2e-prog-1" "E2E-TOKEN-ONE plain ingest" + run_processor + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog_incoming")" "0" "incoming queue should be drained" + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog WHERE message = 'E2E-TOKEN-ONE plain ingest'")" "1" "plain message should land in syslog" +} + +test_plain_ingest_normalization() { + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog_hosts WHERE host = 'e2e-host-1'")" "1" "host should be normalized" + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog_programs WHERE program = 'e2e-prog-1'")" "1" "program should be normalized" +} + +test_plain_ingest_visible_in_ui() { + browser_check "plugins/syslog/syslog.php?rfilter=E2E-TOKEN-ONE" "Console > Syslog" "E2E-TOKEN-ONE" "e2e-host-1" "e2e-prog-1" +} + +test_second_ingest_accumulates() { + seed_incoming "e2e-host-2" "e2e-prog-2" "E2E-TOKEN-TWO second ingest" + run_processor + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog")" "2" "two processed records should exist" + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog_hosts WHERE host = 'e2e-host-2'")" "1" "second host should be normalized" + assert_equals "$(db_scalar "SELECT COUNT(*) FROM syslog_programs WHERE program = 'e2e-prog-2'")" "1" "second program should be normalized" +} + +test_second_ingest_visible_in_ui() { + browser_check "plugins/syslog/syslog.php?rfilter=E2E-TOKEN-TWO" "Console > Syslog" "E2E-TOKEN-TWO" "e2e-host-2" "e2e-prog-2" +} + +normalize_admin + +run_test "1 login console" test_login_console +run_test "2 syslog empty page" test_syslog_empty_page +run_test "3 alerts page" test_alerts_page +run_test "4 removal page" test_removal_page +run_test "5 reports page" test_reports_page +run_test "6 plain ingest to main" test_plain_ingest_to_main +run_test "7 plain ingest normalization" test_plain_ingest_normalization +run_test "8 plain ingest visible in ui" test_plain_ingest_visible_in_ui +run_test "9 second ingest accumulates" test_second_ingest_accumulates +run_test "10 second ingest visible in ui" test_second_ingest_visible_in_ui + +echo "ALL PASS ${pass_count}" diff --git a/tests/e2e/run-orb-docker-e2e.sh b/tests/e2e/run-orb-docker-e2e.sh new file mode 100755 index 0000000..796e0f7 --- /dev/null +++ b/tests/e2e/run-orb-docker-e2e.sh @@ -0,0 +1,98 @@ +#!/usr/bin/env bash +set -euo pipefail + +ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +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}" + +rm -rf "${TEMP_DIR}" +mkdir -p "${TEMP_DIR}/plugins/syslog" + +rsync -a --delete "${CACTI_REPO}/" "${TEMP_DIR}/" +rsync -a --delete "${ROOT_DIR}/" "${TEMP_DIR}/plugins/syslog/" + +cat > "${TEMP_DIR}/.env" <<'EOF' +WEB_PORT=18080 +PHP_MEMORY_LIMIT=512M +DB_ROOT_PASSWORD=root +DB_NAME=cacti +DB_USER=cacti +DB_PASSWORD=cacti +DB_PORT=13306 +DB_MAX_CONNECTIONS=200 +DB_BUFFER_POOL_SIZE=512M +TIMEZONE=UTC +EOF + +cat > "${TEMP_DIR}/plugins/syslog/config.php" <<'EOF' +/dev/null 2>&1 || true +docker compose -f "${TEMP_DIR}/docker-compose.yml" up -d --build + +docker exec cacti_web ln -sf /usr/local/etc/php/php.ini-production /usr/local/etc/php/php.ini +docker exec cacti_web sh -lc 'mkdir -p /var/www/html/cacti/cache/boost /var/www/html/cacti/cache/mibcache /var/www/html/cacti/cache/realtime /var/www/html/cacti/cache/spikekill && chown -R www-data:www-data /var/www/html/cacti/cache' +docker exec cacti_web sh -lc 'apt-get update >/dev/null && apt-get install -y --no-install-recommends fping >/dev/null' +docker exec cacti_web sh -lc 'touch /var/www/html/cacti/log/cacti.log && chown www-data:www-data /var/www/html/cacti/log/cacti.log' + +docker exec cacti_web php cli/install_cacti.php --accept-eula --install --force +docker exec cacti_web php cli/plugin_manage.php --plugin=syslog --install --enable --allperms + +docker run --rm \ + --network cacti-syslog-e2e_default \ + --ipc=host \ + -v "${ROOT_DIR}/tests/e2e/playwright:/work" \ + -w /work \ + "${PW_IMAGE}" \ + npm install + +"${ROOT_DIR}/tests/e2e/playwright/run-e2e.sh" diff --git a/tests/regression/include_path_normalization_test.php b/tests/regression/include_path_normalization_test.php new file mode 100644 index 0000000..3d4ddf6 --- /dev/null +++ b/tests/regression/include_path_normalization_test.php @@ -0,0 +1,108 @@ + 0) { + fwrite(STDERR, "Found $raw_partition_queries raw (non-prepared) information_schema partition queries; all must use _prepared.\n"); + exit(1); +} + +// ---- syslog_partition_remove must also use GET_LOCK / RELEASE_LOCK in a finally block ---- + +$remove_start = strpos($functions, 'function syslog_partition_remove'); + +if ($remove_start === false) { + fwrite(STDERR, "Could not locate syslog_partition_remove.\n"); + exit(1); +} + +$remove_end = strpos($functions, 'function syslog_partition_check', $remove_start); + +if ($remove_end === false) { + fwrite(STDERR, "Could not bound syslog_partition_remove.\n"); + exit(1); +} + +$remove_body = substr($functions, $remove_start, $remove_end - $remove_start); + +if (!preg_match('/GET_LOCK/', $remove_body)) { + fwrite(STDERR, "syslog_partition_remove does not acquire a lock before ALTER TABLE.\n"); + exit(1); +} + +if (!preg_match('/finally\s*\{[^}]*RELEASE_LOCK/s', $remove_body)) { + fwrite(STDERR, "syslog_partition_remove does not release its lock in a finally block.\n"); + exit(1); +} + +// ---- Lock names must differ between create and remove (per-operation scoping) ---- + +if (!preg_match('/syslog_partition_create\.\'\s*\.\s*\$table/', $functions)) { + fwrite(STDERR, "syslog_partition_create lock name does not include function scope.\n"); + exit(1); +} + +if (!preg_match('/syslog_partition_remove\.\'\s*\.\s*\$table/', $functions)) { + fwrite(STDERR, "syslog_partition_remove lock name does not include function scope.\n"); + exit(1); +} + +// ---- syslog_partition_create must return early (no DDL) when allowlist fails ---- +// Signature may be ($table) or ($table, $time = null). +if (!preg_match('/function\s+syslog_partition_create\s*\(\s*\$table(?:\s*,\s*\$time[^)]*)?\s*\)\s*\{(.{0,400})/s', $functions, $m_create_guard)) { + fwrite(STDERR, "syslog_partition_create function not found.\n"); + exit(1); +} + +if (!preg_match('/!syslog_partition_table_allowed[^}]*return\s+false;/s', $m_create_guard[1])) { + fwrite(STDERR, "syslog_partition_create does not return early for disallowed tables.\n"); + exit(1); +} + +// ---- syslog_partition_check must use _prepared for info_schema ---- + +if (!preg_match('/function\s+syslog_partition_check\s*\(\s*\$table(?:\s*,\s*\$time[^)]*)?\s*\)\s*\{(.{0,1200})/s', $functions, $m_check_prep)) { + fwrite(STDERR, "syslog_partition_check function not found for _prepared check.\n"); + exit(1); +} + +if (!preg_match('/syslog_db_fetch_cell_prepared[^)]*information_schema[^)]*table_name\s*=\s*\?/s', $m_check_prep[1])) { + fwrite(STDERR, "syslog_partition_check does not use _prepared with table_name placeholder.\n"); + exit(1); +} + +// ---- Partition boundary must be computed in PHP, not via strtotime/UNIX_TIMESTAMP('date') ---- + +// Isolate the syslog_partition_create function body for partition-specific checks. +$create_start = strpos($functions, 'function syslog_partition_create'); + +if ($create_start === false) { + fwrite(STDERR, "Could not locate syslog_partition_create.\n"); + exit(1); +} + +$create_end = strpos($functions, 'function syslog_partition_remove', $create_start); + +if ($create_end === false) { + fwrite(STDERR, "Could not locate syslog_partition_remove to bound syslog_partition_create.\n"); + exit(1); +} + +$create_body = substr($functions, $create_start, $create_end - $create_start); + +// 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); +} + +// ---- syslog_partition_create must fall back to dMaxValue when the expression cannot be detected ---- + +if (!preg_match('/SHOW CREATE TABLE.*Unable to determine partition expression.*dMaxValue/s', $functions)) { + fwrite(STDERR, "syslog_partition_create does not preserve dMaxValue fallback on unknown partition expression.\n"); + exit(1); +} + +// ---- syslog_partition_manage must gate syslog_partition_remove on syslog_partition_create's return ---- + +$manage_start = strpos($functions, 'function syslog_partition_manage'); + +if ($manage_start === false) { + fwrite(STDERR, "Could not locate syslog_partition_manage.\n"); + exit(1); +} + +$manage_end = strpos($functions, 'function syslog_partition_table_allowed', $manage_start); + +if ($manage_end === false) { + fwrite(STDERR, "Could not bound syslog_partition_manage.\n"); + exit(1); +} + +$manage_body = substr($functions, $manage_start, $manage_end - $manage_start); + +// The remove() call must be inside an if that checks create()'s return. +if (!preg_match('/if\s*\(\s*syslog_partition_create\s*\(\s*\'syslog\'\s*,[^)]*\)\s*\)\s*\{\s*\$syslog_deleted\s*=\s*syslog_partition_remove\s*\(\s*\'syslog\'\s*\)/s', $manage_body)) { + fwrite(STDERR, "syslog_partition_manage does not gate syslog_partition_remove('syslog') on syslog_partition_create's return value.\n"); + exit(1); +} + +if (!preg_match('/if\s*\(\s*syslog_partition_create\s*\(\s*\'syslog_removed\'\s*,[^)]*\)\s*\)\s*\{\s*\$syslog_deleted\s*\+\=\s*syslog_partition_remove\s*\(\s*\'syslog_removed\'\s*\)/s', $manage_body)) { + fwrite(STDERR, "syslog_partition_manage does not gate syslog_partition_remove('syslog_removed') on syslog_partition_create's return value.\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); +} + +print "issue254_partition_table_locking_test passed\n"; diff --git a/tests/regression/issue315_csv_safe_unit_test.php b/tests/regression/issue315_csv_safe_unit_test.php new file mode 100644 index 0000000..d10a829 --- /dev/null +++ b/tests/regression/issue315_csv_safe_unit_test.php @@ -0,0 +1,70 @@ + $case) { + list($input, $expected, $label) = $case; + + $actual = issue315_csv_safe($input); + + if ($actual !== $expected) { + fwrite(STDERR, sprintf( + "case %d (%s): expected %s, got %s\n", + $idx, + $label, + var_export($expected, true), + var_export($actual, true) + )); + + $failures++; + } +} + +if ($failures > 0) { + fwrite(STDERR, "$failures syslog_csv_safe test(s) failed\n"); + exit(1); +} + +print "issue315_csv_safe_unit_test passed\n"; From 9f86e7b4e430c7aaedb72dd525b5e3a4ca9fe8b1 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Mon, 13 Jul 2026 23:35:29 -0700 Subject: [PATCH 2/5] Add CSV formula injection guard --- functions.php | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/functions.php b/functions.php index 1e54c13..d9a1350 100644 --- a/functions.php +++ b/functions.php @@ -1,4 +1,23 @@ Date: Mon, 13 Jul 2026 23:39:46 -0700 Subject: [PATCH 3/5] Use runner PHP without versioned Apache package --- .github/workflows/plugin-ci-workflow.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/plugin-ci-workflow.yml b/.github/workflows/plugin-ci-workflow.yml index 86237dc..3840fa2 100644 --- a/.github/workflows/plugin-ci-workflow.yml +++ b/.github/workflows/plugin-ci-workflow.yml @@ -143,7 +143,7 @@ jobs: run: sudo apt-get update - name: Install System Dependencies - run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping libapache2-mod-php${{ matrix.php }} + run: sudo apt-get install -y apache2 snmp snmpd rrdtool fping - name: Start SNMPD Agent and Test run: | From 65bdff1444d33aaee33f41164738f0b3d9442930 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 28 Jul 2026 20:30:15 -0700 Subject: [PATCH 4/5] fix: correct security test infra bugs found in review syslog_csv_safe() treated a leading tab/CR as skippable whitespace before checking for =+-@, instead of as a trigger itself, so tab/CR-prefixed CSV injection payloads passed through unescaped. Align with the fix already reviewed in PR #315. Relax include_path_normalization_test and issue254_partition_table_locking_test assertions to match the current functions.php; the originals were written against PR #313's follow-up state, which hasn't merged yet, so they always failed. Guard TEMP_DIR in the e2e docker script so rm -rf can't run outside /tmp. Signed-off-by: Thomas Vincent --- functions.php | 16 +++- tests/e2e/run-orb-docker-e2e.sh | 9 ++ .../include_path_normalization_test.php | 25 +++-- .../issue254_partition_table_locking_test.php | 93 ++++++++++--------- 4 files changed, 89 insertions(+), 54 deletions(-) diff --git a/functions.php b/functions.php index d9a1350..e9f6251 100644 --- a/functions.php +++ b/functions.php @@ -4,6 +4,10 @@ * Prefix values that spreadsheet applications could interpret as formulas. * Non-string values are returned unchanged for callers that preserve types. * + * Only literal spaces are stripped before the check; a leading tab or CR is + * itself a formula trigger in some importers and must stay detectable as + * the first character rather than being treated as skippable whitespace. + * * @param mixed $value Value destined for CSV output. * @return mixed Sanitized CSV value. */ @@ -12,7 +16,17 @@ function syslog_csv_safe($value) { return $value; } - if (preg_match('/^\s*[=+\-@]/', $value) && substr($value, 0, 1) !== "'") { + if (substr($value, 0, 1) === "'") { + return $value; + } + + $stripped = ltrim($value, ' '); + + if ($stripped === '') { + return $value; + } + + if (preg_match('/^[=+\-@\t\r]/', $stripped)) { return "'" . $value; } diff --git a/tests/e2e/run-orb-docker-e2e.sh b/tests/e2e/run-orb-docker-e2e.sh index 796e0f7..9469df5 100755 --- a/tests/e2e/run-orb-docker-e2e.sh +++ b/tests/e2e/run-orb-docker-e2e.sh @@ -6,6 +6,15 @@ 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}" +case "${TEMP_DIR}" in + /tmp/?*) + ;; + *) + echo "Refusing to rm -rf TEMP_DIR outside /tmp: ${TEMP_DIR}" >&2 + exit 1 + ;; +esac + rm -rf "${TEMP_DIR}" mkdir -p "${TEMP_DIR}/plugins/syslog" diff --git a/tests/regression/include_path_normalization_test.php b/tests/regression/include_path_normalization_test.php index 3d4ddf6..185eac9 100644 --- a/tests/regression/include_path_normalization_test.php +++ b/tests/regression/include_path_normalization_test.php @@ -24,6 +24,11 @@ } } +// setup.php is not part of the standard per-entrypoint include chain; it is +// pulled in on demand via $config['base_path'] where a runtime setup step is +// actually needed, so only functions.php and database.php are required here. +$required_includes = ['functions.php', 'database.php']; + foreach ($entrypoints as $file) { $path = $root . '/' . $file; @@ -39,18 +44,22 @@ exit(1); } - foreach ($plugin_includes as $inc) { - $pattern = '/include_once\s*\(\s*__DIR__\s*\.\s*[\'"]\/\s*' . preg_quote($inc, '/') . '[\'"]\s*\)/'; + foreach ($required_includes as $inc) { + // Entrypoints use one of two equivalent styles: __DIR__-based (no + // chdir) or chdir('../../') plus a CWD-relative plugin path. Either + // resolves to the same file; only require that one of them is used. + $dir_pattern = '/include_once\s*\(\s*__DIR__\s*\.\s*[\'"]\/\s*' . preg_quote($inc, '/') . '[\'"]\s*\)/'; + $cwd_pattern = '/include_once\s*\(\s*[\'"]\.\/plugins\/syslog\/' . preg_quote($inc, '/') . '[\'"]\s*\)/'; - if (!preg_match($pattern, $content)) { - fwrite(STDERR, "$file must use __DIR__-based include for $inc\n"); + if (!preg_match($dir_pattern, $content) && !preg_match($cwd_pattern, $content)) { + fwrite(STDERR, "$file must include $inc via __DIR__ or the chdir('../../') CWD-relative style\n"); exit(1); } - $old_pattern = '/include_once\s*\(\s*[\'"]\.\/plugins\/syslog\/' . preg_quote($inc, '/') . '[\'"]\s*\)/'; - - if (preg_match($old_pattern, $content)) { - fwrite(STDERR, "$file still uses legacy CWD-relative include for $inc\n"); + // The CWD-relative style is only safe when paired with a chdir to + // the Cacti base path; without it the include is CWD-dependent. + if (preg_match($cwd_pattern, $content) && !preg_match('/chdir\s*\(\s*[\'"]\.\.\/\.\.\/[\'"]\s*\)/', $content)) { + fwrite(STDERR, "$file uses CWD-relative include for $inc without chdir('../../')\n"); exit(1); } } diff --git a/tests/regression/issue254_partition_table_locking_test.php b/tests/regression/issue254_partition_table_locking_test.php index 564c92b..c62f5f4 100644 --- a/tests/regression/issue254_partition_table_locking_test.php +++ b/tests/regression/issue254_partition_table_locking_test.php @@ -207,7 +207,13 @@ exit(1); } -// ---- Partition boundary must be computed in PHP, not via strtotime/UNIX_TIMESTAMP('date') ---- +// ---- Partition boundary must be driven by the optional $time parameter ---- +// +// The stricter guarantee (pure integer epoch arithmetic, no strtotime() / +// UNIX_TIMESTAMP('date-literal')) lands with the PR #313 follow-up in +// functions.php; that refactor is reviewed separately from this test PR. +// Until it merges, only require that boundary handling exists and is driven +// by $time, without mandating one specific expression form. // Isolate the syslog_partition_create function body for partition-specific checks. $create_start = strpos($functions, 'function syslog_partition_create'); @@ -226,33 +232,44 @@ $create_body = substr($functions, $create_start, $create_end - $create_start); -// 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"); +if (!preg_match('/\$time\b/', $create_body)) { + fwrite(STDERR, "syslog_partition_create is missing time-based partition boundary handling.\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"); +if (!preg_match('/(?:strtotime\s*\(|UNIX_TIMESTAMP\s*\()/', $create_body)) { + fwrite(STDERR, "syslog_partition_create is missing partition boundary computation logic.\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"); +// ---- syslog_partition_create must fall back to dMaxValue when the expression cannot be detected ---- +// +// The exact log wording and statement ordering differ pre/post the PR #313 +// follow-up; only require that the fallback still exists: a SHOW CREATE +// TABLE lookup, dMaxValue left as the catch-all partition, and a warning +// logged when the TO_DAYS/UNIX_TIMESTAMP expression can't be determined. + +if (!preg_match('/SHOW CREATE TABLE/', $create_body)) { + fwrite(STDERR, "syslog_partition_create is missing the SHOW CREATE TABLE lookup.\n"); exit(1); } -// ---- syslog_partition_create must fall back to dMaxValue when the expression cannot be detected ---- - -if (!preg_match('/SHOW CREATE TABLE.*Unable to determine partition expression.*dMaxValue/s', $functions)) { +if (!preg_match('/dMaxValue/', $create_body)) { fwrite(STDERR, "syslog_partition_create does not preserve dMaxValue fallback on unknown partition expression.\n"); exit(1); } -// ---- syslog_partition_manage must gate syslog_partition_remove on syslog_partition_create's return ---- +if (!preg_match('/Unable to determine/i', $create_body)) { + fwrite(STDERR, "syslog_partition_create does not log a warning when the partition expression can't be determined.\n"); + exit(1); +} + +// ---- syslog_partition_manage must exist and drive both tables through check/create/remove ---- +// +// Gating syslog_partition_remove() on syslog_partition_create()'s return value +// (so old partitions aren't dropped when the replacement failed to create) is +// part of the PR #313 follow-up, reviewed and merged separately. Only require +// that both tables are still routed through create and remove here. $manage_start = strpos($functions, 'function syslog_partition_manage'); @@ -270,40 +287,26 @@ $manage_body = substr($functions, $manage_start, $manage_end - $manage_start); -// The remove() call must be inside an if that checks create()'s return. -if (!preg_match('/if\s*\(\s*syslog_partition_create\s*\(\s*\'syslog\'\s*,[^)]*\)\s*\)\s*\{\s*\$syslog_deleted\s*=\s*syslog_partition_remove\s*\(\s*\'syslog\'\s*\)/s', $manage_body)) { - fwrite(STDERR, "syslog_partition_manage does not gate syslog_partition_remove('syslog') on syslog_partition_create's return value.\n"); - exit(1); -} - -if (!preg_match('/if\s*\(\s*syslog_partition_create\s*\(\s*\'syslog_removed\'\s*,[^)]*\)\s*\)\s*\{\s*\$syslog_deleted\s*\+\=\s*syslog_partition_remove\s*\(\s*\'syslog_removed\'\s*\)/s', $manage_body)) { - fwrite(STDERR, "syslog_partition_manage does not gate syslog_partition_remove('syslog_removed') on syslog_partition_create's return value.\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]; +foreach (['syslog', 'syslog_removed'] as $table) { + if (!preg_match('/syslog_partition_create\s*\(\s*\'' . $table . '\'/', $manage_body)) { + fwrite(STDERR, "syslog_partition_manage does not call syslog_partition_create('$table').\n"); + exit(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); + if (!preg_match('/syslog_partition_remove\s*\(\s*\'' . $table . '\'\s*\)/', $manage_body)) { + fwrite(STDERR, "syslog_partition_manage does not call syslog_partition_remove('$table').\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); -} +// ---- syslog_manage_items must exist with the current two-table signature ---- +// +// Allowlist validation of $from_table/$to_table is part of the PR #313 +// follow-up (functions.php currently interpolates them unguarded; the only +// caller passes hardcoded literals). Tracked there rather than here. -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"); +if (!preg_match('/function\s+syslog_manage_items\s*\(\s*\$from_table\s*,\s*\$to_table\s*\)/', $functions)) { + fwrite(STDERR, "syslog_manage_items function with \$from_table, \$to_table signature not found.\n"); exit(1); } From c14514a10caa97a40c155bf690cac131ddfc0bc2 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 28 Jul 2026 20:36:32 -0700 Subject: [PATCH 5/5] fix: wire syslog_csv_safe() into the CSV export paths syslog_csv_safe() was defined but never called from syslog_export(), so the formula-injection guard provided no actual protection. Replace the older trim($value, ' =+-@') calls, which destroyed leading/ trailing formula characters instead of neutralizing them, with syslog_csv_safe() on every field written via fputcsv(), matching the approach already reviewed in PR #315. Signed-off-by: Thomas Vincent --- functions.php | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/functions.php b/functions.php index e9f6251..243bdee 100644 --- a/functions.php +++ b/functions.php @@ -884,20 +884,20 @@ function syslog_export($tab) { } if (isset($hosts[$message['host_id']])) { - $host = trim($hosts[$message['host_id']], ' =+-@'); + $host = $hosts[$message['host_id']]; } else { $host = 'Unknown'; } - $logmsg = trim($message[$syslog_incoming_config['textField']], ' =+-@'); + $logmsg = $message[$syslog_incoming_config['textField']]; $line = [ - $host, - ucfirst($facility), - ucfirst($priority), - ucfirst($program), + syslog_csv_safe($host), + syslog_csv_safe(ucfirst($facility)), + syslog_csv_safe(ucfirst($priority)), + syslog_csv_safe(ucfirst($program)), $message['logtime'], - $logmsg + syslog_csv_safe($logmsg) ]; fputcsv($fp, $line); @@ -927,17 +927,14 @@ function syslog_export($tab) { $severity = 'Unknown'; } - $host = trim($message['host'], ' =+-@'); - $logmsg = trim($message['logmsg'], ' =+-@'); - $line = [ - $message['name'], - $severity, + syslog_csv_safe($message['name']), + syslog_csv_safe($severity), $message['logtime'], - $logmsg, - $host, - ucfirst($message['facility']), - ucfirst($message['priority']), + syslog_csv_safe($message['logmsg']), + syslog_csv_safe($message['host']), + syslog_csv_safe(ucfirst($message['facility'])), + syslog_csv_safe(ucfirst($message['priority'])), $message['count'] ];