WordPress.org

Make WordPress Core

Opened 13 months ago

Last modified 6 weeks ago

#38946 new enhancement

WP_Upgrader: Protection against deleting files in destination directory

Reported by: shivapoudel Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Upgrade/Install Keywords: has-patch
Focuses: Cc:

Description

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 ) );
}


Attachments (3)

class-wp-upgrader.php (33.1 KB) - added by shivapoudel 13 months ago.
Changeset in but fix
38946.patch (4.9 KB) - added by shivapoudel 13 months ago.
Fix for 38946
38946.diff (4.9 KB) - added by shivapoudel 13 months ago.
Fix for 38946 - Diff

Download all attachments as: .zip

Change History (10)

@shivapoudel
13 months ago

Changeset in but fix

#1 follow-up: @dd32
13 months ago

@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
13 months ago

Fix for 38946

@shivapoudel
13 months ago

Fix for 38946 - Diff

#2 @shivapoudel
13 months ago

  • Keywords has-patch added

#3 in reply to: ↑ 1 @shivapoudel
13 months ago

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 :)

Last edited 13 months ago by shivapoudel (previous) (diff)

#4 @shivapoudel
12 months ago

  • Keywords dev-feedback added

#5 @shivapoudel
6 weeks ago

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

#6 @dd32
6 weeks ago

  • Keywords dev-feedback removed

@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.

#7 @shivapoudel
6 weeks ago

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 :)

Note: See TracTickets for help on using tickets.