Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

#5694 closed defect (bug) (fixed)

Issue with the ampersand character in members search

Reported by: dontdream Owned by: imath
Milestone: 2.1 Priority: normal
Severity: normal Version:
Component: Members Keywords: has-patch commit
Cc:

Description

In the Members Directory page, entering in the "Search Members..." search box a string containing an ampersand returns incorrect results.

It looks like the ampersand and any following text are ignored by the search, e.g. if you search for "M&M" you get search results for "M", or if you search for "Wiley & Sons" you get search results for "Wiley ".

Attachments (4)

5694.patch (1.0 KB) - added by imath 5 years ago.
5694.02.patch (644 bytes) - added by imath 5 years ago.
5694.03.patch (1.3 KB) - added by imath 5 years ago.
5694.04.patch (2.9 KB) - added by imath 5 years ago.

Download all attachments as: .zip

Change History (17)

#1 @imath
5 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 2.1

Hello dontdream,

Thanks for your feedback. I confirm the issue. Suggesting 5694.patch to fix it.

If you can test it and tell me if it did fix the problem for you, it would be great.

@imath
5 years ago

#2 @boonebgorges
5 years ago

  • Component changed from Template Pack to Members
  • Keywords needs-unit-tests added

#3 @dontdream
5 years ago

Thank you imath, I tested the patch and it works fine.

Actually it works for me even in a simpler form, patching buddypress.js only, with

if ( search_terms.length ) {
    search_terms = search_terms.split( '&' ).join( '&' );

#4 @DJPaul
5 years ago

  • Keywords needs-patch added; has-patch removed

#5 follow-up: @DJPaul
5 years ago

Also, not sure why you've added esc_html. It doesn't seem to affect the reported issue directly. I'd use something like sanitize_text_field and commit it separately.

#6 in reply to: ↑ 5 @imath
5 years ago

  • Keywords has-patch added; needs-patch removed

Replying to DJPaul:

Also, not sure why you've added esc_html

In Activity table '&' are stored '&', so we need a function to encode & to & esc_html() does it, htmlentities2() is also doing, i've updated the patch this way in 5694.02.patch.

@imath
5 years ago

#7 @imath
5 years ago

OOps i made a confusion with another ticket, this one is about the member search issue with '&'. Sorry. Well the problem also exist in activity search ;)

Anyway, after looking at it. The member search is a bit more complex as in bp_legacy_theme_ajax_querystring() a querystring is built from the posted data. And the problem is that when a search is containing '&', it's parsed like if there were a new query var.

For instance (if javascript is enabled) searching for '&mp' will generate a query string like this :
type=alphabetical&action=alphabetical&page=1&search_terms=&mp
Then once parsed in bp_has_members(), the search terms argument is empty and there is a new 'mp' var

So i suggest 5694.03.patch to try to fix this issue.

@imath
5 years ago

#8 @DJPaul
5 years ago

  • Keywords needs-patch added; has-patch removed

Is this another case where we should change htmlentities2 for addslashes? (I assume the problem with that is that our search parameters expect data with slashes, because we incorrectly store slashed data in the DB).

#9 follow-up: @dontdream
5 years ago

No, this time the problem is that we (incorrectly?) store '&' as '&' so we need something like

search_terms = search_terms.split( '&' ).join( '&' );

see comment:3

#10 in reply to: ↑ 9 @imath
5 years ago

Replying to dontdream:

No, this time the problem is that we (incorrectly?) store '&' as '&' so we need something like

htmlentities2() is doing this conversion, but yes "&" are stored in db "&amp", reason of the use of this function.

#11 @imath
5 years ago

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

Ok i've explored it a bit more.

When a profile field is saved, the filter xprofile_sanitize_data_value_before_save enters in action.

It uses xprofile_filter_kses which is using wp_kses which is using wp_kses_normalize_entities() which is replacing & by & see in kses.php at line 1150

So i'm suggesting this new version of the patch : 5694.04.patch, where i use wp_kses_normalize_entities() directly in bp_xprofile_bp_user_query_search() and BP_User_Query. This way the search in group members will also benefit of it.

I've tested the patch with javascript enabled and not enabled, it's working.
I've built unit tests and tested it and it's also working.

@imath
5 years ago

#12 @DJPaul
5 years ago

  • Keywords commit added

Good job. Also, nice work spotting that urlencode correction.

#13 @imath
5 years ago

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

In 8928:

Make sure BP_User_Query returns correct results when search term contains the Ampersand character

  1. Search terms containing this character was problematic as the "&" is a query var delimiter used in bp_legacy_theme_ajax_querystring() to build the ajax querystring
  1. When a xProfile field is saved, the value is sanitized using the xprofile_filter_kses() filter which is converting "&" to "&" before inserting the value in database.

Urlencoding the search terms in bp_legacy_theme_ajax_querystring() and applying wp_kses_normalize_entities() to search terms in BP_User_Query & in bp_xprofile_bp_user_query_search() make sure the correct results are returned to the user.

Fixes #5694

Note: See TracTickets for help on using tickets.