fix: site provisioning hardening — limitations, membership inference, domain promotion#965
Merged
superdav42 merged 5 commits intomainfrom Apr 29, 2026
Merged
Conversation
When limitations are serialized through external flows (e.g. the
WooCommerce addon), the enabled flag for capabilities like
domain_mapping can arrive as '' (empty string) instead of being
absent. Previously, (bool)'' evaluated to false, causing the
capability to be treated as explicitly disabled instead of
inheriting from the parent plan/membership.
This affected 23 of 31 customer subsites in a production report
where domain_mapping.enabled was '' — Pro customers could not see
the 'Add Custom Domain' UI despite their plan having it enabled.
Two fixes:
1. Limit::setup() — treat '' for enabled the same as 'not-set',
marking has_own_enabled=false so the default/inherited value
is used instead of casting '' to false.
2. Limitations::merge_recursive() — use strict false check instead
of falsy check when deciding to collapse a module to
{enabled:false}. This prevents empty-string or absent enabled
values from wiping inherited limits during the waterfall merge.
When a site is created through an external gateway (e.g. the WooCommerce addon), set_membership_id() may not be called before save(). This leaves wu_membership_id empty in blogmeta, which cascades: get_membership() returns false, customer_id is never written, and downstream code (Frontend Admin, renewal emails, admin panels) sees the site as unlinked. Add a defensive fallback: if get_membership() returns false but the site has a customer_id, look up the customer's active memberships. If exactly one exists, auto-link it. This is conservative — only acts when the inference is unambiguous (single active membership) to avoid misattribution. The primary fix belongs in the WooCommerce addon's gateway class which should call set_membership_id() and set_customer_id() before save(). This core-side fallback is a safety net.
When a custom domain (e.g. elimartz.com) completes DNS/SSL verification and reaches 'done' or 'done-without-ssl' stage, it should become the primary domain for its blog — demoting the default subdomain (e.g. elimartz.kursopro.com). Previously, both domains ended up with primary_domain=0, causing: - No 301 redirect from subdomain to custom domain - SEO duplicate content across both URLs - Customer confusion about which URL to use The new handler hooks into wu_domain_post_save and auto-promotes when: 1. The domain just reached done/done-without-ssl stage 2. It is a custom domain (not a subdomain) 3. It is not already primary 4. No other custom domain is already primary for the blog This works regardless of whether the stage was set by core's async_process_domain_stage or by an external plugin.
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLimit handling now treats absent vs empty-string Changes
Sequence Diagram(s)sequenceDiagram
participant Event as "wu_domain_post_save (event)"
participant DomainManager as "Domain_Manager::maybe_auto_promote_primary_domain"
participant Domain as "Domain Model"
participant DB as "DB / Blog Meta"
participant Logger as "Logger"
Event->>DomainManager: invoke with ($data, $domain, $new)
DomainManager->>Domain: check is_primary, is_main_domain, stage
Domain-->>DomainManager: stage done/done-without-ssl?
alt eligible
DomainManager->>DB: query for other primary main-domain on same blog
DB-->>DomainManager: found? (yes/no)
alt none found
DomainManager->>Domain: set_primary_domain(true)
DomainManager->>Domain: save()
Domain-->>DomainManager: save result / WP_Error
DomainManager->>Logger: log auto-promotion or error
else already exists
DomainManager->>Logger: log skipped due to existing primary
end
else not eligible
DomainManager->>Logger: log skipped (subdomain / already primary)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@inc/managers/class-domain-manager.php`:
- Around line 1165-1177: The code calls $domain->set_primary_domain(true) then
$domain->save() but always writes a success log with
wu_log_add("domain-{$domain_url}", ...) regardless of whether save() succeeded;
update the flow in the promotion routine to capture the boolean/return value
from $domain->save(), only call wu_log_add(...) with the "Auto-promoted..."
message when save() returns success, and add a failure path that logs an error
(using wu_log_add with a distinct key/message) including $domain_url and
$blog_id if save() fails so failed promotions are visible.
- Around line 1151-1158: The current guard only skips auto-promotion for domains
where self::is_main_domain($existing->get_domain()) is true, which misses custom
primary domains like "www.example.com"; replace that condition with a check that
detects any existing primary domain (not just main ones) — for example, check
the domain object's primary flag (e.g., $existing->is_primary() or
$existing->get_is_primary()) or compare $existing->get_domain() against the
blog's known primary domain via the manager API — and return early when any
existing domain is marked primary to avoid auto-promoting another domain.
In `@inc/models/class-site.php`:
- Around line 1990-2007: After inferring and persisting the membership ID you
must also refresh the object's in-memory membership cache so subsequent calls to
get_membership() in the same request don't return the stale false value; after
calling $this->set_membership_id($membership->get_id()) and
update_site_meta(...), either set the cached membership directly (e.g.
$this->set_membership($membership) if a setter exists) or clear/refresh the
cached value and then call $this->get_membership() to reload it, ensuring the
object's membership cache reflects the newly inferred membership.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9181ad4b-78f0-433d-a0e9-d538e5fc3057
📒 Files selected for processing (4)
inc/limitations/class-limit.phpinc/managers/class-domain-manager.phpinc/models/class-site.phpinc/objects/class-limitations.php
Comment on lines
+1151
to
+1158
| foreach ($existing_domains as $existing) { | ||
| if (self::is_main_domain($existing->get_domain())) { | ||
| /* | ||
| * Another custom domain is already primary for this blog. | ||
| * Do not auto-promote to avoid overriding explicit choice. | ||
| */ | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Existing primary check can miss custom subdomains and override explicit primary choice.
The guard only blocks when an existing primary passes is_main_domain(). A primary like www.example.com is a custom domain but won’t pass that check, so a later domain can still be auto-promoted unexpectedly.
💡 Suggested fix
- foreach ($existing_domains as $existing) {
- if (self::is_main_domain($existing->get_domain())) {
+ $network_domain = defined('DOMAIN_CURRENT_SITE') ? strtolower((string) DOMAIN_CURRENT_SITE) : '';
+ foreach ($existing_domains as $existing) {
+ $existing_domain = strtolower((string) $existing->get_domain());
+ $is_custom_domain = '' === $network_domain
+ ? self::is_main_domain($existing_domain)
+ : 0 === preg_match('/(^|\.)' . preg_quote($network_domain, '/') . '$/', $existing_domain);
+
+ if ($is_custom_domain) {
/*
* Another custom domain is already primary for this blog.
* Do not auto-promote to avoid overriding explicit choice.
*/
return;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@inc/managers/class-domain-manager.php` around lines 1151 - 1158, The current
guard only skips auto-promotion for domains where
self::is_main_domain($existing->get_domain()) is true, which misses custom
primary domains like "www.example.com"; replace that condition with a check that
detects any existing primary domain (not just main ones) — for example, check
the domain object's primary flag (e.g., $existing->is_primary() or
$existing->get_is_primary()) or compare $existing->get_domain() against the
blog's known primary domain via the manager API — and return early when any
existing domain is marked primary to avoid auto-promoting another domain.
|
Performance Test Results Performance test results for d46e177 are in 🛎️! Note: the numbers in parentheses show the difference to the previous (baseline) test run. Differences below 2% or 0.5 in absolute values are not shown. URL:
|
… updates
When external code (e.g. the WooCommerce addon's
sync_subscription_status) constructs a Site object from partial
data, the Base_Model constructor calls set_membership_id('') and
set_customer_id('') — writing empty values into $this->meta.
When save() then iterates $this->meta and writes to blogmeta,
the previously correct values (set at signup) get overwritten
with empties.
Real case: customer Alejandro Perretti (blog 306), paid manually
2026-04-28 11:17 UTC. WCS subscription flipped to active, addon's
sync handler ran, and wu_membership_id/wu_customer_id were wiped
from blogmeta.
The fix adds a guard: during updates (not new site creation),
skip writing wu_membership_id and wu_customer_id to blogmeta
when the value in $this->meta is empty. This prevents stale
empty defaults from overwriting correct values while still
allowing legitimate clears (which would go through the explicit
update_site_meta call in the 'Handles membership' section below
the loop).
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
🔨 Build Complete - Ready for Testing!📦 Download Build Artifact (Recommended)Download the zip build, upload to WordPress and test:
🌐 Test in WordPress Playground (Very Experimental)Click the link below to instantly test this PR in your browser - no installation needed! Login credentials: |
superdav42
added a commit
that referenced
this pull request
Apr 29, 2026
…part domains (#991) The wu_transition_domain_stage hook replaces the previous wu_domain_post_save hook, which was re-firing on every save (including async_remove_old_primary_domains clearing the flag), causing the domain to be immediately re-promoted. The is_main_domain() TLD-counting heuristic (2-part = custom, 3-part = subdomain) incorrectly blocked promotion of legitimate 3-part custom domains such as shop.mybrand.com or prosite.example.com. Replaced with an explicit check against the WP network host so only native WP multisite subdomains (site.kursopro.com when network is kursopro.com) are excluded. Fixes: checklist item 5 (primary_domain=1 auto on DNS verify) Refs: #965 (original implementation)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes three core-side issues reported by a customer running ~30 production subsites with the WooCommerce addon. These are defensive hardening on the core side; the primary addon-side fixes are tracked as separate issues on the addon repo.
Bug #3 — Limitations
enabledflag treats empty string as disabled (CRITICAL)Root cause:
Limit::setup()cast(bool) ''tofalsefor theenabledflag, treating empty-string as "explicitly disabled" instead of "not set / inherit from parent." Themerge_recursive()method then used a falsy check (! wu_get_isset(...)) which collapsed entire module data to{enabled: false}whenenabledwas''.Impact: 23 of 31 customer subsites had
domain_mapping.enabled = ''— Pro customers could not see the "Add Custom Domain" UI despite their plan having the feature enabled.Fix:
Limit::setup()— treat''forenabledthe same as'not-set', markinghas_own_enabled = falseso the default/inherited value takes overLimitations::merge_recursive()— use strictfalse ===check instead of falsy check when collapsing modulesFiles:
inc/limitations/class-limit.php,inc/objects/class-limitations.phpBug #1/#2 — Defensive membership inference when external gateway skips
set_membership_id()(CRITICAL)Root cause: The core
Site::save()flow writes$this->meta(includingwu_membership_id) to blogmeta, then callsget_membership()to setwu_customer_id. When an external gateway (WooCommerce addon) creates a site without callingset_membership_id()first,wu_membership_idis empty,get_membership()returns false,wu_customer_idis never written, and downstream code sees the site as unlinked.Fix: After
get_membership()returns false, if the site has acustomer_id, look up the customer's active memberships. If exactly one exists, auto-link it. Conservative — only acts when inference is unambiguous.File:
inc/models/class-site.phpBug #4 — Auto-promote custom domain to primary when stage reaches done (MEDIUM)
Root cause: When a custom domain (e.g.
elimartz.com) completes DNS/SSL and reachesdonestage, nothing promotes it to primary. Both the custom domain and default subdomain end up withprimary_domain = 0— no redirect, SEO duplicates.Fix: Hook into
wu_domain_post_saveand auto-promote when: (1) domain reached done/done-without-ssl, (2) it is a custom domain (not subdomain), (3) no other custom domain is already primary for the blog. Works regardless of whether stage was set by core or a third-party plugin.File:
inc/managers/class-domain-manager.phpVerification
vendor/bin/phpstan analyse— passes clean on all changed filesaidevops.sh v3.13.6 plugin for OpenCode v1.3.17 with gemma4:e4b spent 7d 22h and 43,427 tokens on this as a headless worker.
Summary by CodeRabbit
New Features
Bug Fixes