all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses
@ 2024-04-08  9:30 Friedrich Weber
  2024-04-08 10:36 ` Stefan Sterz
  0 siblings, 1 reply; 4+ messages in thread
From: Friedrich Weber @ 2024-04-08  9:30 UTC (permalink / raw)
  To: pve-devel

Currently, `Proxmox.window.Edit` initializes `extraRequestParams` and
`submitOptions` to two objects that, if not overwritten, are shared
between all instances of subclasses. This bears the danger of
modifying the shared object in a subclass instead of overwriting it,
which affects all edit windows of the current session and can cause
hard-to-catch GUI bugs.

One such bug is the following: Currently, the `PVE.pool.AddStorage`
component inadvertently adds `poolid` to an `extraRequestParams`
object that is shared between all instances of `Proxmox.window.Edit`.
As a result, after adding a storage to a pool, opening any edit window
will send a GET request with a superfluous `poolid` parameter and
cause an error in the GUI:

> Parameter verification failed. (400)
> poolid: property is not defined in schema and the schema does not
> allow additional properties

This breaks all edit windows of the current session. A workaround is
to reload the current browser session.

To avoid this class of bugs in the future, implement a constructor
that makes copies of `extraRequestParams` and `submitOptions`. This
ensures that any subclass instance modifies only its own copies, and
modifications do not leak to other subclass instances.

Suggested-by: Stefan Sterz <s.sterz@proxmox.com>
Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    @Thomas, I've added a Suggested-by, feel free to remove/keep as you
    prefer.
    
    Changes from v1+v2:
    - As suggested by sterzy (thx!), avoid this class of bugs in a more
      generic fashion by introducing a `Proxmox.window.Edit` constructor
      that copies custom config objects
    - Added full error message to commit message for better searchability
    
    v2: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062561.html
    v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html

 src/window/Edit.js | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/window/Edit.js b/src/window/Edit.js
index d4a2b551..d5163dd7 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -69,6 +69,16 @@ Ext.define('Proxmox.window.Edit', {
     // onlineHelp of our first item, if set.
     onlineHelp: undefined,
 
+    constructor: function(conf) {
+	let me = this;
+	// make copies in order to prevent subclasses from accidentally writing
+	// to objects that are shared with other edit window subclasses
+	me.extraRequestParams = Object.assign({}, me.extraRequestParams);
+	me.submitOptions = Object.assign({}, me.submitOptions);
+	me.initConfig(conf);
+	me.callParent();
+    },
+
     isValid: function() {
 	let me = this;
 
-- 
2.39.2





^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses
  2024-04-08  9:30 [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses Friedrich Weber
@ 2024-04-08 10:36 ` Stefan Sterz
  2024-04-08 12:36   ` Thomas Lamprecht
  0 siblings, 1 reply; 4+ messages in thread
From: Stefan Sterz @ 2024-04-08 10:36 UTC (permalink / raw)
  To: Proxmox VE development discussion

On Mon Apr 8, 2024 at 11:30 AM CEST, Friedrich Weber wrote:
> Currently, `Proxmox.window.Edit` initializes `extraRequestParams` and
> `submitOptions` to two objects that, if not overwritten, are shared
> between all instances of subclasses. This bears the danger of
> modifying the shared object in a subclass instead of overwriting it,
> which affects all edit windows of the current session and can cause
> hard-to-catch GUI bugs.
>
> One such bug is the following: Currently, the `PVE.pool.AddStorage`
> component inadvertently adds `poolid` to an `extraRequestParams`
> object that is shared between all instances of `Proxmox.window.Edit`.
> As a result, after adding a storage to a pool, opening any edit window
> will send a GET request with a superfluous `poolid` parameter and
> cause an error in the GUI:
>
> > Parameter verification failed. (400)
> > poolid: property is not defined in schema and the schema does not
> > allow additional properties
>
> This breaks all edit windows of the current session. A workaround is
> to reload the current browser session.
>
> To avoid this class of bugs in the future, implement a constructor
> that makes copies of `extraRequestParams` and `submitOptions`. This
> ensures that any subclass instance modifies only its own copies, and
> modifications do not leak to other subclass instances.
>
> Suggested-by: Stefan Sterz <s.sterz@proxmox.com>
> Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
>
> Notes:
>     @Thomas, I've added a Suggested-by, feel free to remove/keep as you
>     prefer.
>
>     Changes from v1+v2:
>     - As suggested by sterzy (thx!), avoid this class of bugs in a more
>       generic fashion by introducing a `Proxmox.window.Edit` constructor
>       that copies custom config objects
>     - Added full error message to commit message for better searchability
>
>     v2: https://lists.proxmox.com/pipermail/pve-devel/2024-April/062561.html
>     v1: https://lists.proxmox.com/pipermail/pve-devel/2024-March/062179.html
>
>  src/window/Edit.js | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/src/window/Edit.js b/src/window/Edit.js
> index d4a2b551..d5163dd7 100644
> --- a/src/window/Edit.js
> +++ b/src/window/Edit.js
> @@ -69,6 +69,16 @@ Ext.define('Proxmox.window.Edit', {
>      // onlineHelp of our first item, if set.
>      onlineHelp: undefined,
>
> +    constructor: function(conf) {
> +	let me = this;
> +	// make copies in order to prevent subclasses from accidentally writing
> +	// to objects that are shared with other edit window subclasses
> +	me.extraRequestParams = Object.assign({}, me.extraRequestParams);
> +	me.submitOptions = Object.assign({}, me.submitOptions);
> +	me.initConfig(conf);
> +	me.callParent();


so, this seems like a fix bug a) creates bug b) type of situation...
this patch means that editing a pool allows changing the name suddenly,
but since we don't support that in the backend, that just creates a new
pool :/

this is due to the `editable` attribute depending on `isCreate`, which
in turn depends on the configs poolid being set. to fix this, the config
needs to also be passed to `callParent` so it can set the configurations
there too. so this line should be:

    me.callParent([conf]);

sorry, could have noticed that earlier in my suggestion. also this needs
to be an arrray as `callParent` expects a list of arguments to pass to
parent's function and not the parameters themselves directly.

> +    },
> +
>      isValid: function() {
>  	let me = this;
>





^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses
  2024-04-08 10:36 ` Stefan Sterz
@ 2024-04-08 12:36   ` Thomas Lamprecht
  2024-04-09  7:55     ` Friedrich Weber
  0 siblings, 1 reply; 4+ messages in thread
From: Thomas Lamprecht @ 2024-04-08 12:36 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stefan Sterz

Am 08/04/2024 um 12:36 schrieb Stefan Sterz:
> On Mon Apr 8, 2024 at 11:30 AM CEST, Friedrich Weber wrote:
>> +    constructor: function(conf) {
>> +	let me = this;
>> +	// make copies in order to prevent subclasses from accidentally writing
>> +	// to objects that are shared with other edit window subclasses
>> +	me.extraRequestParams = Object.assign({}, me.extraRequestParams);
>> +	me.submitOptions = Object.assign({}, me.submitOptions);
>> +	me.initConfig(conf);
>> +	me.callParent();
> 
> 
> so, this seems like a fix bug a) creates bug b) type of situation...
> this patch means that editing a pool allows changing the name suddenly,
> but since we don't support that in the backend, that just creates a new
> pool :/
> 
> this is due to the `editable` attribute depending on `isCreate`, which
> in turn depends on the configs poolid being set. to fix this, the config
> needs to also be passed to `callParent` so it can set the configurations
> there too. so this line should be:
> 
>     me.callParent([conf]);
> 
> sorry, could have noticed that earlier in my suggestion. also this needs
> to be an arrray as `callParent` expects a list of arguments to pass to
> parent's function and not the parameters themselves directly.

Using `me.callParent(arguments)` might be more proof to future changes and
arguments is an array (or well, iterator) already







^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses
  2024-04-08 12:36   ` Thomas Lamprecht
@ 2024-04-09  7:55     ` Friedrich Weber
  0 siblings, 0 replies; 4+ messages in thread
From: Friedrich Weber @ 2024-04-09  7:55 UTC (permalink / raw)
  To: Proxmox VE development discussion, Thomas Lamprecht, Stefan Sterz

On 08/04/2024 14:36, Thomas Lamprecht wrote:
> Am 08/04/2024 um 12:36 schrieb Stefan Sterz:
>> [...]
>> so, this seems like a fix bug a) creates bug b) type of situation...
>> this patch means that editing a pool allows changing the name suddenly,
>> but since we don't support that in the backend, that just creates a new
>> pool :/
>>
>> this is due to the `editable` attribute depending on `isCreate`, which
>> in turn depends on the configs poolid being set. to fix this, the config
>> needs to also be passed to `callParent` so it can set the configurations
>> there too. so this line should be:
>>
>>     me.callParent([conf]);
>>
>> sorry, could have noticed that earlier in my suggestion. also this needs
>> to be an arrray as `callParent` expects a list of arguments to pass to
>> parent's function and not the parameters themselves directly.
> 
> Using `me.callParent(arguments)` might be more proof to future changes and
> arguments is an array (or well, iterator) already

Thanks for spotting this, missed that during testing. Will send a v4.




^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2024-04-09  7:55 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-08  9:30 [pve-devel] [PATCH widget-toolkit v3] window: edit: avoid sharing custom config objects between subclasses Friedrich Weber
2024-04-08 10:36 ` Stefan Sterz
2024-04-08 12:36   ` Thomas Lamprecht
2024-04-09  7:55     ` Friedrich Weber

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal