Make WordPress Core

Opened 14 years ago

Closed 9 years ago

Last modified 9 years ago

#13905 closed defect (bug) (fixed)

No sanity check in map_meta_cap caps throws PHP notices

Reported by: filosofo's profile filosofo Owned by: filosofo's profile filosofo
Milestone: 4.4 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 (5)

check-object-existence-in-caps-logic.13905.diff (9.5 KB) - added by filosofo 14 years ago.
check-object-existence-in-caps-logic.13905.2.diff (9.4 KB) - added by filosofo 14 years ago.
13905.3.diff (1.6 KB) - added by nerrad 10 years ago.
add sanity checks for result from get_post()
13905.4.diff (2.9 KB) - added by ocean90 9 years ago.
13905.diff (2.8 KB) - added by wonderboymusic 9 years ago.

Download all attachments as: .zip

Change History (31)

#1 @filosofo
14 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.

#2 @filosofo
14 years ago

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

#3 @kevinB
14 years ago

  • Cc kevinB added

#4 @nacin
14 years ago

  • Milestone changed from 3.0.1 to 3.1

#5 @nacin
14 years ago

  • Keywords needs-refresh added; has-patch removed

Patch needs a refresh based on #14122.

#6 @filosofo
14 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.

#7 follow-up: @nacin
14 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?

#8 in reply to: ↑ 7 @filosofo
14 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.

#9 @westi
14 years ago

  • Keywords needs-patch added; has-patch removed

Changing to needs-patch for the refresh discussed above.

#10 @nacin
14 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?

#11 @ryan
14 years ago

  • Milestone changed from 3.1 to Future Release

#12 follow-up: @filosofo
14 years ago

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

#13 in reply to: ↑ 12 @westi
14 years ago

Replying to filosofo:

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

#14 @filosofo
14 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.

#15 @nacin
14 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.

#16 @nacin
14 years ago

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

#17 @nacin
12 years ago

#22698 was marked as a duplicate.

#18 @nacin
12 years ago

#17912 was marked as a duplicate.

#19 @c3mdigital
11 years ago

#21031 was marked as a duplicate.

#20 @nacin
11 years ago

#23377 was marked as a duplicate.

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


10 years ago

@nerrad
10 years ago

add sanity checks for result from get_post()

#22 @SergeyBiryukov
10 years ago

#30821 was marked as a duplicate.

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


9 years ago

@ocean90
9 years ago

@wonderboymusic
9 years ago

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


9 years ago

#25 @wonderboymusic
9 years ago

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

Tagged wrong ticket in [34113]

#26 @wonderboymusic
9 years ago

  • Milestone changed from Future Release to 4.4
Note: See TracTickets for help on using tickets.