Make WordPress Core

Opened 6 years ago

Last modified 2 years ago

#21292 assigned defect (bug)

XML-RPC: wp_upload_bits should act like wp_handle_upload

Reported by: markoheijnen Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 3.0
Component: XML-RPC Keywords: has-patch needs-refresh
Focuses: Cc:

Description (last modified by markoheijnen)

At this moment there isn't a check for file size when uploading an image through the XML-RPC. The reason is that the method wp_upload_bits is used. This only checks it does is if the file size is to big for a network installation.

The function check_upload_size() is something what you want except that it will use wp_die() if there is an error like this. The function is used as a filter: wp_handle_upload_prefilter. Which only get applied in the function wp_handle_upload.

We should probably change check_upload_size() a bit so it doesn't use wp_die() but does it somewhere else.

related: #6559 and #21085

Attachments (4)

21292.diff (3.5 KB) - added by markoheijnen 6 years ago.
21292-2.diff (3.9 KB) - added by markoheijnen 6 years ago.
Added unfiltered_upload capability check
21292-3.diff (5.4 KB) - added by markoheijnen 6 years ago.
Added mime type matching
21292-unittests.diff (1.0 KB) - added by MikeHansenMe 3 years ago.
see #30284

Download all attachments as: .zip

Change History (33)

#1 @markoheijnen
6 years ago

  • Description modified (diff)

#3 @daniloercoli
6 years ago

  • Cc ercoli@… added

#4 @markoheijnen
6 years ago

Finding a way to add https://github.com/daniloercoli/WordPress-streaming-xml-rpc in the new media method. And when it works well in 3.5 also add it in 3.6 for the current one.

#5 @markoheijnen
6 years ago

Just a weird thought. Would it make sense to fake a $_FILES array to pass through wp_handle_upload(). By using the code of Danilo to store the image localy that would make sense.

Other solution is not using it and fix wp_upload_bits(). Looking again to it and they should both do the same thing. The only difference is the input and how to act on that.

6 years ago

#6 @markoheijnen
6 years ago

Uploaded my first try to fix the upload limit

6 years ago

Added unfiltered_upload capability check

#7 @markoheijnen
6 years ago

  • Description modified (diff)
  • Summary changed from Using wp_handle_upload in the XML-RPC to XML-RPC: wp_upload_bits should act like wp_handle_upload

Added a new patch that checks if a user can upload any file.

#8 @markoheijnen
6 years ago

Removed wp_check_filetype in the beginning of wp_upload_bits() and added wp_check_filetype_and_ext in the end.
In this way you can manage the check trough the 4th parameter. (copied and edited the code from #11946)

The only bad part is the need of unlink on error since you need to upload the image.

6 years ago

Added mime type matching

#9 @Viper007Bond
6 years ago

  • Cc viper007bond added

#10 @Viper007Bond
6 years ago

  • Cc Viper007Bond added; viper007bond removed

#11 @markoheijnen
6 years ago

  • Keywords has-patch added; needs-patch removed

#12 @nacin
5 years ago

In 22471:

Account for unfiltered_upload cap in wp_upload_bits(). see #21292.

#13 @nacin
5 years ago

  • Owner set to westi
  • Status changed from new to assigned

Two remaining issues:

  • wp_upload_bits() does not use wp_check_filetype_and_ext().
  • wp_upload_bits() does not consult check_upload_size().

westi, your call for 3.5 or punt.

#14 @westi
5 years ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release

I think we should punt this from 3.5 and come back in 3.6 to resolve those two issues.

#15 @sc0ttkclark
5 years ago

  • Cc lol@… added

#16 @markoheijnen
5 years ago

Well for me this is -1. Totally understand why people don't want to contribute on regular basis because of this.
I tried multiple times to get feedback on my latest patch but never gotten one.

#17 @nacin
5 years ago

1) It's a significant patch with quite a bit of changes, and a lot of potential to break things.

2) There were more pressing changes in XML-RPC, image handling, and the upload APIs for 3.5. Most of that, you participated in.

3) It has no unit tests.

4) It is not a regression, and can easily be traced back to 3.0 — when MU was merged and wp_check_filetype_and_ext() was introduced.

Overall, we're looking at a low priority fix for a low priority ticket. It'll get addressed in a future release. It would be good if more people reviewed it — but the fact that no one did is more indicative of it being a low priority than something that got lost.

#18 @markoheijnen
5 years ago

  • Keywords 3.6-early removed
  • Milestone changed from Future Release to 3.6
  • Version changed from 1.5 to 3.0

Yes, all those issues you can track back to 3.0 since this function didn't changed then.
It is however a high priority ticket since it can effect people badly (no disk space) and it shouldn't matter if no one did reviewed it or had see this issues.

It does has some unit tests but only indirectly through the XML-RPC method unit tests. I will write some more unit tests for it and refresh the patch this week.

#19 @markoheijnen
5 years ago

  • Keywords 3.7-early added; dev-feedback removed
  • Milestone changed from 3.6 to Future Release

#20 @wonderboymusic
5 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#21 @nacin
5 years ago

  • Keywords needs-refresh added

#22 @markoheijnen
4 years ago

  • Keywords 3.7-early removed
  • Milestone changed from 3.7 to 3.8

Punting to 3.8 but I marked this on my todo list with high priority ;)

#23 @markoheijnen
4 years ago

  • Keywords 3.9-early added
  • Milestone changed from 3.8 to Future Release

Didn't had time to really look into this. Will tackle this in the next weeks but to late for 3.8

#24 @wonderboymusic
2 years ago

In 34577:

XML-RPC: add a wp_uploadFile unit test.

Props MikeHansenMe.
See #21292.

#25 @wonderboymusic
2 years ago

  • Keywords 3.9-early removed
  • Milestone changed from Future Release to Awaiting Review
  • Owner westi deleted

#26 @netweb
2 years ago

r34577 fails all multisite unit tests, see https://travis-ci.org/aaronjorbin/develop.wordpress/builds/82269676

There was 1 failure:
1) Tests_XMLRPC_wp_uploadFile::test_network_limit
Failed asserting that Array (...) is an instance of class "IXR_Error".

#27 @wonderboymusic
2 years ago

In 34603:

After [34577], alter wp_xmlrpc_server::mw_newMediaObject() to check upload space in multisite.

See #21292.

#28 @boonebgorges
2 years ago

The test has to come out - the reason it had been removed in [30530] was that the test has never passed. It'll only pass once the issue raised in this ticket is resolved: namely, that wp_upload_bits() enforces multisite upload space restrictions.

#29 @boonebgorges
2 years ago

In 34604:

Remove failing xmlrpc upload test.

This test was added [34577], but shouldn't have been, because the bug it
describes has not been fixed.

See #21292.

Note: See TracTickets for help on using tickets.