Skip to content

Hardening: Site_Duplicator::override_site() should validate source and destination site ids #1115

@superdav42

Description

@superdav42

Summary

Site_Duplicator::override_site($from_site_id, $to_site_id) (inc/helpers/class-site-duplicator.php) does not validate that the destination site id passed in actually corresponds to a real site. While debugging the customer-panel template-switching flow we observed a call with a non-existent destination id (999999) returning a successful-looking result — the duplication mechanics ran without short-circuiting on the missing target.

That is a hardening defect rather than a customer-visible regression: the actual call sites in core (the customer-panel switching element, the admin template-switching tools) all derive $to_site_id from a real Site object, so a bad id never reaches the helper in production. But it makes the helper unsafe to expose to addons or to the REST API surface, and it produces confusing log entries when something further up the call stack does pass a stale id.

Why a separate issue

#1113 fixed the customer-visible template-switching bugs. The override_site helper hardening is a separate code-quality concern with its own test plan. Bundling it would have made #1113 less reviewable.

Files to inspect

  • inc/helpers/class-site-duplicator.phpoverride_site($from_site_id, $to_site_id) at the function signature; entry point at line ~94.
  • inc/ui/class-template-switching-element.phpswitch_template() at line ~305, the production caller.
  • inc/admin-pages/site-edit-admin-page.php (or similar admin path) — secondary admin-side caller for cross-reference.

Symptom

While reproducing the template-switching flow as a real customer, an earlier debug invocation Site_Duplicator::override_site($template_id, 999999) ran to completion and wrote the misleading log entry:

Attempt to override site 999999 with data from site 5 successful

The log wording is itself misleading (from/to are flipped in the message text relative to the parameter order — see follow-up below) but the more important defect is that the helper accepted a destination id that resolved to no row in wp_blogs.

Proposed fix

  1. Validate $to_site_id resolves to an existing Site (or WP_Site / blog row) at the top of override_site(), return false and log a WP_Error-equivalent if not.
  2. Validate $from_site_id likewise — the helper should not silently no-op if the source template was deleted between the customer's grid render and their click.
  3. Fix the misleading log wording so from and to in the message match the parameter order. A separate one-line change but bundles naturally with the validation work.
  4. Add a unit test covering the three failure modes (missing source, missing destination, valid both) so future regressions are caught.

Verification

  • After the fix, calling Site_Duplicator::override_site($real_template_id, 999999) should return false and log a clear "destination site not found" message.
  • Calling Site_Duplicator::override_site(999999, $real_site_id) should return false and log a clear "source template not found" message.
  • The customer-panel template-switching flow continues to work for real customers (regression check).

Severity

Low. No customer-visible impact. Hardening + log clarity only.

Metadata

Metadata

Assignees

Labels

bugSomething isn't workingorigin:workerAuto-created by pulse labelless backfill (t2112)priority:mediumMedium severity — moderate quality issuestatus:in-reviewPR open, awaiting review/merge

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions