Skip to content

fix(snapshot): make vfs_restore fail closed and apply atomically#1586

Merged
chaliy merged 1 commit into
mainfrom
claude/fix-1576-snapshot-restore-fail-closed
May 7, 2026
Merged

fix(snapshot): make vfs_restore fail closed and apply atomically#1586
chaliy merged 1 commit into
mainfrom
claude/fix-1576-snapshot-restore-fail-closed

Conversation

@chaliy
Copy link
Copy Markdown
Contributor

@chaliy chaliy commented May 7, 2026

Closes #1576.

Summary

restore_snapshot_inner previously restored shell state first and then called fs.vfs_restore(...) ignoring its boolean return. The FileSystemExt contract returned false for unsupported restore, and InMemoryFs::restore returned early without clearing on validation failure. A forged unkeyed snapshot whose VFS section failed path validation therefore returned Ok after mutating the shell state, while leaving previous-tenant files readable in the VFS — a half-restored, cross-tenant leak.

Fix

  • FileSystemExt::vfs_restore now returns crate::Result<()>. The default impl returns Err(Unsupported); InMemoryFs/OverlayFs/MountableFs propagate the inner result.
  • InMemoryFs::restore returns Result<()> with the underlying limit error.
  • restore_snapshot_inner reordered to: validate shell limits → apply VFS (can fail) → apply shell state (cannot fail past validation). Mutation order guarantees no half-restored state.

Tests

New integration test snapshot_restore_fails_closed_on_malformed_vfs forges a poisoned VFS path inside an otherwise valid snapshot and asserts:

  • restore_snapshot returns Err.
  • Shell variable values are unchanged.
  • Previous-tenant VFS contents remain intact.

Existing snapshot tests updated to handle the new Result return on InMemoryFs::restore.

Test plan

  • cargo test -p bashkit --test snapshot_tests (38/38 passing)
  • cargo test -p bashkit --lib fs::memory (49/49 passing)
  • cargo test -p bashkit --lib --no-default-features (2207 passing)
  • cargo fmt --all -- --check
  • cargo clippy -p bashkit --all-features --all-targets -- -D warnings

Generated by Claude Code

Previously, restore_snapshot_inner restored shell state first and then
called fs.vfs_restore(...) ignoring its boolean return. The
FileSystemExt contract returned `false` for unsupported restore, and
InMemoryFs::restore returned early without clearing entries on
validation failure. A forged unkeyed snapshot whose VFS section failed
path validation would therefore return Ok after mutating the shell
state, while leaving previous-tenant files readable in the VFS — a
half-restored, cross-tenant leak.

- Change `FileSystemExt::vfs_restore` to return `crate::Result<()>`.
  The default impl returns Err(Unsupported); InMemoryFs/OverlayFs/
  MountableFs propagate the inner restore result.
- Change `InMemoryFs::restore` to return `Result<()>` with the
  underlying limit error and never clear on validation failure.
- Reorder restore_snapshot_inner: validate shell limits, then apply
  VFS (which can fail), then apply shell state (cannot fail past
  validation). Mutation order guarantees no half-restored state.

Add an integration regression test
(snapshot_restore_fails_closed_on_malformed_vfs) that forges a
poisoned VFS path inside an otherwise valid snapshot and verifies:
- restore_snapshot returns Err
- shell variable values are unchanged
- previous-tenant VFS contents remain intact

Closes #1576
@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
bashkit c4757fb Commit Preview URL

Branch Preview URL
May 07 2026, 04:22 AM

@chaliy chaliy merged commit 447e1d6 into main May 7, 2026
29 checks passed
@chaliy chaliy deleted the claude/fix-1576-snapshot-restore-fail-closed branch May 7, 2026 06:50
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.

DeepSec: snapshot restore can report success while retaining stale VFS contents

1 participant