WP_Upgrader: Protection against deleting files in destination directory

[shivapoudel]

While creating a plugin I have to upload zip file and extract it in the custom destination directory with its inside main folder name just like how Theme_Upgrader and Plugin_Upgrader does.

While extending the class WP_Upgrader and using method install() like this:

<?php
/**
 * Install a demo package.
 *
 * @since 2.8.0
 * @since 3.7.0 The `$args` parameter was added, making clearing the update cache optional.
 * @access public
 *
 * @param string $package The full local path or URI of the package.
 * @param array  $args {
 *     Optional. Other arguments for installing a demo package. Default empty array.
 *
 *     @type bool $clear_update_cache Whether to clear the updates cache if successful.
 *                                    Default true.
 * }
 *
 * @return bool|WP_Error True if the install was successful, false or a WP_Error object otherwise.
 */
public function install( $package, $args = array() ) {

        $defaults = array(
                'clear_update_cache' => true,
        );
        $parsed_args = wp_parse_args( $args, $defaults );

        $this->init();
        $this->install_strings();

        add_filter( 'upgrader_source_selection', array( $this, 'check_package' ) );

        $this->run( array(
                'package' => $package,
                'destination' => WP_CONTENT_DIR . '/uploads/demo-packs',
                'clear_destination' => false, // Do not overwrite files.
                'protect_destination' => true, // Let me utilize this args in `install_package()` to fix this error, happy :)
                'clear_working' => true,
                'hook_extra' => array(
                        'type' => 'demo',
                        'action' => 'install',
                )
        ) );

        remove_filter( 'upgrader_source_selection', array( $this, 'check_package' ) );

        if ( ! $this->result || is_wp_error( $this->result ) ) {
                return $this->result;
        }

        return true;
}

Now always the zip file content are extracted into that destination directory but I want to index just like this where {uploaded-zip} is the zip file inside folder name:

WP_CONTENT_DIR . '/uploads/demo-packs/{uploaded-zip}'

And finally the detected issue is that the destination directory are no more protected just like theme and plugins directories does :)

<?php
/*
 * Protection against deleting files in any important base directories.
 * Theme_Upgrader & Plugin_Upgrader also trigger this, as they pass the
 * destination directory (WP_PLUGIN_DIR / wp-content/themes) intending
 * to copy the directory into the directory, whilst they pass the source
 * as the actual files to copy.
 */
$protected_directories = array( ABSPATH, WP_CONTENT_DIR, WP_PLUGIN_DIR, WP_CONTENT_DIR . '/themes' );

if ( is_array( $wp_theme_directories ) ) {
        $protected_directories = array_merge( $protected_directories, $wp_theme_directories );
}

if ( in_array( $destination, $protected_directories ) ) {
        $remote_destination = trailingslashit( $remote_destination ) . trailingslashit( basename( $source ) );
        $destination = trailingslashit( $destination ) . trailingslashit( basename( $source ) );
}

And this is where the magic begins by adding protect_destination args and fixing it:

<?php
if ( in_array( $destination, $protected_directories ) || $args['protect_destination'] ) {
        $remote_destination = trailingslashit( $remote_destination ) . trailingslashit( basename( $source ) );
        $destination = trailingslashit( $destination ) . trailingslashit( basename( $source ) );
}

[shivapoudel]

Changeset in but fix

[dd32]

@shivapoudel Wouldn't it be possible to simply include the subdirectory in the destination arg?

That being said, having some way of triggering this specific code check for others, either through a filter or a parameter sounds like a good idea.

Would you be able to ​attach your changes as a `diff` file rather than the entire file? Please also make sure that any args added have defaults set correctly too.

[shivapoudel]

Fix for 38946

[shivapoudel]

Fix for 38946 - Diff

[shivapoudel]

Replying to dd32:

It wouldn't be possible to simply guess the basename of package, strip .zip extension and set that match as subdirectory for destination arg. This is because it may result subdirectory like uploaded-demo-pack-{1|2|3|..} and inside zip folder the directory name is predefined and look like uploaded-demo-pack.

This above patch tries to implement protect_destination arg and have defaults set to false :)

[shivapoudel]

@swissspidy @dd32 Can you please kindly review this too nearly 1 year :)

[dd32]

@shivapoudel Would you be able to upload an example of the scenario, or a way to trigger it? Due to the limited experience many committers have with the WP_Upgrader classes it's not as easy for others to review/weigh in on the ticket.

Despite my earlier comments, I don't think extending the $protected_directories functionality is something I support.

The better option, and probably only option I can think of, would be an action within the install routines which allows changing some of the args which has been passed in, after the package has been extracted, and probably at other points of the process.

[shivapoudel]

Hey @dd32 sorry for the late response!

Look at this method install_package in TG_Demo_Upgrader class which extends WP_Upgrader ​https://github.com/themegrill/themegrill-demo-importer/blob/4779becd5dfe90c689efa5bee137cdc5a2108865/includes/admin/class-demo-upgrader.php#L137-L351. This was the hackathon I have used to protect my demo files directory.

Likewise that method has never been extended in class like Plugin_Upgrader or themes etc. in anyway because themes and plugins directories are always protected but we want to protect our custom directories too. Read this and its worth for placing our directories or not without args, just kidding you can brainstorm some thought too ​https://github.com/themegrill/themegrill-demo-importer/blob/4779becd5dfe90c689efa5bee137cdc5a2108865/includes/admin/class-demo-upgrader.php#L256-L263.

Finally above patch can allow us to provide args to protect_destination so we are safe side without hassle. Look here ​https://github.com/themegrill/themegrill-demo-importer/blob/4779becd5dfe90c689efa5bee137cdc5a2108865/includes/admin/class-demo-upgrader.php#L85

This solution can help me to get the name of main folder inside zip and install as it is :)

That is my solution to get the name of main folder inside zip and install. BUT I don't want to extend the WP core upgrader install_package method in anyway in my upcoming plugin version so I am raising this patch :)