[5.3] Using cached controller can lead to race conditions with middleware

[tomschlick]

Just ran into an issue with a recent change in 5.3 in which Illuminate\Routing\Route::getController() now caches it's result.

Basically if you are running auth middleware via a group parameter in your routes file

$router->group(['prefix' => 'projects', 'middleware' => ['auth']], function($router) {
    // Controller setups here
});

and you use the __construct() method in your controller to bootstrap things that require auth in your controller

public function __construct()
{
    parent::__construct();

    // this checks the currently logged in user
    $this->participant = $this->project->currentParticipant();
}

What you will see is that the Routing class will call gatherMiddleware() which will in turn call controllerMiddleware(). That controllerMiddleware() inits the controller class (to do reflection for any other middleware calls) before any route specified middleware is fired (the auth in this case) and caches the result.

So when the example above happens, $this->participant is null because the auth middleware hasn't fired yet, logging the user in for that request.

By removing the cache of $this->controller it allows the controller to be re-built after all the middleware is initiated and clears this bug up.

It looks like the bug was unintentionally introduced in #14097 by @JosephSilber ... Joseph can you take a look at it and see if removing the cache causes any issues?

[JosephSilber]

You shouldn't be using anything in your controller's constructor that requires middleware.

Even before the changes in #14097, doing this would not have worked:

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

    // do anything with the logged-in user
}

...since the middleware is run after the controller instance is returned.

Granted, before you were able to utilize middleware that were declared on the route. That was an implementation detail, and is not something that should be relied upon.

---

As a side note, instantiating the controller twice feels really wrong to me:

1. It's really unexpected. You may not want whatever you're doing in there to run twice.
2. It's not even a reliable solution to your problem. What if currentParticipant were to throw an error when there's a missing user? You solved this for your particular situation, but it makes the overall expectation worse.

---

As a rule of thumb, you shouldn't use anything in the controller's constructor that relies on middleware.

[GrahamCampbell]

I agree with Joseph. Never access auth in a controller constructor.

[GrahamCampbell]

Constructors are good for dependency injection, and that's about that, but even then, you can still use method injection, so there is no real need for them at all.

[tomschlick]

It's an implementation that has worked reliably since L4. Historically in Laravel, many people have used BaseControllers to setup this type of common functionality between controllers that are alike. This will not just break auth calls, but anything that has to do with middleware (cookies, csrf, etc).

This just breaks that common practice for little to no benefit and will cause issues for others as they start to upgrade to 5.3.

One way that we could have the best of both worlds though is a boot method on the controller that fires after all middleware is run. Would you see anything wrong with that?

[fernandobandeira]

I'm 👎 here, since you can associate middlewares to a controller action inside the construct it doesn't seems like a good place to access a middleware since it may not have been instantiated yet.

However i'm 👍 with the creation of the boot method, this can be useful and will create a migration path for people that are using this technique.

[tomschlick]

Just added a pull request for the boot() method I described above. Let me know what you think...

#14834