From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id BA6A59DD68 for ; Tue, 6 Jun 2023 11:41:37 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 93E8C32821 for ; Tue, 6 Jun 2023 11:41:07 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 6 Jun 2023 11:41:06 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 7234348C79; Tue, 6 Jun 2023 11:41:06 +0200 (CEST) Message-ID: Date: Tue, 6 Jun 2023 11:41:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Thomas Lamprecht , Proxmox VE development discussion References: <20230606083914.1400960-1-d.csapak@proxmox.com> <20230606083914.1400960-2-d.csapak@proxmox.com> <7f0da808-115b-6f31-2cf2-3bd3f0e7e27b@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <7f0da808-115b-6f31-2cf2-3bd3f0e7e27b@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.015 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH common v2 1/3] JSONSchema: add support for array parameter in api calls, cli and config X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 06 Jun 2023 09:41:37 -0000 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 >> --- >> 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 >