[somethingwithproof]

Title

Fix alert, removal, dashboard and import defects found in a code review of develop

Description

This change fixes twelve defects found in a review of the syslog processing path, the rule pages and the install and upgrade code. It contains security-relevant fixes: the alert command token substitution (first item below) and the import validation changes. The commits are atomic, one fix per commit, each with its own tests.

Per-finding summary:

1. Alert command tokens (includes/functions.php, alert_replace_variables). Tokens were replaced one after another, so a substituted value that contained another token name (for example a device name sent by a remote syslog sender) was expanded a second time, inside the quotes of the first value. The command string is now built in one strtr() pass, so substituted text is never scanned again. Affects device-level alerts whose command uses both the host name and message tokens.
2. Upgrade "Truncate" (includes/schema.php, includes/installer.php). The default upgrade choice dropped and recreated the alert, removal and report rule tables, although the advisor says rules are kept. It now discards stored messages only. The advisor text now says so, and cacti.pot is updated.
3. Host-level alert rules (includes/functions.php). Every * in the rule SQL was rewritten when the grouped query was built, which broke raw SQL rules such as message REGEXP 'a.*b'. Only the leading SELECT * is rewritten now.
4. Combined alert count query (includes/functions.php). All enabled rules are counted in one statement, so one rule that does not run made every alert count zero. If that statement fails, each rule is now counted on its own and only the failing rule is skipped.
5. Alert import (syslog_alerts.php). Imported rules are checked with the same compile and test query as the editor, and an id in the file can no longer choose the row to overwrite. A file without a hash no longer reads an undefined id.
6. Max Report Records (includes/functions.php). The counter used for the limit was only incremented after the loop, so the limit never applied. Threshold alert messages now stop at the configured number of rows. The query that fetches the matches is not limited, because the breach marking needs every matching message.
7. "All records" dashboard queries (lib/syslog_dashboard.php). The two branches of the UNION had an aggregate and no GROUP BY. Panels showed one arbitrary label with the grand total, or one time bucket, and the queries fail under ONLY_FULL_GROUP_BY. Each branch is now grouped.
8. Archiving removal rules (includes/functions.php). For host and program rules, and SQL rules that name those columns, the moving INSERT joined the reference tables and used unqualified host and program, which MySQL and MariaDB reject as ambiguous. The WHERE clause now filters the staging rows in a derived table before the join.
9. Reprocess (includes/functions.php, syslog_removal.php). "Reprocess" on one removal rule ran every enabled rule over the stored history, including delete rules. syslog_remove_items() takes an optional rule id and reprocess passes it.
10. Owner columns (includes/schema.php). user was varchar(32) in the rule, report, template, dashboard and device rule tables, while Cacti allows 50 character user names. New installs create varchar(50). Existing installs are altered once during the schema check; the marker row schema_user_width in syslog_status is written only after all four tables were altered, and a failed alter makes the check fail so it is retried.
11. Hostname lookups (includes/functions.php). Lookups were unbounded, so a flood of distinct sender-chosen names could stall a processing run. Lookups stop after SYSLOG_RESOLVE_BUDGET_SECONDS (60) per call; names that were not looked up are left as received and are never prefixed unresolved-. A warning is logged when the budget is spent.
12. Saved search template import (syslog_saved_searches.php). Imported templates are now parsed with the logical search parser, and the record type and grouping values are validated, as in the editor and the save action.

Test tooling in this change:

• tests/regression/review_fixes_database_test.php runs the SQL-level checks against a real MariaDB (dashboard GROUP BY under both SQL modes, archiving rules, reprocess, rule isolation, truncate scope, owner column widening). It is added to the CI step that runs the other database regression checks. It exits with 404 unless run from the CLI with the test database settings, and a unit test covers both cases.
• The Playwright E2E run is repaired (the package version did not match the pinned image, and two titles and a table name were out of date) and extended with six checks (11 to 16).
• New tests load page functions through a private temporary file and do not use eval().

Related Issue

None filed. These were found in a review of develop. Issue numbers can be added to the CHANGELOG entries if maintainers want them.

Motivation and Context

Items 3 to 5, 7, 8 and 9 give wrong results or silently skip work. Item 2 deletes configuration during a documented upgrade path. Item 1 is a command-injection path reachable by a remote syslog sender when an administrator uses both tokens in a device-level alert command. Items 10 and 11 are compatibility and reliability issues.

How Has This Been Tested?

Executed locally, on the head of this branch rebased on origin/develop (7635b1a):

• Pest unit tests: docker run --rm -v <cacti>:/w -w /w prof-php sh -c 'php -d xdebug.mode=coverage include/vendor/bin/pest --configuration=plugins/syslog/phpunit.xml --coverage-clover=plugins/syslog/coverage/clover.xml' on PHP 8.4.27: 531 passed, 1 failed. The failing test is QueryBuilderGuardTest ("fails each filter compile closed when lib/QueryBuilder.php is missing"); it fails the same way on origin/develop in this environment (its subprocess check returns null) and is not touched by this change. It could not be confirmed here whether CI passes it.
• Patch coverage: php tests/bin/patch-coverage.php coverage/clover.xml origin/develop 100 reports 100.00% (50 of 50 measured changed lines). includes/installer.php, includes/schema.php, syslog_alerts.php, syslog_removal.php and syslog_saved_searches.php are not in Clover and are covered by the existing allowlist; their changes are exercised by the unit tests and the database and E2E runs below.
• Every new test file was run against the unchanged origin/develop sources: 20 of the 26 new unit tests that exercise changed behavior fail there, and the rest are guards for behavior that must not change.
• Database regression checks on MariaDB 11.8.9 (the version the CI service uses): review_fixes_database_test.php passes with 24 checks and reports 21 failures against origin/develop. cli_schema_upgrade_test.php, threshold_recovery_database_test.php and incoming_preservation_database_test.php pass; quality_must_fix_database_test.php passes inside the Cacti container (it needs a Cacti tree).
• E2E: tests/E2E/playwright/run-e2e.sh against a Cacti core 1.3.0 - Dev tree, PHP 8.4, MariaDB 11.8.9 and the Playwright 1.52.0 image: 16 of 16 pass. Checks 11 to 16 all fail against the origin/develop sources.
• Install check on a plain Cacti with no other plugins and no requires entries: php cli/plugin_manage.php --plugin=syslog --install --enable --allperms installs and enables syslog 4.11, creates all plugin tables, and cacti.log has no FATAL, ERROR or WARNING lines. An upgrade-style run (owner columns set back to varchar(32), marker removed, then syslog_process.php) widens all six tables and writes the marker. The CI-style processor run (syslog_process.php --debug) logs SYSTEM SYSLOG STATS with no error lines.
• php -l on every changed PHP file with PHP 8.4, and php tests/bin/check-i18n-pot.php origin/develop.

Not run locally: the GitHub Actions workflow itself (PHP 8.2 and 8.3 legs, Cacti 1.2.x core, the SNMP and poller steps, Sonar), and anything on MySQL. The E2E run used Cacti core 1.3.0 - Dev, not 1.2.x. PHP 8.2 and 8.3 were not run; the code uses no syntax newer than PHP 8.0.

Performance

Fixture: MariaDB 11.8.9 in Docker Desktop on an Apple silicon Mac, default server settings, PHP 8.4.27 with Xdebug off. 200,000 rows in syslog, 50,000 in syslog_removed, 100,000 staged rows in syslog_incoming, 200 hosts, 50 programs. Five runs per case on a freshly built database; the table shows medians. Statements are server-side counts from SHOW SESSION STATUS LIKE 'Questions'. Peak PHP memory was 4 MB in every case.

Case	origin/develop	This branch
Dashboard breakdown, "All records"	33.6 ms, 1 statement, 1 row (wrong)	79.4 ms, 1 statement, 200 rows
Dashboard timeseries, "All records"	37.5 ms, 1 statement, 2 rows	67.8 ms, 1 statement, 24 rows
20 enabled alert rules, all valid	965 ms, 79 statements	969 ms, 79 statements
20 valid rules plus 1 that does not run	14 ms, 19 statements, 0 alerts raised	1223 ms, 100 statements, 20 alerts raised
Archiving rule (contains, moves 200 rows)	97.6 ms, 12 statements	93.2 ms, 12 statements
Reprocess one of 10 delete rules over 200k rows	696 ms, 17 statements	82 ms, 8 statements

What the numbers say:

• The dashboard queries are slower because they now do the grouping the panels need; the old numbers are for results that were wrong. No faster or slower claim is made beyond that.
• With a failing rule the old code skipped all alert work; the new code does the work for the valid rules, which is where the added time comes from. The fallback itself costs one extra count statement per rule (21 more statements here) and only runs when the combined statement fails.
• The normal alert path is unchanged within noise. The archive rule shape change (derived table) shows no measurable cost at this size.
• Reprocess is faster because it runs one rule instead of ten; that is the point of the fix, not a general speedup.

Not measured: hostname lookup timing (it depends on the resolver; the added check is one clock read per name), alert e-mail and command paths, the added validation statement per imported alert or template (one extra test query each, by inspection), the extra SELECT for the owner column marker (one statement per schema check, by inspection; the benchmark case for it returned different results on the two trees and is not reported), larger tables, remote collectors, page rendering, and a MySQL server.

Risk and rollback

• The owner column change runs ALTER TABLE ... MODIFY COLUMN on four small rule tables once per install. Widening a varchar is a metadata-only change on InnoDB for these sizes; it needs the usual ALTER privilege the plugin already uses. If it fails, the schema check fails and is retried on the next request; the marker row is not written.
• The "Truncate" choice now keeps rules. Anyone who relied on it to reset rules must delete them on the rule pages.
• Hostname lookups can now be skipped under load. Skipped names reach the log tables as received instead of with an unresolved- prefix.
• Rollback: revert the commits. The widened columns are compatible with the old code, and syslog_status keeps an unused schema_user_width row.

Screenshots (if appropriate):

Not applicable.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation repository accordingly.

[TheWitness]

Some questions and general comments. Since we are making all these changes, we should ensure that the ALERT variables are in the environment so that we don't have to pass to the script. It may be there already, I did not do a review of anything but the diff.

[TheWitness]

This stuff really belongs in the environment of the script so that we don't have to pass it in. It was a big mistake at the time. Everywhere else I have done it just puts the variables into the environment.

[TheWitness]

Looks like it's there though.

[TheWitness]

The db_update_table should handle this automatically when it's called on upgrade. I'm wondering why you increased user to 50 chars though?

[TheWitness]

Indentation issue.

[TheWitness]

Indentation issue.

[TheWitness]

Indentation issue.

[TheWitness]

Indentation issue.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud