From 47c12c04b7bb611ad368b5790351e9ba0456e59b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 6 Mar 2026 19:03:42 -0800 Subject: [PATCH 1/4] refactor: centralize XML import payload loading Fixes #277 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Thomas Vincent --- CHANGELOG.md | 1 + functions.php | 22 ++++++++- syslog_alerts.php | 14 +----- syslog_removal.php | 14 +----- syslog_reports.php | 14 +----- .../issue277_import_payload_loader_test.php | 46 +++++++++++++++++++ 6 files changed, 71 insertions(+), 40 deletions(-) create mode 100644 tests/regression/issue277_import_payload_loader_test.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 0f1e7f2..7538fc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- develop --- +* issue#277: Centralize XML import payload loading for alert/report/removal rule imports * issue: Making changes to support Cacti 1.3 * issue: Don't use MyISAM for non-analytical tables * issue: The install advisor for Syslog was broken in current Cacti releases diff --git a/functions.php b/functions.php index 9bd3223..7b5610c 100644 --- a/functions.php +++ b/functions.php @@ -126,6 +126,27 @@ function syslog_sendemail($to, $from, $subject, $message, $smsmessage = '') { } } +function syslog_get_import_xml_payload($redirect_url) { + if (trim(get_nfilter_request_var('import_text')) != '') { + /* textbox input */ + return get_nfilter_request_var('import_text'); + } + + if (isset($_FILES['import_file']['tmp_name']) && + $_FILES['import_file']['tmp_name'] != 'none' && + $_FILES['import_file']['tmp_name'] != '') { + /* file upload */ + $fp = fopen($_FILES['import_file']['tmp_name'], 'r'); + $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); + fclose($fp); + + return $xml_data; + } + + header('Location: ' . $redirect_url); + exit; +} + function syslog_is_partitioned() { global $syslogdb_default; @@ -2421,4 +2442,3 @@ function alert_replace_variables($alert, $results, $hostname = '') { return $command; } - diff --git a/syslog_alerts.php b/syslog_alerts.php index 74c49b9..9e7a04e 100644 --- a/syslog_alerts.php +++ b/syslog_alerts.php @@ -939,18 +939,7 @@ function import() { } function alert_import() { - if (trim(get_nfilter_request_var('import_text') != '')) { - /* textbox input */ - $xml_data = get_nfilter_request_var('import_text'); - } elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) { - /* file upload */ - $fp = fopen($_FILES['import_file']['tmp_name'],'r'); - $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); - fclose($fp); - } else { - header('Location: syslog_alerts.php?header=false'); - exit; - } + $xml_data = syslog_get_import_xml_payload('syslog_alerts.php?header=false'); $xml_array = xml2array($xml_data); @@ -1009,4 +998,3 @@ function alert_import() { header('Location: syslog_alerts.php'); } - diff --git a/syslog_removal.php b/syslog_removal.php index f7b5e94..4ac9333 100644 --- a/syslog_removal.php +++ b/syslog_removal.php @@ -739,18 +739,7 @@ function import() { } function removal_import() { - if (trim(get_nfilter_request_var('import_text') != '')) { - /* textbox input */ - $xml_data = get_nfilter_request_var('import_text'); - } elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) { - /* file upload */ - $fp = fopen($_FILES['import_file']['tmp_name'],'r'); - $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); - fclose($fp); - } else { - header('Location: syslog_removal.php?header=false'); - exit; - } + $xml_data = syslog_get_import_xml_payload('syslog_removal.php?header=false'); /* obtain debug information if it's set */ $xml_array = xml2array($xml_data); @@ -810,4 +799,3 @@ function removal_import() { header('Location: syslog_removal.php'); } - diff --git a/syslog_reports.php b/syslog_reports.php index f0caec2..d0a4683 100644 --- a/syslog_reports.php +++ b/syslog_reports.php @@ -801,18 +801,7 @@ function import() { } function report_import() { - if (trim(get_nfilter_request_var('import_text') != '')) { - /* textbox input */ - $xml_data = get_nfilter_request_var('import_text'); - } elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) { - /* file upload */ - $fp = fopen($_FILES['import_file']['tmp_name'],'r'); - $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); - fclose($fp); - } else { - header('Location: syslog_reports.php?header=false'); - exit; - } + $xml_data = syslog_get_import_xml_payload('syslog_reports.php?header=false'); /* obtain debug information if it's set */ $xml_array = xml2array($xml_data); @@ -872,4 +861,3 @@ function report_import() { header('Location: syslog_reports.php'); } - diff --git a/tests/regression/issue277_import_payload_loader_test.php b/tests/regression/issue277_import_payload_loader_test.php new file mode 100644 index 0000000..aa2c936 --- /dev/null +++ b/tests/regression/issue277_import_payload_loader_test.php @@ -0,0 +1,46 @@ + Date: Fri, 6 Mar 2026 23:12:23 -0800 Subject: [PATCH 2/4] fix: add fopen/fread error handling in syslog_get_import_xml_payload fopen and fread can both return false on I/O failure; the original code ignored both return values and would pass false to fclose/return. Log the error and redirect back on failure instead of proceeding with bad data. Also add GPL/copyright header to the regression test file (was missing). Signed-off-by: Thomas Vincent --- functions.php | 13 +++++++++++ .../issue277_import_payload_loader_test.php | 22 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/functions.php b/functions.php index 7b5610c..9a3c95b 100644 --- a/functions.php +++ b/functions.php @@ -137,9 +137,22 @@ function syslog_get_import_xml_payload($redirect_url) { $_FILES['import_file']['tmp_name'] != '') { /* file upload */ $fp = fopen($_FILES['import_file']['tmp_name'], 'r'); + + if ($fp === false) { + cacti_log('SYSLOG ERROR: Failed to open uploaded import file', false, 'SYSTEM'); + header('Location: ' . $redirect_url); + exit; + } + $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); fclose($fp); + if ($xml_data === false) { + cacti_log('SYSLOG ERROR: Failed to read uploaded import file', false, 'SYSTEM'); + header('Location: ' . $redirect_url); + exit; + } + return $xml_data; } diff --git a/tests/regression/issue277_import_payload_loader_test.php b/tests/regression/issue277_import_payload_loader_test.php index aa2c936..d36b4a9 100644 --- a/tests/regression/issue277_import_payload_loader_test.php +++ b/tests/regression/issue277_import_payload_loader_test.php @@ -1,4 +1,26 @@ Date: Sat, 7 Mar 2026 22:27:36 -0800 Subject: [PATCH 3/4] fix: validate upload error code and use is_uploaded_file() in payload loader Add UPLOAD_ERR_OK check and is_uploaded_file() guard before reading the uploaded XML file. Without these, a PHP-level upload failure (e.g. size limit exceeded) or a path-traversal attempt on tmp_name would silently fall through to fopen(). Also open in binary mode ('rb') for correctness on all platforms. Signed-off-by: Thomas Vincent --- functions.php | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/functions.php b/functions.php index 9a3c95b..573f54b 100644 --- a/functions.php +++ b/functions.php @@ -136,7 +136,19 @@ function syslog_get_import_xml_payload($redirect_url) { $_FILES['import_file']['tmp_name'] != 'none' && $_FILES['import_file']['tmp_name'] != '') { /* file upload */ - $fp = fopen($_FILES['import_file']['tmp_name'], 'r'); + $tmp_name = $_FILES['import_file']['tmp_name']; + + if (!isset($_FILES['import_file']['error']) || $_FILES['import_file']['error'] !== UPLOAD_ERR_OK) { + header('Location: ' . $redirect_url); + exit; + } + + if (!is_uploaded_file($tmp_name)) { + header('Location: ' . $redirect_url); + exit; + } + + $fp = fopen($tmp_name, 'rb'); if ($fp === false) { cacti_log('SYSLOG ERROR: Failed to open uploaded import file', false, 'SYSTEM'); @@ -144,7 +156,7 @@ function syslog_get_import_xml_payload($redirect_url) { exit; } - $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); + $xml_data = fread($fp, filesize($tmp_name)); fclose($fp); if ($xml_data === false) { From 45ff495c8932ecf08406ebfd4d46ae9d320a20fe Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Sun, 8 Mar 2026 03:55:29 -0700 Subject: [PATCH 4/4] chore: add CHANGELOG entry for develop Signed-off-by: Thomas Vincent --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7538fc0..a3b4433 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ --- develop --- -* issue#277: Centralize XML import payload loading for alert/report/removal rule imports +* issue#277: Add shared helper for loading XML import payload from text input or upload * issue: Making changes to support Cacti 1.3 * issue: Don't use MyISAM for non-analytical tables * issue: The install advisor for Syslog was broken in current Cacti releases