Make WordPress Core

Opened 3 years ago

Closed 7 weeks ago

#54738 closed task (blessed) (fixed)

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: joedolson's profile joedolson
Milestone: 6.8 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests commit
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.

Attachments (3)

Change History (39)

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


3 years ago

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


3 years ago

#3 @mitogh
3 years ago

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.

Last edited 8 months ago by joedolson (previous) (diff)

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


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


17 months ago

#6 @hellofromTonya
17 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.


14 months ago

#8 @antpb
14 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.


11 months ago

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


11 months ago

#11 @antpb
11 months ago

  • Milestone changed from 6.6 to 6.7

Moving this to 6.7 as it still needs work and investigation

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


8 months ago

#13 @joedolson
8 months ago

  • Owner set to joedolson
  • Status changed from new to accepted

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.

#14 @masteradhoc
8 months ago

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

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


8 months ago

#16 @desrosj
7 months ago

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

#17 @joedolson
7 months ago

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

#18 @joedolson
7 months ago

  • Keywords 2nd-opinion added; needs-refresh removed

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.

This ticket was mentioned in PR #7466 on WordPress/wordpress-develop by @joedolson.


7 months ago
#19

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.

#20 @joedolson
7 months ago

  • Milestone changed from 6.7 to 6.8

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.

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


3 months ago

#22 follow-up: @joedolson
2 months ago

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

#23 in reply to: ↑ 22 @masteradhoc
2 months ago

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!

#24 @navi161
8 weeks ago

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.

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


7 weeks ago

#26 @joedolson
7 weeks ago

  • Keywords commit added; needs-testing 2nd-opinion removed

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.

#27 @joedolson
7 weeks ago

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

#28 @joedolson
7 weeks ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

#29 @johnbillion
7 weeks ago

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

#30 @johnbillion
7 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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


7 weeks ago

#32 @johnbillion
7 weeks ago

  • Keywords needs-unit-tests added; has-patch has-unit-tests commit removed
  • Type changed from enhancement to task (blessed)

#33 @joedolson
7 weeks ago

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

This ticket was mentioned in PR #8458 on WordPress/wordpress-develop by @joedolson.


7 weeks ago
#34

  • Keywords has-patch has-unit-tests added; needs-unit-tests removed

#35 @joedolson
7 weeks ago

  • Keywords commit added

Updated tests and resolved error; marking for commit.

#36 @joedolson
7 weeks ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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.

Note: See TracTickets for help on using tickets.