Unexpected argument exception within login process

[webmake]

• Laravel Version: 5.2.45 till 5.4.17
• PHP Version: 7.1

Description:

[2017-04-06 20:52:09] local.ERROR: ErrorException: mb_strtolower() expects parameter 1 to be string, array given in /www/vendor/laravel/framework/src/Illuminate/Support/Str.php:182
Stack trace:
#0 [internal function]: Illuminate\Foundation\Bootstrap\HandleExceptions->handleError(2, 'mb_strtolower()...', '/www/...', 182, Array)
#1 /www/vendor/laravel/framework/src/Illuminate/Support/Str.php(182): mb_strtolower(Array, 'UTF-8')
#2 /www/vendor/laravel/framework/src/Illuminate/Foundation/Auth/ThrottlesLogins.php(92): Illuminate\Support\Str::lower(Array)
#3 /www/vendor/laravel/framework/src/Illuminate/Foundation/Auth/ThrottlesLogins.php(22): App\Http\Controllers\LoginController->throttleKey(Object(Illuminate\Http\Request))
#4 /www/vendor/laravel/framework/src/Illuminate/Foundation/Auth/AuthenticatesUsers.php(35): App\Http\Controllers\LoginController->hasTooManyLoginAttempts(Object(Illuminate\Http\Request))
#5 [internal function]: App\Http\Controllers\LoginController->login(Object(Illuminate\Http\Request))
... 

due security issues cannot provide full stack trace.

Steps To Reproduce:

Include in any controller AuthenticatesUsers trait, and provide such values:

       $this->post('/admin', [
            'email' => [$user->email],
            'password' => $password
        ]);

To sum up, AuthenticatesUsers has method login, which calls ThrottlesLogins trait to get the key throttleKey, but username method extracts 'email' field, which was array, so lower gives exception.

How to filter properly before the trait?

[webmake]

As temporary solution found:

    protected function validateLogin(Request $request)
    {
        $this->validate($request, [
            $this->username() => 'required|string',
            'password' => 'required|string',
        ]);
    }

by adding string validations everywhere. I don't know is it valid solution, maybe this patch should be included in core?

[ntzm]

If this is an actual security issue (which I'm not sure it is), you should email Taylor, not post it publicly

[webmake]

This is not so critical, because you cannot bypass login and authorise, but if logs are turned on, and stack trace is visible only

[fernandobandeira]

You can submit a PR for that if you don't mind,, if you forget / don't want to submit one then I'll submit one for the fix... What you did here should be good to go, also we can close this and discuss it there...

[jerguslejko]

why do you pass the email field wrapped in an array?

'email' => [$user->email],

should be

'email' => $user->email,

[ntzm]

@jerguslejko they're saying if the user passed that value, like ?email[]=test@test.com&password=xxx

[webmake]

@jerguslejko, I follow simple rule: "Don't trust user input", they shown for me that this part is not bullet proof as you can see.

[laurencei]

@webmake - you need to use rules like string or array on your rules as needed. That will ensure it is bullet proof.

[webmake]

@laurencei do you understand that is core laravel function? Why I should overwrite and fix laravel? I made PR that everybody would take this patch :) Thanks for the merge

[laurencei]

But there are times you want an array as input. So you cant say there should only ever be string input on all input across all requests.

I made a PR to help include string validation for login as part of a default application to help people for future projects - which has been merged.

[webmake]

If you are talking about this laravel/laravel#4212 as I understand it fixes only registration, but login is fixed by this https://github.com/webmake/framework/pull/1/files or not? o.O Maybe I didn't find proper place to validate, but by default there is no array in forms, so might arrise error as in my case

[laurencei]

@webmake - yes - that would seem like a good idea for a PR. You should submit it for consideration for Taylor and see what he thinks IMO.

[webmake]

Seems I didn't make proper PR :D this was standing in my repo and invisible for others...

[devcircus]

The methods in these traits are simply starter boilerplate code. The intent is for you to use them if they match your specific use-case and if not, override them in your controller. Personally, I would get more mileage from these methods if they were left untouched. With these added validations, I'll need to override more often.

[webmake]

I'd say that most people take this functionality and suppose that it is working out of box, but actually now validators are too weak. Move over, if people overwrite, it won't break anything. What was your case that you overwrote @devcircus ? Do you really have arrays?

[themsaid]

Closing since the PR was merged: #18746

Thanks :)