WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#15575 closed defect (bug) (fixed)

ftp upgrade fails on some system due to slash in remote destination

Reported by: lordandrei Owned by: dd32
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.0
Component: Filesystem API Keywords: has-patch 3.2-early needs-testing
Focuses: Cc:

Description

Routinely my users were unable to upgrade plugins on Mac OS X (10.5) server hosted sites.

The error occurred within upgrade.php in
class-wp-upgrader.php

The response was:

Could not create directory. <path to ftp wp-content>/plugins/<my-plugin>/

(At least the ftp on Max OS X (10.5) Server) The problem is that in ftp one can not do:

mkdir foo/


if foo doesn't exist then "mkdir foo/" will fail

The correct syntax is to remove the end slash:

mkdir foo

Then two solutions present themselves:

Either in class-wp-upgrader.php

//Protection against deleting files in any important base directories.
if ( in_array( $destination, array(ABSPATH, WP_CONTENT_DIR, WP_PLUGIN_DIR, WP_CONTENT_DIR . '/themes') ) ) {
	$remote_destination = trailingslashit($remote_destination) . trailingslashit(basename($source));
	$destination = trailingslashit($destination) . trailingslashit(basename($source));
}

Adding

if ( substr($remote_destination,-1,1) == '/' ) {
	$path = substr($remote_destination,0,-1);
}

Better, we add it to the top of class-wp-filestream-ftpext.php where the problem is occurring. Since we may not be in filesystem->ftpext when running through wp-class-upgrader.php

function mkdir($path, $chmod = false, $chown = false, $chgrp = false) {

Adding

	if ( substr($path,-1,1) == '/' ) {
		$path = substr($path,0,-1);
	}

This however seems to be potentially an FTP client issue as I've not tested this on other servers. So the inevitable fix may yet require a server capabilities test flag so as not to screw up other servers that might require the end slash.

If however the end slash is completely syntactically wrong for all mkdir commands:
One potential other fix is to change

$remote_destination = trailingslashit($remote_destination) . trailingslashit(basename($source));
			$destination = trailingslashit($destination) . trailingslashit(basename($source));

to

$remote_destination = trailingslashit($remote_destination) . basename($source);
			$destination = trailingslashit($destination) . basename($source);

I'm hazarding the solution but am not sure what is the best fix for this. I will be using the mkdir hack on my system until I see how this bug pans out.

Thanks

-A

Attachments (2)

class-wp-filesystem-ftpext.diff (490 bytes) - added by lordandrei 3 years ago.
Suggested patch
15575.patch (1002 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 lordandrei3 years ago

  • Summary changed from ftp upgrade fails on some system due to slash in remote destination to ftp upgrade fails on some system due to slash in remote destination (w/solution)

comment:2 lordandrei3 years ago

  • Cc lordandrei added
  • Severity changed from normal to major

lordandrei3 years ago

Suggested patch

comment:4 lordandrei3 years ago

  • Summary changed from ftp upgrade fails on some system due to slash in remote destination (w/solution) to ftp upgrade fails on some system due to slash in remote destination (w/patch)

comment:5 lordandrei3 years ago

  • Keywords has-patch added

comment:6 lordandrei3 years ago

  • Severity changed from major to normal
  • Summary changed from ftp upgrade fails on some system due to slash in remote destination (w/patch) to ftp upgrade fails on some system due to slash in remote destination

comment:7 lordandrei3 years ago

  • Version changed from 3.0.1 to 3.0.3

Upgraded that bug persists in 3.0.3

comment:8 nacin3 years ago

  • Keywords needs-patch 3.2-early added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release

Easier way to fix this would be using rtrim(). Specifically, we have a function imaginatively called untrailingslashit().

comment:9 dd323 years ago

  • Component changed from Upgrade/Install to Filesystem
  • Version changed from 3.0.3 to 3.0

SergeyBiryukov3 years ago

comment:10 SergeyBiryukov3 years ago

  • Keywords has-patch added; needs-patch removed

I've added untrailingslashit() to class-wp-filesystem-ftpext.php and class-wp-filesystem-ftpsockets.php. In class-wp-filesystem-direct.php and class-wp-filesystem-ssh2.php it's already there.

Not sure about class-ftp.php and other functions like rmdir(). Should we use it there too?

comment:11 dd323 years ago

Not sure about class-ftp.php and other functions like rmdir(). Should we use it there too?

If the function will error out without having an untrailingslash'd then it should be added. class-ftp.php shouldn't be touched as class-wp-filesystem-ftpsockets.php is a wrapper for it.

There are some strange cases with / and rm from memory, things like folder/file detecting issues, I wouldnt be surprised if that's one reason why a trailing slash is included on some of them.

comment:12 SergeyBiryukov3 years ago

  • Keywords needs-testing added

comment:13 lordandrei3 years ago

  • Version changed from 3.0 to 3.3

I have tested this patch extensively with the last 3 releases on several different installs.
Who do I need to pass this to so that I can stop having to manually re-add this every time I patch?

comment:14 dd323 years ago

  • Milestone changed from Future Release to 3.3
  • Owner set to dd32
  • Status changed from new to accepted
  • Version changed from 3.3 to 3.0

I'll throw this into 3.3 before beta2, to hopefully catch any side effects of it, whilst there shouldn't be any, if some servers don't like the trailing slash, there's a chance of others wanting it.

For what it's worth, the RFC specs for FTP don't mention anything about the expected format of the path to create, however the examples don't mention a trailing slash as part of the path. It seems like no trailing slash is the least ambiguous option.

The Version field is used to track when a issue was first raised/diagnosed, Please do not change it to a later version if it affects the earlier versions

comment:15 dd323 years ago

In [18964]:

WP_Filesystem_*::mkdir() untrailingslash path consistently, don't waste time attempting to create an "empty" path. See #15575. Props lordandrei and SergeyBiryukov for initial patches.

comment:16 dd323 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed
Note: See TracTickets for help on using tickets.