Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#42238 closed defect (bug) (fixed)

Notice when Rest Comments controler is checking permision for a non existent post type

Reported by: dragosh635's profile dragosh635 Owned by: kadamwhite's profile kadamwhite
Milestone: 5.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: good-first-bug has-patch has-unit-tests commit
Focuses: Cc:

Description

The problem comes when I try to make a rest get request for the comments. The request is something like that http://demo.wp-api.org/wp-json/wp/v2/comments ( taken from the docs, in my local environment ), made in postman.

The function check_read_post_permission is usign WP_REST_Posts_Controller to get a post controller based on the post type.

<?php
protected function check_read_post_permission( $post, $request ) {
                $posts_controller = new WP_REST_Posts_Controller( $post->post_type );
                $post_type        = get_post_type_object( $post->post_type );

Next, the WP_REST_Posts_Controller constructor tries to find the post type object in order to establish the rest_base variable. The problem is that the post type doesn't exists anymore ( for me this happened because I deactivate a plugin, woocommerce in my case, but I think this could happen in other cases also ). So, the get_post_type_object function returns null and than it tries to get a property from a null object -> Notice: Trying to get property of non-object in
...

<?php
public function __construct( $post_type ) {
                $this->post_type = $post_type;
                $this->namespace = 'wp/v2';
                $obj = get_post_type_object( $post_type ); //$obj is null

                $this->rest_base = ! empty( $obj->rest_base ) ? $obj->rest_base : $obj->rest_base; //notice is thrown

Obviously since I am on my local machine, the WP_DEBUG was set on true and if I set it on false, the notice doesn't appear anyomore. But still, I think it's a small issue, that could be solved in a more elegant way.

To solve this I would probably suggest a check for null on the $obj and maybe in that case rest_base could become a default value or an error should be thrown ( I am not sure what would be the best approach )

If there is someone that needs more information on this, let me know and I will do my best to help.

Attachments (5)

42238.patch (543 bytes) - added by subrataemfluence 6 years ago.
#42238.patch (1.1 KB) - added by imani3011 4 years ago.
New patch for #42238
42238-updated.patch (1.2 KB) - added by imani3011 4 years ago.
Updated Patch for #42238 fix
42238-unittest.patch (1.9 KB) - added by imani3011 4 years ago.
Unit test patch file
42238.diff (2.2 KB) - added by TimothyBlynJacobs 4 years ago.

Download all attachments as: .zip

Change History (31)

#1 @subrataemfluence
6 years ago

  • Keywords reporter-feedback added

Hi @dragosh635 welcome to trac and thanks for the ticket.

Here is how I tried to reproduced your issue:

Step 1:

  1. Created a plugin to create a custom post type named "dealers" to collect dealer's name, email and phone number
  2. Activated the plugin
  3. Added one record (published)
  4. The endpoint /wp-json/wp/v2/dealers gives desired output with one dealer.

Step 2:

  1. Deactivated the plugin
  2. Running the same end point again does not give me any result and shows me the following message instead:
{"code":"rest_no_route","message":"No route was found matching the URL and request method","data":{"status":404}}

which is different from yours.

I tested with WP_DEBUG set to both true and false separately. Either cases the above message stayed the same for me.

Please note, I did not test with Woocommerce as it is a third party plugin and preferred to stay with WP core functionality for creating and registering above custom post type.

If you think I made any mistake in the above process please let me know exactly how I can reproduce the issue. However, I would always prefer not to use any third party plugin to test WordPress core functionality :)

#2 @dragosh635
6 years ago

Hello, @subrataemfluence

Thank you for looking into the matter and sorry for not beeing able to respond earilier.

I remade the test with a custom plugin in the same way you did ( without using woocommerce ) and the notice was still there.

Your first few steps were ok ( 1 - 3 ), from than I think it was a misunderstanding. Basically I was speaking of getting the comments from the post using the rest api. I think your example refers to get the posts created with that custom post type.

So, my error is when getting the comments using the rest api and in the comments list there is a comment on a post of a custom post type that not longer exists.

Your next steps in debugging would be: ( the post type needs to be set to accept comments and enable rest api). I used this function to create my custom post type

<?php
register_post_type( 'bug_post',
                array(
                        'labels'       => array(
                                'name'          => __( 'Bug Post' ),
                                'singular_name' => __( 'Bugs' )
                        ),
                        'public'       => true,
                        'has_archive'  => true,
                        'supports'     => array( 'title', 'editor', 'author', 'excerpt', 'comments', 'revisions' ),
                        'show_in_rest' => true,
                )
        );

4.After publishing the post, go on it and add a few comments.
5.Run the next rest route http://{insert_your_local_url}/wp-json/wp/v2/comments -> you should get the list of comments
6.Disable the plugin
7.Run the rest route again -> a PHP notice will appear ( only when WP_DEBUG is set to true )

I hope that this clarifies the issue.
Thank you for the heads up to not use a third party plugin in my testing.

If there is anything else that I can clear please feel free to comment on my ticket.

Thanks,
Dragos

#3 @dragosh635
6 years ago

  • Keywords reporter-feedback removed

#4 @subrataemfluence
6 years ago

Thank you for driving me to the right direction. I now can reproduce the issue. I am trying to dig more into it.

#5 @subrataemfluence
6 years ago

  • Keywords needs-patch added

#6 @subrataemfluence
6 years ago

  • Keywords has-patch added; needs-patch removed

#7 @subrataemfluence
6 years ago

Hello @dragosh635, I am submitting a patch which works for me. When you have time can you please verify whether the same works for you as well?

Thank you!

#8 follow-up: @dragosh635
6 years ago

Hello, @subrataemfluence

The patch you submitted works for me also and I think it's a good solution.
Please keep me posted if it was accepted.

Dragos

#9 in reply to: ↑ 8 @subrataemfluence
6 years ago

Thank you for testing the patch and confirming that it works for you too!

Replying to dragosh635:

Hello, @subrataemfluence

The patch you submitted works for me also and I think it's a good solution.
Please keep me posted if it was accepted.

Dragos

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


5 years ago

#11 @TimothyBlynJacobs
5 years ago

  • Owner set to TimothyBlynJacobs
  • Status changed from new to accepted

Going to take a look at this. Is related to #40383.

#12 follow-up: @TimothyBlynJacobs
4 years ago

  • Keywords needs-patch needs-unit-tests good-first-bug added; has-patch removed
  • Milestone changed from Awaiting Review to Future Release
  • Version changed from 4.8.2 to 4.7

Thanks for the patch @subrataemfluence! And apologies for the delay in getting to your ticket @dragosh635.

Looking at the code, WP_REST_Posts_Controller::check_read_permission() will return false if the post type isn't registered. So instead of trying to adjust the WP_REST_Posts_Controller constructor to allow for invalid post types, I think it'd be best to short circuit WP_REST_Comments_Controller::check_read_post_permission() and return false if the post type does not exist.

@subrataemfluence are you interested in writing a new patch?

#13 in reply to: ↑ 12 @subrataemfluence
4 years ago

Thank you @TimothyBlynJacobs for giving me the opportunity. Unfortunately I am travelling right now and won't be able to work on this for next few days. I am fine and actually would be great if somebody else completes it. :)

Replying to TimothyBlynJacobs:

Thanks for the patch @subrataemfluence! And apologies for the delay in getting to your ticket @dragosh635.

Looking at the code, WP_REST_Posts_Controller::check_read_permission() will return false if the post type isn't registered. So instead of trying to adjust the WP_REST_Posts_Controller constructor to allow for invalid post types, I think it'd be best to short circuit WP_REST_Comments_Controller::check_read_post_permission() and return false if the post type does not exist.

@subrataemfluence are you interested in writing a new patch?

@imani3011
4 years ago

New patch for #42238

#14 @imani3011
4 years ago

  • Keywords has-patch added; needs-patch removed

Hello @TimothyBlynJacobs

I have created a patch the way you suggested. It works at my end. When you have time can you please verify whether the same works for you as well?

Thanks in advance!

Version 0, edited 4 years ago by imani3011 (next)

#15 @TimothyBlynJacobs
4 years ago

Thanks for working on a patch @imani3011!

Some minor corrections:

We can just check if the value is non-falsey instead of explicitly checking that the value is_null. For example: https://github.com/WordPress/wordpress-develop/blob/master/src/wp-admin/edit.php#L35

WordPress also has slightly different PHP coding standards. So there should be a space around the parentheses.

Are you interested in writing a unit test to test this fix? It could be added to the WP_Test_REST_Comments_Controller test class.

#16 @subrataemfluence
4 years ago

@imani3011, Thank you for submitting the patch. Just a quick note about the naming convention for patch versioning. Please use ticket_number.n.diff or ticket_number.n.patch. It gives a better understanding.

Like here the name of your patch file could be 42238.2.patch.
All patches usually use this convention.

Thank you!

@imani3011
4 years ago

Updated Patch for #42238 fix

#17 @imani3011
4 years ago

@subrataemfluence Thanks for addressing this. I will take care of these convention from now onwards. Unfortunately I read your comment after uploading a new patch. Will take care of this in future,

Thanks again!

#18 @imani3011
4 years ago

@TimothyBlynJacobs Thank you for guiding me the more appropriate way. I have made the changes and created a new patch
https://core.trac.wordpress.org/attachment/ticket/42238/42238-updated.patch

When you have time can you please verify whether the same works for you as well?

Yes, I am interested in writing a unit test to test this fix. Will provide it soon, probably by tomorrow. Is that ok?

Thanks again!

Last edited 4 years ago by imani3011 (previous) (diff)

@imani3011
4 years ago

Unit test patch file

#19 @imani3011
4 years ago

@TimothyBlynJacobs I have added a patch file for Unit test as well https://core.trac.wordpress.org/attachment/ticket/42238/42238-unittest.patch.

Awaiting feedback from you,
Thanks!

#20 @TimothyBlynJacobs
4 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed
  • Milestone changed from Future Release to 5.4

Thanks for adding the unit test @imani3011!

I uploaded a patch combining your two patches into one and made some code style fixes

Instead of checking for a 200 response on the collection, I changed the test to specifically test for a 403 response. That way we are sure the comment isn't being inadvertently exposed.

#21 @imani3011
4 years ago

@TimothyBlynJacobs Thank you very much!

This ticket was mentioned in Slack in #core-restapi by timothybjacobs. View the logs.


4 years ago

#23 @kadamwhite
4 years ago

  • Keywords commit added
  • Owner changed from TimothyBlynJacobs to kadamwhite
  • Status changed from accepted to assigned

#24 @kadamwhite
4 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 47036:

REST API: Short-circuit comment controller permissions check if commented-upon post type does not exist.

Props imani3011, dragosh635, subrataemfluence, timothyblynjacobs.
Fixes #42238.

#25 @audrasjb
4 years ago

  • Keywords needs-dev-note added

#26 @audrasjb
4 years ago

  • Keywords needs-dev-note removed

Not sure this one needs a dev note.
Feel free to ping me is it's needed.

Note: See TracTickets for help on using tickets.