[wp-trac] [WordPress Trac] #51340: Stop chmodding files and folders

[WordPress Trac]

#51340: Stop chmodding files and folders
----------------------------+------------------------------
 Reporter:  malthert        |       Owner:  (none)
     Type:  defect (bug)    |      Status:  new
 Priority:  normal          |   Milestone:  Awaiting Review
Component:  Filesystem API  |     Version:  5.3
 Severity:  major           |  Resolution:
 Keywords:  dev-feedback    |     Focuses:
----------------------------+------------------------------

Comment (by costdev):

 @kkmuffme I see your point regarding malware, however it's likely that
 errors due to `chmod()` calls will be the least of the concerns if malware
 has had that level of access.

 > @azaozz Hmmm, not sure if returning true when chmod() was not run is
 good?

 In the `wp_chmod()` suggestion you posted, hooking `wp_chmod` and
 returning a non-`false` value would return `true`. In most if not all
 cases, a check would be on `if ( ! wp_chmod() ) { return new WP_Error()
 }`, so I think `true` is an appropriate return value, whether for a short-
 circuit filter, or if `chmod()` was disabled.

 While a filter would add flexibility for those who need it, I'm mindful
 that this same flexibility would mean that the purpose of this ticket -
 Not running `chmod()` at all - could be overwritten by a callback with a
 later priority. What are your thoughts on this @azaozz?

 `is_readable()`, `is_writable()` and `function_exists()` may be sufficient
 in some circumstances. However, other cases such as
 `WP_Filesystem_Direct::chmod()`, which accepts permissions as an octal
 integer, won't know what is being requested, and `is_readable()` wouldn't
 account for all possible read permissions that might be desired.

 -----

 With the above in mind, I wonder if something like this would be
 appropriate:

 {{{#!php
 <?php

 function wp_chmod( $path, $permissions, $context = '' ) {
     /*
      * If chmod() is not available, assume this has been disabled
      * due to alternative server configuration, such as umask,
      * and that server administrators have configured things correctly.
      *
      * Return true to ensure that boolean checks treat the operation
      * as a success.
      */
     if ( ! function_exists( 'chmod' ) ) {
         return true;
     }

     // 511 = 8^3. PHP treats 0777 as strictly equal to 511.
     if ( ! is_int( $permissions ) || $permissions < 0 || $permissions >
 511 ) {
         trigger_error(
             sprintf(
                 /* translators: %s: The '$permissions' argument. */
                 __( 'The %s argument must be a valid octal integer' ),
                 '$permissions'
             )
         );

         return false;
     }

     $changed = apply_filters( 'wp_chmod', false, $path, $permissions,
 $context );

     if ( false !== $changed ) {
         return true;
     }

     return chmod( $path, $permissions );
 }

 }}}

 - If the server admin doesn't want `chmod()` to run, they can disable
 `chmod()` in the `disable_functions` directive (either globally, or just
 for WordPress sites).
 - If a plugin wants to have its own management of this, it can hook
 `wp_chmod` and act accordingly.
 - If nothing is otherwise blocking `chmod()`, run `chmod()`.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/51340#comment:16>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform