WP_REST_Templates_Revisions_Controller not checking parent ID correctly

[rockfire]

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.

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

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.

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.

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.

[swissspidy]

In 58156:

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

Props rockfire.
See #60699, #61113.

[martin.krcho]

It was agreed in ​https://github.com/WordPress/wordpress-develop/pull/6472#discussion_r1638104809 that this is not something that needs fixing because this this bug cannot be triggered by making an actual request via rest_do_request.

[sabernhardt]

(The documentation commit is tied to the 6.6 milestone by #60699.)