[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 at 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.