refactor: centralize XML import payload loading#280
refactor: centralize XML import payload loading#280somethingwithproof wants to merge 2 commits intoCacti:developfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR centralizes the duplicated “XML import payload loading” logic (text area vs uploaded file, otherwise redirect/exit) into a shared helper in functions.php, and updates the alert/report/removal import handlers to use it—addressing issue #277.
Changes:
- Add
syslog_get_import_xml_payload($redirect_url)helper to load XML from request text or uploaded file, otherwise redirect/exit. - Update
alert_import(),report_import(), andremoval_import()to use the shared helper. - Add a regression test ensuring all three handlers call the helper and remove legacy per-file upload handling; update changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
functions.php |
Introduces shared XML import payload loader helper. |
syslog_alerts.php |
Replaces duplicated payload loading with helper call in alert_import(). |
syslog_reports.php |
Replaces duplicated payload loading with helper call in report_import(). |
syslog_removal.php |
Replaces duplicated payload loading with helper call in removal_import(). |
tests/regression/issue277_import_payload_loader_test.php |
Adds regression test to enforce helper usage and legacy logic removal. |
CHANGELOG.md |
Documents issue #277 change. |
| $fp = fopen($_FILES['import_file']['tmp_name'], 'r'); | ||
| $xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name'])); | ||
| fclose($fp); | ||
|
|
There was a problem hiding this comment.
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.
| $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; | |
| } |
There was a problem hiding this comment.
Fixed — both fopen() and fread() return values checked.
| <?php | ||
|
|
||
| $root = dirname(__DIR__, 2); | ||
|
|
||
| $functions = file_get_contents($root . '/functions.php'); | ||
| if ($functions === false) { | ||
| fwrite(STDERR, "Failed to load functions.php\n"); | ||
| exit(1); | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Fixed — GPL header added.
Fixes Cacti#277 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
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 <thomasvincent@gmail.com>
052bf88 to
1882acb
Compare
Summary
Fixes #277
Validation