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:
This form is submitted to this Controller method:
public function store(StoreUserRequest $request) { User::create($request->all()); return redirect()->route('dashboard');}
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.
class User extends Authenticatable{ protected $fillable = [ 'name', 'email', 'password', 'is_admin', ];
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?
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:
class StoreUserRequest extends FormRequest{ public function rules() { return [ 'name' => ['required', 'string', 'max:255'], 'email' => ['required', 'email', 'string', 'max:255'], 'password' => ['nullable', 'string', 'confirmed', 'min:8'], ]; }}
Then, in the Controller, you should use the request filtered after that validation. For that, just replace $request->all()
with $request->validated()
.
public function store(StoreUserRequest $request) { User::create($request->validated()); return redirect()->route('dashboard');}
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()
:
public function store(StoreUserRequest $request) { User::create($request->only('name', 'email', 'password')); return redirect()->route('dashboard');}
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.
public function store(StoreUserRequest $request) { User::create([ 'name' => $request->name, 'email' => $request->email, 'password' => $request->password, ]); return redirect()->route('dashboard');}
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.
Very enlightening and informative article. Simple to understand and very practical
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.
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.
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;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.
thanks for sharing...
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?$request->all is used most of the controllers when we genreate via Quickadmin? do I need to change the best method?
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()
.Great explanation ...