Make WordPress Core

Opened 7 years ago

Closed 6 years ago

#45206 closed defect (bug) (fixed)

Logic error in do_meta_boxes() for Gutenberg support

Reported by: johnjamesjacoby's profile johnjamesjacoby Owned by: pento's profile pento
Milestone: 5.0 Priority: normal
Severity: normal Version: 5.0
Component: Options, Meta APIs Keywords: has-patch fixed-5.0
Focuses: Cc:

Description (last modified by johnjamesjacoby)

When testing 5.0 beta 1 on an existing error-free installation, I started seeing fatal errors in the logs of a multisite network I maintain.

To quickly duplicate, install and activate the WP User Profiles plugin (which uses meta-boxes for user profile pages) and visit your profile inside of WordPress admin.

PHP message: PHP Fatal error:  Uncaught Error: Cannot use object of type WP_User as array in /public/wordpress/wp-admin/includes/template.php:1066

Specifically:

thrown in /public/wordpress/wp-admin/includes/template.php on line 1066" while reading upstream, client: 184.58.156.205, server: , request: "GET /wp-admin/users.php?page=profile HTTP/1.0", upstream: "fastcgi://unix:/var/run/php/php7.2-fpm.sock:", host: "jjj.blog", referrer: "https://jjj.blog/wp-admin/users.php?page=account"

In short, it is incorrect to assume that meta-boxes are always for posts, and that the args key is always present, and is an array. (The is_array( $box['args'] ) check from 2016, r37972, supports this notion.)

Related to #45112, and r43779.

Patch with a recommended fix imminent.

Attachments (1)

45206.patch (2.2 KB) - added by johnjamesjacoby 7 years ago.

Download all attachments as: .zip

Change History (17)

#1 @johnjamesjacoby
7 years ago

45206.patch addresses the regression by wrapping the new Gutenberg-support code in a...

if ( is_array( $box[ 'args' ] ) ) { ... }

...condition. I think this is correct, as it only addresses the new code causing new fatal errors. Another potential fix might be to the add_meta_box() function, to change the $callback_args default value from null to array(), but I chose not to touch an unrelated function at this time to avoid introducing other unexpected errors elsewhere.

Outside of this specific plugin incompatibility, I believe this patch is a nice-to-have bit of hardening anyways. Since $box['args'] is not guaranteed to be an array, it makes sense to continue to check it in new code the same as do with old, at least until such a time where meta-box code can be scrutinized more deeply.

#2 @SergeyBiryukov
7 years ago

  • Component changed from General to Options, Meta APIs

#3 @pento
7 years ago

  • Milestone changed from Awaiting Review to 5.0

#4 @birgire
7 years ago

In 45206.patch there seems to be a typo:

$block_compatible |= (bool) $box['args']['__back_compat_meta_box'];

This will not be triggered by the plugin though.

I assume it was meant to be:

$block_compatible != (bool) $box['args']['__back_compat_meta_box'];

(The is_array( $box['args'] ) check from 2016, r37913, supports this notion.)

This changeset reference seems to be ajax related and unrelated to the ticket?

Related to #45112, and r43608.

Also this changeset reference seems to be cron related and seems unrelated?

#5 @johnjamesjacoby
7 years ago

@birgire

  1. Not a typo in the patch. That's the original code and is a bitwise comparison in PHP.
  2. I forget that Trac auto links to core and not develop. Sorry about that. Will fix.
Last edited 7 years ago by johnjamesjacoby (previous) (diff)

#6 @johnjamesjacoby
7 years ago

  • Description modified (diff)

#7 @johnjamesjacoby
7 years ago

  • Description modified (diff)

#8 @johnjamesjacoby
7 years ago

  • Description modified (diff)

#9 @birgire
7 years ago

@johnjamesjacoby thanks updating the changeset references and thanks for the clarification, I totally missed that this comparison came from original code :-)

Last edited 7 years ago by birgire (previous) (diff)

#10 follow-up: @johnjamesjacoby
7 years ago

@birgire let’s assume the bitwise operator is correct, but it might be a good idea to document it in the code as such. They aren’t popular, and I have a hunch you won’t be the last person to question it.

#11 in reply to: ↑ 10 @birgire
6 years ago

Replying to johnjamesjacoby:

@birgire let’s assume the bitwise operator is correct, but it might be a good idea to document it in the code as such. They aren’t popular, and I have a hunch you won’t be the last person to question it.

Sounds like a good idea. Even though I've occasionally worked with bitwise operators, it's application in this context here didn't turn on the light bulb :-)

I also wonder if the add_meta_box() documentation should be adjusted, regarding the support for the $callback_args input type.

Currently it's documented as @param array:

https://core.trac.wordpress.org/browser/tags/4.9.8/src/wp-admin/includes/template.php#L923

but as you noted, your WP User Profiles plugin is using an WP_User object:

https://plugins.trac.wordpress.org/browser/wp-user-profiles/trunk/wp-user-profiles/includes/metaboxes.php#L51

// Register the "Status" side meta box
add_meta_box(
    'submitdiv',
    _x( 'Status', 'users user-admin edit screen', 'wp-user-profiles' ),
    'wp_user_profiles_status_metabox',
    $hook,
    'side',
    'high',
    $user
);

#12 @johnjamesjacoby
6 years ago

I’ve tweaked my WP User Profiles plugin to be an array in the next release. It’s also odd how it not being an array manifests itself once meta boxes are being output to the page.

Areas in WordPress core slyly avoid the error by having meta boxes without titles, or otherwise passing null in areas to trigger their own unique edge-cases.

Comments, Links, and Nav Menus all have their own little nuances, as does the Block Editor now. I don’t believe any of that logic belongs in do_meta_boxes() but that’s another ticket for another day. :)

#13 @pento
6 years ago

  • Keywords has-patch added
  • Owner set to pento
  • Status changed from new to assigned

Thanks for the bug report and the patch, @johnjamesjacoby!

I agree that we need to handle args not being an array, this patch does the job nicely.

While I'm there, I'll also make the logic that @birgire referred to a little clearer. 🙂

#14 @pento
6 years ago

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

In 43838:

Meta boxes: Don't assume that callback args are an array.

While the documentation for add_meta_box() specifices that $callback_args should be an array, this has never been enforced, and we have workarounds in place for when it's passed as something other than an array.

Rather than break sites that are passing unexpected data, we can quietly just allow for it, instead.

Props johnjamesjacoby, birgire.
Fixes #45206.

#15 @SergeyBiryukov
6 years ago

  • Keywords fixed-5.0 added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for trunk.

#16 @atimmer
6 years ago

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

In 44174:

Meta boxes: Don't assume that callback args are an array.

While the documentation for add_meta_box() specifices that $callback_args should be an array, this has never been enforced, and we have workarounds in place for when it's passed as something other than an array.

Rather than break sites that are passing unexpected data, we can quietly just allow for it, instead.

Props johnjamesjacoby, birgire, pento.
Merges [43838] to trunk.
Fixes #45206.

Note: See TracTickets for help on using tickets.