diff --git a/inc/admin-pages/customer-panel/class-template-switching-admin-page.php b/inc/admin-pages/customer-panel/class-template-switching-admin-page.php index 071691702..ae3b014aa 100644 --- a/inc/admin-pages/customer-panel/class-template-switching-admin-page.php +++ b/inc/admin-pages/customer-panel/class-template-switching-admin-page.php @@ -166,5 +166,46 @@ public function output(): void { public function register_widgets(): void { \WP_Ultimo\UI\Simple_Text_Element::get_instance()->as_inline_content(get_current_screen()->id, 'wu_dash_before_metaboxes'); \WP_Ultimo\UI\Template_Switching_Element::get_instance()->as_inline_content(get_current_screen()->id, 'wu_dash_before_metaboxes'); + + /* + * Defence-in-depth: if both as_inline_content() calls bailed silently + * (e.g. via a third-party `wu_template_switching_should_display` + * filter, or because the page was reached on an unsupported screen), + * the page would render with three empty meta-box columns and no + * explanation. Wrap the action in a buffer so we can detect + * "nothing printed" and emit a fallback notice. Priority 5 starts + * the buffer; priority 999 closes it and emits either the captured + * output or a fallback message. + */ + add_action( + 'wu_dash_before_metaboxes', + static function (): void { + ob_start(); + }, + 5 + ); + + add_action( + 'wu_dash_before_metaboxes', + static function (): void { + $captured = ob_get_clean(); + + if (false === $captured || '' === trim((string) $captured)) { + printf( + '
' . + '

%1$s

' . + '

%2$s

' . + '
', + esc_html__('Template switching is not available right now.', 'ultimate-multisite'), + esc_html__('No template switching widgets are available on this page. If you reached this page expecting to switch your site template, please contact your network administrator.', 'ultimate-multisite') + ); + + return; + } + + echo $captured; // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped -- previously-buffered widget HTML, already escaped at source. + }, + 999 + ); } } diff --git a/inc/models/class-checkout-form.php b/inc/models/class-checkout-form.php index a29f1b899..3257a75c7 100644 --- a/inc/models/class-checkout-form.php +++ b/inc/models/class-checkout-form.php @@ -1030,11 +1030,9 @@ public static function convert_steps_to_v2($steps, $old_settings = []) { $templates[] = $site->get_id(); } - $old_template_list = is_array($old_template_list) ? $old_template_list : []; + $old_template_list = is_array($old_template_list) ? $old_template_list : []; - $template_list = array_flip($old_template_list); - - $template_list = ! empty($template_list) ? $template_list : $templates; + $template_list = ! empty($old_template_list) ? $old_template_list : $templates; $step['fields'] = [ 'template_selection' => [ diff --git a/inc/models/class-membership.php b/inc/models/class-membership.php index 7b746c0e4..0bc92d09d 100644 --- a/inc/models/class-membership.php +++ b/inc/models/class-membership.php @@ -467,7 +467,23 @@ public function is_customer_allowed($customer_id = false) { $customer_id = $customer ? $customer->get_id() : 0; } - $allowed = absint($customer_id) === absint($this->get_customer_id()); + $customer_id = absint($customer_id); + $membership_customer_id = absint($this->get_customer_id()); + + /* + * Reject the "no customer in context vs membership with no customer" case. + * + * See the matching note on Site::is_customer_allowed(). Treating + * `0 === 0` as "allowed" was a privilege-escalation surface: a + * logged-in user with no UM customer association would be granted + * access to memberships whose customer link is missing. Default to + * denied unless both sides are known and match. + */ + if (0 === $customer_id || 0 === $membership_customer_id) { + $allowed = false; + } else { + $allowed = $customer_id === $membership_customer_id; + } return apply_filters('wu_membership_is_customer_allowed', $allowed, $customer_id, $this); } diff --git a/inc/models/class-site.php b/inc/models/class-site.php index 3dceaa8a1..6d9e1b538 100644 --- a/inc/models/class-site.php +++ b/inc/models/class-site.php @@ -1053,7 +1053,28 @@ public function is_customer_allowed($customer_id = false) { $customer_id = $customer ? $customer->get_id() : 0; } - $allowed = absint($customer_id) === absint($this->get_customer_id()); + $customer_id = absint($customer_id); + $site_customer_id = absint($this->get_customer_id()); + + /* + * Reject the "no customer in context vs site with no customer" case. + * + * Previously this method returned true when both ids were 0 (`0 === 0`), + * which silently granted access on sites that have not been linked to a + * customer (e.g. orphaned customer_owned sites or fixture/test sites + * with missing wu_customer_id meta). That was a privilege-escalation + * surface: a logged-in user with no UM customer association could be + * treated as the "owner" of any site whose customer link was missing. + * + * The correct interpretation is: only super admins (handled above) and + * the actual linked customer can be considered allowed. If either side + * is unknown, default to denied. + */ + if (0 === $customer_id || 0 === $site_customer_id) { + $allowed = false; + } else { + $allowed = $customer_id === $site_customer_id; + } return apply_filters('wu_site_is_customer_allowed', $allowed, $customer_id, $this); } diff --git a/inc/ui/class-template-switching-element.php b/inc/ui/class-template-switching-element.php index a49a33dc6..762212f8c 100644 --- a/inc/ui/class-template-switching-element.php +++ b/inc/ui/class-template-switching-element.php @@ -23,6 +23,26 @@ class Template_Switching_Element extends Base_Element { use \WP_Ultimo\Traits\Singleton; + /** + * Permission state: site exists, customer is allowed, full switching UI. + */ + const STATE_OK = 'ok'; + + /** + * Permission state: site exists, customer is allowed, but no membership + * is linked. Switching is still permitted; the available templates fall + * back to whatever the site's limitations expose. + */ + const STATE_NO_MEMBERSHIP = 'no_membership'; + + /** + * Permission state: no site, or the site exists but the current user is + * not its customer (and not a network admin). UI shows a denial notice + * instead of the switching grid so the user is not left staring at an + * empty page wondering what went wrong. + */ + const STATE_NOT_ALLOWED = 'not_allowed'; + /** * The id of the element. * @@ -57,6 +77,18 @@ class Template_Switching_Element extends Base_Element { */ protected $products; + /** + * Permission state computed during setup(). + * + * Used by output() to decide whether to render the full grid, the grid + * with a "no membership" notice, or a denial notice. Always set to one + * of the STATE_* constants by the time output() runs. + * + * @since 2.5.2 + * @var string + */ + protected $permission_state = self::STATE_OK; + /** * The icon of the UI element. * e.g. return fa fa-search @@ -227,17 +259,27 @@ public function setup() { $this->site = wu_get_current_site(); + /* + * Decide which UI state to render. + * + * Previously this method called $this->set_display(false) whenever the + * customer was not allowed or no site was found, which left the page + * with three empty meta-box columns and no explanation — a confusing + * dead-end for end users. We now always render something: either the + * full grid, the grid with a "no membership" notice, or a denial + * notice. The actual server-side authorization for AJAX switches + * stays in switch_template(). + */ if ( ! $this->site || ! $this->site->is_customer_allowed()) { - $this->set_display(false); + $this->permission_state = self::STATE_NOT_ALLOWED; + $this->membership = null; + $this->products = []; return; } $this->membership = $this->site->get_membership(); - - $this->products = []; - - $all_membership_products = []; + $this->products = []; if ($this->membership) { $all_membership_products = $this->membership->get_all_products(); @@ -247,6 +289,19 @@ public function setup() { $this->products[] = $product['product']->get_id(); } } + + $this->permission_state = self::STATE_OK; + } else { + /* + * The customer owns this site but no membership is linked. This + * happens for sites created outside the normal checkout flow + * (manual admin creation, fixtures, legacy migrations, or after + * a membership is deleted but the site is preserved). The + * customer should still be able to switch templates — we simply + * skip the per-product template restriction and let the site's + * own limitations drive the available list. + */ + $this->permission_state = self::STATE_NO_MEMBERSHIP; } } @@ -284,6 +339,21 @@ public function switch_template() { return; } + /* + * Authorization: confirm the requesting user owns this site. + * + * The wu-ajax-nonce check in class-light-ajax.php protects against + * CSRF, but the nonce is shared across all logged-in users on the + * install. Without this check, customer A could replay a valid nonce + * to switch the template (and overwrite content) on customer B's + * site by passing a forged site context. Network admins bypass this + * via the manage_network short-circuit inside is_customer_allowed(). + */ + if ( ! $this->site->is_customer_allowed()) { + wp_send_json_error(new \WP_Error('not_authorized', __('You do not have permission to switch templates on this site.', 'ultimate-multisite'))); + return; + } + $template_id = (int) wu_request('template_id', ''); // false means MODE_DEFAULT (no restriction) — all templates are allowed. @@ -356,6 +426,27 @@ public function switch_template() { */ public function output($atts, $content = null) { + /* + * Render an explicit denial notice when the customer is not allowed + * to switch templates on this site (or there is no site context at + * all). Previously this branch produced an empty page with no + * explanation; users would see a "Switch Template" header and a + * blank body. Showing a notice keeps the UX informative. + */ + if (self::STATE_NOT_ALLOWED === $this->permission_state) { + ?> +
+

+ +

+

+ +

+
+ site) { $filter_template_limits = \WP_Ultimo\Limits\Site_Template_Limits::get_instance(); @@ -363,6 +454,27 @@ public function output($atts, $content = null) { $template_selection_field = $filter_template_limits->maybe_filter_template_selection_options($atts); + /* + * When the customer's site has no linked membership we have an + * empty $atts['products']. The shared limits filter only + * populates $attributes['sites'] when products are non-empty + * (see Site_Template_Limits::maybe_filter_template_selection_options), + * so without intervention we'd render the friendly "no + * membership" banner above an empty grid — defeating the whole + * point of letting the customer switch templates anyway. + * + * Fall back to every registered site template (the same list + * defaults() builds via wu_get_site_templates()). The customer + * still cannot bypass per-site rules: server-side authorization + * lives in switch_template() which calls is_customer_allowed() + * before applying the chosen template. + */ + if (self::STATE_NO_MEMBERSHIP === $this->permission_state && ! isset($template_selection_field['sites'])) { + $default_sites = wu_get_site_templates(['fields' => 'ids']); + + $template_selection_field['sites'] = is_array($default_sites) ? $default_sites : []; + } + if ( ! isset($template_selection_field['sites'])) { $template_selection_field['sites'] = []; } @@ -489,6 +601,24 @@ public function output($atts, $content = null) { ], ]; + /* + * Inform the customer when their site has no membership link. + * They can still switch templates, but pricing/product-tier + * restrictions don't apply, so the available list may differ + * from what they would normally see. Without this notice the UI + * looks identical to the normal flow but quietly behaves + * differently — better to be explicit. + */ + if (self::STATE_NO_MEMBERSHIP === $this->permission_state) { + ?> +
+

+ +

+
+ assertFalse($result, 'is_customer_allowed should return false for a different customer ID.'); } + /** + * Test is_customer_allowed denies the "no customer in context vs site + * with no customer" case. + * + * Regression guard: previously, when both the requesting customer id and + * the site's stored customer id were 0 (e.g. an orphaned customer_owned + * site with missing wu_customer_id meta, queried by a logged-in user + * with no UM customer association), the method returned true because of + * the `0 === 0` comparison. That silently granted "owner" access on + * unlinked sites — used to render the customer-panel template-switching + * UI and (worse) to gate the AJAX switch handler indirectly. The fix + * defaults to denied when either side is unknown. + */ + public function test_is_customer_allowed_zero_versus_zero_denied(): void { + // Ensure the current user is NOT a network admin (manage_network short-circuits). + $subscriber_id = $this->factory()->user->create(['role' => 'subscriber']); + wp_set_current_user($subscriber_id); + + $this->site->set_customer_id(0); + + // Calling with explicit 0 customer id (the path used by the AJAX handler + // when no UM customer is in context). + $this->assertFalse( + $this->site->is_customer_allowed(0), + 'is_customer_allowed must deny when both the requesting customer id and the site customer id are 0.' + ); + } + + /** + * Test is_customer_allowed denies a real customer when the site has no + * customer link. + * + * A site with customer_id == 0 has no owner. Even a real customer should + * not be considered "allowed" on it — only network admins (handled by + * the manage_network short-circuit) can act on unlinked sites. + */ + public function test_is_customer_allowed_known_customer_versus_unlinked_site_denied(): void { + $subscriber_id = $this->factory()->user->create(['role' => 'subscriber']); + wp_set_current_user($subscriber_id); + + $this->site->set_customer_id(0); + + $customer_id = $this->customer->get_id(); + $this->assertFalse( + $this->site->is_customer_allowed($customer_id), + 'is_customer_allowed must deny when the site has no linked customer, even if a real customer id is provided.' + ); + } + /** * Test get_customer returns false when customer_id is 0. */ diff --git a/tests/WP_Ultimo/UI/Template_Switching_Element_Test.php b/tests/WP_Ultimo/UI/Template_Switching_Element_Test.php index 7f4711af5..e45ccdb05 100644 --- a/tests/WP_Ultimo/UI/Template_Switching_Element_Test.php +++ b/tests/WP_Ultimo/UI/Template_Switching_Element_Test.php @@ -168,6 +168,18 @@ public function test_switch_template_missing_site_emits_json_error(): void { */ public function test_switch_template_missing_template_id_emits_json_error(): void { + /* + * Bypass the new is_customer_allowed() authorization gate added to + * switch_template(). The freshly-created blog has customer_id 0, so + * a normal user would hit the "not_authorized" branch before + * reaching the template_id check we want to exercise here. Network + * admins short-circuit is_customer_allowed() via the manage_network + * capability — that's what we use to land on the template_id path. + */ + $super_admin_id = $this->factory()->user->create( [ 'role' => 'administrator' ] ); + grant_super_admin( $super_admin_id ); + wp_set_current_user( $super_admin_id ); + // Force a fake site object so the "missing site context" guard is bypassed // and we hit the template_id check. $site_id = $this->factory()->blog->create(); @@ -196,4 +208,68 @@ public function test_switch_template_missing_template_id_emits_json_error(): voi $this->assertSame( false, $decoded['success'] ); } + /** + * Unauthorized caller must be rejected with a JSON error body. + * + * Regression guard for the privilege-escalation hole: the wu-ajax-nonce + * shared across all logged-in users meant any non-customer with a valid + * nonce could call wu_ajax_wu_switch_template against another customer's + * site (or against an orphan customer_owned site with customer_id 0, + * which previously satisfied is_customer_allowed() via 0 === 0). The + * handler must now refuse to proceed before reaching Site_Duplicator. + */ + public function test_switch_template_rejects_unauthorized_caller(): void { + + // A logged-in user who is NOT the site's customer and NOT a network + // admin. With the security fix in place, is_customer_allowed() must + // return false for this user. + $subscriber_id = $this->factory()->user->create( [ 'role' => 'subscriber' ] ); + wp_set_current_user( $subscriber_id ); + + $site_id = $this->factory()->blog->create(); + $site = wu_get_site( $site_id ); + $site->set_type( 'customer_owned' ); + // customer_id stays at 0 — this is the orphan-site case that used + // to satisfy is_customer_allowed() via the 0 === 0 comparison. + $site->save(); + + $element = Template_Switching_Element::get_instance(); + $ref = new \ReflectionProperty( $element, 'site' ); + + if ( PHP_VERSION_ID < 80100 ) { + $ref->setAccessible( true ); + } + + $ref->setValue( $element, $site ); + + // Even with a valid template_id, the auth check must fire first. + $_REQUEST['template_id'] = '1'; + + $result = $this->call_switch_template(); + + $this->assertTrue( + $result['exception'], + 'wp_send_json_error must be reached so the unauthorized request terminates with a JSON body.' + ); + + $decoded = $this->decode_json( $result['output'] ); + $this->assertSame( false, $decoded['success'] ); + + // The WP_Error code travels in $decoded['data'][0]['code'] when a + // WP_Error is passed to wp_send_json_error(). + $this->assertNotEmpty( + $decoded['data'], + 'Unauthorized response must include a payload so the JS can surface the reason.' + ); + + // Be tolerant of payload shape variations across WP versions: search + // the serialized payload for the not_authorized error code. + $serialized = wp_json_encode( $decoded ); + $this->assertStringContainsString( + 'not_authorized', + (string) $serialized, + 'Unauthorized response must carry the not_authorized error code, not a generic failure.' + ); + } + }