REST API: Insert URL sideloads through the shared attachment path - #12846
REST API: Insert URL sideloads through the shared attachment path#12846adamsilverstein wants to merge 13 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.
Sideloading an external image with the `url` parameter on `POST /wp/v2/media` works around a cross-origin fetch the browser cannot make. That fetch fails regardless of whether client-side media processing is enabled, so the argument was registered too narrowly. Leaving the argument unregistered did not disable it. `create_item()` reads the parameter either way, so on a site without client-side media processing the sideload still ran, but without the `sanitize_url` and `wp_http_validate_url()` callbacks the registered argument carries, and an unsafe URL returned a bare `http_request_failed` rather than a 400. Register `url` unconditionally so its validation always applies. The `generate_sub_sizes` and `convert_format` parameters are different: they hand image processing to the client, and the route the client uploads the results to is only registered when the feature is enabled. Honoring them otherwise would leave an attachment with no sub-sizes and no way to add them, and would relax the unsupported image type check in `create_item_permissions_check()` on a site that never opted in. Both are now ignored unless client-side media processing is enabled. Follow-up to [62659]. See #65517.
`create_item_from_url()` built and returned its own response, so a request supplying a `url` skipped everything `create_item()` does around the insert. Fields the schema accepts were dropped, `rest_pre_insert_attachment` and `rest_insert_attachment` never fired, and neither did `wp_after_insert_post`: `wp_insert_post()` returns for attachments before its after-insert hooks, so `media_handle_sideload()` could not fire it despite the comment claiming it did. Make a URL the third source of the file, alongside a multipart upload and a raw request body. `upload_from_url()` downloads the remote file and hands it to `wp_handle_sideload()`, returning the same data as the other two handlers, so `insert_attachment()` and `create_item()` perform the insert for every path. A request that supplies both a file and a `url` is now rejected instead of having one of the two silently discarded. `insert_attachment()` also checks `prepare_item_for_database()` for an error, so an error returned from `rest_pre_insert_attachment` is honored rather than used as an attachment. The capability check in the removed method is dropped as redundant, since `create_item_permissions_check()` already denies a user without `upload_files` before the callback runs. See #65810.
|
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. |
jeremyfelt
left a comment
jeremyfelt
left a comment
There was a problem hiding this comment.
@adamsilverstein I like the direction! I won't have time to do a full review until at least later tonight, if not tomorrow morning. Don't let me hold anything up from hitting before RC, but I do think this is an improvement.
As you worked out, "Upload to Media Library" and the pre-publish external media panel both send only
postandurl, so they never hit any of it.
To clarify (I think): these flows are the only thing that uses the url attribute on wp/v2/media. Which is probably the best argument for it being supported this flow even when client processing is disabled.
I have not been able to trigger any requests related to client-side processing with url. Those all send file as a binary blob.
The comment described why an earlier trunk iteration changed rather than documenting the feature, which read as out of place for new 7.1 code. The registration stands on its own. See WordPress#12846 (comment)
…item-parity # Conflicts: # src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php # tests/phpunit/tests/rest-api/rest-attachments-controller.php
…item-parity # Conflicts: # src/wp-includes/rest-api/endpoints/class-wp-rest-attachments-controller.php # tests/phpunit/tests/rest-api/rest-attachments-controller.php
|
Thanks for the ping — I've run out of time to test this today, but conceptually I like it! Makes sense to go through the shared path to me. I'll give it a closer look tomorrow if it's still open 👍 |
3 participants
Addresses the four items @jeremyfelt found while walking the 7.1 media upload paths in #12833 (comment).
Three of the four fall out of one thing:
create_item_from_url()builds and returns its own response, so a request that supplies aurlreturns beforecreate_item()does any of its work around the insert. Fields the schema accepts are dropped,rest_pre_insert_attachmentandrest_insert_attachmentnever fire, and neither doeswp_after_insert_post. The comment sayingmedia_handle_sideload()handles that last one isn't right -wp_insert_post()returns for attachments at post.php:5212, before the$fire_after_hooksblock, so it can't fire from there no matter what is passed.create_item()calls it explicitly, which is why the uploaded-file path is fine.Rather than copy that tail into the URL path, this makes a URL the third source of the file, alongside a multipart upload and a raw request body.
create_item_from_url()becomesupload_from_url(), a sibling ofupload_from_file()andupload_from_data(): it downloads the remote file, hands it towp_handle_sideload(), and returns the same data they return.insert_attachment()picks the handler andcreate_item()does the rest for every path, so the three items are fixed by construction rather than by re-implementing them. That's your second suggestion @jeremyfelt - it turned out to fit the existing shape of the class better than merging the twocreate_itemmethods.The fourth item is separate and is just an explicit 400: a request that supplies both a file and a
urlwas silently discarding the file. The check looks at the multipart file params and at the Content-Disposition filename, since that's howupload_from_data()identifies a raw body upload.Also added the
is_wp_error()check on the pre-hook you mentioned, so an error returned fromrest_pre_insert_attachmentis honored instead of being used as if it were an attachment.Trac ticket: https://core.trac.wordpress.org/ticket/65810
Two things worth a look
upload_from_url()is a rename ofcreate_item_from_url(), which was added in [62659] this cycle and isn't in any released version, so there's no back-compat cost. But it is aprotectedmethod that's been sitting in beta, so flagging it rather than burying it.upload_filescheck that was inside the old method.create_item_permissions_check()already denies that capability before the callback runs, so the check was unreachable through the endpoint - its 403 could never actually be returned, the permission check's 400 always won first. The test for it now dispatches through the endpoint instead of invoking the method by reflection. Happy to put it back if you'd rather keep the belt and braces.Sideloading also picks up two things it didn't have, both just from joining the shared path: the parent post's date is used to place the file in the uploads folder, matching
media_handle_upload(), and exif title and caption defaults are read from the image.Note this doesn't change anything for the editor. As you worked out, "Upload to Media Library" and the pre-publish external media panel both send only
postandurl, so they never hit any of it.Stacked on two other PRs
This branch includes #12825 (size ceiling on the sideload path) and #12833 (always register the
urlargument), since all three touch these same two methods. The diff against trunk therefore shows all three. The parity commit itself is the only new one here: 0b7ea15.How has this been tested
urltogether are rejected, a raw body upload and aurltogether are rejected, the request fields are applied, the three insert hooks fire, and an error fromrest_pre_insert_attachmentis honored. All five fail without the change - I checked by stashing the source change and re-running.WP_Test_REST_Attachments_Controllerclass: 201 tests on single site and 207 on multisite, 2 skipped in each, 0 failures.restapigroup: 3554 tests. One pre-existing unrelated error,Test_oEmbed_Controller::test_proxy_with_classic_embed_provider, which fails identically with the change stashed.phpcs --standard=phpcs.xml.distclean on both changed files.wp-api-generated.jsneeds no update.Manual, worth trying if you're testing:
POST /wp/v2/mediawith aurlplustitle,caption,descriptionandalt_text. Before this the fields are dropped; after it they're stored.rest_insert_attachmentandwp_after_insert_postand post the same request. Neither fires before, both do after.POST /wp/v2/mediawith both a file and aurl. Before this the file is silently discarded; after it the request returns a 400.Types of changes
create_item_from_url()withupload_from_url(), returningwp_handle_sideload()data like the other upload handlers.insert_attachment()so the URL path shares the insert, hooks, fields and terms handling.url.prepare_item_for_database()for aWP_Errorininsert_attachment().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 PR. I reviewed and verified the behavior and test results myself.