Skip to content

PROD-10104 - #5033

Open
jitendrabanjara1991 wants to merge 5 commits into
releasefrom
PROD-10104
Open

PROD-10104#5033
jitendrabanjara1991 wants to merge 5 commits into
releasefrom
PROD-10104

Conversation

@jitendrabanjara1991

@jitendrabanjara1991 jitendrabanjara1991 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Jira Issue:

https://buddyboss.atlassian.net/browse/PROD-10104
https://buddyboss.atlassian.net/browse/PROD-10328

General Note

Keep all conversations related to this PR in the associated Jira issue(s). Do NOT add comment on this PR or edit this PR’s description.

Notes to Developer

  • Ensure the IDs (i.e. PROD-1) of all associated Jira issues are reference in this PR’s title
  • Ensure that you have achieved the Definition of Done before submitting for review
  • When this PR is ready for review, move the associate Jira issue(s) to “Needs Review” (or “Code Review” for Dev Tasks)

Notes to Reviewer

  • Ensure that the Definition of Done have been achieved before approving a PR
  • When this PR is approved, move the associated Jira issue(s) to “Needs QA” (or “Approved” for Dev Tasks)

…et before assigning its role

Fixes a privilege-escalation path (CWE-269) where a self-registering
visitor could assign themselves any Profile Type — and its mapped WP
role — by injecting the member-type xprofile field into the signup POST.
The membertypes field's is_valid() accepted any post ID and nothing
re-checked, at activation, whether the submitted type is actually
offered on the registration form, so a Profile Type hidden from the
registration dropdown but mapped to a privileged role (editor/admin)
could be self-assigned.

bp_assign_default_member_type_to_activate_user() now ignores a
self-submitted Profile Type that is not offered at registration
(bb_is_member_type_allowed_on_registration(): active member-type post
whose _bp_member_type_enable_profile_field is unset or '1', mirroring
the registration dropdown's own gate), falling back to the
admin-configured default type. This is the single chokepoint the three
registrant branches read, and it fires on bp_core_activated_user, so it
covers both the web form and REST /signup. Admin-driven paths
(send-invite type, default registration type) are unaffected.

BP_XProfile_Field_Type_Member_Types::is_valid() additionally restricts
the value to member-type posts (was: any post), rejecting an arbitrary
post ID crafted into the field. Context-free, so profile edits that keep
a member's currently-assigned type are unaffected.
…n the signup form

Fixes a privilege-escalation path (CWE-269): a self-registering visitor
could inject the member-type xprofile field into the signup POST and
assign themselves any Profile Type — and its mapped WP role. The
membertypes field's is_valid() accepted any post ID, and nothing checked
that the submitted type is actually offered on the registration form, so
a Profile Type hidden from the registration dropdown but mapped to a
privileged role (editor/admin) could be self-assigned at activation.

register.php now rejects, with a "Please select a valid profile type"
error, a submitted member type that is not offered at registration
(bb_is_member_type_allowed_on_registration(): active member-type post
whose _bp_member_type_enable_profile_field is unset or '1', mirroring the
registration dropdown's own gate). As a backstop,
bp_assign_default_member_type_to_activate_user() ignores a not-offered
self-submitted type at activation and falls back to the admin-configured
default; this runs on bp_core_activated_user so it also covers REST
signup. Admin-driven paths (send-invite type, default registration type)
are unaffected.

BP_XProfile_Field_Type_Member_Types::is_valid() additionally restricts
the value to member-type posts (was: any post). Context-free, so profile
edits that keep a member's currently-assigned type are unaffected.
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
Guards the self-submitted member-type value against an array-crafted
field_<id> (e.g. field_85[]=x) before absint(), avoiding a PHP 8.1+
deprecation notice in bp_core_screen_signup(). Also updates the
BP_XProfile_Field_Type_Member_Types::is_valid() docblock to note that
registration eligibility is enforced at the register.php input check
and, as a backstop, at bp_assign_default_member_type_to_activate_user().
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
@buddyboss buddyboss deleted a comment from claude Bot Aug 20, 2026
@buddyboss buddyboss deleted a comment from claude Bot Aug 21, 2026
@jitendrabanjara1991

Copy link
Copy Markdown
Contributor Author

@claude Please review PR

@claude

claude Bot commented Aug 21, 2026

Copy link
Copy Markdown

Claude finished @jitendrabanjara1991's task in 2m 53s —— View job


Claude finished reviewing this PR

Todo list:

  • Gather context (PR diff, changed files, Jira refs)
  • Understand the request (code review)
  • Review src/bp-members/bp-members-functions.php
  • Review src/bp-members/screens/register.php
  • Review src/bp-xprofile/classes/class-bp-xprofile-field-type-member-types.php
  • Post final review feedback

Summary

This PR closes a mass-assignment / privilege-escalation vector: the registration form's Profile Type field is a hidden _bp_member_type_enable_profile_field-gated dropdown, but nothing previously stopped a visitor from POSTing the ID of a Profile Type that isn't offered on the form (and whose _bp_member_type_wp_roles mapping would then get applied to the new account at activation). The fix is applied in three complementary layers, which is a solid defense-in-depth design:

  1. bb_is_member_type_allowed_on_registration() (bp-members-functions.php:5602) — new shared helper that mirrors the exact gate the dropdown itself uses in BP_XProfile_Field_Type_Member_Types::edit_field_options_html() (active member-type post + _bp_member_type_enable_profile_field unset/'1').
  2. Registration input validation (register.php:179-209) — rejects the submission up front with a friendly error (Please select a valid profile type.) and correctly blocks the signup from proceeding (the "save-details" step only runs when $bp->signup->errors is empty), including a guard against array input short-circuiting before absint().
  3. Activation-time backstop (bp-members-functions.php:4113-4118) — even if a payload reaches bp_assign_default_member_type_to_activate_user() some other way, the submitted type is re-validated and falls back to the admin-configured default. Admin-driven flows (invite-mapped type, default registration type) correctly bypass this gate since they aren't user input.
  4. BP_XProfile_Field_Type_Member_Types::is_valid() (class-bp-xprofile-field-type-member-types.php:245) — tightened from "any existing post" to "must actually be a member-type post," closing a related hole for direct profile-field saves (e.g. profile edit).

I verified bp_get_active_member_types() returns post IDs ('fields' => 'ids'), so the array_map( 'absint', ... ) / in_array( ..., true ) usage in the new helper is correct, and that bp_assign_default_member_type_to_activate_user() is hooked to bp_core_activated_user, which fires regardless of entry point.

Findings

1. REST signup endpoint doesn't get the same friendly validation (minor, not a security hole)
src/bp-members/classes/class-bp-rest-signup-endpoint.php:788-794 has its own parallel field-validation loop for /signup REST registrations, but it wasn't updated with the new bb_is_member_type_allowed_on_registration() check. This isn't an actual vulnerability — the activation-time backstop (item 3 above) still resets the type/role to the default — but a REST client submitting a hidden/disallowed member type will silently get the default type instead of a clear validation error, unlike the form-based flow. Consider adding the same check there for consistency, if REST signups are a supported client-facing path.

2. Minor @since inconsistency
class-bp-xprofile-field-type-member-types.php:234 adds @since BuddyBoss 1.0.0 to is_valid(), but every other method in this class (introduced together) uses @since BuddyBoss 1.1.3 (see the class docblock at line 6 and all sibling methods). 1.0.0 looks like a copy/paste artifact rather than the method's actual origin version.

3. Pre-existing, not introduced by this PR
is_valid( $values ) (and the xprofile_set_field_data() path that calls it, e.g. during profile edits) doesn't guard against $values being an array before calling get_post_type( $values ), which could emit a PHP notice/warning on an array-to-int coercion. The new registration-path code explicitly guards against this (is_array( $bb_submitted_member_type ) short-circuit), but the same defensive treatment isn't mirrored inside is_valid() itself. Not a regression from this PR (the old get_post( $values ) had the same gap), just an easy follow-up if you want to close it everywhere at once.

Notes

  • Could not run composer lint-php / php -l / PHPUnit in this sandbox — Bash execution requires interactive approval that wasn't available here. Manual review of the diff didn't surface syntax issues, and the added code follows existing WPCS conventions (tabs, Yoda comparisons, phpcs:ignore used appropriately for the pre-existing no-nonce registration pattern).
  • No functional issues found; the core logic is correct and consistent with the codebase's existing patterns for this field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant