Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[11.x] Prevent destructive commands from running #51376

Merged
merged 9 commits into from
May 21, 2024

Conversation

jasonmccreary
Copy link
Contributor

@jasonmccreary jasonmccreary commented May 10, 2024

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());

Update
For future readers, Taylor "Wordsmith" Otwell changed the method name to prohibit and added a single method for preventing all these commands at once.

You may add the following call to the boot method of your AppServiceProvider to prevent these commands in production environments:

DB::prohibitDestructiveCommands($this->app->isProduction());

Co-authored with @joelclermont

@PerryvanderMeer
Copy link
Contributor

PerryvanderMeer commented May 10, 2024

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?

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented May 10, 2024

@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.

@shalvah
Copy link
Contributor

shalvah commented May 10, 2024

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.)

@jasonmccreary
Copy link
Contributor Author

jasonmccreary commented May 10, 2024

@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.

@ahinkle
Copy link
Contributor

ahinkle commented May 10, 2024

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();
}

@jasonmccreary
Copy link
Contributor Author

@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.

@hameedraha
Copy link

Wonderful idea. This should definitely be a framework-level feature.

@driesvints driesvints changed the title Prevent destructive commands from running [11.x] Prevent destructive commands from running May 13, 2024
@morloderex
Copy link
Contributor

morloderex commented May 13, 2024

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.

@joshuaziering
Copy link

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.

@jasonmccreary
Copy link
Contributor Author

@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. 😅

@pxpm
Copy link
Contributor

pxpm commented May 17, 2024

@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.

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

@johanrosenson
Copy link
Contributor

@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. 😅

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.

Copy link
Contributor

@msonowal msonowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree its one more step per project level so we do that

@taylorotwell taylorotwell merged commit 075c69f into laravel:11.x May 21, 2024
28 checks passed
@justinkekeocha
Copy link

justinkekeocha commented May 22, 2024

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.

@jasonmccreary jasonmccreary deleted the preventable-commands branch May 22, 2024 16:56
@h-sigma
Copy link

h-sigma commented May 30, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.