i see the fill method is used a lot with Input::all().
i think people appreciate it more to have the burden taken by Eloquent that just silently takes what it needs instead of having to write extra checking/setting lines that probably need change everytime the form changed - don't you think?
The guarded attributes prevent those table fields being changed with mass assignment only. If you define a specific array (which is not Input::all()), than they will get set.
As Zwacky said, using ::create(Input::all()) is awesomely convenient, but also dangerous.. That's why the guarded attribute exists.
If what you actually want is a completely unwritable field, than maybe you could try playing with different MySql users and granting them different access on specific columns, or even better, use model events (http://laravel.com/docs/eloquent#model-events)!
I think if you are doing it correctly it's really more of a convenience mechanism than anything (allowing you to specify 'safe' or 'unsafe' fields then quickly do a User::create(Input::all()) rather than manually specify all the fields).
If you are validating your data correctly and using filters to protect against CSRF attacks then I don't think you have too much to worry about. Maybe create your administrative log/report when you detect a CSRF attack?
Other than that you can create a Service class to grab the model fillable / guarded keys and check them against the input keys manually. It's a little more work, but you can write a small class once to handle this throughout your app.
Mass assignment vulnerability is different from csrf.. Imagine someone editing their profile.. they could just open dev tools, insert a new hidden field named "admin", which is often a valid field in "users" table and set it to true. If the dev uses Input:all() without the guarded attribute, the user will make himself admin of the system. The request will pass, cause all data will be valid, including the csrf token. That's the whole point of the guarded attribute.
To protect fields try using this snippet in your models. I didn't test it, but you'll get the idea..
Does this mean that using
Post::create(Input::get('all')) is dangerous even if you mass assignment and gaurded is set correctly?
Or is there a a way to check for a "role" per field being updated?
For example the admin field can only be set by a user who is an admin.
Would guarded be used for that?
I think Laravel will even throw an error if you try using mass assignment without setting the guarded attribute.
Maybe a better approuch woud be to whitelist, instead of blacklist. So you could set the "fillable" attribute to whatever fields you want to let your users edit. And if you want to update the rest, you define all the exact fields and check roles, of course..
protected $fillable = array('name','email','password','birthdate');
Thank you all, but apparently my point was unclear. I'll try with an example.
Let's say we have a User model with a guarded "id" attribute. The form for creating a new user doesn't (and of course mustn't) provide any "id" field. The form uses a CSRF token. Authentication is required to access the form and we took every possible precaution.
Nevertheless, at some point the server receives a request to the form action and, inexplicably, the request data contains an "id" parameter. The parameter gets ignored, but the action is still performed with User::create(Input::all()).
Now, wasn't that request suspicious enough to deserve a rejection?
If, in order to detect security threats, I have to validate the input also against the presence of guarded attributes, then what's the point of having a fillable/guarded attribute set? Hope the question is clearer now.
I think Input::all() is a bad bad idea in every shape or form, second, write an model observer that throws an exception when trying to save it like Elena wrote.
Laravel might do stuff you don't like but it allows you to change/add it without getting in your way
@zenry: The final question remains. If I have to do those checks on my own, whatever way, then the predefined behaviour of the fillable/guarded system is somewhat useless imho. Input::all() isn't necessarily a bad idea, as long as the system is well designed.
popolla said:
@zenry: The final question remains. If I have to do those checks on my own, whatever way, then the predefined behaviour of the fillable/guarded system is somewhat useless imho. Input::all() isn't necessarily a bad idea, as long as the system is good designed.
Input::all() is only useful for development, other use of it means the programmer is lazy*
fillable/guarded is useful and does what it needs to do, yes, some people would like an exception to be thrown, others like the it the way it is. I highly dislike Input::all() but that doesn't mean I think it's useless, yes for my purposes it is useless but for other (lazy) people it just works ;)
@zenry: First of all, thank you for your time. I'm not saying fillable/guarded is useless in an absolute way. When the application is well designed, we can say that bad requests come from attackers. Therefore, if the fillable/guarded system detects a bad request, the same system should really reject it. If I have to reject it by myself, than I don't need such a system. Of course, that's an opinion. Bye
mass assignment, is good wen you have lots of inputs, and it saves code-lines.
zenry said:
popolla said:
@zenry: The final question remains. If I have to do those checks on my own, whatever way, then the predefined behaviour of the fillable/guarded system is somewhat useless imho. Input::all() isn't necessarily a bad idea, as long as the system is good designed.
Input::all() is only useful for development, other use of it means the programmer is lazy*
fillable/guarded is useful and does what it needs to do, yes, some people would like an exception to be thrown, others like the it the way it is. I highly dislike Input::all() but that doesn't mean I think it's useless, yes for my purposes it is useless but for other (lazy) people it just works ;)
- in my most humble opinion
Here's the fill method: http://laravel.com/api/source-class-Illuminate.Database.Eloquent.Model.html#272-298
So, I guess if you override this method in your model files, you could use it anyway you want...
Also, maybe you should open an issue and/or submit a pull request, cause.. you're right, some kind of notification that something suspicious is going on would be very welcome in those cases..
elena-kolevska said:
Here's the fill method: http://laravel.com/api/source-class-Illuminate.Database.Eloquent.Model.html#272-298
So, I guess if you override this method in your model files, you could use it anyway you want...
Also, maybe you should open an issue and/or submit a pull request, cause.. you're right, some kind of notification that something suspicious is going on would be very welcome in those cases..
Thank you very much Elena, so glad to know that I'm not the only one out there! Yes, I think that overriding the fill method would likely be the best solution for now, and a pull request should be the very next step.
Sign in to participate in this thread!
The Laravel portal for problem solving, knowledge sharing and community building.
The community