Skip to content

Commit b577308

Browse files
somethingwithproofsomethingwithproof
committed
fix: trim xml import payload value before emptiness check
Refs #272 Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
1 parent 9f6715d commit b577308

5 files changed

Lines changed: 155 additions & 3 deletions

File tree

‎syslog_alerts.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -939,7 +939,20 @@ function import() {
939939
}
940940

941941
function alert_import() {
942-
$xml_data = syslog_get_import_xml_payload('syslog_alerts.php?header=false');
942+
$import_text = get_nfilter_request_var('import_text');
943+
944+
if (trim($import_text) != '') {
945+
/* textbox input */
946+
$xml_data = $import_text;
947+
} elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) {
948+
/* file upload */
949+
$fp = fopen($_FILES['import_file']['tmp_name'],'r');
950+
$xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name']));
951+
fclose($fp);
952+
} else {
953+
header('Location: syslog_alerts.php?header=false');
954+
exit;
955+
}
943956

944957
$xml_array = xml2array($xml_data);
945958

‎syslog_removal.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -739,7 +739,20 @@ function import() {
739739
}
740740

741741
function removal_import() {
742-
$xml_data = syslog_get_import_xml_payload('syslog_removal.php?header=false');
742+
$import_text = get_nfilter_request_var('import_text');
743+
744+
if (trim($import_text) != '') {
745+
/* textbox input */
746+
$xml_data = $import_text;
747+
} elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) {
748+
/* file upload */
749+
$fp = fopen($_FILES['import_file']['tmp_name'],'r');
750+
$xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name']));
751+
fclose($fp);
752+
} else {
753+
header('Location: syslog_removal.php?header=false');
754+
exit;
755+
}
743756

744757
/* obtain debug information if it's set */
745758
$xml_array = xml2array($xml_data);

‎syslog_reports.php‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -801,7 +801,20 @@ function import() {
801801
}
802802

803803
function report_import() {
804-
$xml_data = syslog_get_import_xml_payload('syslog_reports.php?header=false');
804+
$import_text = get_nfilter_request_var('import_text');
805+
806+
if (trim($import_text) != '') {
807+
/* textbox input */
808+
$xml_data = $import_text;
809+
} elseif (($_FILES['import_file']['tmp_name'] != 'none') && ($_FILES['import_file']['tmp_name'] != '')) {
810+
/* file upload */
811+
$fp = fopen($_FILES['import_file']['tmp_name'],'r');
812+
$xml_data = fread($fp, filesize($_FILES['import_file']['tmp_name']));
813+
fclose($fp);
814+
} else {
815+
header('Location: syslog_reports.php?header=false');
816+
exit;
817+
}
805818

806819
/* obtain debug information if it's set */
807820
$xml_array = xml2array($xml_data);

‎tests/regression/issue269_import_text_branch_logic_test.php‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
<?php
2+
3+
/*
4+
* Regression test for issue #269 -- branch-logic invariants.
5+
*
6+
* These assertions verify the structural properties that make whitespace-only
7+
* input fall through to the file-upload branch instead of the textbox branch,
8+
* and that a non-empty payload is assigned to $xml_data without further
9+
* modification. Pure source inspection: the functions themselves cannot be
10+
* called in isolation because they depend on the Cacti runtime.
11+
*/
12+
13+
$root = dirname(__DIR__, 2);
14+
$targets = array(
15+
'alert_import' => $root . '/syslog_alerts.php',
16+
'removal_import' => $root . '/syslog_removal.php',
17+
'report_import' => $root . '/syslog_reports.php',
18+
);
19+
20+
foreach ($targets as $func => $target) {
21+
$content = file_get_contents($target);
22+
23+
if ($content === false) {
24+
fwrite(STDERR, "Failed to load $target\n");
25+
exit(1);
26+
}
27+
28+
/*
29+
* 1. The request variable must be captured into a local first.
30+
* Whitespace-only input falls through only because trim() is applied
31+
* to the local; if the assignment were missing the condition would
32+
* be wrong.
33+
*/
34+
if (!preg_match('/\$import_text\s*=\s*get_nfilter_request_var\s*\(\s*\'import_text\'\s*\)/', $content)) {
35+
fwrite(STDERR, "$func: \$import_text assignment via get_nfilter_request_var missing in $target\n");
36+
exit(1);
37+
}
38+
39+
/*
40+
* 2. The branch condition must trim the local variable, not the raw
41+
* request call. This is what makes whitespace-only values fall
42+
* through to the file-upload branch.
43+
*/
44+
if (!preg_match('/trim\s*\(\s*\$import_text\s*\)\s*!=\s*\'\'/', $content)) {
45+
fwrite(STDERR, "$func: trim(\$import_text) != '' condition missing in $target\n");
46+
exit(1);
47+
}
48+
49+
/*
50+
* 3. Inside the textbox branch, $xml_data must be assigned the
51+
* untrimmed local. A non-empty payload is preserved as-is.
52+
*/
53+
if (!preg_match('/\$xml_data\s*=\s*\$import_text\s*;/', $content)) {
54+
fwrite(STDERR, "$func: \$xml_data = \$import_text assignment missing in $target\n");
55+
exit(1);
56+
}
57+
58+
/*
59+
* 4. The file-upload branch must still exist (elseif on $_FILES).
60+
* Ensures the fallback path was not accidentally removed.
61+
*/
62+
if (!preg_match('/elseif\s*\(\s*\(\s*\$_FILES\s*\[/', $content)) {
63+
fwrite(STDERR, "$func: \$_FILES elseif branch missing in $target\n");
64+
exit(1);
65+
}
66+
}
67+
68+
echo "issue269_import_text_branch_logic_test passed\n";

‎tests/regression/issue269_import_text_trim_check_test.php‎

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
<?php
2+
3+
$root = dirname(__DIR__, 2);
4+
$targets = array(
5+
$root . '/syslog_alerts.php',
6+
$root . '/syslog_reports.php',
7+
$root . '/syslog_removal.php'
8+
);
9+
10+
$legacy = "trim(get_nfilter_request_var('import_text') != '')";
11+
12+
foreach ($targets as $target) {
13+
$content = file_get_contents($target);
14+
15+
if ($content === false) {
16+
fwrite(STDERR, "Failed to load $target\n");
17+
exit(1);
18+
}
19+
20+
if (strpos($content, $legacy) !== false) {
21+
fwrite(STDERR, "Legacy import_text trim/comparison bug remains in $target\n");
22+
exit(1);
23+
}
24+
25+
$fixedPattern = '/trim\s*\(\s*\$import_text\s*\)\s*!=\s*\'\'/';
26+
if (!preg_match($fixedPattern, $content)) {
27+
fwrite(STDERR, "Fixed import_text trim/comparison check missing in $target\n");
28+
exit(1);
29+
}
30+
31+
/* After the local $import_text assignment, there must be no second
32+
get_nfilter_request_var('import_text') call. A duplicate call
33+
would bypass the cached local variable. */
34+
$needle = "\$import_text = get_nfilter_request_var('import_text')";
35+
$assignPos = strpos($content, $needle);
36+
if ($assignPos !== false) {
37+
$afterAssign = substr($content, $assignPos + strlen($needle));
38+
if (preg_match('/get_nfilter_request_var\s*\(\s*\'import_text\'\s*\)/', $afterAssign)) {
39+
fwrite(STDERR, "Redundant get_nfilter_request_var('import_text') call after local assignment in $target\n");
40+
exit(1);
41+
}
42+
}
43+
}
44+
45+
echo "issue269_import_text_trim_check_test passed\n";

0 commit comments

Comments
 (0)