Skip to content

refactor: centralize XML import payload loading#280

Open
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:refactor/shared-import-payload-loader
Open

refactor: centralize XML import payload loading#280
somethingwithproof wants to merge 2 commits intoCacti:developfrom
somethingwithproof:refactor/shared-import-payload-loader

Conversation

@somethingwithproof
Copy link

Summary

Fixes #277

Validation

  • php -l functions.php
  • php -l syslog_alerts.php
  • php -l syslog_reports.php
  • php -l syslog_removal.php
  • php -l tests/regression/issue277_import_payload_loader_test.php
  • php tests/regression/issue277_import_payload_loader_test.php

Copilot AI review requested due to automatic review settings March 7, 2026 03:03
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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(), and removal_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.

Comment on lines +139 to +142
$fp = fopen($_FILES['import_file']['tmp_name'], 'r');
$xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name']));
fclose($fp);

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

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.

Comment on lines +1 to +9
<?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);
}
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

Choose a reason for hiding this comment

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

Fixed — GPL header added.

somethingwithproof and others added 2 commits March 7, 2026 05:32
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>
@somethingwithproof somethingwithproof force-pushed the refactor/shared-import-payload-loader branch from 052bf88 to 1882acb Compare March 7, 2026 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

refactor: centralize XML import payload loading logic for alert/report/removal rules

2 participants