XML-RPC: mw_newMediaObject, check 4th arg is an array - #12482
Closed
josephscott wants to merge 21 commits into
Closed
XML-RPC: mw_newMediaObject, check 4th arg is an array#12482josephscott wants to merge 21 commits into
josephscott wants to merge 21 commits into
Conversation
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR hardens the XML-RPC mw_newMediaObject endpoint to avoid fatals when the 4th argument (attachment data struct) is not an array, and adds a PHPUnit regression test for that case.
Changes:
- Add an
is_array()guard for$args[3]inwp_xmlrpc_server::mw_newMediaObject()and return anIXR_Error( 400, ... )for invalid data. - Add a unit test asserting that passing a non-array 4th argument returns an
IXR_Errorinstead of triggering a fatal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/phpunit/tests/xmlrpc/wp/uploadFile.php | Adds a regression test for non-array attachment data passed to mw_newMediaObject. |
| src/wp-includes/class-wp-xmlrpc-server.php | Adds input validation to return an IXR_Error when attachment data is not an array. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
Author
|
The test failures appear unrelated to this change. |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Checking that the data argument is an array left the name, type, and bits members being read unconditionally, so a struct omitting any of them still emitted an undefined array key notice before the upload failed. Require the name in the existing guard, since an absent or non-string name can never produce a successful upload. Default the optional type and bits members instead, preserving the existing outcomes for clients that omit them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The data argument was defaulted inline, so a call with fewer than four arguments reported the same invalid attachment data error as a call whose struct was malformed. Guard with minimum_args instead, matching the other methods on the server and reporting the insufficient arguments error the rest of them already use. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reported fatal error was reachable without credentials, so the validation has to run before the login for it to be prevented. Every existing test supplies a valid user, leaving that ordering unguarded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The data struct was read before the login was attempted, which is what made the fatal error reachable without credentials. Validating it there in turn made this the only method on the server to return an error ahead of its login. Read and validate the struct once the request is authenticated and the upload capability is confirmed, as every other method does. An anonymous request now stops at the login, so the struct is never touched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fourth argument was documented only as "Data", which left the shape of the struct undiscoverable from the docblock. Now that `name` is required and `type`, `bits` and `post_id` have explicit fallbacks, expand it into a nested hash listing each member, its type and its default. `post_id` was already read by the method but had never been documented. Use the nested-hash form and the "top-level arguments must be ordered as documented" wording already used by `wp_newPost()` in the same class. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Only the `name` member was type-checked, so a client could still send a struct for `type` or `bits` and get a non-string through: - `bits` reached `fwrite()` by way of `wp_upload_bits()`, which throws `TypeError: fwrite(): Argument WordPress#2 ($data) must be of type string, array given` on PHP 8. - `type` survived `sanitize_mime_type()` — `preg_replace()` returns an array for an array subject — and reached the database as the attachment's post MIME type. Both are the same class of bug as the reported one, just behind an authenticated request that can `upload_files`. Reject them alongside `name`. Values arriving over XML-RPC are unaffected: `IXR_Message` decodes `base64` and `string` values to strings, and an empty `<value/>` omits the member entirely, so the `??` fallbacks still apply. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An empty string satisfies the `is_string()` check on the `name` member, and
`sanitize_file_name()` runs after it, so a name built only from characters
the sanitizer removes still reached `wp_upload_bits()` as an empty string.
That surfaced to the client as:
IXR_Error(500): Could not write file (Empty filename).
Three things were wrong with it. A 500 tells the client the server failed
and the call is worth retrying, when in fact the request was malformed. The
message renders with a doubled space, since the file name interpolated into
it is empty. And a missing or non-string name already returns a 400, so the
same user error was reported two different ways.
Check the name once it has been sanitized, and return the same 400 used for
the other unusable names. Reusing the existing string keeps this from adding
to the translatable strings.
`''`, `'...'`, `'---'`, `'___'`, `' '`, `'///'` and `'?'` are all reduced to
an empty string, so all of them are covered.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The `type` member is optional, and when it was absent the attachment was stored with an empty `post_mime_type`. That leaves a usable file that the site cannot find: it is not matched by media library queries filtered by MIME type, it is not counted under any type by `wp_count_attachments()`, and `wp_attachment_is()` does not recognize it, so an image gets no intermediate sizes and `wp_get_attachment_image()` renders nothing. It applies to every kind of upload, not just images — a PDF, MP3, MP4, text file and archive are all stored the same way. `wp_upload_bits()` already returns the type it determined from the file name, which is the same one that decided whether the upload was permitted at all, so use it when the client did not supply one. Only use it when it is a string. `wp_check_filetype()` reports false for an extension WordPress does not recognize, which reaches this point when the user can upload unfiltered, and the `wp_handle_upload` filter can replace the returned array with anything. An explicitly supplied type is left alone, including one that does not match the file name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This reverts commit 6e0e54e. Storing an attachment with an empty `post_mime_type` is long-standing behaviour rather than anything this branch introduced: before the argument validation was added, an absent `type` member raised an undefined array key notice and left the same empty string behind. Silencing the notice did not change what gets stored. It is also only half the problem. An explicitly supplied type is never checked against the file name, so a struct claiming `text/plain` for a `.jpg` produces exactly the attachment the fallback was meant to prevent, and that path is untouched by it. Deciding what to do about a type the client got wrong is a larger behaviour change and belongs with its own ticket. Keep this branch to the reported fatal error and the argument validation around it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every other XML-RPC method that checks a capability has a `test_incapable_user` pinning it, but `mw_newMediaObject()` had none. It matters more here than usual. The attachment data is now read after the capability check, so the order of the two decides whether a user who cannot upload is told that or is told their data is malformed. Nothing recorded that, and moving the validation back above the capability check passes every other test in the file. Pass unusable data along with a subscriber, so the test fails if the two are ever reordered rather than only when the capability check disappears. `wp_newPost()` pins the same property the same way, by sending an empty struct. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
westonruter
approved these changes
Aug 4, 2026
pento
pushed a commit
that referenced
this pull request
Aug 4, 2026
Passing anything other than a struct as the fourth argument caused a fatal error, and because the struct was read before the login was attempted, an unauthenticated request was enough to trigger it. Read and validate the struct only once the request is authenticated and the `upload_files` capability is confirmed, as every other method on the server does, and reject a call with too few arguments using `minimum_args()`. The `name`, `type` and `bits` members must all be strings: a struct sent for `bits` reached `fwrite()` by way of `wp_upload_bits()` and threw a `TypeError`, while one sent for `type` survived `sanitize_mime_type()` to reach the database as the attachment's post MIME type. A `name` left empty by `sanitize_file_name()` is now reported as a malformed request too, rather than as the server failure `wp_upload_bits()` produced for it. The fourth argument is expanded into a nested hash in the documentation, covering the previously undocumented `post_id` member. Tests cover each rejected shape, the optional members that remain tolerated when absent, and the ordering of the login and capability checks ahead of the validation. Developed in #12482. Follow-up to r32579, r53881. Props josephscott, westonruter, mukesh27. See #65600. Fixes #65611. git-svn-id: https://develop.svn.wordpress.org/trunk@63006 602fd350-edb4-49c9-b593-d223f7449a82
markjaquith
pushed a commit
to markjaquith/WordPress
that referenced
this pull request
Aug 4, 2026
Passing anything other than a struct as the fourth argument caused a fatal error, and because the struct was read before the login was attempted, an unauthenticated request was enough to trigger it. Read and validate the struct only once the request is authenticated and the `upload_files` capability is confirmed, as every other method on the server does, and reject a call with too few arguments using `minimum_args()`. The `name`, `type` and `bits` members must all be strings: a struct sent for `bits` reached `fwrite()` by way of `wp_upload_bits()` and threw a `TypeError`, while one sent for `type` survived `sanitize_mime_type()` to reach the database as the attachment's post MIME type. A `name` left empty by `sanitize_file_name()` is now reported as a malformed request too, rather than as the server failure `wp_upload_bits()` produced for it. The fourth argument is expanded into a nested hash in the documentation, covering the previously undocumented `post_id` member. Tests cover each rejected shape, the optional members that remain tolerated when absent, and the ordering of the login and capability checks ahead of the validation. Developed in WordPress/wordpress-develop#12482. Follow-up to r32579, r53881. Props josephscott, westonruter, mukesh27. See #65600. Fixes #65611. Built from https://develop.svn.wordpress.org/trunk@63006 git-svn-id: http://core.svn.wordpress.org/trunk@62225 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://core.trac.wordpress.org/ticket/65611
AI assistance: Yes
Tool(s): Claude
Model(s): Opus 4.8
Used for: Writing the unit test
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.