fix: re-encrypt file metadata on move and bin restore#507
Merged
Conversation
A file's FileMetadata IPNS record is AES-256-GCM encrypted with its parent folder's folderKey at upload (sdk-core file/index.ts). moveItem only moved the FilePointer between folders without re-encrypting the per-file record, so preview/edit/download after a move decrypted with the destination folderKey and failed with CryptoError: Decryption failed. In CipherBoxClient.moveItem, when the moved child is a file, unwrap its IPNS key, resolve FileMetadata with source.folderKey, then updateFileMetadata with folderKey=dest.folderKey and createVersion=false — before the folder publishes, so a failure aborts the move. Folders skip re-encryption (children keep the subfolder's own folderKey). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: b5de67334463
restoreFromBin re-inserted the FilePointer and published folder metadata but never re-encrypted the file's FileMetadata IPNS record, which addToBin leaves encrypted under the original parent's folderKey. Restoring to a folder with a different folderKey (the SDK accepts an arbitrary target) would make the file undecryptable — same root cause as the moveItem bug. Re-encrypt only when the restore target differs from the original parent (restoring in place is a no-op): resolve FileMetadata with the original parent's folderKey, then updateFileMetadata under the target folderKey with createVersion=false, before the folder publish so a failure aborts cleanly. If the original parent is not loaded, throw rather than risk corruption. Also guard the moveItem re-encrypt path against a missing file IPNS key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: ee9f190ef86c
…store Extend the bin-restore re-encryption to survive the case where the original parent folder no longer exists (delete file, delete its parent, then restore elsewhere). addToBin now captures the source folder's folderKey on the BinEntry (originalFolderKeyEncrypted, ECIES-wrapped for the vault) at delete time; restoreFromBin prefers it as the source decryption key and falls back to the live folder tree for legacy entries. Adds the BinEntry field + schema validation. Restore can now re-encrypt a file's metadata to any destination regardless of whether its original parent survives. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: da44d706495c
Web e2e (move-restore-content.spec.ts): upload a file with known content, move it into a subfolder, and assert the decrypted content via the text editor (a fresh decrypt under the destination folderKey — the exact path the bug broke); same for delete-to-bin + restore. Desktop e2e (test-cross-client-sync.sh): move a file via the FUSE mount and verify content through a fresh SDK read under the destination folderKey. verify-filepointer.mjs gains optional --folder-name to traverse one subfolder level (unwraps the child folderKey from its FolderEntry). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 9567670ea97c
…ove todo Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: bb3068e0d9ff
github-actions
Bot
added
release:sdk:feat
Contributor
Release Preview
Cascade Details
|
|
Warning Review limit reached
More reviews will be available in 46 minutes and 16 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughFixes a deterministic ChangesFileMetadata Re-encryption on Move and Bin Restore
Sequence DiagramssequenceDiagram
participant UI as Web/FUSE Client
participant Client as CipherBoxClient
participant reencrypt as reencryptFileMetadataForFolderChange
participant sdkCore
participant Crypto as crypto
UI->>Client: moveItem(srcFolderId, dstFolderId, childId)
Client->>sdkCore: moveItem(srcFolderId, dstFolderId, childId)
sdkCore-->>Client: movedItem {type:"file", ipnsPrivateKeyEncrypted}
Client->>sdkCore: updateFolderMetadataAndPublish(dest)
Client->>Crypto: unwrapKey(ipnsPrivateKeyEncrypted, vaultPrivateKey)
Crypto-->>Client: ipnsPrivateKey
Client->>reencrypt: fileMetaIpnsName, ipnsPrivateKey, src.folderKey, dest.folderKey
reencrypt->>sdkCore: resolveFileMetadata(ipnsName, src.folderKey)
sdkCore-->>reencrypt: FileMetadata
reencrypt->>sdkCore: updateFileMetadata(ipnsKey, dest.folderKey, createVersion=false)
sdkCore-->>reencrypt: updated record
reencrypt-->>Client: done
Client->>sdkCore: updateFolderMetadataAndPublish(source)
Client-->>UI: move complete
sequenceDiagram
participant Caller
participant restoreFromBin
participant Crypto as crypto
participant reencrypt as reencryptFileMetadataForFolderChange
participant sdkCore
Caller->>restoreFromBin: restore(binEntry, targetFolderId)
restoreFromBin->>restoreFromBin: compute mustReencrypt, validate preconditions
restoreFromBin->>sdkCore: updateFolderMetadataAndPublish(target)
restoreFromBin->>Crypto: unwrapKey(originalFolderKeyEncrypted)
Crypto-->>restoreFromBin: srcFolderKey
restoreFromBin->>Crypto: unwrapKey(ipnsPrivateKeyEncrypted)
Crypto-->>restoreFromBin: ipnsPrivateKey
restoreFromBin->>reencrypt: fileMetaIpnsName, ipnsPrivateKey, srcFolderKey, dstFolderKey
reencrypt->>sdkCore: resolveFileMetadata(ipnsName, srcFolderKey)
sdkCore-->>reencrypt: FileMetadata
reencrypt->>sdkCore: updateFileMetadata(ipnsKey, dstFolderKey, createVersion=false)
sdkCore-->>reencrypt: updated
reencrypt-->>restoreFromBin: done
restoreFromBin-->>Caller: restore complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/client.ts (1)
715-760:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRe-encryption happens before destination publish, which breaks crash-safety on partial failure.
At Line 737 the file metadata is re-published under
dest.folderKeybefore any folder publish. If the destination publish at Line 749 fails, the file remains referenced only from the source folder, but its metadata is no longer decryptable withsource.folderKey.This creates a deterministic undecryptable state on one failed call path. Please enforce an ordering/compensation strategy that preserves at least one readable pointer-key pair across partial failures.
🤖 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 `@packages/sdk/src/client.ts` around lines 715 - 760, The file metadata re-encryption block that re-publishes the FileMetadata under dest.folderKey (the section checking for ipnsPrivateKeyEncrypted and calling sdkCore.updateFileMetadata) is executing before the destination folder metadata is published at line 749. Move this entire re-encryption block to execute after the destResult from sdkCore.updateFolderMetadataAndPublish completes, so that the destination folder is published first with the moved file reference before re-encrypting the file's metadata. This preserves crash-safety by ensuring at least one valid readable pointer-key pair exists at any failure point.
🧹 Nitpick comments (3)
packages/sdk/src/__tests__/bin.test.ts (1)
308-563: ⚡ Quick winAdd a direct test assertion that
addToBinpersistsoriginalFolderKeyEncryptedfor files.Current new restore tests hand-craft this field, so they don’t prove the capture path in
addToBinworks. Please assert it in the existingaddToBinfile case (and/or add a folder case asserting it stays undefined).✅ Minimal test addition
-import { unwrapKey } from '`@cipherbox/crypto`'; +import { unwrapKey, wrapKey } from '`@cipherbox/crypto`'; @@ expect(result.updatedBinState.entries).toHaveLength(1); expect(result.updatedBinState.entries[0].name).toBe('doc.txt'); + expect(result.updatedBinState.entries[0].originalFolderKeyEncrypted).toBe('aabb'); + expect(wrapKey).toHaveBeenCalledTimes(1); expect(result.updatedBinState.sequenceNumber).toBe(1);As per coding guidelines, "
**/*.test.ts: Focus on test coverage, edge cases, and test quality. Ensure tests are meaningful and not just for coverage metrics."🤖 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 `@packages/sdk/src/__tests__/bin.test.ts` around lines 308 - 563, The restore tests hand-craft the originalFolderKeyEncrypted field in BinState objects, but there is no direct test asserting that the addToBin function actually captures and persists this field when deleting files. Add an assertion to the existing addToBin file test case that verifies the persisted entry in the binState contains the originalFolderKeyEncrypted field with the correct folder key value. Optionally, add a test case for folder items to verify that originalFolderKeyEncrypted remains undefined for non-file items, ensuring the capture path in addToBin is properly tested rather than just assuming it works through the restore tests.Source: Coding guidelines
tests/web-e2e/tests/move-restore-content.spec.ts (1)
130-156: ⚡ Quick winRestore e2e currently misses the cross-folder re-encryption path.
This test restores to the original parent (Line 146), so it does not exercise the “restore to different destination folderKey” branch that required this fix. Please add a variant restoring into a different folder and assert decrypted content there.
🤖 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/web-e2e/tests/move-restore-content.spec.ts` around lines 130 - 156, The test '3. Content survives delete-to-bin then restore' currently only restores content to the original parent folder (root), missing the cross-folder re-encryption scenario. Add a test variant that creates or navigates to a different folder before restoring the deleted content, then uses binPage.restoreItem to restore it to that different destination folder. After restoration, verify that readContentViaEditor still returns the correct decrypted content in the new folder location to ensure the re-encryption with the different folderKey works properly.packages/sdk/src/__tests__/client-move-reencrypt.test.ts (1)
188-243: ⚡ Quick winAdd an edge-case test for missing
ipnsPrivateKeyEncryptedon moved files.
moveItemnow has a hard guard for this malformed pointer path, but this suite only covers valid file pointers and folder moves. A dedicated test for the throw path will prevent silent regressions in this safety check.As per coding guidelines,
**/*.test.ts: “Focus on test coverage, edge cases, and test quality.”🤖 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 `@packages/sdk/src/__tests__/client-move-reencrypt.test.ts` around lines 188 - 243, Add a new test case within this suite to verify that moveItem throws an error when encountering a file pointer missing the ipnsPrivateKeyEncrypted field. Create a test similar to the "does NOT re-encrypt file metadata when moving a folder" test by constructing a fileChild object without the ipnsPrivateKeyEncrypted property, setting it in the source folder tree, calling client.moveItem with this invalid file, and asserting that the expected error is thrown. This will ensure the guard validation in moveItem catches this malformed pointer edge case and prevents silent regressions.Source: Coding guidelines
🤖 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 `@packages/sdk-core/scripts/verify-filepointer.mjs`:
- Around line 130-133: The `unwrapKey` function call with its arguments is not
formatted according to Prettier's style rules. Reformat the multiline function
call starting with `const subFolderKey = await unwrapKey(` by either condensing
it to a single line if it fits within the line length limit, or if it must
remain multiline, adjust the indentation and argument placement to match
Prettier's expected formatting rules for function calls with multiple arguments.
In `@packages/sdk/src/bin/index.ts`:
- Around line 277-283: The computation of originalFolderKeyEncrypted using the
wrapKey function is currently occurring after updateFolderMetadataAndPublish,
which creates a race condition where if wrapKey fails, the file has already been
removed from the folder but never committed to bin metadata. Move the wrapKey
call that computes originalFolderKeyEncrypted to execute before the
updateFolderMetadataAndPublish call to ensure that any wrapping failures occur
before the folder deletion is published, preventing the split state condition.
- Around line 381-416: The unwrapped key material (sourceFolderKey and
fileIpnsPrivateKey) created in this block are sensitive and must be cleared from
memory after use. Wrap the entire code block starting from where sourceFolderKey
is first assigned through the completion of the updateFileMetadata call in a
try-finally statement. In the finally block, zeroize both sourceFolderKey and
fileIpnsPrivateKey using the appropriate zeroization function to ensure these
sensitive bytes are cleared from memory regardless of whether updateFileMetadata
succeeds or throws an error.
In `@packages/sdk/src/client.ts`:
- Around line 728-746: The variable fileIpnsPrivateKey contains sensitive
cryptographic material that is not being cleared from memory if
resolveFileMetadata or subsequent operations throw an exception. Wrap the code
block starting with the fileIpnsPrivateKey unwrapping through the
sdkCore.updateFileMetadata call in a try-finally block, placing the cleanup code
(clearing fileIpnsPrivateKey) in the finally block to ensure it always executes
regardless of whether any operation throws an error, following the security
guideline to clear sensitive data from memory after use.
---
Outside diff comments:
In `@packages/sdk/src/client.ts`:
- Around line 715-760: The file metadata re-encryption block that re-publishes
the FileMetadata under dest.folderKey (the section checking for
ipnsPrivateKeyEncrypted and calling sdkCore.updateFileMetadata) is executing
before the destination folder metadata is published at line 749. Move this
entire re-encryption block to execute after the destResult from
sdkCore.updateFolderMetadataAndPublish completes, so that the destination folder
is published first with the moved file reference before re-encrypting the file's
metadata. This preserves crash-safety by ensuring at least one valid readable
pointer-key pair exists at any failure point.
---
Nitpick comments:
In `@packages/sdk/src/__tests__/bin.test.ts`:
- Around line 308-563: The restore tests hand-craft the
originalFolderKeyEncrypted field in BinState objects, but there is no direct
test asserting that the addToBin function actually captures and persists this
field when deleting files. Add an assertion to the existing addToBin file test
case that verifies the persisted entry in the binState contains the
originalFolderKeyEncrypted field with the correct folder key value. Optionally,
add a test case for folder items to verify that originalFolderKeyEncrypted
remains undefined for non-file items, ensuring the capture path in addToBin is
properly tested rather than just assuming it works through the restore tests.
In `@packages/sdk/src/__tests__/client-move-reencrypt.test.ts`:
- Around line 188-243: Add a new test case within this suite to verify that
moveItem throws an error when encountering a file pointer missing the
ipnsPrivateKeyEncrypted field. Create a test similar to the "does NOT re-encrypt
file metadata when moving a folder" test by constructing a fileChild object
without the ipnsPrivateKeyEncrypted property, setting it in the source folder
tree, calling client.moveItem with this invalid file, and asserting that the
expected error is thrown. This will ensure the guard validation in moveItem
catches this malformed pointer edge case and prevents silent regressions.
In `@tests/web-e2e/tests/move-restore-content.spec.ts`:
- Around line 130-156: The test '3. Content survives delete-to-bin then restore'
currently only restores content to the original parent folder (root), missing
the cross-folder re-encryption scenario. Add a test variant that creates or
navigates to a different folder before restoring the deleted content, then uses
binPage.restoreItem to restore it to that different destination folder. After
restoration, verify that readContentViaEditor still returns the correct
decrypted content in the new folder location to ensure the re-encryption with
the different folderKey works properly.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e5a7d7a-d110-423f-ab2f-57bdb914927a
📒 Files selected for processing (13)
.planning/debug/resolved/decrypt-fail-after-move.md.planning/todos/pending/2026-06-17-shared-folder-move-must-reencrypt-file-metadata.mdpackages/core/src/bin/schema.tspackages/core/src/bin/types.tspackages/sdk-core/scripts/verify-filepointer.mjspackages/sdk/src/__tests__/bin.test.tspackages/sdk/src/__tests__/client-extended.test.tspackages/sdk/src/__tests__/client-move-reencrypt.test.tspackages/sdk/src/bin/index.tspackages/sdk/src/client.tsrelease-please-config.jsontests/desktop-e2e/scripts/test-cross-client-sync.shtests/web-e2e/tests/move-restore-content.spec.ts
- verify-filepointer.mjs: apply prettier formatting on the --folder-name unwrap call that was failing the Lint CI job. - file.test.ts: replace 10 `any` annotations with precise types via local FileMetadata/EncryptedFileMetadata mirrors and SdkContext/IpfsAddResult. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: effbeaf44cb9
…re folder key on delete The desktop FUSE/WinFsp filesystem implements rename and unlink in Rust, bypassing the SDK moveItem/addToBin paths fixed for the web client. A file's FileMetadata is AES-256-GCM sealed with its parent folderKey, so: - handle_rename now re-encrypts a moved file's FileMetadata to the destination folderKey before the move settles, via a new spawn_file_meta_reencrypt that resolves under the source key and republishes under the dest key. Mirrored in the FUSE and WinFsp handlers. Without it, fresh resolves under the new folder key fail with the staging 'Decryption failed' bug. - handle_unlink now captures the file's parent folderKey, ECIES-wrapped to the user, into a new BinEntry.originalFolderKeyEncrypted field, so a later SDK restore can re-encrypt to any destination even if the original parent is gone. The Rust BinEntry gains original_folder_key_encrypted, JSON-compatible with the TypeScript schema the web restore reads. Tests: the per-platform shell/PowerShell move test is replaced by one cross-platform test-move-content.mjs run on macOS, Linux and Windows. It moves a file through the mount and reads it back via a fresh SDK client under the destination folderKey, strict on every platform. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 90bd3edfdce4
github-actions
Bot
added
release:cipherbox-core:fix
- addToBin: compute originalFolderKeyEncrypted (wrapKey) BEFORE publishing the folder deletion, so a wrap failure aborts before any folder/bin mutation rather than leaving a split state. - restoreFromBin: wrap the restore-time re-encryption in try/finally and zeroize the unwrapped source folderKey and file IPNS private key. Only the freshly unwrapped key is cleared, never the shared folderTree key from the legacy fallback. - moveItem: guard fileIpnsPrivateKey with try/finally so it is cleared even if resolveFileMetadata throws before updateFileMetadata runs. - bin.test.ts: add clearBytes to the @cipherbox/crypto mock. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 8ffe6d7ccf28
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #507 +/- ##
==========================================
+ Coverage 63.92% 66.88% +2.96%
==========================================
Files 141 158 +17
Lines 10875 17588 +6713
Branches 1209 1226 +17
==========================================
+ Hits 6952 11764 +4812
- Misses 3678 5579 +1901
Partials 245 245
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Add failure-path tests for CipherBoxClient.moveItem re-encryption, addressing the CodeRabbit CLI review: - unwrapKey / resolveFileMetadata / updateFileMetadata failures reject the move with no folder publish (re-encrypt-before-publish aborts cleanly). - A destination-publish failure after a successful re-encryption pins the known non-atomic window where the FileMetadata is re-keyed but the pointer remains in the source folder. File a follow-up todo for a retry-safe recovery mechanism, since neither publish ordering closes that window without idempotent re-encryption. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: d0c14b0318f6
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/sdk/src/client.ts (1)
715-752:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid re-encrypting before the move is durably visible.
Line 740 persists the file metadata under
dest.folderKeybefore either folder publish runs. If Line 755 or Line 766 fails afterward, the source folder can remain authoritative while itsFilePointernow points at metadata that only decrypts with the destination key. Reorder this so a successful destination publish is visible before re-encryption, or add rollback/recovery for failures after the metadata update.As per coding guidelines, “Include proper error handling for crypto operations.”
🤖 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 `@packages/sdk/src/client.ts` around lines 715 - 752, The updateFileMetadata call is re-encrypting the file metadata under dest.folderKey before the destination folder publish becomes visible, creating a state mismatch if subsequent folder publish operations fail. Reorder the code so that updateFileMetadata is called after the destination folder publish completes successfully, or implement rollback/recovery logic to handle failures that occur after the metadata re-encryption. This ensures the destination folder is durably visible before re-encrypting metadata under its key, maintaining consistency between the source folder's state and the FilePointer's decryption requirements.Source: Coding guidelines
🤖 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 `@crates/fuse/src/lib.rs`:
- Around line 718-764: The file metadata re-encryption after a cross-folder move
is currently performed as a detached, best-effort background task spawned with
std::thread::spawn that only logs errors. This can leave the system in an
inconsistent state if the task fails, crashes, or is interrupted. Instead of
spawning a detached thread that ignores failures, refactor the rename operation
to either wait for the re-encryption to complete synchronously before the rename
commits (ensuring atomicity between the folder move and metadata key
transition), or implement a persistent retry journal that logs failed
re-encryption attempts so they can be retried and never silently lost. This
ensures the destination folder key and FileMetadata encryption state cannot
diverge.
In `@release-please-config.json`:
- Around line 141-142: The "release-as" value for crates/core is currently set
to "0.5.1" which indicates a patch version bump, but it should be changed to
"0.6.0" to reflect a minor version bump. This is required because a public
optional field was added to a public struct without the #[non_exhaustive]
attribute, which under Rust semver conventions necessitates a minor version bump
rather than a patch bump. Update the "release-as" configuration value from
"0.5.1" to "0.6.0".
In `@tests/desktop-e2e/scripts/test-move-content.mjs`:
- Around line 92-97: The spawnSync() call in the verify() function lacks a
timeout and maxBuffer configuration, causing indefinite hangs if the child
process stalls, with no recovery possible from the pollVerify() retry loop. Add
timeout: 60_000 and maxBuffer: 1024 * 1024 to the options object passed to
spawnSync(), and after the call, check result.error to handle spawn failures
before evaluating result.status === 0.
---
Outside diff comments:
In `@packages/sdk/src/client.ts`:
- Around line 715-752: The updateFileMetadata call is re-encrypting the file
metadata under dest.folderKey before the destination folder publish becomes
visible, creating a state mismatch if subsequent folder publish operations fail.
Reorder the code so that updateFileMetadata is called after the destination
folder publish completes successfully, or implement rollback/recovery logic to
handle failures that occur after the metadata re-encryption. This ensures the
destination folder is durably visible before re-encrypting metadata under its
key, maintaining consistency between the source folder's state and the
FilePointer's decryption requirements.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 67f09364-b428-4ad5-bbad-166fa2db19b5
📒 Files selected for processing (16)
.planning/todos/pending/2026-06-17-move-restore-reencrypt-partial-failure-recovery.mdcrates/core/src/bin.rscrates/fuse/src/lib.rscrates/fuse/src/platform/windows/write_ops.rscrates/fuse/src/write_ops.rspackages/sdk-core/scripts/verify-filepointer.mjspackages/sdk-core/src/__tests__/file.test.tspackages/sdk/src/__tests__/bin.test.tspackages/sdk/src/__tests__/client-move-reencrypt.test.tspackages/sdk/src/bin/index.tspackages/sdk/src/client.tsrelease-please-config.jsontests/desktop-e2e/scripts/run-all.ps1tests/desktop-e2e/scripts/run-all.shtests/desktop-e2e/scripts/test-cross-client-sync.shtests/desktop-e2e/scripts/test-move-content.mjs
✅ Files skipped from review due to trivial changes (3)
- .planning/todos/pending/2026-06-17-move-restore-reencrypt-partial-failure-recovery.md
- tests/desktop-e2e/scripts/test-cross-client-sync.sh
- packages/sdk-core/src/tests/file.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/sdk-core/scripts/verify-filepointer.mjs
- packages/sdk/src/tests/bin.test.ts
- packages/sdk/src/bin/index.ts
spawnSync had no timeout, so a stalled verify-filepointer.mjs would hang the whole desktop E2E suite (pollVerify cannot recover from a blocking spawnSync). Add timeout (60s) + maxBuffer, and surface spawn failures via result.error. Addresses CodeRabbit review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 3585dd5af847
Reorders moveItem to publish dest -> re-encrypt FileMetadata -> publish source (was re-encrypt -> publish dest -> publish source). The metadata is re-keyed to the destination only once the destination folder is durably visible, so at every intermediate failure the file stays readable from a folder that still lists it under the matching key (source before re-encrypt, destination after) instead of being readable from neither. Add-before-remove crash safety is preserved. Addresses the CodeRabbit out-of-diff review on client.ts (move re-encryption before the move is durably visible). Rewrites the failure-path tests to pin the new ordering and adds an ordering invariant test. Narrows the follow-up todo: moveItem is now reorder-hardened; the residual is idempotent retry, the same reorder for restoreFromBin, and a desktop re-encrypt retry. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 597561399b8b
Owner
Author
|
Addressed the out-of-diff CodeRabbit comment on
Failure-path tests rewritten to pin the new ordering (+ an ordering-invariant test): destination-publish failure now aborts cleanly before any re-encrypt; a re-encrypt failure leaves the file readable from the source; a source-publish failure leaves it readable from the destination. The residual (idempotent clean-retry, the same reorder for |
…ial failures Extract a shared idempotent re-key helper used by moveItem and restoreFromBin: if the source-key resolve fails to decrypt, confirm the record under the destination key and treat the re-encryption as already done, so a retry after a partial failure completes instead of throwing. Reorder restoreFromBin to publish the target folder before re-keying, mirroring moveItem, and validate preconditions up front so a guaranteed failure aborts without a broken listing. Make the restore add idempotent by child id so a retry after a post-publish failure replaces rather than duplicates the listing. Add a bounded in-memory retry with exponential backoff to the desktop FUSE/WinFsp spawn_file_meta_reencrypt, with the same dest-key idempotency check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 6f1555823b1f
Owner
Author
Recovery hardening for move/restore re-encryption (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/sdk/src/__tests__/client-move-reencrypt.test.ts`:
- Around line 246-269: The test for moveItem uses identical 'publish' markers
for both the destination and source folder publishes, which means the order
array cannot distinguish between them and could pass with an incorrect sequence.
Modify the vi.mocked(sdkCore.updateFolderMetadataAndPublish).mockImplementation
to distinguish the destination publish from the source publish by using
different markers or by asserting on the specific payload parameters (such as
checking the ipnsName argument to identify whether it's the destination or
source), then update the expected order array to reflect these distinct markers
so the invariant is properly enforced.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4dc26613-f40a-4e3c-a1b3-d4692f48d0e2
📒 Files selected for processing (8)
.planning/todos/completed/2026-06-17-move-restore-reencrypt-partial-failure-recovery.mdcrates/fuse/src/lib.rspackages/sdk/src/__tests__/bin.test.tspackages/sdk/src/__tests__/client-move-reencrypt.test.tspackages/sdk/src/bin/index.tspackages/sdk/src/client.tspackages/sdk/src/reencrypt.tstests/desktop-e2e/scripts/test-move-content.mjs
✅ Files skipped from review due to trivial changes (1)
- .planning/todos/completed/2026-06-17-move-restore-reencrypt-partial-failure-recovery.md
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/desktop-e2e/scripts/test-move-content.mjs
- packages/sdk/src/client.ts
- packages/sdk/src/bin/index.ts
…ertion Tag each updateFolderMetadataAndPublish call by its target folder so the ordering invariant rejects a wrong source-then-destination sequence, which a bare publish/reencrypt/publish marker would have passed (CodeRabbit review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: 04ad6af54908
Merged
This was referenced Jun 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a repeatable
CryptoError: Decryption failedwhen previewing/editing/downloading a file after it's moved into a subfolder (reported on staging), and the same latent bug on bin restore.Root cause: a file's
FileMetadataIPNS record is AES-256-GCM encrypted with its parent folder'sfolderKeyat upload (sdk-core/file/index.ts). Re-parenting a file moved only theFilePointerbetween folders without re-encrypting the per-file record, so the decrypt path (which uses the current parent'sfolderKey) failed. Listing worked because that only reads theFilePointer(not folder-key-encrypted).Changes
fix(sdk)— move re-encryptCipherBoxClient.moveItem: when the moved child is a file, unwrap its IPNS key, resolveFileMetadatawithsource.folderKey, thenupdateFileMetadataunderdest.folderKey(createVersion: false) before the folder publishes, so a failure aborts cleanly. Folders skip it (children keep the subfolder's own key).fix(sdk)+feat(core,sdk)— bin restore re-encrypt (robust)restoreFromBinhad the same gap. It now re-encrypts when the restore target differs from the original parent. To survive "delete file → delete its parent → restore elsewhere" (where the original parent's key is no longer in the live tree),addToBincaptures the sourcefolderKeyon theBinEntry(originalFolderKeyEncrypted, ECIES-wrapped for the vault); restore prefers it and falls back to the folder tree for legacy entries. Adds theBinEntryfield + schema validation.test(e2e)— content verificationmove-restore-content.spec.ts, validated locally — 3/3): asserts decrypted content (not just listing) after a move into a subfolder and after delete-to-bin + restore, read via the text editor (a fresh decrypt under the destination key — the exact path the bug broke).test-cross-client-sync.sh): moves a file via the FUSE mount and verifies content through a fresh SDK read under the destination key.verify-filepointer.mjsgained--folder-nameto traverse one subfolder level. Runs indesktop-e2e.yml(needs FUSE) — not validated locally.Sibling-path triage
moveItemVerification
client-move-reencrypt(3) +bin.test(15: captured-key, legacy-fallback, skip-in-place, missing-parent-throw) green; core bin/schema green (196).tsc+ eslint clean across changed packages.move-restore-content.spec.ts— 3/3 against the local full stack.desktop-e2e.yml(FUSE), not validated locally. Note: that workflow triggers onapps/desktop/crateschanges, so it won't auto-run on this PR's paths; can be dispatched manually.Notes
c.txtto~/a/b/after deletingb" sequence fails earlier atrequireFolder(target parent gone) — restore-b-first works; the captured key now also enables restoring such a file to any existing folder.Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests