Skip to content

refactor: centralize XML import payload loading #280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
somethingwithproof wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

refactor: centralize XML import payload loading #280

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
47c12c0
refactor: centralize XML import payload loading
somethingwithproof Mar 7, 2026
1882acb
fix: add fopen/fread error handling in syslog_get_import_xml_payload
somethingwithproof Mar 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
35 changes: 34 additions & 1 deletion functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,40 @@ 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');

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);

Comment on lines +139 to +149
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The upload-path payload loader does not handle file read failures (e.g., fopen()/filesize()/fread() can return false), which can trigger PHP warnings and pass non-string data to xml2array(). Consider checking the upload error status, reading in binary mode, and falling back to the existing redirect behavior when the file cannot be read.

Suggested change
$fp = fopen($_FILES['import_file']['tmp_name'], 'r');
$xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name']));
fclose($fp);
$tmp_name = $_FILES['import_file']['tmp_name'];
/* ensure upload was successful */
if (!isset($_FILES['import_file']['error']) || $_FILES['import_file']['error'] !== UPLOAD_ERR_OK) {
header('Location: ' . $redirect_url);
exit;
}
/* ensure this is a valid uploaded file */
if (!is_uploaded_file($tmp_name)) {
header('Location: ' . $redirect_url);
exit;
}
$fp = @fopen($tmp_name, 'rb');
if ($fp === false) {
header('Location: ' . $redirect_url);
exit;
}
$xml_data = '';
$size = @filesize($tmp_name);
if ($size !== false && $size > 0) {
/* read exactly $size bytes, guarding against partial reads */
$remaining = $size;
while ($remaining > 0 && !feof($fp)) {
$chunk = fread($fp, $remaining);
if ($chunk === false) {
fclose($fp);
header('Location: ' . $redirect_url);
exit;
}
$xml_data .= $chunk;
$remaining -= strlen($chunk);
}
} else {
/* fallback: read in chunks if filesize() is unavailable */
while (!feof($fp)) {
$chunk = fread($fp, 8192);
if ($chunk === false) {
fclose($fp);
header('Location: ' . $redirect_url);
exit;
}
$xml_data .= $chunk;
}
}
fclose($fp);
if (!is_string($xml_data) || $xml_data === '') {
header('Location: ' . $redirect_url);
exit;
}

Copilot uses AI. Check for mistakes.
Copy link
Author

@somethingwithproof somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — both fopen() and fread() return values checked.

Copy link
Author

@somethingwithproof somethingwithproof Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed -- fopen and fread failures are now checked and logged before proceeding.

if ($xml_data === false) {
cacti_log('SYSLOG ERROR: Failed to read uploaded import file', false, 'SYSTEM');
header('Location: ' . $redirect_url);
exit;
}

return $xml_data;
}

header('Location: ' . $redirect_url);
exit;
}

function syslog_is_partitioned() {
global $syslogdb_default;

Expand Down Expand Up @@ -2421,4 +2455,3 @@ function alert_replace_variables($alert, $results, $hostname = '') {

return $command;
}

14 changes: 1 addition & 13 deletions syslog_alerts.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -1009,4 +998,3 @@ function alert_import() {

header('Location: syslog_alerts.php');
}

14 changes: 1 addition & 13 deletions syslog_removal.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -810,4 +799,3 @@ function removal_import() {

header('Location: syslog_removal.php');
}

14 changes: 1 addition & 13 deletions syslog_reports.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -872,4 +861,3 @@ function report_import() {

header('Location: syslog_reports.php');
}

68 changes: 68 additions & 0 deletions tests/regression/issue277_import_payload_loader_test.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
<?php
/*
+-------------------------------------------------------------------------+
| Copyright (C) 2004-2026 The Cacti Group |
| |
| This program is free software; you can redistribute it and/or |
| modify it under the terms of the GNU General Public License |
| as published by the Free Software Foundation; either version 2 |
| of the License, or (at your option) any later version. |
| |
| This program is distributed in the hope that it will be useful, |
| but WITHOUT ANY WARRANTY; without even the implied warranty of |
| MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
| GNU General Public License for more details. |
+-------------------------------------------------------------------------+
| Cacti: The Complete RRDTool-based Graphing Solution |
+-------------------------------------------------------------------------+
| This code is designed, written, and maintained by the Cacti Group. See |
| about.php and/or the AUTHORS file for specific developer information. |
+-------------------------------------------------------------------------+
| http://www.cacti.net/ |
+-------------------------------------------------------------------------+
*/

$root = dirname(__DIR__, 2);

$functions = file_get_contents($root . '/functions.php');
if ($functions === false) {
fwrite(STDERR, "Failed to load functions.php\n");
exit(1);
}
Comment on lines +1 to +31
Copy link

Copilot AI Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new test file is missing the standard Cacti GPL/copyright header block that appears at the top of other PHP files in this plugin (e.g., syslog_reports.php:1-23, functions.php:1-23). Adding it keeps licensing/attribution consistent across the codebase.

Copilot uses AI. Check for mistakes.
Copy link
Author

@somethingwithproof somethingwithproof Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed — GPL header added.

Copy link
Author

@somethingwithproof somethingwithproof Mar 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed -- standard Cacti GPL header added to the test file.


if (strpos($functions, 'function syslog_get_import_xml_payload(') === false) {
fwrite(STDERR, "Shared import payload loader helper is missing.\n");
exit(1);
}

if (strpos($functions, "trim(get_nfilter_request_var('import_text')) != ''") === false) {
fwrite(STDERR, "Shared import payload loader is missing trimmed text handling.\n");
exit(1);
}

$targets = array(
$root . '/syslog_alerts.php',
$root . '/syslog_reports.php',
$root . '/syslog_removal.php'
);

foreach ($targets as $target) {
$content = file_get_contents($target);

if ($content === false) {
fwrite(STDERR, "Failed to load $target\n");
exit(1);
}

if (strpos($content, 'syslog_get_import_xml_payload(') === false) {
fwrite(STDERR, "Shared import payload loader is not used in $target\n");
exit(1);
}

if (strpos($content, "\$_FILES['import_file']['tmp_name']") !== false) {
fwrite(STDERR, "Legacy per-file upload payload loading remains in $target\n");
exit(1);
}
}

echo "issue277_import_payload_loader_test passed\n";