Nested Validation Data

[jopicornell]

• Laravel Version: v5.7.16
• PHP Version: 7.2.13
• Database Driver & Version: no applies

Description:

I try to validate an object field of an array on a JSON request and I want not to allow the user to introduce more fields on every object than the specified on that validator. When I call the validated() method on the request it returns all the fields.

It is the same as issue laravel/framework#23988 but for arrays on a nested field.

Steps To Reproduce:

You should have a validator with a field that's an array and this array includes an object field that has an id and some other field:

    Validator::make([
            'items' => [
                [
                    'product' => [
                        'id' => 1,
                        'name' => 'myName'
                    ]
                ], [
                    'product' => [
                        'id' => 2,
                        'name' => 'myName2'
                    ]
                ]
            ]
        ], ['items'=> 'required|array', 'items.*.product.id' => 'required'])->validate();

it should return:

{
	"items": [{
			"product": {
				"id": 1,
			}
		},
		{
			"product": {
				"id": 2,
			}
		}
	]
}

but it returns the name field of the products too.

As a note, the bug comes when using the 'items'=> 'required|array' validator, but I can't remove this validator because this array field should be present on the request always, even if it's empty. It has to be present on the request.

[dmitry-ivanov]

@jpicornell, if you don't mind, I've simplified your example a bit:

$data = [
    'items' => [
        ['product' => ['id' => 1, 'name' => 'One']],
        ['product' => ['id' => 2, 'name' => 'Two']],
    ],
];

$rules = [
    'items' => 'required', // this rule causes the problem
    'items.*.product.id' => 'required',
];

$validator = Validator::make($data, $rules);

$validated = $validator->validate();

dd($validated);

As you've already noted, this problem is caused by the 'items' => 'required' rule.

I will continue to investigate, but, @driesvints, are you sure that it's not expected behavior?

I understand that @jpicornell is trying to get only validated IDs, but we also can look at this from another angle.

As we do have the rule for items, which are required, that's why Laravel merges those rules "results", and returns the full objects with names.

I feel that it's not an obvious bug, and possible PR can be rejected.

[driesvints]

I'm indeed not certain so I'm gonna de-label this as a bug for now. Will take a look at another time.

[dmitry-ivanov]

I've checked Validator's tests, and there are no tests which cover a similar case.

The validated data is composed here.

It's just a simple data_get/array_set combo for all rules' keys.

So, probably, we have to analyze those keys in some way to solve this issue.

I'm going to think about it more, but I'm still not sure that Taylor would accept such PR.

[jopicornell]

@dmitry-ivanov No problem changing my example! The problem is that I control the request items to create Eloquent models. I see that in a way to build relations, if it's only the id, I associate the parent object, if I receive more fields, I associate and update. That's giving me problems and security concerns because a user can update fields I don't want them to in a given request.

I don't know your previous thoughts about that, but I get it as if it is like not having nested validations. Required is only that this field should be present and not null, bot nothing more. According to the documentation:
The field under validation must be present in the input data and not empty.
So I get that as it don't have to check anything about nested data but it's a required array. required.

The main problem is that I cannot do that an array with nested validation is required if it's not through that validator, but if I remove the required validator the field can be empty, and I don't want that.

Also I want to point that on first level validation ( items..name) is correctly validated, but nested of nested (items..product.id) is not.

Maybe the solution to not break things is to create a "required_array" validation.

I'll be here to give my thoughts and anything I could do to help, I'll do it.

Thanks!

[dmitry-ivanov]

@jpicornell Thank you for your detailed explanation!

I feel your pain about that, and I've made a PR yesterday, which solves this problem.

Would appreciate your and @driesvints feedback about PR.

[dmitry-ivanov]

So, my first PR was closed.

I've split it into two smaller PRs:

• [5.7] Validator code style fixes (from #27205) framework#27230 (merged)
• [5.7] More accurate validated data for intersecting rules framework#27231 (the fix is here, I hope it will be merged too)

[themsaid]

I don't think this is a bug, you have a items key in the validation rules so when you ask for validated data you get the entire value of the items key. If we change this it's a breaking change, also it's very opinionated so I don't think we have to change it because we will receive a bug report shortly after that items is not fully returned when the validation passes.

[jopicornell]

Hi @themsaid ,
What do you think about this bugfix (#23988) introduced in 5.7? It is also opinionated? It's the same I'm asking for, but for double nested validated data in an array of items (intead of item.field1, I'm asking for items.*.item.field).

As I pointed, required only should be that The field under validation must be present in the input data and not empty., anything else. It's a breaking change, but a bug in the end, because it's not doing what the required validator is supposed to do. And we're talking about required validator, because without that, it works as expected.

[themsaid]

You explicitly have a rule on the key items, and that's why items are returned. It's different from the issue you shared.

Again it's opinionated and if it was ever merged I believe it should be in 5.8 and documented as a breaking change, and I'm almost sure the other camp will report the change as a bug :)

[jopicornell]

@themsaid Let's see how it goes. I believe it should be in 5.8 too and documented as a breaking change, but I also see that's an error that should be addressed. If the field under validation is an array, an array should be considered "present and not empty" as if it is present on the request as an array and has more than 0 items. Returning all item's data is a possible security concern as it's happening to me, because a user could add any number of fields there. This can be a problem for NoSQL databases because I can add as many fields as I want without restriction.

Maybe I'm wrong, but the validators is a great feature to transform and validate the user input into a data that fits your code, not vice versa.

Thanks for your thoughts!

[donnysim]

Make an option to specify the strategy? I've never had a use case where I wanted to get the whole array instead of specific fields from it, so it's always been a pain to deal with so I'd like this to at least be an option 🙏 maybe a dummy rule to omit it from validated data?

[jopicornell]

@donnysim Yes, that's the point I wanted to say. Getting the whole array only by specifying a "required" is giving "required" on arrays too much meaning that isn't on the documentation. I see this options by now: or we have another option to work the way we're saying and change the documentation of required to specify this behaviour (so we treat this as a feature) or we change the behaviour of required for arrays and make it like I said before: Arrays with required means that they have to be present and non-empty (more than one element).

[donnysim]

@jpicornell I don't think current logic should be changed how required is treated, but add additional rule to provide flexibility when and what to grab, maybe you will want to grab the whole array, despite the content in it, but it will be marked with required. e.g.

[
    'data' => ['required', 'array', 'min:1'],
    'data.*.title' => ['required'],
    'data.*.meta' => ['required', 'array'],
]

We don't want to grab the whole data object, but we want to grab each item meta despite it's structure or rules. It could possibly be:

[
    'data' => ['required', 'array', 'min:1', 'omit'],
    'data.*.title' => ['required'],
    'data.*.meta' => ['required', 'array'],
]

Mark rules with omit to ignore/skip when collecting validated data. If we change it to work one way or the other, then as @themsaid said, it will lead to one or the other side complaining about it.

[joelharkes]

as of now neither: omit exists nor is this fixed.

This is a gigantic security risk: where the whole purpose of the FormRequest class is to validate request and filter out other values. Now these other values are not filtered out!

A solution to fix this in your local code, would be to extend the Illuminate\Validation\Validator and overwrite:

    public function validated()
    {
        if ($this->invalid()) {
            throw new ValidationException($this);
        }

        $results = [];

        $missingValue = Str::random(10);

        foreach ($this->getRules() as $key => $rules) {
            if(in_array('array', $rules)){
               continue; // skip array rules, expect to be sub rules.
            }
            $value = data_get($this->getData(), $key, $missingValue);

            if ($value !== $missingValue) {
                Arr::set($results, $key, $value);
            }
        }

        return $results;
    }

warning: this solution renders usage of free arrays impossible! (these rules will simply be ignored and not part of the validated result set).

[joelharkes]

Complete solution i'm using now:

Note: also has a perf improvement by directly filling the validated result array, instead of separately filling this later (looping again over all rules)

use Illuminate\Support\Arr;
use Illuminate\Support\MessageBag;
use Illuminate\Validation\Factory;

class ValidationFactory extends Factory
{
    /** {@inheritdoc} */
    protected function resolve(array $data, array $rules, array $messages, array $customAttributes)
    {
        if (is_null($this->resolver)) {
            return new Validator($this->translator, $data, $rules, $messages, $customAttributes);
        }

        return call_user_func($this->resolver, $this->translator, $data, $rules, $messages, $customAttributes);
    }
}

class Validator extends \Illuminate\Validation\Validator
{
    protected $result = [];

    /** {@inheritdoc} */
    public function passes(): bool
    {
        $this->messages = new MessageBag();
        $this->result = [];

        $this->distinctValues = [];

        foreach ($this->rules as $attribute => $rules) {
            $attributeArrow = str_replace('\.', '->', $attribute);
            $addToResult = true;
            foreach ($rules as $rule) {
                if ('array' === $rule) {
                    $addToResult = false;
                }
                $this->validateAttribute($attributeArrow, $rule);

                if ($this->shouldStopValidating($attributeArrow)) {
                    break;
                }
            }
            if ($addToResult) {
                $this->setResultValueIfExists($attribute);
            }
        }

        foreach ($this->after as $after) {
            call_user_func($after);
        }

        return $this->messages->isEmpty();
    }

    /**
     * Sets value of the validated request input in result.
     *
     * So result array will only contain null as value if this was also injected in the request.
     * If property was omitted, this will also be omitted in result array. (Laravel base class uses same logic).
     *
     * Warning: using this in combination with the Eloquent\Model->fill() function,
     * will lead to not updated attributes to null when they are omitted in the request.
     *
     * @param string $attribute key name of the parameter.
     */
    protected function setResultValueIfExists(string $attribute): void
    {
        // potential perf improvement by using tryGet(array, attribute, outValue): bool method.
        if(Arr::has($this->data, $attribute)){
            Arr::set($this->result, $attribute, Arr::get($this->data, $attribute));
        }
    }

    /** {@inheritdoc} */
    public function validated(): array
    {
        return $this->result;
        // return parent::validated();
    }
}

class ValidationServiceProvider extends \Illuminate\Validation\ValidationServiceProvider
{
    protected function registerValidationFactory()
    {
        $this->app->singleton('validator', function ($app) {
            $validator = new ValidationFactory($app['translator'], $app);

            // The validation presence verifier is responsible for determining the existence of
            // values in a given data collection which is typically a relational database or
            // other persistent data stores. It is used to check for "uniqueness" as well.
            if (isset($app['db'], $app['validation.presence'])) {
                $validator->setPresenceVerifier($app['validation.presence']);
            }

            return $validator;
        });
    }
}

[yaBliznyk]

Maybe we need to use just this code

$data = [
    'items' => [
        ['product' => ['id' => 1, 'name' => 'One']],
        ['product' => ['id' => 2, 'name' => 'Two']],
    ],
];

$rules = [
    // 'items' => 'required', ignore this
    'items.*.product.id' => 'required', // we mark this is required, and it's must be required
];

$validator = Validator::make($data, $rules);

$validated = $validator->validate();

dd($validated);

this code is not working for now if we try to validate $data = [];

[driesvints]

What happens if you try:

$data = [
    'items' => [
        ['product' => ['id' => 1, 'name' => 'One']],
        ['product' => ['id' => 2, 'name' => 'Two']],
    ],
];

$rules = [
    'items.*' => 'required',
    'items.*.product.id' => 'required',
];

$validator = Validator::make($data, $rules);

$validated = $validator->validate();

dd($validated);

[trevorgehman]

I ran into this issue myself with a complex form submission and nested array. I basically did something like this as a workaround:

$rules = [
    'items.0.product.id' => 'required',
    'items.*.product.id' => 'required',
];

Basically it seems that if you're using $request->validated() and are validating nested array data, you can't run any validation on a parent array or it will be included along with all its keys in $request->validated().

So something like the following will likely not give you want you expect with $request->validated():

$rules = [
  'items' => 'required|array|min:1|max:10',
  'items.*' => 'required|array',
  'items.*.product' => 'required|array',
  'items.*.product.id' => 'required|integer', // Only one you can use to avoid issue
];

The biggest issue as far as I can tell: you can't validate an array input (for example, to ensure it has a min/max number of elements) and also validate its nested data if you are relying on $request->validated().

[driesvints]

@trevorgehman but isn't that more of a feature request than an actual bug?

[donnysim]

The latest addition of exclude_if/unless removes everything that matches it so

$rules = [
    'has_appointments' => ['required', 'bool'],
    'appointments.*' => ['exclude_if:has_appointments,false', 'required'],
    'appointments.*.name' => ['required', 'string'],
];
$data = [
    'has_appointments' => false,
    'appointments' => [
        ['name' => 'should be excluded?'],
        ['name' => 'should be excluded?'],
    ],
];

removes everything that matches appointments.*, meaning appointments.0.name etc..

Now that there's a exclude_if and exclude_unless rules, seems like the plan would be to go with exclude(d) rule? for example exclude:nested for excluding nested data (though not sure if it would be needed) and exclude to exclude just this entry, ignoring nested matches (or the opposite).

I think in this case exclude_if/unless works fine with removing all nested keys as I can't think of any case where you'd use it without wanting all data removed. A solution for ignoring whole arrays but including their nested keys is still needed.

[trevorgehman]

@trevorgehman but isn't that more of a feature request than an actual bug?

@driesvints Correct, I don't think this is a bug, just unexpected behavior.

To your point, it make sense that $rules = ['stuff' => 'array']; would include the stuff array in $request->validated() since that key was validated. But I do think that arrays are a special case, so there is good reason to treat them slightly differently.

I've thought about the simplest way to add this functionality, and personally think we could change this method:

https://github.com/laravel/framework/blob/9308a4e150e43c33eaf42ce4f631fb42d9ce8643/src/Illuminate/Validation/Validator.php#L384

To something like: public function validated(bool $excludeNestedKeys = false)

Or something more generic like: public function validated(bool $strict = false) with an explanation in the docblock.

This way it wouldn't be a breaking change.

[driesvints]

@trevorgehman maybe yeah.

I've transferred this issue to the Laravel ideas repo instead.

[danpospisil]

I have ran into this right now, when I started to see some unexpected data in MySQL JSON column in my application. It didnt occur to me that requiring parent filed presence will put any unvalidated nested data into the "validated" data. This is definitely confusing and validation is useless for this purpouse. I hope there will be some way how to modify this behavior.

[bakanyaka]

I have run into this too. At least there should be a warning or something in the docs so people would not rely on validation for filtering nested array input data.

[trevorgehman]

There was a recent Twitter exchange about whether to use $guarded or $fillable. It seems that many prefer to simply pass $request->validated() to the create()/update() methods instead of relying on those properties. I think this is actually the best pattern.

With that in mind, this issue is somewhat annoying because we can't rely on the $request->validated() method to consistently return only validated data.

However, I also saw that this might be a solution: laravel/framework#32452.

[ghost]

I think this should be treated as a security vulnerability. Yes, there is fillable and guarded but that doesnt change the fact that the validator is misleading. At the 1st level it only includes the specified fields to give you a false sense of security and then at the second level comes the big gotcha.

That solution is something but its inconsistent with how the 1st level params validate and then the docs should still have a big warning next to array validation in the docs.