Project

General

Profile

Actions

Bug #9074

closed

Alias URL lists only storing last-most list in config.

Added by Paul Bramhall almost 6 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Category:
Aliases / Tables
Target version:
Start date:
10/28/2018
Due date:
% Done:

100%

Estimated time:
Plus Target Version:
Release Notes:
Affected Version:
2.4.x
Affected Architecture:
All

Description

When creating an Alias URL list under Firewall->Aliases->URLs, only the IP's from the last-most URL in the list is what is stored in the /cf/conf/config.xml file. The other previous URLs in the list, even though correctly validated & checked, are not kept.

It looks as though the issue lies within the /usr/local/www/firewall_aliases_edit.php file. The issue appears to be that during the loop where each url is downloaded & parsed, the $addresses array is replaced & not appended to when each URL is pulled in & validated. (lines 292 to 299)

 if (file_exists("{$temp_filename}/aliases")) {
         $address = parse_aliases_file("{$temp_filename}/aliases", $_POST['type'], 5000);
         if ($address == null) {
                 /* nothing was found */
                 $input_errors[] = sprintf(gettext("A valid URL must be provided. Could not fetch usable data from '%s'."), $_POST['address' . $x]);
         }
         mwexec("/bin/rm -rf " . escapeshellarg($temp_filename));
 } else {

The following patch solves the issue for me, where the array is appended during each iteration.

@@ -253,6 +253,7 @@
         for ($x = 0; $x < $max_alias_addresses - 1; $x++) {
             $_POST['address' . $x] = trim($_POST['address' . $x]);
             if ($_POST['address' . $x]) {
+                $t_address = array();
                 /* fetch down and add in */
                 $temp_filename = tempnam("{$g['tmp_path']}/", "alias_import");
                 unlink_if_exists($temp_filename);
@@ -290,10 +291,13 @@
                 }

                 if (file_exists("{$temp_filename}/aliases")) {
-                    $address = parse_aliases_file("{$temp_filename}/aliases", $_POST['type'], 5000);
-                    if ($address == null) {
+                    $t_address = parse_aliases_file("{$temp_filename}/aliases", $_POST['type'], 5000);
+                    if ($t_address == null) {
                         /* nothing was found */
                         $input_errors[] = sprintf(gettext("A valid URL must be provided. Could not fetch usable data from '%s'."), $_POST['address' . $x]);
+                    } else {
+                        $address += $t_address;
                     }
+                    unset($t_address);
                      mwexec("/bin/rm -rf " . escapeshellarg($temp_filename));
                 } else {
```                     mwexec("/bin/rm -rf " . escapeshellarg($temp_filename));
                 } else {

Actions #1

Updated by Paul Bramhall almost 6 years ago

There still appeared to be some odd behaviour with the change I did above where it was not always appending the array, the following fixes it, as instead of just appending (+=) I am now using the array_push functionality:

@@ -253,6 +253,7 @@
         for ($x = 0; $x < $max_alias_addresses - 1; $x++) {
             $_POST['address' . $x] = trim($_POST['address' . $x]);
             if ($_POST['address' . $x]) {
+                $t_address = array();
                 /* fetch down and add in */
                 $temp_filename = tempnam("{$g['tmp_path']}/", "alias_import");
                 unlink_if_exists($temp_filename);
@@ -290,10 +291,13 @@
                 }

                 if (file_exists("{$temp_filename}/aliases")) {
-                    $address = parse_aliases_file("{$temp_filename}/aliases", $_POST['type'], 5000);
-                    if ($address == null) {
+                    $t_address = parse_aliases_file("{$temp_filename}/aliases", $_POST['type'], 5000);
+                    if ($t_address == null) {
                         /* nothing was found */
                         $input_errors[] = sprintf(gettext("A valid URL must be provided. Could not fetch usable data from '%s'."), $_POST['address' . $x]);
+                    } else {
+                        array_push($address, ...$t_address);
                     }
+                    unset($t_address);
                      mwexec("/bin/rm -rf " . escapeshellarg($temp_filename));
                 } else {
Actions #3

Updated by Danilo Zrenjanin over 5 years ago

Applied https://github.com/pfsense/pfsense/pull/4002/commits/f5c56bf8189d515af203c398f473c9b3adfff98b and https://github.com/pfsense/pfsense/pull/4002/commits/9df78d6b92b98b88055c2feffc29111ee15ea2b0 via system_patches and not only the IPs from the last-most URL appeared in config.xml

Before the patch:

                <alias>
                        <name>Bug_9074_test</name>
                        <type>url</type>
                        <aliasurl>https://127.0.0.1/danilo_test.txt</aliasurl>
                        <aliasurl>https://127.0.0.1/danilo_test1.txt</aliasurl>
                        <address>1.1.1.1 8.8.8.8 8.8.4.4</address>
                        <descr><![CDATA[test]]></descr>
                        <detail><![CDATA[Entry added Sat, 02 Mar 2019 10:53:28 +0100||Entry added Sat, 02 Mar 2019 11:57:33 +0100]]></detail>
                </alias>

After the patch:

                <alias>
                        <name>Bug_9074_test</name>
                        <type>url</type>
                        <aliasurl>https://127.0.0.1/danilo_test.txt</aliasurl>
                        <aliasurl>https://127.0.0.1/danilo_test1.txt</aliasurl>
                        <address>195.252.110.81 18.211.55.24 18.210.212.54 18.191.54.162 18.219.105.141 8.8.8.8 1.1.1.1 8.8.4.4</address>
                        <descr></descr>
                        <detail><![CDATA[Entry added Sat, 02 Mar 2019 12:53:50 +0000||Entry added Sat, 02 Mar 2019 12:53:50 +0000]]></detail>
                </alias>

Looks good now.

Actions #4

Updated by Jim Pingle over 5 years ago

  • Target version set to 2.5.0
  • Affected Architecture All added
  • Affected Architecture deleted ()
Actions #5

Updated by Renato Botelho over 5 years ago

  • Assignee set to Renato Botelho

PR has been merged. Thanks!

Actions #6

Updated by Renato Botelho over 5 years ago

  • Status changed from New to Feedback
  • % Done changed from 0 to 100
Actions #7

Updated by Chris Linstruth about 5 years ago

Tested. Table populated with last URL contents under 2.4.4-p3 and both URL contents using latest snapshot. Looks good.

Actions #8

Updated by Jim Pingle about 5 years ago

  • Status changed from Feedback to Resolved
Actions #9

Updated by Jim Pingle about 5 years ago

  • Category changed from Rules / NAT to Aliases / Tables
Actions

Also available in: Atom PDF