Follow-up to PR #313: partition correctness, CSV/XSS hardening, SQL rule gating

[somethingwithproof]

Summary

Follow-up tracking issue for eight correctness and security hardening findings discovered while reviewing PR #313 after it merged to develop. A fix PR is attached.

The issues fall into four buckets:

1. Partition rotation correctness (TZ-mixed arithmetic, silent failure modes).
2. Stored SQL injection in the type=sql rule handlers (alerts, removal rules, reports).
3. CSV export data corruption and spreadsheet formula injection.
4. Host dropdown attribute injection and the onChange callback dispatcher.

Severities below follow CVSS v3.1 qualitative bands. All issues are post-authentication (the Cacti Syslog realm), but several are reachable by any authenticated viewer, not only admins.

---

Findings

1. Partition boundary arithmetic mixes PHP and MySQL time zones (HIGH, correctness)

functions.php::syslog_partition_create() computed the next partition boundary with strtotime('+1 day', $time) (evaluated in PHP's local TZ) and passed the result to UNIX_TIMESTAMP('YYYY-MM-DD') (evaluated in MySQL's session TZ). syslog_partition_check() then compared UTC gmdate('Ymd') against the existing partition suffix. On any server whose PHP or MySQL session TZ was not UTC, the three steps drifted, producing either partitions with off-by-one-hour boundaries, duplicate names, or skipped rotations at UTC day crossings.

Evidence: PR #313 functions.php syslog_partition_create, syslog_partition_check; setup.php::syslog_create_partitioned_syslog_table().

Fix: Compute the boundary as an integer epoch in PHP (((int)($time / 86400) + 1) * 86400) and inject it as a numeric literal. TO_DAYS legacy tables still get a gmdate('Y-m-d') string because TO_DAYS ignores the hour/minute component. Both code paths now hard-fail and log an ERROR if SHOW CREATE TABLE returns neither TO_DAYS nor UNIX_TIMESTAMP, instead of silently warning and stalling rotation.

---

2. Silent no-op when partition expression cannot be detected (HIGH, reliability)

The PR #313 fallback for SHOW CREATE TABLE emitted a single cacti_log('WARNING: Unable to determine Partition type for rotation', false, 'SYSLOG') and returned without creating a partition. Ingest continues into the tail partition and eventually overflows.

Fix: Log at ERROR severity and return false so the poller reports the rotation failed, and a downstream operator sees the error in the Cacti log.

---

3. Stored SQL injection via type=sql rules (CRITICAL, post-auth)

Three handlers inline user-stored SQL expressions directly into the WHERE clause:

• functions.php::syslog_get_alert_sql() (the PR Multiple Fixes: Range Pruning, Callback Execution, Early Rotation, CSV Export Hardening, Prepared Statements, SQL Readability #313 source kept // TODO: Make Injection proof)
• functions.php::syslog_get_report_sql()
• functions.php::syslog_manage_items() (also emitted invalid syntax WHERE message (expr) that could never execute)

Any user with rights to create an alert, removal, or report rule could store an arbitrary SQL fragment that executes with the plugin's database credentials.

Fix: Gate all three handlers behind a new Syslog setting syslog_allow_sql_rules, off by default, surfaced in a new Security Settings section. When disabled, the handlers skip the rule and log a warning with the rule name. When enabled, the operator has knowingly opted in. syslog_manage_items also validates $from_table / $to_table against a three-value allowlist (syslog, syslog_incoming, syslog_removed) before interpolation.

---

4. CSV export corrupts data and under-protects against formula injection (HIGH, data integrity + security)

PR #313 added trim($host, ' =+-@') and trim($logmsg, ' =+-@') before fputcsv. This strips leading and trailing characters, so a legitimate host like @router1 becomes router1 and syslog messages starting with - are mangled. It also misses interior formula triggers and does not handle leading \t or \r.

Fix: New syslog_csv_safe() helper that prepends a single quote only when the first character is one of = + - @ \t \r. OWASP-recommended approach; no content is lost, and Excel/LibreOffice/Google Sheets all treat the resulting cell as literal text.

---

5. Host autocomplete dropdown attribute injection (HIGH, post-auth XSS)

js/functions.js::initSyslogMain() builds <option class="..." value="..."></option> via HTML string concatenation wrapped in DOMPurify.sanitize(). DOMPurify normalizes HTML fragments but does not escape attribute delimiters. A host or class value containing " breaks out of the class/value attribute, even though DOMPurify ran.

Fix: Build the option element with jQuery's .attr() and .text(), which escape attribute and text content correctly. Drop the DOMPurify call at that site.

---

6. Autocomplete onChange dispatcher allows arbitrary global calls and drops arguments (MEDIUM, post-auth)

PR #313 replaced eval(onChange) with a helper that walks a dotted path on window, but it stripped ( and ) with chained .replace() calls, silently discarding any arguments and still allowing any global identifier (alert, fetch, Function, ...).

Fix:

• Reject anything that is not a single bare identifier (^[A-Za-z_$][A-Za-z0-9_$]*$).
• Look the callback up directly on window[name]; if it is not a function, log a console.warn and return.
• Strip a trailing () if present for backwards compat with callers that pass "applyFilter()".
• Server-side, syslog.php now JSON-encodes the three arguments passed into initSyslogAutocomplete, so a stored onChange can no longer close the string literal.

---

7. syslog_manage_items table-name interpolation (MEDIUM, defense-in-depth)

$from_table and $to_table were interpolated into every query in the function. Only one caller today passes safe literals, but a future caller would turn this into a SQL injection surface with no barrier.

Fix: Validate both parameters against ['syslog', 'syslog_incoming', 'syslog_removed'] at function entry; return the expected ['removed' => 0, 'xferred' => 0] shape if either is rejected.

---

8. Regression test deleted without replacement (LOW, process)

PR #313 deleted tests/regression/issue254_partition_table_locking_test.php (202 lines) while materially refactoring the code path it covered.

Fix: Restored and ported to the new two-argument syslog_partition_check/syslog_partition_create signatures. Extended to assert:

• Partition boundary uses the ((int)($time / 86400) + 1) * 86400 UTC-midnight arithmetic.
• syslog_partition_create has no strtotime() or UNIX_TIMESTAMP('...') literal calls.
• syslog_partition_create hard-fails on unknown partition expression.
• syslog_manage_items has an in_array allowlist gate on $from_table and $to_table.

---

Verification

• php -l clean on every touched file (functions.php, setup.php, syslog.php, js/functions.js validated with node -c).
• tests/regression/issue254_partition_table_locking_test.php passes.
• All tests that pass on develop continue to pass on the fix branch.
• Three pre-existing failures (issue253, issue269 x2) are unchanged versus develop, so they are not regressions from this PR.

Non-goals

• This PR does not touch the logtime timestamp column type change shipped in PR Multiple Fixes: Range Pruning, Callback Execution, Early Rotation, CSV Export Hardening, Prepared Statements, SQL Readability #313. The TIMESTAMP type carries a 2038 ceiling and its interaction with non-UTC servers deserves a separate RFC; the boundary arithmetic fix here is independent of the column choice.
• This PR does not rewrite the type=sql rule UX. The runtime gate documents the risk and requires opt-in; a future PR could replace the free-form SQL fragment with a structured expression language.

Linked PR

Fix PR: (attached as a follow-up to this issue)