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.csa...@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 _______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel