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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 03F759D7EA for ; Mon, 5 Jun 2023 10:36:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DE50B24DB4 for ; Mon, 5 Jun 2023 10:36:22 +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 ; Mon, 5 Jun 2023 10:36:17 +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 62FE048917 for ; Mon, 5 Jun 2023 10:36:17 +0200 (CEST) Date: Mon, 5 Jun 2023 10:36:12 +0200 From: Wolfgang Bumiller To: Dominik Csapak Cc: pve-devel@lists.proxmox.com Message-ID: References: <20230512122355.3244212-1-d.csapak@proxmox.com> <20230512122355.3244212-2-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230512122355.3244212-2-d.csapak@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.131 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 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: Mon, 05 Jun 2023 08:36:23 -0000 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 > --- > 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