[Bug] Backpack controllers affect on main app (security/perfomance issue)

[mgralikowski]

After upgrade backpack, I noticed some changes in app logic and I noticed finally that backpack affects on standard web controllers.

My all backpack controllers extend my custom admin base controller. In this controller, I have some common logic in the construct method. And the worst is - this constructor is triggered X times where X is the amount of Route::crud in a custom routes file. It is not about extending because the effect is also noticeable in certain class construct. For example, if we add dd() or Log::info() it will be called even on our home app page! Another test - make some errors in crud controllers - entering to any page will raise the error. Visualization (app home page - outside admin area):

I noticed this on Laravel 6 and backpack 4.1 but I tried demo (Laravel 7) and there is the same problem.

[tabacitu]

@mgralikowski indeed - the setupXxxRoutes() get called inside each Operation trait. That's what Route::crud() does, goes through the corresponding controller, looks at all methods that look like setupXxxRoutes() and executes the logic inside them. So if your CrudControllers are broken, all routes are broken. It was a choice we made with Backpack 4.x routing. But all our tests show there's no security or performance issue with this. See the PR #1937 for more information about how it's implemented.

To rephrase:

• usually in Laravel, route definition is completely isolated from controllers; if one controller has a HUGE error that stops execution (like a PHP error), most routes will still work; only the routes that point to that one controller will stop working;
• starting with Backpack 4.0, routes that are defined using Route::crud() are actually defined inside the CrudController; that means that yes, if one of your CrudControllers is broken (again, huge page-stopping error like PHP error, syntax error or an entire controller missing, like your screenshot above, not minor errors like a field missing or something not working correctly with JS - those are fine), then ALL routes will stop working; including the routes for the front-end;

We made the choice to add this "routing in Controller" functionality because it helps Backpack developers easily use & create Operation traits. Basically just adding that one trait for CloneOperation adds not only the methods on the CrudController, but also the needed routes. Which makes it SUPER EASY to create, use and re-use Operations across CrudControllers.

Before deciding to implement it, we've tried it in quite a few real-life projects, and we discovered it works great:

• PRO: it does NOT significantly impact performance, since only very small bits of code get executed (the route loading, which would have been run anyways not matter what file they were in);
• PRO: it helps A LOT in having all operation code bundled into one file;
• PRO: it makes it so that you can forget about editing the routes/backpack/custom.php, the generated code is all you need most of the times;
• CONS: it does have this one caveat that you have to make sure your CrudControllers aren't broken;

But in real-life projects we've discovered this never ever happens (the CON). You never push an app to production with missing Controllers, or with PHP errors. So we decided the PROs heavily outweigh the CONs.

---

There's a workaround, of course. If you definitely don't want this to happen, instead of using Route::crud() in your routes/backpack/custom.php, you can manually define the routes for the operations you use, for each CrudController. The routes can easily be taken from each operation's trait, from the setupXxxRoutes() method. But in my opinion it doesn't ever make sense to do this.

---

I'm going to close the issue, because the way I understand this, it isn't really a bug:

• the screenshot above shows that you have a CRUD route pointing to a missing CRUD controller; which is a developer error;
• the setupXxxRoutes() methods are supposed to be called in Backpack 4.x, even when loading a page that has nothing to do with Backpack; our tests showed no performance or security issues;

I'll gladly continue the discussion, even if the issue is closed, to clarify things.
I'm open to feedback too, of course.

Cheers!

[mgralikowski]

Sure, the example with the crash was only for visualization purposes and it helped me in my investigation (script stack/debug). My app was crashing because I use in my admin area `Eloquent::unguard() (because of #1568 ) and it was affecting all app post actions.

But what in case if I want to share some common logic between controllers and I use the parent base controller? I putting some heavy logic, maybe with DB queries. Now, it will be triggered 25 times (in my example case). Not using Route::crud() is the only one workaround?

[tabacitu]

But what in case if I want to share some common logic between controllers and I use the parent base controller? I putting some heavy logic, maybe with DB queries. Now, it will be triggered 25 times (in my example case). Not using Route::crud() is the only one workaround?

If you use a parent CustomCrudController instead of Backpack's CrudController the performance will still not be affected. Because nothing gets called inside it (by the routing system) except for the setupRoutes() method. So no, it should not be triggered 25 times.

---

With the current controller-based routing in traits, Route::crud() will trigger:

• inside CrudController - only setupRoutes() (or your CustomCrudController::setupRoutes() if you do it that way);
• inside ProductCrudController - nothing;
• inside XxxOperation trait - only the setupXxxRoutes() method;

So as long as you don't put that heavy logic inside the routing methods, there will be no security or performance hit. Because only the routing methods get called. And you have no reason to bloat up those methods. Rephrased: those files get loaded but no other methods get called. Not even __construct() is called on them. Just the routing methods mentioned above. So yes they have to be valid PHP files. With no syntax error. But that's about the only gotcha.

---

Btw, I believe in Backpack 4.1 the relationship method saves all relationships (even 1-1 or 1-n) using sync() - because the name of the field is the relationship name (ex: category instead of category_id). If I remember correctly that was one thing you didn't like about Backpack - that it used the foreign key to save the relationship, instead of doing sync() right? 😄 And you preferred to have your foreign keys guarded. Well now you can do that, if you just use relationship instead of select. Same syntax. You might not need Eloquent::unguard() anymore 🥳