[wp-trac] [WordPress Trac] #58541: WP_Filesystem_SSH2:put_contents (and others) does not check for $sftp_link to be up

[WordPress Trac]

#58541: WP_Filesystem_SSH2:put_contents (and others) does not check for $sftp_link
to be up
-----------------------------------------+------------------------------
 Reporter:  jobst                        |       Owner:  (none)
     Type:  defect (bug)                 |      Status:  new
 Priority:  normal                       |   Milestone:  Awaiting Review
Component:  Filesystem API               |     Version:
 Severity:  major                        |  Resolution:
 Keywords:  has-patch reporter-feedback  |     Focuses:
-----------------------------------------+------------------------------
Changes (by costdev):

 * keywords:  has-patch => has-patch reporter-feedback


Comment:

 Hi @jobst, thanks for opening this ticket!

 > In the file ABSPATH/wp-admin/includes/file.php (around line 2051) the
 function WP_Filesystem() simply sets up an instance of the class defined
 by FS_METHOD, but **does NOT connect if FS_METHOD is set to 'ssh2'**.

 `WP_Filesystem()` calls the `::connect()` method
 [https://github.com/WordPress/wordpress-develop/blob/6.2.2/src/wp-
 admin/includes/file.php#L2101 here].

 The only scenarios in which this won't be called are:
 - When `get_filesystem_method()` returns a falsy value.
 [https://github.com/WordPress/wordpress-develop/blob/6.2.2/src/wp-
 admin/includes/file.php#L2056-L2060 Ref]
 - When a non-existent filesystem abstraction class is passed to the
 `filesystem_method_file` filter hook. [https://github.com/WordPress
 /wordpress-develop/blob/6.2.2/src/wp-admin/includes/file.php#L2077 Ref]
 - When the filesystem contains errors. [https://github.com/WordPress
 /wordpress-develop/blob/6.2.2/src/wp-admin/includes/file.php#L2097 Ref]

 As you mentioned that `$wp_filesystem` is being set to an instance of
 `WP_Filesystem_SSH2`, it appears that `WP_Filesystem()` is returning early
 in your environment due to the last scenario above.

 After calling `WP_Filesystem()`, can you try dumping
 `$wp_filesystem->errors->get_error_messages()` and see what errors are
 occurring? These should point to potential issues in your configuration
 that would need to be resolved.

 -----

 Regarding adding checks to `::put_contents()`, most/all of the Filesystem
 API methods and Filesystem API general functions (`move_dir()`,
 `copy_dir()`, etc.) assume that the filesystem has been correctly set up
 and was able to connect without errors.

 Developers may be using any number of Filesystem API methods. If we
 patched these methods to add checks, each run of these methods would be
 needlessly checking if the filesystem is setup, rather than the developer
 checking it once at the start and stopping if setup and connection failed.

 They're already checking whether `$wp_filesystem` is falsy/empty, so they
 should follow this up by checking the return value of `WP_Filesystem()`.

 i.e.

 {{{#!php
 <?php

 global $wp_filesystem;

 if ( ! $wp_filesystem ) {
     require_once ABSPATH . 'wp-admin/includes/file.php';

     if ( ! WP_Filesystem() || ! $wp_filesystem->put_contents( $file,
 $contents ) ) {
         return new WP_Error( 'writing_file_failed', __( 'Failed to write
 to the file.' ), $file );
     }
 }
 }}}

 -----

 Props to @afragen for rubberducking on this.

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