Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#40891 closed defect (bug) (fixed)

map_meta_cap() causing PHP notice on term meta permissions

Reported by: caercam Owned by: boonebgorges
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.7
Component: Role/Capability Keywords: has-patch needs-testing
Focuses: Cc:


Mentioned in #40889: when checking for permission to add/edit/delete term meta, map_meta_cap() only performs a simple boolean test to make sure the term exists: https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/capabilities.php#L277

If get_term() returns a WP_Error the test fails and $term->taxonomy causes an "Undefined property" notice. There should be an additional check using is_wp_error(), a simple isset( $term->taxonomy ) or maybe a simpler term_exists() to make sure the term actually exists.

Attachments (3)

40891.diff (368 bytes) - added by caercam 4 years ago.
40891.2.diff (1.3 KB) - added by caercam 4 years ago.
40891.3.diff (1.2 KB) - added by boonebgorges 4 years ago.

Download all attachments as: .zip

Change History (9)

4 years ago

#1 @caercam
4 years ago

  • Keywords has-patch added

#2 follow-up: @boonebgorges
4 years ago

  • Keywords needs-refresh needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.9

Thanks for the ticket and for the patch, @caercam !

isset( $term->taxonomy ) feels like a roundabout way of checking whether we actually have a term. Maybe better to check this more directly: $term instanceof WP_Term. What do you think?

Are you able to write a unit test that demonstrates the error, so we can avoid future regressions?

4 years ago

#3 in reply to: ↑ 2 @caercam
4 years ago

Replying to boonebgorges:
$term instanceof WP_Term makes sense! Updated patch + added unit test. First ever released unit test, please bare with me and point any mistake I may have made.

4 years ago

#4 @boonebgorges
4 years ago

  • Keywords needs-testing added; needs-refresh needs-unit-tests removed

Thank you, @caercam! The test is a good start, but (a) as written, it fails after the patch is applied (because the expected notice is no longer thrown), and (b) it doesn't actually test anything. I've written a more targeted test that checks for the actual return value of user_can() when passing a bad term ID, which fails with an error before the patch. Can you double check to see that the logic is correct? 40891.3.diff

#5 @caercam
4 years ago

Logic confirmed. Thanks for putting up with me!

#6 @boonebgorges
4 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 40999:

Avoid PHP notices when checking termmeta capabilities against a non-existent term.

Previously, checks like current_user_can( 'edit_term_meta', $term_id )
returned the proper value, but generated a PHP notice due to the fact
that get_term( $term_id ) could, in certain instances, return
WP_Error objects.

Props caercam.
Fixes #40891.

Note: See TracTickets for help on using tickets.