Skip to content

REST API: Return an error when updating a setting that is not registered. - #14867

Open
Sabbir345 wants to merge 4 commits into
WordPress:trunkfrom
Sabbir345:fix/41604-settings-unknown-params
Open

Sabbir345 wants to merge 4 commits into
WordPress:trunkfrom
Sabbir345:fix/41604-settings-unknown-params

Conversation

@Sabbir345

Copy link
Copy Markdown

Trac ticket: https://core.trac.wordpress.org/ticket/41604

A request to /wp/v2/settings that contains a setting that is not registered (or not exposed with show_in_rest) returns a 200 response, and the setting is silently ignored:

POST /wp/v2/settings  {"this_setting_does_not_exist": "value"}  → 200 (expected 400)

This PR returns a 400 rest_invalid_param error when the request body contains a setting that is not registered. The query string is not checked.

Why only the request body

The two previous fixes (r60301, r60357) were reverted in r61324 because they rejected valid requests that use the query string. The two open PRs, #10662 and #12967, use an allow-list of global parameters instead, but any parameter missing from that list breaks requests again. For example, on a site with plain permalinks every REST request is sent as /?rest_route=/wp/v2/settings, and WP_REST_Server::serve_request() passes $_GET as-is, so rest_route becomes a request parameter. With either open PR applied, saving any setting on such a site returns 400 Invalid parameter(s): rest_route.

The query string can contain:

  • settings, which are supported there in place of the body (the Web Stories case in comment:33),
  • global parameters such as rest_route and _locale (comment:7),
  • parameters added by other code, such as cache busters.

None of these can be reliably told apart from a mistyped setting, so the query string is left as it is. The request body has no such ambiguity, apart from global parameters, which are prefixed with an underscore and are ignored.

Changes

  • WP_REST_Settings_Controller::update_item() returns 400 rest_invalid_param ("Invalid parameter(s): …") if the form or JSON body contains a setting that is not registered. No settings are updated in that case.
  • Body parameters prefixed with an underscore (_wpnonce, _method, _locale, …) are ignored.
  • The error message reuses the existing Invalid parameter(s): %s string, so there is no new string to translate.

Notes

  • An empty request is not rejected. Rejecting it is what broke requests that pass settings only in the query string, and the ticket is about unknown settings.
  • An unknown setting passed only in the query string is still ignored, as before.
  • Follow-up to r60301, r60357, and r61324.

Testing

New tests in tests/phpunit/tests/rest-api/rest-settings-controller.php:

  • An unregistered setting in a form body (POST) and a JSON body (POST and PUT) returns a 400, and neither setting is updated. These fail on trunk.
  • The request still succeeds with ?rest_route=/wp/v2/settings, ?_locale=user, an unknown query parameter, a setting in the query string with an empty body, and _wpnonce in the body.
npm run test:php -- --filter WP_Test_REST_Settings_Controller
npm run test:php -- --group restapi

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.

…red.

Previously, a request to the `/wp/v2/settings` endpoint that contained a setting that is not registered, or not exposed with `show_in_rest`, returned a 200 response and the setting was silently ignored.

The request body is now checked, and a 400 `rest_invalid_param` error is returned if it contains a setting that is not registered. No settings are updated in that case.

The query string is not checked. Besides settings, which are supported there in place of the request body, it carries global parameters such as `rest_route` on sites without pretty permalinks and `_locale` from the block editor, as well as parameters added by other code. Earlier attempts to fix this were reverted because they rejected requests like these. Global parameters in the request body, which are prefixed with an underscore, are ignored as well.

Follow-up to [60301], [60357], [61324].

See #41604.
@github-actions

Copy link
Copy Markdown

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs.

Core Committers: Use this line as a base for the props when committing in SVN:

Props sabbir345.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.


Props Bot commands

You can trigger Props Bot actions by commenting on this PR:

  • @propsbot add <username> to give props to a GitHub user. Requires write access.
  • @propsbot remove <username> to leave a user out of the props. Requires write access.
  • @propsbot refresh to refresh this list. Adding the props-bot label works too.

@github-actions

Copy link
Copy Markdown

Test using WordPress Playground

The 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

  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant