Crud: [4.0][Security][Proposal] In store() and update(), only save explicitly defined fields, not all request inputs

[tabacitu]

I think @eduardoarandah raised a good problem in this issue. I don't see it as a bug, since this part of the security should be handled by the model's $fillable attributes, imho. But if _we can_ make Backpack create/update forms more foolproof, I think we should.

The solution _should_ be pretty simple, but it has to be tested in all use cases, to make sure it doesn't break niche use cases, translations or fake fields. We could use the defined fields in storeCrud and updateCrud, instead of the general request->all().

So something like:

    /**
     * Store a newly created resource in the database.
     *
     * @param StoreRequest $request - type injection used for validation using Requests
     *
     * @return \Illuminate\Http\RedirectResponse
     */
    public function storeCrud(StoreRequest $request = null)
    {
        $this->crud->hasAccessOrFail('create');
        // fallback to global request instance
        if (is_null($request)) {
            $request = \Request::instance();
        }
        // replace empty values with NULL, so that it will work with MySQL strict mode on
        foreach ($request->input() as $key => $value) {
            if (empty($value) && $value !== '0') {
                $request->request->set($key, null);
            }
        }
        // insert item in the db
-        $item = $this->crud->create($request->except(['save_action', '_token', '_method']));
+        $item = $this->crud->create($request->only(collect($this->crud->getCurrentFields())->list('name'));
        $this->data['entry'] = $this->crud->entry = $item;
        // show a success message
        \Alert::success(trans('backpack::crud.insert_success'))->flash();
        // save the redirect choice for next time
        $this->setSaveAction();
        return $this->performSaveAction($item->getKey());
    }

and

    /**
     * Update the specified resource in the database.
     *
     * @param UpdateRequest $request - type injection used for validation using Requests
     *
     * @return \Illuminate\Http\RedirectResponse
     */
    public function updateCrud(UpdateRequest $request = null)
    {
        $this->crud->hasAccessOrFail('update');

        // fallback to global request instance
        if (is_null($request)) {
            $request = \Request::instance();
        }

        // replace empty values with NULL, so that it will work with MySQL strict mode on
        foreach ($request->input() as $key => $value) {
            if (empty($value) && $value !== '0') {
                $request->request->set($key, null);
            }
        }

        // update the row in the db
-        $item = $this->crud->update($request->get($this->crud->model->getKeyName()), $request->except('save_action', '_token', '_method'));
+        $item = $this->crud->update($request->get($this->crud->model->getKeyName()), $request->only(collect($this->crud->getCurrentFields())->list('name'));
        $this->data['entry'] = $this->crud->entry = $item;

        // show a success message
        \Alert::success(trans('backpack::crud.update_success'))->flash();

        // save the redirect choice for next time
        $this->setSaveAction();

        return $this->performSaveAction($item->getKey());
    }

This is more pseudocode than actual working code, but that's the gist of it.

Does anybody see a reason this would break people's apps? Are there Backpack use cases where this would be a breaking change?

[lloy0076]

It would break code that was expecting $request->all() (even though it shouldn't be).

I wonder if it should be configurable.

I wonder if it would become stupidly confusing if someone see the $fillable but then doesn't understnd why it's not being filled on save/update?

I'm also aware that the moment you make something foolproof a more foolish fool turns up 😈

[eduardoarandah]

instead of foolproof I see it more like "future proof"

Usually, systems begin as a quick-and-dirty tool, and then grow as requirements change.
More users added and permissions get complex.

Is this field fillable? I don't know, sometimes, in some modules...

We don't want backpack to be removed in the "grow" phase. That's where the money appears and licenses get sold =D

[lloy0076]

/**
 * Get the fields to save.
 *
 * @return array
 */
protected function getFieldsToSave()
{
     request()->only(collect($this->crud->getCurrentFields())->list('name')
}

/**
 * Get the fields to update.
 *
 * @return array
 */
protected function getFieldsToUpdate()
{
     request()->only(collect($this->crud->getCurrentFields())->list('name')
}

I'd live with them not being hard coded.

[pxpm]

Agree with @lloy0076, no need to hard code it.

[tabacitu]

@lloy0076 I agree abstracting this into getFieldsToSave() and getFieldsToUpdate() would make a lot of sense, if we go down this route.

BUT. I just thought of at least three cases where this change would break people's apps:

1. the checklist_dependency field type - it uses a different name than the actual fields' names;

2. the address field type - if not used as JSON - it has multiple inputs, with a bunch of names;

3. the date_range field type uses something for name but the actual input names are taken from start_name and end_name;

---

This change would not only prevent those fields from working, but any other custom fields people might have created, whose inputs have other names than those defined using addField(). So this change is starting to look more difficult than expected :-)

My current line of thinking on this:

PROs:

• it would be great to increase security by default, even for sloppy developers;

CONs:

• checklist_dependency, address and date_range fields will stop working, and possibly other custom field types; so definitely a breaking change;
• we'll require a workaround for such fields, since they are important (perhaps defining the names as an array?? or a separate parameter for them?);
• this will make developing complex fields like this a lot more difficult to create; developers might not understand why their items aren't being saved, even though their input is in the form, and the attribute is fillable;
• this could also possibly make it difficult/impossible for us to develop the matrix field, since it relies on the same principle;

I don't know... Thoughts? How do you weight these pros and cons?

[eduardoarandah]

Well, If a tool hides complexity, must be responsible for it.

If backpack has a complex field type, then backpack must know how to validate and save it.

[MarcosBL]

At first look: a -1; almost all my app uses custom views to account for special cases I can't cover with default Backpack fields, but I can easily solve with a custom view, injecting special inputs/etc. In the "before create/update" of Backpack I just procees those and delete them from the request prior to pass everything to the parent, so this change could be safe for me, but not for others expecting to use it in the model beforeSave(), for example, so I really think it would lead into a huge breaking change.

Can't think right now of more use cases where this would lead into a problems, but forced validation of N fields, even if automatic, sounds just wrong in my head at first sight.

[tabacitu]

I see no reason to do this, since it would require us to write custom save logic for a bunch of fields, and force some users to find other solutions to their custom fields. Huge breaking change with little benefit, since developers should _definitely_ be using $fillable.

Let's table the issue for now. Reopen/comment if you think I'm wrong. @eduardoarandah definitely thinks I am :-)

[tabacitu]

@MajedDH has surfaced a use case where except() instead of only() could be a serious vulnerability. Say in the Create/Update operations, the developer shows:

• "name" and "email" fields for admins
• "name" for users/moderators/whatever

In this case, the user would be able to modify the email by modifying the POST request. $fillable would not be a good gatekeeper.

I'm wondering, though:

• If you have a slightly different form for User and Admin, wouldn't you have separate FormRequest files? Wouldn't the UserFormRequest check that the User is authorized to modify the email? In this case the FormRequest would be a great gatekeeper, I think.

@MajedDH, @pxpm , @OliverZiegler , @tswonke , what do you think?

---

Solution 1

We could make this customizable like @lloy0076 proposed here. Developers could choose between only() and except(). But:

• if we default to except(), there will be no "security by default"; so... problem not solved;
• some complex fields like date_range, checklist_dependency and many custom fields will break; and have no way of working if you _choose_ to use only(); so they still need an alternative solution;

So I don't think having some developers use only() and other developers using except() is a perfect solution.

Solution 2

Maybe we add allow name to be an array, or provide an array alternative to name. Say... names (plural). _Some_ complex field types could use that to explicitly declare that they have multiple inputs, and want them all to get saved.

Something like:

        [
-            'name' => 'event_date_range', // a unique name for this field
-            'start_name' => 'start_date', // the db column that holds the start_date
-            'end_name' => 'end_date', // the db column that holds the end_date
+           'names' => ['start_date', 'end_date'], // names of the db columns
            'label' => 'Event Date Range',
            'type' => 'date_range',
            // OPTIONALS
            'start_default' => '1991-03-28 01:01', // default value for start_date
            'end_default' => '1991-04-05 02:00', // default value for end_date
            'date_range_options' => [ // options sent to daterangepicker.js
                'timePicker' => true,
                'locale' => ['format' => 'DD/MM/YYYY HH:mm']
            ],
        ]

Thoughts anyone? Do you think it's a real issue we should be solving by default OR let the developer be sloppy if he wants to? Do you see any way this second solution would be a bad one? Any better solutions?

[tabacitu]

Done! PR https://github.com/Laravel-Backpack/CRUD/pull/2045 fixes this using the solution 2 mentioned above: name can now also be an array, for fields that hold multiple inputs, so that users can specify all input names when adding the field.