[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:
-----------------------------------------+------------------------------

Comment (by jobst):

 @costdev thanks for the reply.

 I did not see the "->connect()" call (in ssh2) when I first read the code,
 I saw it now, then traced the problem places.

 I see that quite a number of plugins I am using (astra, elementor (pro),
 ultimate plugins etc) all call 'WP_Filesystem()' without any parms at all
 while some others themes/plugins (enable-media-replace, filebird,
 wordfence) all call 'WP_Filesystem($creds)' correctly.

 Now, considering this to be ssh2, the constructor of WP_Filesystem_SSH2
 SHOULD make sure to call "request_filesystem_credential()" in case NO
 PARAMETERS were passed - this too would get rid of many problems, and I
 can tell you I will not be the only one but I am one who SAW the problem.

 A simple test could be done to check whether the parms passed to the
 contructor are empty, and if they are empty a call to
 "request_filesystem_credential()" should be done.

 I would have thought this would be the proper way, after all ssh2 needs
 credentials. Anyone using the SSH2 filesystem method KNOWS they have to
 provide the SSH2 details in wp-config.php.

 My dilemma is the errors created by the call to 'WP_Filesystem()' will
 NEVER pop up anywhere (they are truly swallowed!) so any developer
 creating plugins/themes will not see incorrect coding practices - only if
 they care to read the PHP logs.

 My other dilemma is when the port is NOT specified as a parameter in the
 constructor, a default of 22 is set in the constructor.
 Why not doing this for all other parameters as well instead of raising an
 error?

 Until today I would have thought the SSH2 filesystem method would setup
 the required parameters automatically instead of setting an ERROR.

 Here is an example what I would do (for hostname only, all others would be
 the same)
 {{{

 public function __construct( $opt = '' ) {
   $this->method = 'ssh2';
   /* some lines cut */

   // In the original code the port is filled with a number if the
 parameters passed are empty
   // why no doing this for the other values required for a SSH connection?
   if ( empty( $opt['port'] ) ) {
     $this->options['port'] = 22;
   } else {
     $this->options['port'] = $opt['port'];
   }

   // go and get what we can find in DB and/or wp-config.php
   // this will set any KEY not found to '', so we can test for that
   $ajaxURL = admin_url('admin-ajax.php');
   $credentials = request_filesystem_credentials($ajaxURL, 'ssh', false,
 ABSPATH )

   // now fill ALL the credentials with the data required for a proper
 connection
   // I am just giving ONE example
   if ( empty( $opt['hostname'] ) ) {
     if ( $credentials['hostname'] == '' ) {
       // by all means create/raise an error here, that's fine
       $this->errors->add( 'empty_hostname', __( 'SSH2 credentials
 (hostname) are required!' ) );
     } else {
       // assign what has been found in the DB and/or wp-config.php
       $this->options['hostname'] = $credentials['hostname'];
     }
   } else {
     $this->options['hostname'] = $opt['hostname'];
   }

   /* lots of other lines cut */
 }

 }}}

 Point is that I - as a user of class-wp-filesystem-ssh2.php - would expect
 the constructor to SET the required credentials as I provided them in wp-
 config.php.

 It might also be what all the developers of plugins/themes are thinking
 who call 'WP_Filesystem()' without parameters - and there are plenty.

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