No Method Name Validation or Whitelist

[jpnut]

At the moment, no validation or normalisation runs in the filterInput function. i.e. any key present in the input array will call a function with that name. This means any method can be called arbitrarily. For example, using the implementation as discussed in the readme, if one defines any (public or protected) function (e.g. superSecretMethod), then this could be called by visiting the appropriate endpoint with 'super_secret_method' as a query argument.

This is unlikely to cause a major security issue, since no actions should really be defined within a Filter, and the output of the function is never returned to the user, however I don't think arbitrary method calls are a good idea.

Some possible solutions:

• create a whitelist of possible filters (i.e. require a parameter or function to be defined which specifies all the available filters)
• come up with a common naming scheme for filters (e.g. get{FILTER_NAME}Filter, à la getFooAttribute)

https://github.com/Tucker-Eric/EloquentFilter/blob/3a0ebf73cf436b18078d230406eb1c9b6e0ef496/src/ModelFilter.php#L185-L198

[Tucker-Eric]

You bring up a good point.

I'm not sure how I feel about a whitelist as that would require more boilerplate/config for each filter and I'd like to keep the filter setup as quick and simple as possible. As for the naming schema, that would break backwards compatibility which I'd like to stay away from.

What do you think about the following changes to solve this issue:

• Make ModelFilter an abstract class:

• In the filterInput() method update the method check to ensure the method does not exist on ModelFilter like:

   /** 
    * Filter with input array. 
    */ 
   public function filterInput() 
   { 
       foreach ($this->input as $key => $val) { 
           // Call all local methods on filter 
           $method = $this->getFilterMethod($key); 
  
           if (method_exists($this, $method) && !method_exists(ModelFilter::class, $method)) { 
               $this->{$method}($val); 
           } 
       } 
   } 

Then also add a $blacklist parameter or $internal or something that can be an array of method names that are internal filter methods only to not be executed. Then add that check to the above method.

Thoughts/changes on this approach?

[jpnut]

I hadn't considered backwards compatibility - with that in mind, your solution sounds like a more appropriate idea.

I think a whitelist or naming schema would be best practice from a security point of view, however the likelihood of dangerous or malicious actions being available is pretty low to start with. It might be worth keeping in mind if there are any major re-factors in the future.

[robclancy]

Being explicit is much better, if I was using this right now I would simply apply it by using $request->only() instead of $request->all().

[jpnut]

@robclancy 100% agree, and to be completely honest as a rule of thumb I try to avoid $request->all() as much as possible. Far better to be explicit when dealing with untrusted input. No doubt there are many people who would disagree and prefer the simplicity that $request->all() provides, so I'm still firmly of the belief this should be resolved.

[robclancy]

Yep, $request->all() is simply not allowed here.

[Tucker-Eric]

Updates added to 1.3.0