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

Prevent execution of ddev config in the wrong directory #5493

Open
1 task done
gitressa opened this issue Nov 2, 2023 · 42 comments
Open
1 task done

Prevent execution of ddev config in the wrong directory #5493

gitressa opened this issue Nov 2, 2023 · 42 comments

Comments

@gitressa
Copy link
Contributor

gitressa commented Nov 2, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Is your feature request related to a problem?

The other day I was creating a new DDEV instance using the ddev composer create command.

I wasn't concentrating (I know, bad move) and accidentally didn't enter the folder I had just created for this purpose. These should have been the steps:

cd ~/dev
mkdir drupal10
cd drupal10
ddev config --auto
ddev composer create drupal/recommended-project

Except, I got distracted and skipped the cd drupal10 step ...

I quickly hit "Enter" through the prompts from the ddev config and ddev composer create commands, which looked all right. But since I wasn't in ~/dev/drupal10, but instead in the ~/dev folder, where all my projects are (around 15) they were almost all deleted, until the process got stopped by a write-only folder.

Yes I know, I should have double checked the prompts.

Describe your solution

It would be awesome if it was possible to designate folders, where DDEV will gladly run project commands such as ddev composer create (i.e. those where you can get a could not find a project message) and reject them elsewhere. ddev list and similar commands which doesn't alter anything should still be allowed from any folder.

We only need to check for ddev config, since without a .ddev sub-folder present, no file altering commands will run, but simply return a "Failed to get project(s): could not find a project in /home/username.".

For example, I only ever run DDEV commands in project sub-folders within a development folder called ~/dev (/home/username/dev), such as /home/username/dev/drupal10.

So a rule could be, to only allow DDEV to execute file altering commands in sub-folders of ~dev with the pattern /home/username/dev/*.

Meaning: Only allow running ddev config from for example /home/username/dev/drupal10

Everywhere else, for example /home/username/dev or /home/username you get a message like this:

You can only run ddev config to start new DDEV projects from /home/username/dev/projectname

Defining the rule in plain speech: "Only allow running ddev config from these folders". Since there can be more than one, it should probably be an array, like this:

allow_ddev_config: ["/home/rfay/workspace/*", "/tmp/*"]

How it could look in .ddev/global_config.yaml:

# Only allow running ddev config from these folders
# Separate multiple values with comma, quoted
# allow_ddev_config: ["/home/username/workspace/*", "/tmp/*"]

The ddev config command already checks if it is run from the home folder, which it doesn't allow and returns the message "could not create new config: ddev config is not useful in your home directory (/home/username)".

https://github.com/ddev/ddev/blob/master/pkg/ddevapp/config.go#L67-L70

Maybe the allow_ddev_config setting can be added similarly?

Describe alternatives

No response

Additional context

No response

@rfay
Copy link
Member

rfay commented Nov 2, 2023

I have done this many times. I do it in the ~/workspace/ddev folder periodically because I'm working on DDEV code and am in the wrong window. My problem is typically doing it with ddev config in the wrong directory. Also, ddev config will create a project in a subdirectory of a project that already exists in some situations, which is also annoying. However, I'm not sure how to actually deal with it appropriately. I create projects in ~/workspace/* and ~/tmp/* sometimes. I don't ever want to create one in ~/workspace or ~/tmp, always in a subdirectory, but never in ~/workspace/ddev.

More thoughts about how this would work? I don't think we could encourage most of the people that need it to use it, and experienced users already know exactly what happened, and novices wouldn't know how to configure it...

If you use ddev config in a subdirectory of a project it checks things,

$ ddev config --auto
it usually does not make sense to `ddev config` in a subdirectory of an existing project. Is it possible you wanted to `ddev config` in parent directory /Users/rfay/workspace/d10?

But sometimes you don't see that and don't realize that your config didn't work.

@rfay rfay changed the title Add DDEV command safe folders feature ddev config can accidentally be done in the wrong directory, how to prevent that? Nov 2, 2023
@gitressa
Copy link
Contributor Author

gitressa commented Nov 2, 2023

Thanks for sharing, and we are probably not the only two who have done this, but it is slightly embarrassing to admit ... :-)

And thanks for updating the title, it's more precise now. It also made me realize that we don't need to keep an eye on a lot of commands, only ddev config, since without a .ddev sub-folder present, no file altering commands will run, but simply return a "Failed to get project(s): could not find a project in /home/username.".

Also, ddev config will create a project in a subdirectory of a project that already exists in some situations, which is also annoying.

I think I haven't tried that yet, pure luck? I am not sure how to prevent it ... And also, like you ask, might there be a use case for allowing it?

Anyway, for me having a whitelist of folder(s) would be a great start, and help in using DDEV, by preventing errors by blocking all folders, except those I define. So sub-folders can maybe be dealt with in a follow up issue?

About defining the rule in plain speech, I see it as "Only allow running ddev config from these folders". Since there can be more than one, it should probably be an array, like this:

allow_ddev_config: ["/home/rfay/workspace/*", "/home/rfay/tmp/*"]

How it could look in .ddev/global_config.yaml:

# Only allow running ddev config from these folders
# Separate multiple values with comma, quoted
# allow_ddev_config: ["/home/username/workspace/*", "/tmp/*"]

More thoughts about how this would work? I don't think we could encourage most of the people that need it to use it, and experienced users already know exactly what happened, and novices wouldn't know how to configure it...

The ddev config command already checks if it is run from the home folder, which it doesn't allow and returns the message "could not create new config: ddev config is not useful in your home directory (/home/username)".

https://github.com/ddev/ddev/blob/master/pkg/ddevapp/config.go#L67-L70

Maybe the allow_ddev_config setting can be added similarly?

@rfay
Copy link
Member

rfay commented Nov 2, 2023

The ddev config command already checks if it is run from the home folder, which it doesn't allow and returns the message "could not create new config: ddev config is not useful in your home directory (/home/username)".

Yes, it could be added to that check.

I think this would help me. I'm not sure it would help the general population. It would help you :)

Interested in trying a PR? There are a number of PRs out there that update global config...

@gitressa
Copy link
Contributor Author

gitressa commented Nov 2, 2023

Heh, yes it could help us :)

I have no experience with Go, so I am probably not able to create a PR, sadly ...

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

So...... this actually just happened to me :D (exactly the same thing) It was on a ~/Sites/drupal folder well nothing there was too important, yet I have to download a lot of it again.

I am curious and I think you mentioned it at some point, but why are we wiping everything on composer create-project.. I'd think there's a way to do what composer create does without that so dangerous step.

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

@gitressa

I have no experience with Go, so I am probably not able to create a PR, sadly ...

Only if this encourages you, I had no experience with Go either but it wasn't too hard to pick up.

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

Why not just doing the creation on the temp folder and then simply copy over the files.. if you wanna go "clean", do a removal of only the files and directories that were created by the composer-create command.. .but honestly, I don't see the need of running a create-project command on a non-empty ddev project.

Another opinionated approach, much safer, is to the same validation that composer does when you try to do it on an empty directory:

#composer create "drupal/recommended-project:^9" .
Creating a "drupal/recommended-project:^9" project at "./"

In CreateProjectCommand.php line 371:
                                                     
  Project directory "/var/www/html/." is not empty.  

Only that what ddev considers empty is a folder with just a .ddev directory and nothing else. Maybe also an empty configured webroot if you happened to use --create-docroot when you config it.

I don't think ddev should be in a position where it can accidentally remove unwated thing, even if it's a user error.

@rfay
Copy link
Member

rfay commented Nov 3, 2023

Related:

Why not just doing the creation on the temp folder and then simply copy over the files

That approach has potential could easily result in completely incorrect set of files. Imagine an existing vendor directory with "whatever" in it. Then you build new project and copy all the new stuff in there.

Another opinionated approach, much safer, is to the same validation that composer does when you try to do it on an empty directory:

That means that the .ddev, .tarballs, and .git directory couldn't exist, right?

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

I am working on a PR

@gitressa
Copy link
Contributor Author

gitressa commented Nov 3, 2023

@hanoii: Another opinionated approach, much safer, is to the same validation that composer does when you try to do it on an empty directory

That sounds like a pragmatic approach.

I don't think ddev should be in a position where it can accidentally remove unwated thing, even if it's a user error.

I do agree with that.

@rfay rfay changed the title ddev config can accidentally be done in the wrong directory, how to prevent that? Prevent execution of ddev config in the wrong directory Nov 3, 2023
@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

That means that the .ddev, .tarballs, and .git directory couldn't exist, right?

No, I meant that ddev can validate the emptyness of where it's running create-project, about to send a PR, actually quite happy with it. It avoids restarting ddev and all that.

@rfay
Copy link
Member

rfay commented Nov 3, 2023

But it already does create an empty directory for the installation, then copies that to the project directory. (But I do think we're getting somewhere with this conversation!)

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

@rfay following up from #5499 (comment)

And another thing to think about (another PR) is controlling ddev config as discussed in #5493

I wonder if without composer create doing any removing this is too important. This works the same as most init commands (thinking on git init, npm init and a lot others), if you config on the wrong dir, you can fix it, at most you will have files to remove, but I think adding logic to prevent that is adding more of a hassle than an improvements and I can see how that ends up letting to a lot of other support issues. Just a thought.

@rfay
Copy link
Member

rfay commented Nov 3, 2023

It definitely has messed me up at the confusion level. I use ~/workspace as base for projects. If I accidentally ddev config there then all new ddev config in subdirectories (where they belong) will fail. And of course it tells me they're failing... but I'm not looking.

Also, ddev start in a subdirectory will try to start the project from ~/workspace, leading to further confusion.

All commands other than config look for a parent project, and use the parent project. So a ddev exec in project web directory just just the right thing with the right project.

@hanoii
Copy link
Collaborator

hanoii commented Nov 3, 2023

I'd expect ddev start to start the closest project up in the tree, that the further away, same way npm works, it accepts nested projects without much of an issue. So that's a potential fix.

Then I am not sure if you should validate running ddev config inside a directory. I agree it's likely never wanted. Maybe instead of an error prompt for a warning.. it's something so rare.

@rfay
Copy link
Member

rfay commented Nov 3, 2023

It does start the closest.

@rfay
Copy link
Member

rfay commented Nov 4, 2023

One way this could be partially addressed is for ddev config to look for .ddev directories in subdirectories of the current dir, just like it looks for them currently in parent directories. This would almost certainly have some negative effects, as users do in fact do multiple projects in one repo.

@hanoii
Copy link
Collaborator

hanoii commented Nov 6, 2023

@gitressa if you can give #5499 a try that'd be awesome.

@gitressa
Copy link
Contributor Author

gitressa commented Nov 7, 2023

It works well, thanks @hanoii! There is no removal of files if I create a Drupal 10 project, go up one level, start another DDEV project, and from that folder run ddev composer create. The process stops with an error.

I ran the command from this folder, which already has a project in it:

$ tree -La 1
.
├── .ddev
├── drupal10ddev
└── web

The composer create process is stopped:

$ ddev config --project-type=drupal10 --create-docroot --docroot=web
$ ddev start
$ ddev composer create drupal/recommended-project
Failed to create project: '/home/username/test/drupal10ddev' is not allowed to be present. composer create needs to be run on a recently init project with only the following paths: [web web/sites web/sites/default web/sites/default/.gitignore web/sites/default/files web/sites/default/files/sync web/sites/default/settings.ddev.php web/sites/default/settings.php] 

I verified the already existing Drupal 10 installation, and it is still there and works well.

@rfay
Copy link
Member

rfay commented Nov 7, 2023

I remain convinced that we should try to deal with the basic problem described here, accidental ddev config in the wrong directory.

The destructive behavior is definitely the case where you do a ddev composer create in the wrong directory, but just doing ddev config in the wrong directory is still quite confusing.

@gitressa thanks so much for reviewing! It would be more useful if you reviewed in the PR though. So reviewing in #5499 would be more effective communication, and I know you're all about effective communication!

@gitressa
Copy link
Contributor Author

gitressa commented Nov 7, 2023

I also tried with just a folder with a single file in it, and that also blocked the command from running, as desired:

$ tree -La 1
.
├── .ddev
├── my-folder
└── web
$ ddev composer create drupal/recommended-project
Failed to create project: '/home/username/test/my-folder' is not allowed to be present. composer create needs to be run on a recently init project with only the following paths: [web web/sites web/sites/default web/sites/default/.gitignore web/sites/default/files web/sites/default/files/sync web/sites/default/settings.ddev.php web/sites/default/settings.php]

@gitressa
Copy link
Contributor Author

gitressa commented Nov 7, 2023

Sure @rfay, I'll comment in the PR :-)

rfay added a commit that referenced this issue Nov 22, 2023
…5499)

Co-authored-by: Randy Fay <randy@randyfay.com>
@gitressa
Copy link
Contributor Author

Fixed in #5499, thanks!

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

@gitressa let's keep this one open as I think it's still valid as far what @rfay steered it to. For that matter, I would expect this to work pretty much like npm:

  • npm install will apply to the closest parent package-json.lock
  • npm init can still be run inside a subdirectory

So I would do the same here:

  • allow ddev config to run inside subdirectories of parent projects you can maybe add a warning, or at most a prompt, but I think whatever you do, it's not destructive so you can easily delete, again, same as npm init or git init
  • consider the project root the closest config.yaml in the tree, not the further away.

@hanoii hanoii reopened this Nov 23, 2023
@gitressa gitressa changed the title Prevent execution of ddev config in the wrong directory Prevent accidentally deleting folders with ddev composer create Nov 23, 2023
@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

@gitressa sorry to keep undoing what you are doing, but don't change the title (or please change it back) so we keep this issue open for the main focus of @rfay 's

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

You can unsubscribe from this issue if you are not longer interested on the bigger scope of it.

@gitressa
Copy link
Contributor Author

gitressa commented Nov 23, 2023

No no, I just thought that the PR was fixing this issue, just with another method. I'll revert the title.

You can unsubscribe from this issue if you are not longer interested on the bigger scope of it.

Just the opposite, I am very interested :)

@gitressa gitressa changed the title Prevent accidentally deleting folders with ddev composer create Prevent execution of ddev config in the wrong directory Nov 23, 2023
@gitressa gitressa changed the title Prevent execution of ddev config in the wrong directory Prevent execution of ddev config in the wrong directory Nov 23, 2023
@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

The issue summary can probably be updated I think if we want to keep this issue open with the bigger scope, probably something for @rfay to do if he want and can, I can't

@gitressa
Copy link
Contributor Author

gitressa commented Nov 23, 2023

Feel free to post it here, and I can add it ... or perhaps you can be granted permissions to edit issue summaries?

@rfay
Copy link
Member

rfay commented Nov 23, 2023

Yes, I would still like to solve the original problem, because although I've never been bitten by the catastrophic ddev composer create problem that @hanoii has had, I've absolutely mistakenly done config in the wrong directory, and it would be really nice to sort it out. But thanks @hanoii for preventing people from having the catastrophic version again!

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

@gitressa had the catastrophic problem too! It is the issue summary after all!

@rfay
Copy link
Member

rfay commented Nov 23, 2023

So glad that can't happen any more. Thanks so much for taking on this initiative.

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

@gitressa let's keep this one open as I think it's still valid as far what @rfay steered it to. For that matter, I would expect this to work pretty much like npm:

  • npm install will apply to the closest parent package-json.lock
  • npm init can still be run inside a subdirectory

So I would do the same here:

  • allow ddev config to run inside subdirectories of parent projects you can maybe add a warning, or at most a prompt, but I think whatever you do, it's not destructive so you can easily delete, again, same as npm init or git init
  • consider the project root the closest config.yaml in the tree, not the further away.

@rfay what about this idea?

@rfay
Copy link
Member

rfay commented Nov 23, 2023

allow ddev config to run inside subdirectories of parent projects you can maybe add a warning, or at most a prompt, but I think whatever you do, it's not destructive so you can easily delete, again, same as npm init or git init

I think this might be just right, but of course it doesn't solve my problem, which is typically executing it in ~/workspace (the directory where I keep all my projects).

But ddev config in a subdirectory already detects and warns. But I think it mght be better if it just acted on the parent directory, as I think you're suggesting. Not sure a warning is needed in this case.

consider the project root the closest config.yaml in the tree, not the further away.

I believe this is already the case.

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

I believe this is already the case.

Ah ok

But ddev config in a subdirectory already detects and warns. But I think it mght be better if it just acted on the parent directory, as I think you're suggesting. Not sure a warning is needed in this case.

No, now detects it and prevents from running. I am saying that this should be allowed.

@rfay
Copy link
Member

rfay commented Nov 23, 2023

No, now detects it and prevents from running. I am saying that this should be allowed.

Agreed, as long as it operates on the parent project.

@hanoii
Copy link
Collaborator

hanoii commented Nov 23, 2023

Agreed, as long as it operates on the parent project.

I am saying the opposite. I would expect it work exactly as npm init, git init and any other init cli out there one is used to. But that's my take. If I want a ddev project inside another ddev project, so be it.

@rfay
Copy link
Member

rfay commented Nov 24, 2023

It would be easy to create one by just manually creating a .ddev/config.yaml as well, if people want that.. Good conversation!

@hanoii
Copy link
Collaborator

hanoii commented Nov 24, 2023

I still feel strongly about the above, and I have another suggestion which you will probably hate (or not :)):

  1. add a ddev init command that's pretty much ddev config but without the warning and that you can only run it on a directory where there's not a previously init ddev project. This can be run anywhere.
  2. I would like to have ddev config only work for already init configs (same as npm config), that would default to the closest project in up in the tree and not allow for creating new projects with this. So if you no ddev project is found upwards, you can say something like: There's no project found to config, please run ddev init to set one up.

If you don't like two as it means changing something so hardcoded in ddev mindset you can continue to leave it as is and allow both commands to co-exist for some time but update the docs to refere to this new command for new projects.

If you do 2, however, you are, by escence, fixing this issue, you cannot run ddev config in the wrong directory anymore.

@rfay
Copy link
Member

rfay commented Nov 25, 2023

How about ddev config --force and/or ddev config --current-directory (with the behavior of default configuring the parent project). These would be things people would never even discover, but people like you could use for your off-road adventures.

None of these solve my biggest problem, which is configuring in the ~/workspace/ddev or ~/workspace, just places I don't want them. I don't think we're going to find a solution to those necessarily.

I actually could use a ddev composer create --force, was struggling a bit with having to clean everything up all the time when testing #5489

@hanoii
Copy link
Collaborator

hanoii commented Nov 25, 2023

the thing with init or allowing config in any directory, is that even if you end up configuring a project in the workspace dir, it won't cause any harm.

Again, same as git init. You are in /workspace/a and run git init, and then you run git init in /workspace, you can continue doing git init in /workspace/b without much hindrance

@rfay
Copy link
Member

rfay commented Nov 25, 2023

the thing with init or allowing config in any directory, is that even if you end up configuring a project in the workspace dir, it won't cause any harm.

It already doesn't do "harm" in my case. Just confusion.

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

No branches or pull requests

3 participants