[Form][Validation] Add forbid_extra_data option
#7229
Conversation
| @@ -85,6 +85,7 @@ public function setDefaultOptions(OptionsResolverInterface $resolver) | |||
| 'cascade_validation' => false, | |||
| 'invalid_message' => 'This value is not valid.', | |||
| 'invalid_message_parameters' => array(), | |||
| 'ignore_extra_data' => false, | |||
There was a problem hiding this comment.
This is misleading IMO. The extra data is always ignored when binding the form (it cannot be bound as there is no field for them). Thus option controls whether it is allowed to send extra data, not whether the form ignores it.
There was a problem hiding this comment.
I agree. How about allow_send_extra_data?
There was a problem hiding this comment.
I still prefer the name Marco suggested. Technically the form doesn't ignore metadata if it would ignore it then it wouldn't throw an exception.
There was a problem hiding this comment.
I think allow_send_extra_data is more clear, but is too verbose (~20% longer).
$factory->create('form.name', new Model(), ['allow_send_extra_data' => true]);VS
$factory->create('form.name', new Model(), ['ignore_extra_data' => true]);There was a problem hiding this comment.
What about allow_extra_data which would default to false. However I still prefer ignore...
There was a problem hiding this comment.
@mvrhov I had thought in this name, but in fact we're not allowing extra data (it does not have any usefulness), we're signaling whether the form should add a violation or not. So, ignore_extra_data brings me to the real goal of this option (if the extra data is ignored, then it must be ignored during validation).
There was a problem hiding this comment.
@marcospassos But the name is confusing IMO, because you don't know that this option is related to the validation when you see the configuration of a form.
There was a problem hiding this comment.
How about extra_data_violation (default true)?
There was a problem hiding this comment.
Another solution could be forbid_extra_data (defaulting to true)
There was a problem hiding this comment.
@stof Sounds good for me! 👍
|
a big 👍 for this one as this will also greatly simply bounding the form from query parameters. |
Yep but there is also a big implication on security (see what happened to Ruby / GH a few months ago) |
|
@vicb This PR does not introduce a security issue for the extra params AFAIK. These extra params won't be bound to the underlying object. They will simply be ignored silently instead of marking the form as invalid. |
|
Really? Can you give me an example? As far as I know, the issue was something like this: id = 123 #(target's project id)
project['name'] = 'Title'
project['project_id'] = 321 #(hacker id)
# After execution that POST we got hacker owning target's project.
Project.find(params[:id]).update_attributes(params[:project]) You can't reproduce this kind of attack in SF2. |
|
Like stof said. The issue in ruby was different. The form automatically bind everything that matched. The Form component in symfony requires a form field with correct property path for something to be bound. |
|
Renamed. |
|
You are right, I was wrong. Christophe Coevoet notifications@github.com wrote:
|
|
ping @fabpot |
|
ref #1341 |
|
Thank you for the PR @marcospassos! We already discussed this issue a lot in #5576 and came to a conclusion about the API, but no-one implemented it yet. Do you have time to adapt your implementation? |
|
@bschussek Let me see if I've got. Instead of setting as option at form creation, it should be passed as second argument of |
|
Exactly :) The implementation in #5576 was already quite good, just the API was wrong. |
Fixed indentation.
Added
ignore_extra_dataoption to allow form validation pass, even with extra data.