From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 98B2A1FF13B for ; Wed, 22 Apr 2026 13:21:32 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 76B771B334; Wed, 22 Apr 2026 13:16:39 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v1 51/54] test: add more tests for 'import' vtype & guard against nested subdirs Date: Wed, 22 Apr 2026 13:13:17 +0200 Message-ID: <20260422111322.257380-52-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260422111322.257380-1-m.carrara@proxmox.com> References: <20260422111322.257380-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: 1776856370672 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.082 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: 5CVS6DR5AEDCPK76AHONXSTWWE7KFFQE X-Message-ID-Hash: 5CVS6DR5AEDCPK76AHONXSTWWE7KFFQE 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 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! (? (? ($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 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