[PATCH proxmox, pve-{common,network,storage} v2 0/5] fix #7077: Improve error messages for ID verification

[PATCH proxmox, pve-{common,network,storage} v2 0/5] fix #7077: Improve error messages for ID verification

[Arthur Bied-Charreton]

A lot of the logic around verifying ID constraints like their length,
or that they follow a specific pattern, hides in registered format functions,
which makes them invisible in the API docs.

This series addresses this, as well as the not-ideal UX reported in #7077, by
moving some of these checks to the registered JSON schemas and adding explicit
length checks where no schema is defined.

As discussed in v1 [0], this renders some of the registered format functions
obsolete. I did remove formats that I was able to verify are not
depended on anywhere, however in order to keep the scope of this patch
series in check, I did not refactor any of the ones that do have dependants
(i.e., the format is referred to anywhere else than in the call to
`register_standard_option`). 

I would be very thankful for a review by someone with more experience with
those registered formats, as I may have missed dependencies my poor grepping
skills could not catch.

Changes since v1:
- Update JSON schema verification to check length constraints before custom
format verification code and/or regex
- Register length/regex constraints for IDs directly in the
`register_standard_option` calls, if applicable

[0] https://lore.proxmox.com/pve-devel/mp3ggxjqimualakem43bge5szrphz2dyat2wynowsujwl7wcbu@6pf4sbwxam6u/T/#u


pve-common:

Arthur Bied-Charreton (1):
  fix #7077: Change JSON Schema attribute validation order

 src/PVE/JSONSchema.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)


pve-network:

Arthur Bied-Charreton (2):
  fix #7077: Enforce ID format in JSON schema definitions
  sdn: Remove unneeded registered formats

 src/PVE/Network/SDN/Controllers/IsisPlugin.pm |  3 +++
 src/PVE/Network/SDN/Controllers/Plugin.pm     | 17 +++--------------
 src/PVE/Network/SDN/Dns/Plugin.pm             | 15 ++-------------
 src/PVE/Network/SDN/Fabrics.pm                |  3 +++
 src/PVE/Network/SDN/Ipams/Plugin.pm           | 15 ++-------------
 src/PVE/Network/SDN/VnetPlugin.pm             | 17 +++--------------
 src/PVE/Network/SDN/Zones/Plugin.pm           | 17 +++--------------
 7 files changed, 19 insertions(+), 68 deletions(-)


pve-storage:

Arthur Bied-Charreton (1):
  fix #7077: lvm: Improve ID verification error messages

 src/PVE/Storage/Plugin.pm | 5 +++++
 1 file changed, 5 insertions(+)


proxmox:

Arthur Bied-Charreton (1):
  fix #7077: Improve SDN ID validation error messages

 pve-api-types/src/types/verifiers.rs | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)


Summary over all repositories:
  10 files changed, 47 insertions(+), 83 deletions(-)

-- 
Generated by murpp 0.9.0

[PATCH pve-common v2 1/1] fix #7077: Change JSON Schema attribute validation order

[Arthur Bied-Charreton]

The JSON Schema validation first checks input against the registered
format function, then against the registered pattern, and only then
against {min,max}-length.

This causes length constraints to be poorly reported when they are
implicit in custom format verification code or regex patterns, which 
results in generic format violation error messages instead of the
more specific length violation.

Change the validation order to check length constraints before anything
else. This is functionally equivalent, and comes with the UX advantage
of providing more precise error messages in a lot of cases.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/JSONSchema.pm | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 0c9bb82..531a52c 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1463,31 +1463,31 @@ sub check_prop {
 
     } else {
 
-        if (my $format = $schema->{format}) {
-            eval { check_format($format, $value, $path); };
-            if ($@) {
-                add_error($errors, $path, "invalid format - $@");
+        if (defined(my $max = $schema->{maxLength})) {
+            if (length($value) > $max) {
+                add_error($errors, $path, "value may only be $max characters long");
                 return;
             }
         }
 
-        if (my $pattern = $schema->{pattern}) {
-            if ($value !~ m/^$pattern$/) {
-                add_error($errors, $path, "value does not match the regex pattern");
+        if (defined(my $min = $schema->{minLength})) {
+            if (length($value) < $min) {
+                add_error($errors, $path, "value must be at least $min characters long");
                 return;
             }
         }
 
-        if (defined(my $max = $schema->{maxLength})) {
-            if (length($value) > $max) {
-                add_error($errors, $path, "value may only be $max characters long");
+        if (my $format = $schema->{format}) {
+            eval { check_format($format, $value, $path); };
+            if ($@) {
+                add_error($errors, $path, "invalid format - $@");
                 return;
             }
         }
 
-        if (defined(my $min = $schema->{minLength})) {
-            if (length($value) < $min) {
-                add_error($errors, $path, "value must be at least $min characters long");
+        if (my $pattern = $schema->{pattern}) {
+            if ($value !~ m/^$pattern$/) {
+                add_error($errors, $path, "value does not match the regex pattern");
                 return;
             }
         }
-- 
2.47.3

[PATCH pve-network v2 1/2] fix #7077: Enforce ID format in JSON schema definitions

[Arthur Bied-Charreton]

Add ID format constraints checked in registered formats' custom
verifiers to JSON schema definitions, so that:

1. The constraints are visible in the API docs.

2. Length constraints are checked *before* regex, which will
generate more precise error messages in case a regex has implicit
length constraints

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---

 src/PVE/Network/SDN/Controllers/IsisPlugin.pm | 3 +++
 src/PVE/Network/SDN/Controllers/Plugin.pm     | 3 +++
 src/PVE/Network/SDN/Dns/Plugin.pm             | 2 ++
 src/PVE/Network/SDN/Fabrics.pm                | 3 +++
 src/PVE/Network/SDN/Ipams/Plugin.pm           | 2 ++
 src/PVE/Network/SDN/VnetPlugin.pm             | 3 +++
 src/PVE/Network/SDN/Zones/Plugin.pm           | 3 +++
 7 files changed, 19 insertions(+)

diff --git a/src/PVE/Network/SDN/Controllers/IsisPlugin.pm b/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
index 3a9acfd..4c11af3 100644
--- a/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
+++ b/src/PVE/Network/SDN/Controllers/IsisPlugin.pm
@@ -43,6 +43,9 @@ sub properties {
             description => "Network Entity title for this node in the IS-IS network.",
             type => 'string',
             format => 'pve-sdn-isis-net',
+            pattern => '[a-fA-F0-9]{2}(\.[a-fA-F0-9]{4}){3,9}\.[a-fA-F0-9]{2}',
+            minLength => 20,
+            maxLength => 50,
         },
     };
 }
diff --git a/src/PVE/Network/SDN/Controllers/Plugin.pm b/src/PVE/Network/SDN/Controllers/Plugin.pm
index d70e518..73eaa0b 100644
--- a/src/PVE/Network/SDN/Controllers/Plugin.pm
+++ b/src/PVE/Network/SDN/Controllers/Plugin.pm
@@ -22,6 +22,9 @@ PVE::JSONSchema::register_standard_option(
         description => "The SDN controller object identifier.",
         type => 'string',
         format => 'pve-sdn-controller-id',
+        minLength => 2,
+        maxLength => 64,
+        pattern => '[a-zA-Z][a-zA-Z0-9_-]*[a-zA-Z0-9]',
     },
 );
 
diff --git a/src/PVE/Network/SDN/Dns/Plugin.pm b/src/PVE/Network/SDN/Dns/Plugin.pm
index 2864d4c..8e623ba 100644
--- a/src/PVE/Network/SDN/Dns/Plugin.pm
+++ b/src/PVE/Network/SDN/Dns/Plugin.pm
@@ -24,6 +24,8 @@ PVE::JSONSchema::register_standard_option(
         description => "The SDN dns object identifier.",
         type => 'string',
         format => 'pve-sdn-dns-id',
+        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
+        minLength => 2,
     },
 );
 
diff --git a/src/PVE/Network/SDN/Fabrics.pm b/src/PVE/Network/SDN/Fabrics.pm
index d90992a..c65eae6 100644
--- a/src/PVE/Network/SDN/Fabrics.pm
+++ b/src/PVE/Network/SDN/Fabrics.pm
@@ -28,6 +28,9 @@ PVE::JSONSchema::register_standard_option(
         description => "Identifier for SDN fabrics",
         type => 'string',
         format => 'pve-sdn-fabric-id',
+        pattern => '[a-zA-Z0-9][a-zA-Z0-9-]{0,6}[a-zA-Z0-9]',
+        minLength => 2,
+        maxLength => 8,
     },
 );
 
diff --git a/src/PVE/Network/SDN/Ipams/Plugin.pm b/src/PVE/Network/SDN/Ipams/Plugin.pm
index a986a92..422fbc8 100644
--- a/src/PVE/Network/SDN/Ipams/Plugin.pm
+++ b/src/PVE/Network/SDN/Ipams/Plugin.pm
@@ -25,6 +25,8 @@ PVE::JSONSchema::register_standard_option(
         description => "The SDN ipam object identifier.",
         type => 'string',
         format => 'pve-sdn-ipam-id',
+        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
+        minLength => 2,
     },
 );
 
diff --git a/src/PVE/Network/SDN/VnetPlugin.pm b/src/PVE/Network/SDN/VnetPlugin.pm
index 717438c..5ceb3a1 100644
--- a/src/PVE/Network/SDN/VnetPlugin.pm
+++ b/src/PVE/Network/SDN/VnetPlugin.pm
@@ -22,6 +22,9 @@ PVE::JSONSchema::register_standard_option(
         description => "The SDN vnet object identifier.",
         type => 'string',
         format => 'pve-sdn-vnet-id',
+        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
+        minLength => 2,
+        maxLength => 8,
     },
 );
 
diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
index 826ebdf..2a02278 100644
--- a/src/PVE/Network/SDN/Zones/Plugin.pm
+++ b/src/PVE/Network/SDN/Zones/Plugin.pm
@@ -24,6 +24,9 @@ PVE::JSONSchema::register_standard_option(
         description => "The SDN zone object identifier.",
         type => 'string',
         format => 'pve-sdn-zone-id',
+        pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
+        minLength => 2,
+        maxLength => 8,
     },
 );
 
-- 
2.47.3

[PATCH pve-network v2 2/2] sdn: Remove unneeded registered formats

[Arthur Bied-Charreton]

Simple ID format verifications like length constraints and patterns have
been moved to the schema definitions. Remove the now redundant custom
verification code, iff no other code depends on the registered format.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Network/SDN/Controllers/Plugin.pm | 14 --------------
 src/PVE/Network/SDN/Dns/Plugin.pm         | 13 -------------
 src/PVE/Network/SDN/Ipams/Plugin.pm       | 13 -------------
 src/PVE/Network/SDN/VnetPlugin.pm         | 14 --------------
 src/PVE/Network/SDN/Zones/Plugin.pm       | 14 --------------
 5 files changed, 68 deletions(-)

diff --git a/src/PVE/Network/SDN/Controllers/Plugin.pm b/src/PVE/Network/SDN/Controllers/Plugin.pm
index 73eaa0b..0a6e4e8 100644
--- a/src/PVE/Network/SDN/Controllers/Plugin.pm
+++ b/src/PVE/Network/SDN/Controllers/Plugin.pm
@@ -21,26 +21,12 @@ PVE::JSONSchema::register_standard_option(
     {
         description => "The SDN controller object identifier.",
         type => 'string',
-        format => 'pve-sdn-controller-id',
         minLength => 2,
         maxLength => 64,
         pattern => '[a-zA-Z][a-zA-Z0-9_-]*[a-zA-Z0-9]',
     },
 );
 
-PVE::JSONSchema::register_format('pve-sdn-controller-id', \&parse_sdn_controller_id);
-
-sub parse_sdn_controller_id {
-    my ($id, $noerr) = @_;
-
-    if ($id !~ m/^[a-z][a-z0-9_-]*[a-z0-9]$/i) {
-        return undef if $noerr;
-        die "controller ID '$id' contains illegal characters\n";
-    }
-    die "controller ID '$id' can't be more length than 64 characters\n" if length($id) > 64;
-    return $id;
-}
-
 my $defaultData = {
 
     propertyList => {
diff --git a/src/PVE/Network/SDN/Dns/Plugin.pm b/src/PVE/Network/SDN/Dns/Plugin.pm
index 8e623ba..4fc15b2 100644
--- a/src/PVE/Network/SDN/Dns/Plugin.pm
+++ b/src/PVE/Network/SDN/Dns/Plugin.pm
@@ -23,24 +23,11 @@ PVE::JSONSchema::register_standard_option(
     {
         description => "The SDN dns object identifier.",
         type => 'string',
-        format => 'pve-sdn-dns-id',
         pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
         minLength => 2,
     },
 );
 
-PVE::JSONSchema::register_format('pve-sdn-dns-id', \&parse_sdn_dns_id);
-
-sub parse_sdn_dns_id {
-    my ($id, $noerr) = @_;
-
-    if ($id !~ m/^[a-z][a-z0-9]*[a-z0-9]$/i) {
-        return undef if $noerr;
-        die "dns ID '$id' contains illegal characters\n";
-    }
-    return $id;
-}
-
 my $defaultData = {
 
     propertyList => {
diff --git a/src/PVE/Network/SDN/Ipams/Plugin.pm b/src/PVE/Network/SDN/Ipams/Plugin.pm
index 422fbc8..26d0931 100644
--- a/src/PVE/Network/SDN/Ipams/Plugin.pm
+++ b/src/PVE/Network/SDN/Ipams/Plugin.pm
@@ -24,24 +24,11 @@ PVE::JSONSchema::register_standard_option(
     {
         description => "The SDN ipam object identifier.",
         type => 'string',
-        format => 'pve-sdn-ipam-id',
         pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
         minLength => 2,
     },
 );
 
-PVE::JSONSchema::register_format('pve-sdn-ipam-id', \&parse_sdn_ipam_id);
-
-sub parse_sdn_ipam_id {
-    my ($id, $noerr) = @_;
-
-    if ($id !~ m/^[a-z][a-z0-9]*[a-z0-9]$/i) {
-        return undef if $noerr;
-        die "ipam ID '$id' contains illegal characters\n";
-    }
-    return $id;
-}
-
 my $defaultData = {
 
     propertyList => {
diff --git a/src/PVE/Network/SDN/VnetPlugin.pm b/src/PVE/Network/SDN/VnetPlugin.pm
index 5ceb3a1..e041575 100644
--- a/src/PVE/Network/SDN/VnetPlugin.pm
+++ b/src/PVE/Network/SDN/VnetPlugin.pm
@@ -21,26 +21,12 @@ PVE::JSONSchema::register_standard_option(
     {
         description => "The SDN vnet object identifier.",
         type => 'string',
-        format => 'pve-sdn-vnet-id',
         pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
         minLength => 2,
         maxLength => 8,
     },
 );
 
-PVE::JSONSchema::register_format('pve-sdn-vnet-id', \&parse_sdn_vnet_id);
-
-sub parse_sdn_vnet_id {
-    my ($id, $noerr) = @_;
-
-    if ($id !~ m/^[a-z][a-z0-9]*[a-z0-9]$/i) {
-        return undef if $noerr;
-        die "vnet ID '$id' contains illegal characters\n";
-    }
-    die "vnet ID '$id' can't be more length than 8 characters\n" if length($id) > 8;
-    return $id;
-}
-
 my $defaultData = {
 
     propertyList => {
diff --git a/src/PVE/Network/SDN/Zones/Plugin.pm b/src/PVE/Network/SDN/Zones/Plugin.pm
index 2a02278..5d858af 100644
--- a/src/PVE/Network/SDN/Zones/Plugin.pm
+++ b/src/PVE/Network/SDN/Zones/Plugin.pm
@@ -23,26 +23,12 @@ PVE::JSONSchema::register_standard_option(
     {
         description => "The SDN zone object identifier.",
         type => 'string',
-        format => 'pve-sdn-zone-id',
         pattern => '[a-zA-Z][a-zA-Z0-9]*[a-zA-Z0-9]',
         minLength => 2,
         maxLength => 8,
     },
 );
 
-PVE::JSONSchema::register_format('pve-sdn-zone-id', \&parse_sdn_zone_id);
-
-sub parse_sdn_zone_id {
-    my ($id, $noerr) = @_;
-
-    if ($id !~ m/^[a-z][a-z0-9]*[a-z0-9]$/i) {
-        return undef if $noerr;
-        die "zone ID '$id' contains illegal characters\n";
-    }
-    die "zone ID '$id' can't be more length than 8 characters\n" if length($id) > 8;
-    return $id;
-}
-
 my $defaultData = {
 
     propertyList => {
-- 
2.47.3

[PATCH pve-storage v2 1/1] fix #7077: lvm: Improve ID verification error messages

[Arthur Bied-Charreton]

The regex in Storage::parse_lvm_name has an implicit `length >= 2` 
constraint, and shorter IDs failed with "contains illegal characters". 
Add explicit length check to return a clearer error message in this case.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 src/PVE/Storage/Plugin.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/PVE/Storage/Plugin.pm b/src/PVE/Storage/Plugin.pm
index 6f3d691..58f714c 100644
--- a/src/PVE/Storage/Plugin.pm
+++ b/src/PVE/Storage/Plugin.pm
@@ -333,6 +333,11 @@ PVE::JSONSchema::register_format('pve-storage-vgname', \&parse_lvm_name);
 sub parse_lvm_name {
     my ($name, $noerr) = @_;
 
+    if (length($name) < 2) {
+        return undef if $noerr;
+        die "lvm name '$name' can't be shorter than 2 characters\n";
+    }
+
     if ($name !~ m/^[a-z0-9][a-z0-9\-\_\.]*[a-z0-9]$/i) {
         return undef if $noerr;
         die "lvm name '$name' contains illegal characters\n";
-- 
2.47.3

[PATCH proxmox v2 1/1] fix #7077: Improve SDN ID validation error messages

[Arthur Bied-Charreton]

The regexes for verifying SDN (controller)? IDs have implicit `length >=
2` constraints resulting in less-than-ideal error messages in case of
length mismatch.

Add explicit length checks before checking the regexes.

Signed-off-by: Arthur Bied-Charreton <a.bied-charreton@proxmox.com>
---
 pve-api-types/src/types/verifiers.rs | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/pve-api-types/src/types/verifiers.rs b/pve-api-types/src/types/verifiers.rs
index c45063b5..f731f13b 100644
--- a/pve-api-types/src/types/verifiers.rs
+++ b/pve-api-types/src/types/verifiers.rs
@@ -266,7 +266,11 @@ pub fn verify_vlan_id_or_range(s: &str) -> Result<(), Error> {
 
 pub fn verify_sdn_id(s: &str) -> Result<(), Error> {
     if s.len() > 8 {
-        bail!("SDN ID cannot be longer than 8 characters")
+        bail!("SDN ID cannot be longer than 8 characters");
+    }
+
+    if s.len() < 2 {
+        bail!("SDN ID cannot be shorter than 2 characters");
     }
 
     if !SDN_ID.is_match(s) {
@@ -278,7 +282,11 @@ pub fn verify_sdn_id(s: &str) -> Result<(), Error> {
 
 pub fn verify_sdn_controller_id(s: &str) -> Result<(), Error> {
     if s.len() > 64 {
-        bail!("SDN controller ID cannot be longer than 64 characters")
+        bail!("SDN controller ID cannot be longer than 64 characters");
+    }
+
+    if s.len() < 2 {
+        bail!("SDN controller ID cannot be shorter than 2 characters");
     }
 
     if !SDN_CONTROLLER_ID.is_match(s) {
-- 
2.47.3