BP_Groups_Group::get_all method generates bad mysql request

[Fairweb]

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'");

[MrMaz]

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

[Fairweb]

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.

[MrMaz]

Reopening because patch has not been applied to source yet.

[johnjamesjacoby]

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...

[MrMaz]

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

[apeatling]

Thanks, I will test and fix this for 1.1.3.

[apeatling]

(In [2103]) Fixes #1284 props MrMaz

[apeatling]

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.

[MrMaz]

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.

[apeatling]

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

[apeatling]

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

[johnjamesjacoby]

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