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

[Dominik Csapak <d.csapak@proxmox.com>]

On 6/6/23 11:12, Thomas Lamprecht wrote:
> 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'
> ) {
>      # ...
> 

sure, sorry

>> +
>> +		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;

yours is shorter and still understandable
> 
>> +		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.

mhmm isn't that because there we use variables from outside the
function? here we only use the parameters themselves

anyway the solution there is to set the sub to undef after use, but
we can do that here only if we move the sub into the regular
function

i can also make it a proper sub if that's better?

how can i test for these things properly?

>> +
>> +$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.

oops, one shift+j to many without noticing^^

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

sure, any suggestions? ;)

> 
> 
>> +
>> +    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?

yes, we have to return the original param here in both cases

> 
>> +
>> +    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' && ...
> 

yes

> 
>> +		$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
>