[5.0] route model binding runing before middleware

[aliuygur]

Route model binding runing before middleware, this is bug.

[GrahamCampbell]

Pull request to fix?

[aliuygur]

@GrahamCampbell, i have no idea that how is it gonna be resolved.

[aliuygur]

@taylorotwell this is critical bug. please fix it.

[GrahamCampbell]

@alioygur This is NOT critical. Any bug that affects laravel 5 is low priority.

DO NOT USE LARAVEL 5 IN PRODUCTION

[taylorotwell]

Why would this be a bug?

[aliuygur]

@Taylorotwell, I thinking this is bug. Because,

Imagine you use http basic authentication. (Stateless)

So, user authenticated at middleware (AuthenticatedWithBasic)

And you have a route and model binding, like this

// routes.php

$router->group(["prefix" => "me", "middleware" => "App\Http\Middleware\AuthenticatedWithBasic"], function($router) {

$router->resource("posts", "App\Http\Controllers\PostsController");

});

// routeserviceprovider@before

$router->bind("posts", function($post_id) use ($router){
return $router->user()->posts()->findOrFail($post_id);
});

So, I maybe do somethings with current logged in user order model binding. Currently I can not this because model binding running before middlewares.

I am sorry, for my bad English . I am on mobile.

[taylorotwell]

I still don't understand.

On Tuesday, October 21, 2014, ali oygur notifications@github.com wrote:

@Taylorotwell https://github.com/Taylorotwell, I thinking this is bug. Because,

Imagine you use http basic authentication. (Stateless)

So, user authenticated at middleware (AuthenticatedWithBasic)

And you have a route and model binding, like this

// routes.php

$router->group(["prefix" => "me", "middleware" => "App\Http\Middleware\AuthenticatedWithBasic"], function($router) {

$router->resource("posts", "App\Http\Controllers\PostsController");

});

// routeserviceprovider@before

$router->bind("posts", function($post_id) use ($router){ return $router->user()->posts()->findOrFail($post_id); });

So, I maybe do somethings with current logged in user order model binding. Currently I can not this because model binding running before middlewares.

I am sorry, for my bad English . I am on mobile.

— Reply to this email directly or view it on GitHub https://github.com/laravel/framework/issues/6118#issuecomment-60001449.

[aliuygur]

okey, just one question

SO, what I supposed to do if I try to get the current logged in user information while binding model. Note: I am using http basic authentication.

/**
 * Called before routes are registered.
 *
 * Register any model bindings or pattern based filters.
 *
 * @param  Router  $router
 * @return void
 */
public function before(Router $router)
{
    $router->bind('resumes', function ($value, $route) {
        $user = app('auth')->user();
        $model = $user->resumes()->findOrFail($value);

    });
}

I am getting following error.

FatalErrorException in RouteServiceProvider.php line 43:
Error: Call to a member function resumes() on a non-object

because AuthenticatedWithBasic middleware did not run before model binding.

this is AuthenticatedWithBasicAuth file

<?php namespace App\Http\Middleware;

use Closure;
use Illuminate\Contracts\Auth\Guard;
use Illuminate\Contracts\Routing\Middleware;

class AuthenticatedWithBasicAuth implements Middleware {

    /**
     * The Guard implementation.
     *
     * @var Guard
     */
    protected $auth;

    /**
     * Create a new filter instance.
     *
     * @param  Guard  $auth
     * @return void
     */
    public function __construct(Guard $auth)
    {
        $this->auth = $auth;
    }

    /**
     * Handle an incoming request.
     *
     * @param  \Illuminate\Http\Request  $request
     * @param  \Closure  $next
     * @return mixed
     */
    public function handle($request, Closure $next)
    {
        return $this->auth->onceBasic() ?: $next($request);
    }

}

[JoostK]

So basically, in order to find out if the bound object belongs to the authenticated user, you will need access to the current user in the bind callback.

[aliuygur]

@JoostK yep! you are right.

[robinmitra]

I'm in the same boat as @alioygur. Any possible solution to this issue yet? Otherwise, I'm afraid I would have to get rid of all my route model bindings sadly :(

[JoostK]

This shouldn't have been closed, delaying model binding until after the session data is available makes kind of sense.

[MrAtiebatie]

I came across the same issue. Imagine you have a admin panel where you can manage the users that can login into your website. As admin you want to be able to see users by browsing to admin/users/5 for example. Of course you only want admin's to be able to see that page so you put a middleware in your controller to redirect guests to the login page. In case that user 5 doesn't exist you see a nice 404 not found which is fine to me. BUT, when the guest browses to the admin/users/5 page, he doesn't get redirected to the login page but also sees the 404 not found page which is not fine with me. I hope you can fix this.

[taylorotwell]

None of that helps me without seeing actual code.

On Thu, Jan 15, 2015 at 4:28 AM, MrAtiebatie notifications@github.com wrote:

I came across the same issue. Imagine you have a admin panel where you can manage the users that can login into your website. As admin you want to be able to see users by browsing to admin/users/5 for example. Of course you only want admin's to be able to see that page so you put a middleware in your controller to redirect guests to the login page. In case that user 5 doesn't exist you see a nice 404 not found which is fine to me. BUT, when the guest browses to the admin/users/5 page, he doesn't get redirected to the login page but also sees the 404 not found page which is not fine with me. I hope you can fix this.

— Reply to this email directly or view it on GitHub https://github.com/laravel/framework/issues/6118#issuecomment-70066489.

[MrAtiebatie]

My code or framework code?

[taylorotwell]

Your code. Show me where the problems are actually happening.

On Thu, Jan 15, 2015 at 9:02 AM, MrAtiebatie notifications@github.com wrote:

My code or framework code?

— Reply to this email directly or view it on GitHub https://github.com/laravel/framework/issues/6118#issuecomment-70097997.

[MrAtiebatie]

This is my (resource) controller constructor:

/**
 * Constructor
 * @param User $user
 */
function __construct(User $user)
{
    $this->user = $user;

    $this->middleware('guest');
}

This is my guest middleware:

/**
 * Handle an incoming request.
 *
 * @param  \Illuminate\Http\Request  $request
 * @param  \Closure  $next
 * @return mixed
 */
public function handle($request, Closure $next)
{
    if ($this->auth->guest())
    {
        return redirect('auth/login');
    }

    return $next($request);
}

And in routes.php:

 Route::resource('admin/users', 'Admin\UsersController');

I have tried the beforeFilter in the constructor and a group filter in routes.php but both still gave a 404 not found, browsing as guest.

[robinmitra]

Just to chime in, I was roughly trying to do something like the following, but it fails due to this issue:

Route model (or repository in my case) bindings in AppServiceProvider::register() were something like:

public function register()
{
    $repoAbstract = 'App\Contracts\Repositories\Order';
    $router->bind('order', function ($value) use ($repoAbstract) {
        $repo = App::make($repoAbstract);
        if ($model = $repo->find($value)) {
            return $repo;
        }

        throw new NotFoundHttpException('Repository ' . $repoAbstract . ' could not find the model!');
    });
}

And this is Order repository's find() method:

public function find($id)
{
    $model = $this->model
        ->where('id', $id)
        ->where('user_id', Auth::id())
        ->first();

    if ($model) {
        $this->setModel($model);
    }

    return $model;
}

The find() method in the repository expects the user to be authenticated, in order to only return orders that belong to that user, which is pretty standard stuff.

However, using route model (or repository in my case) bindings will fail for the above scenario, since the user hasn't yet been authenticated, which will cause the repository's find() method to fail.

As it stands right now, although I think route model bindings are pretty cool, I've had to disable all of them sadly, since they won't work for my scenario which is quite common frankly.

[landons]

I just ran into a similar issue, but it was pretty easy to get around with dependency injection:

$router->bind('project', function($id)
{
    App::bind('App\Contracts\Project', function() use($id)
    {
        if (($project = Project::findByClient(Auth::user(), $id))) {
            return $project;
        }

        throw new NotFoundHttpException;
    });

    return $id;
});

[GrahamCampbell]

This is not a bug, but a feature. I was talking to Taylor about this earlier today, not in relation to this though.

[vtalbot]

Could there be a third arguments to bind that defer the binding after the middlewares or an arguments to tell the binding to be done after a middleware or an array of middlewares?

[jordandobrev]

There is an easy fix for those who are stuck with this issue. Instead of using the bind method in the service provider, just replace the route parameter with the object using middleware.

public function handle($request, Closure $next)
{
    // some middleware logic ....
    if ($request->route()->hasParameter('user')) {
        $user = $this->app->make(UserRepository::class)->getByIdOrFail($request->route()->parameter('user'));
        $request->route()->setParameter('user', $user);
    }

    return $next($request);
}

Cheers ;)

[zot24]

Thanks @LPMadness good example!

[malhal]

Binding has always been less than ideal because it occurred before filters in 4, and now occurs before middleware in 5. For un-authorized requests it needlessly hits the database to find a record, only to fail authorisation afterwards, a complete drain on resources. There is no doubt model binding should be done as the very last thing in the chain of events. This might be one of those times where the design that best fits peoples needs doesn't align well with the framework's original technical design, personally I hate when that happens ;-)

[ont]

Just another possible solution. Declare middleware as global in app/Http/Kernel.php:

class Kernel extends HttpKernel {

    /**
     * The application's global HTTP middleware stack.
     *
     * @var array
     */
    protected $middleware = [
        'Illuminate\Foundation\Http\Middleware\CheckForMaintenanceMode',
        'Illuminate\Cookie\Middleware\EncryptCookies',
        'Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse',
        'Illuminate\Session\Middleware\StartSession',
        'Illuminate\View\Middleware\ShareErrorsFromSession',
        'App\Http\Middleware\VerifyCsrfToken',
        'App\Http\Middleware\YourAwesomeAuthMiddleware',   /// <<<< HERE
    ];
....

By some reason global middlewares are called before bindings.

[slaughter550]

@taylorotwell @GrahamCampbell I am not sure if this helps, but this is our use case which would currently benefit from making route bindings resolve after the request part of the middleware stack has executed.

We currently have several types of bindings in prod. Two of them look like this.

// This example pulls from the User class directly
Route::bind('userResourceKey', function ($value, $route) {
    return bind_obj('User', 'resource_key', $value);
});

and 

// This example pulls from the posts function on the User model
Route::bind('postResourceKey', function ($value, $route) {
    return bind_obj('posts', 'resource_key', $value, auth()->user());
});

the abbreviated version of the bind_obj function looks like this. Please keep in mind this was written in shorthand for the purposes of conveying a point.

function bind_obj($resource, $key, $value, $scope = null, $options = [])
{
    $obj = null;
    if ($scope) {
        $obj = $scope->$resource()->where($key, $value);
    } elseif (($resource = "App\Models\\$resource") && class_exists($resource)) {
        $obj = $resource::where($key, $value);
    }

    // more code to determine permissions and whether this is a protected path

    if ($obj && $obj = $obj->first()) {
        return $obj;
    } else {
        throw new NotFoundHttpException;
    }
}

The bind_obj function is responsible for resolving the issue of whether the user is already logged in once passed auth, for determining permissions. However, because our Laravel stack runs a restful api, we encounter this specific issue when the user is not logged in.

The request on an existing resource like https://api.example.com/users/{userResourceKey} returns a 403 as expected The request on an also existing resource like https://api.example.com/posts/{postResourceKey} returns a 404, because the dependency on auth()->user() is null. This behavior is not desirable. It is desirable for the response code to be a 403.

Although the above problem is solvable with more logic, it gets trickier for different types of routes and whether they are protected and what their dependencies are. This in turn, leads to a lot of messy and duplicate logic all to determine whether a bind should actually run or not.

The next issue, and the biggest for us currently is for objects that are constructed like userResourceKey. It is not desirable in production for a user that is not authenticated to be able to make a http request which forces a DB action even though it is not required for that request.

I would expect them to hit the auth middleware first and have Laravel throw a 403 before going any further.

Hope this helps. Happy to answer any questions.

[simondubois]

Here is my contribution (Laravel 5.1.15). I am trying to bind model to route parameters. For security reasons, only models related to the authenticated user should be bound.

<?php namespace App\Providers;

use Auth;
use Illuminate\Routing\Router;
use Illuminate\Foundation\Support\Providers\RouteServiceProvider as ServiceProvider;

class RouteServiceProvider extends ServiceProvider
{
    public function boot(Router $router)
    {
        $router->bind('account', function($id) {
            if (Auth::check() === false) {
                return null;
            }

            return Auth::user()->accounts()->find($id);
        });

As nor session neither authentication are already loaded, Laravel seems to generate new session id on each request.

[slaughter550]

If it helps anyone, we ended up using the controller middleware, and doing this.

abstract class Controller extends BaseController
{
    use DispatchesCommands, ValidatesRequests;

    public function __construct()
    {
        $this->middleware('bind');
    }
}

where the middleware "bind" looks like this.

namespace App\Http\Middleware;

use Closure;
use App\Configs\Domain;

class BindRequests
{
    protected $binders = [];

    public function __construct()
    {
        $this->binders = Domain::getBindables();
    }

    public function handle($request, Closure $next)
    {
        foreach ($request->route()->parameters() as $key => $value) {
            if (isset($this->binders[$key])) {
                $boundObject = $this->performBinding($key, $value, $request->route());
                $request->route()->setParameter($key, $boundObject);
            }
        }

        return $next($request);
    }

    private function performBinding($key, $value, $route)
    {
        return call_user_func_array('routeBinder', $this->binders[$key] );
    }
}

[simondubois]

@slaughter550 Thanks for sharing, but $request->route() is null (Laravel 5.1). Anyway, I remain with implicit controller routes and bind input parameter inside controller. I just feel parameter binding is quiet useless for application with access control...

[ryanmortier]

I ran into this issue today as well. The Authenticate middleware should definitely be running before route model binding otherwise users do not get redirected to the signin page when hitting a URL that uses route model binding.

I don't believe this to be a bug in the way middleware works, just the way auth is implemented.

Here is how I worked around the issue, although it isn't the most elegant solution. I hate to modify auth in case I introduce a hole but I definitely needed auth to run before route model binding and I didn't want to stop using route model binding.

\App\Http\Middleware\Authenticate

    public function handle($request, Closure $next)
    {
        if ($this->auth->guest() && ! $request->is('signin')
                                 && ! $request->is('password/email')
                                 && ! $request->is('password/reset')
                                 && ! $request->is('password/reset/*'))
        {
            if ($request->ajax())
            {
                return response('Unauthorized.', 401);
            }
            else
            {
                return redirect()->guest('signin');
            }
        }

        return $next($request);
    }

\App\Http\Kernel

    protected $middleware = [
        \Illuminate\Foundation\Http\Middleware\CheckForMaintenanceMode::class,
        \App\Http\Middleware\EncryptCookies::class,
        \Illuminate\Cookie\Middleware\AddQueuedCookiesToResponse::class,
        \Illuminate\Session\Middleware\StartSession::class,
        \Illuminate\View\Middleware\ShareErrorsFromSession::class,
        \App\Http\Middleware\VerifyCsrfToken::class,
        \App\Http\Middleware\Authenticate::class,
    ];

Edit: Yeah, I didn't feel comfortable with this workaround so just ignore it. I guess I'll just stop using route-model-binding.

[andreygrehov]

@taylorotwell this is a bug. Once again: For un-authorized requests it needlessly hits the database to find a record, only to fail authorisation afterwards, a complete drain on resources. I can have sleep(100) in the resolution logic, which should not be even reached if I'm not authorized.

[JosephSilber]

I completely disagree that this is a bug. I believe it makes perfect sense for route model binding to be resolved before middleware; your middleware may want to access said model.

When using route model bindings, the idea is to never fiddle with the actual raw data in the route; it's as if the model is part of the actual route.

If you have to compare a user to a model, that should be done with authorization or some other way in the controller. This is not a bug, and definitely not a security issue.

For a real-world example of when it is actually very useful to resolve the model before the middleware, see the popular authorize middleware.

[andreygrehov]

@JosephSilber route model binding is a framework's feature, whereas authorization is, sort of speak, a higher level. The fact that authorize middleware is popular doesn't guarantee its architectural correctness. Now let's imaging a case - your database is very large, huge - millions of rows. You only have 2 clients, who are allowed to access this database. Because: a) operational costs are very high, b) you don't need more clients. The route model binding in this sample case is using a few parameters to find the required row/model (not just a simple primary key - and that's how you decided to model your database). Such query takes a good amount of time, because of the size of the database and lack of a simple way to access data by PK.

Now I am a bad guy who wants to kill your business, write a simple script that hits your API endpoint a thousand times a second. I don't have access to API, except the full url. In this case, model binding will execute the code to run thousand queries per second, making the CPU go to 100% of its use. Memory usage will jump to the top and the whole server will, presumably, go down. Now what will happen next?

1. You will receive a very nice bill from your hosting provider.
2. The only two of your clients are not able to do business with you anymore.

Maybe this is not a bug and I was wrong calling it so, but this is quite a big flaw in the architecture.

P.S. Keep in mind, I'm not talking about accessing a user object here.

[JosephSilber]

If you are really concerned about that, you should be rate-limiting your API at a layer prior to your actual application.

[andreygrehov]

@JosephSilber this is a work around, whereas I'm talking about architecture. What are the cons of my suggestion? I'm happy to agree I'm wrong if there are reasonable arguments.

[GrahamCampbell]

Or just don't use route model binding if you're concerned about performance...

[GrahamCampbell]

The only two of your clients are not able to do business with you anymore.

That's nothing to do with Laravel. To be frank, if you're code sucks, it's not our fault.

[GrahamCampbell]

you should be rate-limiting your API at a layer prior to your actual application.

If you are concerned about ddosing, you're defiantly going to want protection before the php layer.

CloudFlare offer a basic free service to put your app behind. Others are available too. :P

[andreygrehov]

@GrahamCampbell once again, what are the cons? Good framework should encourage to do a better job. I can of course do anything and not use bindings, but again, why would model binding exist at all then? If there is an auth middleware, why is it even allowed to do anything besides auth? That's what I don't understand.

[andreygrehov]

@GrahamCampbell well, that's not an answer, you could say "Go use Ruby" or "Find another job", essentially, this is the same as saying go buy CloudFlare solutions. I solved the problem by using the middleware, but that's the wrong path down the road. I don't know - it's simply wrong philosophy.

[GrahamCampbell]

You're free to choose ruby if it fits your needs better.

[malhal]

@GrahamCampbell please stop being childish. A lot of us want this problem fixed and you aren't helping. I think this is a pretty major design flaw, and that people are getting so passionate about it surely justifies it as so.

[GrahamCampbell]

I'm being totally serious, but the reality is that you need to just stop using route model binding because it doesn't fit your use case.

[GrahamCampbell]

Route model binding should definitely be done before the middlewares so the route parameter access is consistant.

You should note that you can actually have some middlewares that are processed BEFORE route model binding. The stuff registered in the kernel class should be processed before all the routing stuff has happened.

[malhal]

I sort of agree, because in the past when I've made use of cool Laravel features I usually turn them off as I need to do more advanced things. So it would be my guess if this was even fixed, we'd probably end up not using model binding for some other reason down the line.

[GrahamCampbell]

Alternatively, you could actually implement route model binding through a middleware. :)

[andreygrehov]

That's what I ended up doing. Original route model binding is a candidate for DDoS.

[GrahamCampbell]

Proper DDoS protection really belongs above the php level though, so I don't agree route model binding is really a target, especially since it is actually possible to bail out using a global middleware before route model binding has made any database queries.

[malhal]

Yeh I wouldn't use the DDoS argument for trying to get this fixed since there likely are a high number of other candidates, and remember all an attacker would need to do is log in with a fake account and then its the same problem. This is more about a design flaw, that plays with the mind of the coder who cares, who likely knows about trying to be as efficient with database comms as possible, and this problem is like a big fat middle finger in their face ;-)

[andreygrehov]

I agree that proper DDoS protection is out of PHP scope. But I still consider this architecture a bit awkward.

@malhal the thing is that an attacker does not need to log in at all in this case. He doesn't need to know any tokens, credentials, etc. Just the url. But I guess you understand that anyway.