WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#14963 closed enhancement (fixed)

Plugin update shouldn't delete package from local filesystem

Reported by: joelhardi Owned by: dd32
Milestone: 3.2 Priority: normal
Severity: normal Version: 3.1
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

While working on #14915 I discovered a small separate issue.

Currently, when upgrading a plugin, the package (zip) file is extracted to the filesystem and then deleted, no matter what the source of the file is.

  • Remotely fetched packages, after they're extracted, are deleted from their temporary location (which is fine, that's garbage collection).
  • Local files, however, are also deleted from wherever they happen to be. I don't think that it's right to silently unlink from the user's local filesystem like this. If I've got a file sitting on my local system (especially outside WordPress' file tree) I don't expect WordPress to delete it (without asking).

I doubt this issue has come up as I can't imagine it affects many people (who has or would manually download a plugin package and then hack the WordPress upgrader to install it, instead of just unzipping into the plugins folder?). Nevertheless I think it's undesirable behavior, and the attached patch is a trivial enhancement.

Attachments (1)

class-wp-upgrader.php.15659.diff (661 bytes) - added by joelhardi 5 years ago.
patch to check that zip package is downloaded copy before unlinking it

Download all attachments as: .zip

Change History (9)

@joelhardi5 years ago

patch to check that zip package is downloaded copy before unlinking it

comment:1 in reply to: ↑ description @nacin5 years ago

Replying to joelhardi:

(who has or would manually download a plugin package and then hack the WordPress upgrader to install it, instead of just unzipping into the plugins folder?)

Patch looks sane enough, but the upgrader isn't really designed for that. We don't delete files that aren't temporary, but that's not designed to be a non-temporary file. Can you provide a use case?

comment:2 @dd325 years ago

(who has or would manually download a plugin package and then hack the WordPress upgrader to install it, instead of just unzipping into the plugins folder?)

For reference, When I hack WordPress up like that, I hack the download handler to return http://localhost/..../ for the filename i'm after, Result is the rest of the code doesnt know it's been messed with..

That being said, The patch looks sane, But, I'm hesidant to commit it without there being a functionality which utilises it..

Whilst I've not looked too deeply, This may also affect plugin zip's which are uploaded, that temporary file should be deleted once done with as well (even though PHP should clean up after itself anyway)

comment:3 @joelhardi5 years ago

  • Cc joel@… added

I think what I'm suggesting is already consistent with the way the code works. If you look at source:trunk/wp-admin/includes/class-wp-upgrader.php@15978#L297 (the lines just before my patch is applied) you'll see that the variable $download is already set to either a URL or a local filepath by download_package(). If you look at download_package(), you'll see it already checks to see whether file is local or remote. Only if the file is remote is it downloaded to a temporary location by download_url() [in includes/file.php].

So, my patch isn't determining whether file is local or remote, just using the result as already determined by download_package(). In the case where the file is a downloaded temp copy, it's deleted as before. In the case where the file is local, it hasn't been downloaded via download_url() and so isn't a temporary file that WordPress should be deleting.

And unpack_package() already accepts the optional second argument to tell it whether to delete the package or not after unpacking.

So IMHO WordPress is already doing the right thing and checking to see whether the file is local or remote, the only problem is that it is deleting the local file anyway. Thus the minor patch, to fix what I think is technically a bug, even if users aren't encountering it.

(Also ... in the case where the file is local but WordPress lacks filesystem permissions to unlink it, an E_WARNING is raised by the call to unlink() in unpack_package().)

As far as a regular user's use case ... I don't know of any. My use case, as a plugin developer, is that when I am writing a plugin that has install/activation hooks and I want to test it through the plugin upgrader, I mod the upgrader to use a local file rather than download via http.

An alternate fix would be to change download_package() so that, if it determines a file is local, it copies it to the temp location and returns that filepath, so that the file that is later deleted by unpack_package() is the temp copy, not the original. This way, download_package() would always return an actual "downloaded" package in a temp dir, rather than the current return value of "sometimes a downloaded file, sometimes something from the local fs." That might be more transparent than my patch's branch in run().

comment:4 @dd324 years ago

  • Component changed from Plugins to Upgrade/Install
  • Keywords 3.2-early added; dev-feedback removed
  • Milestone changed from Awaiting Review to Future Release
  • Owner set to dd32
  • Status changed from new to accepted

comment:5 @dd324 years ago

This accepts a local package for the purposes of the zip uploader, the local path of the uploaded file is passed in, instead of the url.

Patch looks sane (well, i simplified it to $delete_package = ($download != $package);) the only thing that needs to be checked, is that the temporary Zip file will be deleted by PHP after the request is completed.

comment:6 @dd324 years ago

  • Keywords 3.2-early removed
  • Milestone changed from Future Release to 3.2

PHP will, as expected, delete the temporary upload file upon script end.

comment:7 @dd324 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

(In [17660]) Do not delete local package files in WP_Upgrader. Props joelhardi. Fixes #14963

Note: See TracTickets for help on using tickets.