#1408 Fix - Prevent membership privilege escalation and open redirect after login - #1422
Conversation
|
Build for ⬇️ Download user-registration-5.2.7.zip (8.1M) Installs directly via Plugins → Add New → Upload Plugin. |
There was a problem hiding this comment.
🟡 Changes recommended
There is at least one confirmed functional bug that can cause runtime errors (auth_callback signature mismatch) and should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens the membership and login flows to address two reported vulnerabilities: (1) preventing membership plan–based role/privilege escalation and (2) preventing open redirects after login, while preserving legitimate admin-configured behavior (e.g., External URL redirects and privileged roles intentionally granted by authorized admins).
Changes:
- Lock down membership-related CPT capabilities and protect membership/group meta keys from core’s custom-fields write path.
- Add layered role safety validation/backstops during membership save and role assignment, and harden membership purchase/upgrade AJAX paths (plan allowlist + payment-method allowlist).
- Fix login redirects by validating/safeguarding redirects (including AJAX login) and allowing only the admin-configured External URL host when that setting is enabled.
File summaries
| File | Description |
|---|---|
| modules/membership/includes/AJAX.php | Rejects non-purchasable plans and invalid payment methods in membership purchase/upgrade AJAX handlers. |
| modules/membership/includes/Admin/Services/SubscriptionService.php | Centralizes payment-method validation via shared MembershipService logic. |
| modules/membership/includes/Admin/Services/MembersService.php | Applies role “safe” backstop during immediate and deferred (pending) role grants. |
| modules/membership/includes/Admin/Services/MembershipService.php | Adds purchasable-plan check and shared payment-method validation (including 100%-coupon free order rules). |
| modules/membership/includes/Admin.php | Locks CPT capabilities and registers protected post meta with auth_callback. |
| includes/Functions/CoreFunctions.php | Introduces privileged-role detection and a “safe role” resolver with logging + filters. |
| includes/functions-ur-core.php | Fixes open redirect by validating/safely redirecting on both success and failed login paths (incl. AJAX). |
| includes/frontend/class-ur-frontend.php | Whitelists admin-configured External URL host via allowed_redirect_hosts when that feature is enabled. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if ( ur_is_ajax_login_enabled() && empty( $_POST['resubmitted'] ) ) { // phpcs:ignore | ||
| wp_send_json_success( array( 'message' => $redirect ) ); | ||
| wp_send_json( $user ); | ||
| } else { | ||
| wp_redirect( wp_validate_redirect( $redirect, $redirect ) ); // phpcs:ignore | ||
| wp_safe_redirect( $redirect ); |
| public function can_manage_membership_meta() { | ||
| return current_user_can( 'manage_options' ); | ||
| } |
…ture and drop dead AJAX-login code
|
Thanks — addressed both in 6e71d1e. 1. Dead code in the AJAX-login success path ( 2. That said, the underlying suggestion is a good one, so I applied it: the method now declares the documented callback parameters and returns PHPCS: clean on both changed files. |
|
@tg-autopilot review Addressed both review comments in 6e71d1e (dead AJAX-login code removed; meta |
There was a problem hiding this comment.
🟡 Changes recommended
The new register_post_meta auth_callback has an incompatible signature and authorization logic that can cause PHP 8+ runtime errors and/or unintended always-deny behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
includes/frontend/class-ur-frontend.php:268
- allow_custom_redirect_host() appends the external host to $hosts every time the allowed_redirect_hosts filter runs, which can introduce duplicate entries within a request and across multiple validations. Deduplicating here avoids unnecessary growth/processing and keeps the allow-list stable.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| /** | ||
| * Whether the current user may write membership post meta. | ||
| * | ||
| * Registered as a register_post_meta() auth_callback, which WordPress invokes with the | ||
| * filter arguments below. AND-ing with $allowed means this can only ever narrow a core | ||
| * decision, never widen one. | ||
| * | ||
| * @since 5.2.8 | ||
| * | ||
| * @param bool $allowed Whether the write is allowed so far. | ||
| * @param string $meta_key Meta key being written. | ||
| * @param int $object_id Post ID. | ||
| * @param int $user_id User attempting the write. | ||
| * @return bool True when the user can manage the site's options. | ||
| */ | ||
| public function can_manage_membership_meta( $allowed = false, $meta_key = '', $object_id = 0, $user_id = 0 ) { | ||
| return $allowed && current_user_can( 'manage_options' ); | ||
| } |
There was a problem hiding this comment.
Addressed in 20e777d. Two notes on the specifics, since both claims here are testable and don't hold on this codebase:
-
ArgumentCountError— PHP only raises this for too few required arguments, never for extra positional ones (a callback declaring fewer params than WordPress passes simply ignores the surplus). Verified: a 0-arg function called with 6 args returns normally, no error. This plugin also targets PHP 7.4 (Requires PHP: 7.4). So there was no runtime error — but I've still widened the signature to the full documented six params ($allowed, $meta_key, $object_id, $user_id, $cap, $caps) so it matches the filter exactly. -
"always deny" —
$allowedis onlyfalsehere if the meta were protected. WordPress passes$allowed = ! is_protected_meta( $meta_key, 'post' ), and these keys have no leading underscore, so it istrue. The= falsein the signature is just the zero-arg default, which the filter never triggers. Verified live before and after this change:apply_filters( 'auth_post_meta_ur_membership_for_ur_membership', … )returnstruefor an administrator andfalsefor an Author. To remove the ambiguity entirely, the body now returnscurrent_user_can( 'manage_options' )directly rather than combining with$allowed.
Also deduped the host in allow_custom_redirect_host() (the suppressed note) so the allow-list can't grow within a request. PHPCS clean on both files.
…re and dedupe the redirect host
|
@tg-autopilot review Follow-up in 20e777d: |
There was a problem hiding this comment.
🔵 Needs a closer look
Security-critical changes span multiple membership and login entry points and should receive final human verification despite only minor review notes.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
modules/membership/includes/Admin/Services/MembershipService.php:128
- is_membership_purchasable() currently loads the entire active membership list (DB query + filtering) and then linearly scans it. This method is called in AJAX handlers, so this adds avoidable overhead and duplicates other membership lookups. Consider checking the specific membership ID and then applying the same frontend-list filter to that single item.
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
| * @return bool True when the user can manage the site's options. | ||
| */ | ||
| public function can_manage_membership_meta( $allowed = false, $meta_key = '', $object_id = 0, $user_id = 0, $cap = '', $caps = array() ) { | ||
| return current_user_can( 'manage_options' ); |
There was a problem hiding this comment.
Good catch — fixed in 66ae9c0. The callback now returns user_can( $user_id, 'manage_options' ), so it evaluates the user WordPress passes rather than the current request user. Verified: evaluating for an Author while the current user is an admin now correctly returns false (the old current_user_can() would have wrongly allowed it); the exploit path — where the acting user is the current user — still denies Authors and allows admins.
On the suppressed note about is_membership_purchasable(): keeping it as-is, deliberately. It reuses list_active_memberships() because that applies the build_membership_list_frontend filter, so a plan a site has hidden through that filter can't be purchased either — and this site does hook that filter. Checking a single membership ID and applying a list-context filter to one item wouldn't reproduce that reliably and could let a hidden plan through. At realistic plan counts (16 here) the scan cost is negligible next to that guarantee.
…s passes, not the current user
|
@tg-autopilot review 66ae9c0: meta auth_callback now checks the passed |
There was a problem hiding this comment.
🔵 Needs a closer look
The new centralized payment-method validator currently treats free plans as accepting any submitted payment method, which can drive inconsistent order/subscription behavior in purchase flows.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
modules/membership/includes/Admin/Services/MembershipService.php:147
- is_valid_payment_method_for_membership() currently returns true for free memberships regardless of the submitted payment method. Because purchase flows branch on $data['payment_method'] (e.g., treating non-'free' as a gateway and setting initial status to pending), a forged request can force a free plan down a paid-gateway path and create inconsistent orders/subscriptions. For free plans, only accept an empty payment method or 'free'.
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
… take the paid order path
|
@tg-autopilot review 376e9de addresses the suppressed note on |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes multiple security-critical pathways (role assignment, purchasability checks, and redirect handling) where subtle regressions can have high impact and merits final human validation.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
y000yal
left a comment
y000yal
left a comment
There was a problem hiding this comment.
LGTM 👍
Traced this in depth given the severity:
- CPT capability lock + meta
auth_callback— correct signature/hook order, doesn't touchread/read_private_postsso plan listings still render, doesn't affect the plugin's ownupdate_post_meta()calls (those bypassauth_callbackentirely, only core's classic-editor/REST meta paths consult it). ur_membership_get_safe_role()backstop inMembersService::update_user_meta()— confirmed $membership_id resolves correctly for every registration path (free, paid immediate, deferred), not just deferred ones, by tracingprepare_members_data()—membership_data['membership']is always set from$data['membership']regardless of plan type. So a legitimate admin-authored plan with a privileged role (e.g. a real staffeditorplan) won't get falsely downgraded.validate_membership_data()correctly requirespromote_usersto save a privileged role at write time.- Payment-method/purchasability checks consolidated into one shared method and applied at all three self-service entry points (registration, upgrade, add_multiple_membership) — no drift between them.
- Open redirect: validation hoisted above the AJAX branch so it also covers the client-side
window.location.hrefnavigation, empty-referer case guarded explicitly, and the External URL allowlist is scoped to only when that setting is actually active.
Checked MembersRepository::create() (the other wp_insert_user()/set_role() site) — capability-gated behind add_users, not reachable unauthenticated, so no sibling gap there. The edit_member / promote_users mismatch you flagged as separately-filed is correctly out of scope for this one.
One tracking note: includes/Functions/CoreFunctions.php, includes/frontend/class-ur-frontend.php, includes/functions-ur-core.php, and all 5 changed modules/membership files are currently byte-identical between this repo and themegrill/user-registration-pro — per the issue's own instruction that's intentional (fix free, let sync propagate), but pro stays exploitable until that sync PR actually lands and merges there, so worth someone tracking that it does.
All Submissions:
Changes proposed in this Pull Request:
Fixes the two vulnerabilities from the WPScan report (themegrill/user-registration-pro#1408), both reproduced live against the current code and re-verified fixed. Researchers to credit: Baikuya (Jonathan Dersch) (privilege escalation) and Sai Praneeth Koti (open redirect).
Vulnerability 1 — Author → administrator privilege escalation (CVSS 7.2). The chain was: the
ur_membershipCPT registered withcapability_type => 'post'and nocapabilitiesmap, so any Contributor+ could author and publish a plan and, because theur_membershipmeta key is not protected, writerole: administratorinto it through core's own edit-post/add-meta path. The self-serviceadd_multiple_membershipAJAX handler then granted that role synchronously on aselected_pg=freerequest, gated only by a nonce that is localized to every visitor of a membership page. Reproduced end to end: an Author reachedadministrator+manage_optionsin three requests.The fix is layered so no single change carries the whole weight, and — importantly — so it does not break plans an administrator has legitimately configured with a privileged role (e.g. a real "staff" plan granting
editor):modules/membership/includes/Admin.php) — bothur_membershipandur_membership_groupsnow map every primitive post capability tomanage_options, keeping non-admins out ofpost-new.php/post.phpand, becauseadd_post_metamaps throughedit_post, out of the custom-fields write path too. This is the load-bearing layer.auth_callback(Admin.php) — theur_membership/ur_membership_groupsmeta keys are registered with anauth_callbackrequiringmanage_options, so core's add-meta path stays refused even if a site re-widens the post type through theuser_registration_membership_post_typefilter. The plugin's ownupdate_post_meta()calls are unaffected.MembershipService::validate_membership_data()) — a privileged role can only be attached to a plan by a user who holdspromote_users, and a non-existent role is rejected. This closes the injection at its source while leaving legitimate admin-authored plans intact.ur_membership_get_safe_role()inincludes/Functions/CoreFunctions.php, applied inMembersService::update_user_meta()andmaybe_grant_pending_role()) — a plan whose author cannot assign roles can never grant a privileged one, whatever its stored value; the requested role is honoured only when the plan's author holdspromote_users. Every downgrade is logged. The blocklist deliberately excludesunfiltered_htmlandlist_usersso the stockeditorand WooCommerceshop_managerroles remain valid plan roles.modules/membership/includes/AJAX.php) —add_multiple_membershipandupgrade_membershipnow reject a plan the site does not currently offer, andadd_multiple_membershipgained the payment-method allowlist the registration and upgrade paths already had (extracted into one sharedMembershipServicemethod so the three cannot drift). This closes theselected_pg=freelever, which was also free access to any paid plan.Vulnerability 2 — open redirect in the login handler (CVSS 6.1).
ur_process_login()redirected to a request-controlled target without a same-host check: the failed-login branch used the raw referer unvalidated (exploitable fully unauthenticated), and the success branch calledwp_validate_redirect( $redirect, $redirect ), which is a no-op because the untrusted value was its own fallback. Reproduced: a login response reflectedhttps://evil.example/phish; after the fix it returns the home URL.wp_safe_redirect()with a local fallback, and the validation is hoisted above the AJAX branch so it also covers the client-side navigation (window.location.href = res.data.message) that a fix on the header redirect alone would miss.wp_validate_redirect( '', $fallback )returns'', not the fallback.allowed_redirect_hostsfilter (UR_Frontend::allow_custom_redirect_host()) whitelists the host of the admin-configured External URL login redirect, and only when that setting is active, so validating the redirect does not break that documented feature.Not changed, deliberately: the unauthenticated registration path was already hardened (the plan id is read server-side and validated against the form's offered plans), so it is left as-is. Two related items are out of scope and will be filed separately: two open redirects in pro-only code (
includes/pro/functions-ur-pro.phppasswordless magic-link,includes/pro/addons/sms-integration/Frontend.php) that the free→pro sync cannot carry, and a separateedit_memberrole-assignment gap (edit_usersreachingset_role()where core requirespromote_users).Note: this diff overlaps the approved #1417 on one line (
functions-ur-core.php:5408); whichever merges second resolves a trivial one-line conflict, and this version keeps #1417'sallowed_redirect_hostsapproach so the two are compatible.@sincetags assume the next release is 5.2.8. The changelog is intentionally not touched, per CONTRIBUTING.Closes themegrill/user-registration-pro#1408
How to test the changes in this Pull Request:
Privilege escalation (as an Author):
wp-admin/post-new.php?post_type=ur_membership.403.editposttowp-admin/post.phpsettingmetakeyinput=ur_membershipandmetavalue={"type":"free","role":"administrator",...}.auth_callbackrefuses it.upgrade_membership_noncefrom any membership page and POSTaction=user_registration_membership_add_multiple_membership,selected_pg=free,selected_membership_id=<a plan whose stored role is administrator>.administrator. After: the account gainssubscriber, and a warning is logged ("Refused to grant privileged role ...").administrator(oreditor) — the role must still be granted. Save a plan as an administrator with roleadministrator(must succeed) and as a lower role (must fail with "Sorry, you are not allowed to assign that role to a membership.").selected_pg=freeagainst a paid plan must be rejected ("Invalid payment method for this membership."); a real purchase per configured gateway and a 100%-coupon order must still work.Open redirect (unauthenticated where noted):
_wp_http_referer=https://evil.example/phish→ must land on My Account, notevil.example. With no referer at all → an absolute My Account URL.action=user_registration_ajax_login_submitwith valid credentials andredirect=https://evil.example/phish→data.messagemust be the home URL, notevil.example. Also try//evil.exampleandhttps:evil.example.https://example.com/welcome→ a successful login must still reach it; switch the setting to Internal Page and confirmexample.comis no longer whitelisted.Types of changes:
Other information:
PHPCS: clean on all changed lines (ruleset
phpcs.xml, PHP_CodeSniffer 3.13.5, 8 files). The twoWordPress.Security.NonceVerification.Missingwarnings on the new$_POSTreads carry a per-linephpcs:ignore— the nonce is verified viaur_membership_verify_nonce()at the top of each handler, which PHPCS cannot see. Verified end to end on a live site with the pro plugin active (the free plugin'sincludes/modulesare the sync source for pro): the full escalation was reproduced before the fix and blocked after, each layer independently; the open redirect reflectedevil.examplebefore and the home URL after.Changelog entry
Fix - Privilege escalation via membership role and open redirect after login.