From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id BBA611FF0AB for ; Wed, 23 Sep 2026 17:16:59 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D33DA21625; Wed, 23 Sep 2026 17:16:39 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v2 47/50] test: add more tests for 'import' vtype & guard against nested subdirs Date: Wed, 23 Sep 2026 17:16:07 +0200 Message-ID: <20260923151611.544333-4-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260923151611.544333-1-m.carrara@proxmox.com> References: <20260923150606.531239-1-m.carrara@proxmox.com> <20260923151611.544333-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790176578027 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.370 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: YDYHVKGNDA32NVWMHAKQCOUUEMOOSONO X-Message-ID-Hash: YDYHVKGNDA32NVWMHAKQCOUUEMOOSONO X-MailFrom: m.carrara@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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 e665f081..e64ac719 100644 --- a/src/PVE/Storage/Common/Parse.pm +++ b/src/PVE/Storage/Common/Parse.pm @@ -134,6 +134,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! (? (? ($RE_SAFE_CHAR_CLASS)+ \. (? $RE_IMPORT_EXTENSIONS) ) diff --git a/src/PVE/Storage/Common/test/parser_tests.pl b/src/PVE/Storage/Common/test/parser_tests.pl index a5402390..01daf1b7 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 d0b054c9..c185fdee 100644 --- a/src/test/list_volumes_test.pm +++ b/src/test/list_volumes_test.pm @@ -1290,6 +1290,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, + }, ], }, { @@ -1385,6 +1405,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, + }, ], }, { @@ -1512,6 +1552,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