WP_Filesystem_SSH2:put_contents (and others) does not check for $sftp_link to be up

[jobst]

This is a bit long, as I need to explain the reason why it is a problem not to check for the link '$sftp_link' to be up.

In short: WordPress allows choosing between various FS_METHODS (wp-config.php), e.g. 'direct' or 'ssh2'. While neither choice will affect WordPress updating itself at all, it has implications when some plugins updating files writing content to a file (htaccess, css etc) via

$wp_filesystem->put_contents($file, $content);

The function put_contents should check whether the link is up.

There is a big difference how one needs to setup the '$wp_filesystem' instance if you use 'direct' or 'ssh2' - the first one does not need to connect, the second needs to setup a connection before being able to write.

For FS_METHODS 'direct':

  global $wp_filesystem;
  if(empty($wp_filesystem))
  {
    require_once ABSPATH . '/wp-admin/includes/file.php';
    WP_Filesystem();
  }
  $wp_filesystem->put_contents($file, $content);

For FS_METHODS 'ssh2':

  global $wp_filesystem;
  if(empty($wp_filesystem))
  {
    require_once ABSPATH . '/wp-admin/includes/file.php';
    WP_Filesystem();
    // this is the ONLY difference to 'direct'
    $wp_filesystem->connect();
  }
  $wp_filesystem->put_contents($file, $content);

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'.

Now many plugins that need to write a file (css,htacess,etc) simply assume that FS_METHOD is set to 'direct' or even assume WP_Filesystem() will connect as well.

I have three plugins (there are more, but these are the ones I am 100% sure) that have problems writing

• Ultimate Addons for Elementor
• Astra Addons
• Sensei

Now I could tell those developers to do it properly.

However I think the function $wp_filesystem->put_contents() should CHECK whether the link is up and if NOT, call a function within the class and setup the link to the server, after all I would consider this is proper coding pratice.

  public function put_contents( $file, $contents, $mode = false ) {

    // so this is for people who come from the outside
    // just setting up the class and dont care whether
    // a call to "connect" is required.
    error_log("class-wp-filesystem-ssh2.php -> put_contents -> $file ");
    if(!$this->sftp_link)
    {
      error_log("class-wp-filesystem-ssh2.php link is null, connecting ....");
      // this function is similar to connect
      $rc = $this->build_options_connect();
    }

    // put the contents
    $ret = file_put_contents( $this->sftp_path( $file ), $contents );

    if ( strlen( $contents ) !== $ret ) {
      return false;
    }

    $this->chmod( $file, $mode );

    return true;
  }

The function $this->build_options_connect() sets up the required data structure similar to the function "request_filesystem_credentials()" in file ABSPATH/wp-admin/includes/file.php (around line 2250) and then sets up the connection similar to the function $wp_filesystem->connect() in file ABSPATH/wp-admin/includes/class-wp-filesystem-ssh2.php (around line 120).

I have done this on all of my servers for a few weeks now.
Message like this one example (of many) below have completely disappeared.

[10-Jun-2023 18:25:12 UTC] PHP Warning:  file_put_contents(ssh2.sftp:///HIDDEN/htdocs/wp-content/uploads/uael_uploads/.htaccess): failed to open stream: operation failed in /HIDDEN/htdocs/wp-admin/includes/class-wp-filesystem-ssh2.php on line 283

While I stated 'has patch' (I do), let's first see what people say about this.

[costdev]

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 ​here.

The only scenarios in which this won't be called are:

• When get_filesystem_method() returns a falsy value. ​Ref
• When a non-existent filesystem abstraction class is passed to the filesystem_method_file filter hook. ​Ref
• When the filesystem contains errors. ​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

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.

[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.

[costdev]

Hi @jobst, thanks for getting back to me on this!

I can call WP_Filesystem() without any parameters and with the correct constants set in wp-config.php, WP_Filesystem() connects just fine and $wp_filesystem->put_contents() works.

In case you didn't see this in my last comment:

After calling WP_Filesystem(), can you try dumping $wp_filesystem->errors->get_error_messages() and see what errors are occurring?

If you can then post the value that's dumped into a comment here, that would be great!

[jobst]

Sorry I prepared the message but I think I forgot to paste it ...

Just before we continueI just want to be sure ... the error (in the php file) is caused in a plugin (ultimate elementor), do you want to get the dump straight after THEIR call?

[costdev]

Hi @jobst, yeah that would be great!

[jobst]

Hi @costdev

As I expected none of the required parms for the connection are initiated when you call 'WP_Filesystem()' without parms.

I have read the code over and over again, I checked what happens when people pass the proper values. When you pass credentials, all works correctly, if you do not pass credentials a call to

 $wp_filesystem->put_contents($file, $content);

will fail.

Below is one of the problems I have to show passing without credentials WILL fail.
'pinf' is a function I wrote which can take arrays/objects/strings/whatever and knows what to do with it.

private static function create_files() {
  // Allow us to easily interact with the filesystem.
  require_once ABSPATH . 'wp-admin/includes/file.php';
  pinf("============================================= this is JUST before");
  global $wp_filesystem;
  WP_Filesystem();
  pinf($wp_filesystem, "============================================= this is JUST after");

  // Install files and folders for uploading files and prevent hotlinking.
  $upload_dir = wp_upload_dir();
  $files      = array(
    'base'    => $upload_dir['basedir'] . '/uael_uploads',
    'file'    => '.htaccess',
    'content' => 'deny from all',
  );
  pinf($files,"UAEL -> modules/display-conditions/module.php");

  if ( wp_mkdir_p( $files['base'] ) && ! file_exists( trailingslashit( $files['base'] ) . $files['file'] ) ) {
    $wp_filesystem->put_contents( $files['base'] . '/' . $files['file'], $files['content'], FS_CHMOD_FILE );
  }
}

Below is the printout of the pinf function JUST after the call 'WP_Filesystem();' with no parms passed.
The port number and method are correctly assigned,m everything else is empty - expected, there is no way after I read all the code that hostname etc will have any values.

The error it produces (note this is the wp_filesytem object):

[20-Jun-2023 01:14:11 UTC] ============================================= this is JUST after WP_Filesystem_ssh2 Object
(
    [link] =>
    [sftp_link] =>
    [keys] =>
    [verbose] =>
    [cache] => Array
        (
        )

    [method] => ssh2
    [errors] => WP_Error Object
        (
            [errors] => Array
                (
                    [empty_hostname] => Array
                        (
                            [0] => SSH2 hostname is required
                        )

                    [empty_username] => Array
                        (
                            [0] => SSH2 username is required
                        )

                    [empty_password] => Array
                        (
                            [0] => SSH2 password is required
                        )

                )

            [error_data] => Array
                (
                )

            [additional_data:protected] => Array
                (
                )

        )

    [options] => Array
        (
            [port] => 22
        )

)

My wp-config.php file is correct, I am 100% sure, below are some of the lines.
Also I use KEYS, not a password.

define('FS_METHOD',       'ssh2');
define('FTP_USER',        'REDACTED');
define('FTP_PASS',        '');
define('FTP_HOST',        '127.0.0.1:22');

[costdev]

Thanks for posting this @jobst!

It's important to have this detail on the ticket for historical purposes. That way, years from now, the exact issue and data can be seen without needing to review code/install an old version to reproduce.

As we don't yet have a testable patch (i.e. a .diff, .patch or a PR), I'll remove has-patch and add needs-patch. Removing reporter-feedback and milestoning for Future Release until we have a patch and test reports from someone other than the patch author.

In the meantime, for example, a Must-Use plugin that initializes the $wp_filesystem global variable as a WP_Filesystem_SSH2 object with valid arguments for the system, followed by a call to wp_filesystem->connect() may ensure the later calls by plugins work as expected, as ! $wp_filesystem conditions should fail and its current (correct) value should be used used.

[costdev]

Also pinging @dd32, @azaozz and @pbiron for their thoughts on this.

[jobst]

I will make a patch file based on the code below for all the required parameters to be placed INTO the constructor of the class 'WP_Filesystem_SSH2', so it will actually function when NO parameter (or too few) are passed when calling 'WP_Filesystem();'.

This code will make sure that ANY parameters passed WILL survive, and only iff a parameters was NOT passed I'll fetch them from 'request_filesystem_credentials'.

 // 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'];
  }

[jobst]

Patch for the problem

[jobst]

I am currently running the PATCHED version of class-wp-filesystem-ssh2.php on all of my websites.

With the patch provided the same debug printout of '$wp_filesystem' (as in comment #7) now looks as follows with no errors and a non null resource link

[20-Jun-2023 05:33:01 UTC] ============================================= this is JUST after WP_Filesystem_SSH2 Object
(
    [link] => Resource id #1529
    [sftp_link] => Resource id #1530
    [keys] => 1
    [verbose] =>
    [cache] => Array
        (
        )

    [method] => ssh2
    [errors] => WP_Error Object
        (
            [errors] => Array
                (
                )

            [error_data] => Array
                (
                )

            [additional_data:protected] => Array
                (
                )

        )

    [options] => Array
        (
            [port] => 22
            [hostname] => 127.0.0.1
            [username] => REDACTED
            [public_key] => REDACTED
            [private_key] => REDACTED
            [hostkey] => Array
                (
                    [hostkey] => ssh-rsa,ssh-ed25519
                )

        )

)

[jobst]

I am sorry, but there is a spelling mistake in the FIRST patch file. This file fixes the spelling mistake.

[jobst]

Hey @costdev

Just following up ... what's happening with this one?

[costdev]

Hey @jobst!

Let's update some keywords to indicate that there's a patch and that it needs testing.

Even though bugfixes can be committed during a Beta period, this particular issue is a little more involved, so it's better committed during an Alpha period so it gets more testing time. Let's keep this in the Future Release milestone for now to allow time for feedback and initial testing. We can then milestone it for a release. FWIW, if all goes well, I'm aiming for a 6.4 milestone for this one.

After another brief scan of the patch, it looks like it needs some WPCS fixes, so I'll also add changes-requested and try to update this later today.

[stuurluidevelopment]

@costdev just following this ticket, as we are running into this issue as well with some plugins.

We see this is not in the RC for version 6.4. What are the plans with adding this fix to Core?