public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Thomas Lamprecht <t.lamprecht@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Dominik Csapak <d.csapak@proxmox.com>,
	Wolfgang Bumiller <w.bumiller@proxmox.com>
Subject: Re: [pve-devel] [PATCH RFC common] fix #4778: fix boolean type check for json parameters over the api
Date: Thu, 15 Jun 2023 14:28:02 +0200	[thread overview]
Message-ID: <a3aaf668-1f5e-edff-d167-17d97e915f25@proxmox.com> (raw)
In-Reply-To: <320da32f-cb75-3f4c-4535-c52736325915@proxmox.com>

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.




  reply	other threads:[~2023-06-15 12:28 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-06-15  9:32 Dominik Csapak
2023-06-15  9:51 ` Wolfgang Bumiller
2023-06-15 11:12   ` Dominik Csapak
2023-06-15 12:28     ` Thomas Lamprecht [this message]
2023-06-15 12:43       ` Wolfgang Bumiller

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a3aaf668-1f5e-edff-d167-17d97e915f25@proxmox.com \
    --to=t.lamprecht@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    --cc=w.bumiller@proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal