Re: [pve-devel] [PATCH common 1/3] JSONSchema: add support for array parameter in api calls, cli and config

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

On Fri, May 12, 2023 at 02:23:48PM +0200, Dominik Csapak wrote:
> only three small things were missing for it to work:
> * on the cli, we have to get the option as an array if the type is an array
> * the untainting must be done recursively, otherwise, the regex matching
>   converts an array hash into the string 'ARRAY(0x123412341234)'
> * JSONSchema::parse_config did not handle array formats specially, but
>   we want to allow to specify them multiple time
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/JSONSchema.pm  | 12 ++++++++++++
>  src/PVE/RESTHandler.pm | 41 ++++++++++++++++++++++++++++++++++-------
>  2 files changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 527e409..526fc2b 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -1709,6 +1709,8 @@ sub get_options {
>  	} else {
>  	    if ($pd->{format} && $pd->{format} =~ m/-a?list/) {
>  		push @getopt, "$prop=s@";
> +	    } elsif ($pd->{type} eq 'array') {
> +		push @getopt, "$prop=s@";
>  	    } else {
>  		push @getopt, "$prop=s";
>  	    }
> @@ -1869,6 +1871,16 @@ sub parse_config : prototype($$$;$) {
>  
>  		$value = parse_boolean($value) // $value;
>  	    }
> +	    if ($schema->{properties}->{$key} &&
> +		$schema->{properties}->{$key}->{type} eq 'array') {
> +
> +		if (defined($cfg->{$key})) {
> +		    push $cfg->{$key}->@*, $value;
> +		} else {
> +		    $cfg->{$key} = [$value];
> +		}
> +		next;
> +	    }
>  	    $cfg->{$key} = $value;
>  	} else {
>  	    warn "ignore config line: $line\n"
> diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
> index 944a04b..20714a5 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,38 @@ sub find_handler {
>      return ($handler_class, $method_info);
>  }
>  
> +my $untaint_recursive;
> +
> +$untaint_recursive = sub {
> +    my ($param) = @_;
> +
> +    my $ref = ref $param;
> +    if ($ref eq 'HASH') {
> +	while (my ($key, $val) = each %$param) {
> +	    my $newval = $untaint_recursive->($val);
> +	    if (defined($newval)) {

if defined, assign untainted value

> +		($param->{$key}) = $newval;
> +	    } else {
> +		$param->{$key} = undef;

if ==undef, assign undef

> +	    }
> +	}

So this entire case could just be:

    $param->{$_} = $untaint_recursive->($param->{$_) for keys %$param;

can it not?

However - here, we're explicitly assigning `undef`s, but in the array
case below...

> +    } elsif ($ref eq 'ARRAY') {
> +	my $newparam = [];
> +	for my $val (@$param) {
> +	    my $newval = $untaint_recursive->($val);
> +	    push @$newparam, $newval if defined($newval);

...here we're skipping undef.
I'd like comments explaining the cases in within this sub.

Further, the 'hash' case replaces the existing hash, while the array
case produces a _new_ array, which could make an actual difference for
the toplevel caller of this method.

The caller below expects a return value for a hash, discarding the
original, so this might be fine, but I'd prefer to either stay
consistent, or a have a big fat side effect warning comment on top of
the sub.

> +	}
> +	$param = $newparam;
> +    } else {
> +	if (defined($param)) {
> +	    my ($newval) = $param =~ /^(.*)$/s;
> +	    $param = $newval;
> +	}
> +    }
> +
> +    return $param;
> +};
> +
>  sub handle {
>      my ($self, $info, $param, $result_verification) = @_;
>  
> @@ -437,16 +469,11 @@ sub handle {
>  
>      if (my $schema = $info->{parameters}) {
>  	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> +
>  	PVE::JSONSchema::validate($param, $schema);
>  	# untaint data (already validated)
>  	my $extra = delete $param->{'extra-args'};
> -	while (my ($key, $val) = each %$param) {
> -	    if (defined($val)) {
> -		($param->{$key}) = $val =~ /^(.*)$/s;
> -	    } else {
> -		$param->{$key} = undef;
> -	    }
> -	}
> +	$param = $untaint_recursive->($param);

And after this, I think the line below can be dropped?

>  	$param->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
>      }
>  
> -- 
> 2.30.2