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