Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#56962 closed defect (bug) (wontfix)

current_user_can( 'read_post' ) not working.

Reported by: jcorbin's profile jcorbin Owned by:
Milestone: Priority: normal
Severity: major Version: 6.1
Component: Role/Capability Keywords: close
Focuses: Cc:

Description (last modified by SergeyBiryukov)

current_user_can( 'read_post' ) is not working in our case.

current 6.0.3 sites (https://upstatetoday.com for instance) feature the following in out single.php:

        if (
	       
	         current_user_can( 'read_post' ) 
	        
... Allow access...

This exists so that logged in WP users in WP backend can see the actual content, no our paywall)

In WP 6.1 current_user_can( 'read_post' ) does not appear to work; logged in WP admins, etc are given the paywall.

rolling back to 6.0.3 fixes this.

Attachments (1)

single.php.txt (5.4 KB) - added by jcorbin 2 years ago.
full single.php file that does not function in 6.1

Download all attachments as: .zip

Change History (12)

@jcorbin
2 years ago

full single.php file that does not function in 6.1

#1 follow-up: @TimothyBlynJacobs
2 years ago

I'm not sure yet what may have caused this regression. However, I did want to note that that is improper use of the read_post capability. You must pass the post ID that you want to check the user has permission for as the second parameter to the function call.

This would be the correct usage:

current_user_can( 'read_post', $post->ID )

#2 @TimothyBlynJacobs
2 years ago

  • Component changed from Users to Role/Capability
  • Keywords close added

This was due to #44591. The reason this worked previously is that Core was previously passing on null to get_post. But we now check if a post ID was provided to avoid a PHP error.

Instead of forcing a do_not_allow, the post caps could fallback to the global post to restore this behavior. Flagging this for @SergeyBiryukov to provide his opinion as well. However, given that this code would have been generating PHP notices for years, I personally don't think we should introduce more magic to this function.

#3 in reply to: ↑ 1 @SergeyBiryukov
2 years ago

  • Description modified (diff)

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Replying to TimothyBlynJacobs:

However, I did want to note that that is improper use of the read_post capability. You must pass the post ID that you want to check the user has permission for as the second parameter to the function call.

This would be the correct usage:

current_user_can( 'read_post', $post->ID )

Yes, that is correct.

As noted in comment:10:ticket:44591, these capabilities check for a particular post and do require a post ID:

  • current_user_can( 'delete_post', $post_id )
  • current_user_can( 'edit_post', $post_id )
  • current_user_can( 'read_post', $post_id )
  • current_user_can( 'publish_post', $post_id )

[53408] / #44591 aimed to address this in a consistent way. Performing these checks without passing in a post ID is not supported and could only work by accident.

If you need a more general check, I would suggest using one of the capabilities that don't require a post ID, e.g. read, or just the is_user_logged_in() function, based on your use case.

Last edited 2 years ago by SergeyBiryukov (previous) (diff)

#4 @peterwilsoncc
2 years ago

[53408] / #44591 aimed to address this in a consistent way. Performing these checks without passing in a post ID is not supported and could only work by accident.

I'm inclined to close this and the related ticket, #57120, without a fix.

In the past, the current_user_can() checks for the post meta capabilities would default to the global post object if a post ID was not passed. While this could be appropriate in some circumstances, in other circumstances it could incorrectly give a user permission to see data they are not expected to have access to.

Without a post ID been passed to the permission check, it's not possible to guess the developer's intent so defaulting to disallow access seems the safest option.

#5 @TimothyBlynJacobs
2 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Agreed. Mixing magic and security APIs is worth avoiding when possible I think.

#6 @SergeyBiryukov
2 years ago

#57120 was marked as a duplicate.

#7 @pikamander2
2 years ago

We ran into the same issue with save_post, where it suddenly began return false instead of true, even for administrators.

Changing it to save_posts worked, though it was frustrating to debug because there was no warning or error being thrown.

Ideally, I think that a breaking change in behavior like this should be accompanied by a deprecation warning.

#8 @johnbillion
2 years ago

This isn't a breaking change or a deprecation, this is a fix for a bug that meant these capability checks only worked by accident in the past. Your PHP error log is probably full of PHP notices (or warnings in PHP 8) caused by this incorrect usage, see the discussion on #44591 for more details.

#9 follow-up: @pikamander2
2 years ago

@johnbillion - It's breaking behavior even if the previous behavior wasn't intended. The incorrect syntax previously returned true and now it returns false. That's a major change given that it's often used as a permission check.

I'm fine with the behavior being changed, but I don't see anything in the error log that clearly indicates where the source of the problem is, and that's problematic for people whose sites are now breaking in unpredictable ways.

Here are some links to other people experiencing the same issue, in addition to myself and the bug filer:

https://stackoverflow.com/questions/74363261

https://stackoverflow.com/questions/74443198

Ideally, if one of those four capabilities (save_post, publish_post, edit_post, delete_post) is passed to current_user_can without the additional argument(s), it would be nice for it to write a clear message to the error log explaining what the problem is.

Last edited 2 years ago by pikamander2 (previous) (diff)

#10 in reply to: ↑ 9 ; follow-up: @SergeyBiryukov
2 years ago

Replying to pikamander2:

Ideally, if one of those four capabilities (save_post, publish_post, edit_post, delete_post) is passed to current_user_can, it would be nice for it to write a clear message to the error log explaining what the problem is.

I might be missing something, but [53408] / #44591 does include a _doing_it_wrong() message that should be added to the error log if WP_DEBUG is enabled:

Notice: Function map_meta_cap was called incorrectly. When checking for the edit_post capability, you must always check it against a specific post. Please see Debugging in WordPress for more information. (This message was added in version 6.1.0.)

#11 in reply to: ↑ 10 @pikamander2
2 years ago

Replying to SergeyBiryukov:

I might be missing something, but [53408] / #44591 does include a _doing_it_wrong() message that should be added to the error log if WP_DEBUG is enabled:

Notice: Function map_meta_cap was called incorrectly. When checking for the edit_post capability, you must always check it against a specific post. Please see Debugging in WordPress for more information. (This message was added in version 6.1.0.)

Most sites have WP_DEBUG disabled by default, including ours.

It's good to know that there was a debug message there, though out of curiosity, why is that message hidden behind WP_DEBUG? I know that I've previously seen WordPress point out argument/syntax issues without having WP_DEBUG enabled.

Given that it's a breaking change (true -> false) and a clear cut issue (i.e. there's never any valid reason to call one of those four caps without a second parameter), would it be possible to escalate it to a normal warning/error instead of a debug message?

I'll admit that I may not understand the full situation there, but it seems like a major enough and common enough issue to be more visible than it is now.

Last edited 2 years ago by pikamander2 (previous) (diff)
Note: See TracTickets for help on using tickets.