Re: [pbs-devel] [PATCH v4 proxmox{, -backup} 0/2] close #4763: client: added command to forget backup group

[Christian Ebner <c.ebner@proxmox.com>]

On 4/18/24 11:13, Gabriel Goller wrote:
> On Wed Apr 17, 2024 at 4:15 PM CEST, Christian Ebner wrote:
>> Hi,
>> thanks for tackling this issue.
>>
>> The group forget command is something I missed quite a lot when doing
>> testing on PBS with accumulating snapshots in a group an not wanting to
>> fallback to the UI (I actually always went the route of deleting the
>> snapshot folder).
>>
>> Tested the following:
>> - Created a few backup snapshots, creating thereby a new backup group
>> - Tried to delete the group while backup is running
>>     result was as expected: delete failed, missing lock error
>> - Tried to delete the group while a restore is running
>>     result was as expected: delete failed, missing lock (although
>> different from the previous one)
>> - Tried to remove without any other task running
>>     result was as expected: group deleted, including the group folder in
>> the datastore
>> - Tried to delete empty group
>>     result was as expected: group deleted, including the group folder in
>> the datastore
>>
>> Two further things I noticed:
>>
>> The confirmation dialog also allows me to type in `yolo` or any string
>> starting with `yY` and nevertheless accepts this as valid confirmation
>> input (similar for negation). Should we limit this further?
> 
> It seems 'apt' also matches just on the beginning character, but
> nevertheless we can do this, wouldn't hurt restricting it more
> 
> I think adding a '$' on the regex should do the trick:
> 
>      let no_regex: Regex = Regex::new("^[nN]$").unwrap();
> 

Yeah, however allowing more might be a feature (probably this should 
also allow at least `yes` and `no`). Feel free to decide, I just wanted 
to mention this, in case it was not intentional.

> 
>> When one tries to delete a non existing group, the dialog asks me for
>> confirmation, failing however afterwards with an error message, leaking
>> also the datastore path to the client. While the former is not an issue
>> and the intention is to be able to remove empty groups, the latter is
>> not okay in my opinion.
>> So either check if the group even exists before asking for confirmation,
>> or map the error to not leak the datastore path.
> 
> The thing is that we don't differentiate between an empty group or a
> nonexistent group — at least when using the api. This means that even
> the list-groups api call will **not** return a group if it doesn't contain
> any snapshots, but deleting it will succeed (because it still exists) >
> What we can do is obviously ignore the error message and simply return a
> generic "failed to remove group" or "group not found" to avoid leaking
> stuff. Although debugging a issue will be much harder with these vague
> error messages.
> 
> IMO leaking the datastore is not a big issue, as the client can also
> list task logs and the datastore path is going to be visible in there as
> well.

It is not a big issue, agreed, but nevertheless an information leak. But 
you are right, the task log does contain that information as well, so 
there is at least no additional information here. Given that, this is 
fine I guess.

> 
>> For the rest consider this:
>>
>> Tested-by: Christian Ebner <c.ebner@proxmox.com>
> 
> Thanks for testing this!



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel