WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#30815 closed defect (bug) (fixed)

WP_Filesystem_FTPext::exists() returns false if directory exists but is empty

Reported by: Unyson Owned by: ocean90
Milestone: 4.2 Priority: normal
Severity: normal Version: 2.8
Component: Filesystem API Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

/**
 * Check if a file or directory exists.
 *
 * @since 2.5.0
 * @abstract
 * @param string $file Path to file/directory.
 * @return bool Whether $file exists or not.
 */
public function exists($file) {
	$list = @ftp_nlist($this->link, $file);
	return !empty($list); //empty list = no file, so invert.
}

This method doesn't take in consideration that directory may exist but is empty.

The WP_Filesystem_FTPext::is_dir() method works correct (returns true). But the exists() method must be fixed, I think it's a major bug.

Attachments (2)

30815.patch (558 bytes) - added by ocean90 3 years ago.
30815.2.patch (1.2 KB) - added by ocean90 3 years ago.
Same for FTP sockets

Download all attachments as: .zip

Change History (9)

#1 @Unyson
3 years ago

  • Severity changed from normal to critical

@ocean90
3 years ago

#2 @ocean90
3 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.2

Noticed the same while testing #30802.

Setup:

define( 'FS_METHOD', 'ftpext' );
define( 'FTP_HOST', 'develop.wp.dev' );
define( 'FTP_USER', 'user' );
define( 'FTP_PASS', 'password' );

define('WP_CONTENT_DIR', '/srv/www/wp-shared/develop-content-empty' );
define('WP_LANG_DIR', '/srv/www/wp-shared/develop-languages-empty' );

30815.patch checks if $file is an empty directory.

Last edited 3 years ago by ocean90 (previous) (diff)

This ticket was mentioned in Slack in #core by ocean90. View the logs.


3 years ago

#4 @DrewAPicture
3 years ago

  • Keywords needs-unit-tests added

@ocean90: Latest patch still applies, though I suppose having unit tests here would be helpful to test the intent.

@ocean90
3 years ago

Same for FTP sockets

#5 @ocean90
3 years ago

  • Severity changed from critical to normal
  • Version changed from 4.1 to 2.8

From dd32: "I think the documentation is wrong though, since it's obviously designed to check for a file not a directory"

#6 @dd32
3 years ago

Basically I revert to using the logic of the Direct class in all cases such as this, Direct uses file_exists(), so it should be both file & directory.

The SSH2 class trims the trailing slash off a path to cause it to list the contents of the parent directory, limited to that child node.

So; The patch here looks good.

#7 @ocean90
3 years ago

  • Owner set to ocean90
  • Resolution set to fixed
  • Status changed from new to closed

In 31815:

WP_Filesystem: Change WP_Filesystem_FTPext::exists() and WP_Filesystem_ftpsockets::exists() to return true for empty directories.

Both methods are using *nlist() which returns a list of files in a given directory or the file itself for a given file. If the result was an empty list we assumed that the file doesn't exists. This includes also cases where $file is actually an empty directory. To prevent this we now check if $file is a directory before returning the result of an empty list.
Other filesystem methods are using file_exists() which already checks whether a file or directory exists.

fixes #30815.

Note: See TracTickets for help on using tickets.