Opened 14 years ago
Last modified 17 months ago
#15134 accepted defect (bug)
WordPress should not try to remove themes or plugins recursively if the directory is a symlink
Reported by: | vladimir_kolesnikov | Owned by: | pbiron |
---|---|---|---|
Milestone: | Future Release | Priority: | normal |
Severity: | normal | Version: | |
Component: | Upgrade/Install | Keywords: | has-patch needs-testing dev-feedback has-testing-info |
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 (4)
Change History (69)
#2
follow-up:
↓ 4
@
14 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.
#3
in reply to:
↑ 1
@
14 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).
#4
in reply to:
↑ 2
@
14 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
#5
follow-up:
↓ 6
@
14 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.
#6
in reply to:
↑ 5
@
14 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.
#7
@
14 years ago
- Milestone set to Awaiting Review
- Resolution wontfix deleted
- Status changed from closed to reopened
#8
follow-up:
↓ 16
@
14 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.
#12
@
14 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
#13
@
14 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.
#14
@
8 years ago
- Resolution maybelater deleted
- Status changed from closed to reopened
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.
#16
in reply to:
↑ 8
@
8 years ago
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.
#17
@
7 years ago
- Severity changed from normal to major
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.
This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.
4 years ago
#19
@
4 years ago
- Milestone changed from Awaiting Review to 5.6
- Owner changed from dd32 to pbiron
- Status changed from reopened to accepted
This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.
4 years ago
#21
@
4 years ago
- Milestone changed from 5.6 to Future Release
With 5.6 beta 1 in a few hours, punting this.
#22
@
4 years ago
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().
This ticket was mentioned in Slack in #core-auto-updates by pbiron. View the logs.
4 years ago
This ticket was mentioned in Slack in #core-test by pbiron. View the logs.
3 years ago
This ticket was mentioned in Slack in #core by pbiron. View the logs.
3 years ago
#27
in reply to:
↑ description
;
follow-ups:
↓ 28
↓ 30
@
3 years ago
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).
#28
in reply to:
↑ 27
;
follow-up:
↓ 29
@
3 years ago
- Summary changed from WordPress should not try to remove theme's or plugin's directory recursively if the directory is a symlink to WordPress should not try to remove themes or plugins recursively if the directory is a symlink
@marybaum done
#30
in reply to:
↑ 27
@
3 years ago
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...
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-test by boniu91. View the logs.
3 years ago
#34
in reply to:
↑ 1
@
3 years ago
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.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
3 years ago
This ticket was mentioned in Slack in #core-auto-updates by mike. View the logs.
3 years ago
@
3 years ago
Refreshes 15134.2.diff with PHPCS fixes and a slight cleanup of a multi-line condition.
#38
@
3 years ago
- Keywords dev-feedback added
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.
This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.
3 years ago
#40
@
3 years ago
- Milestone changed from 6.0 to 6.1
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.
This ticket was mentioned in Slack in #core-auto-updates by afragen. View the logs.
2 years ago
#42
@
2 years ago
- Severity changed from critical to blocker
Setting the symlink target to read-only doesn't help. The stupid WordPress update recurses the directories and deletes the files individually.
#44
@
2 years ago
- Severity changed from blocker to normal
@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.
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-auto-updates by costdev. View the logs.
2 years ago
This ticket was mentioned in Slack in #core by audrasjb. View the logs.
2 years ago
This ticket was mentioned in PR #3372 on WordPress/wordpress-develop by audrasjb.
2 years ago
#49
Trac ticket: https://core.trac.wordpress.org/ticket/15134
This ticket was mentioned in Slack in #core by chaion07. View the logs.
2 years ago
#51
@
2 years ago
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.
#52
@
2 years ago
- Milestone changed from 6.1 to 6.2
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.
This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-upgrade-install by afragen. View the logs.
2 years ago
This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.
2 years ago
#57
@
23 months ago
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.
- Download an older version of a plugin. Example: Gutenberg 14.8.4
- Unzip to a directory other than "wp-content/plugins/". Example:
unzip ~/Downloads/gutenberg.14.8.4.zip -d ~/wp-test/plugins
- 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 {} \;
- Navigate to plugin directory. Example:
cd src/wp-content/plugins
- Create symlink. Example:
ln -s ~/wp-test/plugins/gutenberg/ gutenberg
- 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 notftp
orssh2
. - Also, it looks like pr:3372 currently only addresses the
direct
FS method.
This ticket was mentioned in Slack in #core by costdev. View the logs.
22 months ago
#60
@
22 months ago
- Milestone changed from 6.2 to 6.3
there are still some unresolved issues with this and given how late we are in the 6.2 cycle, punting this to 6.3.
This ticket was mentioned in Slack in #core by chaion07. View the logs.
18 months ago
#62
@
18 months ago
Thanks @vladimir_kolesnikov for reporting this. We discussed this ticket during a recent bug-scrub session. Here's the summary of the feedback received:
- 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.
- The ticket was punted to 6.3 just before 6.2 Beta 4. So it might have some time to get resolved.
- 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.
This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.
18 months ago
This ticket was mentioned in Slack in #core-upgrade-install by costdev. View the logs.
17 months ago
#65
@
17 months ago
- Milestone changed from 6.3 to Future Release
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.
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.