wpmu_delete_blog function clears the whole uploads directory

[katazina]

Hi,

I have a wordpress multisite, which was installed a long time ago, when blogs data was saved to the blogs.dir directory.
The wordpress core is always up to date, but in the documentation was described, that we have to keep the blogs.dir directory when we updated the core.
So my wordpress version is now the latest 4.0 and I still have the blogs.dir. And all media of all blogs is there.
Directory structure like:

- blogs.dir
  |- 12 <-- this is the id of the blog
     |- 2014
       |- 01
       |- 02
       |- ...
  |- 13
     |- 2013
     |- 2014
  |- ...

The problem is with site (blog) deleting.

What I do:

• I'm on the /wp-admin/network/sites.php
• I click on the "Delete" link under a selected blog.
• Then I'm on the delete confirmation page: /wp-admin/network/sites.php?action=confirm&action2=deleteblog&id=...

If I click once on the comfirm button, then everythings works just fine.
But it could happen accidentally, that you click twice on that button and here comes the problem, becuase then everything in the uploads directory will be deleted.

I tried it with a fresh new multisite install. I donwloaded an old version of wp, when blogs.dir was used, then updated the wp to the latest version. I created multiple blogs, then deleted one and clicked twice on the confirm button. Same happened, the whole uploads directory cleared.

It's not a new bug, I just didn't know up to now where to report it.

I'm not entirely sure, that the bug is in the wpmu_delete_blog function, but it looks like, because if I click on the confirm button second time, then the blog data doesn't exists anymore, so the function fails.
And then in the wp-admin/includes/ms.php from line 124 will clear the whole content of the uploads directory.

[SergeyBiryukov]

Thank you for the report, we should definitely fix this.

Also mentioned in comment:26:ticket:29692 (as a side effect of another issue).

[katazina]

Thanks, it's good news.

[jeremyfelt]

Confirmed, this traces back to the ms-files removal in [21823], specifically the switch from a hard coded blogs.dir uploads URL when deleting a site to the use of wp_upload_dir().

wp_upload_dir() doesn't do any check to see if a site exists and therefor the non-existent options to determine site URL and upload path return false and result in the default uploads directory being returned. This kind of makes sense because when one wants a path to upload to, we should return something.

This doesn't make sense for wpmu_delete_blog because if one wants a path to delete, it should be the right path.

We already use get_blog_details() here. I think we can use that to determine if a site exists before attempting to delete its upload directory.

[nofearinc]

Was just looking into that as well, after the blog is marked as delete, but using the same URL and confirming, the $dh is for example /wp-content/uploads/2013 - getting the base there returns the uploads, and then it's iterated over and deleted completely.

Given the folder structure I'd say that we could add a conditional and skip the process if uploads is returned instead of the blog ID.

[jeremyfelt]

@katazina - I forgot to mention - this is an excellent bug report, thank you!

@nofearinc - There may be something in a check for the uploads dir. I'd like to do some more testing with what happens on a site that has defined a custom uploads URL and path.

Current thoughts:

• This can happen with or without ms-files enabled. This means we can't use that as a check.
• I'm not sure that we can rely on a hard coded path check like we did when we knew that blogs.dir existed in the structure. It may be enough to check for blog ID, but it may not.
• I think we can be confident in the get_blog_details() response providing $blog and using get_option( 'siteurl' ) to provide the $blog->siteurl property. If that site URL is not available, I think we can assume that the wrong path will be found by wp_upload_path().

[katazina]

@jeremyfelt - thanks

The first and simpliest thing to do is with javascript: disable or hide the confirm button after it is clicked.
But in the wpmu_delete_blog() it is still necessary to check whether the blog exists or not.

And should be also checked in the function that the $blog_id parameter is empty or not. I don't find anything what handles that.

Also in the $blog = get_blog_details( $blog_id ); line if the $blog_id is empty, then we got the current site in the $blog variable, which could be the "main or root" blog, so because of that and for other reasons it should be checked immediately after that line and not just later in line 88.

[jeremyfelt]

Replying to jeremyfelt:

• This can happen with or without ms-files enabled. This means we can't use that as a check.

To clarify - this is most likely with ms-files enabled. It will also happen if a site's options are not available and upload_path returns empty. We can test the second scenario, I'm not sure about the first.

• I think we can be confident in the get_blog_details() response providing $blog

This issue is more noticeable with WP_DEBUG on, as a bunch of notices are thrown on the second use of wpmu_delete_blog(). We do a sanity check to compare $blog->path and $blog->domain with the current network's data, but we don't ever check for the existence of $blog.

I don't think we'll want to change anything with wp_upload_dir(), as it should fallback to a standard uploads directory for uploading files if nothing else can be found.

I think we're good here if we add the check for $blog as a valid object to ensure we aren't trying to delete something that isn't there. A check for the existence of the upload_path option would also be sane, as wp_upload_dir() requires this to even have a chance at giving us the correct directory.

Also - this is unfortunate embarassing - #26920 reported the same issue 9 months ago and I was unable to reproduce. I'm going to close that as a duplicate of this one since more progress has been made. Sorry, @npetetin!

[jeremyfelt]

#26920 was marked as a duplicate.

[jeremyfelt]

In 30121.diff:

• Test for the existence of $blog before trying to drop anything. If get_blog_details() has returned false, we should be able to assume that no dropping will take place.
• Test for a non empty upload_path if ms_files_rewriting is enabled. If upload_path is empty in this scenario, we cannot be confident in the response of wp_upload_dir().
• Move the call to wp_upload_dir() above the deletion of the site's options tables. If the upload_path option is not cached and the tables are deleted, an empty upload_dir will be provided which results in the deletion of the main site files.
• Add unit tests for a double wpmu_delete_blog() fire.

BUT

As soon as we set ms_upload_constants() here, another test fails as things are polluted all over the place. I've commented those out for now - it may be worth setting up another tests file for ms_files_rewriting specific tests.

[jeremyfelt]

Replying to jeremyfelt:

As soon as we set ms_upload_constants() here, another test fails as things are polluted all over the place. I've commented those out for now - it may be worth setting up another tests file for ms_files_rewriting specific tests.

I think #30256 is the answer for this.

[jeremyfelt]

30121.2.diff expands the tests a bit to include ms-files specific tests from #30256. I tried and tried to make this work in separate processes, but no go yet. For now, tests should be run with the --group ms-files flag to initiate.

This is looking pretty solid and has been testing great. Tests fail as expected without the patch.

[jeremyfelt]

In 30404:

Prevent wpmu_delete_blog from removing the wrong uploads directory

wp_upload_dir() includes some logic to fall back to the default site's upload directory if a specific directory for the requested site cannot be found. Because of this, if wpmu_delete_blog() is fired twice in a row for the same site, the main site's upload directory could be deleted as well.

This adds some checks in wpmu_delete_blog() so that we are confident in the site and it's upload directory's existence before dropping the site. Tests are added for when ms_files_rewriting is enabled or disabled.

Fixes #30121