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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 06/06/2023 um 10:39 schrieb Dominik Csapak:
> a few 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
> * the biggest point: in the RESTHandler, to be compatible with the
>   current gui behavior, we have to rewrite two parameter types:
>   - when the api defines a '-list' format for a string type, but we get
>     a list (because of the changes in http-server), we join the list
>     with a comma into a string
>   - when the api defines an 'array' type, but we get a scalar value,
>     wrap the value in an array (because for www-form-urlencoded, you
>     cannot send an array with a single value) add tests for this
>     behavior, some of which we want to deprecate and remove in the
>     future
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> changes from v1:
> * include wolfangs feedback
> * include auto-conversion from string <-> list where appropriate and add
>   tests for it
> 
>  src/PVE/JSONSchema.pm      |  12 +++++
>  src/PVE/RESTHandler.pm     |  61 ++++++++++++++++++----
>  test/Makefile              |   9 +++-
>  test/api_parameter_test.pl | 100 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 172 insertions(+), 10 deletions(-)
>  create mode 100755 test/api_parameter_test.pl
> 
> 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') {

code style, and can be fixed up:
for multi-line if's place the closing parenthesis and opening block { on it's own line:

It also doesn't hurt to move all expressions part of the condition in a separate line
(albeit that part is not a rule in our style guide):

if (
    $schema->{properties}->{$key}
    && $schema->{properties}->{$key}->{type} eq 'array'
) {
    # ...

> +
> +		if (defined($cfg->{$key})) {
> +		    push $cfg->{$key}->@*, $value;
> +		} else {
> +		    $cfg->{$key} = [$value];
> +		}

Could be written shorter, but just fine as above

$cfg->{$key} //= [];
push $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 db86af2..369e302 100644
> --- a/src/PVE/RESTHandler.pm
> +++ b/src/PVE/RESTHandler.pm
> @@ -426,6 +426,56 @@ sub find_handler {
>      return ($handler_class, $method_info);
>  }
>  
> +my $untaint_recursive;

I got flash backs w.r.t. refcount cycles here keeping all variables, and thus memory
inside the body alive forever, don't we need a weaken?

E.g., like we had to do in PVE::Status::Graphite's assemble.
> +
> +$untaint_recursive = sub {
> +    my ($param) = @_;
> +
> +    my $ref = ref($param);
> +    if ($ref eq 'HASH') {
> +	$param->{$_} = $untaint_recursive->($param->{$_}) for keys $param->%*;
> +    } elsif ($ref eq 'ARRAY') {
> +	for (my $i = 0; $i < scalar($param->@*); $i++) {
> +	    $param->[$i] = $untaint_recursive->($param->[$i]);
> +	}
> +    } else {
> +	if (defined($param)) {

could be merged into upper branch as elsif, but no hard feelings.

> +	    my ($newval) = $param =~ /^(.*)$/s;
> +	    $param = $newval;
> +	}
> +    }
> +
> +    return $param;
> +};
> +
> +# convert arrays to strings where we expect a '-list' format and convert scalar
> +# values to arrays when we expect an array (because of www-form-urlencoded)
> +#
> +# only on the top level, since www-form-urlencoded cannot be nested anyway
> +#
> +# FIXME: change gui/api calls to not rely on this during 8.x, mark the
> +# behaviour deprecated with 9.x, and remove it with 10.x
> +my $convert_params = sub { my ($param, $schema) = @_;

please keep the method paramethers on it's own line.

Also, maybe go for a more telling names, as convert_params could mean everytrhing
and nothing ^^ 



> +
> +    return if !$schema->{properties};
> +    return if (ref($param) // '') ne 'HASH';

doesn't this breaks the assignment when used below? I.e.,:

$param = $convert_params->($param, $schema);

or messes with silenting parameters sent to a endpoint without properties, which would
create an extra param error otherwise?

> +
> +    for my $key (keys $schema->{properties}->%*) {
> +	if (my $value = $param->{$key}) {
> +	    my $type = $schema->{properties}->{$key}->{type} // '';
> +	    my $format = $schema->{properties}->{$key}->{format} // '';
> +	    my $ref = ref($value);
> +	    if ($ref eq 'ARRAY' && $type eq 'string' && $format =~ m/-list$/) {

Should this also check ref to not be undef, i.e.

if ($ref && $ref eq 'ARRAY' && ...


> +		$param->{$key} = join(',', $value->@*);
> +	    } elsif (!$ref && $type eq 'array') {
> +		$param->{$key} = [$value];
> +	    }
> +	}
> +    }
> +
> +    return $param;
> +};
> +
>  sub handle {
>      my ($self, $info, $param, $result_verification) = @_;
>  
> @@ -437,17 +487,10 @@ sub handle {
>  
>      if (my $schema = $info->{parameters}) {
>  	# warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
> +	$param = $convert_params->($param, $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->{'extra-args'} = [map { /^(.*)$/ } @$extra] if $extra;
> +	$param = $untaint_recursive->($param);
>      }
>  
>      my $result = $func->($param); # the actual API code execution call