Make WordPress Core

Opened 12 years ago

Last modified 17 months ago

#21292 assigned defect (bug)

XML-RPC: wp_upload_bits should act like wp_handle_upload

Reported by: markoheijnen's profile markoheijnen Owned by:
Milestone: 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 12 years ago.
21292-2.diff (3.9 KB) - added by markoheijnen 12 years ago.
Added unfiltered_upload capability check
21292-3.diff (5.4 KB) - added by markoheijnen 12 years ago.
Added mime type matching
21292-unittests.diff (1.0 KB) - added by MikeHansenMe 10 years ago.
see #30284

Download all attachments as: .zip

Change History (34)

#1 @markoheijnen
12 years ago

  • Description modified (diff)

#3 @daniloercoli
12 years ago

  • Cc ercoli@… added

#4 @markoheijnen
12 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
12 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.

@markoheijnen
12 years ago

#6 @markoheijnen
12 years ago

Uploaded my first try to fix the upload limit

@markoheijnen
12 years ago

Added unfiltered_upload capability check

#7 @markoheijnen
12 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
12 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.

@markoheijnen
12 years ago

Added mime type matching

#9 @Viper007Bond
12 years ago

  • Cc viper007bond added

#10 @Viper007Bond
12 years ago

  • Cc Viper007Bond added; viper007bond removed

#11 @markoheijnen
12 years ago

  • Keywords has-patch added; needs-patch removed

#12 @nacin
12 years ago

In 22471:

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

#13 @nacin
12 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
12 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
12 years ago

  • Cc lol@… added

#16 @markoheijnen
12 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
12 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
12 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
11 years ago

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

#20 @wonderboymusic
11 years ago

  • Milestone changed from Future Release to 3.7

these are all marked 3.7-early

#21 @nacin
11 years ago

  • Keywords needs-refresh added

#22 @markoheijnen
11 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
11 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
9 years ago

In 34577:

XML-RPC: add a wp_uploadFile unit test.

Props MikeHansenMe.
See #21292.

#25 @wonderboymusic
9 years ago

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

#26 @netweb
9 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".
/home/travis/build/aaronjorbin/develop.wordpress/tests/phpunit/tests/xmlrpc/wp/uploadFile.php:53

#27 @wonderboymusic
9 years ago

In 34603:

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

See #21292.

#28 @boonebgorges
9 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
9 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.

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


17 months ago

Note: See TracTickets for help on using tickets.