WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 4 months 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 dev-feedback
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 5 months ago.
Changeset in but fix
38946.patch (4.9 KB) - added by shivapoudel 5 months ago.
Fix for 38946
38946.diff (4.9 KB) - added by shivapoudel 5 months ago.
Fix for 38946 - Diff

Download all attachments as: .zip

Change History (7)

@shivapoudel
5 months ago

Changeset in but fix

#1 follow-up: @dd32
5 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
5 months ago

Fix for 38946

@shivapoudel
5 months ago

Fix for 38946 - Diff

#2 @shivapoudel
5 months ago

  • Keywords has-patch added

#3 in reply to: ↑ 1 @shivapoudel
5 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 5 months ago by shivapoudel (previous) (diff)

#4 @shivapoudel
4 months ago

  • Keywords dev-feedback added
Note: See TracTickets for help on using tickets.