fix(site-exporter): support resilient large transfers - #1689
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change improves export download streaming, adds site-specific export views and download links, supports server-side ZIP imports, and resolves export and import commands dynamically when dependencies are unavailable. ChangesSite export flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Admin
participant SiteExporter
participant ExportFolder
participant ImportCommand
Admin->>SiteExporter: Select server-side ZIP
SiteExporter->>ExportFolder: Validate ZIP path
ExportFolder-->>SiteExporter: Return validated local file
SiteExporter->>ImportCommand: Process import metadata
ImportCommand-->>SiteExporter: Return import result
SiteExporter->>ExportFolder: Delete processed ZIP
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: |
|
Performance Test Results Performance test results for 603a292 are in 🛎️! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/site-exporter/class-export-download-handler.php`:
- Around line 223-242: Update the download method around the file-streaming
logic to open the file before sending the 200 response and Content-Length
headers, returning an appropriate failure response if fopen fails. In the fread
failure branch, log the read error before breaking from the partial stream,
while preserving the existing cleanup and connection-abort handling.
In `@inc/site-exporter/class-site-exporter.php`:
- Around line 2729-2738: In the export-command availability branch of
wu_exporter_export(), delete the wu_pending_site_export_{$hash} transient before
returning the export-dependency-missing WP_Error. Preserve the existing error
response and only clear the pending export when class_exists($command_class) is
false.
🪄 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 Plus
Run ID: 7a9baa59-0745-4d3c-941d-1c770c8128ab
📒 Files selected for processing (3)
inc/site-exporter/class-export-download-handler.phpinc/site-exporter/class-site-exporter.phptests/WP_Ultimo/Site_Exporter_Test.php
superdav42
changed the title
🔨 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: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/WP_Ultimo/Site_Exporter_Test.php (1)
338-358: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert that nested buffer contents are discarded.
test_export_download_handler_clears_nested_output_buffers()only checks the final buffer level. Add and assert on sentinel content; then verify that nested content does not reach the baseline buffer afterprepare_streaming_environment().🤖 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/Site_Exporter_Test.php` around lines 338 - 358, Update test_export_download_handler_clears_nested_output_buffers() to write distinct sentinel content into the nested output buffers before invoking prepare_streaming_environment(). Afterward, capture the baseline buffer contents and assert the sentinels are absent, while preserving the existing buffer-level assertion and cleanup.inc/site-exporter/class-site-exporter.php (2)
726-749: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter pending exports for site-scoped downloads.
render_export_import_dashboard()filters completed exports by$site_id, but the Pending Exports table still receives and renders allwu_exporter_get_pending()rows. Since pending site export entries store the target site ID as the first option, filter them by$site_idwhen it is set, or label the pending tables as network-wide activity.🤖 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/site-exporter/class-site-exporter.php` around lines 726 - 749, Update render_wp_export_page() to filter pending_exports by the requested site ID when $site_id is set, using the target site ID stored as the first option in each pending export entry, before passing the results to render_export_import_dashboard(). Preserve all pending entries when no site ID is provided.
3090-3104: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winDelete URL-based network-import ZIPs after import.
Network_Importer::import()can receivedelete_file, but it does not delete the ZIP or attachment.handle_network_import()only deletes whenserver_zip_fileresolves to the same server-side ZIP, leaving URL imports withremove_zipenabled with the source file intact. Add a fallback similar tohandle_site_import()that resolves$options['zip_url']to an attachment and deletes the attachment when no server-side ZIP path matches.🤖 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/site-exporter/class-site-exporter.php` around lines 3090 - 3104, Update handle_network_import() so that when delete_file is enabled and no matching server-side ZIP is deleted, resolve options['zip_url'] to its attachment and delete that attachment as the fallback, mirroring handle_site_import(). Preserve the existing server_zip_file path comparison and successful-import guard.
🧹 Nitpick comments (2)
tests/WP_Ultimo/Site_Exporter_Test.php (1)
331-332: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the site ID on the download link itself.
assertStringContainsString('site_id=1', $html)searches the entire HTML. The test can pass if the site ID appears in unrelated text or attributes while the anchor points to an unfiltered page.Inspect the rendered anchor and assert that its
hrefcontains the dedicated downloads route andsite_id=1.🤖 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/Site_Exporter_Test.php` around lines 331 - 332, Update the assertions in the site exporter test to inspect the rendered download anchor rather than searching the entire HTML; verify that the anchor’s href uses the dedicated downloads route and includes site_id=1, while preserving the existing “Open Downloads Page” assertion.inc/site-exporter/class-site-exporter.php (1)
2624-2700: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSolid path-traversal protection; consider validating inside
get_server_export_url()too.
get_server_export_path()at Lines 2667-2685 correctly layersbasename(),sanitize_file_name(), an extension check, and arealpath()prefix check to prevent path traversal. Every current caller invokesget_server_export_path()beforeget_server_export_url()and only uses the URL after the path check succeeds, so there is no exploitable path today.
get_server_export_url()(Lines 2694-2699) itself builds a URL directly from$file_namewithout re-validating it. If a future caller invokesget_server_export_url()on its own, an unsanitized$file_namewould still produce a URL string (thoughfile_exists()checks downstream would likely catch a wrong path before deletion). Add the same validation used inget_server_export_path()here, or haveget_server_export_url()callget_server_export_path()internally, so the guarantee holds independent of call order.🔒️ Proposed defensive fix
private function get_server_export_url(string $file_name): string { + if (! $this->get_server_export_path($file_name)) { + return ''; + } + $upload_dir = wp_upload_dir(); return trailingslashit($upload_dir['baseurl']) . 'wu-site-exports/' . $file_name; }🤖 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/site-exporter/class-site-exporter.php` around lines 2624 - 2700, Update get_server_export_url() to independently validate the supplied file name using the same basename, sanitize_file_name(), and .zip checks enforced by get_server_export_path(), or reuse get_server_export_path() before constructing the URL. Return an empty string or otherwise reject invalid selections, while preserving the existing canonical uploads URL for validated server-side ZIP names.
🤖 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 `@tests/WP_Ultimo/Site_Exporter_Test.php`:
- Around line 233-242: Update create_server_export_zip() to skip before
constructing ZipArchive when the class is unavailable, then require both
addFromString('manifest.json', '{}') and close() to return true before recording
the path in server_export_files. Skip the test when either operation fails so
only successfully created ZIP fixtures are accepted.
---
Outside diff comments:
In `@inc/site-exporter/class-site-exporter.php`:
- Around line 726-749: Update render_wp_export_page() to filter pending_exports
by the requested site ID when $site_id is set, using the target site ID stored
as the first option in each pending export entry, before passing the results to
render_export_import_dashboard(). Preserve all pending entries when no site ID
is provided.
- Around line 3090-3104: Update handle_network_import() so that when delete_file
is enabled and no matching server-side ZIP is deleted, resolve
options['zip_url'] to its attachment and delete that attachment as the fallback,
mirroring handle_site_import(). Preserve the existing server_zip_file path
comparison and successful-import guard.
In `@tests/WP_Ultimo/Site_Exporter_Test.php`:
- Around line 338-358: Update
test_export_download_handler_clears_nested_output_buffers() to write distinct
sentinel content into the nested output buffers before invoking
prepare_streaming_environment(). Afterward, capture the baseline buffer contents
and assert the sentinels are absent, while preserving the existing buffer-level
assertion and cleanup.
---
Nitpick comments:
In `@inc/site-exporter/class-site-exporter.php`:
- Around line 2624-2700: Update get_server_export_url() to independently
validate the supplied file name using the same basename, sanitize_file_name(),
and .zip checks enforced by get_server_export_path(), or reuse
get_server_export_path() before constructing the URL. Return an empty string or
otherwise reject invalid selections, while preserving the existing canonical
uploads URL for validated server-side ZIP names.
In `@tests/WP_Ultimo/Site_Exporter_Test.php`:
- Around line 331-332: Update the assertions in the site exporter test to
inspect the rendered download anchor rather than searching the entire HTML;
verify that the anchor’s href uses the dedicated downloads route and includes
site_id=1, while preserving the existing “Open Downloads Page” assertion.
🪄 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 Plus
Run ID: d9862e20-fa47-4082-bdbc-c406af95a5ee
📒 Files selected for processing (2)
inc/site-exporter/class-site-exporter.phptests/WP_Ultimo/Site_Exporter_Test.php
| $zip = new \ZipArchive(); | ||
|
|
||
| if (true !== $zip->open($file_path, \ZipArchive::CREATE)) { | ||
| $this->markTestSkipped('Unable to create a server-side ZIP fixture'); | ||
| } | ||
|
|
||
| $zip->addFromString('manifest.json', '{}'); | ||
| $zip->close(); | ||
|
|
||
| $this->server_export_files[] = $file_path; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
php -r 'exit(class_exists("ZipArchive") ? 0 : 1);'Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- candidate file ---\n'
fd 'Site_Exporter_Test\.php$' . || true
printf '\n--- relevant source lines ---\n'
file="$(fd 'Site_Exporter_Test\.php$' . | head -n1 || true)"
if [ -n "$file" ]; then
sed -n '200,260p' "$file" | nl -ba -v200
fi
printf '\n--- server_export_files usages ---\n'
rg -n "server_export_files|addFromString|close\(\)" tests/WP_Ultimo/Site_Exporter_Test.php || true
printf '\n--- PHP ZipArchive return values probe ---\n'
tmp="$(mktemp)"
php - <<'PHP' "$tmp"
$zip = new ZipArchive();
$file = $argv[1];
echo "open_create=" . var_export($zip->open($file, ZipArchive::CREATE), true) . "\n";
echo "add_empty=" . var_export($zip->addFromString('manifest.json', '{}'), true) . "\n";
echo "close_after_empty=" . var_export($zip->close(), true) . "\n";
if (file_exists($file)) {
`@unlink`($file);
}
echo "file_after_exists=" . var_export(file_exists($file), true) . "\n";
echo "file_after_is=" . var_export(is_file($file), true) . "\n";
PHPRepository: Ultimate-Multisite/ultimate-multisite
Length of output: 266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate file ---'
fd 'Site_Exporter_Test\.php$' . || true
printf '%s\n' '--- relevant source lines ---'
file="$(fd 'Site_Exporter_Test\.php$' . | head -n1 || true)"
if [ -n "$file" ]; then
sed -n '200,260p' "$file" | nl -ba -v200
fi
printf '%s\n' '--- server_export_files usages ---'
if [ -n "$file" ]; then
rg -n "server_export_files|addFromString|close\(\)" "$file" || true
fi
printf '%s\n' '--- PHP ZipArchive return values probe ---'
tmp="$(mktemp)"
php - <<'PHP' "$tmp"
$zip = new ZipArchive();
$file = $argv[1];
echo "open_create=" . var_export($zip->open($file, ZipArchive::CREATE), true) . "\n";
echo "add_empty=" . var_export($zip->addFromString('manifest.json', '{}'), true) . "\n";
echo "close_after_empty=" . var_export($zip->close(), true) . "\n";
if (file_exists($file)) {
`@unlink`($file);
}
echo "file_after_exists=" . var_export(file_exists($file), true) . "\n";
echo "is_file_after=" . var_export(is_file($file), true) . "\n";
PHP
rm -f "$tmp"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 310
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant source lines ---'
sed -n '200,260p' './tests/WP_Ultimo/Site_Exporter_Test.php'
printf '%s\n' '--- server_export_files usages ---'
rg -n "server_export_files|addFromString|close\(\)" './tests/WP_Ultimo/Site_Exporter_Test.php' || true
printf '%s\n' '--- PHP ZipArchive return values probe ---'
tmp="$(mktemp)"
php - <<'PHP' "$tmp"
$zip = new ZipArchive();
$file = $argv[1];
echo "open_create=" . var_export($zip->open($file, ZipArchive::CREATE), true) . "\n";
echo "add_empty=" . var_export($zip->addFromString('manifest.json', '{}'), true) . "\n";
echo "close_after_empty=" . var_export($zip->close(), true) . "\n";
if (file_exists($file)) {
`@unlink`($file);
}
echo "file_after_exists=" . var_export(file_exists($file), true) . "\n";
echo "is_file_after=" . var_export(is_file($file), true) . "\n";
PHP
rm -f "$tmp"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 2189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- teardown cleanup lines ---'
sed -n '55,80p' './tests/WP_Ultimo/Site_Exporter_Test.php'
printf '%s\n' '--- ZIP API return values ---'
tmp="$(mktemp)"
php - <<'PHP' "$tmp" || :
<?php
$zip = new ZipArchive();
echo "class_exists=" . var_export(class_exists('ZipArchive'), true) . "\n";
echo "open_create=" . var_export($zip->open($tmp, ZipArchive::CREATE), true) . "\n";
echo "add_empty=" . var_export($zip->addFromString('manifest.json', '{}'), true) . "\n";
echo "close_after_empty=" . var_export($zip->close(), true) . "\n";
echo "file_after_exists=" . var_export(file_exists($tmp), true) . "\n";
echo "is_file_after=" . var_export(is_file($tmp), true) . "\n";
if (file_exists($tmp)) {
unlink($tmp);
}
PHP
echo "exit=$?"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PHP ZipArchive return values probe ---'
tmp="$(mktemp)"
php -s <<'PHP'
<?php
$zip = new ZipArchive();
echo "class_exists=" . var_export(class_exists('ZipArchive'), true) . "\n";
echo "open_create=" . var_export($zip->open($argv[1], ZipArchive::CREATE), true) . "\n";
echo "add_empty=" . var_export($zip->addFromString('manifest.json', '{}'), true) . "\n";
echo "close_after_empty=" . var_export($zip->close(), true) . "\n";
echo "file_after_exists=" . var_export(file_exists($argv[1]), true) . "\n";
echo "is_file_after=" . var_export(is_file($argv[1]), true) . "\n";
if (file_exists($argv[1])) {
unlink($argv[1]);
}
PHP
"$tmp"
rm -f "$tmp"Repository: Ultimate-Multisite/ultimate-multisite
Length of output: 4629
Check ZipArchive fixture creation outcomes before accepting the archive.
create_server_export_zip() records the file path only after calling ZipArchive::addFromString() and ZipArchive::close(), and both results are ignored. A failed write or close can still return the ZIP path, making later file-based assertions unreliable.
If ext-zip is required by these tests, assert both operations return true; otherwise skip before instantiating ZipArchive if the class is unavailable.
🤖 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/Site_Exporter_Test.php` around lines 233 - 242, Update
create_server_export_zip() to skip before constructing ZipArchive when the class
is unavailable, then require both addFromString('manifest.json', '{}') and
close() to return true before recording the path in server_export_files. Skip
the test when either operation fails so only successfully created ZIP fixtures
are accepted.
🔨 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: |
Rebase needed — PR has merge conflicts and no
|
|
Maintainer review needed: this PR is Reason: Options:
This comment is posted once per cooldown window (1800s) so the sweep stays quiet. Triggered by aidevops.sh v3.32.210 automated scan. |
Rebase needed — branch has diverged from
|
1 participant
Summary
HEADexport-download requests without streaming the archive body.Server-side import workflow
Network administrators can place large ZIP archives in
wp-content/uploads/wu-site-exports/and select them from the import modal.realpath()and rejects traversal or paths outside that folder.Files changed
inc/site-exporter/class-export-download-handler.php: safe streaming andHEADresponse handling.inc/site-exporter/class-site-exporter.php: download-page navigation, exact export filtering, server-side import selection and validation.tests/WP_Ultimo/Site_Exporter_Test.php: export-download and server-side ZIP selection regression coverage.Verification
php -l inc/site-exporter/class-site-exporter.php && php -l tests/WP_Ultimo/Site_Exporter_Test.phpvendor/bin/phpcs inc/site-exporter/class-site-exporter.php tests/WP_Ultimo/Site_Exporter_Test.phpvendor/bin/phpstan analyse inc/site-exporter/class-site-exporter.php --memory-limit=1Gvendor/bin/phpunit --filter Site_Exporter_Test— 28 tests, 76 assertionsSummary by CodeRabbit
New Features
Bug Fixes
Tests