WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#15134 closed defect (bug) (maybelater)

WordPress should not try to remove theme's or plugin's directory recursively if the directory is a symlink

Reported by: vladimir_kolesnikov Owned by: dd32
Milestone: Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

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

Attachments (1)

filesystem.patch (1.7 KB) - added by vladimir_kolesnikov 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 follow-up: @Denis-de-Bernardy5 years ago

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.

comment:2 follow-up: @scribu5 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.

comment:3 in reply to: ↑ 1 @vladimir_kolesnikov5 years ago

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

comment:4 in reply to: ↑ 2 @vladimir_kolesnikov5 years ago

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

comment:5 follow-up: @scribu5 years ago

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

comment:6 in reply to: ↑ 5 @vladimir_kolesnikov5 years ago

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 http://core.trac.wordpress.org/ticket/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.

comment:7 @scribu5 years ago

  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:8 @scribu5 years ago

  • Owner set to dd32
  • Status changed from reopened to assigned

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.

comment:9 follow-up: @vladimir_kolesnikov5 years ago

Sorry, wrong files were attached :-(

comment:10 @scribu5 years ago

  • Keywords has-patch added

comment:11 in reply to: ↑ 9 @scribu5 years ago

Replying to vladimir_kolesnikov:

Sorry, wrong files were attached :-(

It's ok, I removed them.

comment:12 @dd325 years ago

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

comment:13 @dd324 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

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.

Note: See TracTickets for help on using tickets.