groups_update_groupmeta() mangles line breaks

[rogercoathup]

In wp3.6.1:

Attempting to store text with line breaks using groups_update_groupmeta() results in the line breaks being removed and replaced with 'rn'.

This is caused by the esc_sql call in lines 1047-1048 of bp-groups-functions.php:

if ( is_string( $meta_value ) )

$meta_value = stripslashes( esc_sql( $meta_value ) );

Which is removing slashes before the r and n, and hence destroying the line breaks.

To Fix:

groups_update_groupmeta should be reworked to use the same stripping function as WordPress's update_metadata -

Replace 1047-1048 with:

$meta_value = wp_unslash($meta_value);
$meta_value = sanitize_meta( $meta_key, $meta_value, 'group' );

Note: sanitize_meta just applies any filters that have been defined on the type (group) and the specific meta_key.

[rogercoathup]

further info - the esc_sql function was modified (July) in WordPress to do a 'real escape' as opposed to a 'weak escape', which may explain why groups_update_groupmeta() worked previously with line breaks, but doesn't with WP3.6.1

change set 24718: ​http://core.trac.wordpress.org/changeset/24718

[imath]

Hi,
Just a side note : ​wp_unslash appeared in 3.6

[boonebgorges]

Thanks for the ticket. A few thoughts:

• stripslashes() vs wp_unslash() is beside the point here. That's not affecting \r and \n.
• The sanitize_meta() suggestion is reasonable as a feature request, but I don't think it does what you intend it to do here. The only thing sanitize_meta() does is provide a wrapper for a more specific filter; by default, there is nothing hooked to it. In other words, despite its name, it doesn't actually sanitize anything. So I suggest we leave it out of this ticket.
• We don't need to be escaping $meta_value here. We use $wpdb->prepare() to assemble the query, which eventually calls WP's core sanitization functions. esc_sql() or whatever are only necessary when we are assembling SQL queries that do not get passed through $wpdb->prepare() (such as concatenated LIKE queries).

Patch attached. I'd like a sanity check on this, before I commit (and apply the same change to similar functions in other components).

[boonebgorges]

In 7469:

Sanitize more gently in component *_update_meta() functions

Previous sanitization techniques resulted in double-sanitization. Recent
changes in how WP's SQL sanitization routines work have surfaced this problem,
in particular as regards line breaks. By removing the extraneous call to
esc_sql(), we ensure that line breaks are preserved, and sanitization is left
to $wpdb->prepare().

Change applied in update_meta() functions through bp-groups, bp-activity, and
bp-xprofile. Also adds corresponding unit tests.

Fixes #5180