userfrosting / UserFrosting

Modern PHP user login and management framework
https://www.userfrosting.com
Other
1.64k stars 366 forks source link

[Documentation][UF5.1]Input Arrays and Fortress #1251

Closed StrykeSlammerII closed 6 months ago

StrykeSlammerII commented 7 months ago

Input arrays

Years ago when I was first exposed to PHP, I learned about input arrays.

In a form, you can give multiple <input> elements the same name= attribute, if it ends with (or just uses?) PHP's array notation, such as <input name="AllTheseInputs[]">.

If your page dynamicly generates rows of input fields, it's much easier to use an array Input[] than having to track Input1, Input2, Input3 (etc) --and when you parse <input name="AllTheseInputs[]">, PHP treats AllTheseInputs like any other array.

Fortress

Now, what happens when we try to transform and validate an array like that in Fortress?

Validators

The Fortress /learn page says the validation step is built on Valitron, and the usage notes there say it supports dot notation with * wildcards.

Given an input array SomeInput[], this lets us validate each element by using SomeInput.* in our Fortress schema.

Validations against the base array SomeInput will instead test against the array itself--so we can use the required or the (undocumented in UF) array validators here.

Transforms

Manual testing on a small UF install shows that transforms are (silently) not applied using the Valitron element format SomeInput.*.

However, if we use the standard format SomeInput, the transform is applied to all elements.

Whitelisting

I did not test how whitelisting is applied to arrays, but the above demo shows that CSRF fields are stripped by Transform when they are not included in the schema.

If an input array will be sent through Fortress on the server side, it's probably good practice to use require and/or array validators on the array object, and this should be enough to whitelist it.

Updating the schema in the above demo should be easy enough if additional testing is needed on this point.

Sample YAML schema

Putting all of this together, we come up with a schema something like this to validate and transform <input name="InputArray[]" ...:

InputArray:
  validators:
    required:
      message: "InputArray is required"
    array:
      message: "InputArray must be an array (of input fields)"
  #these will transform the entire array
  transformations:
  - purge
  #- trim

InputArray.*:
  validators:
    integer:
      message: "Input elements must be integers."
  #these won't transform anything
  transformations:
  #- purge
  - trim

Disclaimer

Much later in /learn, there's a mention that in UserFrosting, forms are usually submitted via an AJAX request.

I don't have experience with AJAX yet, so I don't know whether input arrays are as useful there.

If not, it may be worth an earlier mention instead? "If you were thinking of using input arrays, please stop and consider this other method that we'll explain in detail later."

Currently, Fortress is shown in chapter 8-f-1 vs AJAX Forms in chapter 15-d-1. That's quite a lot of material between the two concepts!

Multidimensional Arrays

I also did not test multidimensional arrays, as they're outside of my personal use case and it felt like setting one up in my display Twig might be excessive.

Valitron already supports them via dot notation, so our validators are fine.

At some point we may need to check how Fortress Transforms treat them, and update either Fortress or /learn appropriately.

lcharette commented 6 months ago

Manual testing on a small UF install shows that transforms are (silently) not applied using the Valitron element format SomeInput.*. However, if we use the standard format SomeInput, the transform is applied to all elements.

I think this is the real issue here. It makes sense to me that both InputArray and InputArray.* are required in the schema. InputArray will validate the element (array) is there (as in some cases we may want this value to be optional) and InputArray.* will validate each element individually.

The same logic should be applied with transform. They should obey InputArray.*, but maybe InputArray should also apply transform as some sort of wrapper.

However, I did test the multidimensional arrays. Something like this :

<input type="number" name="Settings[0][threshold]" value="20"/>
<input type="text" name="Settings[0][name]" value="My String"/>

<input type="number" name="Settings[1][threshold]" value="20"/>
<input type="text" name="Settings[1][name]" value="My String"/>

And this exception is thrown : strip_tags(): Argument #1 ($string) must be of type string, array given

So second issue.

lcharette commented 6 months ago

Alright... It took all my engineering skills, but I think I got it.

I added a bunch of tests in 2dd8d72 to go through many InputArray and Multidimensional Array scenario : https://github.com/userfrosting/framework/commit/2dd8d72e6783bc99d0d80454e98ac861d217d1b6#diff-4341183ecfbf3df898324f27fcbd6cb719b7010332fd97b095b955c6d90e04e1R338

The same logic should be applied with transform. They should obey InputArray.*, but maybe InputArray should also apply transform as some sort of wrapper.

You can check out the Multidimensional.yaml and InputArray.yaml as an example of how this is implemented.

And this exception is thrown : strip_tags(): Argument #1 ($string) must be of type string, array given

This is also fixed.

If you find another scenario not tested, I'll add it. I'll review the doc when I get the chance and release the fix then.

lcharette commented 6 months ago

Fix released in Framework 5.1.1 👍