framework
framework copied to clipboard
laravel
[11.x] Prevent destructive commands from running
This PR introduces a Preventable trait which may be added to "destructive" commands to prevent them from running.
While many commands are "confirmed" in production, they are still possible to run. This trait takes that a step further by effectively disabling them. So, for example, you can't inadvertently run migrate:fresh in production (like I did this morning).
Similar to other safety methods, you may add calls to prevent certain commands to the boot method of your AppServiceProvider:
public function boot(): void
{
FreshCommand::preventFromRunning();
RefreshCommand::preventFromRunning();
ResetCommand::preventFromRunning();
WipeCommand::preventFromRunning();
}
By default, this method will prevent the command from running - immediately terminating the command with a warning and non-zero exit code.
Note: you may limit this to certain environments by passing a boolean argument to this method. For example, only preventing them in production.
FreshCommand::preventFromRunning($this->app->isProduction());
Co-authored with @joelclermont
Would love to use this. Is there a way to easily extend this to all commands, for example, when I also want to prevent the use of queue:clear?
@PerryvanderMeer, yes. If merged, other native commands or even third-party commands could use the Preventable trait.
I started with these as I felt they were things you'd probably never want to run in production, whereas queue:clear you might.
If merged, other native commands or even third-party commands could use the Preventable trait.
This is cool! But this is actually 3 opt-ins (trait + service provider + manual check in handle() method), which seems a bit redundant. How about adding the Preventable trait directly to the base Command class, which means you only need the service provider call to opt in? IMO, that should be enough, as it still won't do anything unless you explicitly opt in. (It also makes sense since the trait is called Preventable, not ShouldBePrevented.)
@shalvah, adding it to the base Command seems extreme in reach. I was following what existed with the ConfirmableTrait added to individual commands already. But I'm open to whatever the team thinks.
Love this. Nice work!
What are your thoughts on providing an option to encapsulate all preventive commands throughout the framework?
// All
public function boot(): void
{
FreshCommand::preventFromRunning();
RefreshCommand::preventFromRunning();
ResetCommand::preventFromRunning();
WipeCommand::preventFromRunning();
}
// eg. Grouped Example
public function boot(): void
{
DestructiveCommands::preventFromRunning();
}
@ahinkle, absolutely. Joel and I discussed it while pairing. But I like to keep my PR atomic to first see if this is something the team wants. If so, I can imagine that being one of the fast follows, along with potentially other commands being preventable.
I like the idea, but i wish the implementation had some more flexibility.
How about in addition to accepting simple boolean parameter that it also accepts a \Closure|callable that when executed should return a boolean? This way we can do stuff like fn() => $this->app->isProduction() || $this->app->isDownForMaintiance(). And have it be executed correctly in octane or similar ecosystems that have the entire application once in memory.
Not that the \Closure in the example is a good idea but the general idea is not possible currently.
I love this. What if it just made you type "YES" the way Github makes you type the repo name to delete it? Rather than getting rid of all the functionality.
@joshuaziering, the ConfirmTrait has that covered for production environments. This is kind of that next level, protect me from myself type thing.
The state of mind I was in, I truly thought I was on another site and knew what I was doing and would have typed "YES" anyway. 😅
@shalvah, adding it to the base
Commandseems extreme in reach. I was following what existed with theConfirmableTraitadded to individual commands already. But I'm open to whatever the team thinks.
Hey @jasonmccreary excellent contribution. Much appreciated 🙏
I just won't 100% agree here with you. I think you have a valid point too, but in my opinion this is something that shouldn't be opt-in for commands. Preventing something from running has different security implications than: "type yes to run".
Adding it as a trait to "core" commands, or the commands you develop is fine I guess, but it's also something that's currently not implemented in the millions of commands developed by 3rd parties that shouldn't run in production.
It's mainly a developer decision on what commands to run on what environments, so why not just provide a way in the framework to disable some commands on some given conditions ? Similar to how we configure preventsLazyLoading, a similar preventsFromRunning.
Anyway this lands in the framework, I think everyone will benefit from the added security.
Cheerss
@joshuaziering, the
ConfirmTraithas that covered for production environments. This is kind of that next level, protect me from myself type thing.The state of mind I was in, I truly thought I was on another site and knew what I was doing and would have typed "YES" anyway. 😅
I agree 100%, there should not be a way to override it and run the command anyway, there are many occasions where I would have run something on the wrong server even if asked to confirm it.
This Laravel package Database-dump can also help with generating a dump file whenever you run migrate:fresh. This can be very helpful when working in development environment and you don't want to accidentally lose data. You can also use the package to seed the database with the same dump file generated. This can be helpful when one wants to make breaking changes in production.
So, for example, you can't inadvertently run migrate:fresh in production (like I did this morning).
Programming equivalent of seeing another guy get kicked somewhere... unpleasant.
