Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#49218 closed defect (bug) (fixed)

WP_Filesystem_SSH2 ssh2_sftp_mkdir permissions

Reported by: bbrdaric's profile bbrdaric Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Filesystem API Keywords: has-patch commit
Focuses: Cc:

Description

Hello,

I would like to follow up on ticket:10304, especially comments ticket:10304#comment:1 and ticket:10304#comment:4

When installing plugins from wp-admin using SSH2 connection, I am observing unexpected behavior regarding plugin folder permissions.

My installations are mostly on VPS servers, and I am counting on group writeable permissions (775 for folders and 664 for files).

eg. on Debian Linux (bbrdaric being my user account, and wp being group with members www-data and bbrdaric )

# bbrdaric @ sandbox in /var/www/wordpress/wp-content
$ la
-rw-rw-r-- 1 bbrdaric wp   28 Jan  8  2012 index.php
drwxrwsr-x 4 bbrdaric wp 4.0K Jan 16 15:10 plugins
drwxrwsr-x 6 bbrdaric wp 4.0K Dec 18 23:16 themes
drwxrwsr-x 2 bbrdaric wp 4.0K Jan 16 15:10 upgrade
# bbrdaric @ sandbox in /var/www/wordpress/wp-content/plugins
$ la
drwxrwxr-x 4 bbrdaric wp 4.0K Dec 18 23:16 akismet
-rw-rw-r-- 1 bbrdaric wp   28 Jun  5  2014 index.php

if I install plugin hello-dolly using SSH2 connection, the result is

# bbrdaric @ sandbox in /var/www/wordpress/wp-content/plugins 
$ la
drwxrwxr-x 4 bbrdaric wp 4.0K Dec 18 23:16 akismet
drwxr-sr-x 2 bbrdaric wp 4.0K Jan 16 15:10 hello-dolly
-rw-rw-r-- 1 bbrdaric wp   28 Jun  5  2014 index.php

but I expect hello-dolly folder permissions like this

drwxrwxr-x 2 bbrdaric wp 4.0K Jan 16 15:10 hello-dolly

So then I tried installing again, but this time using WP-CLI and got this

# bbrdaric @ sandbox in /var/www/wordpress/wp-content/plugins [14:01:51] 
$ wp plugin install hello-dolly
Installing Hello Dolly (1.7.2)
...
Unpacking the package...
Installing the plugin...
Plugin installed successfully.
Success: Installed 1 of 1 plugins.

# bbrdaric @ sandbox in /var/www/wordpress/wp-content/plugins
$ la
drwxrwxr-x 4 bbrdaric wp 4.0K Dec 18 23:16 akismet
drwxrwxr-x 2 bbrdaric wp 4.0K Jan 17 14:02 hello-dolly
-rw-rw-r-- 1 bbrdaric wp   28 Jun  5  2014 index.php

Well this was what I expected at the first place, but it raised question why are directory permissions different when installing using wp-admin and SSH2?

Then I looked up, started debugging and learned how is plugin managment done, and also grasped a bit of permissions management and Filesystem API

In the process I discovered r25739 which clarified permissions to me, but I wasn't sure if this is applied when using SSH2 connection.

It turned that yes, FS_CHMOD_DIR is used in WP_Filesystem_SSH2 for creating folders, but no, this permissions are not applied as expected.

This is because ssh2_sftp_mkdir function is for some reason affected by system umask settings, and the problem is this isn't documented in the php documentation.

On the most Linux distributions, umask is set to 022 that will effectively cut group write permissions when creating files and folders, which means permissions won't be applied as wanted.

Luckily, the fix is really simple and has no side effects. In class WP_Filesystem_SSH2, in mkdir method, just after creating folder, permissions need to be set explicilty using a ssh2_sftp_chmod function which will set proper permissions as intended.

I will try to add PR/patch when I figure out how :)

Regards,
Boris.

Change History (8)

This ticket was mentioned in PR #171 on WordPress/wordpress-develop by bbrdaric.


4 years ago
#1

Explicitly set proper directory permissions as intended when using ssh2 connection (for example when installing plugin or theme).

Fixes https://core.trac.wordpress.org/ticket/49218

#2 @bbrdaric
4 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by bbrdaric. View the logs.


4 years ago

This ticket was mentioned in Slack in #core by bbrdaric. View the logs.


4 years ago

#5 @SergeyBiryukov
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

Hi there, welcome to WordPress Trac!

Thanks for the patch and a very detailed bug report. The fix seems reasonable at a glance.

This reminded me of #40572, which may or may not be related.

#6 @SergeyBiryukov
4 years ago

  • Keywords commit added

#7 @SergeyBiryukov
4 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 48090:

Filesystem API: Expicitly set directory permissions in WP_Filesystem_SSH2::mkdir().

This ensures the permissions are correct after a directory is created with ssh2_sftp_mkdir(), which appears to be affected by system umask settings.

Props bbrdaric.
Fixes #49218.

#8 @bbrdaric
3 years ago

I also wanted to make sure this gets mentioned in the PHP documentation, and now it is 🎉

https://bugs.php.net/bug.php?id=79136

https://www.php.net/manual/en/function.ssh2-sftp-mkdir.php

Note: See TracTickets for help on using tickets.