DRAFT: fix(security): PR #313 follow-up - partition correctness, CSV/XSS hardening - #315
Open
somethingwithproof wants to merge 21 commits into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Follow-up hardening and correctness fixes for the Syslog plugin after PR #313, focusing on partition rotation correctness, SQL-rule opt-in gating, and UI/export injection defenses.
Changes:
- Reworks partition boundary arithmetic to use UTC epoch integer math and hard-fail when partition expressions can’t be detected.
- Adds
syslog_allow_sql_rules(disabled by default) and gates SQL-type alert/removal/report rules behind it; adds table-name allowlists for interpolated identifiers. - Hardens CSV export and host autocomplete UI against injection vectors; restores/extends regression coverage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
functions.php |
Partition boundary/rotation hardening; SQL-rule gating; CSV export sanitization helper; table allowlists. |
setup.php |
Partition creation uses UTC epoch boundaries; adds Security Settings + syslog_allow_sql_rules option. |
syslog.php |
Passes initSyslogAutocomplete args via json_encode() instead of raw-quoted strings. |
js/functions.js |
Builds host <option> safely via jQuery APIs; replaces callback dispatcher with identifier-only invocation. |
tests/regression/issue254_partition_table_locking_test.php |
Restores and expands regression guard assertions for partition and allowlist invariants. |
somethingwithproof
added a commit
to somethingwithproof/plugin_syslog
that referenced
this pull request
Apr 10, 2026
- syslog_partition_manage now gates syslog_partition_remove on the return value of syslog_partition_create. Previously a hard failure in create would still drop the oldest partition on every poller cycle, with no replacement, because remove ran unconditionally. - syslog_alerts.php and syslog_reports.php refuse to save a type=sql rule when 'syslog_allow_sql_rules' is off and raise an admin-visible error message naming the setting to flip. The previous empty-result fallback displayed a generic "SQL was invalid" error that pointed editors at the wrong problem. - syslog.php now passes JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT to json_encode() for the initSyslogAutocomplete arguments. Defense-in-depth so a future tainted value cannot close the <script> block. - syslog_csv_safe strips leading spaces (not tabs or CRs) before checking the first character, so " =SUM(A1)" is prefixed while leading tab and CR remain detectable as triggers themselves. - syslog_debug prints H:i:s instead of H:m:s (was printing month where minutes should be). - Regression test tightens the syslog_manage_items allowlist check to match the exact allowlist literal and the fail-closed return shape. Added a guard that syslog_partition_manage wraps syslog_partition_remove in an if on syslog_partition_create's return value. - New tests/regression/issue315_csv_safe_unit_test.php extracts syslog_csv_safe via source and exercises empty/null/int/benign inputs, every trigger character, leading-space variants, mid-string occurrences, and the already-escaped case. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
added a commit
to somethingwithproof/plugin_syslog
that referenced
this pull request
Apr 10, 2026
TheWitness
requested changes
Apr 10, 2026
somethingwithproof
added a commit
to somethingwithproof/plugin_syslog
that referenced
this pull request
Apr 10, 2026
somethingwithproof
marked this pull request as draft
April 10, 2026 23:17
somethingwithproof
changed the title
Follow-up to Cacti#313. Addresses eight correctness and hardening findings: - Partition boundary math is now computed as integer epochs in PHP (next UTC midnight) and injected as numeric literals. strtotime() and UNIX_TIMESTAMP('date-literal') both pulled the PHP/MySQL session TZ into UTC-intended math and caused drift at day boundaries on non-UTC servers. - syslog_partition_create() now hard-fails with an error log instead of silently warning when SHOW CREATE TABLE does not expose either TO_DAYS or UNIX_TIMESTAMP. Silent no-ops caused rotations to stall unnoticed. Rewrote str_contains() to strpos() for portability. - syslog_manage_items() validates $from_table/$to_table against a three-value allowlist before interpolation. Defense-in-depth for the one caller that currently passes safe literals. - Alert, removal, and report rule handlers of type sql are gated on a new 'syslog_allow_sql_rules' setting (off by default). These handlers inline admin-defined SQL into the WHERE clause and cannot be parameterised; the previous removal handler also emitted invalid syntax ("WHERE message (expr)"). Added a Security Settings section to setup.php with a warning description. - CSV export replaces lossy trim($x, ' =+-@') with a syslog_csv_safe() helper that prepends a single quote only when the first character is one of =+-@, TAB, CR. Preserves content verbatim while defusing spreadsheet formula injection per OWASP guidance. - Host autocomplete dropdown builds <option> elements via jQuery's attr()/text() instead of HTML string concatenation wrapped in DOMPurify.sanitize(). DOMPurify does not escape attribute delimiters, so a host containing a double quote could still break out of the class/value attribute. - Autocomplete onChange dispatcher enforces a bare-identifier whitelist and looks the callback up as a direct window[name] property. Rejects dotted paths, arguments, and anything that is not a function, with a console.warn. Server side, the three initSyslogAutocomplete arguments now go through json_encode() instead of single-quote interpolation. - Ported tests/regression/issue254_partition_table_locking_test.php to the new syslog_partition_check/create two-argument signatures and extended it to assert the UTC-midnight boundary arithmetic, the hard-fail on unknown partition expression, and the syslog_manage_items table-name allowlist. Verification: - php -l on every touched file - node -c on js/functions.js - all eight regression tests in tests/regression/ that pass on develop continue to pass; issue254 was restored and passes; three pre-existing failures (issue253, issue269 x2) are unchanged on develop. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Addresses code review feedback on the PR Cacti#313 follow-up. The boundary epoch calculation ((int)($time / 86400) + 1) * 86400 assumes a non-negative UTC timestamp; non-numeric or pre-epoch values would silently underflow. Reject them at function entry with a logged error and a false return, matching the other guard clauses in the function. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Addresses a fourth type=sql path missed in the initial follow-up. syslog_remove_items() at functions.php:654 also inlined the admin rule message into the WHERE clause. Apply the same syslog_allow_sql_rules opt-in that already gates syslog_manage_items, syslog_get_alert_sql, and syslog_get_report_sql. When the setting is off, the rule is skipped with a log entry naming the rule. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
- syslog_partition_manage now gates syslog_partition_remove on the return value of syslog_partition_create. Previously a hard failure in create would still drop the oldest partition on every poller cycle, with no replacement, because remove ran unconditionally. - syslog_alerts.php and syslog_reports.php refuse to save a type=sql rule when 'syslog_allow_sql_rules' is off and raise an admin-visible error message naming the setting to flip. The previous empty-result fallback displayed a generic "SQL was invalid" error that pointed editors at the wrong problem. - syslog.php now passes JSON_HEX_TAG | JSON_HEX_AMP | JSON_HEX_APOS | JSON_HEX_QUOT to json_encode() for the initSyslogAutocomplete arguments. Defense-in-depth so a future tainted value cannot close the <script> block. - syslog_csv_safe strips leading spaces (not tabs or CRs) before checking the first character, so " =SUM(A1)" is prefixed while leading tab and CR remain detectable as triggers themselves. - syslog_debug prints H:i:s instead of H:m:s (was printing month where minutes should be). - Regression test tightens the syslog_manage_items allowlist check to match the exact allowlist literal and the fail-closed return shape. Added a guard that syslog_partition_manage wraps syslog_partition_remove in an if on syslog_partition_create's return value. - New tests/regression/issue315_csv_safe_unit_test.php extracts syslog_csv_safe via source and exercises empty/null/int/benign inputs, every trigger character, leading-space variants, mid-string occurrences, and the already-escaped case. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
force-pushed
the
fix/pr313-followup-security-and-partitions
branch
from
July 14, 2026 10:46
70de561 to
0f33598
Compare
somethingwithproof
added a commit
to somethingwithproof/plugin_syslog
that referenced
this pull request
Jul 29, 2026
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 Cacti#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 Cacti#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 <thomasvincent@gmail.com>
somethingwithproof
added a commit
to somethingwithproof/plugin_syslog
that referenced
this pull request
Jul 29, 2026
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 Cacti#315. Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
Three defects, all failing open or failing at setup:
- the plugin syntax check redirected find's own output rather than php's, so
PHP errors never reached the grep testing for them; the step could not fail
- MYSQL_AUTH_USR carried a literal tilde, because parameter expansion happens
after tilde expansion, so MySQL was handed a path it could not resolve
- the Cacti checkout took the default branch, which is 1.3 in development and
whose CLI installer currently fatals with an undefined __()
plugin_syslog additionally installed libapache2-mod-php${{ matrix.php }},
which Ubuntu does not package, so apt exited 100 before Cacti was reached.
Verified with actionlint, which is clean on the result.
Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
somethingwithproof
marked this pull request as ready for review
August 17, 2026 21:24
# Conflicts: # .github/workflows/plugin-ci-workflow.yml
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
Follow-up to #313, now narrowed to the concrete correctness and hardening fixes that survived review.
dMaxValuefallback semanticsjson_encode(..., JSON_HEX_*)syslog_process_reports()__DIR__-based paths so web and CLI entrypoints bootstrap reliablydrp_actionand encoding filter state in nav linksinitSyslogMain()request/translatable strings withjson_encode(..., JSON_HEX_*)so tab/init text cannot break script contextsyslog_incomingThe earlier SQL-rule gating/settings work has been removed from this draft PR.
Test Plan
php -lclean on touched PHP filesnode -c js/functions.jsphp tests/regression/issue254_partition_table_locking_test.phpphp tests/regression/include_path_normalization_test.phpphp tests/regression/issue279_bulk_form_and_nav_encoding_test.phptests/e2e/run-orb-docker-e2e.shEnd-to-End Coverage
The Docker/Orb Playwright run exercises 10 browser-backed flows against a disposable Cacti + syslog-plugin stack, with messages inserted into
syslog_incomingand processed by the plugin CLI:Closes #314