From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 4BA1A1FF0AB for ; Wed, 23 Sep 2026 17:10:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 44994216F3; Wed, 23 Sep 2026 17:09:04 +0200 (CEST) From: "Max R. Carrara" To: pve-devel@lists.proxmox.com Subject: [PATCH pve-storage v2 27/50] tree-wide: partially replace usages of regexes for 'import' vtype Date: Wed, 23 Sep 2026 17:05:41 +0200 Message-ID: <20260923150606.531239-28-m.carrara@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260923150606.531239-1-m.carrara@proxmox.com> References: <20260923150606.531239-1-m.carrara@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1790176023328 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.416 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: Z3XQ6TAJSTCND2CKNOAQ2LRRUG6XGYLE X-Message-ID-Hash: Z3XQ6TAJSTCND2CKNOAQ2LRRUG6XGYLE 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: 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 --- 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>> +as well as forward slash C and colon C<:>. Use C<$self> to remember it for the C or C 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! + (? + (? ($RE_SAFE_CHAR_CLASS)+ \. (? $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 + +Parsing code of inbuilt plugins and other modules in the C> +namespace should instead use the constants defined in +C> 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