fix: preserve incomplete objects during duplication#1419
Conversation
📝 WalkthroughWalkthrough
ChangesIncomplete Class Serialization Handling
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
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. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/WP_Ultimo/Duplication/MUCD_Data_Test.php (1)
305-328: ⚡ Quick winAdd coverage for the double-serialized incomplete-object branch.
Line 312 validates single-serialized incomplete objects;
try_replace()also has explicit double-serialized handling. Add one companion assertion usingserialize($serialized)so that branch stays protected from regressions.🧪 Suggested test addition
+ public function test_try_replace_preserves_double_serialized_incomplete_object() { + $class_name = 'Missing\\Plugin\\Notification'; + $old_url = 'https://example.com/old/page'; + $inner = sprintf( + 'O:%d:"%s":1:{s:3:"url";s:%d:"%s";}', + strlen($class_name), + $class_name, + strlen($old_url), + $old_url + ); + $outer = serialize($inner); + $row = ['meta_value' => $outer]; + + $result = \MUCD_Data::try_replace($row, 'meta_value', 'example.com/old', 'example.com/new'); + + $this->assertSame($outer, $result); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/WP_Ultimo/Duplication/MUCD_Data_Test.php` around lines 305 - 328, Add a companion assertion to the test_try_replace_preserves_incomplete_serialized_object that covers the double-serialized incomplete-object branch: after building $serialized and calling MUCD_Data::try_replace($row, 'meta_value', 'example.com/old', 'example.com/new'), also call try_replace with ['meta_value' => serialize($serialized)] (i.e. serialize($serialized)) and assert the result equals serialize($serialized) and still contains the original $old_url; this ensures MUCD_Data::try_replace's double-serialized handling is exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@inc/duplication/data.php`:
- Around line 451-454: In try_replace, harden the unsafe
`@unserialize`($row[$field]) and the inner unserialize call by disabling object
instantiation and removing the error-suppression operator: call
unserialize($row[$field], ['allowed_classes' => false]) (and the same for the
inner_unserialized call), check the return value explicitly (is_array/is_scalar)
before using it, and fall back to $original_value when unserialize returns false
or an unexpected type; also remove the "@" to avoid hiding errors so failures
are handled deterministically.
---
Nitpick comments:
In `@tests/WP_Ultimo/Duplication/MUCD_Data_Test.php`:
- Around line 305-328: Add a companion assertion to the
test_try_replace_preserves_incomplete_serialized_object that covers the
double-serialized incomplete-object branch: after building $serialized and
calling MUCD_Data::try_replace($row, 'meta_value', 'example.com/old',
'example.com/new'), also call try_replace with ['meta_value' =>
serialize($serialized)] (i.e. serialize($serialized)) and assert the result
equals serialize($serialized) and still contains the original $old_url; this
ensures MUCD_Data::try_replace's double-serialized handling is exercised.
🪄 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: bf7bdacd-0e99-4960-be53-59c006b27fb3
📒 Files selected for processing (2)
inc/duplication/data.phptests/WP_Ultimo/Duplication/MUCD_Data_Test.php
| $double_serialize = false; | ||
| $original_value = $row[ $field ]; | ||
| $row[ $field ] = @unserialize($row[ $field ]); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Read-only verification: inspect deserialization callsites and data-flow entry points.
rg -n "unserialize\\(" inc/duplication/data.php
rg -n "db_update_data\\(|update\\(|try_replace\\(" inc/duplication/data.php -C3
rg -n "get_fields_to_update|get_fields_to_update\\(" --type=php -C3Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 3620
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the deserialization logic around the confirmed callsites
sed -n '430,520p' inc/duplication/data.php
# Look for incomplete-class handling and object branch logic
rg -n "__PHP_Incomplete_Class|incomplete|allowed_classes|is_object|return .*object|get_class\\(" inc/duplication/data.phpRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 3055
Harden unserialize to prevent PHP object injection during duplication.
inc/duplication/data.php (try_replace) uses @unserialize($row[$field]) (and again for $inner_unserialized) with default class instantiation enabled. Even though there’s an __PHP_Incomplete_Class guard that returns $original_value, the unsafe instantiation (and potential gadget execution via magic methods) can already occur before that check.
🔧 Suggested hardening
public static function try_replace($row, $field, $from_string, $to_string) {
if (is_serialized($row[ $field ])) {
$double_serialize = false;
$original_value = $row[ $field ];
- $row[ $field ] = `@unserialize`($row[ $field ]);
+ $allowed_classes = apply_filters('wu_duplication_allowed_unserialize_classes', false);
+ $row[ $field ] = `@unserialize`($row[ $field ], [ 'allowed_classes' => $allowed_classes ]);
@@
if (is_serialized($row[ $field ])) {
- $inner_unserialized = `@unserialize`($row[ $field ]);
+ $inner_unserialized = `@unserialize`($row[ $field ], [ 'allowed_classes' => $allowed_classes ]);🧰 Tools
🪛 GitHub Check: Code Quality Checks
[warning] 453-453:
unserialize() found. Serialized data has known vulnerability problems with Object Injection. JSON is generally a better approach for serializing data. See https://www.owasp.org/index.php/PHP_Object_Injection
[warning] 453-453:
Silencing errors is strongly discouraged. Use proper error checking instead. Found: @unserialize($row[ ...
🪛 OpenGrep (1.22.0)
[ERROR] 453-453: unserialize() with untrusted input can lead to remote code execution via PHP object injection. Use json_decode() or set allowed_classes to false.
(coderabbit.deserialization.php-unserialize)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@inc/duplication/data.php` around lines 451 - 454, In try_replace, harden the
unsafe `@unserialize`($row[$field]) and the inner unserialize call by disabling
object instantiation and removing the error-suppression operator: call
unserialize($row[$field], ['allowed_classes' => false]) (and the same for the
inner_unserialized call), check the return value explicitly (is_array/is_scalar)
before using it, and fall back to $original_value when unserialize returns false
or an unexpected type; also remove the "@" to avoid hiding errors so failures
are handled deterministically.
Source: Linters/SAST tools
There was a problem hiding this comment.
@coderabbitai Verified.
The premise is still valid in the merged PR code: inc/duplication/data.php:453 and inc/duplication/data.php:463 both call @unserialize(...) without allowed_classes, before the incomplete-class guard at inc/duplication/data.php:474 can return the original value.
Evidence: git merge-base --is-ancestor HEAD origin/main succeeded and git diff --name-only HEAD..origin/main -- inc/duplication/data.php tests/WP_Ultimo/Duplication/MUCD_Data_Test.php returned no file changes, so the affected code is now in origin/main. I also ran php -l inc/duplication/data.php and php -l tests/WP_Ultimo/Duplication/MUCD_Data_Test.php successfully.
Because PR #1419 is already merged, I am not pushing to the merged PR branch and I am not resolving this thread. This needs a follow-up fix on main that disables object instantiation for both unserialize calls while preserving the incomplete-object fallback behavior.
There was a problem hiding this comment.
@superdav42, thanks for verifying and providing the evidence. The concern is confirmed valid — the two @unserialize() calls at lines 453 and 463 can still instantiate objects (triggering __wakeup, __destruct, or autoloaders) before the __PHP_Incomplete_Class guard at line 474 can fire.
Here's the proposed follow-up fix:
- Harden both
unserialize()calls intry_replace()with['allowed_classes' => false] - Remove the
@error-suppression operator (failures are already handled by the explicitfalse ===checks) - Add a filter
wu_duplication_allowed_unserialize_classesfor extensibility (defaults tofalse)
Would you also like me to open a GitHub issue on main to track this follow-up fix?
🧠 Learnings used
Learnt from: superdav42
Repo: Ultimate-Multisite/ultimate-multisite PR: 1410
File: inc/class-requirements.php:39-39
Timestamp: 2026-06-12T00:44:09.258Z
Learning: Ultimate Multisite (Ultimate-Multisite/ultimate-multisite) supports PHP 8.2+ as the minimum supported PHP version (per PR `#1410`). During code review, do not flag code that uses PHP 8.0+ built-in functions or PHP 8+ language features as “compatibility” concerns—for example: str_ends_with(), str_starts_with(), str_contains(), named arguments, and match expressions—since they are natively available without polyfills.
The requested changes have been implemented and a pull request has been created: View PR
superdav42
added
the
review-feedback-scanned
1 participant
Summary
__PHP_Incomplete_Classoption/meta values during site duplication instead of trying to mutate their propertiesMUCD_Data::try_replace()Why
Verification
php -l inc/duplication/data.phpphp -l tests/WP_Ultimo/Duplication/MUCD_Data_Test.phppreservedgit diff --checkNotes
vendor/bin/phpunit --filter MUCD_Data_Testis blocked locally by missing/tmp/wordpress-tests-lib/includes/functions.phpaidevops.sh v3.20.58 plugin for OpenCode v1.17.4 with gpt-5.5 spent 21h 30m and 2,546,286 tokens on this with the user in an interactive session.
Summary by CodeRabbit
Bug Fixes
Tests