WireInputData should allow multidimensional arrays

[abdusco]

Short description of the issue

I'm building an inputfield for a custom fieldtype. The inputfield will display a (possibly) long list of items with a number of fields in each. Instead of setting input name attributes beforehand and posting all field data back and forth and filtering saveable data on the backend, I'm trying to reduce the amount of processing by setting input names using JS only if it has been changed.

// using vue.js
<input @change="changed($event, item, 'author')" type="text" v-model="item.author">

changed(event, item, key) {
    input = event.target;
    if (!input.name) input.name = `changed[${item.id}][${key}]`; // name as multidimensional array
}

When the page is submitted, this was supposed to give me an array like this

But after some head scratching I found out that WireInputData class actually removes multidimensional arrays from $_POST (and $_GET) in cleanArray() method

/**
 * Clean an array of data
 * Removes multi-dimensional arrays and slashes (if applicable) 
 * @param array $a
 * @return array
 */
protected function cleanArray(array $a) {
    $clean = array();
    foreach($a as $key => $value) {
        if(is_array($value)) continue; // we only allow one dimensional arrays
        if(is_string($value) && $this->stripSlashes) $value = stripslashes($value);
        $clean[$key] = $value;
    }
    return $clean;
}

Expected behavior

$input->post should allow multidimensional arrays

Actual behavior

It removes them

Optional: Suggestion for a possible fix

Removing the following line solves the issue

if(is_array($value)) continue; // we only allow one dimensional arrays

Steps to reproduce the issue

Create a set of inputs like this

<input name="changed[1][text]"/> // defaults to type=text
<input name="changed[1][status]"/>
<input name="changed[2][status]"/>

Post the form, $input->post->changed will be an empty array.

Setup/Environment

PW 3.0.76

[LostKobrakai]

Just so you know: This is an intentional limitation by @ryancramerdesign, which was often discussed and requested to be changed. I can't remember the exact reasoning right now.

[abdusco]

I'm guess this is why:
https://security.stackexchange.com/questions/127808/is-array-injection-a-vulnerability-and-what-is-the-proper-term-for-it

2-3 levels should be enough. No need to allow arbitrarily deep arrays.

[matjazpotocnik]

https://processwire.com/talk/topic/691-wireinput-only-allows-one-dimensional-arrays-why/?tab=comments#comment-5719

[abdusco]

Probably what I will do is just limit the recursion to 2 or 3 levels to keep it safe. I don't see any potential issues with adding it.

Hopefully :]

[teppokoivula]

One argument against enabling an arbitrary number of levels (2, 3, or whatever) would be that this will, again, cause unexpected issues that can be somewhat difficult to debug. Though it may solve most use cases, it could cause even more head scratching when deeper structures are needed.

That being said, I'm not strongly against enabling a number of levels. Another solution would be a configuration setting, or some other method of declaring that "right now I need this many levels".

[abdusco]

Another solution would be a configuration setting, or some other method of declaring that "right now I need this many levels".

That sounds like the best approach, just like maxUrlSegments setting, a $config setting for input array depth would be great to have, instead of removing the value unexpectedly.

[netcarver]

@ryancramerdesign Do you want to take any further action on this issue? How about making the cleanArray() method hookable so people can override default behaviour if needed?

[ryancramerdesign]

@netcarver Thanks for your help in checking through all these issue reports, I really appreciate your efforts here.

Since neither the core or any of the PW Inputfields use multi-dimensional arrays, and aren't likely to, I don't want to add support for them at this time. After all, the PHP superglobals are already available when/if someone needs multi-dimensional arrays in input. If it weren't already easily accessible from the PHP superglobals then of course we'd support it. But since one can already get to this input by other means, I think it's better for PW to limit the possible inputs to it when possible. PW has always focused on really limiting those input vectors and and multi-dimensional arrays are just one of those doors I don't want to open.

[netcarver]

@ryancramerdesign @abdusco @LostKobrakai @matjazpotocnik @teppokoivula Ok, unless anyone else wants to make another argument for this, I intend to add the "won't fix" label and close this in a week or so.

[teppokoivula]

My vote would still go for a two-fold solution, where this would a) be configurable behaviour, i.e. a new config setting for the max depth, and b) where one could override that max depth value on a case by case basis when requesting data.

I understand the point that @ryancramerdesign makes – and obviously it's his choice in the end – but personally I prefer $input over PHP superglobals in all the ProcessWire related code I write, and see it as an important part of the toolset ProcessWire provides to developers.

As such, I do think that it's worth considering this, even if the core itself doesn't currently require it.

I'll admit that it's pretty rare that I personally really require multidimensional arrays of input – but when I do, having to use a mixture of $input and superglobals feels less than optimal.

[netcarver]

At the moment this seems to clearly be a "won't fix" for Ryan. I am therefore moving it to the requests repo where this conversation can be continued if needed.