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 F376C9DCBA for ; Tue, 6 Jun 2023 10:39:47 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D347B31712 for ; Tue, 6 Jun 2023 10:39:17 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 6 Jun 2023 10:39:16 +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 1118848C62 for ; Tue, 6 Jun 2023 10:39:16 +0200 (CEST) From: Dominik Csapak To: pve-devel@lists.proxmox.com Date: Tue, 6 Jun 2023 10:39:08 +0200 Message-Id: <20230606083914.1400960-2-d.csapak@proxmox.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230606083914.1400960-1-d.csapak@proxmox.com> References: <20230606083914.1400960-1-d.csapak@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit 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: [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 08:39:48 -0000 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') { + + 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 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; + +$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)) { + 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) = @_; + + return if !$schema->{properties}; + return if (ref($param) // '') ne 'HASH'; + + 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$/) { + $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 diff --git a/test/Makefile b/test/Makefile index 5c64867..82f40ab 100644 --- a/test/Makefile +++ b/test/Makefile @@ -1,4 +1,11 @@ SUBDIRS = etc_network_interfaces +TESTS = lock_file.test \ + calendar_event_test.test \ + convert_size_test.test \ + procfs_tests.test \ + format_test.test \ + section_config_test.test \ + api_parameter_test.test \ all: @@ -6,7 +13,7 @@ all: export PERLLIB=../src -check: lock_file.test calendar_event_test.test convert_size_test.test procfs_tests.test format_test.test section_config_test.test +check: $(TESTS) for d in $(SUBDIRS); do $(MAKE) -C $$d check; done %.test: %.pl diff --git a/test/api_parameter_test.pl b/test/api_parameter_test.pl new file mode 100755 index 0000000..7ade386 --- /dev/null +++ b/test/api_parameter_test.pl @@ -0,0 +1,100 @@ +#!/usr/bin/perl +package PVE::TestAPIParameters; + +# Tests the automatic conversion of -list and array parameter types + +use strict; +use warnings; + +use lib '../src'; + +use PVE::RESTHandler; +use PVE::JSONSchema; + +use Test::More; + +use base qw(PVE::RESTHandler); + +my $setup = [ + { + name => 'list-format-with-list', + parameter => { + type => 'string', + format => 'pve-configid-list', + }, + value => "foo,bar", + 'value-expected' => "foo,bar", + }, + { + name => 'array-format-with-array', + parameter => { + type => 'array', + items => { + type => 'string', + format => 'pve-configid', + }, + }, + value => ['foo', 'bar'], + 'value-expected' => ['foo', 'bar'], + }, + # TODO: below behaviour should be deprecated with 9.x and fail with 10.x + { + name => 'list-format-with-alist', + parameter => { + type => 'string', + format => 'pve-configid-list', + }, + value => "foo\0bar", + 'value-expected' => "foo\0bar", + }, + { + name => 'array-format-with-non-array', + parameter => { + type => 'array', + items => { + type => 'string', + format => 'pve-configid', + }, + }, + value => "foo", + 'value-expected' => ['foo'], + }, + { + name => 'list-format-with-array', + parameter => { + type => 'string', + format => 'pve-configid-list', + }, + value => ['foo', 'bar'], + 'value-expected' => "foo,bar", + }, +]; + +for my $data ($setup->@*) { + __PACKAGE__->register_method({ + name => $data->{name}, + path => $data->{name}, + method => 'POST', + parameters => { + additionalProperties => 0, + properties => { + param => $data->{parameter}, + }, + }, + returns => { type => 'null' }, + code => sub { + my ($param) = @_; + return $param->{param}; + } + }); + + my ($handler, $info) = __PACKAGE__->find_handler('POST', $data->{name}); + my $param = { + param => $data->{value}, + }; + + my $res = $handler->handle($info, $param); + is_deeply($res, $data->{'value-expected'}, $data->{name}); +} + +done_testing(); -- 2.30.2