Re: [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas

[Robert Obkircher <r.obkircher@proxmox.com>]

On 3/27/26 11:47, Dominik Csapak wrote:
>
>
> On 3/27/26 11:19 AM, Robert Obkircher wrote:
>>
>> On 3/27/26 10:18, Dominik Csapak wrote:
>>> just my 2 cents:
>>>
>>> we should probably just adapt the frontend parser for propertystrings
>>> to also handle quoting. one idea was to use the rust crate + wasm
>>> to do that (so we have a consistent parser across frontend+backed)
>>> not sure it worth the effort to integrate wasm just for this single
>>> parser
>> Just to be clear, rust currently doesn't quote [1] correctly either:
>>
>> |letmode=MaintenanceMode{ ty:MaintenanceType::ReadOnly,
>> message:Some("abc,def".to_string()), };
>> lets=proxmox_schema::property_string::PropertyString::new(mode)
>> .to_property_string() .unwrap();
>> assert_eq!(s,"read-only,message=abc,def");
>> letd=MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
>>
>> &s, &MaintenanceMode::API_SCHEMA, )) .expect("from
>> to_property_string");// panics| 
Oops, sorry about the formatting.
>>
>>
>> This is documented here[1].
>>
>> [1]
>> https://git.proxmox.com/?p=proxmox.git;a=blob;f=proxmox-schema/src/schema.rs;h=47ee94dfa05a4bb38cb9f7772ffed72576408353;hb=HEAD#l1766
>>
>>
>
> thanks for pointing that out, I thought we had proper quoting support
> there (for some reason?) maybe only the parser and not the
> serializer.... IMO we should fix that too? 

It is mostly supported, the problem was just serialization of strings
that contain commas.

>
>>>
>>> we can ofc extend the javascript parser to do that. I have some
>>> intial
>>> js implementation of that lying around (from 2022, and only for
>>> pve-manager though).
>> Note that parseMaintenanceMode is a custom parser that doesn't use
>> parsePropertyString.
>>
>
> sure but that has a big overlap so we could probably reuse
> a proper parser here too... 
It would be nice to re-use it via wasm but a JS version shouln't be
too difficult.

I also found parse_property_string in JSONSchema.pm, which would have
to change as well.

>
>>>
>>> i could send that for pbs if it's desired
>>>
>>> if neither is an option, we could ofc go with url encoding/decoding,
>>> but the downside is that every (api)client has to do it themselves
>>> (think pdm for example)
>>>
>>>
>>> On 3/26/26 2:48 PM, Robert Obkircher wrote:
>>>> Commas and equal signs caused problems because the maintenance mode
>>>> message is stored in a property string.
>>>>
>>>> With a comma and no quotes (e.g. 'a,b'), the backend failed to
>>>> update
>>>> datastore.cfg because 'read-only,message=a,b' couldn't be re-parsed.
>>>> Adding a quote triggered the correct escape logic in the backend,
>>>> but
>>>> then the frontend displayed the mode and message incorrectly. It
>>>> also
>>>> cut off everything after the first equal sign and silently stripped
>>>> backslashes.
>>>>
>>>> Percent encoding was chosen because MaintenanceMode::check already
>>>> decoded the message. Previously, this potentially caused the error
>>>> message to differ from what was displayed in the web UI.
>>>>
>>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>>> ---
>>>> I'm not sure if this is a good idea or if we should simply forbid
>>>> those
>>>> characters.
>>>>
>>>> I also tried changing ElementSerializer::serialize_str to quote like
>>>> ElementSerializeSeq, but that wouldn't be sufficient because the
>>>> parser
>>>> in the frontend would still split by comma and parse something like
>>>> 'read-only,message="a,b"' as type 'b"' and message '"a'.
>>>>
>>>>    www/Utils.js                     | 6 +++++-
>>>>    www/window/MaintenanceOptions.js | 5 ++---
>>>>    2 files changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/www/Utils.js b/www/Utils.js
>>>> index fc9a5916..5e1ee0c6 100644
>>>> --- a/www/Utils.js
>>>> +++ b/www/Utils.js
>>>> @@ -800,7 +800,11 @@ Ext.define('PBS.Utils', {
>>>>                ([m, msg], pair) => {
>>>>                    const [key, value] = pair.split('=');
>>>>                    if (key === 'message') {
>>>> -                    return [m, value.replace(/^"(.*)"$/,
>>>> '$1').replace(/\\"/g, '"')];
>>>> +                    try {
>>>> +                        return [m, decodeURIComponent(value)];
>>>> +                    } catch {
>>>> +                        return [m, value];
>>>> +                    }
>>>>                    } else {
>>>>                        return [value ?? key, msg];
>>>>                    }
>>>> diff --git a/www/window/MaintenanceOptions.js
>>>> b/www/window/MaintenanceOptions.js
>>>> index 9a735e5e..e9740843 100644
>>>> --- a/www/window/MaintenanceOptions.js
>>>> +++ b/www/window/MaintenanceOptions.js
>>>> @@ -39,9 +39,8 @@ Ext.define('PBS.window.MaintenanceOptions', {
>>>>                if (values.delete === 'maintenance-type') {
>>>>                    values.delete = 'maintenance-mode';
>>>>                } else if (values['maintenance-type']) {
>>>> -                const message = (values['maintenance-msg'] ?? '')
>>>> -                    .replaceAll('\\', '')
>>>> -                    .replaceAll('"', '\\"');
>>>> +                // property string values can't contain symbols
>>>> like commas and equal signs
>>>> +                const message =
>>>> encodeURIComponent(values['maintenance-msg'] ?? '');
>>>>                    const maybe_message = values['maintenance-msg'] ?
>>>> `,message="${message}"` : '';
>>>>                    values['maintenance-mode'] =
>>>> `type=${values['maintenance-type']}${maybe_message}`;
>>>>                }
>>>
>