Make WordPress Core

Opened 2 years ago

Last modified 8 days ago

#54738 new enhancement

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

Reported by: masteradhoc's profile masteradhoc Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests needs-testing needs-refresh
Focuses: rest-api Cc:

Description

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.

Change History (9)

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


2 years ago

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


2 years ago

#3 @mitogh
2 years ago

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

$file = download_url( 'https://media.istockphoto.com/photos/coppersmith-repair-copper-kettle-on-fire-in-kashgar-in-xinjiang-picture-id1298102169?s=612x612' )
$result = media_handle_sideload( $file )

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

This ticket was mentioned in PR #2690 on WordPress/wordpress-develop by mitogh.


2 years ago
#4

  • Keywords has-patch has-unit-tests added

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

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


6 months ago

#6 @hellofromTonya
6 months ago

  • Keywords needs-testing needs-refresh added
  • Milestone changed from Awaiting Review to 6.5

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.

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


3 months ago

#8 @antpb
3 months ago

  • Milestone changed from 6.5 to 6.6
  • Type changed from defect (bug) to enhancement

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.

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


8 days ago

Note: See TracTickets for help on using tickets.