Skip to content

Commit 5fe348a

Browse files
fix(csrf): distinct error codes, audit log on non-POST, honest lint header
- Distinct raise_message IDs per failure mode (syslog_method_error, syslog_csrf_error, syslog_csrf_unavailable) so log triage can differentiate non-POST, invalid token, and missing-helper paths - Add cacti_log entry on the non-POST rejection path so the audit trail is symmetric with the other two fail-closed branches - Document csrf_check($fatal=false) arg semantics inline so future readers see the helper contract - Rename the regression test comment block to call out explicitly that it is a source-scan lint, not a behavioral test; flag follow-up for real behavioral coverage once a DB-backed test harness exists Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent 75e4ad6 commit 5fe348a

2 files changed

Lines changed: 25 additions & 6 deletions

File tree

setup.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1609,20 +1609,25 @@ function syslog_utilities_action($action) {
16091609

16101610
if ($action == 'purge_syslog_hosts') {
16111611
if ($_SERVER['REQUEST_METHOD'] !== 'POST') {
1612-
raise_message('syslog_error', __('Invalid request. This action requires a CSRF protected POST.', 'syslog'), MESSAGE_LEVEL_ERROR);
1612+
cacti_log('WARNING: syslog purge blocked -- non-POST request', false, 'SYSLOG');
1613+
raise_message('syslog_method_error', __('Invalid request. This action requires a CSRF protected POST.', 'syslog'), MESSAGE_LEVEL_ERROR);
16131614
header('Location: utilities.php');
16141615
exit;
16151616
}
16161617

1618+
// csrf_check($fatal) returns bool; $fatal=false tells the helper not to
1619+
// die/exit on failure so we can log and redirect with a user-visible
1620+
// message ourselves.
16171621
if (function_exists('csrf_check')) {
16181622
if (!csrf_check(false)) {
1619-
raise_message('syslog_error', __('Invalid request. This action requires a CSRF protected POST.', 'syslog'), MESSAGE_LEVEL_ERROR);
1623+
cacti_log('WARNING: syslog purge blocked -- CSRF token validation failed', false, 'SYSLOG');
1624+
raise_message('syslog_csrf_error', __('Invalid request. This action requires a CSRF protected POST.', 'syslog'), MESSAGE_LEVEL_ERROR);
16201625
header('Location: utilities.php');
16211626
exit;
16221627
}
16231628
} else {
16241629
cacti_log('WARNING: syslog purge blocked -- CSRF validation unavailable', false, 'SYSLOG');
1625-
raise_message('syslog_error', __('Invalid request. Please try again.', 'syslog'), MESSAGE_LEVEL_ERROR);
1630+
raise_message('syslog_csrf_unavailable', __('Invalid request. Please try again.', 'syslog'), MESSAGE_LEVEL_ERROR);
16261631
header('Location: utilities.php');
16271632
exit;
16281633
}

tests/regression/issue259_csrf_purge_test.php

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,18 @@
11
<?php
2+
/*
3+
* Source-scan lint for the #259 purge-syslog-hosts CSRF hardening.
4+
*
5+
* This is a lint, NOT a behavioral test. It asserts that the expected
6+
* CSRF-enforcement snippets exist in setup.php so regressions that
7+
* silently delete the POST/csrf_check/JS-post-flow are caught at CI
8+
* time. A full behavioral test would require bootstrapping Cacti's
9+
* session, CSRF token, and database, which this plugin's regression
10+
* harness cannot currently provide.
11+
*
12+
* If the guard is refactored in a way that preserves the strings but
13+
* breaks the logic, this lint will NOT catch it — follow-up issue for
14+
* real behavioral coverage once a DB-backed test harness exists.
15+
*/
216

317
$setup = file_get_contents(dirname(__DIR__, 2) . '/setup.php');
418

@@ -79,8 +93,8 @@
7993
exit(1);
8094
}
8195

82-
// Verify user-facing message does not expose CSRF internals (log message may use "CSRF")
83-
if (strpos($setup, "raise_message('syslog_error', __('CSRF") !== false) {
96+
// Verify user-facing messages do not expose CSRF internals (log messages may use "CSRF")
97+
if (preg_match("/raise_message\\('syslog_[a-z_]*', __\\('CSRF/", $setup)) {
8498
fwrite(STDERR, "User-facing raise_message must not expose CSRF internals to end users.\n");
8599
exit(1);
86100
}
@@ -92,7 +106,7 @@
92106
}
93107

94108
// Verify fail-closed raise_message uses MESSAGE_LEVEL_ERROR severity
95-
if (strpos($setup, "raise_message('syslog_error', __('Invalid request. Please try again.', 'syslog'), MESSAGE_LEVEL_ERROR)") === false) {
109+
if (strpos($setup, "raise_message('syslog_csrf_unavailable', __('Invalid request. Please try again.', 'syslog'), MESSAGE_LEVEL_ERROR)") === false) {
96110
fwrite(STDERR, "Fail-closed branch raise_message must use MESSAGE_LEVEL_ERROR severity.\n");
97111
exit(1);
98112
}

0 commit comments

Comments
 (0)