feat: switch file IPNS keys from deterministic HKDF to random#181
Merged
Conversation
Moved to done/, beginning implementation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: ddd3dce423f5
… to FilePointer Add random Ed25519 keypair generation for file IPNS records, replacing deterministic HKDF derivation for new files. HKDF derivation is kept for backward compatibility with legacy FilePointers. - Add optional ipnsPrivateKeyEncrypted field to FilePointer type - Add generateFileIpnsKeypair() using random Ed25519 keygen - Export new function from file and root index - Update folder metadata validation to accept optional field - Add tests for random keypair generation and validation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… updates - createFileMetadata() now generates random Ed25519 keypair instead of HKDF derivation, returns ECIES-wrapped ipnsPrivateKeyEncrypted - Add getFileIpnsPrivateKey() helper: decrypts from FilePointer or falls back to HKDF for legacy files - updateFileMetadata/restoreVersion/deleteVersion now accept the IPNS private key directly instead of re-deriving via HKDF - addFileToFolder/addFilesToFolder store ipnsPrivateKeyEncrypted in FilePointer for new files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- handleAddFile/handleAddFiles pass userPublicKey to createFileMetadata and forward ipnsPrivateKeyEncrypted to addFileToFolder/addFilesToFolder - handleUpdateFile decrypts IPNS key from FilePointer via getFileIpnsPrivateKey before calling updateFileMetadata - handleRestoreVersion/handleDeleteVersion use the same pattern Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add optional ipns_private_key_encrypted to Rust FilePointer struct (skip_serializing_if None, serde default for backward compat) - create() generates random Ed25519 keypair via OsRng instead of HKDF - build_folder_metadata() ECIES-wraps file IPNS private key into FilePointer when available - populate_folder() decrypts ipns_private_key_encrypted from remote metadata when present, falls back to None (HKDF on demand) for legacy FilePointers Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…IPNS keys - Add ipnsPrivateKeyEncrypted field to FilePointer table (Section 7) - Document migration strategy and HKDF fallback behavior - Reorganize Section 14 (IPNS Key Derivation Summary) into HKDF-derived and random keypair categories - Update FileMetadata storage note to reflect dual-source IPNS keys Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CRITICAL: Add HKDF fallback in Rust populate_folder for legacy files without ipnsPrivateKeyEncrypted (prevents silent data divergence) - HIGH: Zero IPNS private key after use in createFileMetadata - HIGH: Cache ECIES-wrapped key hex in inode to avoid re-wrapping on every metadata publish - MEDIUM: Add minimum length validation (64 chars) on ipnsPrivateKeyEncrypted in validateFolderMetadata - MEDIUM: Update stale doc comments referencing HKDF derivation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0b61d0d623d1
Desktop-created files are not enrolled with TEE for IPNS republishing. Logged as future work from security review LOW-01. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0edd75511b27
Mark CRITICAL-01, HIGH-01, HIGH-02, MEDIUM-01, MEDIUM-02 as FIXED with commit references. Remove suggested code (now implemented). Update summary table and remaining open items. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 46966f48dfa5
Legacy files without ipnsPrivateKeyEncrypted fall back to HKDF derivation indefinitely. Should wrap and write back on next publish. Logged from security review LOW-02. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: fc71f7dc4708
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis PR replaces deterministic HKDF-derived per-file IPNS keys with random Ed25519 keypairs, adds ECIES-wrapped storage via a new Changes
Sequence DiagramsequenceDiagram
participant Client as Web Client
participant FM as FileMetadataService
participant Crypto as Crypto Module
participant FS as FolderService
participant IPFS as Folder Metadata (IPFS)
Client->>FM: createFileMetadata(userPublicKey, ...)
FM->>Crypto: generateFileIpnsKeypair()
Crypto-->>FM: {privateKey, publicKey, ipnsName}
FM->>Crypto: wrapKey(privateKey, userPublicKey) (ECIES)
Crypto-->>FM: ipnsPrivateKeyEncrypted (hex)
FM->>FM: buildIpnsRecord(ipnsName, privateKey, teeWrapped?)
FM-->>Client: {ipnsRecord, ipnsPrivateKeyEncrypted}
Client->>FS: addFileToFolder({..., ipnsPrivateKeyEncrypted})
FS->>FS: createFilePointer(include ipnsPrivateKeyEncrypted)
FS->>IPFS: publishFolderMetadata(filePointers)
IPFS-->>FS: confirmed
FS-->>Client: {filePointer, newSequenceNumber}
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.planning/STATE.md (1)
164-174:⚠️ Potential issue | 🟡 MinorTwo new pending todos are missing from the
Pending Todoslist.The PR adds
.planning/todos/pending/2026-02-21-desktop-tee-enrollment-for-new-files.mdand.planning/todos/pending/2026-02-21-lazy-migration-legacy-file-pointers.md, but neither appears in thePending Todossection, and the count still reads7instead of9.📝 Proposed fix
-7 pending todo(s): +9 pending todo(s): - `2026-02-07-web-worker-large-file-encryption.md` -- ... ... - `2026-02-21-ipns-resolution-alternatives.md` -- ... +- `2026-02-21-desktop-tee-enrollment-for-new-files.md` -- Enroll desktop-created files into TEE for IPNS republishing (area: desktop) +- `2026-02-21-lazy-migration-legacy-file-pointers.md` -- Lazy migration of legacy FilePointers to random IPNS keys (area: web)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.planning/STATE.md around lines 164 - 174, Update the "Pending Todos" section in .planning/STATE.md to include the two new pending todo files introduced by the PR: `2026-02-21-desktop-tee-enrollment-for-new-files.md` and `2026-02-21-lazy-migration-legacy-file-pointers.md`, add appropriate brief descriptions/area tags consistent with the existing entries, and bump the pending count from 7 to 9 so the list and count reflect the actual files present.
🧹 Nitpick comments (4)
packages/crypto/src/folder/metadata.ts (1)
74-85: Consider tightening the minimum length toECIES_MIN_CIPHERTEXT_SIZE * 2and fix the inline comment.Two small issues:
- The threshold
64is well below the actual minimum hex length of a valid ECIES-wrapped 32-byte key (~162 hex chars). Strings in the[64, 161]range will pass validation but then fail atunwrapKey()with a less useful error. TheECIES_MIN_CIPHERTEXT_SIZEconstant is already exported from the same package and is the right source of truth.- The inline comment at Line 74 says
"non-empty hex string"but the actual check is a 64-char minimum — slightly misleading.♻️ Proposed refinement
Add import at the top of the file:
+import { ECIES_MIN_CIPHERTEXT_SIZE } from '../constants';Then update the validation block:
- // Optional: ipnsPrivateKeyEncrypted must be a non-empty hex string if present + // Optional: ipnsPrivateKeyEncrypted must be a hex string long enough to hold ECIES output if (entry.ipnsPrivateKeyEncrypted !== undefined) { if ( typeof entry.ipnsPrivateKeyEncrypted !== 'string' || - entry.ipnsPrivateKeyEncrypted.length < 64 + entry.ipnsPrivateKeyEncrypted.length < ECIES_MIN_CIPHERTEXT_SIZE * 2 ) { throw new CryptoError( - 'Invalid metadata format: ipnsPrivateKeyEncrypted must be a hex string (min 64 chars)', + 'Invalid metadata format: ipnsPrivateKeyEncrypted must be a hex string of sufficient length', 'DECRYPTION_FAILED' ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/folder/metadata.ts` around lines 74 - 85, Update the ipnsPrivateKeyEncrypted validation to use the canonical ECIES minimum length and fix the comment: import ECIES_MIN_CIPHERTEXT_SIZE from the package and replace the hardcoded 64 check with a hex-length check of ECIES_MIN_CIPHERTEXT_SIZE * 2 (since hex chars are 2 per byte); also change the inline comment to accurately state "must be a hex string at least ECIES_MIN_CIPHERTEXT_SIZE bytes (hex length ECIES_MIN_CIPHERTEXT_SIZE*2)". Keep the CryptoError thrown (with same code 'DECRYPTION_FAILED') and ensure this prevents passing values that would later fail in unwrapKey().packages/crypto/src/file/derive-ipns.ts (1)
1-17: File header JSDoc describes only the deterministic HKDF path — update to reflect the new random generation function.The module comment accurately describes
deriveFileIpnsKeypairbut doesn't mention the new randomgenerateFileIpnsKeypair. As both functions now live in this file, the header should be updated to avoid misleading future readers.📝 Proposed update to file header
/** * `@cipherbox/crypto` - File IPNS Key Derivation * - * Derives a deterministic Ed25519 IPNS keypair for a specific file - * from the user's secp256k1 privateKey + fileId using HKDF-SHA256. + * Two IPNS keypair strategies for per-file IPNS records: + * + * 1. Deterministic (legacy): derives an Ed25519 keypair from the user's + * secp256k1 privateKey + fileId via HKDF-SHA256 (`deriveFileIpnsKeypair`). + * 2. Random (new files): generates a fresh Ed25519 keypair via CSPRNG + * (`generateFileIpnsKeypair`). The private key is ECIES-wrapped and stored + * in the parent folder's FilePointer so it can be recovered from any device. * * Derivation path: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/crypto/src/file/derive-ipns.ts` around lines 1 - 17, Update the file JSDoc to describe both available key derivation methods: the existing deterministic HKDF-based flow used by deriveFileIpnsKeypair (HKDF-SHA256 with salt "CipherBox-v1" and info "cipherbox-file-ipns-v1:{fileId}" → 32-byte Ed25519 seed → keypair → IPNS name) and the new random generation provided by generateFileIpnsKeypair (random Ed25519 keypair → IPNS name). Mention that both functions coexist in this module, note their different use-cases (deterministic per-file vs random), and keep the domain-separation note distinguishing this domain from vault and registry derivations.apps/desktop/src-tauri/src/fuse/inode.rs (1)
804-843: Test should verifyfile_ipns_key_encrypted_hexpropagation for populated file pointers.The
test_populate_folder_with_file_pointerstest doesn't assert on the newfile_ipns_key_encrypted_hexorfile_ipns_private_keyfields after population. Since the testFilePointerhas noipns_private_key_encrypted, both should beNone(HKDF will also fail with a zero private key). Consider asserting these to lock in the expected behavior for legacy file pointers.🧪 Suggested assertion additions
match &child.kind { - InodeKind::File { file_meta_ipns_name, file_meta_resolved, .. } => { + InodeKind::File { file_meta_ipns_name, file_meta_resolved, file_ipns_key_encrypted_hex, .. } => { assert_eq!( file_meta_ipns_name.as_deref(), Some("k51qzi5uqu5dljtg5upm7x7ugan9lql3ewyknv4r4mhhkwzn8n7cnbd1unfwgx") ); assert!(!file_meta_resolved, "FilePointer should not be resolved yet"); + assert!(file_ipns_key_encrypted_hex.is_none(), "Legacy FilePointer should have no cached encrypted hex"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/inode.rs` around lines 804 - 843, The test test_populate_folder_with_file_pointers should also assert that new fields related to IPNS keys remain None for legacy FilePointer entries: after calling InodeTable::populate_folder and fetching the child inode (created from the FolderChild::File FilePointer), add assertions that the inode's file_ipns_key_encrypted_hex and file_ipns_private_key are None (since the provided FilePointer has no ipns_private_key_encrypted and the provided private_key is a zeroed vector and HKDF would not produce a valid value). Update the assertions near the InodeKind::File match to explicitly check these two fields to lock in expected behavior.apps/desktop/src-tauri/src/fuse/mod.rs (1)
462-521: ECIES wrapping result for legacy files is not cached back to the inode.When
file_ipns_key_encrypted_hexisNonebutfile_ipns_private_keyisSome(HKDF-fallback / legacy files), the ECIES-wrapped hex is computed on everybuild_folder_metadatacall but never stored back into the inode'sfile_ipns_key_encrypted_hex. This means every metadata publish for a legacy file triggers a redundant ECIESwrap_keyoperation.Consider caching the result after the first wrap to avoid repeated ECIES operations. Since
build_folder_metadatatakes&self, you'd need interior mutability or a post-publish cache update.♻️ Example: cache after build
One approach is to collect the
(ino, hex)pairs produced duringbuild_folder_metadataand apply them afterward:// After build_folder_metadata returns successfully, update inode cache: // (requires &mut self, so call from update_folder_metadata / flush_publish_queue) if let Some(inode) = self.inodes.get_mut(child_ino) { if let InodeKind::File { ref mut file_ipns_key_encrypted_hex, .. } = inode.kind { if file_ipns_key_encrypted_hex.is_none() { *file_ipns_key_encrypted_hex = Some(computed_hex); } } }Based on learnings: the PR summary mentions "ECIES caching" as an addressed security fix, and
build_folder_metadatais called on every publish (debounced or immediate).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src-tauri/src/fuse/mod.rs` around lines 462 - 521, build_folder_metadata computes ECIES-wrapped hex for legacy files (when file_ipns_key_encrypted_hex is None but file_ipns_private_key is Some) but never writes that result back to the inode, causing repeated wrap_key calls; collect the newly-computed (ino, hex) pairs inside build_folder_metadata (or return them) and after a successful publish/update call mutate the inode to set file_ipns_key_encrypted_hex (locate InodeKind::File handling, file_ipns_key_encrypted_hex, file_ipns_private_key and crate::crypto::ecies::wrap_key) — update self.inodes (or via the caller like update_folder_metadata / flush_publish_queue) to store the hex so future build_folder_metadata calls skip wrap_key. Ensure synchronization (interior mutability or caller-held &mut self) when applying the cache updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/operations.rs`:
- Around line 969-990: The current create() flow generates a per-file Ed25519
keypair and attempts to ECIES-wrap the IPNS private key using
crate::crypto::ecies::wrap_key, but on wrap failure it only logs a warning and
continues with ipns_key_encrypted_hex = None, risking irreversible loss of the
random IPNS key if later re-wrap also fails; change this so that a wrap_key()
Err is treated as fatal in the create() function: in the Err(e) branch for
wrap_key replace the log::warn+None path with a log::error that includes the
error, then call reply.error(libc::EIO) (or otherwise propagate the error) and
return, ensuring the code does not proceed to create metadata without an
encrypted IPNS key (also note build_folder_metadata will then not be invoked
with a missing ipnsPrivateKeyEncrypted).
---
Outside diff comments:
In @.planning/STATE.md:
- Around line 164-174: Update the "Pending Todos" section in .planning/STATE.md
to include the two new pending todo files introduced by the PR:
`2026-02-21-desktop-tee-enrollment-for-new-files.md` and
`2026-02-21-lazy-migration-legacy-file-pointers.md`, add appropriate brief
descriptions/area tags consistent with the existing entries, and bump the
pending count from 7 to 9 so the list and count reflect the actual files
present.
---
Nitpick comments:
In `@apps/desktop/src-tauri/src/fuse/inode.rs`:
- Around line 804-843: The test test_populate_folder_with_file_pointers should
also assert that new fields related to IPNS keys remain None for legacy
FilePointer entries: after calling InodeTable::populate_folder and fetching the
child inode (created from the FolderChild::File FilePointer), add assertions
that the inode's file_ipns_key_encrypted_hex and file_ipns_private_key are None
(since the provided FilePointer has no ipns_private_key_encrypted and the
provided private_key is a zeroed vector and HKDF would not produce a valid
value). Update the assertions near the InodeKind::File match to explicitly check
these two fields to lock in expected behavior.
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Around line 462-521: build_folder_metadata computes ECIES-wrapped hex for
legacy files (when file_ipns_key_encrypted_hex is None but file_ipns_private_key
is Some) but never writes that result back to the inode, causing repeated
wrap_key calls; collect the newly-computed (ino, hex) pairs inside
build_folder_metadata (or return them) and after a successful publish/update
call mutate the inode to set file_ipns_key_encrypted_hex (locate InodeKind::File
handling, file_ipns_key_encrypted_hex, file_ipns_private_key and
crate::crypto::ecies::wrap_key) — update self.inodes (or via the caller like
update_folder_metadata / flush_publish_queue) to store the hex so future
build_folder_metadata calls skip wrap_key. Ensure synchronization (interior
mutability or caller-held &mut self) when applying the cache updates.
In `@packages/crypto/src/file/derive-ipns.ts`:
- Around line 1-17: Update the file JSDoc to describe both available key
derivation methods: the existing deterministic HKDF-based flow used by
deriveFileIpnsKeypair (HKDF-SHA256 with salt "CipherBox-v1" and info
"cipherbox-file-ipns-v1:{fileId}" → 32-byte Ed25519 seed → keypair → IPNS name)
and the new random generation provided by generateFileIpnsKeypair (random
Ed25519 keypair → IPNS name). Mention that both functions coexist in this
module, note their different use-cases (deterministic per-file vs random), and
keep the domain-separation note distinguishing this domain from vault and
registry derivations.
In `@packages/crypto/src/folder/metadata.ts`:
- Around line 74-85: Update the ipnsPrivateKeyEncrypted validation to use the
canonical ECIES minimum length and fix the comment: import
ECIES_MIN_CIPHERTEXT_SIZE from the package and replace the hardcoded 64 check
with a hex-length check of ECIES_MIN_CIPHERTEXT_SIZE * 2 (since hex chars are 2
per byte); also change the inline comment to accurately state "must be a hex
string at least ECIES_MIN_CIPHERTEXT_SIZE bytes (hex length
ECIES_MIN_CIPHERTEXT_SIZE*2)". Keep the CryptoError thrown (with same code
'DECRYPTION_FAILED') and ensure this prevents passing values that would later
fail in unwrapKey().
When getFileIpnsPrivateKey() falls back to HKDF derivation for legacy FilePointers lacking ipnsPrivateKeyEncrypted, the derived key is now ECIES-wrapped with the user's public key and returned to the caller. The caller (useFolder hooks) persists the wrapped key by updating the FilePointer in folder metadata via a fire-and-forget folder re-publish. This gradually migrates all actively-used files to random IPNS keys, allowing eventual removal of the HKDF derivation code path. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: eb87c793767c
Add public_key parameter to populate_folder so that when HKDF fallback derives an IPNS key for a legacy FilePointer, the key is ECIES-wrapped and cached in the inode's file_ipns_key_encrypted_hex. The next folder metadata publish persists the wrapped key in the FilePointer, migrating the file to the random-key format. Also fix 3 test compilation errors (missing ipns_private_key_encrypted field) and add backward/forward compatibility tests per the Metadata Evolution Protocol (Section 6.1). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 26e64a4fa8be
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/hooks/useFolder.ts (2)
832-847:⚠️ Potential issue | 🟡 MinorDuplicate
// 5.step labelLine 832 (
// 5. Update file metadata...) and line 847 (// 5. Publish only the file IPNS record...) both use the same step number. The second occurrence should start at// 6., shifting// 6.→// 7.and// 6b.→// 7b..🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolder.ts` around lines 832 - 847, The comments numbering is duplicated at the update/publish steps; update the inline step comments around the updateFileMetadata call and the subsequent publish block so they increment correctly: change the second "// 5. Publish only the file IPNS record..." to "// 6. Publish only the file IPNS record..." and then bump the subsequent "// 6." → "// 7." and "// 6b." → "// 7b." (search around the updateFileMetadata call and the publish logic that references fileIpnsPrivateKey, filePointer.fileMetaIpnsName, parentFolder.folderKey, and createVersion to find the exact comment locations).
821-844:⚠️ Potential issue | 🟠 Major
fileIpnsPrivateKeyis never zeroed after use — same issue exists inhandleRestoreVersionandhandleDeleteVersion
createFileMetadataestablishes the zeroing pattern (ipnsKeypair.privateKey.fill(0)) within the service. Here, the decrypted file IPNS private key remains in memory until the call stack unwinds. Wrapping the signing operations intry/finallymatches the established pattern and minimises the exposure window.🔐 Proposed fix (apply the same pattern in `handleRestoreVersion` and `handleDeleteVersion`)
- const { privateKey: fileIpnsPrivateKey, migratedIpnsPrivateKeyEncrypted } = - await getFileIpnsPrivateKey( - filePointer, - auth.vaultKeypair.privateKey, - auth.vaultKeypair.publicKey - ); - - // 4. Determine whether to create a version entry + const { privateKey: fileIpnsPrivateKey, migratedIpnsPrivateKeyEncrypted } = + await getFileIpnsPrivateKey( + filePointer, + auth.vaultKeypair.privateKey, + auth.vaultKeypair.publicKey + ); + + // 4. Determine whether to create a version entry const createVersion = shouldCreateVersion(currentMetadata, fileData.forceVersion ?? false); - // 5. Update file metadata and publish new IPNS record - const { ipnsRecord, prunedCids } = await updateFileMetadata({ - fileIpnsPrivateKey, - ... - }); + let ipnsRecord: FileIpnsRecordPayload; + let prunedCids: string[]; + try { + ({ ipnsRecord, prunedCids } = await updateFileMetadata({ + fileIpnsPrivateKey, + fileMetaIpnsName: filePointer.fileMetaIpnsName, + folderKey: parentFolder.folderKey, + currentMetadata, + updates: { ... }, + createVersion, + })); + } finally { + fileIpnsPrivateKey.fill(0); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolder.ts` around lines 821 - 844, The decrypted file IPNS private key returned from getFileIpnsPrivateKey (assigned to fileIpnsPrivateKey) is never zeroed after use — wrap the code that uses fileIpnsPrivateKey (the call to updateFileMetadata and any signing operations) in a try/finally and in the finally call fileIpnsPrivateKey.fill(0) (matching the pattern used in createFileMetadata where ipnsKeypair.privateKey.fill(0)); apply the same try/finally zeroing pattern in the sibling handlers handleRestoreVersion and handleDeleteVersion so the private key is wiped as soon as operations complete.
🧹 Nitpick comments (3)
apps/web/src/hooks/useFolder.ts (2)
872-888: Lazy migration fire-and-forget block is duplicated three times — extract to a helperThe same ~15-line pattern (null-guard →
folderService.updateFolderMetadata→.then(updateSequence)→.catch(warn)) appears verbatim inhandleUpdateFile,handleRestoreVersion, andhandleDeleteVersion.♻️ Suggested helper
function triggerLazyMigrationPublish( parentId: string, parentFolder: FolderNode, updatedChildren: FolderChild[] ): void { folderService .updateFolderMetadata({ folderId: parentFolder.id, children: updatedChildren, folderKey: parentFolder.folderKey, ipnsPrivateKey: parentFolder.ipnsPrivateKey, ipnsName: parentFolder.ipnsName, sequenceNumber: parentFolder.sequenceNumber, }) .then(({ newSequenceNumber }) => { useFolderStore.getState().updateFolderSequence(parentId, newSequenceNumber); }) .catch((err) => { console.warn('Lazy IPNS key migration: folder re-publish failed, will retry:', err); }); }Each caller reduces to:
if (migratedIpnsPrivateKeyEncrypted) { triggerLazyMigrationPublish(parentId, parentFolder, updatedChildren); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolder.ts` around lines 872 - 888, The repeated lazy-migration pattern in handleUpdateFile, handleRestoreVersion, and handleDeleteVersion (null-guard → folderService.updateFolderMetadata → .then(updateFolderSequence) → .catch(console.warn)) should be extracted into a single helper function (e.g., triggerLazyMigrationPublish) that accepts parentId, parentFolder, and updatedChildren; replace each duplicated block with a guard if (migratedIpnsPrivateKeyEncrypted) { triggerLazyMigrationPublish(parentId, parentFolder, updatedChildren); } and have the helper call folderService.updateFolderMetadata(...) and on success call useFolderStore.getState().updateFolderSequence(parentId, newSequenceNumber) and on failure console.warn the error.
724-732: CapturepublicKeybeforePromise.allto avoid the non-null assertion
auth.vaultKeypairis checked for null before this block, but TypeScript can't narrow it inside the asyncPromise.allcallback, hence the!. Extracting it once before the async boundary is safer and consistent withhandleAddFile(line 649).♻️ Proposed fix
+ const userPublicKey = auth.vaultKeypair.publicKey; + const filesWithRecords = await Promise.all( filesData.map(async (f) => { const fileId = crypto.randomUUID(); const { ipnsRecord, ipnsPrivateKeyEncrypted } = await createFileMetadata({ ... - userPublicKey: auth.vaultKeypair!.publicKey, + userPublicKey, ... });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/hooks/useFolder.ts` around lines 724 - 732, Capture auth.vaultKeypair.publicKey into a local const before entering the async/Promise.all boundary (e.g., const userPublicKey = auth.vaultKeypair.publicKey) and replace the non-null assertion usage auth.vaultKeypair!.publicKey inside the createFileMetadata call with that local userPublicKey; update the code in useFolder (the block that calls createFileMetadata inside Promise.all) to use userPublicKey to avoid the null-assertion and ensure type-safety consistent with handleAddFile.apps/web/src/services/file-metadata.service.ts (1)
56-92: Document caller responsibility to zero the returnedprivateKey
createFileMetadataexplicitly zeros its keypair (line 181), establishing a clear in-module convention.getFileIpnsPrivateKeytransfers ownership of a live private key buffer to the caller but doesn't document that the caller must zero it after use. Adding a@returnsnote (or a@remarks) makes the lifetime contract explicit and prevents the gap seen in the callers.📝 Suggested JSDoc addition
* `@returns` Decrypted Ed25519 IPNS private key, plus migration data if HKDF fallback was used + * `@remarks` The caller is responsible for zeroing `privateKey` with `.fill(0)` after use. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/services/file-metadata.service.ts` around lines 56 - 92, Update the JSDoc for getFileIpnsPrivateKey to document the ownership and lifecycle of the returned privateKey buffer: state that the function returns a live Uint8Array containing the Ed25519 IPNS private key (and optional migratedIpnsPrivateKeyEncrypted string) and that the caller is responsible for zeroing/clearing the privateKey buffer after use (mirroring the convention in createFileMetadata where the keypair is explicitly zeroed); add this as an explicit `@returns` note or `@remarks` so callers know to securely wipe the buffer once finished.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/web/src/hooks/useFolder.ts`:
- Around line 832-847: The comments numbering is duplicated at the
update/publish steps; update the inline step comments around the
updateFileMetadata call and the subsequent publish block so they increment
correctly: change the second "// 5. Publish only the file IPNS record..." to "//
6. Publish only the file IPNS record..." and then bump the subsequent "// 6." →
"// 7." and "// 6b." → "// 7b." (search around the updateFileMetadata call and
the publish logic that references fileIpnsPrivateKey,
filePointer.fileMetaIpnsName, parentFolder.folderKey, and createVersion to find
the exact comment locations).
- Around line 821-844: The decrypted file IPNS private key returned from
getFileIpnsPrivateKey (assigned to fileIpnsPrivateKey) is never zeroed after use
— wrap the code that uses fileIpnsPrivateKey (the call to updateFileMetadata and
any signing operations) in a try/finally and in the finally call
fileIpnsPrivateKey.fill(0) (matching the pattern used in createFileMetadata
where ipnsKeypair.privateKey.fill(0)); apply the same try/finally zeroing
pattern in the sibling handlers handleRestoreVersion and handleDeleteVersion so
the private key is wiped as soon as operations complete.
---
Nitpick comments:
In `@apps/web/src/hooks/useFolder.ts`:
- Around line 872-888: The repeated lazy-migration pattern in handleUpdateFile,
handleRestoreVersion, and handleDeleteVersion (null-guard →
folderService.updateFolderMetadata → .then(updateFolderSequence) →
.catch(console.warn)) should be extracted into a single helper function (e.g.,
triggerLazyMigrationPublish) that accepts parentId, parentFolder, and
updatedChildren; replace each duplicated block with a guard if
(migratedIpnsPrivateKeyEncrypted) { triggerLazyMigrationPublish(parentId,
parentFolder, updatedChildren); } and have the helper call
folderService.updateFolderMetadata(...) and on success call
useFolderStore.getState().updateFolderSequence(parentId, newSequenceNumber) and
on failure console.warn the error.
- Around line 724-732: Capture auth.vaultKeypair.publicKey into a local const
before entering the async/Promise.all boundary (e.g., const userPublicKey =
auth.vaultKeypair.publicKey) and replace the non-null assertion usage
auth.vaultKeypair!.publicKey inside the createFileMetadata call with that local
userPublicKey; update the code in useFolder (the block that calls
createFileMetadata inside Promise.all) to use userPublicKey to avoid the
null-assertion and ensure type-safety consistent with handleAddFile.
In `@apps/web/src/services/file-metadata.service.ts`:
- Around line 56-92: Update the JSDoc for getFileIpnsPrivateKey to document the
ownership and lifecycle of the returned privateKey buffer: state that the
function returns a live Uint8Array containing the Ed25519 IPNS private key (and
optional migratedIpnsPrivateKeyEncrypted string) and that the caller is
responsible for zeroing/clearing the privateKey buffer after use (mirroring the
convention in createFileMetadata where the keypair is explicitly zeroed); add
this as an explicit `@returns` note or `@remarks` so callers know to securely wipe
the buffer once finished.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/fuse/inode.rs`:
- Around line 393-474: The current logic falls back to HKDF whenever decryption
fails, even if file_pointer.ipns_private_key_encrypted is present; modify the
flow so HKDF is used only for legacy pointers (when ipns_private_key_encrypted
is None). Concretely: when attempting to decode/decrypt the encrypted hex (in
the block that assigns file_ipns_key from
file_pointer.ipns_private_key_encrypted), treat any decode/decrypt failure as
unrecoverable and keep file_ipns_key as None but also record that an encrypted
value was present (e.g., a flag or check
file_pointer.ipns_private_key_encrypted.is_some()); then change the HKDF branch
to run only when file_pointer.ipns_private_key_encrypted.is_none() and
private_key is 32 bytes (call sites: file_ipns_key, the HKDF call
crypto::hkdf::derive_file_ipns_keypair, and references to
file_pointer.ipns_private_key_encrypted); finally ensure cached_encrypted_hex
does not wrap/encode an HKDF-derived key when an encrypted value was present but
invalid (only wrap when the original field was None).
- Zero fileIpnsPrivateKey via try/finally in handleUpdateFile, handleRestoreVersion, and handleDeleteVersion (security) - Make ECIES wrap_key failure fatal in desktop create() to prevent unrecoverable loss of random IPNS keys - Use ECIES_MIN_CIPHERTEXT_SIZE constant for validation instead of hardcoded 64 - Add @remarks to getFileIpnsPrivateKey documenting caller zeroing responsibility - Fix duplicate step comment numbers in handleUpdateFile - Capture publicKey before Promise.all to avoid non-null assertion - Update derive-ipns.ts file header to document both key strategies - Add test assertions for new inode fields in populate test Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 0da54a18fcd7
…encrypted key When ipnsPrivateKeyEncrypted is present but decryption fails, HKDF derivation would produce a key that doesn't match the file's random IPNS name, causing invalid per-file IPNS publishes. Now HKDF fallback is gated on the field being absent (legacy files only). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Entire-Checkpoint: 5cfa8adc7a26
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
ipnsPrivateKeyEncryptedin the FilePointer (inside AES-GCM encrypted folder metadata)Motivation
The deterministic HKDF design was justified by "self-sovereign recovery" but recovery already traverses the folder tree via
fileMetaIpnsName— it never derives IPNS names independently. The asymmetry with folder keys (which are already random) also blocks future per-file write-sharing. This aligns file IPNS key management with the existing folder pattern.Changes
Crypto Package
generateFileIpnsKeypair()— random Ed25519 keypair + IPNS nameipnsPrivateKeyEncryptedtoFilePointertypevalidateFolderMetadata()Web App
createFileMetadata()uses random keygen + ECIES wrap withuserPublicKeygetFileIpnsPrivateKey()helper — decrypts from FilePointer or HKDF fallbackupdateFileMetadata(),restoreVersion(),deleteVersion()accept decrypted keyaddFileToFolder()/addFilesToFolder()store wrapped key in FilePointerDesktop (Rust)
create()usesed25519_dalek::SigningKey::generate(OsRng)instead of HKDFpopulate_folder()decrypts ECIES-wrapped key, falls back to HKDF for legacy filesbuild_folder_metadata()uses cached wrapped hex to avoid redundant ECIES re-wrappingfile_ipns_key_encrypted_hexcache field toInodeKind::FileSecurity Review
.planning/security/REVIEW-2026-02-21-random-ipns-keys.mdTest plan
pnpm --filter @cipherbox/crypto test— 230/230 passpnpm --filter web build— cleancargo build(desktop) — cleanipnsPrivateKeyEncrypted🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests