DRAFT: fix(security): PR #313 follow-up - partition correctness, CSV/XSS hardening - #315
Conversation
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. |
- 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>
| $time = time() + 3600; | ||
| } | ||
|
|
||
| // Reject non-numeric or pre-epoch timestamps; boundary math assumes a |
There was a problem hiding this comment.
Rejecting this. We will fix this when we go to typing are arguments.
There was a problem hiding this comment.
Understood. The PR has since moved to draft and been narrowed. The SQL-rule gating/settings work is now removed from this branch. What remains here is just the sink-side callback hardening needed for the concrete dispatcher issue, not a typed-arguments design change.
| * Autocomplete Form Callback Functions | ||
| * ======================================================================== */ | ||
|
|
||
| function syslogExecuteFunctionByName(functionName, context /*, args */) { |
There was a problem hiding this comment.
This function was taken whole cloth from the function you had Claude write you in mainline Cacti. Why is it good for Cacti and not good here?
There was a problem hiding this comment.
The issue here was the old truncation behavior, not the general idea of identifier-based dispatch. The current version keeps the same basic pattern but now rejects argument-bearing strings instead of stripping them, which addresses the concrete review finding. The PR is now draft and narrowed, with the SQL-rule gating/settings work removed from this branch.
| ucfirst($facility), | ||
| ucfirst($priority), | ||
| ucfirst($program), | ||
| syslog_csv_safe($host), |
There was a problem hiding this comment.
I took this out for a reason. The fputcsv() does all the escaping with the exception of formula's. I would prefer we stick with the builtin functions. The Facility, Priority are static known values mapped from an ID even.
There was a problem hiding this comment.
Agreed that fputcsv() remains the CSV escaping mechanism. The helper here is only for spreadsheet-formula defusing before values reach fputcsv(), not a replacement for builtin escaping. The PR is now draft and narrower overall; the SQL-rule gating/settings changes are removed, while the CSV thread is still limited to the formula-defusing behavior.
|
|
||
| if ($debug) { | ||
| print date('H:m:s') . ' SYSLOG DEBUG: ' . trim($message) . PHP_EOL; | ||
| print date('H:i:s') . ' SYSLOG DEBUG: ' . trim($message) . PHP_EOL; |
| global $config, $syslog_cnn, $syslog_incoming_config; | ||
| global $syslogdb_default; | ||
|
|
||
| /* |
There was a problem hiding this comment.
I'm not sure this adds value as it's an internal function and not for general use.
There was a problem hiding this comment.
Fair point. The value I saw here was defense-in-depth at the interpolation sink, since those table names are used directly in DML and DDL. The PR has since been moved to draft and narrowed by removing the SQL-rule gating/settings work, so what remains is the smaller correctness and hardening set.
| $params[] = $max_seq; | ||
| } elseif ($alert['type'] == 'sql') { | ||
| // TODO: Make Injection proof | ||
| /* |
| 'method' => 'checkbox', | ||
| 'default' => '' | ||
| ], | ||
| 'syslog_security_header' => [ |
There was a problem hiding this comment.
Done. The SQL-rule gating and settings work has been removed from this branch. The PR is now draft and scoped back to the narrower partition, callback, JS-emission, report-query, and regression-fix changes.
| ?> | ||
| <script type='text/javascript'> | ||
| initSyslogAutocomplete('<?php print $form_name; ?>', '<?php print $callback; ?>', '<?php print $on_change; ?>'); | ||
| initSyslogAutocomplete(<?php print json_encode($form_name, $js_flags); ?>, <?php print json_encode($callback, $js_flags); ?>, <?php print json_encode($on_change, $js_flags); ?>); |
There was a problem hiding this comment.
I'm okay with this, though it makes no sense. It's assuming that this is not an internal function.
There was a problem hiding this comment.
The intent here remains defense-in-depth at the script-literal boundary, not treating the helper as public. json_encode(..., JSON_HEX_*) keeps the emitted JS safe even if a future internal caller passes quotes or a closing script token through these values. The PR is now draft and narrower overall, with the SQL-rule gating/settings work removed.
| PARTITION $cformat VALUES LESS THAN (UNIX_TIMESTAMP('$lnow')), | ||
| PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); | ||
| } | ||
| if (!cacti_sizeof($create_syntax) || empty($create_syntax['Create Table'])) { |
There was a problem hiding this comment.
I guess I'm okay with this. It means that the database is down though. So, bigger problems.
| PARTITION $cformat VALUES LESS THAN (TO_DAYS('$boundary_date')), | ||
| PARTITION dMaxValue VALUES LESS THAN MAXVALUE)"); | ||
| } elseif (strpos($create_sql, 'UNIX_TIMESTAMP') !== false) { | ||
| syslog_db_execute("ALTER TABLE `$syslogdb_default`.`$table` REORGANIZE PARTITION dMaxValue INTO ( |
There was a problem hiding this comment.
I'm not sure this works we need to keep it set to UNIX_TIMESTAMP(). Using your boundary date will break partition pruning.
There was a problem hiding this comment.
Maybe not. I have to review that. It's now going to be a timestamp though. Let me look at the partition setup...
MariaDB [cacti]> show create table syslog;
+-------+--------------+
| Table | Create Table |
+-------+--------------+
| syslog | CREATE TABLE `syslog` (
`facility_id` int(10) unsigned DEFAULT NULL,
`priority_id` int(10) unsigned DEFAULT NULL,
`program_id` int(10) unsigned DEFAULT NULL,
`host_id` int(10) unsigned DEFAULT NULL,
`logtime` timestamp NOT NULL DEFAULT '0000-00-00 00:00:00',
`message` varchar(1024) NOT NULL DEFAULT '',
`seq` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
PRIMARY KEY (`seq`,`logtime`),
KEY `seq` (`seq`),
KEY `logtime` (`logtime`),
KEY `program_id` (`program_id`),
KEY `host_id` (`host_id`),
KEY `priority_id` (`priority_id`),
KEY `facility_id` (`facility_id`)
) ENGINE=Aria AUTO_INCREMENT=93 DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci ROW_FORMAT=PAGE
PARTITION BY RANGE (unix_timestamp(`logtime`))
(PARTITION `d20260310` VALUES LESS THAN (1773201600) ENGINE = Aria,
PARTITION `d20260311` VALUES LESS THAN (1773288000) ENGINE = Aria,
PARTITION `d20260312` VALUES LESS THAN (1773374400) ENGINE = Aria,
PARTITION `d20260313` VALUES LESS THAN (1773460800) ENGINE = Aria,
PARTITION `d20260314` VALUES LESS THAN (1773547200) ENGINE = Aria,
PARTITION `d20260315` VALUES LESS THAN (1773633600) ENGINE = Aria,
PARTITION `d20260316` VALUES LESS THAN (1773720000) ENGINE = Aria,
PARTITION `d20260317` VALUES LESS THAN (1773806400) ENGINE = Aria,
PARTITION `d20260318` VALUES LESS THAN (1773892800) ENGINE = Aria,
PARTITION `d20260319` VALUES LESS THAN (1773979200) ENGINE = Aria,
PARTITION `d20260320` VALUES LESS THAN (1774065600) ENGINE = Aria,
PARTITION `d20260321` VALUES LESS THAN (1774152000) ENGINE = Aria,
PARTITION `d20260322` VALUES LESS THAN (1774238400) ENGINE = Aria,
PARTITION `d20260323` VALUES LESS THAN (1774324800) ENGINE = Aria,
PARTITION `d20260324` VALUES LESS THAN (1774411200) ENGINE = Aria,
PARTITION `d20260325` VALUES LESS THAN (1774497600) ENGINE = Aria,
PARTITION `d20260326` VALUES LESS THAN (1774584000) ENGINE = Aria,
PARTITION `d20260327` VALUES LESS THAN (1774670400) ENGINE = Aria,
PARTITION `d20260328` VALUES LESS THAN (1774756800) ENGINE = Aria,
PARTITION `d20260329` VALUES LESS THAN (1774843200) ENGINE = Aria,
PARTITION `d20260330` VALUES LESS THAN (1774929600) ENGINE = Aria,
PARTITION `d20260331` VALUES LESS THAN (1775016000) ENGINE = Aria,
PARTITION `d20260401` VALUES LESS THAN (1775102400) ENGINE = Aria,
PARTITION `d20260402` VALUES LESS THAN (1775188800) ENGINE = Aria,
PARTITION `d20260403` VALUES LESS THAN (1775275200) ENGINE = Aria,
PARTITION `d20260404` VALUES LESS THAN (1775361600) ENGINE = Aria,
PARTITION `d20260405` VALUES LESS THAN (1775448000) ENGINE = Aria,
PARTITION `d20260406` VALUES LESS THAN (1775534400) ENGINE = Aria,
PARTITION `d20260407` VALUES LESS THAN (1775620800) ENGINE = Aria,
PARTITION `d20260408` VALUES LESS THAN (1775707200) ENGINE = Aria,
PARTITION `d20260409` VALUES LESS THAN (1775793600) ENGINE = Aria,
PARTITION `d20260410` VALUES LESS THAN (1775880000) ENGINE = Aria,
PARTITION `dMaxValue` VALUES LESS THAN MAXVALUE ENGINE = Aria) |
+-------+--------------+
1 row in set (0.001 sec)
There was a problem hiding this comment.
It's really just differing path to the same end.
There was a problem hiding this comment.
Rechecked again against the current draft branch: the UNIX_TIMESTAMP path still emits numeric epoch literals, and boundary_date is only used for the TO_DAYS branch. Since my earlier reply, the branch has also been narrowed and aligned further with the dMaxValue fallback theme: the SQL-rule gating/settings work is removed, and the partition-maintenance comments/logging now treat failures as maintenance fallback conditions rather than correctness-gate behavior.
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>
70de561 to
0f33598
Compare
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>
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>
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