[PATCH common 0/3] fix #7612

[PATCH common 0/3] fix #7612

[Dominik Csapak]

When editing various properties that are only marked as integer or
numbers without any maximum/minimum, an overly large value can land
in e.g. the guest config. When this value get's interpreted as a number
somewhere along the way to the API, perls JSON encoder prints those
as the bareword inf, which is not a valid JSON value and the UI
can't parse that (JSON.decode throws an exception).

Since such large values do not make sense to have anyway, fix
the 'is_number/integer' check in the JSONSchema to reject such values.

The first patch is just a cleanup, the second one adds test for the
status-quo and the third one changes the parser to reject overly large
integers. For this a new dependency is introduced (Scalar::Util::Numeric)

Dominik Csapak (3):
  json schema: reorder imports to our style guide
  json schema: add tests for numeric behavior
  fix #7612: json schema: don't allow integers that are too large

 debian/control           |   1 +
 src/PVE/JSONSchema.pm    |  28 ++++--
 test/json-schema-test.pl | 198 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 219 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH common 1/3] json schema: reorder imports to our style guide

[Dominik Csapak]

external and PVE modules separately, sorted alphabetically.
While at it, drop the unused Data::Dumper import.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/JSONSchema.pm | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 34f2949..db2c167 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -4,17 +4,17 @@ use strict;
 use warnings;
 use feature 'signatures';
 
-use Storable; # for dclone
-use Getopt::Long;
+use Devel::Cycle -quiet; # todo: remove?
 use Encode::Locale;
 use Encode;
-use Devel::Cycle -quiet; # todo: remove?
-use PVE::ParseUtils qw(split_list $IPV6RE $IPV4RE);
-use PVE::Exception qw(raise);
+use Getopt::Long;
 use HTTP::Status qw(:constants);
 use JSON;
 use Net::IP qw(:PROC);
-use Data::Dumper;
+use Storable; # for dclone
+
+use PVE::Exception qw(raise);
+use PVE::ParseUtils qw(split_list $IPV6RE $IPV4RE);
 
 use base 'Exporter';
 
-- 
2.47.3

[PATCH common 2/3] json schema: add tests for numeric behavior

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 test/json-schema-test.pl | 166 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 166 insertions(+)

diff --git a/test/json-schema-test.pl b/test/json-schema-test.pl
index f1c968f..c5014e9 100755
--- a/test/json-schema-test.pl
+++ b/test/json-schema-test.pl
@@ -22,6 +22,10 @@ use Test::More;
 #   in - input of the sub-test
 #   out - expected output of the sub-test, falls back to outer `out`
 #   must_fail - if set the test must fail and the error must match the regex defined here.
+
+# exceeds the range of a double, so it numifies to infinity
+my $huge_int = '1' . ('0' x 400);
+
 my $property_string_tests = [
     {
         name => 'default-key-with-type-boolean',
@@ -99,6 +103,38 @@ my $property_string_tests = [
             },
         ],
     },
+    {
+        name => 'huge-numeric-values',
+        format => {
+            int => { type => 'integer', optional => 1 },
+            num => { type => 'number', optional => 1 },
+            'int-ranged' => { type => 'integer', optional => 1, minimum => -10, maximum => 10 },
+            'num-ranged' => { type => 'number', optional => 1, minimum => -1, maximum => 1 },
+        },
+        subtests => [
+            { in => "int=$huge_int", out => { int => $huge_int } },
+            { in => "int=-$huge_int", out => { int => "-$huge_int" } },
+            { in => "num=$huge_int", out => { num => $huge_int } },
+            { in => "num=1e400", out => { num => '1e400' } },
+            { in => "num=-1e400", out => { num => '-1e400' } },
+            {
+                in => "int-ranged=$huge_int",
+                must_fail => qr/int-ranged: value must have a maximum value of 10/,
+            },
+            {
+                in => "int-ranged=-$huge_int",
+                must_fail => qr/int-ranged: value must have a minimum value of -10/,
+            },
+            {
+                in => "num-ranged=1e400",
+                must_fail => qr/num-ranged: value must have a maximum value of 1/,
+            },
+            {
+                in => "num-ranged=-1e400",
+                must_fail => qr/num-ranged: value must have a minimum value of -1/,
+            },
+        ],
+    },
     # TODO: more tests, like complex formats and ranges and the like
 ];
 
@@ -863,4 +899,134 @@ for my $test ($check_one_of->@*) {
     };
 }
 
+# check numeric type and range checks with values exceeding the range of a double
+#
+# Properties of a test:
+#
+# name - describes the test
+# schema - the schema of the property 'value'
+# subtests - list of:
+#   name - describes the sub-test
+#   in - the value to check
+#   must_fail - regex that must match the error of the property
+my $numeric_tests = [
+    {
+        name => 'huge integer',
+        schema => { type => 'integer' },
+        subtests => [
+            { name => 'positive', in => "100" },
+            { name => 'negative', in => "-100" },
+            { name => 'explicit plus sign', in => "+100" },
+            {
+                name => 'exponent notation',
+                in => '1e2',
+                must_fail => qr/^type check \('integer'\) failed - got '1e2'/,
+            },
+            {
+                name => 'not a number',
+                in => 'foo',
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
+        ],
+    },
+    {
+        name => 'huge integer',
+        schema => { type => 'integer' },
+        subtests => [
+            { name => 'positive', in => $huge_int },
+            { name => 'negative', in => "-$huge_int" },
+            { name => 'explicit plus sign', in => "+$huge_int" },
+            {
+                name => 'exponent notation',
+                in => '1e400',
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
+            {
+                name => 'fractional part',
+                in => "$huge_int.0",
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
+        ],
+    },
+    {
+        name => 'huge integer with range',
+        schema => { type => 'integer', minimum => -10, maximum => 10 },
+        subtests => [
+            {
+                name => 'above maximum',
+                in => $huge_int,
+                must_fail => qr/^value must have a maximum value of 10$/,
+            },
+            {
+                name => 'below minimum',
+                in => "-$huge_int",
+                must_fail => qr/^value must have a minimum value of -10$/,
+            },
+        ],
+    },
+    {
+        name => 'huge number',
+        schema => { type => 'number' },
+        subtests => [
+            { name => 'integer notation', in => $huge_int },
+            { name => 'negative integer notation', in => "-$huge_int" },
+            { name => 'fractional part', in => "$huge_int.5" },
+            { name => 'exponent notation', in => '1e400' },
+            { name => 'negative exponent notation', in => '-1e400' },
+            { name => 'tiny exponent notation', in => '1e-400' },
+            {
+                name => 'infinity literal',
+                in => 'inf',
+                must_fail => qr/^type check \('number'\) failed/,
+            },
+        ],
+    },
+    {
+        name => 'huge number with range',
+        schema => { type => 'number', minimum => 0, maximum => 1 },
+        subtests => [
+            {
+                name => 'above maximum',
+                in => '1e400',
+                must_fail => qr/^value must have a maximum value of 1$/,
+            },
+            {
+                name => 'above maximum in integer notation',
+                in => $huge_int,
+                must_fail => qr/^value must have a maximum value of 1$/,
+            },
+            {
+                name => 'below minimum',
+                in => '-1e400',
+                must_fail => qr/^value must have a minimum value of 0$/,
+            },
+            { name => 'tiny value within range', in => '1e-400' },
+        ],
+    },
+];
+
+for my $test ($numeric_tests->@*) {
+    subtest $test->{name}, sub {
+        for my $subtest ($test->{subtests}->@*) {
+            my $name = $subtest->{name};
+            my $schema = {
+                type => 'object',
+                properties => { value => $test->{schema} },
+            };
+            my $value = { value => $subtest->{in} };
+
+            my $errors = {};
+            PVE::JSONSchema::check_prop($value, $schema, undef, $errors);
+
+            if (my $must_fail = $subtest->{must_fail}) {
+                like(delete($errors->{value}) // '', $must_fail, "$name - failed as expected");
+            }
+            my $err_str = join("\n", map { "$_: $errors->{$_}" } sort keys %$errors);
+            is($err_str, '', "$name - no unexpected errors");
+            is($value->{value}, $subtest->{in}, "$name - value remains unchanged");
+        }
+        done_testing();
+    };
+}
+
 done_testing();
-- 
2.47.3

[PATCH common 3/3] fix #7612: json schema: don't allow integers that are too large

[Dominik Csapak]

very large integers (such as 1e400) are represented as 'inf' which
are serialized out as such by the JSON module. Since that is invalid
JSON, we want to catch these invalid values already during parsing,
because +/- infinity rarely make sense when we expect a number.

Improve the check by also converting the string to a number and back,
then check this for 'inf/nan' variants via Scalar::Util::Numeric.

This now let's some numeric test fail for huge numbers, while still
allowing smaller ones and correctly identifying non-numeric
strings.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 debian/control           |  1 +
 src/PVE/JSONSchema.pm    | 16 +++++++--
 test/json-schema-test.pl | 76 ++++++++++++++++++++++++++++------------
 3 files changed, 69 insertions(+), 24 deletions(-)

diff --git a/debian/control b/debian/control
index ef9b0eb..3c78e55 100644
--- a/debian/control
+++ b/debian/control
@@ -48,6 +48,7 @@ Depends: libanyevent-perl,
          libnetaddr-ip-perl,
          libproxmox-acme-perl,
          libproxmox-rs-perl,
+         libscalar-util-numeric-perl,
          libstring-shellquote-perl,
          libtimedate-perl,
          liburi-perl,
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index db2c167..f06b282 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -11,6 +11,7 @@ use Getopt::Long;
 use HTTP::Status qw(:constants);
 use JSON;
 use Net::IP qw(:PROC);
+use Scalar::Util::Numeric qw(isinf isnan);
 use Storable; # for dclone
 
 use PVE::Exception qw(raise);
@@ -1162,17 +1163,28 @@ sub add_error {
     }
 }
 
+# test values if they're in numeric finite range with the given REGEX
+my sub numeric_test {
+    my ($value, $NUM_RE) = @_;
+
+    my $res = $value =~ $NUM_RE;
+    return $res if !$res;
+    my $num = $value;
+    $num += 0;
+    return !isinf($num) && !isnan($num);
+}
+
 sub is_number {
     my $value = shift;
 
     # see 'man perlretut'
-    return $value =~ /^[+-]?(\d+\.\d+|\d+\.|\.\d+|\d+)([eE][+-]?\d+)?\z/;
+    return numeric_test($value, qr/^[+-]?(\d+\.\d+|\d+\.|\.\d+|\d+)([eE][+-]?\d+)?\z/);
 }
 
 sub is_integer {
     my $value = shift;
 
-    return $value =~ m/^[+-]?\d+\z/;
+    return numeric_test($value, qr/^[+-]?\d+\z/);
 }
 
 sub check_type {
diff --git a/test/json-schema-test.pl b/test/json-schema-test.pl
index c5014e9..cda7c4d 100755
--- a/test/json-schema-test.pl
+++ b/test/json-schema-test.pl
@@ -112,26 +112,26 @@ my $property_string_tests = [
             'num-ranged' => { type => 'number', optional => 1, minimum => -1, maximum => 1 },
         },
         subtests => [
-            { in => "int=$huge_int", out => { int => $huge_int } },
-            { in => "int=-$huge_int", out => { int => "-$huge_int" } },
-            { in => "num=$huge_int", out => { num => $huge_int } },
-            { in => "num=1e400", out => { num => '1e400' } },
-            { in => "num=-1e400", out => { num => '-1e400' } },
+            { in => "int=$huge_int", must_fail => qr/type check \('integer'\) failed/ },
+            { in => "int=-$huge_int", must_fail => qr/type check \('integer'\) failed/ },
+            { in => "num=$huge_int", must_fail => qr/type check \('number'\) failed/ },
+            { in => "num=1e400", must_fail => qr/type check \('number'\) failed/ },
+            { in => "num=-1e400", must_fail => qr/type check \('number'\) failed/ },
             {
                 in => "int-ranged=$huge_int",
-                must_fail => qr/int-ranged: value must have a maximum value of 10/,
+                must_fail => qr/type check \('integer'\) failed/,
             },
             {
                 in => "int-ranged=-$huge_int",
-                must_fail => qr/int-ranged: value must have a minimum value of -10/,
+                must_fail => qr/type check \('integer'\) failed/,
             },
             {
                 in => "num-ranged=1e400",
-                must_fail => qr/num-ranged: value must have a maximum value of 1/,
+                must_fail => qr/type check \('number'\) failed/,
             },
             {
                 in => "num-ranged=-1e400",
-                must_fail => qr/num-ranged: value must have a minimum value of -1/,
+                must_fail => qr/type check \('number'\) failed/,
             },
         ],
     },
@@ -933,9 +933,21 @@ my $numeric_tests = [
         name => 'huge integer',
         schema => { type => 'integer' },
         subtests => [
-            { name => 'positive', in => $huge_int },
-            { name => 'negative', in => "-$huge_int" },
-            { name => 'explicit plus sign', in => "+$huge_int" },
+            {
+                name => 'positive',
+                in => $huge_int,
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
+            {
+                name => 'negative',
+                in => "-$huge_int",
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
+            {
+                name => 'explicit plus sign',
+                in => "+$huge_int",
+                must_fail => qr/^type check \('integer'\) failed/,
+            },
             {
                 name => 'exponent notation',
                 in => '1e400',
@@ -955,12 +967,12 @@ my $numeric_tests = [
             {
                 name => 'above maximum',
                 in => $huge_int,
-                must_fail => qr/^value must have a maximum value of 10$/,
+                must_fail => qr/^type check \('integer'\) failed/,
             },
             {
                 name => 'below minimum',
                 in => "-$huge_int",
-                must_fail => qr/^value must have a minimum value of -10$/,
+                must_fail => qr/^type check \('integer'\) failed/,
             },
         ],
     },
@@ -968,11 +980,31 @@ my $numeric_tests = [
         name => 'huge number',
         schema => { type => 'number' },
         subtests => [
-            { name => 'integer notation', in => $huge_int },
-            { name => 'negative integer notation', in => "-$huge_int" },
-            { name => 'fractional part', in => "$huge_int.5" },
-            { name => 'exponent notation', in => '1e400' },
-            { name => 'negative exponent notation', in => '-1e400' },
+            {
+                name => 'integer notation',
+                in => $huge_int,
+                must_fail => qr/^type check \('number'\) failed/,
+            },
+            {
+                name => 'negative integer notation',
+                in => "-$huge_int",
+                must_fail => qr/^type check \('number'\) failed/,
+            },
+            {
+                name => 'fractional part',
+                in => "$huge_int.5",
+                must_fail => qr/^type check \('number'\) failed/,
+            },
+            {
+                name => 'exponent notation',
+                in => '1e400',
+                must_fail => qr/^type check \('number'\) failed/,
+            },
+            {
+                name => 'negative exponent notation',
+                in => '-1e400',
+                must_fail => qr/^type check \('number'\) failed/,
+            },
             { name => 'tiny exponent notation', in => '1e-400' },
             {
                 name => 'infinity literal',
@@ -988,17 +1020,17 @@ my $numeric_tests = [
             {
                 name => 'above maximum',
                 in => '1e400',
-                must_fail => qr/^value must have a maximum value of 1$/,
+                must_fail => qr/^type check \('number'\) failed/,
             },
             {
                 name => 'above maximum in integer notation',
                 in => $huge_int,
-                must_fail => qr/^value must have a maximum value of 1$/,
+                must_fail => qr/^type check \('number'\) failed/,
             },
             {
                 name => 'below minimum',
                 in => '-1e400',
-                must_fail => qr/^value must have a minimum value of 0$/,
+                must_fail => qr/^type check \('number'\) failed/,
             },
             { name => 'tiny value within range', in => '1e-400' },
         ],
-- 
2.47.3