Networks and Sites: Improve search term handling in wp_ajax_autocomplete_user() - #11530
Networks and Sites: Improve search term handling in wp_ajax_autocomplete_user()#11530rajeshcpr wants to merge 13 commits into
Conversation
User-supplied search term is concatenated directly into the get_users() search argument without sanitize_text_field() or wp_unslash().
|
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. |
The user autocomplete Ajax endpoint had no test coverage, so the search term sanitization added on this branch was unverified. These tests lock in that behavior: HTML tags are stripped before the term reaches get_users(), and a missing `term` request variable no longer raises a PHP warning. Both fail without the sanitization change. The remaining tests document the surrounding contract that the sanitization must not break: the response shape, the `add` vs `search` autocomplete types, the `user_email` field, the `promote_users` and `manage_network_users` capability checks, the `autocomplete_users_for_site_admins` filter, and large networks. The handler bails out early unless multisite is active, so the class is marked with the `ms-required` group. Blank lines are added around the `$term` assignment to satisfy the alignment sniff. See #65051. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses unsafe handling of the term request parameter in the wp_ajax_autocomplete_user() Ajax handler by ensuring the value is unslashed and sanitized before being used in the get_users() search query, and adds PHPUnit coverage for the expected behavior.
Changes:
- Sanitize
$_REQUEST['term']viawp_unslash()+sanitize_text_field()before building theget_users()searchargument. - Add a dedicated Ajax test suite for the
autocomplete-useraction, covering expected results, permissions, large-network denial, and term sanitization/error behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/wp-admin/includes/ajax-actions.php |
Sanitizes the term request value before using it in the user query search string. |
tests/phpunit/tests/ajax/wpAjaxAutocompleteUser.php |
Adds PHPUnit tests to verify autocomplete-user behavior, including sanitization and permission/denial cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e_user() Note that the search term is now sanitized and that a missing, non-string, or empty term results in a `0` response instead of an empty array. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Since wildcards are appended to the search term, a term consisting only of asterisks would be reduced to an empty search by WP_User_Query, causing all network users to be returned. Trim asterisks before the empty-term check so such a term short-circuits with a `0` response instead. Add tests for an asterisk-only term and for an asterisk-wrapped term, the latter of which still matches as before. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Cover the remaining guard arms in wp_ajax_autocomplete_user(): an empty or whitespace-only term and a non-string (array) term each result in a `0` response. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Convert the tag-stripping test to a data provider and add a case where tags wrap the searchable value. The existing script-element case exercises wp_strip_all_tags() removing an element along with its contents, while the new case exercises tags being stripped with their inner text retained. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
In `wp_ajax_autocomplete_user()`, unslash and sanitize the `term` request parameter before it is passed to `get_users()`. Unslashing fixes searching for an email address containing an apostrophe (valid per `is_email()`), which could previously never match because `wp_magic_quotes()` added a slash which `wpdb::esc_like()` then escaped as a literal. Note that the raw term was already safely handled in the user query, since `WP_User_Query` passes the search term through `wpdb::prepare()`, so this is a hardening and correctness fix rather than a security fix. Additionally, a missing, non-string, or empty term now short-circuits with a `0` response instead of returning an empty array, avoiding a PHP warning and needless user queries. Asterisks are also trimmed from the term given that wildcards are appended to it; a term consisting only of asterisks previously resulted in an empty search which matched all users on the network. Also introduce the `Tests_Ajax_wpAjaxAutocompleteUser` test class covering the Ajax action's search behavior, input handling, and capability checks. Developed in #11530. Follow-up to r19897, r20279. Props rajeshcp, wildworks, westonruter, liaison, gaurangsondagar, vgnavada, saadtajik. Fixes #65051. git-svn-id: https://develop.svn.wordpress.org/trunk@63016 602fd350-edb4-49c9-b593-d223f7449a82
In `wp_ajax_autocomplete_user()`, unslash and sanitize the `term` request parameter before it is passed to `get_users()`. Unslashing fixes searching for an email address containing an apostrophe (valid per `is_email()`), which could previously never match because `wp_magic_quotes()` added a slash which `wpdb::esc_like()` then escaped as a literal. Note that the raw term was already safely handled in the user query, since `WP_User_Query` passes the search term through `wpdb::prepare()`, so this is a hardening and correctness fix rather than a security fix. Additionally, a missing, non-string, or empty term now short-circuits with a `0` response instead of returning an empty array, avoiding a PHP warning and needless user queries. Asterisks are also trimmed from the term given that wildcards are appended to it; a term consisting only of asterisks previously resulted in an empty search which matched all users on the network. Also introduce the `Tests_Ajax_wpAjaxAutocompleteUser` test class covering the Ajax action's search behavior, input handling, and capability checks. Developed in WordPress/wordpress-develop#11530. Follow-up to r19897, r20279. Props rajeshcp, wildworks, westonruter, liaison, gaurangsondagar, vgnavada, saadtajik. Fixes #65051. Built from https://develop.svn.wordpress.org/trunk@63016 git-svn-id: http://core.svn.wordpress.org/trunk@62235 1a063a9b-81f0-0310-95a4-ce76da25c4cd
In `wp_ajax_autocomplete_user()`, unslash and sanitize the `term` request parameter before it is passed to `get_users()`. Unslashing fixes searching for an email address containing an apostrophe (valid per `is_email()`), which could previously never match because `wp_magic_quotes()` added a slash which `wpdb::esc_like()` then escaped as a literal. Note that the raw term was already safely handled in the user query, since `WP_User_Query` passes the search term through `wpdb::prepare()`, so this is a hardening and correctness fix rather than a security fix. Additionally, a missing, non-string, or empty term now short-circuits with a `0` response instead of returning an empty array, avoiding a PHP warning and needless user queries. Asterisks are also trimmed from the term given that wildcards are appended to it; a term consisting only of asterisks previously resulted in an empty search which matched all users on the network. Also introduce the `Tests_Ajax_wpAjaxAutocompleteUser` test class covering the Ajax action's search behavior, input handling, and capability checks. Developed in WordPress#11530. Follow-up to r19897, r20279. Props rajeshcp, wildworks, westonruter, liaison, gaurangsondagar, vgnavada, saadtajik. Fixes #65051. git-svn-id: https://develop.svn.wordpress.org/trunk@63016 602fd350-edb4-49c9-b593-d223f7449a82
This PR improves the handling of the
termrequest parameter inwp_ajax_autocomplete_user()and adds full test coverage for the Ajax action. Per the analysis on the Trac ticket, the previous code was not exploitable — the search term is passed throughwpdb::prepare()/wpdb::esc_like()byWP_User_Query, and the term is never reflected in the response — so this is a correctness and coding-standards improvement rather than a security fix.Changes to
wp_ajax_autocomplete_user()sanitize_text_field( wp_unslash( $_REQUEST['term'] ) )(the originally submitted change). Unslashing fixes searches for terms containing quotes or backslashes — notably email addresses containing an apostrophe (e.g.o'brien@example.com), which are valid peris_email()but could previously never match becausewp_magic_quotes()added a slash whichwpdb::esc_like()then escaped as a literal.0response instead of an empty[]array. This avoids a PHP warning for an undefined array key (or an array-to-string conversion warning whentermis submitted as an array) and skips the twoget_users()include/exclude queries that previously ran for nothing. This is not a change in behavior for core's own JS, since jQuery UI autocomplete enforcesminLength: 2and treats a0response as "no results".*wildcards,WP_User_Querytrims all leading/trailing asterisks; a term consisting only of asterisks would previously reduce to an empty search that matched all users on the network.@sincechangelog entry on the function.New tests
The new
Tests_Ajax_wpAjaxAutocompleteUserclass (15 tests,ms-required) covers:searchautocomplete type, and the email address is returned as the value whenautocomplete_field=user_email.addtype.pre_get_users) while the sanitized term still matches.$_GETviawp_slash()to simulatewp_magic_quotes(), proving the unslash fix).0response, while an asterisk-wrapped term still matches.-1withoutpromote_users,-1for site administrators by default, success when theautocomplete_users_for_site_adminsfilter is enabled, and-1on large networks.Trac ticket: https://core.trac.wordpress.org/ticket/65051
Use of AI Tools
AI assistance: Yes
Tool(s): Claude Code
Model(s): Claude Fable 5
Used for: Reviewing the original patch, authoring the follow-up commits (input validation, asterisk trimming,
@sincedocumentation) and the expanded test suite, and drafting this description. All changes were directed, reviewed, and edited by @westonruter.Appendix: Original PR title and description
Title:
$_REQUEST['term']used unsanitized in user search queryThis 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.