Make WordPress Core

Opened 10 years ago

Last modified 5 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 10 years ago.
returnMetaArray.php (502 bytes) - added by alexwbaumann 10 years ago.
Attempted unit test.
returnMetaArray.2.php (520 bytes) - added by alexwbaumann 10 years ago.
This is the correct one.
34405.diff (1.3 KB) - added by realloc 10 years ago.
Test moved to post/meta.php

Download all attachments as: .zip

Change History (10)

#1 @SergeyBiryukov
10 years ago

  • Keywords has-patch needs-unit-tests added

#2 @boonebgorges
10 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
10 years ago

Attempted unit test.

#3 @alexwbaumann
10 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 10 years ago by alexwbaumann (previous) (diff)

@alexwbaumann
10 years ago

This is the correct one.

#4 @johnbillion
10 years ago

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

@realloc
10 years ago

Test moved to post/meta.php

#5 @johnbillion
10 years ago

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

#6 @johnbillion
5 years ago

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