Make WordPress Core

Opened 9 years ago

Closed 12 months ago

Last modified 12 months ago

#33058 closed defect (bug) (fixed)

$wp_filesystem->exists('') returns a wrong answer (when FS_METHOD='ftpext').

Reported by: zdrobau's profile Zdrobau Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.3 Priority: normal
Severity: normal Version: 2.5
Component: Filesystem API Keywords: has-patch has-testing-info commit needs-docs
Focuses: Cc:

Description

If the method $wp_filesystem->exists() is called with an empty string and FS_METHOD = 'ftpext' then the result is true, I think it is wrong, because when FS_METHOD is defined as 'direct' returns false.

Attachments (2)

33058.diff (943 bytes) - added by mkox 2 years ago.
Check for empty filepath arg in exists method
33058_test_filesystem_exists.zip (1.2 KB) - added by costdev 12 months ago.
Test plugin.

Download all attachments as: .zip

Change History (24)

#1 @dd32
9 years ago

  • Version 4.2.2 deleted

Personally I think calling with an empty string is the incorrect behaviour here.

FTP is most likely returning that the current directory exists (Since that's what leaving the parameter off the ls -la calls will do)

#2 @Zdrobau
9 years ago

I want to use the method WP_Upgrader::install_package() with

$args['clear_destination'] = true

In it exists the following code that causes problems:

$removed = true;
if ( $wp_filesystem->exists( $remote_destination ) ) {
    $removed = $wp_filesystem->delete( $remote_destination, true );
}

#3 @dd32
9 years ago

If $remote_destination is an empty string, you've got deeper problems.

Sounds like the destination parameter to WP_Upgrader::install_package() is invalid, doesn't exist, or something else around that.

#4 @Zdrobau
9 years ago

I got this error because I use the following methods:

WP_Upgrader::download_package(); WP_Upgrader::unpack_package()

, and I want to install a theme from the zip archive that have a name like

theme-name-.-templates-.- 1.0.1-.-md5.zip

in a folder like

wp-content/themes/theme-name-parent

None of them has the option to rename this. When I call

WP_Upgrader::install_package()

with

$args['destination'] = WP_CONTENT_DIR . '/ themes /' . 'theme-name-parent'

and

FS_METHOD = 'direct'

this is possible. But not when

FS_METHOD = 'ftpext'

I think that

WP_Upgrader::install_package()

method should have the same behavior in both cases.

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


9 years ago

#6 @dd32
9 years ago

Is it possible to attach a plugin which shows the behaviour you're seeing?

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


7 years ago

#8 @mkox
2 years ago

I was able to see the problem @Zdrobau might have been experiencing here. The behavior for the different FS_METHOD values is inconsistent for wp_filesystem->exists(). This is due to different implementations of the exists methods.

In https://www.ietf.org/rfc/rfc959.txt page 32 you can see that the ftp_nlist is based on ftp_list. If that method receives an empty path, it will check the current working directory. So the ftpext implementation of exists may return true (maybe always for empty paths), but direct returns false.

The exists method should never be called empty, yes. But exists should behave equally in both impls.

@mkox
2 years ago

Check for empty filepath arg in exists method

#9 @mkox
2 years ago

  • Keywords has-patch dev-feedback added
  • Version set to 2.5

The patch will return false for empty filepath args, so both implementations behave equally.

This ticket was mentioned in PR #4224 on WordPress/wordpress-develop by @costdev.


12 months ago
#10

When ftp_nlist() or nlist() receive an empty path, they check the current working directory and may return true.

For consistency with the WP_Filesystem_Direct filesystem abstraction class, this returns false when an empty path is provided to WP_Filesystem_FTPext::exists() or WP_Filesystem_ftpsockets::exists().

Trac ticket: https://core.trac.wordpress.org/ticket/33058

@costdev
12 months ago

Test plugin.

#11 @costdev
12 months ago

  • Keywords needs-testing added; dev-feedback removed

After some testing, it appears that both the FTPext and ftpsockets ::exists() methods return true for an empty path.

PR 4224 updates the ::exists() methods for both WP_Filesystem_FTPext and WP_Filesystem_ftpsockets to return false when passed ''.

The PR uses a strict check on '' rather than empty(), to prevent an existing path of '0' returning false.

I've also attached a test plugin, 33058_test_filesystem_exists.zip, which shows the results for the direct, ftpext and ftpsockets filesystem methods. The test plugin requires that you have an FTP server installed and running.

#12 @costdev
12 months ago

  • Keywords has-testing-info added
  • Milestone set to 6.3

Testing Instructions

These steps define how to reproduce the issue or test a patch, and indicate the expected behavior.

Steps to Test

  1. Set up an FTP server on your local system. Make a note of the user and password.
  2. Download, install and activate the 33058_test_filesystem_exists.zip plugin.
  3. In wp-content/plugins/33058_test_filesystem_exists/33058_test_filesystem_exists.php, make sure that the FTP_BASE, FTP_USER and FTP_PASS constants are set to the appropriate values for your environment.
  4. 🐞 Note the results for each Empty path exists: entry.
  5. Apply PR 4224.
  6. Refresh the admin page.
  7. ✅ Note the results for each Empty path exists: entry.

Expected Results

When reproducing a bug:

  • ❌ The admin notice shows "No" for direct, and "Yes" for ftpext and ftpsockets.

When testing a patch to validate it works as expected:

  • ✅ The admin notice should show "No" for direct, ftpext and ftpsockets.

@costdev commented on PR #4224:


12 months ago
#13

Hi @mukeshpanchal27, unfortunately we don't have an FTP docker image available yet to test this properly.

@mukesh27 commented on PR #4224:


12 months ago
#14

Ok Thanks. @costdev for quick reply.

#15 @pbiron
12 months ago

Test Report

Windows 11
Apache 2.4.53
FileZilla Server 1.6.6 (FTP)

Before Patch Applied

Filesystem method: direct
Empty path exists: No

Filesystem method: ftpext
Empty path exists: Yes

Filesystem method: ftpsockets
Empty path exists: Yes

After Patch Applied

Filesystem method: direct
Empty path exists: No

Filesystem method: ftpext
Empty path exists: No

Filesystem method: ftpsockets
Empty path exists: No

LGTM

Last edited 12 months ago by pbiron (previous) (diff)

#16 follow-up: @costdev
12 months ago

  • Keywords commit added; needs-testing removed

Thanks @pbiron! As this change is a simple guard and doesn't employ any FTP functionality, adding for commit consideration.

#17 in reply to: ↑ 16 ; follow-up: @azaozz
12 months ago

Replying to costdev:

this change is a simple guard and doesn't employ any FTP functionality

Wondering if any of these methods support the FTP command CWD? If yes, seems an empty exists() would be expected to return true as it will be checking the current working directory?

Version 0, edited 12 months ago by azaozz (next)

#18 in reply to: ↑ 17 @pbiron
12 months ago

Replying to azaozz:

Replying to costdev:

this change is a simple guard and doesn't employ any FTP functionality

Wondering if any of these methods support the FTP command CWD? If yes, seems an empty exists() may be expected to return true as it will be checking the current working directory?

Thanx for looking at this @azaozz . The point of this ticket is that the current behavior of ::exists('') for the FTP filesytems is different than that for the direct filesystem.

And that incongruity could cause problems (not likely for core's use of the filesystem classes, but for extenders' uses).

#19 follow-up: @costdev
12 months ago

Wondering if any of these methods support the FTP command CWD? If yes, seems an empty exists() may be expected to return true as it will be checking the current working directory?

Some thoughts:

  • The filesystem classes have a ::cwd() method that returns the CWD (via ftp_pwd() and such). I'm not sure why someone would want to do an exists on an empty string as opposed to the better (IMO) ::exists( ::cwd() ), except for the specific reason that FTP checks the working directory as a sort of "quirk". It certainly wouldn't be a portable approach to use ::exists( '' ) if a site didn't use an FTP filesystem.
  • Let's say I'm going to delete something. I'm going to check if ( ::exists( $possible_empty_string ) ). If that returns true, I'm going to delete it. That means potentially deleting the current working directory if I didn't realise I was passing ''. Instead, I'd prefer a situation where if I run if ( ::exists( $possible_empty_string ) ), I get a "false" false, and when trying to create that directory, get a warning that /your/current/working/directory/ already exists, so I know I need to check something.
  • The direct filesystem is by far the most used filesystem. For consistency, the other classes should generally aim to mimic its behaviour, rather than the other way around.
  • Some documentation in the FTP ::exists() methods' docblocks might be a useful addition here to explain the reasoning.
Last edited 12 months ago by costdev (previous) (diff)

#20 @SergeyBiryukov
12 months ago

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

In 55556:

Filesystem API: Return false for empty paths in FTP ::exists() methods.

When ftp_nlist() receives an empty path, it checks the current working directory and may return true.

This affects:

  • WP_Filesystem_FTPext::exists()
  • WP_Filesystem_ftpsockets::exists()

As the purpose of the API is to provide a consistent interface for various filesystem implementations, this commit updates the affected methods to returns false when an empty path is provided, bringing consistency with the other filesystem abstraction classes, specifically WP_Filesystem_Direct and WP_Filesystem_SSH2.

Follow-up to [6779], [11821], [25274], [31815].

Props mkox, costdev, Zdrobau, dd32, pbiron, azaozz, mukesh27, SergeyBiryukov.
Fixes #33058.

@SergeyBiryukov commented on PR #4224:


12 months ago
#21

Thanks for the PR! Merged in r55556.

#22 in reply to: ↑ 19 @azaozz
12 months ago

  • Keywords needs-docs added

Replying to costdev:

I'm not sure why someone would want to do an exists on an empty string as opposed to the better (IMO) ::exists( ::cwd() )

Makes sense.

  • Some documentation in the FTP ::exists() methods' docblocks might be a useful addition here to explain the reasoning.

Yep, think so too. At least that ::exists( '' ) with empty string will always return false for consistency with the other filesystem methods, even when done after successful ::cwd( $path ).

Last edited 12 months ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.