WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 7 months ago

#13905 reopened defect (bug)

No sanity check in map_meta_cap caps throws PHP notices

Reported by: filosofo Owned by: filosofo
Milestone: Future Release Priority: normal
Severity: normal Version: 3.0
Component: Posts, Post Types Keywords: needs-patch
Focuses: Cc:

Description

map_meta_cap assumes that the ID which it's passed when evaluating edit_post, edit_page and the like actually belongs to a real post object.

In fact, it's quite possible that the object doesn't yet exist (creating a new object, perhaps) or doesn't exist any more (deleted), or that it has otherwise received a syntactically correct ID value that doesn't map to an existing post object (0, e.g.).

Instead, map_meta_cap should check that the post object actually exists before attempting to branch on its properties.

Patch also removes some apparent debugging comments.

Attachments (2)

check-object-existence-in-caps-logic.13905.diff (9.5 KB) - added by filosofo 4 years ago.
check-object-existence-in-caps-logic.13905.2.diff (9.4 KB) - added by filosofo 3 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 filosofo4 years ago

It's a little hard to see in the diff, but all I'm doing is wrapping the object-property-dependent logic in a if ( ! empty( $post->ID ) ) check. And removing that commented-out debugging cruft.

comment:2 filosofo4 years ago

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

comment:3 kevinB4 years ago

  • Cc kevinB added

comment:4 nacin4 years ago

  • Milestone changed from 3.0.1 to 3.1

comment:5 nacin3 years ago

  • Keywords needs-refresh added; has-patch removed

Patch needs a refresh based on #14122.

comment:6 filosofo3 years ago

  • Keywords has-patch added; needs-refresh removed

Patch refreshed. I wish I could remember the real-life scenario that revealed this issue to me, but it's been too long.

comment:7 follow-up: nacin3 years ago

Patch needs refresh again after more work in #14122.

I'm not sure some sanity checks are needed here, such as whether a post type object exists after we've already established that the post exists. They seem fine, though -- but can't we just break instead of re-indenting everything for most checks?

comment:8 in reply to: ↑ 7 filosofo3 years ago

Replying to nacin:

I'm not sure some sanity checks are needed here, such as whether a post type object exists after we've already established that the post exists.

Have we? Maybe that's in the recent changes that I need to refresh against, but as of two weeks ago we didn't check that the post exists.

They seem fine, though -- but can't we just break instead of re-indenting everything for most checks?

Multiple breaks for one case seems like asking for trouble, in that it makes the logic harder to follow, more likely to get overlooked in future patches, etc.

I'll refresh the patch.

comment:9 westi3 years ago

  • Keywords needs-patch added; has-patch removed

Changing to needs-patch for the refresh discussed above.

comment:10 nacin3 years ago

If the post doesn't exist, this patch will return no caps for current_user_can( 'edit_post', $nonexistent_post )..

Should current_user_can( 'edit_post', $nonexistent_post ) still go with the baseline primitive cap, e.g. edit_posts?

comment:11 ryan3 years ago

  • Milestone changed from 3.1 to Future Release

comment:12 follow-up: filosofo3 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from accepted to closed

comment:13 in reply to: ↑ 12 westi3 years ago

Replying to filosofo:

Why did you close this - surely we could address this in future?

comment:14 filosofo3 years ago

My bad, I guess. I interpreted nacin's comment in #wordpress-dev about it as saying it's too trivial to worry about.

comment:15 nacin3 years ago

My bad if that's what you interpreted.

I don't think it's trivial, rather, it only happens when a nonexistent post/something is requested. While it should never occur, it's severe when it does.

I also noticed yesterday that I'm not sure if a nonexistent post should mean no caps get mapped. See comment 10. Hence punt made sense with a patch needing refresh.

comment:16 nacin3 years ago

  • Milestone set to Future Release
  • Resolution wontfix deleted
  • Status changed from closed to reopened

comment:17 nacin17 months ago

#22698 was marked as a duplicate.

comment:18 nacin12 months ago

#17912 was marked as a duplicate.

comment:19 c3mdigital8 months ago

#21031 was marked as a duplicate.

comment:20 nacin7 months ago

#23377 was marked as a duplicate.

Note: See TracTickets for help on using tickets.