Skip to content

feat(flash-backup): stream boot device backup download with selectable compression#2677

Open
elibosley wants to merge 13 commits into
masterfrom
feat/flash-backup-background
Open

feat(flash-backup): stream boot device backup download with selectable compression#2677
elibosley wants to merge 13 commits into
masterfrom
feat/flash-backup-background

Conversation

@elibosley

@elibosley elibosley commented Jun 22, 2026

Copy link
Copy Markdown
Member

Summary

Rework the manual Boot device backup (Main → boot device → Boot Device): no longer synchronous in the request, with a selectable mode, compression level, and a live progress bar.

  • Download to your computer — streams the archive straight to the browser as it is built (include/FlashBackup.php); nothing staged on the server, no size/FAT limit, works with the array stopped.
  • Save to the server — a tracked background job (scripts/flash_backup_save via StartCommand.php) writes the archive to a persistent disk target (cache/SSD pool → array share → boot pool) and keeps it. On completion it raises an Unraid notification (the tray bell) with a click-to-download link, and the GUI shows a Download button next to the saved path. Runs to completion even if you close the dialog or navigate away — ideal when the link to the server is slow.

Both modes share one progress bar fed by the flash_backup nchan channel.

Notable implementation points

  • Compression dropdown: None/Low/Normal/Maximum → zip 0/1/6/9.
  • Fast: one zip -qr - pass piped to its destination (the download client, or the save file) instead of per-item appends — save dropped from ~95 s to ~8 s on a test box.
  • Progress without a Content-Length: the server reports bytes/percent over nchan, tagged with a per-run token so a stale message nchan retained from a previous run can't make a fresh run jump to 100%. One subscriber for the page lifetime (no per-click start/stop race).
  • Download trigger uses a hidden iframe, not window.location (a top-level navigation fires beforeunload, which makes NchanSubscriber stop itself and freezes the bar).
  • Excludes the prior OS release (previous/prev, often >1 GB) and desktop junk.

Files

  • emhttp/plugins/dynamix/include/FlashBackup.php (new) — streaming download endpoint
  • emhttp/plugins/dynamix/scripts/flash_backup — streams the zip to stdout (+ size mode)
  • emhttp/plugins/dynamix/scripts/flash_backup_save (new) — background save-to-disk job + tray notification
  • emhttp/plugins/dynamix/BootInfo.page — mode + compression selectors, progress bar, download button
  • emhttp/plugins/dynamix/include/Download.php — drop the old synchronous cmd=backup branch

Testing

Verified on a live 7.3.2 server:

  • Save: 8 s to write a valid 2.5 GB archive to the pool (level 0), file kept, integrity OK, token-tagged progress published, tray notification raised (confirmed via notify get), download symlink created.
  • Download: streams a valid archive; progress published; size mode reports total.
  • Progress bar confirmed updating in-browser (publishing test values drove it to 77%).

Note on this dev box: it sits on a different subnet reached via NAT (~0.8 MB/s even wired), so the download mode is slow there — that's the network path, not the code (a static file over the same link is just as slow). Save-to-server (local disk speed) is the right tool in that situation.

🤖 Generated with Claude Code

Run the boot device (USB flash) backup as a detached background task
instead of blocking the HTTP request for the entire zip creation.

- Add a compression-level selector (None/Low/Normal/Maximum) on the Boot
  Device page; the chosen zip level (0/1/6/9) is passed to flash_backup.
- Stream live progress over a new 'flash_backup' nchan channel and trigger
  the download automatically on completion (mirrors the Diagnostics flow).
- Run zip under nice/ionice so the backup stays gentle on CPU and disk.
- Prefer a real disk (array/pool share) over the RAM-backed rootfs for the
  archive so a large backup does not consume system memory; fall back to /
  only when no share has room.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Boot device backup is converted from disk staging to direct browser streaming with real-time progress updates. A new FlashBackup.php endpoint calculates expected size, streams the ZIP archive, and publishes progress percentages via nchan. The flash_backup script outputs ZIP to stdout and supports a size-calculation mode for progress tracking. BootInfo.page adds a compression-level dropdown and subscribes to nchan for live progress updates instead of polling. Download.php removes legacy backup handling, and help text describes the streaming behavior and compression options.

Changes

Boot Backup Streaming Architecture

Layer / File(s) Summary
flash_backup: stdout streaming with size-calculation mode
emhttp/plugins/dynamix/scripts/flash_backup
Updates copyright to 2025 and refactors to read/clamp a compression-level CLI argument (0–9, default 6), build an exclusion list for prior-release/junk directories and stray backup zips, enumerate top-level /boot entries, and implement a size mode that computes total bytes via du for progress tracking. Archive creation invokes zip under nice/ionice with stdout output, exiting with zip's return code.
FlashBackup.php: streaming endpoint with size pre-calculation and nchan progress
emhttp/plugins/dynamix/include/FlashBackup.php
New endpoint reads level (compression 0–9, default 6) from query parameters, generates a timestamped ZIP filename from /var/local/emhttp/var.ini, pre-calculates expected size by invoking flash_backup size, sets HTTP streaming headers (ZIP content-type, disposition, no nginx buffering/caching), clears buffers, executes flash_backup via popen, streams output chunk-by-chunk with immediate flush, periodically publishes progress percentages to the flash_backup nchan channel, and publishes a _DONE_ marker upon completion.
BootInfo.page: compression selector and nchan progress subscription
emhttp/plugins/dynamix/BootInfo.page
Adds a select#backupLevel dropdown with compression options 0, 1, 6 (pre-selected), and 9. The backup() function reads selection, shows a SweetAlert with an embedded progress bar, subscribes to /sub/flash_backup via nchan to receive real-time updates, updates progress bar and percentage on numeric messages, and on _DONE_ message sets progress to 100%, stops subscription, and closes the alert after a short delay. Redirects to the streaming endpoint with the selected compression level.
Help text and Download.php cleanup
emhttp/languages/en_US/helptext.txt, emhttp/plugins/dynamix/include/Download.php
Help text for flash_backup_help expands to describe background streaming, direct browser download (nothing stored on server), progress reporting, auto-download on completion, and compression-level selection guidance (excluding prior OS and non-essential files). Download.php removes its case 'backup' branch from switch ($_POST['cmd']).

Sequence Diagram

sequenceDiagram
  participant User
  participant BootInfo as BootInfo.page
  participant FlashBackup as FlashBackup.php endpoint
  participant flashBackup as flash_backup script
  participant nchan as nchan channel
  participant Browser

  User->>BootInfo: Click Backup, select compression level
  BootInfo->>BootInfo: show SweetAlert with progress bar
  BootInfo->>nchan: subscribe to /sub/flash_backup
  BootInfo->>Browser: location = /webGui/include/FlashBackup.php?level=N
  Browser->>FlashBackup: GET request with level parameter
  FlashBackup->>flashBackup: invoke "flash_backup size"
  flashBackup-->>FlashBackup: expected total bytes
  FlashBackup->>FlashBackup: set ZIP headers, disable buffering
  FlashBackup->>flashBackup: popen "flash_backup level"
  flashBackup->>Browser: stream ZIP chunks to stdout
  loop Every chunk streamed
    FlashBackup->>FlashBackup: calculate sent/total %
    FlashBackup->>nchan: publish numeric progress
    nchan->>BootInfo: progress message
    BootInfo->>BootInfo: update `#fbBar/`#fbPct
  end
  FlashBackup->>nchan: publish _DONE_
  nchan->>BootInfo: _DONE_ message
  BootInfo->>BootInfo: set progress 100%, close alert
  Browser->>Browser: auto-download ZIP file

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐇 A backup that streams, no staging in sight,
The browser grabs ZIPs as they flow through the night.
Pick compression wisely, from None down to max,
Real-time progress dancing—the nchan way tracks.
No cookies to poll, no files on the shelf,
The boot backup bunny delivers itself! 📦✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main feature: converting boot device backup to streaming with selectable compression levels, which aligns with the core changes across all modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/flash-backup-background

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown

🔧 PR Test Plugin Available

A test plugin has been generated for this PR that includes the modified files.

Version: 2026.06.23.2013
Build: View Workflow Run

📥 Installation Instructions:

Install via Unraid Web UI:

  1. Go to Plugins → Install Plugin
  2. Copy and paste this URL:
https://preview.dl.unraid.net/pr-plugins/pr-2677/webgui-pr-2677.plg
  1. Click Install

Alternative: Direct Download

⚠️ Important Notes:

  • Testing only: This plugin is for testing PR changes
  • Backup included: Original files are automatically backed up
  • Easy removal: Files are restored when plugin is removed
  • Conflicts: Remove this plugin before installing production updates
  • Post-merge behavior: This preview stays available after merge until preview storage expires or it is manually cleaned up

📝 Modified Files:

Click to expand file list
emhttp/plugins/dynamix/BootInfo.page
emhttp/plugins/dynamix/include/Download.php
emhttp/plugins/dynamix/include/FlashBackup.php
emhttp/plugins/dynamix/scripts/flash_backup
emhttp/plugins/dynamix/scripts/flash_backup_save

🔄 To Remove:

Navigate to Plugins → Installed Plugins and remove webgui-pr-2677, or run:

plugin remove webgui-pr-2677

🤖 This comment is automatically generated and will be updated with each new push to this PR.

elibosley and others added 2 commits June 22, 2026 16:32
…d nchan window

Switch the boot backup from a bespoke modal to the same background-job
mechanism plugin/docker operations use:

- Launch via StartCommand.php (tracked PID, run de-duplication, abortable)
  instead of a one-off nohup in Download.php.
- Stream into the standard progress window (pre#swaltext / pluginProgressTitle)
  and reuse the shared openDone()/openError() sentinel handlers; the script now
  emits the bare _DONE_/_ERROR_ sentinels they expect.
- Keep the _FILE_<name> message so the GUI still auto-downloads the archive on
  completion, then cleans it up.
- Drop the now-unused cmd=backup branch from Download.php.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rst target

Issues found by running the job end-to-end on a live server:

- Fatal "Call to undefined function _()": the job runs under php-cli (launched
  via StartCommand.php) where the GUI translation layer isn't loaded, so every
  _() call aborted the script immediately. Add a no-op _() fallback like other
  CLI scripts (monitor, InstallKey). This broke the feature entirely.
- ionice -c3 (idle) could starve throughput to a crawl; use best-effort lowest
  priority (-c2 -n7) instead. (Note: ionice is a no-op on ZFS, which has its own
  scheduler.)
- Storage target now prefers a cache/SSD pool, then RAM, then an array share -
  and writes straight to the pool mount, bypassing the slower /mnt/user FUSE
  layer. The previous disk-share-first choice routed through FUSE onto array/ZFS
  and was very slow; this keeps it fast on typical systems while still avoiding
  a large archive in RAM unless nothing else fits.

Verified on a live 7.3.2 box: archive builds, integrity OK, prev/previous
excluded, progress streams over nchan, _FILE_/_DONE_ drive the download.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@emhttp/plugins/dynamix/BootInfo.page`:
- Around line 29-32: The nchan_backup.stop() method is called multiple times in
the BootInfo.page file, which causes errors when attempting to stop an
already-stopped subscriber. Create a helper function that guards the stop call
by checking if nchan_backup exists and is in a running state before calling
stop(), then replace all three instances of nchan_backup.stop() calls (at the
locations where openError(data) is checked, openDone(data) is checked, and in
the SweetAlert dialog close callback) with this new helper function to prevent
attempting to stop an already-stopped subscriber.
- Around line 39-41: The box element is using `.html()` method to inject data
that comes from archive entry names (externally-derived content), which creates
a vulnerability to HTML/markup injection. Replace the `.html()` method calls in
line 40 with `.text()` method to safely escape the injected data. Since the
content is in a `<pre>` tag, `.text()` will preserve line breaks naturally
without needing the explicit `<br>` tag, while protecting against any malicious
markup in filenames.

In `@emhttp/plugins/dynamix/scripts/flash_backup`:
- Around line 124-128: The symlink() function call that creates the archive link
does not check for success before publishing the completion markers _FILE_ and
_DONE_. Since PHP's symlink() returns false when the destination already exists,
the code currently reports success even when the symlink creation fails. Add a
return value check on the symlink() call and implement error handling before the
write() calls for _FILE_ and _DONE_, following the same error handling pattern
already established earlier in the script (similar to lines 117-121), so that
failures are properly reported instead of silently continuing.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: c3dbb551-7813-45c1-8ada-abbf4bfcab49

📥 Commits

Reviewing files that changed from the base of the PR and between 644ab82 and d3f4a7f.

📒 Files selected for processing (4)
  • emhttp/languages/en_US/helptext.txt
  • emhttp/plugins/dynamix/BootInfo.page
  • emhttp/plugins/dynamix/include/Download.php
  • emhttp/plugins/dynamix/scripts/flash_backup
💤 Files with no reviewable changes (1)
  • emhttp/plugins/dynamix/include/Download.php

Comment thread emhttp/plugins/dynamix/BootInfo.page Outdated
Comment thread emhttp/plugins/dynamix/BootInfo.page Outdated
Comment thread emhttp/plugins/dynamix/scripts/flash_backup Outdated
elibosley and others added 3 commits June 23, 2026 05:46
…ray stopped

The backup archive is a throwaway staging file (built, downloaded, deleted), so
the destination should be the fastest always-available location. Restore the
original RAM-first behavior: rootfs (always mounted, even with the array
stopped) -> cache/SSD pool -> array share. Pools only mount with the array, so
pool-first failed to deliver a backup when the array was stopped.

Disk tiers are kept only for flashes too large to stage in RAM. Process memory
stays low regardless via the streaming zip binary.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… excludes

- Add the boot device's own pool as a last-resort staging target. /boot is
  always mounted (even with the array stopped), so this guarantees a target when
  RAM is full and no pool/array share is available. Gated to real pool
  filesystems (zfs/btrfs/xfs) so a FAT/USB flash - which is not a pool and can't
  hold a >4GB file - is never chosen.
- Reap any leftover staging archive at the start of each run (readlink the
  docroot symlink + delete its target), so a copy can't linger - especially in
  RAM - if the browser-side cleanup didn't fire (e.g. tab closed mid-download).
  The normal post-download Download.php cmd=unlink still runs.
- Exclude more: prior-release dirs already skipped (previous/prev, ~1GB+), now
  also common desktop junk (System Volume Information, .Trashes, .fseventsd,
  .Spotlight-V100, .TemporaryItems) and any stray *-boot-backup-*.zip a user
  left on the flash.

Verified on a live ZFS box: previous/ (1.1G) excluded, reaper removes a seeded
stale artifact, archive integrity OK.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the staged background-job model with direct streaming: the zip is piped
straight to the browser as it is built, so nothing is ever written to disk or
RAM on the server.

- scripts/flash_backup: now writes the zip to stdout (`zip -qr - ...`), usable
  both from the new endpoint and the CLI (`flash_backup 6 > out.zip`). No target
  selection, no nchan, no _() — just the exclusions and a single streaming zip.
- include/FlashBackup.php: new download endpoint. Sets the attachment headers and
  `X-Accel-Buffering: no` (so nginx streams instead of spooling to a temp file),
  echoes the request's download token back as a cookie so the GUI can tell the
  stream started, then passes the script through.
- BootInfo.page: clicking backup now navigates to the endpoint (browser handles
  the download, page isn't blocked) and shows a light "preparing…" status that
  clears as soon as the cookie appears. Removed the nchan/StartCommand path.

Benefits: no staging file → no size/FAT limit, no cleanup/reaper, no
target-selection logic, works with the array stopped, and it's faster (single
pass; one zip invocation instead of per-item appends — ~44s vs ~115s on a test
ZFS box). Trade-off: browser's native download indicator instead of an in-page
progress log, and errors after streaming starts yield a truncated download.

Verified on a live ZFS box: streamed archive validates, stderr clean (no zip
pollution), previous/junk excluded, nothing left staged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@elibosley elibosley changed the title feat(flash-backup): background boot backup with selectable compression level feat(flash-backup): stream boot device backup download with selectable compression Jun 23, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 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 `@emhttp/plugins/dynamix/BootInfo.page`:
- Around line 32-37: The cookie check in the setInterval function uses indexOf
with substring matching on the flashBackup cookie, which can incorrectly match
partial or stale cookie values and close the modal prematurely. Instead of using
indexOf('flashBackup='+token) to check if the cookie exists, implement exact
matching by parsing the cookie string properly to extract the full flashBackup
value and compare it exactly against the token, ensuring partial matches don't
trigger an early clearInterval call.

In `@emhttp/plugins/dynamix/include/FlashBackup.php`:
- Around line 29-31: The filename construction in the backup file creation
(variables $server, $osVersion, and $name) uses unsanitized values from the
config that may contain special characters like quotes, separators, or control
characters which can break Content-Disposition header parsing. Apply proper
filename sanitization to both the processed $var['NAME'] value (after
str_replace and strtolower) and the $var['version'] value before they are used
in the $name variable. Remove or escape any special characters that are invalid
in HTTP headers or filenames. Apply the same sanitization fix to line 44 which
also appears to have similar filename construction issues.

In `@emhttp/plugins/dynamix/scripts/flash_backup`:
- Around line 36-43: The chdir('/boot') call on line 36 does not verify that the
directory change succeeded before proceeding with glob('*'). If the chdir fails,
glob will execute from the current working directory instead of /boot,
potentially generating incorrect backup data. Add a check after the
chdir('/boot') call to verify it returns true, and exit with an error code if
the directory change fails. This ensures glob() only runs after successfully
changing to the /boot directory.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 6fd20889-aa72-4719-a45d-ebc56196dd00

📥 Commits

Reviewing files that changed from the base of the PR and between 6bcfefe and f1dbf81.

📒 Files selected for processing (4)
  • emhttp/languages/en_US/helptext.txt
  • emhttp/plugins/dynamix/BootInfo.page
  • emhttp/plugins/dynamix/include/FlashBackup.php
  • emhttp/plugins/dynamix/scripts/flash_backup
✅ Files skipped from review due to trivial changes (1)
  • emhttp/languages/en_US/helptext.txt

Comment thread emhttp/plugins/dynamix/BootInfo.page Outdated
Comment on lines +29 to +31
$server = isset($var['NAME']) ? str_replace(' ', '_', strtolower($var['NAME'])) : 'tower';
$osVersion = $var['version'] ?? 'unknown';
$name = "$server-v$osVersion-boot-backup-".date('Ymd-Hi').".zip";

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Sanitize filename parts before sending Content-Disposition.

Lines 29-31 use config-derived NAME/version directly in the attachment filename. Special characters (quotes, separators, control chars) can break filename parsing across clients.

Suggested patch
-$server = isset($var['NAME']) ? str_replace(' ', '_', strtolower($var['NAME'])) : 'tower';
-$osVersion = $var['version'] ?? 'unknown';
-$name = "$server-v$osVersion-boot-backup-".date('Ymd-Hi').".zip";
+$server = isset($var['NAME']) ? str_replace(' ', '_', strtolower($var['NAME'])) : 'tower';
+$osVersion = $var['version'] ?? 'unknown';
+$safeServer = preg_replace('/[^a-z0-9._-]+/i', '_', $server);
+$safeVersion = preg_replace('/[^a-z0-9._-]+/i', '_', $osVersion);
+$name = "$safeServer-v$safeVersion-boot-backup-".date('Ymd-Hi').".zip";

Also applies to: 44-44

🤖 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 `@emhttp/plugins/dynamix/include/FlashBackup.php` around lines 29 - 31, The
filename construction in the backup file creation (variables $server,
$osVersion, and $name) uses unsanitized values from the config that may contain
special characters like quotes, separators, or control characters which can
break Content-Disposition header parsing. Apply proper filename sanitization to
both the processed $var['NAME'] value (after str_replace and strtolower) and the
$var['version'] value before they are used in the $name variable. Remove or
escape any special characters that are invalid in HTTP headers or filenames.
Apply the same sanitization fix to line 44 which also appears to have similar
filename construction issues.

Comment on lines +36 to +43
chdir('/boot');
$items = [];
foreach (glob('*', GLOB_NOSORT) as $entry) {
if (in_array($entry, $out)) continue;
if (preg_match('/-boot-backup-.*\.zip$/', $entry)) continue; // stray old backups
$items[] = $entry;
}
if (!$items) exit(1);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Fail fast if /boot cannot be set as the working directory.

Line 36 does not check chdir('/boot'). If /boot is unavailable, glob('*') runs from the current directory and can produce a valid-but-wrong backup payload.

Suggested patch
-chdir('/boot');
+if (!chdir('/boot')) exit(1);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
chdir('/boot');
$items = [];
foreach (glob('*', GLOB_NOSORT) as $entry) {
if (in_array($entry, $out)) continue;
if (preg_match('/-boot-backup-.*\.zip$/', $entry)) continue; // stray old backups
$items[] = $entry;
}
if (!$items) exit(1);
chdir('/boot') or exit(1);
$items = [];
foreach (glob('*', GLOB_NOSORT) as $entry) {
if (in_array($entry, $out)) continue;
if (preg_match('/-boot-backup-.*\.zip$/', $entry)) continue; // stray old backups
$items[] = $entry;
}
if (!$items) exit(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 `@emhttp/plugins/dynamix/scripts/flash_backup` around lines 36 - 43, The
chdir('/boot') call on line 36 does not verify that the directory change
succeeded before proceeding with glob('*'). If the chdir fails, glob will
execute from the current working directory instead of /boot, potentially
generating incorrect backup data. Add a check after the chdir('/boot') call to
verify it returns true, and exit with an error code if the directory change
fails. This ensures glob() only runs after successfully changing to the /boot
directory.

A streamed zip has no known final size, so the browser can't show a progress
bar. Drive a real bar from the server instead, without giving up streaming:

- scripts/flash_backup: add a `size` mode that prints the total byte size of the
  included set (single source of truth for the file list).
- include/FlashBackup.php: pipe the zip to the client via popen and, as bytes
  flow, publish a percentage (bytes-sent / total) to the 'flash_backup' nchan
  channel, capped at 99% until the stream finishes (then _DONE_). The boot device
  is mostly already-compressed OS files, so output closely tracks input and the
  bar is accurate in practice.
- BootInfo.page: subscribe to the channel and render a real progress bar that
  fills as the download streams, completing on _DONE_.

Verified on a live ZFS box: size=2.55GB, bar climbs monotonically to ~96% then
completes, streamed archive validates.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
emhttp/plugins/dynamix/include/FlashBackup.php (1)

35-37: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win

Sanitize filename parts before placing them in Content-Disposition.

NAME/version from var.ini are used unsanitized in the attachment filename (also at Line 47). Quotes, separators, or control chars can break filename parsing across clients.

🤖 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 `@emhttp/plugins/dynamix/include/FlashBackup.php` around lines 35 - 37, The
filename constructed in the $name variable is using unsanitized values from
var.ini ($var['NAME'] and $var['version']), which can contain quotes, control
characters, or separators that break HTTP Content-Disposition header parsing.
Sanitize both the $server variable (derived from $var['NAME']) and the
$osVersion variable (from $var['version']) by removing or escaping problematic
characters such as quotes, slashes, backslashes, and control characters before
they are concatenated into the $name variable. Apply the same sanitization at
line 47 where the filename is used in the Content-Disposition header.
🤖 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 `@emhttp/plugins/dynamix/BootInfo.page`:
- Around line 23-33: The NchanSubscriber for nchan_backup is created with
default settings that cause it to replay the oldest buffered message from
previous backup runs, which means a stale _DONE_ message can immediately close a
fresh backup dialog when start() is called. Fix this by adding the
nchan_subscriber_first_message option set to 'newest' in the NchanSubscriber
constructor options object (the second parameter) to ensure only new messages
trigger the backup progress updates, preventing stale completion messages from
prior runs from interfering with new backups.

In `@emhttp/plugins/dynamix/include/FlashBackup.php`:
- Around line 52-69: The backup completion signal is published unconditionally
without checking the exit status of the child process. Capture the return value
from the pclose() call to get the exit status of the zip command, then
conditionally publish _DONE_ only when the exit status indicates success
(typically 0). When the exit status indicates failure or when popen() fails to
start the process, publish an appropriate error signal instead (such as _ERROR_)
to ensure the client-side handler can properly report backup failures and
prevent data corruption from incomplete backups.

---

Duplicate comments:
In `@emhttp/plugins/dynamix/include/FlashBackup.php`:
- Around line 35-37: The filename constructed in the $name variable is using
unsanitized values from var.ini ($var['NAME'] and $var['version']), which can
contain quotes, control characters, or separators that break HTTP
Content-Disposition header parsing. Sanitize both the $server variable (derived
from $var['NAME']) and the $osVersion variable (from $var['version']) by
removing or escaping problematic characters such as quotes, slashes,
backslashes, and control characters before they are concatenated into the $name
variable. Apply the same sanitization at line 47 where the filename is used in
the Content-Disposition header.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 17d978ce-7f0d-4e01-929c-85954e8b2c34

📥 Commits

Reviewing files that changed from the base of the PR and between f1dbf81 and 71ae4cf.

📒 Files selected for processing (3)
  • emhttp/plugins/dynamix/BootInfo.page
  • emhttp/plugins/dynamix/include/FlashBackup.php
  • emhttp/plugins/dynamix/scripts/flash_backup
🚧 Files skipped from review as they are similar to previous changes (1)
  • emhttp/plugins/dynamix/scripts/flash_backup

Comment thread emhttp/plugins/dynamix/BootInfo.page
Comment thread emhttp/plugins/dynamix/include/FlashBackup.php Outdated
elibosley and others added 6 commits June 23, 2026 11:43
…survives

The progress bar stayed at 0%: triggering the download with `window.location`
starts a top-level navigation, and NchanSubscriber stops itself on the window's
unload/beforeunload — so the subscriber died the instant the download began and
no progress (nor _DONE_) ever reached the page.

Trigger the download from a hidden iframe instead, so the main window never
navigates and the subscriber keeps receiving progress. Also give the dialog a
Close button as a safety and fix the now-inaccurate status text.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add a Backup mode selector. Download streams to the browser as before
(include/FlashBackup.php). Save to the server reuses the job-based background
backup (scripts/flash_backup_save via StartCommand.php): it writes the archive
to a persistent disk target - cache/SSD pool, then an array share, then the boot
device's own pool (no RAM, since a saved backup must survive a reboot) - keeps
it, and reports the saved path. Useful when the connection to the server is slow.

Both modes share the same nchan progress bar. flash_backup_save publishes a
percentage per item, then _SAVED_<path> + _DONE_ (or _ERROR_<msg>); the page
shows the path on completion for save mode and auto-downloads for download mode.

Verified on a live ZFS box: save writes a valid 2.5GB archive to the pool, keeps
it, integrity OK, progress published throughout.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…upt progress

nchan retains the last message on the channel, so starting a backup made the
fresh subscriber immediately receive a stale "100"/"_DONE_" from a previous run
- the bar jumped to 100% on the first click. Compounding it, the per-click
start()/stop() of the subscriber raced ("already started"), leaving the next run
with a dead subscriber stuck at 0%.

Fix: tag each run with a token (Date.now()). The endpoint and save job publish
"<token>:<payload>"; the page keeps ONE subscriber for the page lifetime and
ignores any message whose token isn't the current run. No more per-click
start/stop, and stale buffered messages are filtered out.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…rop shared-file edits

- Save mode is now much faster: pipe the single-pass streaming zip
  (scripts/flash_backup) into the destination file instead of appending each
  top-level item to a growing archive (one zip invocation vs many).
- On completion the save job raises an Unraid notification (the tray bell) with a
  click-to-download link, exposes the saved file for download via a docroot
  symlink, and the GUI shows a Download button next to the saved path.
- Revert the helptext.txt and .gitignore edits to base so this PR touches only
  its own files. helptext.txt was the single file shared with other in-flight PR
  test plugins (#2671, #2673), which made the stacked PR-plugin patch fail to
  apply; with it gone, #2677 stacks cleanly.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The save-to-server mode symlinked the finished backup into the webGui docroot so
the Download button could fetch it at /<name>.zip. That path is session-gated by
nginx (not open), but it left a predictable, session-reachable link to the
full-flash backup (license, SSH/WireGuard keys, configs) sitting there for the
whole uptime - longer exposure than stock Unraid, which removes its download
symlink right after the download.

Replace it with an authenticated, path-validated serve endpoint:
- include/FlashBackup.php gains a `serve=<name>` mode that streams an already-
  saved backup only if the name matches the flash-backup pattern AND resolves
  onto a pool/array/boot mount (basename-stripped + realpath-checked), so it
  can't be turned into an arbitrary file read. It also sends Content-Length, so
  the browser shows a real progress bar for this download.
- flash_backup_save no longer creates the docroot symlink; the notification's
  link and the GUI Download button point at the serve endpoint instead.

Verified: traversal/bad names return nothing; a real saved backup serves (PK
magic); the previously-exposed symlink removed from the live server.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Make server-side saves discoverable and manageable:
- The save-complete dialog now shows the path plus a Download link and an
  "Open in File Manager" link (/Main/Browse?dir=...) to where it landed.
- The Boot Device page lists any boot backups already saved on the server
  (scanned from pool/array/boot mounts), each with its full path, size, a
  Download link (authenticated serve endpoint), and a File Manager link - so you
  can find them later and clean them up.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant