Skip to:

Opened 14 years ago

Closed 14 years ago

#2310 closed defect (bug) (fixed)

There is a serious bug in buddypress 1.2.3 signup validation for username

Reported by: faisalzulfiqar's profile faisalzulfiqar Owned by:
Milestone: 1.2.5 Priority: critical
Severity: Version:
Component: Core Keywords: has-patch needs-testing
Cc: aesqe


Hi I work at Mindblaze Technologies,

I was deploying a site for one of our clients and I found this.

In bp-core/bp-core-signup.php

This code has serious problem:

$db_illegal_names = get_site_option( 'illegal_names' );
$filtered_illegal_names = apply_filters( 'bp_core_illegal_usernames', array( 'www', 'web', 'root', 'admin', 'main', 'invite', 'administrator', BP_GROUPS_SLUG, BP_MEMBERS_SLUG, BP_FORUMS_SLUG, BP_BLOGS_SLUG, BP_REGISTER_SLUG, BP_ACTIVATION_SLUG ) );

$illegal_names = array_merge( (array)$db_illegal_names, (array)$filtered_illegal_names );

in it "array_merge" function is embedding "array( 'www', 'web', 'root', 'admin', 'main', 'invite', 'administrator', BP_GROUPS_SLUG, BP_MEMBERS_SLUG, BP_FORUMS_SLUG, BP_BLOGS_SLUG, BP_REGISTER_SLUG, BP_ACTIVATION_SLUG )" at the end of "$db_illegal_names" so every time validation function is called it gets appended and the size of this field starts to increase until the point that it breaks the update query which becomes huge after a hundred sign ups or so.

the last line should be like this:
$common_names = array_intersect( (array)$db_illegal_names, (array)$filtered_illegal_names );
$diff_names = array_diff( (array)$db_illegal_names, (array)$filtered_illegal_names );
$illegal_names = array_merge( (array)$diff_names, (array)$common_names );

"array_merge" function merges arrays with numeric keys that is why here we can not use "array_merge"

Attachments (1)

2310.001.diff (1.3 KB) - added by cnorris23 14 years ago.

Download all attachments as: .zip

Change History (11)

#1 @johnjamesjacoby
14 years ago

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

(In [2928]) Fixes #2310 props faisalzulfiqar

#2 @aesqe
14 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

shouldn't that be:

$common_names = array_intersect( (array)$filtered_illegal_names, (array)$db_illegal_names );
$diff_names = array_diff( (array)$filtered_illegal_names, (array)$db_illegal_names );

$filtered_illegal_names should be first argument.

#3 @aesqe
14 years ago

  • Cc aesqe added

#4 @johnjamesjacoby
14 years ago

  • Milestone changed from 1.2.4 to 1.2.5

Seems to be working fine but will review for 1.2.5.

#5 @cnorris23
14 years ago


The two lines you reopened the ticket over are correct in the code. The $common_names and $diff_names variables are being constructed to update the database, and as such, the $db_illegal_names variable should be the first parameter of both array_intersect and array_diff.

#6 @aesqe
14 years ago

actually, it seems that this whole system is wrong:

$one = array(1,2,3,4,7);
$two = array(2,4,5,8,9);

$intersect = array_intersect($one, $two);
$diff = array_diff($one, $two);

$result = array_merge($diff, $intersect); Array ( [0] => 1 [1] => 3 [2] => 7 [3] => 2 [4] => 4 )

in the end, you get the first array again, and the filter does not work.

i thought the solution was to switch array positions because then, the filter worked.

i might be just mad tired, tho, so i'll review this again in the morning, but i think i'm right.

#7 @cnorris23
14 years ago

You're right. I was looking strictly at syntax. Going through and actually testing the code shows that things aren't working correctly. I played around a little, but it looks like the best solution is to go back to the code pre-[2928], and then run $illegal_names through array_unique(). However, I feel like I'm missing something, because this seems like an easier, and more obvious solution. Regardless, I've added a patch to add array_unique(). Can you test this?

14 years ago

#8 @cnorris23
14 years ago

  • Keywords has-patch needs-testing added; register mysql error illegal_names removed

#9 @aesqe
14 years ago

that patch works fine, but can i suggest something else?

// names already in db
$db_illegal_names = get_site_option('illegal_names');

// core values that mustn't be allowed to be registered as usernames:
$core_illegal_names = array( 'www', 'web', 'root', 'admin', 'main', 'invite', 'administrator', BP_GROUPS_SLUG, BP_MEMBERS_SLUG, BP_FORUMS_SLUG, BP_BLOGS_SLUG, BP_REGISTER_SLUG, BP_ACTIVATION_SLUG );

// so why supply users with $core_illegal_usernames via filter?
// aren't those names already saved to db when buddypress is activated?
// if they need to know those values, they can do get_site_option('illegal_names'), right?
// i think that this filter should be used just for adding names, se let's just give users an empty array:
$filtered_illegal_names = apply_filters("bp_core_illegal_usernames", array());

//and then merge all three:
$illegal_names = array_merge( (array)$db_illegal_names, (array)$filtered_illegal_names, (array)$core_illegal_names );
$illegal_names = array_unique( $illegal_names );

i hope i'm not missing something :)

#10 @johnjamesjacoby
14 years ago

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

(In [3037]) Fixes #2310

Note: See TracTickets for help on using tickets.