[pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[Dominik Csapak]

if a real json boolean is sent via the api, $value is a
JSON::PP::Boolean here instead of a string/scalar

so we should validate that too

the $value itself can be used normally in conditions like
----
if ($value) {
----

This worked for most api calls by accident before commit:
f398a3d ("proxy request: forward json content type and parameters")

since when the call was proxied to pvedaemon or another node, it would
get translated to a www-form-urlencoded parameter instead of json
and most (if not all) api calls that accept boolean parameters in the
body (POST/PUT) are forwarded to pvedaemon

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
i tested this with a few api calls (e.g. in the user creation/edit)
and it worked, but maybe the safer option would be to convert those
values to '1'/'0' ? we could reuse the 'normalize_legacy_param_formats'
function in RESTHandler for this, but this only checks the top level
parameters (which would be enough for now)

 src/PVE/JSONSchema.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 85d47f2..ebe443f 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -10,6 +10,7 @@ use Devel::Cycle -quiet; # todo: remove?
 use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
 use PVE::Exception qw(raise);
 use HTTP::Status qw(:constants);
+use JSON;
 use Net::IP qw(:PROC);
 use Data::Dumper;
 
@@ -1039,7 +1040,9 @@ sub check_type {
 	    # qr// regexes can be used as strings and make sense for format=regex
 	    return 1;
 	} else {
-	    if ($vt) {
+	    if ($type eq 'boolean' && JSON::is_bool($value)) {
+		return 1;
+	    } elsif ($vt) {
 		add_error($errors, $path, "type check ('$type') failed - got $vt");
 		return undef;
 	    } else {
-- 
2.30.2

Re: [pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[Wolfgang Bumiller]

On Thu, Jun 15, 2023 at 11:32:15AM +0200, Dominik Csapak wrote:
> if a real json boolean is sent via the api, $value is a
> JSON::PP::Boolean here instead of a string/scalar
> 
> so we should validate that too
> 
> the $value itself can be used normally in conditions like
> ----
> if ($value) {
> ----
> 
> This worked for most api calls by accident before commit:
> f398a3d ("proxy request: forward json content type and parameters")
> 
> since when the call was proxied to pvedaemon or another node, it would
> get translated to a www-form-urlencoded parameter instead of json
> and most (if not all) api calls that accept boolean parameters in the
> body (POST/PUT) are forwarded to pvedaemon
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> i tested this with a few api calls (e.g. in the user creation/edit)
> and it worked, but maybe the safer option would be to convert those
> values to '1'/'0' ? we could reuse the 'normalize_legacy_param_formats'
> function in RESTHandler for this, but this only checks the top level
> parameters (which would be enough for now)
> 
>  src/PVE/JSONSchema.pm | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 85d47f2..ebe443f 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -10,6 +10,7 @@ use Devel::Cycle -quiet; # todo: remove?
>  use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
>  use PVE::Exception qw(raise);
>  use HTTP::Status qw(:constants);
> +use JSON;
>  use Net::IP qw(:PROC);
>  use Data::Dumper;
>  
> @@ -1039,7 +1040,9 @@ sub check_type {
>  	    # qr// regexes can be used as strings and make sense for format=regex
>  	    return 1;
>  	} else {
> -	    if ($vt) {

^ I think we should keep this

> +	    if ($type eq 'boolean' && JSON::is_bool($value)) {

^ and just have this _inside_ the `if ($vt)` case - since that should be
set in this case?

> +		return 1;
> +	    } elsif ($vt) {
>  		add_error($errors, $path, "type check ('$type') failed - got $vt");
>  		return undef;
>  	    } else {
> -- 
> 2.30.2

Re: [pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[Dominik Csapak]

On 6/15/23 11:51, Wolfgang Bumiller wrote:
> On Thu, Jun 15, 2023 at 11:32:15AM +0200, Dominik Csapak wrote:
>> if a real json boolean is sent via the api, $value is a
>> JSON::PP::Boolean here instead of a string/scalar
>>
>> so we should validate that too
>>
>> the $value itself can be used normally in conditions like
>> ----
>> if ($value) {
>> ----
>>
>> This worked for most api calls by accident before commit:
>> f398a3d ("proxy request: forward json content type and parameters")
>>
>> since when the call was proxied to pvedaemon or another node, it would
>> get translated to a www-form-urlencoded parameter instead of json
>> and most (if not all) api calls that accept boolean parameters in the
>> body (POST/PUT) are forwarded to pvedaemon
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> i tested this with a few api calls (e.g. in the user creation/edit)
>> and it worked, but maybe the safer option would be to convert those
>> values to '1'/'0' ? we could reuse the 'normalize_legacy_param_formats'
>> function in RESTHandler for this, but this only checks the top level
>> parameters (which would be enough for now)
>>
>>   src/PVE/JSONSchema.pm | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>> index 85d47f2..ebe443f 100644
>> --- a/src/PVE/JSONSchema.pm
>> +++ b/src/PVE/JSONSchema.pm
>> @@ -10,6 +10,7 @@ use Devel::Cycle -quiet; # todo: remove?
>>   use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
>>   use PVE::Exception qw(raise);
>>   use HTTP::Status qw(:constants);
>> +use JSON;
>>   use Net::IP qw(:PROC);
>>   use Data::Dumper;
>>   
>> @@ -1039,7 +1040,9 @@ sub check_type {
>>   	    # qr// regexes can be used as strings and make sense for format=regex
>>   	    return 1;
>>   	} else {
>> -	    if ($vt) {
> 
> ^ I think we should keep this
> 
>> +	    if ($type eq 'boolean' && JSON::is_bool($value)) {
> 
> ^ and just have this _inside_ the `if ($vt)` case - since that should be
> set in this case?
> 

sure makes sense, i'll wait with the next version if someone has any additional thing
to say about the general approach (@thomas? @fabian? @fiona?)

Re: [pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[Thomas Lamprecht]

Am 15/06/2023 um 13:12 schrieb Dominik Csapak:
> On 6/15/23 11:51, Wolfgang Bumiller wrote:
>> On Thu, Jun 15, 2023 at 11:32:15AM +0200, Dominik Csapak wrote:
>>> if a real json boolean is sent via the api, $value is a
>>> JSON::PP::Boolean here instead of a string/scalar
>>>
>>> so we should validate that too
>>>
>>> the $value itself can be used normally in conditions like
>>> ----
>>> if ($value) {
>>> ----
>>>
>>> This worked for most api calls by accident before commit:
>>> f398a3d ("proxy request: forward json content type and parameters")
>>>
>>> since when the call was proxied to pvedaemon or another node, it would
>>> get translated to a www-form-urlencoded parameter instead of json
>>> and most (if not all) api calls that accept boolean parameters in the
>>> body (POST/PUT) are forwarded to pvedaemon
>>>
>>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>>> ---
>>> i tested this with a few api calls (e.g. in the user creation/edit)
>>> and it worked, but maybe the safer option would be to convert those
>>> values to '1'/'0' ? we could reuse the 'normalize_legacy_param_formats'
>>> function in RESTHandler for this, but this only checks the top level
>>> parameters (which would be enough for now)
>>>
>>>   src/PVE/JSONSchema.pm | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
>>> index 85d47f2..ebe443f 100644
>>> --- a/src/PVE/JSONSchema.pm
>>> +++ b/src/PVE/JSONSchema.pm
>>> @@ -10,6 +10,7 @@ use Devel::Cycle -quiet; # todo: remove?
>>>   use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
>>>   use PVE::Exception qw(raise);
>>>   use HTTP::Status qw(:constants);
>>> +use JSON;
>>>   use Net::IP qw(:PROC);
>>>   use Data::Dumper;
>>>   @@ -1039,7 +1040,9 @@ sub check_type {
>>>           # qr// regexes can be used as strings and make sense for format=regex
>>>           return 1;
>>>       } else {
>>> -        if ($vt) {
>>
>> ^ I think we should keep this
>>
>>> +        if ($type eq 'boolean' && JSON::is_bool($value)) {
>>
>> ^ and just have this _inside_ the `if ($vt)` case - since that should be
>> set in this case?
>>
> 
> sure makes sense, i'll wait with the next version if someone has any additional thing
> to say about the general approach (@thomas? @fabian? @fiona?)
> 

doing it inside seams reasonable, but yeah, as this is the type check code,
not the actual use sites, it feels like we maybe could miss something..

But, fixing that then, e.g., with normalizing to 1 and 0, respectively, wouldn't
be a change to public ABI or the like, so we can always go for that if reports pop
up.

Re: [pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api

[Wolfgang Bumiller]

On Thu, Jun 15, 2023 at 02:28:02PM +0200, Thomas Lamprecht wrote:
> Am 15/06/2023 um 13:12 schrieb Dominik Csapak:
> > On 6/15/23 11:51, Wolfgang Bumiller wrote:
> >> On Thu, Jun 15, 2023 at 11:32:15AM +0200, Dominik Csapak wrote:
> >>> if a real json boolean is sent via the api, $value is a
> >>> JSON::PP::Boolean here instead of a string/scalar
> >>>
> >>> so we should validate that too
> >>>
> >>> the $value itself can be used normally in conditions like
> >>> ----
> >>> if ($value) {
> >>> ----
> >>>
> >>> This worked for most api calls by accident before commit:
> >>> f398a3d ("proxy request: forward json content type and parameters")
> >>>
> >>> since when the call was proxied to pvedaemon or another node, it would
> >>> get translated to a www-form-urlencoded parameter instead of json
> >>> and most (if not all) api calls that accept boolean parameters in the
> >>> body (POST/PUT) are forwarded to pvedaemon
> >>>
> >>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> >>> ---
> >>> i tested this with a few api calls (e.g. in the user creation/edit)
> >>> and it worked, but maybe the safer option would be to convert those
> >>> values to '1'/'0' ? we could reuse the 'normalize_legacy_param_formats'
> >>> function in RESTHandler for this, but this only checks the top level
> >>> parameters (which would be enough for now)
> >>>
> >>>   src/PVE/JSONSchema.pm | 5 ++++-
> >>>   1 file changed, 4 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> >>> index 85d47f2..ebe443f 100644
> >>> --- a/src/PVE/JSONSchema.pm
> >>> +++ b/src/PVE/JSONSchema.pm
> >>> @@ -10,6 +10,7 @@ use Devel::Cycle -quiet; # todo: remove?
> >>>   use PVE::Tools qw(split_list $IPV6RE $IPV4RE);
> >>>   use PVE::Exception qw(raise);
> >>>   use HTTP::Status qw(:constants);
> >>> +use JSON;
> >>>   use Net::IP qw(:PROC);
> >>>   use Data::Dumper;
> >>>   @@ -1039,7 +1040,9 @@ sub check_type {
> >>>           # qr// regexes can be used as strings and make sense for format=regex
> >>>           return 1;
> >>>       } else {
> >>> -        if ($vt) {
> >>
> >> ^ I think we should keep this
> >>
> >>> +        if ($type eq 'boolean' && JSON::is_bool($value)) {
> >>
> >> ^ and just have this _inside_ the `if ($vt)` case - since that should be
> >> set in this case?
> >>
> > 
> > sure makes sense, i'll wait with the next version if someone has any additional thing
> > to say about the general approach (@thomas? @fabian? @fiona?)
> > 
> 
> doing it inside seams reasonable, but yeah, as this is the type check code,
> not the actual use sites, it feels like we maybe could miss something..

Only if we have code that actually expects its booleans to be "1" or "0"
via 'eq'/==, which is IMO always wrong and I'd rather run into *that*
and fix it *there* than antifixing API clients to turn their booleans
into integerstringthings...

The API can already get JSON::Booleans - AFAIU we just happened to not
have any where *we* send them anywhere where we didn't previously have a
'protected' flag as well?

> 
> But, fixing that then, e.g., with normalizing to 1 and 0, respectively, wouldn't
> be a change to public ABI or the like, so we can always go for that if reports pop
> up.