Don't Use $request->all(): It's Insecure

Quite often, I see Laravel developers using $request->all() in Controller methods. It may be a security issue, let me show you why.


Why $request->all() is insecure

Let's imagine you have a regular registration form:

Laravel registration form

This form is submitted to this Controller method:

1public function store(StoreUserRequest $request) {
2 User::create($request->all());
3 
4 return redirect()->route('dashboard');
5}

We use a Form Request class with validation, so it doesn't look harmful, does it? It should save name, email, and password, right?

Notice: I know that the password should be encrypted, but in this article, let's assume the encryption is done somewhere else, like in Observer or Mutator.

Now, let's take a look at the $fillable array in the User model.

1class User extends Authenticatable
2{
3 protected $fillable = [
4 'name',
5 'email',
6 'password',
7 'is_admin',
8 ];

See that is_admin column? It is used to assign the administrator role, and that field should be filled only by other administrators, in some other form than the registration, in a separate admin panel.

But what if I try to call that registration to submit by adding a hidden field called is_admin, directly from my browser, like Chrome dev tools, clicking Inspect?

Laravel registration inspect

Laravel registration hidden field

Guess what: the is_admin will be successfully saved, and I will successfully register myself as an administrator, without anyone's permission.

So, to "hack" the system, all I would need is to guess the non-visual database fields: it may be called is_admin, it may be role_id, just role, or whatever else. Not that hard to write a script to automate trying all the possible options.

This is happening because $request->all() doesn't filter or validate anything, it's just literally all().

So, what to do instead?


Option 1. Form Request and validated()

If you use the Form Request class for the validation, you have the rules() method there:

1class StoreUserRequest extends FormRequest
2{
3 public function rules()
4 {
5 return [
6 'name' => ['required', 'string', 'max:255'],
7 'email' => ['required', 'email', 'string', 'max:255'],
8 'password' => ['nullable', 'string', 'confirmed', 'min:8'],
9 ];
10 }
11}

Then, in the Controller, you should use the request filtered after that validation. For that, just replace $request->all() with $request->validated().

1public function store(StoreUserRequest $request) {
2 User::create($request->validated());
3 
4 return redirect()->route('dashboard');
5}

So, it will fill in only the fields that are present in the rules() method.

Keep in mind, that in this case, you need to add all the needed fields into the rules() array, even if it doesn't require a specific validation, just add them as nullable or string.


Option 2. $request->only()

Of course, another option is to specify the exact fields to be used. One of the options is to use $request->only():

1public function store(StoreUserRequest $request) {
2 User::create($request->only('name', 'email', 'password'));
3 
4 return redirect()->route('dashboard');
5}

Option 3. Field by Field

Finally, the good old way of specifying field by field. The code looks longer, but maybe more readable, with less "hidden" information.

1public function store(StoreUserRequest $request) {
2 User::create([
3 'name' => $request->name,
4 'email' => $request->email,
5 'password' => $request->password,
6 ]);
7 
8 return redirect()->route('dashboard');
9}

All three options above are valid, it's just your personal preference, which may also depend on the exact form or situation. The main thing is not to use $request->all(), just forget about that method's existence, for your safety and security.

avatar

Very enlightening and informative article. Simple to understand and very practical

avatar

title is misleading - it's not insecure as on it's own it doesn't have any bugs/loopholes. it's the way users are putting it into practice that makes it "insecure"., and btw, your comments textarea is making a request for each word typed ? filled 130+ post requests while typing this.

avatar

You're technically correct, but I specifically made that title "screaming" so it would call people to actually read, as the topic is very important.

For comments, I use laravel-comments.com which is powered by Livewire, so you're probably right about the requests, but I don't mind, at least for now, I don't get many comments.

avatar

Thanks a lot for sharing this.

I really do think that the key thing here is "moderation" and using with "caution".

I use $request->all() a lot, especially because it has a way of cleaning things up pretty well.

Consider a scenario where there are so many possible fields that can be updated for the user, some of which I don't even care to validate. Spelling them all out one by one feels like so much pain.

If I have sensitive fields, then there are a couple ways to close loose ends. For example, in the form request, I could take advantage of the authorize method;

public function authorize()
{
    if ($this->has('is_admin') && $this->user()->isNotSuperAdmin()) {
		    return false;
		}
		
		return true;
}

This is especially useful when dealing with updating the model. Since there are different fields that can be updated, some of which are available in some requests, and sometimes there are not.

You just have a PUT api endpoint that handles updating any field on the user, and you only have to authorize what gets updated, and not have to hardcode or list them all out one by one.

Another way might be to use the $guarded property on the model.

In my opinion, I don't want to be bothered about what fields are sent. I just want to do a $this->user->update($this->all()) or maybe $this->user->update($this->except('some_field')).

You just need to pay attention to what is being used and where. You know your application best.

avatar

thanks for sharing...

avatar

How about the most correct option IMO (not saying shouldn't be followed by the others) which would be removing the is_admin from mass assignment?

avatar
Loganathan Natarajan

$request->all is used most of the controllers when we genreate via Quickadmin? do I need to change the best method?

avatar

That is tricky. I have to admit that yes, we use it in QuickAdminPanel, but we had no other choice as we cannot fully guarantee what fields would be in validated() and that there will be no bugs. So we've chosen this less-secure way in favor of just broken adminpanels for customers.

But yes, if you have time to go over forms manually, it would be safe to change it to validated().

Like our articles?

Become a Premium Member for $129/year or $29/month

Written by