WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 6 weeks ago

Last modified 3 weeks ago

#50695 closed enhancement (fixed)

Make media_sideload_image extension list filterable

Reported by: paulschreiber Owned by: johnbillion
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch needs-dev-note commit
Focuses: Cc:

Description

While upload_mimes is filterable and allows you to add additional items to the media library (i.e. SVGs), media_sideload_image() relies on an extension type check, and is not filterable.

Current behaviour:

<?php
preg_match( '/[^\?]+\.(jpe?g|jpe|gif|png)\b/i', $file, $matches );

Desired behaviour:

<?php
/**
 * Filters the file extensions used for media sideload.
 *
 * @since 5.5.0
 *
 * @param array $extensions The original extension list.
 */
$allowed_extensions =  apply_filters( 'media_sideload_extensions',  array( 'jpg', 'jpeg', 'jpe', 'png', 'gif' ) );
preg_match( '/[^\?]+\.(' . join( '|', allowed_extensions ) . ')\b/i', $file, $matches );

Change History (14)

This ticket was mentioned in PR #416 on WordPress/wordpress-develop by paulschreiber.


4 months ago

  • Keywords has-patch added

Makes media_sideload_image() extension list filterable.

Trac ticket: https://core.trac.wordpress.org/ticket/50695#ticket

#2 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.6

This ticket was mentioned in Slack in #core by hellofromtonya. View the logs.


2 months ago

#4 @johnbillion
2 months ago

This could benefit from some preg_quote() in case someone tries to get too clever with an asterisk.

#5 @hellofromTonya
2 months ago

  • Keywords needs-dev-note needs-unit-tests added

New filter added. This one needs-dev-note on the Misc Dev Note.

The extensions are passed through a filter with an expected array of extensions returned from it. Adding needs-unit-tests to add test data to valid the expected and unexpected filtered extensions.

A guard clause might be handy here too to handle the unexpected filtered content, such as a string of extensions or nothing being returned.

#6 @paulschreiber
2 months ago

How about this to clean up the list?

<?php
$allowed_extensions = array_filter(
    array_map(
        function( $item ) {
            return preg_replace( '/[^A-Z0-9]/i', '', $item );
        },
        $allowed_extensions
    )
);

if ( ! $allowed_extensions ) {
    return new WP_Error( 'image_sideload_failed', __( 'Invalid extension list.' ) );
}

This ticket was mentioned in Slack in #core-media by antpb. View the logs.


7 weeks ago

#8 @hellofromTonya
6 weeks ago

@johnbillion What do you think of what @paulschreiber is proposing here?

This ticket was mentioned in Slack in #core-media by hellofromtonya. View the logs.


6 weeks ago

#10 @johnbillion
6 weeks ago

  • Keywords commit added; needs-unit-tests removed

I've been trying to restructure this code so it's nicely testable, but there is so much code that's tightly coupled to actually downloading the file that it's turning into a rabbit hole.

I don't think there's a need to verify that the extension list is valid, this is not something that core typically does with other filters and if an invalid value is returned then it'll trigger a PHP warning, which is an appropriate means of showing an error for incorrect code.

I've just pushed a tweak to Paul's PR to improve the filter name, docs, and add the $file parameter for extra context, as well as preg_quote-ing the list.

#11 @hellofromTonya
6 weeks ago

Made a code suggestion to change join to implode to comply the coding standard fixes happening in ticket 50676.

#12 @johnbillion
6 weeks ago

  • Owner set to johnbillion
  • Resolution set to fixed
  • Status changed from new to closed

In 49198:

Media: Add an image_sideload_extensions filter to the list of allowed file extensions when sideloading an image from a URL.

Props paulschreiber, hellofromTonya

Fixes #50695

This ticket was mentioned in Slack in #core by daisyo. View the logs.


3 weeks ago

Note: See TracTickets for help on using tickets.