Make WordPress Core

Opened 9 years ago

Last modified 4 years ago

#34405 reviewing defect (bug)

Retrieval of meta value that is an array.

Reported by: alexwbaumann's profile alexwbaumann Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3.1
Component: Options, Meta APIs Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

Simply using $post->meta_value automatically runs get_post_meta(). This happens in https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/post.php#L748 (post.php line 748).

That value then goes into sanitize_post_field() https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/post.php#L751 (post.php line 751). The value then gets modified at https://core.trac.wordpress.org/browser/tags/4.3.1/src/wp-includes/post.php#L2215 (post.php line 2251) through esc_attr().

Codex entry for https://codex.wordpress.org/Function_Reference/esc_attr (esc_attr()) shows that the only parameter that should be sent is a string.

This process works for strings. However, if our meta value is an array, we get a PHP notice (Notice: Array to string conversion in /var/www/html/wp/wp-includes/formatting.php on line 959). Then returns a sting(5) 'Array' instead of the actual array.

I think that the best option may be to edit esc_attr() or the subsequent function wp_check_invalid_utf8() to allow for arrays to be passed to them. Alternatively the quick dirty fix would be to not send an array into sanitize_post_field.

Attachments (4)

34405.patch (531 bytes) - added by SergeyBiryukov 9 years ago.
returnMetaArray.php (502 bytes) - added by alexwbaumann 9 years ago.
Attempted unit test.
returnMetaArray.2.php (520 bytes) - added by alexwbaumann 9 years ago.
This is the correct one.
34405.diff (1.3 KB) - added by realloc 9 years ago.
Test moved to post/meta.php

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
9 years ago

  • Keywords has-patch needs-unit-tests added

#2 @boonebgorges
9 years ago

Thanks for the detailed ticket, alexwbaumann, and welcome to Trac.

I've always thought that $post->foo to get arbitrary 'foo' metadata from the database is a pretty weird syntax, but that's the way it works, so oh well :)

Sanitizing a non-scalar value for the 'edit' context feels odd to me. WP uses the 'edit' context when pulling up a post to display on wp-admin/post.php, in which case the values are being sanitized for entry into textareas and input fields. But this is clearly not the case for objects and arrays, so esc_attr() doesn't seem like the right kind of sanitization. In the case of 'edit', it's not clear to me that we should be doing any sanitization at all - who knows how this data is intended to be used?

That said, I don't think esc_attr() will be harmful here - anyone using post data in this way ought to know what they're doing. Since postmeta can be in any non-scalar format, including objects and multi-dimensional arrays, I recommend a more general solution:

if ( is_scalar( $value ) ) {
    $value = esc_attr( $value );
} else {
    $value = map_deep( $value, 'esc_attr' );
}

As SergeyBiryukov notes, this needs unit tests.

@alexwbaumann
9 years ago

Attempted unit test.

#3 @alexwbaumann
9 years ago

This is my first time attempting to do a unit test. I was having trouble with the original. After submitting, I soon found why it was not working for me as expected.

I think that the new file is what you are looking for. If not, let me know if you need anything else and I'll do my best.

Last edited 9 years ago by alexwbaumann (previous) (diff)

@alexwbaumann
9 years ago

This is the correct one.

#4 @johnbillion
9 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

@realloc
9 years ago

Test moved to post/meta.php

#5 @johnbillion
9 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Owner set to johnbillion
  • Status changed from new to reviewing

#6 @johnbillion
4 years ago

  • Owner johnbillion deleted
Note: See TracTickets for help on using tickets.