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

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

[Robert Obkircher]

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}`;
             }
-- 
2.47.3

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

[Dominik Csapak]

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

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).

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}`;
>               }

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

[Robert Obkircher]

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|


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

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

>
> 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}`;
>>               }
>

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

[Dominik Csapak]

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|
> 
> 
> 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?

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

>>
>> 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}`;
>>>                }
>>

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

[Robert Obkircher]

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}`;
>>>>                }
>>>
>