WordPress should not try to remove themes or plugins recursively if the directory is a symlink

[vladimir_kolesnikov]

Consider the situation: there is a server with multiple WordPress blogs hosted in it. Some plugins are common for all/many blogs and to save several (hundreds in our case) megs of the disk space, shared plugins are stored somehwere else (say, /var/www/wp-plugins) and there are symbolic links to /var/www/wp-plugins/<plugins> from /home/<user>/wp-content/plugins/<plugins>.

The onwer of the blog (user1) may not know these details and wants to update one of the plugins (plugin1) using automatic update feature. WordPress will then try to remove /home/user1/wp-content/plugins/plugin1/ recursively although /home/user1/wp-content/plugins/plugin1 is a symlink to /var/www/wp-plugins/plugin1.

The obvious solution is to add a check to the filesystem classes that checks if the file is a symlink and if so, remove symlink with unlink() instead of trying to follow it and remove everything it sees.

The advantage of this approach is that if the user symlinks a plugin to other user's data, those data will not be removed by WordPress (this can be very good for those hosts where all users are served by the same Apache user etc).

[Denis-de-Bernardy]

Seems to me that this is a use-case where site admins shouldn't be allowed to upgrade plugins, and where all installs should be maintained from the shell using svn, git, or scripts.

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

[scribu]

Another thing to note is that activation hooks won't work with sym-linked directories.

You can have a common directory for all plugins - this can be achieved using the WP_PLUGIN_DIR and WP_PLUGIN_URL constants.

Or, as Denis suggested, disable upgrades for common plugins.

[vladimir_kolesnikov]

Replying to Denis-de-Bernardy:

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

No. If the symlink is removed, only one site gets updated (the user wants to have a newer version of the plugin on their own risk). But if the whole tree is removed (what WordPress does now), ALL sites are affected and this is an unexpected result and can be security risk.

Using WP_PLUGIN_DIR/URL is not an option if the sites can have their own plugins (ie, when only a subset of plugins is shared between all installations).

[vladimir_kolesnikov]

Replying to scribu:

Another thing to note is that activation hooks won't work with sym-linked directories.

Have to disagree:

<?php
/*
Plugin Name: Test Plugin
Description: Test Plugin
Version: 1.0
*/

function test_activate()
{
        wp_die("Activation hook");
}

function test_deactivate()
{
        wp_die("Deactivation hook");
}

add_action('activate_test.php', 'test_activate');
add_action('deactivate_test.php', 'test_deactivate');

Works fine.

# pwd
/home/user1/htdocs/wp-content/plugins
# ls -la test.php
lrwxrwxrwx 1 user1 user1 37 2010-10-16 15:59 test.php -> /var/www/em-stuff/wp-plugins/test.php

[scribu]

Yes, it works because you're hardcoding the plugin basename. If you to use register_activation_hook() (which you should) it won't work.

[vladimir_kolesnikov]

Replying to scribu:

Yes, it works because you're hardcoding the plugin basename. If you to use register_activation_hook() (which you should) it won't work.

With the patch from #13550 it will work anyway.

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

[scribu]

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

That makes sense, I guess.

[vladimir_kolesnikov]

Sorry, wrong files were attached :-(

[scribu]

Replying to vladimir_kolesnikov:

Sorry, wrong files were attached :-(

It's ok, I removed them.

[dd32]

What about when the upgrade is being completed through FTP? I realise the scenario's of that are less common than someone using SSH/Direct, however I can see that question coming up later down the line

[dd32]

Symlinks are not something which we are currently willing to handle in WordPress.

In the event that you symlink common plugins into installs, you should also disable upgrades for those specific plugins.

[Dreamsorcerer]

Apparently, WordPress now supports symlinked plugins, but this still appears to be an issue.
​https://make.wordpress.org/core/2014/04/14/symlinked-plugins-in-wordpress-3-9/

What currently happens, is when updating, it says 'Warning: Could not remove the old plugin.' and fails to update. At this point, it has already deleted the contents of the old plugin, so you are left with an empty directory, and still no new plugin. Either upgrades should work through the symlink and upgrade it, or as previously suggested, it should delete the symlink without touching the contents and then install the new version.

[ocean90]

Related: #29408, #36710

[matcho]

Allow me to support this argument and initiative.

AFAIK no removal mechanism in the Unix world ever deletes the target of a symlink unless a specific option is given. While this behaviour might be discussed in the context of Wordpress, it is certainly not expected and might create unexpected large damage to the data.

Note that I'm in a use case where a homemade plugin needs a symlink to point to some application / lib; I agree this is probably not the best way to achieve what I'm looking for but again, doing this should not lead to WP deleting my data.

Thanks
Mathias

Replying to scribu:

OK, probably my use case is not good for this problem but I still think that WP should not follow symlinks when deleting directories because this can cause removal of the files the user is not expecting to have removed. And it just contradicts to Linux/UNIX tradition - if the plugin/theme/etc directory were removed with 'rm -r', 'rm' would not follow the link but would remove the symlink itself instead. Just my opinion.

That makes sense, I guess.

[Dreamsorcerer]

This is actually a fairly serious security flaw as well, e.g. if a plugin author puts a symlink in their plugin, and get it uploaded to the plugin repository.

If the plugin includes a symlink pointing to '../../..', then WP will recursively delete itself. I've tested this with a symlink to '../../themes' and WP successfully deleted all the themes while trying to upgrade the plugin. If the server is really poorly configured, then a symlink to '/' or similar might even be able to wipe out the whole server.

What's worse, is even if the plugin author managed to do something like this accidentally, and later realised their mistake, there would be no way for them to fix it. Providing any update at all to the plugin repository would trigger the deletion.

Attached is a patch which fixes the bug while also closing this security hole.

[Dreamsorcerer]

Fix symlink security flaw

[pbiron]

With 5.6 beta 1 in a few hours, punting this.

[pbiron]

refreshing patch

[pbiron]

15134.2.diff​ refreshes the patch so that it applies cleanly.

The refresh also incorporates the Windows-specific changes to ​WP_Filesystem_Direct::delete() from 36710.diff.

Now, the only difference between 15134.2.diff​ and 36710.diff is 15134.2.diff has the additional check in ​WP_Upgrader::clear_destination().

[marybaum]

Replying to vladimir_kolesnikov:

May we edit the title of this very worthwhile ticket to say the following:

WordPress should not try to remove themes or plugins recursively if the directory is a symlink

In English we do not use apostrophes to indicate plural nouns.

Thanks!

Consider the situation: there is a server with multiple WordPress blogs hosted in it. Some plugins are common for all/many blogs and to save several (hundreds in our case) megs of the disk space, shared plugins are stored somehwere else (say, /var/www/wp-plugins) and there are symbolic links to /var/www/wp-plugins/<plugins> from /home/<user>/wp-content/plugins/<plugins>.

The onwer of the blog (user1) may not know these details and wants to update one of the plugins (plugin1) using automatic update feature. WordPress will then try to remove /home/user1/wp-content/plugins/plugin1/ recursively although /home/user1/wp-content/plugins/plugin1 is a symlink to /var/www/wp-plugins/plugin1.

The obvious solution is to add a check to the filesystem classes that checks if the file is a symlink and if so, remove symlink with unlink() instead of trying to follow it and remove everything it sees.

The advantage of this approach is that if the user symlinks a plugin to other user's data, those data will not be removed by WordPress (this can be very good for those hosts where all users are served by the same Apache user etc).

[pbiron]

@marybaum done

[marybaum]

Replying to pbiron:

@marybaum done

Excellent!

[Dreamsorcerer]

Replying to marybaum:

Replying to vladimir_kolesnikov:

May we edit the title of this very worthwhile ticket to say the following:

WordPress should not try to remove themes or plugins recursively if the directory is a symlink

In English we do not use apostrophes to indicate plural nouns.

That seems very picky, but the original title was "theme's or plugin's directory". i.e. the directory owned by a theme or plugin. It was perfect English...

[jqz]

This has become more of an issue now automatic plugin/theme updates are an option, and now that on occasion a plugin author may choose to force an update to patch a security vulnerability even if automatic updates are disabled - see ​https://github.com/woocommerce/woocommerce/issues/32111 - this can lead to a broken site that could go undetected for days.

Replying to Denis-de-Bernardy:

Seems to me that this is a use-case where site admins shouldn't be allowed to upgrade plugins, and where all installs should be maintained from the shell using svn, git, or scripts.

No, it is possible have some common plugins symlinked, whilst others are updated in the usual WordPress way.

The risk is quite huge in that setup: only one site gets put on maintenance mode, even though the plugin update could be affecting dozens of sites.

I disagree. plugins/some-plugin/ can symlink to ~/shared-plugins/some-plugin/ which in turn symlinks to ~/shared-plugins/some-plugin-1.0/. Then to upgrade it is only necessary to relink ~/shared-plugins/some-plugin/ to ~/shared-plugins/some-plugin-1.1/. There is no downtime at all and therefore no need for maintenance mode.

There is a very small risk that any HTTP requests in progress at the time of the relinking may end up parsing newer versions of any PHP files it hadn't already parsed. However, that and other problems could happen anyway with the normal WordPress update procedure if a separate process was in the middle of serving an HTTP request at the time the update was triggered.

[jqz]

Bump. 11 years, no fix. Disappointing.

[costdev]

Refreshes 15134.2.diff with PHPCS fixes and a slight cleanup of a multi-line condition.

[kirasong]

This ticket was looked into today ​during a 6.0 bug scrub.

The patch still applies as expected.

Adding dev-feedback to try to get a review to help this move forward.

Also, I ​reached out to some of the folks that have been working on auto-updates, to see if a review from that team might be possible, as it overlaps with the maintainers of this component.

[pbiron]

As 6.0 is in Beta 2, moving this to 6.1 to give us more time to test the patch, especially for non-direct filesystems.

[jqz]

Setting the symlink target to read-only doesn't help. The stupid WordPress update recurses the directories and deletes the files individually.

[jqz]

This is causing a lot of issues and downtime. Please fix ASAP.

[peterwilsoncc]

@jqz This is been worked on presently and is on the 6.1 milestone. Periodically increasing the severity of the ticket and providing commentary on the tickets progress does not help, please refrain from doing so.

Trac ticket: https://core.trac.wordpress.org/ticket/15134

[kirasong]

Thanks so much for the updated PR / patch @audrasjb!

If anyone has the chance to test this to see if it resolves the issue for you, it'd be highly appreciated, as we get closer to 6.1 RC.

@audrasjb Do you have a recommended set of testing instructions? I considered providing a recommendation, but it seems like this ticket covers a variety of potential concerns, and I want to be sure it's accurate.

[audrasjb]

With WP 6.1 RC 1 scheduled today (Oct 11, 2022), there is not much time left to address this ticket. Let's move it to the next milestone to give it more time for testing.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next one or two hours, please feel free to move this ticket back to milestone 6.1.

[bgoewert]

Test Report

Patch tested: ​pr:3372

Steps to Test

Feel free to provide alternate instructions. This is just what I was able to come up with.

1. Download an older version of a plugin. Example: ​Gutenberg 14.8.4
2. Unzip to a directory other than "wp-content/plugins/". Example:

     unzip ~/Downloads/gutenberg.14.8.4.zip -d ~/wp-test/plugins
   

3. Ensure whatever directory you place the plugin into is writable by the web server. Example:

     sudo find ~/wp-test/plugins -type d -exec chmod 777 {} \; && '
     sudo find ~/wp-test/plugins -type f -exec chmod 666 {} \;
   

4. Navigate to plugin directory. Example:

     cd src/wp-content/plugins
   

5. Create symlink. Example:

     ln -s ~/wp-test/plugins/gutenberg/ gutenberg
   

6. Attempt to upgrade the plugin from the admin dashboard.

Expected Results

When reproducing the bug/defect:

• ❌ folders/files are deleted during upgrade.

When testing the patch:

• ✅ No folders/files are deleted during upgrade.

Environment

• OS: Pop!_OS
• Web Server: Docker-Desktop & wordpress-develop
• PHP: 7.4.33
• WordPress: 6.2-alpha-54642-src
• Browser: Chrome 109
• Theme: Twenty Twenty-Three
• Active Plugins:
  • Gutenberg 14.8.4

Actual Results

When reproducing the bug/defect:

• ❌ All files/folders in gutenberg/ were deleted. Plugin dashboard displays "Update failed: Could not remove the old plugin." because the files were deleted and there is nothing to remove.

When testing the patch:

• ✅ No folders/files were deleted during upgrade.

Additional Notes

• I only tested with the direct FS method and not ftp or ssh2.
• Also, it looks like ​pr:3372 currently only addresses the direct FS method.

[pbiron]

there are still some unresolved issues with this and given how late we are in the 6.2 cycle, punting this to 6.3.

[chaion07]

Thanks @vladimir_kolesnikov for reporting this. We discussed this ticket during a recent bug-scrub session. Here's the summary of the feedback received:

1. Colin volunteered to contact Paul to ask if he thinks this will get attention in the next couple of weeks and to discuss what's needed and summarize it later.
2. The ticket was punted to 6.3 just before 6.2 Beta 4. So it might have some time to get resolved.
3. As this touches the Filesystem API component, we would realy be hesitant to leave it in the milestone any longer than that.

Props to @costdev for the contribution.

[costdev]

This ticket was mentioned during the Upgrade/Install component meeting.

We still need to establish what's left to do on this one, and as it's getting late in the 6.3 cycle and the patch touches the Upgrade/Install and Filesystem components, I'm moving this to Future Release. It can be moved to a release milestone when we're confident it's ready for review.