Skip to:
Content

BuddyPress.org

Opened 6 years ago

Closed 6 years ago

#5180 closed defect (bug) (fixed)

groups_update_groupmeta() mangles line breaks

Reported by: rogercoathup Owned by: boonebgorges
Milestone: 1.9 Priority: high
Severity: normal Version: 1.8.1
Component: Core Keywords: has-patch
Cc:

Description

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.

Attachments (1)

5180.patch (1.3 KB) - added by boonebgorges 6 years ago.

Download all attachments as: .zip

Change History (5)

#1 @rogercoathup
6 years ago

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

Last edited 6 years ago by rogercoathup (previous) (diff)

#2 @imath
6 years ago

Hi,
Just a side note : wp_unslash appeared in 3.6

#3 @boonebgorges
6 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 1.9
  • Priority changed from normal to high

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
6 years ago

#4 @boonebgorges
6 years ago

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

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

Note: See TracTickets for help on using tickets.