Re: [PATCH pve-storage v1 17/54] api: status: move content type assert for up-/downloads into helper

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

NAK - One issue here which looks minor but affects security:

On Wed, Apr 22, 2026 at 01:12:43PM +0200, Max R. Carrara wrote:
> Signed-off-by: Max R. Carrara <m.carrara@proxmox.com>
> ---
>  src/PVE/API2/Storage/Status.pm | 41 +++++++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/src/PVE/API2/Storage/Status.pm b/src/PVE/API2/Storage/Status.pm
> index 8225c3ad..b9e949ff 100644
> --- a/src/PVE/API2/Storage/Status.pm
> +++ b/src/PVE/API2/Storage/Status.pm
> @@ -41,6 +41,29 @@ __PACKAGE__->register_method({
>      path => '{storage}/file-restore',
>  });
> 
> +# Content types that may currently be up- or downloaded via the API
> +my @ALLOWED_FILE_TRANSFER_CONTENT_TYPES = ('iso', 'vztmpl', 'import');
> +
> +my sub assert_can_transfer_files : prototype($$$) {
> +    my ($scfg, $storeid, $vtype) = @_;
> +
> +    if (!defined($scfg->{path})) {
> +        die "unable to transfer files from / to storage type '$scfg->{type}'"
> +            . " - not a file-based storage\n";
> +    }
> +
> +    if (!$scfg->{content}->{$vtype}) {
> +        die "storage '$storeid' is not configured for content type '$vtype'\n";
> +    }
> +
> +    my $is_allowed_vtype = grep { $vtype } @ALLOWED_FILE_TRANSFER_CONTENT_TYPES;

^ You meant to say `grep { $_ eq $vtype }` - grep takes an expression,
not a needle.
The above line will always contain the whole list making the check below
a no-op.

> +    if (!$is_allowed_vtype) {
> +        raise_param_exc({ content => "content type '$vtype' not allowed for upload / download" });
> +    }
> +
> +    return;
> +}
> +
>  my sub assert_ova_contents {
>      my ($file) = @_;
> 
> @@ -573,11 +596,10 @@ __PACKAGE__->register_method({
>          my ($node, $storage) = $param->@{qw(node storage)};
>          my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
> 
> -        die "can't upload to storage type '$scfg->{type}'\n"
> -            if !defined($scfg->{path});
> -
>          my $content = $param->{content};
> 
> +        assert_can_transfer_files($scfg, $storage, $content);
> +
>          my $tmpfilename = $param->{tmpfilename};
>          die "missing temporary file name\n" if !$tmpfilename;
> 
> @@ -615,13 +637,8 @@ __PACKAGE__->register_method({
>              }
> 
>              $path = PVE::Storage::get_import_dir($cfg, $storage);
> -        } else {
> -            raise_param_exc({ content => "upload content type '$content' not allowed" });
>          }
> 
> -        die "storage '$storage' does not support '$content' content\n"
> -            if !$scfg->{content}->{$content};
> -
>          my $dest = "$path/$filename";
>          my $dirname = dirname($dest);
> 
> @@ -817,13 +834,9 @@ __PACKAGE__->register_method({
>          my ($node, $storage, $compression) = $param->@{qw(node storage compression)};
>          my $scfg = PVE::Storage::storage_check_enabled($cfg, $storage, $node);
> 
> -        die "can't upload to storage type '$scfg->{type}', not a file based storage!\n"
> -            if !defined($scfg->{path});
> -
>          my ($content, $url) = $param->@{ 'content', 'url' };
> 
> -        die "storage '$storage' is not configured for content-type '$content'\n"
> -            if !$scfg->{content}->{$content};
> +        assert_can_transfer_files($scfg, $storage, $content);
> 
>          my $filename = PVE::Storage::normalize_content_filename($param->{filename});
> 
> @@ -856,8 +869,6 @@ __PACKAGE__->register_method({
>              }
> 
>              $path = PVE::Storage::get_import_dir($cfg, $storage);
> -        } else {
> -            raise_param_exc({ content => "upload content-type '$content' is not allowed" });
>          }
> 
>          PVE::Storage::activate_storage($cfg, $storage);
> --
> 2.47.3
> 
> 
> 
> 
> 

--