fix: harden IPNS signedRecord validation, verification, and key zeroization#529
Merged
Conversation
Entire-Checkpoint: 3af957aad5e8
Entire-Checkpoint: 63ac3e2a0b38
Entire-Checkpoint: 3972a20de96b
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- CID mismatch throws BadRequestException (400) - Non-first-publish sequence mismatch throws BadRequestException (400) - First-publish tolerates embedded seq 0n and 1n - Valid CID+seq pass-through succeeds - Anti-rollback ConflictException (409) regression guard
…dation - Add BadRequestException (400) when signedRecord embedded CID differs from metadataCid DTO (strict check via /ipfs/ regex extraction) - Add offset-aware BadRequestException (400) when embedded sequence disagrees with expectedSequenceNumber: first-publish tolerates 0n/1n difference, subsequent publishes require exactly expectedSeq+1n - Reuse already-parsed incoming record from anti-rollback block (parseIpnsRecord called at most once per request) - CAS ConflictException (409) fires before S1 sequence check to preserve concurrent-modification semantics - Fix pre-existing tests: align parseIpnsRecord mock embedded CID and sequence with DTO values now that S1 validates them (Rule 1 auto-fix)
- 79/79 tests pass; S1 CID + offset-aware sequence gates green - 2 Rule-1 auto-fixes documented (CAS ordering, test mock alignment)
- Add 6 vitest cases covering fail-closed behavior for resolveIpnsRecord - Tests 1-2 (present-but-invalid throw, key-substitution throw) FAIL as expected - Tests 3-6 (absent-fields allow, valid pass, 404 null, non-404 propagate) pass - Uses .test.ts suffix required by web vitest include pattern
- Remove swallowing try/catch from inner verification block - Throw on present-but-invalid signature (D-02, S2 fix) - Throw on pubKey to ipnsName mismatch (D-02, key substitution guard) - Keep allow+flag path for absent signature fields (D-03, legacy compat) - Leave outer 404 catch unchanged so only 404 maps to null - Mirrors sdk-core canonical behavior exactly
- Add cipherbox-crypto workspace dep to crates/api-client/Cargo.toml - Add signature_v2, data, pub_key Option<String> fields to IpnsResolveResponse - Add stub verify_ipns_resolve_signature (unimplemented!) with 5 failing tests - Tests: deserialize sig fields, absent→None, invalid→Some(false), valid→Some(true), name mismatch→Some(false)
- D-03: Ok(None) when any sig field absent (backward-compat allow+flag) - D-02: Ok(Some(false)) on invalid Ed25519 signature (fail-closed) - D-04: Ok(Some(true)) on valid sig + pubKey derives to matching IPNS name - Ok(Some(false)) on pubKey length != 32 or name mismatch - All 6 unit tests pass (GREEN)
S3/D-05: close raw Vec<u8> key escapes across Rust crates - ecies::unwrap_key now returns Zeroizing<Vec<u8>> (zeroize on drop) - resolve_folder_key BFS queue is VecDeque<(String, Zeroizing<Vec<u8>>)> - get_folder_key returns Option<Zeroizing<Vec<u8>>> - spawn_file_meta_reencrypt params source/dest_folder_key are Zeroizing - UploadJournalResult.folder_key_for_file_meta is Option<Zeroizing<Vec<u8>>> - Remove redundant Zeroizing::new() wrappers around unwrap_key call sites - Update type annotations in write_ops and windows/write_ops - Fix cross_language.rs and ecies unit tests for new return type
S2/D-04: gate resolve_folder_key BFS descent on signature check - Call verify_ipns_resolve_signature after each resolve_ipns in BFS - Ok(None): warn and continue (D-03, absent fields = legacy record) - Ok(Some(false)): return Err, fail closed (D-02, invalid signature) - Ok(Some(true)): proceed (signature valid + name binding confirmed) - Err: return Err surfacing the verification failure cargo test -p cipherbox-api-client -p cipherbox-fuse: 66 passed, 0 failed
- Add 51-03-SUMMARY.md with task outcomes, test results, deviations - Update STATE.md plan counter (3→4), metrics, key decisions
…on guard - Test A: assert ipnsPrivateKey buffer zeroed after successful createAndPublishIpnsRecord - Test B: assert ipnsPrivateKey buffer zeroed even when publish throws (finally path) - Test C: assert vaultKeyKeypair.privateKey zeroed after publishVaultKeyBlob - Test D: S2 regression guard - resolveIpnsRecord throws on present-but-invalid signature - Tests A/B/C fail RED (no fill(0) yet); Test D passes (S2 already correct) S3/D-05 enforcement guard per T-47-01 caller-owns-key convention
…ultKeyBlob - Wrap createAndPublishIpnsRecord body in try/finally; finally zeroes params.ipnsPrivateKey - Wrap publishVaultKeyBlob publish logic in try/finally; finally zeroes vaultKeyKeypair.privateKey - Both functions are the terminal owners of their respective key buffers (D-05/T-47-01) - S3 zeroization guard tests A/B/C now pass GREEN; S2 regression test D still passes
…ecision - Add T-47-01/D-05 skip comment to folder/index.ts: caller (client.ts folderTree) retains ownership of ipnsPrivateKey and folderKey across the session lifetime - Add SKIP guard test to folder.test.ts: asserts keys are UNCHANGED after return, documenting deliberate non-zeroing and preventing accidental future fill(0) - Contrast with updateFileMetadata which DOES zero (per-use key, terminal consumer) - Full sdk-core suite: 209 tests passing
- 51-04-SUMMARY.md: zeroization convention applied to ipns + vault paths - STATE.md: advance to plan 4/4 complete, add 51-04 decision - Full sdk-core suite: 209 tests passing
Re-stage cargo fmt reformatting of the in-scope 51-03 files that a later pre-commit hook left as uncommitted drift in the working tree. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
S1/S2/S3 hardening for HARD-02 verified: API embedded-vs-DTO 400, web + Rust fail-closed IPNS verification, sdk-core + Rust key zeroization. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t_key Entire-Checkpoint: 0ddf318e81ed
Entire-Checkpoint: ecf81ac2076b
Entire-Checkpoint: e0f0794d988f
Entire-Checkpoint: 530961db1d56
Completes Phase 51 D-05 exhaustive zeroization with three defense-in-depth memory-hygiene fixes flagged by CodeRabbit: - SDK state root_folder_key field is now Option<Zeroizing<Vec<u8>>> instead of plain Vec, so the decrypted root key is wiped on drop. Desktop vault.rs stores the unwrap_key Zeroizing directly; mount_filesystem (macOS + winfsp) takes Zeroizing and no longer re-wraps. - replay_for_vault folder-key cache is HashMap<String, Zeroizing<Vec<u8>>>, so cached folder keys are zeroized when the cache drops at end of replay. - spawn_file_meta_reencrypt wraps its fixed-size [u8; 32] source/dest key copies in Zeroizing. root_ipns_private_key is out of scope (not flagged, HKDF-derived). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: beeb90918390
Wrap transient [u8; 32] key arrays built via try_into() from decrypted key material (file keys, folder keys, IPNS keys) in zeroize::Zeroizing so plaintext key bytes are wiped on scope exit instead of lingering on the stack. Mirrors the established spawn_file_meta_reencrypt pattern. Applies to both the macOS/Linux FUSE and Windows WinFsp operation paths, keeping the two platforms in lockstep. Completes the Phase 51 D-05 exhaustive zeroization sweep. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: b05ee7305656
github-actions
Bot
added
the
release:tee-worker:fix
Contributor
Release Preview
Cascade Details
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (57.93%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #529 +/- ##
==========================================
+ Coverage 64.78% 67.02% +2.23%
==========================================
Files 143 159 +16
Lines 11132 18187 +7055
Branches 1252 1258 +6
==========================================
+ Hits 7212 12190 +4978
- Misses 3676 5752 +2076
- Partials 244 245 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
FSM1
force-pushed
the
feat/crypto-signature-secret-leak-hardening
branch
from
d8ca746 to
b90cbb1
Compare
github-actions
Bot
added
release:api:feat
Greptile SummaryThis PR closes three deferred IPNS security findings from the #448 review: publish-time embedded-CID and sequence validation (S1), fail-closed signature verification in web/SDK-core (S2), and Rust signature verification plus exhaustive key zeroization (S3). It also fixes an SDK E2E regression introduced by a callee incorrectly zeroing a caller-owned key buffer.
Confidence Score: 5/5Safe to merge. The changes are defense-in-depth hardening with no regressions: the SDK E2E callee-zeroing bug is fixed, all 89 SDK E2E and 893 API tests pass, and the fail-closed logic is consistently applied across all three layers. The three IPNS security findings are addressed with matching tests at every layer (API spec, web vitest, sdk-core vitest, Rust unit tests). The zeroization cascade is complete and the caller-owns-key contract is correctly documented and guard-tested. No logic errors were found in the S1 ordering (anti-rollback → CAS → CID → seq), the S2 partial-field fail-closed path, or the Rust BFS verification integration. No files require special attention. The inode.rs and operations.rs changes are formatting-only or straightforward Zeroizing propagation consistent with the rest of the sweep. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Client
participant API as API (ipns.service.ts)
participant Web as Web/SDK-core (resolveIpnsRecord)
participant FUSE as FUSE (resolve_folder_key)
participant Crypto as crates/crypto (unwrap_key)
Note over Client,API: S1 — Publish-time validation
Client->>API: publishRecord(signedRecord, metadataCid, expectedSeq)
API->>API: parseIpnsRecord → incomingParsed
API->>API: "Anti-rollback: incoming.seq >= stored.seq (409 on regress)"
API->>API: "CAS: expectedSeq == DB seq (409 on mismatch)"
API->>API: "S1 CID: incomingParsed.value == /ipfs/metadataCid (400 on mismatch)"
API->>API: "S1 seq: embedded == expectedSeq+1 (400 on mismatch)"
API-->>Client: 200 OK / 400 / 409
Note over Web,Crypto: S2 — Resolve fail-closed (web + sdk-core)
Web->>Web: resolveIpnsRecord(ipnsName)
Web->>Web: Check sig fields: all absent → warn+allow (D-03)
Web->>Web: Partial fields → throw (downgrade attack)
Web->>Web: verifyIpnsSignature(sig, data, pubKey)
Web->>Web: Invalid sig → throw (D-02)
Web->>Web: "deriveIpnsName(pubKey) != ipnsName → throw"
Web-->>Web: "return { cid, signatureVerified: true/false }"
Note over FUSE,Crypto: S3 — Rust verify + zeroization
FUSE->>FUSE: resolve_folder_key BFS loop
FUSE->>FUSE: verify_ipns_resolve_signature(resp, current_ipns)
FUSE->>FUSE: Ok(None) → warn+continue (D-03)
FUSE->>FUSE: Ok(Some(false)) → Err (fail closed)
FUSE->>Crypto: "unwrap_key(enc_key, priv_key) → Zeroizing<Vec<u8>>"
Crypto-->>FUSE: "Zeroizing<Vec<u8>> (zeroed on drop)"
FUSE->>FUSE: BFS queue / cache hold Zeroizing keys
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant Client
participant API as API (ipns.service.ts)
participant Web as Web/SDK-core (resolveIpnsRecord)
participant FUSE as FUSE (resolve_folder_key)
participant Crypto as crates/crypto (unwrap_key)
Note over Client,API: S1 — Publish-time validation
Client->>API: publishRecord(signedRecord, metadataCid, expectedSeq)
API->>API: parseIpnsRecord → incomingParsed
API->>API: "Anti-rollback: incoming.seq >= stored.seq (409 on regress)"
API->>API: "CAS: expectedSeq == DB seq (409 on mismatch)"
API->>API: "S1 CID: incomingParsed.value == /ipfs/metadataCid (400 on mismatch)"
API->>API: "S1 seq: embedded == expectedSeq+1 (400 on mismatch)"
API-->>Client: 200 OK / 400 / 409
Note over Web,Crypto: S2 — Resolve fail-closed (web + sdk-core)
Web->>Web: resolveIpnsRecord(ipnsName)
Web->>Web: Check sig fields: all absent → warn+allow (D-03)
Web->>Web: Partial fields → throw (downgrade attack)
Web->>Web: verifyIpnsSignature(sig, data, pubKey)
Web->>Web: Invalid sig → throw (D-02)
Web->>Web: "deriveIpnsName(pubKey) != ipnsName → throw"
Web-->>Web: "return { cid, signatureVerified: true/false }"
Note over FUSE,Crypto: S3 — Rust verify + zeroization
FUSE->>FUSE: resolve_folder_key BFS loop
FUSE->>FUSE: verify_ipns_resolve_signature(resp, current_ipns)
FUSE->>FUSE: Ok(None) → warn+continue (D-03)
FUSE->>FUSE: Ok(Some(false)) → Err (fail closed)
FUSE->>Crypto: "unwrap_key(enc_key, priv_key) → Zeroizing<Vec<u8>>"
Crypto-->>FUSE: "Zeroizing<Vec<u8>> (zeroed on drop)"
FUSE->>FUSE: BFS queue / cache hold Zeroizing keys
Reviews (4): Last reviewed commit: "fix: close empty-string signature downgr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/fuse/src/operations.rs (1)
224-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not record a successful publish after a conflict.
If
publish_ipnsreturnsConflict, the per-file IPNS update was not accepted, but this function still falls through tocoordinator.record_publish(file_ipns_name, new_seq)and logs success. Return an error or retry instead, otherwise the local sequence cache can advance past the server and mask a lost metadata publish.Proposed fix
match cipherbox_api_client::ipns::publish_ipns(api, &req) .await .map_err(|e| format!("{}", e))? { cipherbox_api_client::PublishResult::Success => {} - cipherbox_api_client::PublishResult::Conflict { .. } => { - log::warn!( - "Unexpected conflict on per-file IPNS publish for {}", - file_ipns_name - ); + cipherbox_api_client::PublishResult::Conflict { + current_sequence_number, + } => { + return Err(format!( + "per-file IPNS publish conflict for {} (server sequence {})", + file_ipns_name, current_sequence_number + )); } } coordinator.record_publish(file_ipns_name, new_seq);🤖 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 `@crates/fuse/src/operations.rs` around lines 224 - 237, When publish_ipns returns PublishResult::Conflict in the match statement, the code currently logs a warning but then falls through to call coordinator.record_publish, which incorrectly marks the publish as successful even though the server rejected it. Modify the Conflict branch to return an error or implement a retry mechanism instead of continuing, ensuring that coordinator.record_publish is only called for the Success case to prevent the local sequence cache from advancing beyond what the server has accepted.crates/fuse/src/platform/windows/operations.rs (1)
383-397:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn on per-file IPNS conflicts before recording success.
The
Conflictarm only logs, then Line 396 recordsnew_seqand Line 397 logs success. If the server rejected the publish, the coordinator advances to a sequence that was never accepted and the new FileMetadata CID can be lost.🐛 Proposed fix
match cipherbox_api_client::ipns::publish_ipns(api, &req) .await .map_err(|e| e.to_string())? { - cipherbox_api_client::PublishResult::Success => {} - cipherbox_api_client::PublishResult::Conflict { .. } => { - log::warn!( - "Unexpected conflict on per-file IPNS publish for {}", - file_ipns_name - ); + cipherbox_api_client::PublishResult::Success => { + coordinator.record_publish(file_ipns_name, new_seq); + log::info!("Per-file IPNS publish succeeded for {}", file_ipns_name); + } + cipherbox_api_client::PublishResult::Conflict { current_sequence_number } => { + return Err(format!( + "Per-file IPNS publish conflict for {}: server has sequence {}", + file_ipns_name, current_sequence_number + )); } } - - coordinator.record_publish(file_ipns_name, new_seq); - log::info!("Per-file IPNS publish succeeded for {}", file_ipns_name);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/fuse/src/platform/windows/operations.rs` around lines 383 - 397, The match statement handling the PublishResult for IPNS publish needs to be modified to return early on conflict. Currently, when a PublishResult::Conflict occurs, the code logs a warning but then continues to execute coordinator.record_publish and the success log message. This means the coordinator advances the sequence number even though the server rejected the publish, potentially losing the new FileMetadata CID. Add a return statement in the Conflict arm of the match statement to exit the function immediately after logging the conflict warning, preventing the subsequent record_publish call and success log from executing.crates/fuse/src/inode.rs (1)
399-412:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset identity-specific state when only the display-name fallback matches.
The new stable-ID lookup still falls back to
find_child(...), but later code treats that inode as the same folder/file identity. If remote metadata replacesfoowith a differentipns_name/file_meta_ipns_namebut keeps the same display name, folders can preserve stale loaded children, and resolved files can keep the old CID whenmodified_atis unchanged. Track whether the stable ID actually matched; if not, clear folder loaded state and force file re-resolution/key refresh.🐛 Proposed direction
- let existing_ino = ipns_to_ino + let matched_by_stable_id = ipns_to_ino.contains_key(&folder.ipns_name); + let existing_ino = ipns_to_ino .get(&folder.ipns_name) .copied() .or_else(|| self.find_child(parent_ino, &folder.name)); @@ - let (existing_children, was_loaded) = if existing_ino.is_some() { + let (existing_children, was_loaded) = if existing_ino.is_some() + && matched_by_stable_id + { let old = self.inodes.get(&ino); let ch = old.and_then(|o| o.children.clone());- if modified != existing.attr.mtime { + let pointer_changed = file_meta_ipns_name.as_deref() + != Some(file_pointer.file_meta_ipns_name.as_str()); + if pointer_changed || modified != existing.attr.mtime { log::info!( - "File '{}': modified_at changed (remote edit detected), marking for re-resolution", + "File '{}': identity or modified_at changed, marking for re-resolution", file_pointer.name ); (true, None)Also applies to: 461-475, 515-580
🤖 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 `@crates/fuse/src/inode.rs` around lines 399 - 412, The code currently treats any inode found by either the ipns_to_ino lookup or the find_child() fallback as the same identity, but when the ipns_name has changed (matched only by display name via find_child), the inode's identity has actually changed and identity-specific state must be reset. Distinguish between a match from ipns_to_ino (stable ID match) and a match only from find_child() (display name fallback), and when only the fallback matches, clear folder loaded state and force file re-resolution by refreshing the CID and metadata keys. Apply this fix consistently across all affected code sections that handle folder and file identity tracking, including the sections around lines 461-475 and 515-580.crates/fuse/src/lib.rs (1)
1717-1741:⚠️ Potential issue | 🟠 MajorKeep cached folder-key returns wrapped in
Zeroizing.Lines 1725 and 1741 clone the cached key into a plain
Vec<u8>, creating an unzeroized plaintext copy that persists until allocator reuse. ReturnZeroizing<Vec<u8>>instead and use.clone()to preserve the zeroizing wrapper. All call sites remain compatible: functions expecting&[u8]will receive it via deref coercion, and the test assertion will work throughPartialEq.🛡️ Proposed fix
async fn resolve_folder_key_cached( cache: &mut std::collections::HashMap<String, Zeroizing<Vec<u8>>>, api: &ApiClient, private_key: &[u8], root_folder_key: &[u8], root_ipns_name: &str, folder_ipns_name: &str, -) -> Result<Vec<u8>, String> { +) -> Result<Zeroizing<Vec<u8>>, String> { if let Some(key) = cache.get(folder_ipns_name) { - return Ok(key.to_vec()); + return Ok(key.clone()); } let key = resolve_folder_key( api, private_key, root_folder_key, root_ipns_name, folder_ipns_name, ) .await?; let cached = cache .entry(folder_ipns_name.to_string()) .or_insert_with(|| key.clone()); - Ok(cached.to_vec()) + Ok(cached.clone()) }🤖 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 `@crates/fuse/src/lib.rs` around lines 1717 - 1741, The function currently returns unzeroized plaintext copies of the cached key by calling `.to_vec()` on the `Zeroizing<Vec<u8>>` values at two locations (line 1725 when returning from cache and line 1741 when returning the resolved key). Change the return type of this function from `Result<Vec<u8>, String>` to `Result<Zeroizing<Vec<u8>>, String>`, and replace both `.to_vec()` calls with `.clone()` to preserve the zeroizing wrapper throughout the key's lifetime. Call sites will remain compatible because `Zeroizing<Vec<u8>>` can be automatically deref coerced to `&[u8]` where needed.
🧹 Nitpick comments (2)
packages/sdk-core/src/__tests__/ipns.test.ts (1)
201-221: ⚡ Quick winMake the S2 regression cover a distinct branch.
Lines 183-199 already assert the same present-fields +
verifyEd25519=falsefail-closed path. Either remove this duplicate or change it to cover a missing edge, such as partial signature fields, invalid base64 propagation, or verifyingderiveIpnsNameis not called after signature failure.As per coding guidelines,
**/*.test.tsshould 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-core/src/__tests__/ipns.test.ts` around lines 201 - 221, The test 'S2 regression: throws on present-but-invalid signature (fail-closed)' is duplicating the test coverage already established in lines 183-199, both testing the same scenario where signature fields are present but verifyEd25519 returns false. Either remove this duplicate test entirely, or modify it to cover a distinct missing edge case such as handling partial/incomplete signature fields, verifying invalid base64 encoding propagation, or confirming that deriveIpnsName is not called when signature verification fails. Ensure the test covers a meaningful scenario distinct from the existing test to maintain code quality standards.Source: Coding guidelines
packages/sdk-core/src/__tests__/vault.test.ts (1)
56-83: ⚡ Quick winAdd a rejected-path guard for the
finallyzeroization.This covers successful publish only, but the production contract says the derived private key is zeroed on all exit paths. Add a rejection case for
addToIpfsorcreateAndPublishIpnsRecordand assertprivateKeyBufis still cleared.Example failure-path coverage
+ it('D: zeroes the derived private key when publish fails', async () => { + const { deriveVaultKeyIpnsKeypair } = await import('`@cipherbox/crypto`'); + const privateKeyBuf = new Uint8Array(32).fill(0xcd); + + vi.mocked(deriveVaultKeyIpnsKeypair).mockResolvedValueOnce({ + ipnsName: 'k51vaultkey-fail', + publicKey: new Uint8Array(32).fill(1), + privateKey: privateKeyBuf, + }); + vi.mocked(addToIpfs).mockRejectedValueOnce(new Error('upload failed')); + + await expect( + publishVaultKeyBlob({ + userPrivateKey: new Uint8Array(32).fill(0x01), + userPublicKey: new Uint8Array(33).fill(0x02), + rootFolderKey: new Uint8Array(32).fill(0x03), + ctx: mockCtx, + }) + ).rejects.toThrow('upload failed'); + + expect(privateKeyBuf.every((b) => b === 0)).toBe(true); + });As per coding guidelines,
**/*.test.tsshould focus on 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-core/src/__tests__/vault.test.ts` around lines 56 - 83, The current test only verifies that the private key buffer is zeroed on the successful publish path, but the code should zero it on all exit paths including when operations fail. Add a new test case that mocks either addToIpfs or createAndPublishIpnsRecord to reject or throw an error, then calls publishVaultKeyBlob and verifies that the privateKeyBuf is still zeroed to zero bytes even when the operation fails. This ensures the finally block zeroization works correctly for both success and error paths.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 `@apps/api/src/ipns/ipns.service.spec.ts`:
- Around line 1491-1499: The test setup for mockParseIpnsRecord uses
mockResolvedValueOnce which depends on call execution order, but
Promise.allSettled only guarantees result order, causing test flakiness. Replace
the two chained mockResolvedValueOnce calls on mockParseIpnsRecord with a
mockImplementation that inspects the distinct record bytes (file vs folder)
passed as arguments and returns the appropriate sequence value based on the
record content rather than call order.
In `@apps/api/src/ipns/ipns.service.ts`:
- Around line 264-269: The validation in the IPNS record check is too permissive
because it only extracts and compares the first CID substring rather than
validating the entire signed value. Instead of using the regex to extract
embeddedCid and comparing it to metadataCid, construct the expected full path as
`/ipfs/${metadataCid}` and directly compare it to the complete
incomingParsed.value to ensure the full signed record matches exactly what will
be published, preventing divergence between database and network resolution.
- Around line 271-294: The sequence validation logic in the S1 sequence check
block only validates the embedded sequence when expectedSequenceNumber is
provided, but omitting this parameter should not skip validation entirely.
Extend the validation to also handle the case when expectedSequenceNumber is
undefined by using the DB sequence as the baseline: for first publish (when
existing is false), validate that incomingParsed.sequence equals 1n, and for
subsequent publishes, validate that incomingParsed.sequence equals the existing
record's sequence plus 1n. Additionally, add idempotent republish handling to
allow equal-sequence publishes without incrementing the stored sequence. This
ensures that arbitrary sequence numbers cannot be embedded when CAS is omitted,
preventing anti-rollback violations.
In `@apps/desktop/src-tauri/src/fuse/mod.rs`:
- Line 79: The root_folder_key and derived subfolder keys are being stored in
plain [u8; 32] arrays at lines 79, 201, and 239, which will not be securely
wiped from memory when dropped. Replace all occurrences of plain [u8; 32] arrays
used for storing these derived folder keys with Zeroizing<[u8; 32]> to ensure
the sensitive key material is properly zeroed out of memory, consistent with the
approach already used in crates/fuse/src/operations.rs.
In `@apps/web/src/services/__tests__/ipns.service.test.ts`:
- Around line 120-128: The test mocks an incorrect error structure by setting
error.status directly, but real Axios errors from the customInstance wrapper use
error.response.status instead. Update the existing test case to mock
error.response = { status: 404 } to match the actual client error shape.
Additionally, update the catch block in the resolveIpnsRecord function to check
both error.status and error.response?.status patterns by extracting the status
code with const status = anyError.status ?? anyError.response?.status before
comparing to 404. Finally, add a separate test case to verify that the fallback
error.status pattern is also handled correctly.
In `@apps/web/src/services/ipns.service.ts`:
- Around line 178-195: The current code in the signature verification block only
handles the case where all three fields (signatureV2, data, and pubKey) are
present, and otherwise treats the response as a legacy record. However,
responses with partial signature data (some but not all three fields) should be
explicitly rejected as incomplete/tampered rather than being treated as legacy.
Modify the logic to detect when any of the signature fields are present but not
all three are present, and throw an error for incomplete signature data in that
case. Only responses where all signature fields are completely absent should
fall through to the legacy path that sets signatureVerified to false.
In `@crates/api-client/src/ipns.rs`:
- Around line 71-112: The current implementation does not verify that the signed
CBOR data matches the response's cid and sequence_number fields, allowing
signature verification to succeed even if these fields were swapped.
Additionally, the current logic returns Ok(None) for any missing signature
field, but should only return Ok(None) if all three fields (signature_v2, data,
pub_key) are absent and fail closed for partial fields. Change the pattern
matching logic to explicitly check for the case where all three fields are
absent (returning Ok(None)) versus partially absent fields (returning
Ok(Some(false))), then after the successful signature verification check with
verify_ed25519, decode the CBOR data payload to extract the cid and
sequence_number fields and compare them against resp.cid and
resp.sequence_number. If they do not match, return Ok(Some(false)) to fail
closed.
In `@crates/fuse/src/platform/windows/operations.rs`:
- Around line 226-230: The code creates plain byte arrays via try_into() before
wrapping them in Zeroizing, leaving untracked sensitive key material in memory.
In the operations.rs file, refactor the file_key_arr assignment around line
226-230 and the folder_key_arr and ipns_key_arr assignments around lines 272-276
and 341-345 respectively to allocate the Zeroizing buffer first with a default
array, validate the slice length, then copy the key bytes into the zeroizing
buffer after the length check succeeds, ensuring all intermediate temporary
arrays are avoided and sensitive data remains within zeroized memory throughout.
In `@packages/sdk-core/src/__tests__/folder.test.ts`:
- Around line 439-443: The mock for createAndPublishIpnsRecord returns
sequenceNumber as a string value '1', but the actual SDK-core contract expects
sequenceNumber to be a bigint type. Locate the
mockFns.createAndPublishIpnsRecord.mockResolvedValueOnce call and change the
sequenceNumber property from the string '1' to a bigint representation (1n) to
match the actual contract and ensure the mock accurately reflects the runtime
behavior.
In `@packages/sdk-core/src/__tests__/ipns.test.ts`:
- Around line 263-272: The try/catch block in the test for
createAndPublishIpnsRecord with the invalid ipnsName parameter lacks an explicit
assertion to verify that an error is actually thrown. Replace the bare catch
block with a proper test assertion that explicitly checks that the promise
rejects (using expect().rejects or similar assertion syntax depending on your
test framework). This ensures the test fails if the function stops throwing an
error unexpectedly, rather than silently passing just because the key zeroing
logic executes.
In `@packages/sdk-core/src/ipns/index.ts`:
- Around line 98-102: The issue is that within the retry loop in
`publishWithCas`, the `ipnsPrivateKey` buffer is zeroed by the `finally` block
in the first `createAndPublishIpnsRecord` call (at lines 98-102), but when a 409
conflict triggers a retry, the same zeroed buffer is passed to the second
`createAndPublishIpnsRecord` call, causing signature failures. To fix this,
create a copy of `ipnsPrivateKey` before entering the retry loop in
`publishWithCas` and pass this copy to `createAndPublishIpnsRecord` on each
iteration, or alternatively create a fresh copy of `ipnsPrivateKey` for each
retry iteration within the loop. This ensures each attempt has an intact key
buffer while respecting the caller-owns-key convention where only the original
caller-provided buffer is ultimately responsible for zeroing.
---
Outside diff comments:
In `@crates/fuse/src/inode.rs`:
- Around line 399-412: The code currently treats any inode found by either the
ipns_to_ino lookup or the find_child() fallback as the same identity, but when
the ipns_name has changed (matched only by display name via find_child), the
inode's identity has actually changed and identity-specific state must be reset.
Distinguish between a match from ipns_to_ino (stable ID match) and a match only
from find_child() (display name fallback), and when only the fallback matches,
clear folder loaded state and force file re-resolution by refreshing the CID and
metadata keys. Apply this fix consistently across all affected code sections
that handle folder and file identity tracking, including the sections around
lines 461-475 and 515-580.
In `@crates/fuse/src/lib.rs`:
- Around line 1717-1741: The function currently returns unzeroized plaintext
copies of the cached key by calling `.to_vec()` on the `Zeroizing<Vec<u8>>`
values at two locations (line 1725 when returning from cache and line 1741 when
returning the resolved key). Change the return type of this function from
`Result<Vec<u8>, String>` to `Result<Zeroizing<Vec<u8>>, String>`, and replace
both `.to_vec()` calls with `.clone()` to preserve the zeroizing wrapper
throughout the key's lifetime. Call sites will remain compatible because
`Zeroizing<Vec<u8>>` can be automatically deref coerced to `&[u8]` where needed.
In `@crates/fuse/src/operations.rs`:
- Around line 224-237: When publish_ipns returns PublishResult::Conflict in the
match statement, the code currently logs a warning but then falls through to
call coordinator.record_publish, which incorrectly marks the publish as
successful even though the server rejected it. Modify the Conflict branch to
return an error or implement a retry mechanism instead of continuing, ensuring
that coordinator.record_publish is only called for the Success case to prevent
the local sequence cache from advancing beyond what the server has accepted.
In `@crates/fuse/src/platform/windows/operations.rs`:
- Around line 383-397: The match statement handling the PublishResult for IPNS
publish needs to be modified to return early on conflict. Currently, when a
PublishResult::Conflict occurs, the code logs a warning but then continues to
execute coordinator.record_publish and the success log message. This means the
coordinator advances the sequence number even though the server rejected the
publish, potentially losing the new FileMetadata CID. Add a return statement in
the Conflict arm of the match statement to exit the function immediately after
logging the conflict warning, preventing the subsequent record_publish call and
success log from executing.
---
Nitpick comments:
In `@packages/sdk-core/src/__tests__/ipns.test.ts`:
- Around line 201-221: The test 'S2 regression: throws on present-but-invalid
signature (fail-closed)' is duplicating the test coverage already established in
lines 183-199, both testing the same scenario where signature fields are present
but verifyEd25519 returns false. Either remove this duplicate test entirely, or
modify it to cover a distinct missing edge case such as handling
partial/incomplete signature fields, verifying invalid base64 encoding
propagation, or confirming that deriveIpnsName is not called when signature
verification fails. Ensure the test covers a meaningful scenario distinct from
the existing test to maintain code quality standards.
In `@packages/sdk-core/src/__tests__/vault.test.ts`:
- Around line 56-83: The current test only verifies that the private key buffer
is zeroed on the successful publish path, but the code should zero it on all
exit paths including when operations fail. Add a new test case that mocks either
addToIpfs or createAndPublishIpnsRecord to reject or throw an error, then calls
publishVaultKeyBlob and verifies that the privateKeyBuf is still zeroed to zero
bytes even when the operation fails. This ensures the finally block zeroization
works correctly for both success and error paths.
🪄 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: 0aac2332-410a-42d4-9e9b-25d1d750a8af
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (46)
.planning/REQUIREMENTS.md.planning/ROADMAP.md.planning/STATE.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-01-PLAN.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-01-SUMMARY.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-02-PLAN.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-02-SUMMARY.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-03-PLAN.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-03-SUMMARY.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-04-PLAN.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-04-SUMMARY.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-CONTEXT.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-DISCUSSION-LOG.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-PATTERNS.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-RESEARCH.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-SECURITY.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-VALIDATION.md.planning/phases/51-crypto-signature-secret-leak-hardening/51-VERIFICATION.md.planning/todos/pending/2026-06-20-fuse-per-file-ipns-publish-conflict-recorded-as-success.md.planning/todos/pending/2026-06-20-ipns-resolve-verify-coverage-and-web-sdk-dedup.mdapps/api/src/ipns/ipns.service.spec.tsapps/api/src/ipns/ipns.service.tsapps/desktop/src-tauri/src/commands/vault.rsapps/desktop/src-tauri/src/fuse/mod.rsapps/desktop/src-tauri/src/fuse/windows/mod.rsapps/web/src/services/__tests__/ipns.service.test.tsapps/web/src/services/ipns.service.tscrates/api-client/Cargo.tomlcrates/api-client/src/ipns.rscrates/api-client/src/types.rscrates/crypto/src/ecies.rscrates/crypto/tests/cross_language.rscrates/fuse/src/inode.rscrates/fuse/src/journal_helpers.rscrates/fuse/src/lib.rscrates/fuse/src/operations.rscrates/fuse/src/platform/windows/operations.rscrates/fuse/src/platform/windows/write_ops.rscrates/fuse/src/write_ops.rscrates/sdk/src/state.rspackages/sdk-core/src/__tests__/folder.test.tspackages/sdk-core/src/__tests__/ipns.test.tspackages/sdk-core/src/__tests__/vault.test.tspackages/sdk-core/src/folder/index.tspackages/sdk-core/src/ipns/index.tspackages/sdk-core/src/vault/index.ts
Phase 51 (S3/D-05) added `params.ipnsPrivateKey.fill(0)` in a finally to
createAndPublishIpnsRecord, treating it as a "buffer-owning boundary". It is
not: it is a callee that receives a caller-owned, reused buffer. The SDK
client caches per-folder IPNS keys in its folderTree (zeroed only on
destroy()) and passes the same buffer on every mutation; publishWithCas
reuses the buffer across CAS retries. After the first publish the buffer was
all-zero, so the next publish derived the wrong public key and the API
rejected it with 400 "publicKey does not correspond to the given ipnsName",
cascading "Child not found" failures across every publish-dependent flow.
This broke 48/89 SDK E2E tests (the only gate exercising the real
cross-package publish round-trip; the phase's unit checks and the S1 spec
mocked parseIpnsRecord, so it slipped through).
Fix: createAndPublishIpnsRecord no longer zeros its ipnsPrivateKey param,
matching publishWithCas's contract ("NEVER zeroes key material — callers are
responsible") and updateFolderMetadataAndPublish ("CALLER RETAINS
OWNERSHIP"). No key leak — terminal owners still zero: client.destroy()
(cached folderTree keys), clearBytes()/finally (transient unwrapped keys),
and publishVaultKeyBlob / shared-write.ts (freshly-derived keypairs). The
S3/D-05 guard tests A/B were inverted to assert the caller-owned buffer is
preserved.
Verified locally against the full stack: SDK E2E 89/89, sdk-core 209/209,
api 893/893, lint clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Entire-Checkpoint: bfbe30d8f64a
Address the actionable CodeRabbit findings on PR #529 that fall within HARD-02 (crypto-signature & secret-leak hardening): S2 — fail closed on PARTIAL signature fields. A resolve response carrying some-but-not-all of {signatureV2, data, pubKey} previously fell into the legacy allow path (signatureVerified=false), letting an attacker strip fields to bypass D-02. Now only an all-absent record is treated as legacy; any partial subset fails closed. Applied consistently across the three verify paths: crates/api-client (Ok(None) only when all three absent, else Ok(Some(false))), apps/web, and packages/sdk-core. S1 — anchor the full signed IPNS value. The publish-time check matched only the first CID substring via regex, so `/ipfs/<cid>/extra` could pass while delegated routing published a divergent raw value. Compare the whole value to `/ipfs/${metadataCid}` exactly. S3 — close secret-leak gaps. crates/fuse resolve_folder_key_cached now returns Zeroizing<Vec<u8>> (cache hits no longer hand back unzeroized plain copies). Desktop mount pre-population wraps derived [u8;32] folder keys in Zeroizing. Test quality: replace the duplicate sdk-core S2 regression test with a partial-fields fail-closed case; assert publish errors propagate (rejects.toThrow) instead of swallowing in try/catch; add a vault rejected- path zeroization guard; fix a bigint mock; make the API concurrent-batch spec order-independent (Promise.allSettled preserves result order, not parse call order). Added partial-field tests for web + sdk-core + api-client. Deferred (captured as todos, out of HARD-02 scope or risky): - per-file IPNS Conflict recorded as success (pre-existing #352, Phase 52) - inode stable-ID identity reset on display-name fallback (Phase 52) - validate embedded sequence when CAS omitted (risks non-CAS publish paths) - bind verified record to resp.cid via CBOR decode (heavy lift) Verified: SDK E2E 89/89, sdk-core 210, api 79 (ipns spec), web 61, cargo api-client/crypto green, cargo check --workspace clean, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: a91dbc72b885
Owner
Author
CodeRabbit review — dispositionThanks @coderabbitai. Triaged all findings; in-scope items fixed in Fixed (in
|
|
Tip For best results, initiate chat on the files or code changes.
All five deferral entries map cleanly to named todos and appropriate future phases. Happy to trigger a fresh review once the post- 🐇 ✓ ✏️ Learnings added
|
There was a problem hiding this comment.
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 `@packages/sdk-core/src/__tests__/ipns.test.ts`:
- Around line 206-220: The current test case in the S2 block covers omitted
signature fields but does not test the edge case where signature fields are
present as empty strings, which could bypass fail-closed logic if the
implementation relies on truthiness checks. Add an additional test case (or
extend the existing mock) that sets signatureV2 to an empty string '' instead of
omitting it entirely, and verify that resolveIpnsRecord still rejects with the
'incomplete signature data' error and does not call verifyEd25519 or
deriveIpnsName, ensuring the fail-closed behavior holds regardless of whether
fields are omitted or present as empty values.
In `@packages/sdk-core/src/__tests__/vault.test.ts`:
- Around line 88-97: The test named 'D: zeroes the derived
vaultKeyKeypair.privateKey when publish fails' currently mocks addToIpfs to
reject immediately, which prevents the code from ever reaching the
createAndPublishIpnsRecord call where the actual publish failure should occur.
To properly exercise the publish-failure path as the test name suggests, change
the mocks so that addToIpfs succeeds (remove or change the mocked rejection) and
instead mock createAndPublishIpnsRecord to fail with an error. This ensures the
test actually reaches and validates the behavior when the IPNS publish step
fails, not the upload step.
In `@packages/sdk-core/src/ipns/index.ts`:
- Around line 213-218: The condition checking for signature fields in the IPNS
resolve response uses truthy checks which treat empty values as absent. Instead
of using the current `if (signatureV2 || data || pubKey)` pattern, use nullish
coalescing checks to distinguish between truly missing fields (null/undefined)
versus present-but-empty fields. This ensures that empty strings or other falsy
values for signatureV2, data, or pubKey are properly treated as invalid data
that should be rejected during verification, rather than being downgraded to the
legacy unsigned path.
🪄 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: 3db650d8-971f-4930-a06e-7c17724cbf21
📒 Files selected for processing (16)
.planning/phases/51-crypto-signature-secret-leak-hardening/51-04-SUMMARY.md.planning/todos/pending/2026-06-20-fuse-inode-stable-id-identity-reset.md.planning/todos/pending/2026-06-20-ipns-publish-validate-embedded-sequence-without-cas.md.planning/todos/pending/2026-06-20-ipns-resolve-verify-coverage-and-web-sdk-dedup.mdapps/api/src/ipns/ipns.service.spec.tsapps/api/src/ipns/ipns.service.tsapps/desktop/src-tauri/src/fuse/mod.rsapps/web/src/services/__tests__/ipns.service.test.tsapps/web/src/services/ipns.service.tscrates/api-client/src/ipns.rscrates/fuse/src/lib.rspackages/sdk-core/src/__tests__/folder.test.tspackages/sdk-core/src/__tests__/ipns.test.tspackages/sdk-core/src/__tests__/vault.test.tspackages/sdk-core/src/ipns/index.tspackages/sdk-core/src/vault/index.ts
✅ Files skipped from review due to trivial changes (4)
- .planning/todos/pending/2026-06-20-ipns-publish-validate-embedded-sequence-without-cas.md
- .planning/todos/pending/2026-06-20-fuse-inode-stable-id-identity-reset.md
- .planning/todos/pending/2026-06-20-ipns-resolve-verify-coverage-and-web-sdk-dedup.md
- .planning/phases/51-crypto-signature-secret-leak-hardening/51-04-SUMMARY.md
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/sdk-core/src/tests/folder.test.ts
- apps/web/src/services/ipns.service.ts
- apps/api/src/ipns/ipns.service.ts
- apps/api/src/ipns/ipns.service.spec.ts
- crates/api-client/src/ipns.rs
- crates/fuse/src/lib.rs
Second round of CodeRabbit follow-ups on PR #529 (all adversarially verified): S2 — empty-string fail-closed. The resolve signature gate used truthy checks (`if (signatureV2 || data || pubKey)`), so a record with all three fields present as empty strings ('') was downgraded to the legacy allow path. Switch to nullish presence checks (`!= null`) in both apps/web and packages/sdk-core so present-but-empty fields are treated as present and fail closed; only all-omitted records take the legacy path. (Rust verifier already fails closed on empty via base64-decode + verify.) Web 404 detection. apps/web resolve only checked `error.status`, but real axios errors from the api-client surface status at `error.response?.status`, so live 404s would throw instead of returning null. Now checks `error.status ?? error.response?.status`, matching sdk-core. S3 — no transient plaintext key. Added a `zeroizing_32_from_slice` helper (preallocate `Zeroizing<[u8;32]>`, length-check, then `copy_from_slice`) and applied it to the file/folder/IPNS key sites in crates/fuse operations.rs and platform/windows/operations.rs, so no plain `[u8;32]` temporary is materialized before the zeroizing wrapper. Tests: add empty-string fail-closed cases (web + sdk-core); web 404 test now mocks the axios `error.response` shape plus a fallback `error.status` case; vault test D now exercises the real publish-failure path (upload succeeds, publish rejects) instead of the upload path. Verified: SDK E2E 89/89, sdk-core 211, web 63, cargo fuse 60, lint + cargo check clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: b3b9ba579e3d
FSM1
added a commit
that referenced
this pull request
Jun 21, 2026
* docs: file resolved hardening todos and add deferred-findings phases 56-58 Bookkeeping (verified against live code via parallel review agents): - File #5 IPNS S1/S2/S3 review to completed/ (bulk shipped in PR #529); residue tracked by phase 58 todos. - File #10 large-file refactor survey to completed/ (Tier-1/2 shipped in PR #538); re-capture the 14 open Tier-3 items. - Fold the per-file-IPNS-conflict todo into the PR #538 robustness todo (one unified site post-#538) and file to completed/. - Trim the metadata zeroize todo to its one remaining helper (spawn_metadata_publish). New phases via gsd-tools phase.add (HARD-07..09): - 56 FUSE & IPNS Durability Hardening — PR #538 deferred findings. - 57 API CID/Provider Hardening & Module Dedup. - 58 IPNS Signature-Verify Coverage — Phase 51 / PR #529 S1/S2 residue. Update ROADMAP (sections, progress table, execution order), REQUIREMENTS (HARD-07..09 + traceability), and STATE body. STATE frontmatter progress counts left unreconciled (pre-existing drift, todo #6). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Entire-Checkpoint: f7d4c3a60cd5 * docs(56): capture phase context Entire-Checkpoint: bd91ecff0d20 * docs(state): record phase 56 context session Entire-Checkpoint: e93a1c24b15f --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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
Phase 51: Crypto-Signature & Secret-Leak Hardening
Goal: Enforce IPNS signedRecord validation/verification and private-key zeroization (HARD-02).
Status: Verified ✓
Closes the three deferred IPNS signed-record findings from the
#448security review (S1/S2/S3). The server stays a zero-knowledge relay and the DB remains the authoritative CID source — these are defense-in-depth / correctness hardening (Medium severity). The web logger redaction + Faro transport item (todo#15) was scoped out of this phase (deferred with end-user monitoring).Changes
51-01 — S1: publish-time embedded-vs-DTO validation
On IPNS publish, reject (400) when the embedded signed-record CID ≠ DTO
metadataCid; offset-aware sequence check tolerating the first-publish pre-increment convention. Runs after the CAS check so concurrency 409s outrank tamper 400s; the pre-shipped anti-rollback 409 is preserved.Key files:
apps/api/src/ipns/ipns.service.ts(+ spec)51-02 — S2: web fail-closed verification
resolveIpnsRecordthrows on a present-but-invalid signature (was warn + return); absent signature fields → allow +signatureVerified=false+ warn.Key files:
apps/web/src/services/ipns.service.ts(+ test)51-03 — S2/S3: Rust verification + zeroization
verify_ipns_resolve_signature+ signature fields onIpnsResolveResponse; the FUSE folder-key descent honorssignatureVerified.unwrap_keyreturnsZeroizing<Vec<u8>>; folder keys threaded asZeroizingthrough the BFS queue / getters.Key files:
crates/api-client/*,crates/crypto/src/ecies.rs,crates/fuse/src/lib.rs51-04 — S3: sdk-core zeroization sweep
fill(0)infinallyoncreateAndPublishIpnsRecord/publishVaultKeyBlob;updateFolderMetadataAndPublishdocumented caller-owns-key skip + guard test; S2 resolve regression guard.Key files:
packages/sdk-core/src/{ipns,vault,folder}/index.ts(+ tests)Post-review hardening
unwrap_keyZeroizingcascade intocommands/vault.rs.crates/api-client.root_folder_key→Zeroizing, folder-key replay cache →Zeroizing, reencrypt worker + FUSEoperations.rstransient[u8;32]arrays →Zeroizing(macOS + WinFsp paths in lockstep).SDK E2E regression fix (commit
e1f822e2c)The initial S3 sweep added
params.ipnsPrivateKey.fill(0)tocreateAndPublishIpnsRecord,but that function is a callee receiving a caller-owned, reused buffer (the client caches
per-folder IPNS keys in its
folderTree, zeroed only ondestroy();publishWithCasreusesthe buffer across CAS retries). Zeroing there corrupted the key on the second+ publish, so the
API rejected it (400
publicKey does not correspond to the given ipnsName) — breaking 48/89 SDKE2E tests. Fixed by removing the callee zeroization (matching
publishWithCas's documentedcaller-owns-key contract); no key leak — terminal owners still zero. Reproduced and verified
green against the full local stack: SDK E2E 89/89, sdk-core 209/209, api 893/893.
Requirements Addressed
Verification
cargo check --workspacegreen.51-VERIFICATION.md).51-SECURITY.md).51-VALIDATION.md).windows/operations.rszeroization) — not locally buildable on macOS; gated by CI.Key Decisions
Deferred (captured todos)
#352), FUSE-durability domain (Phase 52).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes