Direct filesystem chmod emits warnings even if the permissions already match

[redsweater]

When the WordPress Direct Filesystem is asked to change the permissions of a file that it doesn't own, php will emit warnings even if the permissions requested are already set on the file.

This affects WordPress installations where the ownership of files in the WordPress directory are kept separate from the web process that runs WordPress.

In this scenario, although WordPress may have every permission it needs or expects, thanks to a given file's permissions matching the implicit or explicit FS_CHMOD_FILE variable, it still complains because of the underlying lack of ownership.

I discovered this issue when I noticed PHP warnings in my log stemming from the Automattic JetPack Protect plugin attempting to update its own IP Rules files. One way to reproduce this is by using WP-CLI in conjunction with the Jetpack Protect plugin:

1. Create WordPress installation in which the owner of all files is not the web hosting user, but the web hosting user, for example www-data, has all the expected permissions (for example 0775).
2. Install Jetpack Protect.
3. Confirm the Jetpack Protect files, for example its ./wp-content/jetpack-waf/rules/allow-ip.php, have the expected matching permissions (for example 0775).
4. Trigger the Jetpack Protect update rules cron task as the www-data user:

sudo -u www-data WP_DB_PASSWORD=mypassword wp cron event run jetpack_waf_rules_update_cron

Expected: that the cron rules would run without warnings.

Actual: PHP warnings are emitted:

Warning: chmod(): Operation not permitted in .../wp-admin/includes/class-wp-filesystem-direct.php on line 173

I realize these are just warnings but for those administrators who like to keep a cleaning warning log, it would be nice to avoid emitting these spurious log lines. I have been noticing these on my WordPress installation for years but only finally got around to searching out the source of the issue today.

I have a PR to submit as soon as this ticket is published.

This PR includes changes to the core WordPress direct filesystem API that prevent warnings being emitted when chmod is invoked on a file that would cause the existing mode of the file to be redundantly set. This prevents spurious warnings when the web process doesn't have ownership over the files, and thus can't chmod them, even if only to re-assert the existing mode.

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

[redsweater]

I am a little naive about PHP, so I approached this by trying to avoid calling chmod, the function that ultimately issues the warning. But I have since learned about @ for suppressing warnings, and maybe simply adding @ to the chmod call would be suitable?

diff --git a/src/wp-admin/includes/class-wp-filesystem-direct.php b/src/wp-admin/includes/class-wp-filesystem-direct.php
index ed22a821a1..6bd502a92d 100644
--- a/src/wp-admin/includes/class-wp-filesystem-direct.php
+++ b/src/wp-admin/includes/class-wp-filesystem-direct.php
@@ -170,7 +170,7 @@ class WP_Filesystem_Direct extends WP_Filesystem_Base {
                }
 
                if ( ! $recursive || ! $this->is_dir( $file ) ) {
-                       return chmod( $file, $mode );
+                       return @chmod( $file, $mode );
                }
 
                // Is a directory, and we want recursive.

In light of the slight complication with my PR needing to clear the stat cache after calling fileperms I could see how simpler might be better.

Also understand if suppressing warnings isn't a high priority! I can suppress it in my own installs as needed.

[SergeyBiryukov]

Hi there, thanks for the PR!

Replying to redsweater:

I am a little naive about PHP, so I approached this by trying to avoid calling chmod, the function that ultimately issues the warning. But I have since learned about @ for suppressing warnings, and maybe simply adding @ to the chmod call would be suitable?

That would not be the preferred approach here, as it would go against the WordPress Coding Standards, see [45611].

I think we can go ahead with the proposed PR, moving for 7.0 consideration.

[SergeyBiryukov]

In 61601:

Filesystem API: Avoid chmod() warnings if the permissions already match.

This prevents spurious warnings from WP_Filesystem_Direct::chmod() when the web process doesn't have ownership over the files, and thus cannot change the permissions, even if only to reassert the existing mode.

Follow-up to [6779], [11667], [12998].

Props redsweater, mukesh27, SergeyBiryukov.
Fixes #64610.

[redsweater]

Thank you for accepting the PR!