REST API: Bound the size of media sideloaded from a URL - #12825
REST API: Bound the size of media sideloaded from a URL#12825adamsilverstein wants to merge 4 commits into
Conversation
The attachments controller's URL creation path, `create_item_from_url()`, only ran `check_upload_size()`, which returns early when `! is_multisite()`. A single site therefore had no ceiling at all on this path: `upload_max_filesize` and `post_max_size` bound a request body, not a fetch the server makes itself, so any URL could pull a file of any size into the media library. Apply `wp_max_upload_size()` to the download, so a URL cannot bring in a file larger than the same site would accept as a direct upload. The limit is passed to the HTTP request as `limit_response_size`, which stops the transfer once it is passed, so an oversized remote file is no longer written to disk in full before being rejected. Sites that need a different ceiling can adjust it with the existing `upload_size_limit` filter. The multisite checks are unchanged and still run first, so `rest_upload_file_too_big` and `rest_upload_limited_space` continue to be returned for the network file size limit and the site space quota. No limit is applied when `wp_max_upload_size()` cannot determine a size. Follow-up to [62659], [62841]. See #65517.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
Thanks for putting this up, I'd started to look at it and this is pretty much exactly where I'd landed, too 👍
In my view, yes. The behaviour for
This too seems good to me, as it prevents the temp file from growing beyond the threshold, and it's a fairly simple change code-wise. |
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
This is testing great for me, thank you for hardening this! My vote would be to get this in for 7.1.
To test locally, on a single test site, I added a cap at 100kb via:
const S65517_MAX_SIZE = 102400; // 100 KB
if ( null !== S65517_MAX_SIZE ) {
add_filter( 'upload_size_limit', static fn() => S65517_MAX_SIZE, 20 );
}Files under that sideloaded fine, and files over that size returned a pleasing error in the editor:
LGTM!
| return $args; | ||
| }; | ||
|
|
||
| if ( $max_size > 0 ) { |
There was a problem hiding this comment.
here is where we skip setting the limit.
|
I updated the errant doc block, this is "good to go". |
|
Looks like this has been committed as of 5f5d96b. Good to close this PR out now? |
|
Yep, closing - fixed in https://core.trac.wordpress.org/changeset/63015 |
2 participants
Follow up to [62841], addressing the two observations @courane01 made while testing 7.1-beta4 in https://core.trac.wordpress.org/ticket/65517#comment:17.
check_upload_size()returns early when! is_multisite(), so on single site - the common case - the URL sideload path has no size ceiling at all.upload_max_filesizeandpost_max_sizebound a request body, not a fetch the server makes itself, so nothing else was stopping aurlparameter from pulling in a file of any size. This applieswp_max_upload_size()on that path, so a URL can't bring in a file larger than the same site would accept as a direct upload.The second half is that the check ran after
download_url()had already streamed the whole file to disk. The limit is now also passed to the request aslimit_response_size, which stops the transfer once it's passed, so an oversized remote file never lands on disk in full. One byte over the ceiling is enough to fail the size check, so the file is still rejected. Compression is already disabled for streamed requests, so this doesn't change how the response is decoded.The multisite checks are untouched and still run first, so
rest_upload_file_too_bigandrest_upload_limited_spaceare returned as before for the network file size limit and the site space quota. Sites that want a different ceiling can use the existingupload_size_limitfilter.Trac ticket: https://core.trac.wordpress.org/ticket/65517
How has this been tested
Automated:
rest_upload_file_too_big, and one that the download request is capped atwp_max_upload_size() + 1. Both fail on trunk without the change - I checked by stashing the source change and re-running.WP_Test_REST_Attachments_Controllerclass: 190 tests, 2 skipped, 0 failures.phpcs --standard=phpcs.xml.distclean on both changed files.Types of changes
wp_max_upload_size()as a ceiling on the URL sideload path, so it applies on single site as well as multisite.limit_response_sizeso the transfer is bounded rather than inspected after the fact.Open questions
wp_max_upload_size()the right ceiling here? It's derived from the PHP directives that govern request bodies, which don't really apply to a server-side fetch. The argument for it is parity: sideloading shouldn't accept a file the same user couldn't upload directly, and it's the number the media library already shows people. If we'd rather hosts be able to allow larger server-side fetches specifically, a dedicated filter would do it, though I'd lean toward not adding new API this late in 7.1.limit_response_sizetruncates rather than erroring, which is why the ceiling is set one byte high and the size check still does the rejecting. That works, but it does mean the transport's truncation behavior is load-bearing. Happy to drop it and keep only the post-download check if that feels too clever for a late-cycle fix.Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Opus 5
Used for: Drafting the implementation and tests from the review notes on the ticket. I reviewed and verified the behavior and test results myself.
Commit message