JSONSchema: add support for array parameter in api calls, cli and config

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.csapak@proxmox.com>

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 93dfcf9580cfb6d10549dc8fa8891bdf6ce1da3f..430235a9079b5100fc9784a9cc68510c435166d0 100644 (file)
--- 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,15 @@ sub parse_config : prototype($$$;$) {
 
                $value = parse_boolean($value) // $value;
            }
+           if (
+               $schema->{properties}->{$key}
+               && $schema->{properties}->{$key}->{type} eq 'array'
+           ) {
+
+               $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 db86af2a68d887a08aaaa3fab296265c5b42d9a2..6fd70c84df5caaa9904f0a52603fdf50be7118e0 100644 (file)
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -426,6 +426,57 @@ sub find_handler {
     return ($handler_class, $method_info);
 }
 
+my sub untaint_recursive : prototype($) {
+    use feature 'current_sub';
+
+    my ($param) = @_;
+
+    my $ref = ref($param);
+    if ($ref eq 'HASH') {
+       $param->{$_} = __SUB__->($param->{$_}) for keys $param->%*;
+    } elsif ($ref eq 'ARRAY') {
+       for (my $i = 0; $i < scalar($param->@*); $i++) {
+           $param->[$i] = __SUB__->($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 $normalize_legacy_param_formats = sub {
+    my ($param, $schema) = @_;
+
+    return $param if !$schema->{properties};
+    return $param 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 && $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 +488,10 @@ sub handle {
 
     if (my $schema = $info->{parameters}) {
        # warn "validate ". Dumper($param}) . "\n" . Dumper($schema);
+       $param = $normalize_legacy_param_formats->($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 5c64867ba084d7f584047616f11c234246b04849..82f40abc36343733ca3328cedbf21d292d19acd6 100644 (file)
--- 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 (executable)
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();