WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 6 months ago

#39881 closed defect (bug) (fixed)

`WP_REST_Posts_Controller::check_read_permission()` should check if `$parent` exists before calling itself

Reported by: GhostToast Owned by: rachelbaker
Milestone: 4.7.4 Priority: normal
Severity: normal Version: 4.7
Component: REST API Keywords: has-patch has-unit-tests fixed-major
Focuses: rest-api Cc:

Description

In WP_REST_Posts_Controller::check_read_permission() there ends up being a check for if the post_status of the post in question is 'inherit' and if the post_parent is greater than 0. It then checks the parent for permissions regarding the child. However, sometimes (as I have found with attachments), the child believes it has a parent, but the parent is missing. This results in a null post being sent to $this->check_is_post_type_allowed() which fails early and throws error about property of non-object.

I believe if the parent is missing, the next sequence should take precedence: which is that if post_status is 'inherit', check_read_permission() returns true. This would essentially be the same as if the post_parent value was set to 0.

A simple check if $parent has a value before passing it to a recursive call of check_read_permission() can alleviate this.

To reproduce this error, try to access a post with attachments, using _embed on the REST API, where one of the attachments has a post_parent that is invalid (integer that doesn't exist in corresponding wp_posts table).

Attachments (2)

39881.patch (647 bytes) - added by GhostToast 7 months ago.
Check if parent exists before using it
39881.2.diff (2.2 KB) - added by rachelbaker 6 months ago.
Added unit tests for returning media file with an invalid parent id

Download all attachments as: .zip

Change History (13)

@GhostToast
7 months ago

Check if parent exists before using it

#1 @rachelbaker
7 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7.3
  • Owner set to rachelbaker
  • Status changed from new to reviewing
  • Version changed from trunk to 4.7

This ticket was mentioned in Slack in #core by jnylen. View the logs.


7 months ago

#3 @jnylen0
7 months ago

  • Milestone changed from 4.7.3 to 4.7.4

Punting, 4.7.3 is upon us.

#4 @jnylen0
7 months ago

  • Keywords dev-feedback needs-unit-tests added

I investigated this a bit more, thinking that it might be pretty simple to fix, but I'm going to stick with my earlier recommendation to punt. Here's why:

  • It's not clear to me how a post would get into this situation in the first place. wp_delete_post handles this situation by resetting the post_parent of any attachments, so this is likely to be very uncommon.
  • WP core itself is pretty broken when this situation does occur. I forced it manually for the following attachment - https://nylen.io/wp-dev/i-need-dis-otter/ - you'll note the page 404s, which is also the case when I'm logged in. However, wp-admin still points here for the attachment description page, and this is the value of the link field in the REST API response. (The API URL is https://nylen.io/wp-dev/wp-json/wp/v2/media/21, which I can only view when authenticated.)

Given the above, this needs more discussion, probably a broader fix than just the REST API, and of course unit tests. The existing behavior is kind of broken, but at least it's consistently broken in wp-admin and the REST API.

#5 @GhostToast
7 months ago

At the very least I would argue some error handling be in place. It is assumed that because a non-zeropost_parent property is present, a valid $post object will exist with that ID. What about if we check for get_post() having returned something useful? get_post_status() handles this a bit, can see similar example here: https://core.trac.wordpress.org/browser/tags/4.7.2/src/wp-includes/post.php?marks=#L655

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


6 months ago

@rachelbaker
6 months ago

Added unit tests for returning media file with an invalid parent id

#7 @rachelbaker
6 months ago

  • Keywords has-unit-tests commit added; dev-feedback needs-unit-tests removed

In 39881.2.diff I added two unit tests. The first test returns the media file successfully with the post_parent is invalid but the post_status is inherit. The second returns an error if the post_parent is invalid but the post_status is auto-draft.

#8 @rachelbaker
6 months ago

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

In 40306:

REST API: Confirm the parent post object of an attachment exists in WP_REST_Posts_Controller::check_read_permission().

Avoid a PHP Error when attempting to embed the parent post of an attachment, when the parent post ID is invalid. Instead check if the parent post object exists before checking the read permission for the parent post.

Props GhostToast.
Fixes #39881.

#9 @rachelbaker
6 months ago

@jnylen0 This can occur when a post and related post_meta are deleted with a MySQL query, instead of using wp_delete_post(). An attachment can have only one post_parent in WordPress but be used in multiple posts (talk about functionality that is kinda broken). Is this situation common? NO. Should our REST API endpoints return invalid data due to PHP Errors when it does? I wouldn't consider attachment pages as an apple to apple comparison for our media endpoint.

#10 @rachelbaker
6 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Re-opened for porting to the 4.7.4 branch.

#11 @swissspidy
6 months ago

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

In 40337:

REST API: Confirm the parent post object of an attachment exists in WP_REST_Posts_Controller::check_read_permission().

Avoid a PHP Error when attempting to embed the parent post of an attachment, when the parent post ID is invalid. Instead check if the parent post
object exists before checking the read permission for the parent post.

Props GhostToast.
Fixes #39881.

Merges [40306] to the 4.7 branch.

Note: See TracTickets for help on using tickets.