Watch out for Request

Common Pitfalls

Posted by lagbox on June 8, 2016 laravel security tips

Watch out for Request

The Illuminate\Http\Request class is awesome; this is known. Laravel added some magic in this class that extends from Symfony's request class. There are a few things to watch out for.

You don't get it

Avoid the get method. This is a method inherited from Symfony's class and doesn't act the same as Illuminates input method, which is the documented and recommended way to get input from the request.

The input method pulls different input sources together and pulls the input you want by name. The all method calls the input method to retrieve these inputs and adds the files to it. This is important to know.

Validation will use $request->all() to pull the inputs. So if you are doing some validation then after it you do $request->get(...) you may be getting a value that is not the same as what was validated (based on the different input sources used).

The get method is not documented in the Laravel docs and should not be used on Request. The Request and Input facade both use the request binding which is an instance of Illuminate\Http\Request. The one difference is that the Input facade has one real static method on it get. This method is just calling $request->input(...).

Forget that Request@get exists!

All the things are dynamic

The Illuminate Request class has a nice __get method which allows you to use dynamic properties to access request inputs. This isn't a bad thing, the only inputs this won't work as expected for are (attributes, request, query, server, files, cookies and headers). These are real public properties that are inherited from Symfony's Request class. PHP will always access publicly accessible properties; the __get is never called for these as they exist.

The dynamic property works as long as you know this when using it. If it can't find any input matching what you are looking for it will fall back to trying to return a route parameter by that name.

Simple demonstration

To demonstrate this:

Route::get('test', function (Illuminate\Http\Request $request) {
    dd(
        $request->server,
        $request->input('server'),
        $request->attributes,
        $request->input('attributes')
    );
});

Then hit that route: http://yoursite.com/test?server=bob&attributes=tom.

ServerBag {#45 ▼
  #parameters: array:33 [▶]
}

"bob"

ParameterBag {#42 ▼
  #parameters: []
}

"tom"

Security please

This is a big one for me. If you can, avoid using the dynamic property for retrieving route parameters from the request. It is the fallback if no inputs exist by the name.

$request->id;

That is going to look at the inputs returned by all to see if one is named id, if not it will fallback to trying a route parameter named id.

I kinda hope you can see how this can be very dangerous, as it is not explicit. There is actually some security risk here if you aren't paying attention to what you are doing.

The example goes like this.

// some form request

public function authorize()
{
    $somemodel = SomeModel::find($this->id);

    // does the user own this record

    return $this->user()->id == $somemodel->user_id;
}

// controller

public function edit(Request $request, $id)
{
    $model = SomeModel::find($id);

    ....
}

You may think that you are getting the same id in each of these but that may not be true at all.

The controller method is receiving the route parameter. In the FormRequest, using the dynamic property of Request, you are getting what? If there are no inputs named id you are getting the route parameter presumably.

Now lets throw a wrench into the mix.

Simple DB layout:

posts
id      user_id
---------------
1       1
4       2         <--- I am logged in as User 2

Lets hit that route:

http://yoursite.com/posts/1/edit?id=4

In this example Post with id 1 is what I want to edit. The ?id=4, 4 is a Post I know that I own (different user than the user who owns Post 1).

Now look what happens. The authorize method will return true even though I don't own Post 1 because we are not checking Post 1, we are checking Post 4. This is because the dynamic property returns 'inputs' first then falls back to route parameters.

The controller doesn't know about this mistake, it is receiving the route parameter directly, so it recieves 1 for $id. I just bypassed the authorization you have in place by simply passing along an input with the query string.

The correct way to avoid this problem is to be explicit with what you are requesting.

public function authorize()
{
    $somemodel = SomeModel::find($this->route('id'));
    // we are explicitly requesting the route parameter named 'id'

    ...
}

Seems trivial but you need to know what these methods are doing. Yes someone would need to know some things about your application potentially and how your route parameters are named, but this isn't something that someone couldn't just guess and check over and over and figure this out.

Simple demonstration

Simple demonstration of dynamic property pitfall vs explicitly grabbing the route parameter:

Route::get('testit/{id}', function (Illuminate\Http\Request $request, $id) {
    dd($request->id, $request->route('id'), $id);
});

Lets hit that route: http://yoursite.com/testit/1?id=4

"4"

"1"

"1"

Food for thought

Be very aware of what 'magic' methods are doing. As long as you are aware of what is happening you should be okay. It is pretty simple to demonstrate these pitfalls.

Some github action about get pitfalls:

laravel/docs PR - Add a note explaining the pitfalls of ->get #2269

laravel/framework issue - Accidentally selecting from a different ParameterBag than the one validated on #13138

Dynamic Property and Route Parameters:

laravel/docs PR (closed) - [5.2] Updated Requests - dynamic property #2030

Laravel 5.2 Docs - Requests - Retrieving Input - Dynamic Properties