[PATCH pve-storage v1 51/54] test: add more tests for 'import' vtype & guard against nested subdirs

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

Supporting nested subdirectories for the 'import' volume type is
currently not possible, because of a parsing ambiguity that would be
introduced. In particular, it would not be possible to differentiate
whether 'foo.ova' in 'foo.ova/bar.qcow2' refers to a directory ending
with '.ova' or an actual '.ova' file through parsing alone. Note this
down in a comment as well, so it doesn't get overlooked.

Therefore, instead of supporting nested subdirectories for the
'import' volume type, add tests that explicitly guard against
subdirectory scanning support.

Additionally, toss in a few extra tests for the `filesystem_path()`
plugin API method and also for the differences in whitespace handling
for file paths and volume names for the 'import' volume type.

Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
---
 src/PVE/Storage/Common/Parse.pm             |   7 ++
 src/PVE/Storage/Common/test/parser_tests.pl |  40 +++++++
 src/test/filesystem_path_test.pm            |  14 +++
 src/test/list_volumes_test.pm               |  65 +++++++++++
 src/test/parse_volname_test.pm              | 120 ++++++++++++++++++++
 src/test/path_to_volume_id_test.pm          |  13 +++
 6 files changed, 259 insertions(+)

diff --git a/src/PVE/Storage/Common/Parse.pm b/src/PVE/Storage/Common/Parse.pm
index 55fbc129..6bab7e28 100644
--- a/src/PVE/Storage/Common/Parse.pm
+++ b/src/PVE/Storage/Common/Parse.pm
@@ -131,6 +131,13 @@ my $RE_SNIPPETS_FILE_PATH = qr!
     )
 !xn;

+# NOTE: Supporting subdirectories would introduce a parsing ambiguity with
+# volume names; in particular, there would be no way to tell whether 'foo.ova'
+# in 'foo.ova/bar.qcow2' refers to a directory or a file just through parsing
+# alone.
+#
+# Therefore, neither RE_IMPORT_FILE_PATH nor RE_IMPORT_VOLNAME currently
+# accept subdirectory components.
 my $RE_IMPORT_FILE_PATH = qr!
     (?<path>
         (?<file> ($RE_SAFE_CHAR_CLASS)+ \. (?<ext> $RE_IMPORT_EXTENSIONS) )
diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl
index 38d5ebac..a788f2ab 100755
--- a/src/PVE/Storage/Common/test/parser_tests.pl
+++ b/src/PVE/Storage/Common/test/parser_tests.pl
@@ -634,6 +634,46 @@ my $volname_cases_import_invalid = [
         },
         expected => undef,
     },
+    {
+        description => "Subdirectory (import)",
+        args => {
+            path => 'subdir/disk.ovf',
+            vtype => 'import',
+        },
+        expected => undef,
+    },
+    {
+        description => "Nested subdirectory (import)",
+        args => {
+            path => 'deeply/nested/subdir/disk.raw',
+            vtype => 'import',
+        },
+        expected => undef,
+    },
+    {
+        description => "Parent dir reference in path (beginning) (import)",
+        args => {
+            path => '../disk.ova',
+            vtype => 'import',
+        },
+        expected => undef,
+    },
+    {
+        description => "Parent dir reference in path (middle) (import)",
+        args => {
+            path => 'subdir/../disk.raw',
+            vtype => 'import',
+        },
+        expected => undef,
+    },
+    {
+        description => "Parent dir reference in path (end) (import)",
+        args => {
+            path => 'subdir/disk.qcow2/..',
+            vtype => 'import',
+        },
+        expected => undef,
+    },
 ];

 my $cases_valid_all = [
diff --git a/src/test/filesystem_path_test.pm b/src/test/filesystem_path_test.pm
index ff6dffe1..f06682a9 100644
--- a/src/test/filesystem_path_test.pm
+++ b/src/test/filesystem_path_test.pm
@@ -98,6 +98,20 @@ my $tests = [
         expected =>
             ["$DEFAULT_STORAGE_DIR/snippets/foo/bar/baz/something.txt", undef, 'snippets'],
     },
+    {
+        volname => 'import/import.ova',
+        snapname => undef,
+        expected => [
+            "$DEFAULT_STORAGE_DIR/import/import.ova", undef, 'import',
+        ],
+    },
+    {
+        volname => 'import/import.ovf',
+        snapname => undef,
+        expected => [
+            "$DEFAULT_STORAGE_DIR/import/import.ovf", undef, 'import',
+        ],
+    },
 ];

 my sub run_tests($tests) {
diff --git a/src/test/list_volumes_test.pm b/src/test/list_volumes_test.pm
index edcf1ba1..b43b9cff 100644
--- a/src/test/list_volumes_test.pm
+++ b/src/test/list_volumes_test.pm
@@ -1358,6 +1358,26 @@ my $test_param_list = [
                 file => "$DEFAULT_STORAGE_PATH/snippets/1/2/3/4/5/some-hookscript.pl",
                 expected => undef,
             },
+            {
+                file => "$DEFAULT_STORAGE_PATH/import/some-import.ova",
+                expected => {
+                    content => 'import',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'ova',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:import/some-import.ova',
+                },
+            },
+            {
+                # Not allowed regardless
+                file => "$DEFAULT_STORAGE_PATH/import/1/some-import.ova",
+                expected => undef,
+            },
+            {
+                # Not allowed regardless
+                file => "$DEFAULT_STORAGE_PATH/import/1/2/3/4/5/some-import.ova",
+                expected => undef,
+            },
         ],
     },
     {
@@ -1453,6 +1473,26 @@ my $test_param_list = [
                 file => "$DEFAULT_STORAGE_PATH/snippets/1/2/some-hookscript.pl",
                 expected => undef,
             },
+            {
+                file => "$DEFAULT_STORAGE_PATH/import/some-import.ova",
+                expected => {
+                    content => 'import',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'ova',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:import/some-import.ova',
+                },
+            },
+            {
+                # Not allowed
+                file => "$DEFAULT_STORAGE_PATH/import/1/some-import.ova",
+                expected => undef,
+            },
+            {
+                # Not allowed + exceeds max-scan-depth
+                file => "$DEFAULT_STORAGE_PATH/import/1/2/some-import.ova",
+                expected => undef,
+            },
         ],
     },
     {
@@ -1580,6 +1620,31 @@ my $test_param_list = [
                 file => "$DEFAULT_STORAGE_PATH/snippets/1/2/3/4/5/6/some-hookscript.pl",
                 expected => undef,
             },
+            {
+                file => "$DEFAULT_STORAGE_PATH/import/some-import.ova",
+                expected => {
+                    content => 'import',
+                    ctime => $DEFAULT_CTIME,
+                    format => 'ova',
+                    size => $DEFAULT_SIZE,
+                    volid => 'local:import/some-import.ova',
+                },
+            },
+            {
+                # Not allowed
+                file => "$DEFAULT_STORAGE_PATH/import/1/some-import.ova",
+                expected => undef,
+            },
+            {
+                # Still not allowed
+                file => "$DEFAULT_STORAGE_PATH/import/1/2/3/4/5/some-import.ova",
+                expected => undef,
+            },
+            {
+                # Not allowed + exceeds max-scan-depth
+                file => "$DEFAULT_STORAGE_PATH/import/1/2/3/4/5/6/some-import.ova",
+                expected => undef,
+            },
         ],
     },
 ];
diff --git a/src/test/parse_volname_test.pm b/src/test/parse_volname_test.pm
index 0a425ee2..df639df3 100644
--- a/src/test/parse_volname_test.pm
+++ b/src/test/parse_volname_test.pm
@@ -64,6 +64,18 @@ my $tests = [
         expected =>
             ['import', 'import.ova/OS disk.vmdk', undef, undef, undef, undef, 'ova+vmdk'],
     },
+    {
+        description => "Import, ova file on disk with whitespace in name",
+        volname => 'import/My neat import.ova/disk.vmdk',
+        expected =>
+            ['import', 'My neat import.ova/disk.vmdk', undef, undef, undef, undef, 'ova+vmdk'],
+    },
+    {
+        description => "Import, ova file on disk and inner file of ova with whitespace in name",
+        volname => 'import/My neat import.ova/OS disk.vmdk',
+        expected =>
+            ['import', 'My neat import.ova/OS disk.vmdk', undef, undef, undef, undef, 'ova+vmdk'],
+    },
     #
     # failed matches
     #
@@ -448,6 +460,57 @@ my $tests = [

         push($tests->@*, @extra_tests);
     }
+
+    # Failed tests
+    {
+        my $file_name = "$prefix.ova";
+
+        my @extra_failed_tests = (
+            {
+                description => "Import, ova, directory",
+                volname => "import/foo/$file_name",
+                expected => "unable to parse directory volume name 'import/foo/$file_name'\n",
+            },
+            {
+                description => "Import, ova, subdirectories",
+                volname => "import/foo/bar/baz/$file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/foo/bar/baz/$file_name'\n",
+            },
+            {
+                description => "Import, ova, directory with same name as file",
+                volname => "import/$file_name/$file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/$file_name/$file_name'\n",
+            },
+            {
+                description =>
+                    "Import, ova, parent directory reference before volume type prefix",
+                volname => "../import/$file_name",
+                expected => "unable to parse directory volume name '../import/$file_name'\n",
+            },
+            {
+                description =>
+                    "Import, ova, parent directory reference at beginning of volume path",
+                volname => "import/../$file_name",
+                expected => "unable to parse directory volume name 'import/../$file_name'\n",
+            },
+            {
+                description => "Import, ova, parent directory reference at end of volume path",
+                volname => "import/$file_name/..",
+                expected => "unable to parse directory volume name 'import/$file_name/..'\n",
+            },
+            {
+                description =>
+                    "Import, ova, parent directory reference between dir components of volume path",
+                volname => "import/foo/../bar/$file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/foo/../bar/$file_name'\n",
+            },
+        );
+
+        push($tests->@*, @extra_failed_tests);
+    }
 }

 # Test cases for OVA import files with content inside
@@ -477,6 +540,63 @@ my $tests = [

         push($tests->@*, @extra_tests);
     }
+
+    # Failed tests
+    {
+        my $content_file_name = "disk.qcow2";
+
+        my @extra_failed_tests = (
+            {
+                description => "Import, inner file of ova (qcow2), directory",
+                volname => "import/foo/$file_name/$content_file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/foo/$file_name/$content_file_name'\n",
+            },
+            {
+                description => "Import, inner file of ova (qcow2), nested directories",
+                volname => "import/foo/bar/baz/$file_name/$content_file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/foo/bar/baz/$file_name/$content_file_name'\n",
+            },
+            {
+                description =>
+                    "Import, inner file of ova (qcow2), directory with same name as file",
+                volname => "import/$file_name/$file_name/$content_file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/$file_name/$file_name/$content_file_name'\n",
+            },
+            {
+                description => "Import, inner file of ova (qcow2),"
+                    . " parent directory reference before volume type prefix",
+                volname => "../import/$file_name/$content_file_name",
+                expected =>
+                    "unable to parse directory volume name '../import/$file_name/$content_file_name'\n",
+            },
+            {
+                description => "Import, inner file of ova (qcow2),"
+                    . " parent directory reference at beginning of volume path",
+                volname => "import/../$file_name/$content_file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/../$file_name/$content_file_name'\n",
+            },
+            {
+                description => "Import, inner file of ova (qcow2),"
+                    . " parent directory reference at end of disk path",
+                volname => "import/$file_name/../$content_file_name",
+                expected =>
+                    "unable to parse directory volume name 'import/$file_name/../$content_file_name'\n",
+            },
+            {
+                description => "Import, inner file of ova (qcow2),"
+                    . " parent directory reference at end of volume path",
+                volname => "import/$file_name/$content_file_name/..",
+                expected =>
+                    "unable to parse directory volume name 'import/$file_name/$content_file_name/..'\n",
+            },
+        );
+
+        push($tests->@*, @extra_failed_tests);
+    }
 }

 my sub run_tests($tests) {
diff --git a/src/test/path_to_volume_id_test.pm b/src/test/path_to_volume_id_test.pm
index d22f6272..3bfb37a6 100644
--- a/src/test/path_to_volume_id_test.pm
+++ b/src/test/path_to_volume_id_test.pm
@@ -256,6 +256,19 @@ my $tests = [
         file => "$DEFAULT_STORAGE_DIR/import/test.foo",
         expected => [''],
     },
+    {
+        # Allowed in volume names, but not in file paths
+        description => 'Import, ova, whitespace in file name',
+        file => "$DEFAULT_STORAGE_DIR/import/My Import.ova",
+        expected => [''],
+    },
+    {
+        # Supporting subdirectories for the 'import' volume type is
+        # currently not possible because it would introduce parsing ambiguities.
+        description => 'Import, ova, subdirectory',
+        file => "$DEFAULT_STORAGE_DIR/import/foo/import.ova",
+        expected => [''],
+    },
 ];

 my sub run_tests($tests) {
--
2.47.3