Make WordPress Core

Opened 4 weeks ago

Last modified 2 weeks ago

#61113 new defect (bug)

WP_REST_Templates_Revisions_Controller not checking parent ID correctly

Reported by: rockfire's profile rockfire Owned by:
Milestone: 6.6 Priority: normal
Severity: normal Version: 6.4
Component: REST API Keywords: has-patch needs-unit-tests
Focuses: rest-api Cc:

Description

When doing a call to the WP_REST_Templates_Revisions_Controller->get_items_permission_check() with an empty WP_REST_Request (so without a parent parameter) I get a Deprecation notice:

Deprecated: explode(): Passing null to parameter #2 ($string) of type string is deprecated in [..]\wp-includes\block-template-utils.php on line 1074

This is because the get_items_permission_check() does a call to get_parent( $request['parent'] ), which means the parameter is null. The function uses this parameter for a call to get_block_template() without checking if it is even valid.

Other Revision Controllers do actually check if the parent parameter is valid. So in my opinion a check should be added to the WP_REST_Template_Revisions_Controller.

Change History (7)

This ticket was mentioned in PR #6472 on WordPress/wordpress-develop by @rockfire.


4 weeks ago
#1

  • Keywords has-patch added

Prevent deprecation warning when calling WP_REST_Templates_Revisions_Controller->get_items_permission_check() with an ampty WP_REST_Request

#2 @swissspidy
4 weeks ago

  • Component changed from General to REST API
  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.6

#3 @swissspidy
4 weeks ago

  • Version changed from 6.5 to 6.4

@TimothyBlynJacobs commented on PR #6472:


3 weeks ago
#4

Thanks for the ping @swissspidy!

I'm actually not sure about this fix. I _do not_ think it is valid to bypass WP_REST_Server and send request objects to controller directly. I do not think we want to set the precedent that you can. A controller should be able to expect that parameter validation and sanitization has applied, for instance.

If you want to avoid making an actual REST API request, you should use rest_do_request to properly dispatch an internal REST API request.

If you can trigger this error through a regular REST API request that passes through the REST API server, then let's patch it. But the test should reflect that as well.

@rockfire commented on PR #6472:


2 weeks ago
#5

I'm actually not sure about this fix. I _do not_ think it is valid to bypass WP_REST_Server and send request objects to controller directly. I do not think we want to set the precedent that you can. A controller should be able to expect that parameter validation and sanitization has applied, for instance.

If you want to avoid making an actual REST API request, you should use rest_do_request to properly dispatch an internal REST API request.

If you can trigger this error through a regular REST API request that passes through the REST API server, then let's patch it. But the test should reflect that as well.

@TimothyBJacobs I must disagree with you. One of the key principles is that you must always validate your input, you cannot simply assume that the input is of type X. The fact that at this point we cannot think of a way to do a REST API request to cause this error, does in my opinion not mean you shouldn't validate the input.

I can understand that you don't want to set the precendent that you can bypass WP_REST_Server, but it is simply a fact that you can. I can do so for all other endpoints of the WP Core and none of them throw such an error because they do validate their input.

The fact that the docblock is stating an int is expected, while a string is what is actually supplied (and expected by th next function), suggest to me that this code has flaws in it that should be fixed.

@TimothyBlynJacobs commented on PR #6472:


2 weeks ago
#6

I must disagree with you. One of the key principles is that you must always validate your input, you cannot simply assume that the input is of type X. The fact that at this point we cannot think of a way to do a REST API request to cause this error, does in my opinion not mean you shouldn't validate the input.

The REST API has defined semantics for validation and sanitization. They are applied via the WP_REST_Server and the WP_REST_Request objects. By your argument, all of the REST API controllers should apply the same validation the Schema does by hand, just because a developer _could_ pass an unvalidated request object into their methods directly.

Doing the same validation twice, however, doesn't make any sense. It completely defeats the purpose of how the REST API handles validation.

but it is simply a fact that you can.

The contract that the callback methods have is that they are to be passed request objects that have passed through the server. There is always an API layer that you can bypass, but doing so would be incorrect. It's like saying that just because a developer might not call the correct current_user_can function, that update_option should perform it's own permission checks.

If you can trigger this bug by making an actual request via rest_do_request, then we should absolutely fix that. But if the REST API controller method can't even be called when trying to make a request in this way, then that signals that validation _is_ being applied, but it's happening at the server level.

#7 @swissspidy
2 weeks ago

In 58156:

Docs: Improve docblock for WP_REST_Template_Revisions_Controller::get_parent().

Props rockfire.
See #60699, #61113.

Note: See TracTickets for help on using tickets.