Unable to upload images with URL over API when the image doesn’t have a filetype in the filename

[masteradhoc]

Problem: It’s not possible to upload images with URL per API when the image doesn’t have a filetype in the filename.

Elaboration: There are some websites which don’t put filetypes inside the URL for images. For example: ​https://img.ricardostatic.ch/t_1800x1350/pl/1187743940/3/1/

Or: ​https://media.istockphoto.com/photos/coppersmith-repair-copper-kettle-on-fire-in-kashgar-in-xinjiang-picture-id1298102169?s=612x612

The format of the image is set and recognized by the browser with the MIME-Type. In theory WordPress could handle this, but unfortunately it never gets to the point where the correct MIME type is read out of the image signature.

Reason: At the function wp_check_filetype type will always be defined as false - if there is no filetype set inside the filename.

Call of the function: ​https://github.com/WordPress/WordPress/blob/266c58518846201a7e98cd7995ce2c7429caf1db/wp-includes/functions.php#L2984

Further down there would be a function to get back the real mime type from the signature of the image. However, the function is never called since type at this point is always false in that give case:

​https://github.com/WordPress/WordPress/blob/266c58518846201a7e98cd7995ce2c7429caf1db/wp-includes/functions.php#L2999

Possible Bug: If the filename has no filetype the function wp_get_image_mime should always be called to get the type from the signature of the image.

Following Bug: After changing this, the filename at the upload should be extended with the filetype: ​https://github.com/WordPress/WordPress/blob/266c58518846201a7e98cd7995ce2c7429caf1db/wp-admin/includes/file.php#L924

Since it’s only really showing up in the library with the correct filetype.

[mitogh]

Adding the steps to replicate the problem using functions either directly on a script or via WP shell.

$result = media_sideload_image( 'https://img.ricardostatic.ch/t_1800x1350/pl/1187743940/3/1/' );


// This would be an error because file would use a .tmp extension which is not valid to be uplodaed.
var_dump( $result );

Mainly the problem is because the header content is not considered as part of the mime type instead the extension of the file is used to indicate which mime type is.

Edit 9/4/2024: originally reproduction function calls were incorrect.

When an image or a URL is downloaded using WordPress function download_url and if the provided URL does not contain the file extension, instead of using tmp try to guess the extension of the file by using the provided mime type of the file.

An important consideration is to make sure the mime type is supported by the current WordPress installation to prevent download and upload of non supported mime types on the current installation.

Trac ticket: https://core.trac.wordpress.org/ticket/54738

[hellofromTonya]

From today's scrub, I'm updating the keywords to help it make actionable to move forward towards resolution:

• needs-refresh: ​the patch has merge conflicts, thus needs a refresh against the latest trunk.
• needs-testing: test reports are needed once the patch is refreshed.
• Code review of the patch is needed.

Given there's a patch (though it needs a refresh), moving the ticket into 6.5 for visibility, i.e. to help it move forward.

[antpb]

Leaving a note here after some discussion in the recent Media Meeting. We are moving this to 6.6 to allow some time to have review with respect to any security implications this may introduce. Nothing yet identified, just want to be extra cautious changing how we validate files and assume mime types.

[antpb]

Moving this to 6.7 as it still needs work and investigation

[joedolson]

This seems like it's in good shape. The comments from @dmsnell are mostly about clarification, and worth looking at to improve the understandability of the flow, but nothing blocking. I'll work on getting this over the finish line.

[masteradhoc]

Thanks @joedolson for working further on the patch from @mitogh. Happy to help on retesting this again if needed.

[desrosj]

@joedolson Are you able to take a look at this prior to beta 1?

[joedolson]

@desrosj This is on my shortlist to look at today; I'll update it one way or the other by the end of the day.

[joedolson]

Reproduction is a little tricky for this.

media_sideload_image() doesn't work because it has a check for the file extension before it goes to download_url; and download_url returns a string, but media_handle_sideload requires a file array.

$file_array         = array();
$file_array['name'] = 'test.jpg';

// Download file to temp location.
$file_array['tmp_name'] = download_url( 'https://img.ricardostatic.ch/t_1800x1350/pl/1187743940/3/1/' );
$handle                 = media_handle_sideload( $file_array );

This works; but requires you to manually define the file name.

Given that this is really only a tool for extenders, that may be just fine, but I'd like a 2nd opinion before moving this forward.

Trac ticket: https://core.trac.wordpress.org/ticket/54738

This is a refresh of ​https://github.com/WordPress/wordpress-develop/pull/2690 with minor tweaks to simplify code.

[joedolson]

Given the short amount of time until 6.7 beta 1 and the need for a 2nd opinion, I'm going to bump this to 6.8.

[joedolson]

@masteradhoc If you can confirm whether the current PR meets your use case, that would be useful. I've just refreshed the PR, so it's up to date.

[masteradhoc]

Replying to joedolson:

@masteradhoc If you can confirm whether the current PR meets your use case, that would be useful. I've just refreshed the PR, so it's up to date.

Thanks for your work so far and the refreshed PR @joedolson!
Just tested it and so far i think your approach is a good one and doable for us to implement this into our plugin.

Looks like a neat and easy solution for us!

[navi161]

QA Notes:
This looks fixed now. I am able to upload an image with URL without the file types successfully.
Please let me know if there is anything else that needs to be checked.
Screens attached for reference.

[joedolson]

Tested and fixed the unit test (which was using AVIF as a content type that should fail, but it doesn't fail since we added support in 6.5.).

Marking for commit.

[joedolson]

@navi161 FYI, that wasn't a valid test that you performed. This is about sideloading images from a URL that doesn't use a file extension that indicates the file type, but you tested inserting an image that directly references the image from the 3rd party location; it's a different issue. The reproduction instructions are in comment:18.

[joedolson]

In 59902:

Media: Allow uploading images from URLs without extensions.

Enable download_url() to fetch and verify file types if the URL does not contain a file extension. This allows URL downloads to handle media endpoints like istockphoto.com that use file IDs and formatting arguments to deliver images.

Props masteradhoc, mitogh, joedolson, hellofromTonya, antpb, audrasjb, navi161, dmsnell.
Fixes #54738.

[johnbillion]

@joedolson A warning is being emitted by PHPUnit since r59902:

There was 1 risky test:

1) Tests_Admin_IncludesFile::data_download_url_should_use_the_content_type_header_to_set_extension_of_a_file_if_extension_was_not_determined
This test did not perform any assertions

/var/www/tests/phpunit/tests/admin/includesFile.php:423

/var/www/vendor/bin/phpunit:122

PHPUnit is attempting to run data_download_url_should_use_the_content_type_header_to_set_extension_of_a_file_if_extension_was_not_determined() as a test, but it's a data provider. I think this is due to the @test annotation. Is that a mistake?

[joedolson]

Thanks, @johnbillion. Yeah, looks like a simple mistake. I'll get it updated after the release party.

Trac ticket: https://core.trac.wordpress.org/ticket/54738

[joedolson]

Updated tests and resolved error; marking for commit.

[joedolson]

In 59934:

Media: Tests: Remove extraneous @test annotation.

The data provider for tests added in [59902] had an @test annotation, causing it to be run as if it were a test, throwing a risky test warning. Remove the @test annotation to prevent this undesired warning.

Props johnbillion, joedolson.
Fixes #54738.