* [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas @ 2026-03-26 13:46 Robert Obkircher 2026-03-27 9:19 ` Dominik Csapak 0 siblings, 1 reply; 5+ messages in thread From: Robert Obkircher @ 2026-03-26 13:46 UTC (permalink / raw) To: pbs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas 2026-03-26 13:46 [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas Robert Obkircher @ 2026-03-27 9:19 ` Dominik Csapak 2026-03-27 10:20 ` Robert Obkircher 0 siblings, 1 reply; 5+ messages in thread From: Dominik Csapak @ 2026-03-27 9:19 UTC (permalink / raw) To: Robert Obkircher, pbs-devel 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}`; > } ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas 2026-03-27 9:19 ` Dominik Csapak @ 2026-03-27 10:20 ` Robert Obkircher 2026-03-27 10:47 ` Dominik Csapak 0 siblings, 1 reply; 5+ messages in thread From: Robert Obkircher @ 2026-03-27 10:20 UTC (permalink / raw) To: Dominik Csapak, pbs-devel 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}`; >> } > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas 2026-03-27 10:20 ` Robert Obkircher @ 2026-03-27 10:47 ` Dominik Csapak 2026-03-27 12:52 ` Robert Obkircher 0 siblings, 1 reply; 5+ messages in thread From: Dominik Csapak @ 2026-03-27 10:47 UTC (permalink / raw) To: Robert Obkircher, pbs-devel 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}`; >>> } >> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas 2026-03-27 10:47 ` Dominik Csapak @ 2026-03-27 12:52 ` Robert Obkircher 0 siblings, 0 replies; 5+ messages in thread From: Robert Obkircher @ 2026-03-27 12:52 UTC (permalink / raw) To: Dominik Csapak, pbs-devel 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}`; >>>> } >>> > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-03-27 12:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2026-03-26 13:46 [PATCH v1 proxmox-backup] www: percent-encode maintenance mode message to allow commas Robert Obkircher 2026-03-27 9:19 ` Dominik Csapak 2026-03-27 10:20 ` Robert Obkircher 2026-03-27 10:47 ` Dominik Csapak 2026-03-27 12:52 ` Robert Obkircher
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox