Failed form request is allowed to proceed if required is not the first rule

[boneill81]

• Laravel Version: 5.7.19
• PHP Version: 7.1.4

Description:

I had noted that a few records managed to reach the DB which should not have passed validation. In my form request I have the following withValidator function.

    public function withValidator($validator)
    {
        //All rules have passed, convert our spatial reference to multiple formats and merge to the request for persisting.
        if (!$validator->fails()) {
            $this->merge(calculateSpatialReferences($this->spatial_reference));
        }
    }

However I noted that in very rare instances calculateSpatialReferences() was never run but the request still proceeded to the persistence layer in my model which should not have been possible.

After much investigation I believe I have found the issue. One of my validation rules was configured as follows: date_format:H:i|required

For this particular request, the parameter in question was not submitted with the request.

When run using this rule, I can see the required failure from $validator->errors() but the request proceeds to the persistence layer even though it should fail based on the presence of the failed rule.

But if you change the rule to required|date_format:H:i as would be the normal case, the request fails correctly and does not proceed.

This I presume is definitely not intended functionality since it allows a FormRequest to proceed even though it has failed validation.

I appreciate any input you might have on it. Thanks.

[staudenmeir]

Please provide a complete reproducible example.

[driesvints]

Hey there,

Unfortunately we don't support this version anymore. Please check out our support policy on which versions we are currently supporting. Can you please try to upgrade to the latest version and see if your problem persists? If so feel free to reply and we'll try to have a look and re-open this issue.

Thanks!

[boneill81]

@driesvints Thanks very much for getting back to me. I updated to 5.8.16 and can verify the issue remains. It's easy to reproduce. Just define a rule but as described below and place required second.

The rule for example : string|required|max:255

    public function withValidator($validator)
    {

        if ($validator->fails()) {
            Log::info($validator->errors());
        }
        else
        {
            Log::info('Passed');
        }

    }

With required second "Passed" will not be output and the errors array is visible but the FormRequest still succeeds. If you change the rule to required|string|max:255, it works as intended and the FormRequest fails as expected.

[driesvints]

I tried to replicate this:

    public function rules()
    {
        return [
            'foo' => 'date_format:H:i|required|max:255',
        ];
    }

    public function withValidator($validator)
    {
        if ($validator->fails()) {
            die(var_dump($validator->errors()));
        } else {
            die('Passed');
        }
    }

$  curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":"14:01"}'
Passed%                                                                                                                                                                                                                                                                                    $  curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":"99:01"}'
object(Illuminate\Support\MessageBag)#272 (2) {
  ["messages":protected]=>
  array(1) {
    ["foo"]=>
    array(1) {
      [0]=>
      string(38) "The foo does not match the format H:i."
    }
  }
  ["format":protected]=>
  string(8) ":message"
}
$ curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":""}'     
object(Illuminate\Support\MessageBag)#272 (2) {
  ["messages":protected]=>
  array(1) {
    ["foo"]=>
    array(2) {
      [0]=>
      string(38) "The foo does not match the format H:i."
      [1]=>
      string(26) "The foo field is required."
    }
  }
  ["format":protected]=>
  string(8) ":message"
}

So this seems to work as expected.

[boneill81]

In my test the foo was not sent with the request. I wonder if you send the request without foo will it match what I am seeing. In my case when foo does not exist the errors array contains only the required message not the formatting message and the formrequest succeeds. Perhaps submitting without foo will match what I am seeing. Thanks for your help.

…

On Fri 10 May 2019, 18:12 Dries Vints, ***@***.***> wrote: I tried to replicate this: public function rules() { return [ 'foo' => 'date_format:H:i|required|max:255', ]; } public function withValidator($validator) { if ($validator->fails()) { die(var_dump($validator->errors())); } else { die('Passed'); } } $ curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":"14:01"}' Passed% $ curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":"99:01"}' object(Illuminate\Support\MessageBag)#272 (2) { ["messages":protected]=> array(1) { ["foo"]=> array(1) { [0]=> string(38) "The foo does not match the format H:i." } } ["format":protected]=> string(8) ":message" } $ curl -X POST http://test-app.test -H 'Content-Type: application/json' -d '{"foo":""}' object(Illuminate\Support\MessageBag)#272 (2) { ["messages":protected]=> array(1) { ["foo"]=> array(2) { [0]=> string(38) "The foo does not match the format H:i." [1]=> string(26) "The foo field is required." } } ["format":protected]=> string(8) ":message" } So this seems to work as expected. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#28480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5FFV3DHDQRHUTXKGPNYKDPUWUIRANCNFSM4HMBSKHA> .

[driesvints]

@boneill81 I get this:

$ curl -X POST http://test-app.test -H 'Content-Type: application/json'                
object(Illuminate\Support\MessageBag)#272 (2) {
  ["messages":protected]=>
  array(1) {
    ["foo"]=>
    array(1) {
      [0]=>
      string(26) "The foo field is required."
    }
  }
  ["format":protected]=>
  string(8) ":message"
}

Which is also expected. No key was provided so the date_format rule isn't executed.

[boneill81]

Ok great now we are seeing the same error message. Going back to my first post today now, the issue now is that the controller method to which that formrequest is injected will still be executed when the validation rule is configured with required as the second rule. In my tests an echo message is executed in the controller even though the formrequest has failed. But if required is set as the first rule then the formrequest fails and the content of the controller is never executed as you would expect. I'm replying from phone now. I can post a demo in more detail if needed later.

…

On Fri 10 May 2019, 18:30 Dries Vints, ***@***.***> wrote: @boneill81 <https://github.com/boneill81> I get this: $ curl -X POST http://test-app.test -H 'Content-Type: application/json' object(Illuminate\Support\MessageBag)#272 (2) { ["messages":protected]=> array(1) { ["foo"]=> array(1) { [0]=> string(26) "The foo field is required." } } ["format":protected]=> string(8) ":message" } Which is also expected. No value was provided so the date_format rule isn't executed. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5FFV6BZ5QTDB3WASJNAJLPUWWLNANCNFSM4HMBSKHA> .

[staudenmeir]

I can reproduce it.

@driesvints Try this:

class TestController extends Controller
{
    public function index(TestRequest $request)
    {
        dd('Validation failed, but execution continues.');
    }
}

[driesvints]

@staudenmeir still can't reproduce it. For me the validation fails for the request and I get a redirect back. Can any of you two perhaps upload a test app that reproduces this? Please try to commit all the changes necessary to replicate the bug as a separate commit.

[boneill81]

Sure I can do that later or if @staudenmeir gets to it in the meantime no problem. I'm happy that it can be seen by someone else. I gave a good few hours trying to ascertain why my app was behaving in this manner. I'll get back to you both later.

…

On Fri 10 May 2019, 19:27 Dries Vints, ***@***.***> wrote: @staudenmeir <https://github.com/staudenmeir> still can't reproduce it. For me the validation fails for the request and I get a redirect back. Can any of you two perhaps upload a test app that reproduces this? Please try to commit all the changes necessary to replicate the bug as a separate commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5FFV2PRLEAXH7M3MT3XDLPUW5BHANCNFSM4HMBSKHA> .

[staudenmeir]

https://github.com/staudenmeir/bug

[driesvints]

@staudenmeir you're doing a get request there. Form request validation doesn't works with GET requests because no form data is passed.

[devcircus]

The 'withValidator' method is causing the problem. Remove it and see the endless redirect loop as expected. I thought the purpose of 'withValidator' was to add 'after' hooks to the validator. If you do that, then this 'bug' doesn't exist.

Not sure if the docs should just clarify this or if this needs to be fixed. The docs do say that "any" of the validator methods can be called in this method.

[staudenmeir]

@driesvints I can also reproduce this with a POST request.

[boneill81]

Yes indeed, same here. The route connected to the controller function on my app accepts only post requests.

…

On Fri 10 May 2019, 20:46 Jonas Staudenmeir, ***@***.***> wrote: @driesvints <https://github.com/driesvints> I can also reproduce this with a POST request. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#28480 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AD5FFV5TJZEXGXKRD2LXVNTPUXGHTANCNFSM4HMBSKHA> .

[staudenmeir]

This is a fundamental issue:

$v = \Validator::make([], ['foo' => 'required|string']);
dump($v->fails()); // true
dump($v->fails()); // true

$v = \Validator::make([], ['foo' => 'string|required']);
dump($v->fails()); // true
dump($v->fails()); // false

[boneill81]

I can also confirm 100%. I just threw together an identical test app as @staudenmeir and ran the following:

curl -X POST http://test.test/api/foo -H 'Content-Type: application/json'
curl -X POST http://test.test/api/foo -H 'Content-Type: application/json' -d '{"foo":""}'

Route::post('/foo', function (TestRequest $request) {
    dd('Validation failed, but execution continues.');
});

The responses for the two rules were identical and as follows:

'foo' => 'string|required|max:255' : Validation failed, but execution continues.
Errors: {"foo":["The foo field is required."]}

'foo' => 'required|string|max:255'** : Request fails and requests content is correctly not executed.
Errors: {"foo":["The foo field is required."]}

The errors array is populated for each request but as mentioned, the FormRequest still proceeds even though validation failed for the first request.

This seems a serious issue even though the mitigation is simple of course i.e. making sure the required rule is first but the same rules should still be executed in an identical manner.

I'm just happy that I know for sure now that something was up, I was driving myself crazy debugging last night and today hunting for the reason some records reached the DB without a spatial reference.

Can this be opened again now that we have ascertained something is not right? Thanks.

[driesvints]

Re-opening. Thanks for investigating you two. What would be the best approach to fix this?

[boneill81]

I'm looking into this but I believe I see where the problem lies. If you look at the passes() function in https://github.com/laravel/framework/blob/5.8/src/Illuminate/Validation/Validator.php#L264

The determination of whether validation passes depends on the result of $this->messages->isEmpty();

I added a simple log output as below

    /**
     * Determine if the data passes the validation rules.
     *
     * @return bool
     */
    public function passes()
    {
        $this->messages = new MessageBag;

        $this->distinctValues = [];

        // We'll spin through each rule, validating the attributes attached to that
        // rule. Any error messages will be added to the containers with each of
        // the other error messages, returning true if we don't have messages.
        foreach ($this->rules as $attribute => $rules) {
            $attribute = str_replace('\.', '->', $attribute);

            foreach ($rules as $rule) {
                $this->validateAttribute($attribute, $rule);

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

        // Here we will spin through all of the "after" hooks on this validator and
        // fire them off. This gives the callbacks a chance to perform all kinds
        // of other validation that needs to get wrapped up in this operation.
        foreach ($this->after as $after) {
            call_user_func($after);
        }

        Log::info('messages:' . $this->messages);
        Log::info('messages empty?:' . $this->messages->isEmpty());

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

When the rule is 'required|string|max:255' the output is:

[2019-05-12 21:18:41] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 21:18:41] local.INFO: messages empty?:  
[2019-05-12 21:18:41] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 21:18:41] local.INFO: messages empty?:  

However when the rule is 'string|required|max:255' the output is:

[2019-05-12 21:13:16] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 21:13:16] local.INFO: messages empty?:  
[2019-05-12 21:13:16] local.INFO: messages:[]  
[2019-05-12 21:13:16] local.INFO: messages empty?:1  

Before reporting this I had investigated if this could be a result of the double execution bug as documented here: #26731

I'm wondering now if this is in fact the case. From the above you can see the issue clearly, the rule is being triggered twice it would seem, the first time around it is actually correct but the second time around $this->messages returns empty and which leads to passes() returning True.

I'll keep digging into this.

[staudenmeir]

The issue is caused by Validator::$failedRules not being reset. I'll submit a PR.

[boneill81]

@staudenmeir Brilliant, great that we know whats amiss now. I was doing some further investigation before I noticed your fix and for the record I added two more log calls to passes(). See below for results.

public function passes()
    {
        $this->messages = new MessageBag;

        $this->distinctValues = [];

        // We'll spin through each rule, validating the attributes attached to that
        // rule. Any error messages will be added to the containers with each of
        // the other error messages, returning true if we don't have messages.
        foreach ($this->rules as $attribute => $rules) {
            Log::info('attribute:' . $attribute);
            $attribute = str_replace('\.', '->', $attribute);

            foreach ($rules as $rule) {
                Log::info('rule:' . $rule);
                $this->validateAttribute($attribute, $rule);

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

        // Here we will spin through all of the "after" hooks on this validator and
        // fire them off. This gives the callbacks a chance to perform all kinds
        // of other validation that needs to get wrapped up in this operation.
        foreach ($this->after as $after) {
            call_user_func($after);
        }
        Log::info('messages:' . $this->messages);
        Log::info('messages empty?:' . var_dump($this->messages->isEmpty()));
        return $this->messages->isEmpty();
    }

When the rule is 'required|string|max:255' the output is as expected:

[2019-05-12 23:25:09] local.INFO: attribute:foo  
[2019-05-12 23:25:09] local.INFO: rule:required  
[2019-05-12 23:25:09] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 23:25:09] local.INFO: messages empty?:  
[2019-05-12 23:25:09] local.INFO: attribute:foo  
[2019-05-12 23:25:09] local.INFO: rule:required  
[2019-05-12 23:25:09] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 23:25:09] local.INFO: messages empty?: 

But when the rule is 'string|required|max:255' the output is:

[2019-05-12 23:30:06] local.INFO: attribute:foo  
[2019-05-12 23:30:06] local.INFO: rule:string  
[2019-05-12 23:30:06] local.INFO: rule:required  
[2019-05-12 23:30:06] local.INFO: messages:{"foo":["The foo field is required."]}  
[2019-05-12 23:30:06] local.INFO: messages empty?:  
[2019-05-12 23:30:06] local.INFO: attribute:foo  
[2019-05-12 23:30:06] local.INFO: rule:string  
[2019-05-12 23:30:06] local.INFO: messages:[]  
[2019-05-12 23:30:06] local.INFO: messages empty?:  

You can see on the first run both string and required are evaluated but on the second run required is never evaluated.

[boneill81]

I tested the PR with my production app this morning and its working perfect. Thanks guys for the assistance.
All the best!!