[PATCH pve-storage v2 27/50] tree-wide: partially replace usages of regexes for 'import' vtype

["Max R. Carrara" <m.carrara@proxmox.com>]

Add partial support for the 'import' volume type in
`PVE::Storage::Common::Parse`. What remains unsupported right now is
parsing volume names for that type, as those require a little more
care.

Replace most usages of the `PVE::Storage::IMPORT_EXT_RE_1` regex with
parsing functions from `::Common::Parse`.

Replace the one remaining spot where we use the
`PVE::Storage::UPLOAD_IMPORT_EXT_RE_1` regex with a parsing function
as well. Note the removal of this regex in `ApiChangeLog` and add a
corresponding FIXME comment for removing it on the next APIAGE reset.

It is important to note that the `UPLOAD_IMPORT_EXT_RE_1` regex only
exists to handle the special case of excluding .ovf files from the
upload / download_url API methods. Therefore, instead of adding a
separate parser (or a flag etc.) to handle this case, just parse the
path of the up-/downloaded file regardless. Then, raise a parameter
exception when the `ovf` file extension is encountered.

Also, since the `PVE::Storage::SAFE_CHAR_CLASS_RE` regex is now
unused, migrate it to `::Common::Parse` and allow it to be obtained by
using a new helper subroutine. Note this in `ApiChangeLog` as well,
and add a corresponding FIXME comment, too.

The main reason for moving `SAFE_CHAR_CLASS_RE` and making it private
is to discourage "shotgun parsing" throughout the repo and keeping
most of our parsing code in `::Common::Parse` -- just like the other
regexes that are removed.

Additionally, we avoid duplicating the regex that way. An alternative
would have been to import it from `PVE::Storage` and use it in
`::Common::Parse`, but that would have led to cyclic imports. That
approach would not have solved the shotgun parsing problem, though.

However, since that particular regex is still important for
third-party plugin authors (for backup provider plugins in particular)
and perhaps for some isolated parsing subs that don't *really* fit
into `::Common::Parse` in the future, allowing it to be obtained
through a subroutine makes its usage much more explicit. As a side
effect, putting the regex behind a subroutine prevents it from being
altered.

Finally, add a bunch of test cases that target the `import` volume
type for the `list_volumes()` plugin API method. For all of these
tests, sort the expected and resulting list items by their volume ID
in order to make the output comparison deterministic. Iterate over the
declared content / volume types of the mocked storage config instead
of using a fixed list of vtypes as well.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 ApiChangeLog                          |   9 ++
 src/PVE/API2/Storage/Status.pm        |  13 +-
 src/PVE/BackupProvider/Plugin/Base.pm |   6 +-
 src/PVE/Storage.pm                    |   9 +-
 src/PVE/Storage/Common/Parse.pm       |  53 ++++++++
 src/PVE/Storage/Plugin.pm             |   9 +-
 src/test/list_volumes_test.pm         | 180 +++++++++++++++++++++++++-
 7 files changed, 259 insertions(+), 20 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index dc77bf41..b1b6b88e 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -32,6 +32,7 @@ Future changes should be documented in here.
   * `$PVE::Storage::ISO_EXT_RE_0` (`iso` volume type)
   * `$PVE::Storage::VZTMPL_EXT_RE_1` (`vztmpl` volume type)
   * `$PVE::Storage::BACKUP_EXT_RE_2` (`backup` volume type)
+  * `$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1` (`import` volume type)
 
   These regular expressions were mostly used for parsing file paths and volume
   names corresponding to their volume types, noted in parentheses above.
@@ -39,6 +40,14 @@ Future changes should be documented in here.
   Instead, The parsing functions in `PVE::Storage::Common::Parse` should be
   used.
 
+* Migrate the following regular expressions:
+  * `$PVE::Storage::SAFE_CHAR_CLASS_RE`
+
+  These regular expressions are now defined as private constants in the
+  `PVE::Storage::Common::Parse` module and can be obtained by using the
+  corresponding subroutines in the *STANDALONE REGULAR EXPRESSIONS* section in
+  said module.
+
 ## Version 15:
 
 * Add new `$snapname` parameter to the `volume_resize()` plugin method
diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
index 60f08628..d2451898 100644
--- a/src/PVE/API2/Storage/Status.pm
+++ b/src/PVE/API2/Storage/Status.pm
@@ -98,13 +98,18 @@ my sub parse_transferred_file_path_extension : prototype($$) {
     }
 
     if ($vtype eq 'import') {
-        if (
-            $path !~ m!${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::UPLOAD_IMPORT_EXT_RE_1$!
-        ) {
+        my $parts = parse_rel_path_as_volname_parts($path, $vtype);
+
+        if (!defined($parts)) {
             raise_param_exc({ filename => "invalid filename or wrong extension" });
         }
 
-        my $ext = $1;
+        my $ext = $parts->{ext};
+
+        if ($ext eq 'ovf') {
+            raise_param_exc({ filename => "wrong file extension" });
+        }
+
         return $ext;
     }
 
diff --git a/src/PVE/BackupProvider/Plugin/Base.pm b/src/PVE/BackupProvider/Plugin/Base.pm
index 24b1421e..addd1a55 100644
--- a/src/PVE/BackupProvider/Plugin/Base.pm
+++ b/src/PVE/BackupProvider/Plugin/Base.pm
@@ -247,9 +247,9 @@ determined for the backup task and returned to the caller via a hash reference:
     my $res = $backup_provider->backup_init($vmid, $vmtype, $start_time);
     my $archive_name = $res->{'archive-name'};
 
-The archive name must contain only characters from the
-C<$PVE::Storage::SAFE_CHAR_CLASS_RE> character class as well as forward slash
-C</> and colon C<:>.
+The archive name must contain only characters from the character class returned
+by C<L<< get_re_safe_char_class()|PVE::Storage::Common::Parse/"get_re_safe_char_class" >>>
+as well as forward slash C</> and colon C<:>.
 
 Use C<$self> to remember it for the C<backup_container()> or C<backup_vm()>
 method that will be called later.
diff --git a/src/PVE/Storage.pm b/src/PVE/Storage.pm
index 675370a6..ce90f942 100755
--- a/src/PVE/Storage.pm
+++ b/src/PVE/Storage.pm
@@ -27,6 +27,7 @@ use PVE::Storage::Common qw(
     plugin_get_vtype_subdir
 );
 use PVE::Storage::Common::Parse qw(
+    get_re_safe_char_class
     parse_abs_path_as_volid
 );
 use PVE::RESTEnvironment qw(log_warn);
@@ -131,9 +132,11 @@ our $BACKUP_EXT_RE_2 = qr/\.(tgz|(?:tar|vma)(?:\.(${\PVE::Storage::Plugin::COMPR
 
 our $IMPORT_EXT_RE_1 = qr/\.(ova|ovf|qcow2|raw|vmdk)/;
 
+# FIXME: remove this regex on the next APIAGE reset.
 our $UPLOAD_IMPORT_EXT_RE_1 = qr/\.(ova|qcow2|raw|vmdk)/;
 
-our $SAFE_CHAR_CLASS_RE = qr/[a-zA-Z0-9\-\.\+\=\_]/;
+# FIXME: remove this regex on the next APIAGE reset.
+our $SAFE_CHAR_CLASS_RE = get_re_safe_char_class();
 our $SAFE_CHAR_WITH_WHITESPACE_CLASS_RE = qr/[ a-zA-Z0-9\-\.\+\=\_]/;
 
 our $OVA_CONTENT_RE_1 = qr/${SAFE_CHAR_WITH_WHITESPACE_CLASS_RE}+\.(qcow2|raw|vmdk)/;
@@ -776,9 +779,7 @@ sub path_to_volume_id {
         }
 
         if ($vtype eq 'import') {
-            return if $filename !~ m!/(${SAFE_CHAR_CLASS_RE}+${IMPORT_EXT_RE_1})$!;
-            my $name = $1;
-            return "$sid:import/$name";
+            return parse_abs_path_as_volid($sid, $scfg, $path, $vtype);
         }
 
         return;
diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm
index 8e9769fb..2193aa65 100644
--- a/src/PVE/Storage/Common/Parse.pm
+++ b/src/PVE/Storage/Common/Parse.pm
@@ -9,6 +9,8 @@ use PVE::Storage::Common qw(
 use Exporter qw(import);
 
 our @EXPORT_OK = qw(
+    get_re_safe_char_class
+
     parse_rel_path_as_volname_parts
     parse_rel_path_as_volname
     parse_volname_as_parts
@@ -37,6 +39,8 @@ my @VZTMPL_COMPRESSION_EXTENSIONS = ('gz', 'xz', 'zst', 'bz2');
 
 my @BACKUP_COMPRESSION_EXTENSIONS = ('gz', 'lzo', 'zst', 'bz2');
 
+my @IMPORT_EXTENSIONS = ('ova', 'ovf', 'qcow2', 'raw', 'vmdk');
+
 my sub join_to_re_alternations(@list) {
     return join('|', map { quotemeta } @list);
 }
@@ -45,6 +49,10 @@ my $RE_VZTMPL_COMPRESSION_EXTENSIONS = join_to_re_alternations(@VZTMPL_COMPRESSI
 
 my $RE_BACKUP_COMPRESSION_EXTENSIONS = join_to_re_alternations(@BACKUP_COMPRESSION_EXTENSIONS);
 
+my $RE_IMPORT_EXTENSIONS = join_to_re_alternations(@IMPORT_EXTENSIONS);
+
+my $RE_SAFE_CHAR_CLASS = qr/[a-zA-Z0-9\-\.\+\=\_]/;
+
 my $RE_PARENT_DIR = quotemeta('..');
 my $RE_CONTAINS_PARENT_DIR = qr!
     ( ^$RE_PARENT_DIR/ )  #  ../ --> Beginning of path
@@ -112,11 +120,18 @@ my $RE_SNIPPETS_FILE_PATH = qr!
     )
 !xn;
 
+my $RE_IMPORT_FILE_PATH = qr!
+    (?<path>
+        (?<file> ($RE_SAFE_CHAR_CLASS)+ \. (?<ext> $RE_IMPORT_EXTENSIONS) )
+    )
+!xn;
+
 my $RE_FILE_PATH_FOR_VTYPE = {
     iso => qr/^$RE_ISO_FILE_PATH$/,
     vztmpl => qr/^$RE_VZTMPL_FILE_PATH$/,
     backup => qr/^$RE_BACKUP_FILE_PATH$/,
     snippets => qr/^$RE_SNIPPETS_FILE_PATH$/,
+    import => qr/^$RE_IMPORT_FILE_PATH$/,
 };
 
 my $RE_VOLNAME_FOR_VTYPE = {
@@ -170,6 +185,44 @@ my sub split_leading_dir_from_path($path, $directory) {
     return;
 }
 
+=head1 STANDALONE REGULAR EXPRESSIONS
+
+Some of the regular expressions that this module defines can be obtained using
+the subroutines listed below.
+
+B<These subroutines are primarily intended to be used by third-party plugins.>
+
+Parsing code of inbuilt plugins and other modules in the C<L<PVE::Storage>>
+namespace should instead use the constants defined in
+C<L<PVE::Storage::Common::Parse>> directly, and keep the parsing code in there
+as well.
+
+=cut
+
+=head3 get_re_safe_char_class
+
+Returns a regex consisting of a single character class that matches any
+character that is safe to use in more critical places, such as untrusted file
+names.
+
+You would typically use this to obtain a constant that you can use in your own
+parsing code:
+
+
+    my $RE_SAFE_CHAR_CLASS = get_re_safe_char_class();
+
+    # [...]
+
+    my sub is_valid_file_name($file_name) {
+        return $file_name =~ m/${RE_SAFE_CHAR_CLASS}+ \. (json|toml)/xn;
+    }
+
+=cut
+
+sub get_re_safe_char_class() {
+    return $RE_SAFE_CHAR_CLASS;
+}
+
 =head1 PARSERS RELATED TO VOLUMES
 
 The parsing functions in this section primarily deal with parsing data related
diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index b327e4d2..afddca73 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -1799,13 +1799,12 @@ my sub get_subdir_files {
         }
 
         if ($vtype eq 'import') {
-            return
-                if $filename !~
-                m!/(${PVE::Storage::SAFE_CHAR_CLASS_RE}+$PVE::Storage::IMPORT_EXT_RE_1)$!i;
+            my $parts = parse_abs_path_as_volid_parts($storeid, $scfg, $path, $vtype);
+            return if !defined($parts);
 
             return {
-                volid => "$storeid:import/$1",
-                format => "$2",
+                volid => $parts->{volid},
+                format => $parts->{ext},
             };
         }
 
diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
index 08769027..5cb08880 100644
--- a/src/test/list_volumes_test.pm
+++ b/src/test/list_volumes_test.pm
@@ -72,6 +72,7 @@ my $scfg = {
         'images' => 1,
         'snippets' => 1,
         'backup' => 1,
+        'import' => 1,
     },
 };
 
@@ -462,6 +463,171 @@ my @tests = (
         ],
         expected => [], # returns empty list
     },
+    {
+        description => 'VMID: none, valid file names for import',
+        vmid => undef,
+        files => [
+            "$storage_dir/import/import.ova",
+            "$storage_dir/import/import.ovf",
+            "$storage_dir/import/some-disk.qcow2",
+            "$storage_dir/import/some-disk.vmdk",
+            "$storage_dir/import/some-raw-disk.raw",
+        ],
+        expected => [
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/import.ova",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ovf',
+                size => DEFAULT_SIZE,
+                volid => "local:import/import.ovf",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'qcow2',
+                size => DEFAULT_SIZE,
+                volid => "local:import/some-disk.qcow2",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'vmdk',
+                size => DEFAULT_SIZE,
+                volid => "local:import/some-disk.vmdk",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'raw',
+                size => DEFAULT_SIZE,
+                volid => "local:import/some-raw-disk.raw",
+            },
+        ],
+    },
+    {
+        description => 'VMID: none, non-matching file paths for import',
+        vmid => undef,
+        files => [
+            # Malformed file names
+            "$storage_dir/import/import.ovff",
+            "$storage_dir/import/importova",
+            "$storage_dir/import/import.ov",
+            "$storage_dir/import/diskraw",
+            "$storage_dir/import/diskvmdk",
+            "$storage_dir/import/disk.invalid",
+            "$storage_dir/import/.ova",
+            "$storage_dir/import/.raw",
+            # Trailing whitespace must not be trimmed
+            "$storage_dir/import/import.ova\t",
+            "$storage_dir/import/disk.raw    ",
+            # Whitespace in file name
+            "$storage_dir/import/something I want to import.ova",
+            "$storage_dir/import/ .raw",
+            "$storage_dir/import/ disk .vmdk",
+            "$storage_dir/import/disk .qcow2",
+            "$storage_dir/import/ import.ova",
+            # Unsafe characters in file name
+            "$storage_dir/import/linux🐧-vm.ova",
+            "$storage_dir/import/🐪perl-playground🐪.ova",
+            "$storage_dir/import/fish_<><_<><_<><.ova",
+            $storage_dir . '/import/C:\\\\Windows\\Path.ova',
+            # Content inside .ova files may only be specified as part
+            # of volume names, and may never appear when looked up as
+            # a file path
+            "$storage_dir/import/import.ova/disk.qcow2",
+            "$storage_dir/import/import.ova/disk.raw",
+            "$storage_dir/import/import.ova/disk.vmdk",
+            "$storage_dir/import/import.ova/disk.invalid",
+        ],
+        expected => [], # returns empty list
+    },
+    {
+        description => 'VMID: none, weird but valid file names for import',
+        vmid => undef,
+        files => [
+            "$storage_dir/import/import.ova.ova",
+            "$storage_dir/import/import.ova.ova.ova",
+            "$storage_dir/import/import.ova.ova.ova.ova",
+            "$storage_dir/import/ova.ova",
+            "$storage_dir/import/ova.ovf",
+            "$storage_dir/import/ova.vmdk",
+            "$storage_dir/import/raw.raw.qcow2",
+            "$storage_dir/import/raw.raw.qcow2.import.qcow2",
+            "$storage_dir/import/raw.raw.raw.your-boat.ova",
+        ],
+        expected => [
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/import.ova.ova",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/import.ova.ova.ova",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/import.ova.ova.ova.ova",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/ova.ova",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ovf',
+                size => DEFAULT_SIZE,
+                volid => "local:import/ova.ovf",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'vmdk',
+                size => DEFAULT_SIZE,
+                volid => "local:import/ova.vmdk",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'qcow2',
+                size => DEFAULT_SIZE,
+                volid => "local:import/raw.raw.qcow2",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'qcow2',
+                size => DEFAULT_SIZE,
+                volid => "local:import/raw.raw.qcow2.import.qcow2",
+            },
+            {
+                content => 'import',
+                ctime => DEFAULT_CTIME,
+                format => 'ova',
+                size => DEFAULT_SIZE,
+                volid => "local:import/raw.raw.raw.your-boat.ova",
+            },
+        ],
+    },
 );
 
 # provide static vmlist for tests
@@ -497,6 +663,10 @@ $mock_fsi->redefine(
     },
 );
 
+my sub cmp_volinfo_by_volid {
+    return $a->{volid} cmp $b->{volid};
+}
+
 my $plan = scalar @tests;
 plan tests => $plan + 1;
 
@@ -520,14 +690,14 @@ plan tests => $plan + 1;
 
 {
     my $sid = 'local';
-    my $types = ['rootdir', 'images', 'vztmpl', 'iso', 'backup', 'snippets'];
+    my $types = [grep { $scfg->{content}->{$_} } keys $scfg->{content}->%*];
     my @suffixes = ('qcow2', 'raw', 'vmdk', 'vhdx');
 
     # run through test cases
     foreach my $tt (@tests) {
         my $vmid = $tt->{vmid};
         my $files = $tt->{files};
-        my $expected = $tt->{expected};
+        my $expected = [sort cmp_volinfo_by_volid $tt->{expected}->@*];
         my $description = $tt->{description};
         my $parent = $tt->{parent};
 
@@ -550,8 +720,10 @@ plan tests => $plan + 1;
             }
         }
 
-        my $got;
-        eval { $got = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types) };
+        my $got = eval {
+            my $volume_list = PVE::Storage::Plugin->list_volumes($sid, $scfg, $vmid, $types);
+            return [sort cmp_volinfo_by_volid $volume_list->@*];
+        };
         $got = $@ if $@;
 
         is_deeply($got, $expected, $description) || diag(explain($got));
-- 
2.47.3