Skip to:
Content

BuddyPress.org

Opened 13 years ago

Closed 12 years ago

Last modified 12 years ago

#1284 closed defect (bug) (fixed)

BP_Groups_Group::get_all method generates bad mysql request

Reported by: fairweb's profile Fairweb Owned by: mrmaz's profile MrMaz
Milestone: 1.1.3 Priority: major
Severity: Version:
Component: Keywords: has-patch group mysql status public request
Cc: Fairweb, MrMaz

Description

When using this method in the following context :

  • current user is not an admin
  • $public argument set to false

the mysql request string would be :
SELECT * FROM wp_bp_groups g AND g.status != 'hidden'
instead of
SELECT * FROM wp_bp_groups g WHERE g.status != 'hidden'

fix bp-groups-classes.php line 428
$hidden_sql = $only_public ? $wpdb->prepare( " AND g.status != 'hidden'") : $wpdb->prepare( "WHERE g.status != 'hidden'");

Attachments (1)

bp-groups-classes.php.patch (3.3 KB) - added by MrMaz 13 years ago.

Download all attachments as: .zip

Change History (18)

#1 @Fairweb
13 years ago

  • Cc Fairweb added

#2 @MrMaz
13 years ago

  • Cc MrMaz added
  • Keywords group mysql status public request added
  • Milestone set to 1.1.3
  • Owner set to MrMaz
  • Status changed from new to accepted

#3 follow-up: @MrMaz
13 years ago

  • Keywords has_patch added
  • Resolution set to fixed
  • Status changed from accepted to closed

Please test the patch and let me know if it resolves the issue, or tell me how I can test it myself.

#4 @MrMaz
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#5 @MrMaz
13 years ago

  • Status changed from reopened to accepted

#6 @MrMaz
13 years ago

  • Keywords has-patch added; has_patch removed

#7 in reply to: ↑ 3 @Fairweb
13 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

Replying to MrMaz:

Please test the patch and let me know if it resolves the issue, or tell me how I can test it myself.

I have tested, your patch seems to work fine. It's quite hard to tell you how to test as I'm using this method in a home made plugin. My function's result was only working when I was a site admin and that's where I found out the method had a problem.

Seems ok now thanks a lot.

#8 @MrMaz
13 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening because patch has not been applied to source yet.

#9 @johnjamesjacoby
13 years ago

How does this patch react when the sort_by is set? In that case it will have multiple WHERE clauses... Haven't tested it yet, just thinking out loud...

#10 @MrMaz
13 years ago

All where conditions and put into an array and then concatenated using join() so the "WHERE" only appears once.

#11 @apeatling
12 years ago

Thanks, I will test and fix this for 1.1.3.

#12 @apeatling
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [2103]) Fixes #1284 props MrMaz

#13 @apeatling
12 years ago

I'm not sure we need to overcomplicate things. Adding a WHERE 1=1 and then all the AND additions will work just fine in this case.

#14 @MrMaz
12 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This "where 1=1" fix is a bad idea.

  1. Its a hack.
  2. Its the same thing as saying "where true" which could possibly throw off the query optimizer.
  3. 1=1 and other same int/string comparison expressions, like 'a'='a' etc, will show up in any decent security scanner that is sniffing for SQL injection attacks.

It doesn't sit right when I spend a good chunk of my time creating a solution that works properly, and its replaced with a hack that looks like SQL injection.

#15 @apeatling
12 years ago

All good points, I'll change it. :)

#16 @apeatling
12 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [2119]) Fixes #1284 props MrMaz.

#17 @johnjamesjacoby
12 years ago

Andy is an SQL injecting robot sent from the future to remind us that 1 really does equal 1.

Note: See TracTickets for help on using tickets.