feat(platform-wallet): keyring_core secret backends — encrypted-file + OS keyring (secrets feature)

[Claudius-Maginificent]

Summary

Adds the secrets feature to rs-platform-wallet-storage — a SecretStore SPI exposing two backends behind the keyring_core trait:

• EncryptedFileStore — passphrase-protected vault file on disk (Argon2id KDF + XChaCha20-Poly1305 AEAD, atomic write via NamedTempFile::persist)
• OS-native keyring stores — Linux (libsecret/dbus + linux-keyutils), macOS (apple-native), Windows (windows-native), via crate features

Stacks on feat/platform-wallet-sqlite-persistor (PR #3625, which now also includes the storage hardening landed via #3743).

Public surface

• SecretStore trait (re-exports keyring_core::Credential-shaped API for callers)
• SecretBytes / SecretString — Drop-zeroizing buffers with Display redaction and forbid PartialEq (use subtle::ConstantTimeEq for comparison)
• FileStoreError — typed taxonomy (WrongPassphrase, Corruption, Busy, MalformedVault, …) with no raw bytes in Display
• OsKeyringErrorKind — security-enforcing payload-stripping projection of keyring_core::Error (prevents BadEncoding/BadDataFormat raw bytes from propagating, CWE-209/CWE-532)

File vault format

Three core types (post-collapse):

• Vault { version, kdf: KdfParams, salt: [u8; SALT_LEN], verify_nonce: [u8; NONCE_LEN], verify_ct: Vec<u8>, entries: Vec<Entry> }
• Entry { label, nonce: [u8; NONCE_LEN], ciphertext: Vec<u8> }
• KdfParams { id, m_kib, t, p } (Argon2id; id-field validated in enforce_bounds)

Serde-validated via const-generic hex_array adapter on fixed-size byte fields; length mismatches surface as MalformedVault at parse time. AEAD AAD is the typed (format_version, wallet_id, label) triple — never serialized bytes (C1 invariant).

Crate features

Feature	Default	Brings
secrets	yes (via default)	SecretStore SPI + EncryptedFileStore
apple-native-keyring-store	no	macOS keychain backend
windows-native-keyring-store	no	Windows credential manager
linux-keyutils-keyring-store	no	Linux kernel keyrings
dbus-secret-service-keyring-store	no	libsecret/dbus (GNOME/KDE)
Default also enables: sqlite, cli	yes	(inherited from base PR)

Off-state CI invocation (--no-default-features --features sqlite,cli) is exercised by secrets_off_state.rs — confirms the secrets module compiles out cleanly when disabled.

Notable design decisions

• SecretBytes forbid == (EDIT-4) — comparison via subtle::ConstantTimeEq only; prevents accidental short-circuit timing attacks
• SecretString Drop: dropped redundant manual Drop impl (the inner Vec<u8> already zeroizes via zeroize crate)
• Atomic vault write via NamedTempFile::persist — cross-platform, no half-written vault on crash
• serde_json versioned two-step parse: VersionProbe { version: u32 } first, then dispatch to the right Vault shape. Allows forward-compat without runtime version branching today (single version)
• FileStoreError::Busy on rekey collision instead of panic — operators get a typed retry signal
• Lowercase-hex service strings for OS-keyring entries — case-fold-stable across platforms (some keyrings normalize case differently)
• Box typed FileStoreError into keyring_core::NoStorageAccess — lossless SPI recovery without payload leak

Test plan

• cargo fmt --all -- --check
• cargo clippy -p platform-wallet-storage --all-targets -- -D warnings (default features)
• cargo clippy -p platform-wallet-storage --all-targets --no-default-features --features sqlite,cli -- -D warnings (off-state)
• cargo test -p platform-wallet-storage (default features) — 70 secrets lib tests, 5 secrets_api, 1 secrets_default_on_compiles, 18 sqlite integration suites, all green
• cargo test -p platform-wallet-storage --no-default-features --features sqlite,cli --test secrets_off_state — 1/1 pass
• CI on the merged tip — pending dispatch

Recent activity

• Merged origin/feat/platform-wallet-sqlite-persistor (post-feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface #3743) into this branch (commit 680155f896) — brings in V001/V002 schema collapse, symmetric wallet_id validation, typed PersistenceError, B3 abort-on-mutation, and the rest of the feat(platform-wallet)!: rs-platform-wallet-storage crate (SQLite persister) + trait surface #3743 storage hardening
• Collapsed three runtime/wire type mirrors in secrets/file/ (commits 4bfafa50cf → 9884f22725): 6 types → 3 types (Vault, Entry, KdfParams), via const-generic hex_array serde adapter. Net -20 production lines, all tests green.

Related

• feat(platform-wallet): add platform-wallet-storage crate (sqlite persister) #3625 (feat/platform-wallet-sqlite-persistor) — base PR providing the SQLite persister + trait surface
• feat(platform-wallet): watch-only rehydration from persistor (seedless load) #3692 (feat/platform-wallet-rehydration) — stacks on this PR; rehydration / seedless watch-only load
• feat(platform-wallet)!: consumer hardening — CODE-001/003-callsite/017/018 + PROJ-001 FFI + CODE-008/012/013 #3750 (feat/platform-wallet-consumer-hardening) — sibling of this PR (also stacks on feat(platform-wallet): add platform-wallet-storage crate (sqlite persister) #3625); consumer-side hardening + PROJ-001 FFI ABI break

🤖 Generated with Claude Code

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 34756190-44a2-49cb-9795-1bca4606152c

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/platform-wallet-storage-secrets

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[Copilot]

Pull request overview

Adds a default-on secrets subsystem to platform-wallet-storage to persist wallet secret material outside SQLite, using the upstream keyring_core SPI and providing both an encrypted-file vault backend and an OS-keyring backend.

Changes:

• Introduces platform_wallet_storage::secrets (default feature) with EncryptedFileStore (Argon2id + XChaCha20-Poly1305) and default_credential_store() for OS keyrings.
• Adds secret-handling wrappers (SecretBytes, SecretString) plus validation and an error-bridging layer to keyring_core::Error.
• Adds multiple guard tests (secrets_scan, secrets_guard, API shape checks, and “secrets off” build-mode guard) and updates docs/README/Cargo features accordingly.

Reviewed changes

Copilot reviewed 19 out of 20 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
packages/rs-platform-wallet-storage/tests/secrets_scan.rs	Clarifies schema/migrations scan scope vs src/secrets/ exemption.
packages/rs-platform-wallet-storage/tests/secrets_off_state.rs	Adds runtime guard ensuring secrets compile out when feature is disabled.
packages/rs-platform-wallet-storage/tests/secrets_guard.rs	Adds string-level leak-prevention scans for the secrets module.
packages/rs-platform-wallet-storage/tests/secrets_default_on_compiles.rs	Build-only test asserting secrets surface is available in default build.
packages/rs-platform-wallet-storage/tests/secrets_api.rs	API/boundary shape tests for secrets SPI usage and error rendering.
packages/rs-platform-wallet-storage/src/secrets/validate.rs	Adds WalletId newtype + strict label allowlist validation.
packages/rs-platform-wallet-storage/src/secrets/secret.rs	Implements SecretBytes/SecretString zeroizing wrappers and CT equality.
packages/rs-platform-wallet-storage/src/secrets/mod.rs	Wires secrets submodules and public re-exports.
packages/rs-platform-wallet-storage/src/secrets/memory.rs	Adds in-RAM CredentialStoreApi test double behind __secrets-test-helpers.
packages/rs-platform-wallet-storage/src/secrets/keyring.rs	Adds OS-keyring default store constructor with fail-closed behavior.
packages/rs-platform-wallet-storage/src/secrets/file/mod.rs	Implements encrypted vault store + CredentialStoreApi/CredentialApi.
packages/rs-platform-wallet-storage/src/secrets/file/format.rs	Defines vault format framing and canonical AAD construction.
packages/rs-platform-wallet-storage/src/secrets/file/error.rs	Defines file-backend error taxonomy.
packages/rs-platform-wallet-storage/src/secrets/file/error_bridge.rs	Bridges file-backend errors into keyring_core::Error + downcast helper.
packages/rs-platform-wallet-storage/src/secrets/file/crypto.rs	Implements Argon2id KDF + XChaCha20-Poly1305 seal/open helpers.
packages/rs-platform-wallet-storage/src/lib.rs	Exposes secrets module behind feature and adds send/sync/object-safety checks.
packages/rs-platform-wallet-storage/SECRETS.md	Updates spec/docs to present-state secrets implementation and audit hooks.
packages/rs-platform-wallet-storage/README.md	Updates feature table and build modes to include secrets and helpers.
packages/rs-platform-wallet-storage/Cargo.toml	Adds secrets dependencies, platform store deps, features, and dev-dep tweaks.
Cargo.lock	Pulls in keyring-core + platform store crates + crypto dependencies.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Claudius-Maginificent]

📦 Review-response push + base merge-up — 8a5ef7aabd..34c8ecbb9c

12 commits addressing the open review feedback + 1 merge-up commit pulling the recent base hardening + atomicity work from #3625.

Review-response commits

OID	Headline	Closes
b6a84fdc19	refactor!: unify FileStoreError, drop error_bridge; distinguish Corruption from WrongPassphrase	lklimek error.rs:25, copilot file/mod.rs:374
647567e322	fix: remove redundant SecretString Drop (UB) and dangling mlock on empty SecretBytes	copilot secret.rs:131,216
8ab4208332	feat!: serde_json vault format with versioned two-step parse (CMT-007)	lklimek format.rs:84
68ed3d13b5	fix: cross-platform atomic vault write via NamedTempFile::persist (CMT-009)	copilot file/mod.rs:224
0066a5a472	feat!: public SecretStore API exposing SecretBytes, never raw bytes (CMT-002)	lklimek mod.rs:32
c636ac07d0	refactor: string-only keyring_core From; typed-path error distinction	(architecture follow-up)
a5c5bf0c6a	fix: box typed FileStoreError into keyring_core NoStorageAccess for lossless SPI recovery	(post-rework loss-of-typed-error gap)
e1c7fa9418	refactor: remove MemoryCredentialStore; retire __secrets-test-helpers (CMT-008)	lklimek memory.rs:1
671ce69c3f	fix: enforce lowercase-hex service, widen expose_secret guard scan (CMT-012/010)	copilot file/mod.rs:351, copilot tests/secrets_guard.rs:61
dc492ccf89	docs: strip historical comments + license header (CMT-013/014)	lklimek secret.rs:3,17
c58a2b5d00	feat: log swallowed mlock + corruption/write failures (Display-only, secret-free)	(observability gap)
6aa2942d22	docs: drop deleted MemoryCredentialStore / __secrets-test-helpers references (QA-002)	(stale-doc QA finding)

Base merge-up — 34c8ecbb9c

Merged origin/feat/platform-wallet-sqlite-persistor (which moved from 8a5ef7aabd to fe01634684 since this PR was last rebased) — brings in:

• 10 hardening commits from feat(platform-wallet): add platform-wallet-storage crate (sqlite persister) #3625 (CMT-001..012 fixes — 2 BLOCKING data-loss, 2 HIGH security, 6 MEDIUM, 2 LOW)
• 11 atomicity commits (A-1..A-8 + N-1..N-10 — 4 HIGH, 5 MEDIUM, 1 LOW, 5 doc-only) including the breaking commit_writes → CommitReport signature change on SqlitePersister
• 1 polish commit (Drop side-effect comment + README operational notes)

Conflicts: only Cargo.lock (regenerated by cargo). Auto-merged: Cargo.toml, README.md, src/lib.rs (disjoint sections on each side). All 4 WalletStorageError additions (BlobTooLarge, AssetLockEntryMismatch, IntegrityCheckFailed from sqlite side; FileStore-related from secrets side) coexist with wildcard-free classifier matches intact.

Verification post-merge

• cargo fmt --check — clean
• cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets -- -D warnings — clean
• cargo test -p platform-wallet-storage — all green (114 unit + integration + 12 hardening + 11 atomicity + secrets API + 5 restore_staged_validation)
• cargo test --doc -p platform-wallet-storage — 3 passed, 1 ignored

Thread status

• 8 copilot threads — auto-resolved (bot, fixes verified at file:line)
• 6 lklimek threads — replies posted with the addressing commit's OID; left unresolved for the author to ack

Head + state

Head	34c8ecbb9c
Mergeable	should refresh to MERGEABLE once GitHub re-evaluates against the merged base
Base	feat/platform-wallet-sqlite-persistor @ fe01634684 (#3625)

_{🤖 Co-authored by Claudius the Magnificent AI Agent}

[thepastaclaw]

Code Review

Of the 4 prior findings from review 6aa2942d, rekey() changing the store-wide passphrase after re-encrypting only one wallet is STILL VALID, file-backed secret mutations remaining unsynchronized read-modify-write cycles is STILL VALID, forward-schema databases being accepted by SqlitePersister::open() is FIXED, and staged-restore validation failures deleting WAL/SHM before rejection is FIXED. The carried-forward problems are both in the file-backed secrets backend. The latest delta adds 4 new blocking regressions in SQLite restore/persist and SPV runtime paths, plus 4 valid lower-severity filesystem hardening issues; this requires changes before merge.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3672 34c8ecb
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._

Reviewed commit: 34c8ecb

🔴 6 blocking | 🟡 4 suggestion(s)

Verified findings

blocking: Carried-forward prior finding: `rekey()` changes the store passphrase globally after re-encrypting only one wallet vault

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 275)

EncryptedFileStoreInner stores a single passphrase for the whole store, but rekey() only rewrites the vault file for the supplied wallet_id before assigning self.passphrase = new_passphrase. The same store instance serves arbitrary wallet IDs through build(), put(), get(), and delete(), so rekeying wallet_a immediately makes every other existing vault in the directory unreadable through that store instance: subsequent operations derive with the new passphrase even though those other vault files are still encrypted with the old one. This is a real multi-wallet data-loss bug, not just a theoretical inconsistency.

blocking: Carried-forward prior finding: file-backed secret mutations still lose concurrent updates

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 321)

Both put() and delete() still implement mutations as read_vault -> mutate Vec<VaultEntry> -> write_vault with no in-process mutex and no inter-process file lock. CredentialStoreApi::build() returns independent credentials backed by the same Arc<EncryptedFileStoreInner>, so concurrent writers can read the same old snapshot and then each atomically replace the vault with conflicting results. Atomic rename prevents torn files, but it does not serialize the read-modify-write cycle, so the last writer silently discards the other update.

blocking: Latest-delta finding: `restore_from()` still deletes destination WAL/SHM before its last fallible steps

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 242)

The staged-copy validation now runs before the unlink loop, which fixes the earlier rejection path, but the restore is still not atomic. restore_from() removes <dest>-wal and <dest>-shm before the later set_permissions() and tmp.persist(dest_db_path) calls. Either of those later operations can still fail, leaving the old main database file in place after its WAL/SHM sidecars have already been deleted. On a live WAL-mode database that drops committed-but-uncheckpointed state from the destination. The code comment claiming that either both the main DB and siblings are replaced or none are touched is therefore false in the current implementation.

blocking: Latest-delta finding: committed WAL/SHM sidecars are never re-chmodded after ordinary writes

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 171)

SqlitePersister::open() calls apply_secure_permissions(&config.path) immediately after opening the database, but the WAL and SHM sidecars normally do not exist yet. The normal write path in write_changeset_in_one_tx() commits at line 650 and returns without reapplying the helper, and apply_secure_permissions() is only called from open, backup, and restore. That means the first transaction can create wallet.db-wal and wallet.db-shm with the process umask instead of 0600, exposing recently committed wallet pages on multi-user hosts. The new test only proves the helper works when called manually; it does not cover the persistence path that actually creates the sidecars.

        tx.commit()?;
        apply_secure_permissions(&self.config.path)?;
        Ok(())

blocking: Latest-delta finding: `asset_locks::apply()` still writes rows whose typed columns can contradict the serialized blob

packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs (line 36)

The read path now rejects typed-column/blob mismatches in decode_row(), but the write path still does not validate that the map key op matches entry.out_point or that the typed account_index column matches entry.account_index. Unlike identity_keys::apply(), which now rejects mismatched typed-vs-blob representations before persisting them, asset_locks::apply() will serialize contradictory state to disk. Because AssetLockChangeSet and AssetLockEntry have public fields, a malformed caller can construct this inconsistency today. The result is durable on-disk corruption that the next load will hard-fail on.

blocking: Latest-delta finding: `SpvRuntime::run()` holds the client read lock across `await` and blocks `stop()` forever

packages/rs-platform-wallet/src/spv/runtime.rs (line 138)

run() takes self.client.read().await, borrows the inner DashSpvClient, and then awaits client.run() while the RwLock read guard is still alive. stop() needs self.client.write().await before it can call c.stop(), so the advertised shutdown path cannot progress while run() is active. This is not required by the client API: the vendored dash-spv revision used here implements Clone for DashSpvClient, so run() could clone the client out of the lock and drop the guard before awaiting. As written, background sync can deadlock its own shutdown.

        let client = {
            let client_guard = self.client.read().await;
            client_guard
                .as_ref()
                .cloned()
                .ok_or(PlatformWalletError::SpvNotRunning)?
        };

        let result = client
            .run()
            .await
            .map_err(|e| PlatformWalletError::SpvError(e.to_string()));

        let mut client = self.client.write().await;
        let _ = client.take();

        result

suggestion: Vault directory permissions are never hardened or validated

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 73)

EncryptedFileStore::open() creates or accepts the vault directory and immediately trusts it, but only individual vault files are later checked for 0600. On Unix, a default create_dir_all() under a normal umask can leave the directory traversable by other local users, which leaks wallet IDs through <wallet_id>.pwsvault filenames and allows deletion or replacement of vault files in shared or group-writable directories. That weakens the backend's local-filesystem trust boundary even when each vault file itself is 0600.

suggestion: `read_vault()` allocates the entire file before enforcing any size bound or authenticity check

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 217)

read_vault() performs fs::read(path) immediately after metadata lookup, so any replaced or hostile vault file is fully loaded into memory before JSON parsing, KDF parameter validation, or AEAD verification runs. That gives an attacker who can replace the file at rest a straightforward memory-exhaustion and availability kill switch on every get/put/delete/rekey path that opens the vault.

suggestion: Permission validation is vulnerable to a metadata-to-read race

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 217)

read_vault() checks permissions with fs::metadata(path) and then performs a separate fs::read(path). If an attacker can replace the directory entry between those two syscalls, the code can validate one file's mode bits and then parse different bytes. Combined with the missing directory hardening, this bypasses the intended permission gate and leaves the decrypt path open to rollback or denial-of-service through attacker-chosen vault contents.

suggestion: Windows builds still skip the pre-existing permissions check entirely

packages/rs-platform-wallet-storage/src/secrets/file/mod.rs (line 553)

On non-Unix targets check_perms() is a stub that always returns Ok(()), so the documented invariant that a pre-existing vault with loose permissions is rejected is not enforced on Windows at all. A Windows user can therefore open and keep using a vault file with an overly broad ACL, which exposes ciphertext and metadata to other local principals and gives them a practical tamper or rollback point.

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:275-317: Carried-forward prior finding: `rekey()` changes the store passphrase globally after re-encrypting only one wallet vault
  `EncryptedFileStoreInner` stores a single `passphrase` for the whole store, but `rekey()` only rewrites the vault file for the supplied `wallet_id` before assigning `self.passphrase = new_passphrase`. The same store instance serves arbitrary wallet IDs through `build()`, `put()`, `get()`, and `delete()`, so rekeying `wallet_a` immediately makes every other existing vault in the directory unreadable through that store instance: subsequent operations derive with the new passphrase even though those other vault files are still encrypted with the old one. This is a real multi-wallet data-loss bug, not just a theoretical inconsistency.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:321-395: Carried-forward prior finding: file-backed secret mutations still lose concurrent updates
  Both `put()` and `delete()` still implement mutations as `read_vault -> mutate Vec<VaultEntry> -> write_vault` with no in-process mutex and no inter-process file lock. `CredentialStoreApi::build()` returns independent credentials backed by the same `Arc<EncryptedFileStoreInner>`, so concurrent writers can read the same old snapshot and then each atomically replace the vault with conflicting results. Atomic rename prevents torn files, but it does not serialize the read-modify-write cycle, so the last writer silently discards the other update.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:242-274: Latest-delta finding: `restore_from()` still deletes destination WAL/SHM before its last fallible steps
  The staged-copy validation now runs before the unlink loop, which fixes the earlier rejection path, but the restore is still not atomic. `restore_from()` removes `<dest>-wal` and `<dest>-shm` before the later `set_permissions()` and `tmp.persist(dest_db_path)` calls. Either of those later operations can still fail, leaving the old main database file in place after its WAL/SHM sidecars have already been deleted. On a live WAL-mode database that drops committed-but-uncheckpointed state from the destination. The code comment claiming that either both the main DB and siblings are replaced or none are touched is therefore false in the current implementation.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:171-177: Latest-delta finding: committed WAL/SHM sidecars are never re-chmodded after ordinary writes
  `SqlitePersister::open()` calls `apply_secure_permissions(&config.path)` immediately after opening the database, but the WAL and SHM sidecars normally do not exist yet. The normal write path in `write_changeset_in_one_tx()` commits at line 650 and returns without reapplying the helper, and `apply_secure_permissions()` is only called from open, backup, and restore. That means the first transaction can create `wallet.db-wal` and `wallet.db-shm` with the process umask instead of `0600`, exposing recently committed wallet pages on multi-user hosts. The new test only proves the helper works when called manually; it does not cover the persistence path that actually creates the sidecars.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:36-50: Latest-delta finding: `asset_locks::apply()` still writes rows whose typed columns can contradict the serialized blob
  The read path now rejects typed-column/blob mismatches in `decode_row()`, but the write path still does not validate that the map key `op` matches `entry.out_point` or that the typed `account_index` column matches `entry.account_index`. Unlike `identity_keys::apply()`, which now rejects mismatched typed-vs-blob representations before persisting them, `asset_locks::apply()` will serialize contradictory state to disk. Because `AssetLockChangeSet` and `AssetLockEntry` have public fields, a malformed caller can construct this inconsistency today. The result is durable on-disk corruption that the next load will hard-fail on.
- [BLOCKING] In `packages/rs-platform-wallet/src/spv/runtime.rs`:138-158: Latest-delta finding: `SpvRuntime::run()` holds the client read lock across `await` and blocks `stop()` forever
  `run()` takes `self.client.read().await`, borrows the inner `DashSpvClient`, and then awaits `client.run()` while the `RwLock` read guard is still alive. `stop()` needs `self.client.write().await` before it can call `c.stop()`, so the advertised shutdown path cannot progress while `run()` is active. This is not required by the client API: the vendored `dash-spv` revision used here implements `Clone` for `DashSpvClient`, so `run()` could clone the client out of the lock and drop the guard before awaiting. As written, background sync can deadlock its own shutdown.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:73-76: Vault directory permissions are never hardened or validated
  `EncryptedFileStore::open()` creates or accepts the vault directory and immediately trusts it, but only individual vault files are later checked for `0600`. On Unix, a default `create_dir_all()` under a normal umask can leave the directory traversable by other local users, which leaks wallet IDs through `<wallet_id>.pwsvault` filenames and allows deletion or replacement of vault files in shared or group-writable directories. That weakens the backend's local-filesystem trust boundary even when each vault file itself is `0600`.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:217-221: `read_vault()` allocates the entire file before enforcing any size bound or authenticity check
  `read_vault()` performs `fs::read(path)` immediately after metadata lookup, so any replaced or hostile vault file is fully loaded into memory before JSON parsing, KDF parameter validation, or AEAD verification runs. That gives an attacker who can replace the file at rest a straightforward memory-exhaustion and availability kill switch on every `get`/`put`/`delete`/`rekey` path that opens the vault.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:217-220: Permission validation is vulnerable to a metadata-to-read race
  `read_vault()` checks permissions with `fs::metadata(path)` and then performs a separate `fs::read(path)`. If an attacker can replace the directory entry between those two syscalls, the code can validate one file's mode bits and then parse different bytes. Combined with the missing directory hardening, this bypasses the intended permission gate and leaves the decrypt path open to rollback or denial-of-service through attacker-chosen vault contents.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:553-556: Windows builds still skip the pre-existing permissions check entirely
  On non-Unix targets `check_perms()` is a stub that always returns `Ok(())`, so the documented invariant that a pre-existing vault with loose permissions is rejected is not enforced on Windows at all. A Windows user can therefore open and keep using a vault file with an overly broad ACL, which exposes ciphertext and metadata to other local principals and gives them a practical tamper or rollback point.

[thepastaclaw]

Supplemental correction to my review for 34c8ecbb: the prior-finding reconciliation in the review body accidentally dropped two explicitly assigned prior Swift findings. I rechecked all four prior findings from 6aa2942d against the current head; the correct reconciliation is:

• STILL VALID — packages/rs-platform-wallet-storage/src/secrets/file/mod.rs: vault updates are still unsynchronized read-modify-write cycles that can silently discard concurrent writes.
• STILL VALID — packages/rs-platform-wallet-storage/src/secrets/file/mod.rs: rekey(wallet_id, ...) still rewrites one wallet vault and then changes the store-wide passphrase used for every wallet in the directory.
• STILL VALID — packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift: Swift still does not map Rust FFI error code 10 (DriveInternalError), so it falls through to .unknown(message).
• STILL VALID — packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift: Swift warmUpShieldedProver() still promises stronger completion semantics than the Rust FFI provides; awaiting it only confirms the Rust warm-up task was queued, not that the prover is ready.

Those last two Swift items should be treated as carried-forward suggestion-level findings in addition to the findings listed in the review body.

[thepastaclaw]

Code Review

Cumulative revalidation against db7b6b562c4a86e9d9f248115d0a36e661d5f779 found prior findings 1-5 and 9-12 are STILL VALID and remain attributable to this PR’s storage/secrets changes; none of the prior findings are FIXED or INTENTIONALLY DEFERRED. Prior findings 6-8 are OUTDATED for this review because those SPV/Swift files still have the same issues, but they are unchanged from upstream/v3.1-dev and are not part of this PR branch’s scope. The latest delta 34c8ecbb9cbb0e0417723bfd47c54b6a2f6335a2..db7b6b562c4a86e9d9f248115d0a36e661d5f779 does not introduce any new findings, so the review action is driven entirely by carried-forward prior findings.

🔴 5 blocking | 🟡 4 suggestion(s)

3 additional finding(s) omitted (not in diff).

Out-of-scope follow-up suggestions (5)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• SpvRuntime::run() still deadlocks stop(), but that file is untouched by this PR branch — packages/rs-platform-wallet/src/spv/runtime.rs:138-158 still holds the RwLock read guard across client.run().await, so stop() cannot acquire the write lock while the run loop is active. The bug is real, but git diff upstream/v3.1-dev...db7b6b562c4a86e9d9f248115d0a36e661d5f779 shows this file is not part of the PR branch, so it should be tracked separately rather than carried as a PR finding here.
  • Follow-up: Create a separate issue or maintainer-requested PR to clone the client handle out of the lock before awaiting run().
• The Swift SDK still has two FFI-contract mismatches, but neither file is touched by this PR branch — packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift still drops Rust DriveInternalError code 10 to .unknown, and PlatformWalletManagerShieldedSync.swift still exposes warmUpShieldedProver() as an async readiness-looking API even though the Rust FFI only schedules background warm-up. Both issues are valid, but git diff upstream/v3.1-dev...db7b6b562c4a86e9d9f248115d0a36e661d5f779 shows neither file is part of this PR branch, so they are separate follow-up work.
  • Follow-up: Create a separate issue or maintainer-requested PR for the Swift FFI mapping and warm-up API contract cleanup.
• Carried-forward prior finding: SpvRuntime::run() holds the client read lock across await and blocks stop() forever — OUTDATED for this PR review: the bug is still present, but packages/rs-platform-wallet/src/spv/runtime.rs is unchanged from upstream/v3.1-dev and is outside this PR branch’s scope.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Carried-forward prior finding: Swift still does not map Rust DriveInternalError (code 10) — OUTDATED for this PR review: the mismatch is still present, but packages/swift-sdk/Sources/SwiftDashSDK/SDK.swift is unchanged from upstream/v3.1-dev and is outside this PR branch’s scope.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.
• Carried-forward prior finding: Swift warm-up API still promises readiness that the Rust FFI does not provide — OUTDATED for this PR review: the contract mismatch is still present, but packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerShieldedSync.swift is unchanged from upstream/v3.1-dev and is outside this PR branch’s scope.
  • Follow-up: Consider creating a separate issue or author/maintainer-requested PR for this.

[thepastaclaw]

Code Review

Cumulative incremental review at e7e1de8. The latest delta from db7b6b5..e7e1de8 contains zero changes under packages/rs-platform-wallet-storage/ (confirmed by git diff --stat and an empty diff in that path) — it is only an unrelated merge of dapi-cleanup, version bumps, CHANGELOG, and JS package metadata. No new latest-delta findings on the wallet-storage surface. All 9 prior findings from db7b6b5 were re-verified line-by-line against the current head and are STILL VALID and carried forward: (1) STILL VALID rekey() rotates the directory-wide passphrase, (2) STILL VALID lock-free read-modify-write race in put/delete, (3) STILL VALID restore_from() deletes WAL/SHM before fallible persist, (4) STILL VALID ordinary commits never re-harden new WAL/SHM sidecars, (5) STILL VALID asset_locks::apply writes self-contradictory rows, (6) STILL VALID vault directory perms never hardened, (7) STILL VALID read_vault has no size cap, (8) STILL VALID metadata→read TOCTOU, (9) STILL VALID Windows check_perms stub. All 9 fit within the 10-finding budget. Multi-agent convergence: claude/codex/security/rust-quality reviewers all independently flagged each item.

🔴 5 blocking | 🟡 4 suggestion(s)

3 additional finding(s) omitted (not in diff).

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:275-318: [Carried-forward, STILL VALID] rekey() swaps the directory-wide passphrase after rewriting only one wallet vault
  Verified at e7e1de8e. `EncryptedFileStoreInner` holds a single `passphrase: SecretString` at line 67 covering every `<wallet_id>.pwsvault` under `self.dir`. `rekey(wallet_id, new_passphrase)` only re-encrypts the vault file addressed by the supplied `wallet_id` (lines 280-315) before unconditionally assigning `self.passphrase = new_passphrase` (line 316). For multi-wallet stores this corrupts the directory invariant: after rekeying wallet A, every subsequent `get`/`put`/`delete`/`rekey` for wallet B derives keys from A's new passphrase and the header verify-token rejects with `WrongPassphrase`, effectively locking the operator out of every other vault in the same store. The missing-vault early return at lines 281-284 is worse: it overwrites the store-wide passphrase without rewriting any vault. The new `Arc::get_mut` guard at lines 96-98 only prevents in-flight credentials from observing a partial swap; it does not address cross-wallet correctness in a shared directory. Either scope `passphrase` per `wallet_id` (e.g. `HashMap<WalletId, SecretString>`) so the API matches the implementation, or have `rekey` walk every `*.pwsvault` under `self.dir` and re-encrypt all of them under the new passphrase before swapping `self.passphrase`. As a minimum fix, restrict the documented use-model to one wallet per `EncryptedFileStore` handle.
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:320-396: [Carried-forward, STILL VALID] put/delete race: read-modify-write over the vault without any in-process or inter-process lock
  Verified at e7e1de8e. `put` (lines 321-343) and `delete` (lines 380-396) each call `read_vault → mutate Vec<VaultEntry> → write_vault` with no `Mutex` around `EncryptedFileStoreInner` and no `flock`/`LockFileEx` on the on-disk vault. Because `CredentialStoreApi::build()` clones `Arc<EncryptedFileStoreInner>`, two credentials backed by the same store (legitimate per the SPI, common under a tokio runtime) can read the same snapshot, apply disjoint edits, and the later `tempfile::persist` atomically replaces the file — silently discarding the first writer's update with no error returned. Atomic-replace makes each individual write indivisible, but does NOT serialise the read+write window. The same race exists across processes that open the same vault directory. For a secret-storage backend, the lost write is silent secret loss: a subsequent `get` for the dropped label returns `NoEntry` and a wallet rehydration path interprets it as 'never written'. Fix: wrap the read/modify/write critical section in a `parking_lot::Mutex` on `EncryptedFileStoreInner` for in-process safety, and acquire an `fs2` exclusive advisory lock on the vault path (or a sibling `.lock` file) across the same window for cross-process safety.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:73-79: [Carried-forward, STILL VALID] vault directory permissions are never hardened or validated
  Verified at e7e1de8e. `EncryptedFileStore::open` (lines 73-79) calls `fs::create_dir_all(&dir)?` and immediately constructs the store with no chmod or metadata check on the directory itself. On Unix, a directory created under a permissive umask (commonly 0o755 or 0o775) remains traversable and listable by other local users even though each vault file is 0600 (`set_restrictive_perms` at line 254). The leaked surface is the set of `<wallet_id_hex>.pwsvault` filenames — i.e. the set of wallet IDs the user holds — plus the directory's mtime as a wallet-creation/rotation oracle. This undermines the A1 (other-local-user) threat model the file backend claims to defend. Fix: `chmod 0o700` the directory immediately after `create_dir_all`, and refuse to open an existing directory whose `mode & 0o077 != 0`, symmetric with the existing per-file `check_perms` policy.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:216-226: [Carried-forward, STILL VALID] read_vault() reads the full file before enforcing any size bound
  Verified at e7e1de8e. `read_vault` (lines 216-226) performs `fs::metadata` (line 217) for the permission check and then `fs::read(path)` (line 220) before any size cap, JSON header pre-validation, KDF-parameter sanity check, or AEAD verification. `format::deserialize(&bytes)` only runs after the full file has been allocated and read. A locally-authorised attacker who can swap the vault file (or a corrupted backup restore) can force a multi-GiB allocation in the wallet process before any authentication step runs — a trivial pre-auth DoS. Fix: enforce an explicit size cap before reading by checking `meta.len()` against a hard maximum (e.g. 16 MiB, vastly larger than the realistic seed/xpriv workload but bounded), or use `fs::File::open` + `Read::take(MAX).read_to_end(...)` and return a typed `OversizedVault` error.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:217-220: [Carried-forward, STILL VALID] permission validation has a metadata-to-read TOCTOU window
  Verified at e7e1de8e. The code validates permissions with `fs::metadata(path)` (line 217), then performs a separate `fs::read(path)` (line 220) on the same path string — two distinct syscalls operating on path resolution, not on a pinned inode. An attacker with the ability to manipulate the parent directory (the same A1 threat scenario the per-file 0600 check is meant to cover) can replace the directory entry between those syscalls so the code validates one inode's mode and then deserialises bytes from a different file. Composes naturally with finding #6 (un-hardened parent directory). Fix: open the file once with `OpenOptions` (ideally with `O_NOFOLLOW` on Unix), validate `file.metadata()?` against that handle, and read the bytes from the same FD via `file.take(MAX).read_to_end(&mut buf)` — pinning permissions and content to the same inode in one shot, which also composes with the size-cap fix above.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:553-557: [Carried-forward, STILL VALID] Windows check_perms() stub never validates ACLs on pre-existing vault files
  Verified at e7e1de8e. The non-Unix `check_perms` at lines 554-557 is still a stub that unconditionally returns `Ok(())`. The acknowledging `TODO: Windows ACL read-check is not yet implemented; tracked in PR #3672.` (line 553) acknowledges the gap but does not close it. The PR description and SECRETS.md advertise rejection of insecure pre-existing vault files as a backend invariant; on Windows that invariant is unenforced, so a vault file with an overly broad ACL (e.g. inherited Users:Read from a permissive parent) opens silently. Either implement a Windows ACL audit (e.g. via `windows-sys` `GetSecurityInfo` + DACL scan rejecting ACEs that grant access to anyone outside the current user / SYSTEM), or explicitly document in SECRETS.md and the module docs that A1 (other local user) is not enforced on Windows so operators are not misled by claimed parity.

In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/backup.rs:247-278: [Carried-forward, STILL VALID] restore_from() deletes destination WAL/SHM before fallible persist and re-harden steps
  Verified at e7e1de8e. The pre-persist chmod on the temp file (lines 265-270) was correctly added — that closes the chmod-after-persist variant — but the WAL/SHM removal at lines 247-258 still runs BEFORE the remaining fallible steps: `tmp.persist(dest_db_path)` (lines 273-274) and the post-persist `apply_secure_permissions(dest_db_path)` (line 278). If `persist` returns `Err` (rename can fail with EXDEV-like errors, EACCES, ENOSPC, EBUSY) or if `apply_secure_permissions` fails on a sibling chmod, the original `dest_db_path` is left in place while its committed `<dest>-wal` and `<dest>-shm` are already gone. SQLite in WAL mode keeps committed-but-not-checkpointed transactions in the WAL, so reopening the un-replaced destination after the restore appears to fail will silently roll back to a pre-restore committed state — a real data-loss path the caller cannot detect from the returned `Err`. The comment at lines 242-246 frames the ordering as 'every staged-file validation has now passed', but validation passing does not make `persist` infallible. Fix: move the WAL/SHM removal to AFTER `tmp.persist(...)` succeeds (the old sidecars become stale only at that atomic instant anyway), and keep `apply_secure_permissions` as the final step.

In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/persister.rs:605-651: [Carried-forward, STILL VALID] write_changeset_in_one_tx never re-hardens newly created WAL/SHM sidecars
  Verified at e7e1de8e. `apply_secure_permissions` is invoked exactly three times in the crate: `SqlitePersister::open` (persister.rs:176, one-shot at open before any sidecar is materialised), `backup::run_to` (backup.rs:114), and `backup::restore_from` (backup.rs:278). The hot-path commit at `write_changeset_in_one_tx` ends with `tx.commit()?; Ok(())` on lines 650-651 with no re-hardening pass. SQLite in WAL mode lazily materialises `<db>-wal` and `<db>-shm` on the first write transaction after open; those files are created by SQLite itself (not by `tempfile`) and therefore inherit the process umask rather than the intended 0600 hardening that the rest of the crate's threat model assumes. The same SEC-011 threat model that justifies hardening the main DB applies verbatim to the WAL — which contains the most recent committed wallet metadata, identity keys, contacts, and correlation state until checkpoint. Fix: call `apply_secure_permissions(&self.config.path)` immediately after `tx.commit()?` (idempotent and cheap), so newly created sidecars are hardened on first appearance and re-checked on every commit thereafter.

In `packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs:19-62: [Carried-forward, STILL VALID] asset_locks::apply writes self-contradictory rows that the read path will then refuse
  Verified at e7e1de8e. The read path's `decode_row` (lines 85-111) was correctly hardened to reject mismatches with `WalletStorageError::AssetLockEntryMismatch` when `entry.out_point != outpoint` or `entry.account_index != typed_account_index` (the CMT-007 cross-check at lines 104-111). The write path in `apply` (lines 36-51) however still trusts the caller-supplied `AssetLockChangeSet` blindly: it uses the map key `op` to populate the `outpoint` column and `entry.account_index` for the typed column, while the `lifecycle_blob` is `blob::encode(entry)` — with no assertion that `op == &entry.out_point` or that the typed column equals the value embedded in the blob. A malformed upstream caller (programming error, post-rebase migration drift, deserialisation round-trip bug) can persist self-contradictory rows that subsequently hard-fail every `load_state` call with `AssetLockEntryMismatch`, leaving the wallet unable to rehydrate — a self-inflicted DoS that PRAGMA integrity_check will not catch. Fix: at the top of the loop body, before the INSERT executes, assert `op == &entry.out_point` and that the typed `account_index` matches the blob's `entry.account_index`, returning the same typed `AssetLockEntryMismatch` symmetric with the reader. Fail fast at write time rather than corrupting the table.

Out-of-scope follow-up suggestions (3)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Latest delta is an unrelated merge — dapi/dapi-client changes should be reviewed on their originating PR — The db7b6b5..e7e1de8 diff is purely a merge of origin/feat/platform-wallet-sqlite-persistor (which itself merged v3.1-dev) into the secrets branch. It touches CHANGELOG.md, Cargo.lock, package.json version bumps across ~30 packages, js-dapi-client logging refactor, JsonRpcTransport changes, undici fetch shim removal, and the .pnp.cjs update. None of those files are in PR #3672's stated subject (keyring-core secret backends + SQLite hardening). Any security-relevant changes in the dapi/undici area should be reviewed on their originating PR (#3679 and adjacent) rather than absorbed into this PR's review.
  • Follow-up: Confirm the upstream dapi-client / undici / fetch / JsonRpcTransport changes are covered by their originating PR reviews; do not let this PR's blocking issues be conflated with the merged-in delta.
• Upstream key-wallet::compute_wallet_id Debug/log hygiene (AR-7 / R3) — PR description lists this as a known residual accepted risk: the type used to compute WalletId lives upstream in packages/key-wallet/ and is not in this PR's diff. Every wallet_id = %wallet_id.to_hex() tracing call in this PR (e.g. mod.rs:300, mod.rs:367) trusts that the wallet id is a non-secret correlation identifier, which depends on the upstream Debug/log surface never leaking the seed that derived it.
  • Follow-up: Open a separate hardening issue against key-wallet for compute_wallet_id Debug/log audit, and block the un-draft of #3672 (or wider rollout per AR-7) on its resolution as the PR description committed to.
• default_credential_store() fail-closed on headless Linux — operator docs gap — Per the PR description, default_credential_store() returns NoDefaultStore on headless / unknown OS instead of silently falling back to plaintext. That is the correct fail-closed posture, but CI / container / server operators will hit it and need a documented escape hatch (use EncryptedFileStore directly with a passphrase from a sealed secret / env var / k8s Secret / KMS). Not a blocker for this PR.
  • Follow-up: Author-requested follow-up: add a SECRETS.md subsection 'Headless / server deployments' documenting the EncryptedFileStore-only path and the passphrase-source recommendations.

[thepastaclaw]

Code Review

Cumulative incremental review at head 680155f vs prior review e7e1de8. The secrets module — this PR's actual scope (feat/platform-wallet-storage-secrets) — was not touched in the latest delta, which is a merge bringing in stacked PR #3625's SQLite persister work. All 9 prior findings remain STILL VALID against current source. In-scope (secrets) carried-forward findings: #1 (rekey store-wide passphrase), #2 (put/delete races), #6 (dir perms), #7 (unbounded read), #8 (metadata→read TOCTOU), #9 (Windows ACL stub). Out-of-scope (originate in stacked base PR #3625): #3 (restore_from WAL/SHM unlink ordering), #4 (sidecar hardening), #5 (asset_locks writer cross-check), plus new-delta items raised by codex (delete_wallet row-count-only footprint, restore_from lock release before persist, post-commit re-drain) — these belong on #3625, not this PR. No new in-scope defects identified in the latest delta.

🔴 2 blocking | 🟡 4 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:275-318: [carried-forward, STILL VALID] rekey() switches the store-wide passphrase after rewriting only one wallet vault
  Verified at current head — file unchanged in the latest delta. `EncryptedFileStoreInner` still holds a single `passphrase` for the whole directory (line 67), but `rekey()` only re-encrypts the vault for the supplied `wallet_id` (lines 285–315) before assigning `self.passphrase = new_passphrase` (line 316). In a directory containing vaults for wallets A and B, calling `rekey(A, new_pw)` re-encrypts only A's vault and then swaps the global passphrase; every subsequent `get`/`put`/`delete` for B derives a key from the new passphrase against B's still-old salt+ciphertext, so the AEAD verify-token fails and B surfaces as `WrongPassphrase`/`Corruption` until rekeyed by hand. The missing-vault branch at 281–284 is worse: it unconditionally swaps `self.passphrase` without rewriting any vault. The new `Arc::get_mut`/`Busy` guard on `EncryptedFileStore::rekey` (lines 96–99) closes the in-process credential-outstanding race but does not address the multi-vault desync. Either scope `passphrase` per-vault (e.g. keep it on disk in the header + a per-wallet handle), or atomically rekey every vault in the directory inside one fail-loud sequence (and refuse the swap if any vault cannot be opened with the current passphrase).
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:320-396: [carried-forward, STILL VALID] put()/delete() race silently discards concurrent writes
  Verified at current head — file unchanged. `put()` (320-343) and `delete()` (380-396) take `&self` and implement updates as `read_vault → mutate Vec<VaultEntry> → write_vault` with no in-process mutex on `EncryptedFileStoreInner` and no inter-process advisory lock around the read-modify-write window. Two credentials returned by `build()` share the `Arc<EncryptedFileStoreInner>`, so concurrent calls can read the same snapshot, apply different edits, and have the later `tempfile::persist` atomically overwrite the earlier update with a vault that omits the prior edit — no error to either caller, silent loss of wallet credentials. The atomic rename only prevents torn files; it does not serialise read-modify-write. The sibling SQLite arm protects itself with `BEGIN EXCLUSIVE`; the secrets arm has nothing equivalent. A `Mutex<()>` on `EncryptedFileStoreInner` covering read+write would close the in-process case; an OS file/dir lock would close the cross-process case.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:553-557: [carried-forward, STILL VALID] Windows skips the pre-existing vault permissions check entirely
  Verified at current head. The non-Unix `check_perms()` (554-557) is still a stub returning `Ok(())`, with an explicit `TODO` at line 553. The module-level threat-coverage commentary and PR description both claim insecure pre-existing vault files are rejected, but on Windows that invariant is unenforced: a `.pwsvault` file with an overly broad DACL opens successfully. Either implement the ACL audit (e.g. via `windows-sys`/`GetNamedSecurityInfoW`, rejecting any ACE granting access to non-owner principals beyond SYSTEM/Administrators) or surface a typed `PermissionCheckUnsupported`/downgrade the documented invariant in SECRETS.md so operators see the gap.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:73-79: [carried-forward, STILL VALID] vault directory permissions are never hardened or validated
  Verified at current head. `EncryptedFileStore::open()` still does `fs::create_dir_all(&dir)?` with no mode argument and no validation on a pre-existing directory. On Unix, a directory created under a permissive umask is world-traversable; even though each `.pwsvault` is 0600, the directory listing leaks wallet IDs (filename = `<hex_wallet_id>.pwsvault`) and their existence/mtimes to other local users, weakening the local-user threat coverage the file-level hardening advertises. Tighten the directory to 0700 on create and assert `mode & 0o077 == 0` on a pre-existing directory, mirroring `check_perms` on files (with a typed `InsecurePermissions`-style error).
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:216-226: [carried-forward, STILL VALID] read_vault() reads the whole file before enforcing any size bound
  Verified at current head. `read_vault` (216-226) calls `fs::read(path)` immediately after the metadata permission check, with no upper bound, no header-magic sniff, and no incremental validation before the slurp. A maliciously replaced or simply oversized vault file (an attacker who can write to the vault directory, or a corrupted filesystem entry) forces an unbounded `Vec<u8>` allocation before any AEAD/Argon2 step gets a chance to reject the input — process can OOM/exit before the format ever fails. Wallet vaults are small in practice; a defensive size cap (e.g. 8–64 MiB) returned as a typed `VaultTooLarge` before `fs::read` would bound the damage cheaply.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:217-220: [carried-forward, STILL VALID] permission validation has metadata-to-read TOCTOU gap
  Verified at current head. The sequence `fs::metadata(path) → check_perms(&meta) → fs::read(path)` resolves the path twice with two independent syscalls. An attacker with write access in the vault directory (the very scenario `check_perms` is meant to defend against, and compounded by the unfixed directory-mode finding) who can rename a permissive-mode file over the path between those calls makes the code validate one inode's mode and then parse a different inode's bytes. Open the file once via `OpenOptions::new().read(true).open(path)`, then call `metadata()` + permission check on the open `File`, then `read_to_end` from that same descriptor — that pins the inode for the operation. The AEAD tag will still reject foreign content, but `InsecurePermissions` is supposed to be a hard gate.

Out-of-scope follow-up suggestions (5)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• SQLite-side carried-forward findings (#3 restore_from WAL/SHM ordering, #4 sidecar permissions, #5 asset_locks writer cross-check) originate in stacked base PR #3625 — This PR (feat/platform-wallet-storage-secrets) is stacked on feat/platform-wallet-sqlite-persistor (PR #3625). The merge in the latest delta is what surfaces these files in this PR's diff against main, but the code was not authored here. All three findings re-verify against current head: backup.rs 301-352 still unlinks <dest>-wal/<dest>-shm at step 5 before fallible chmod/persist/fsync steps; persister.rs 721-731 still commits write_changeset_in_one_tx without re-running apply_secure_permissions so lazily materialised WAL/SHM sidecars retain process-umask modes; asset_locks.rs 19-62 still inserts (op, entry) without checking op == entry.out_point/account_index consistency that decode_row 104-111 then hard-fails on. They should be landed on #3625 so they don't reach v3.1-dev through this stacked PR.
  • Follow-up: Address on PR #3625 (feat/platform-wallet-sqlite-persistor) before it merges. Do not block this PR on them.
• New-delta SQLite findings raised by codex (delete_wallet row-count-only footprint; restore_from drops EXCLUSIVE lock before persist; post-commit re-drain not a deletion barrier) also belong to #3625 — Codex flagged three additional SQLite-side issues in the latest delta: (a) delete_wallet (persister.rs ~458-513) snapshots only (table_name, row_count) so a peer's in-place row update between auto-backup and EXCLUSIVE acquisition passes the equality check and the pre-delete backup is silently stale; (b) restore_from (backup.rs 336-352) intentionally drops the destination's BEGIN EXCLUSIVE connection before tmp.persist(dest_db_path), with a comment that acknowledges a peer can grab the lock and commit to the old inode in that window only to be unlinked by the rename; (c) the post-commit take_for_flush (persister.rs ~552-559) is a point-in-time drain, not a synchronisation primitive, so a racing store() arriving just after the drain can leave buffered data that a subsequent flush resurrects. I confirmed (a) and (c) by reading the persister source; the comments at backup.rs 336-348 corroborate (b). All three live in code introduced by #3625, not by this PR.
  • Follow-up: Route to PR #3625 review. For (a) consider extending the footprint to include a per-table content hash or row-version. For (b) treat the rename as the atomicity boundary and hold the lock until immediately before persist (or accept that ConcurrentMutationDuringDelete-style detection is the contract). For (c) document that delete_wallet is not a store() barrier and require callers to quiesce writers, or hold the buffer lock across the cascade.
• FFI block_on_worker + usize-laundering invariant in new platform-address asset-lock entry points — The new platform_address_wallet_fund_from_asset_lock_signer family (packages/rs-platform-wallet-ffi/src/platform_addresses/fund_from_asset_lock.rs) casts *mut SignerHandle/*mut MnemonicResolverHandle to usize so the captured future is Send + 'static, then resurrects the pointers inside the async block. Sound today because block_on_worker synchronously parks the FFI caller until the spawned task returns, pinning both handles alive for the call. If block_on_worker is ever changed to be non-blocking, the resurrected references become dangling on a different thread — classic UAF across the FFI boundary. The contract is documented in SAFETY comments but has no compile-time guard.
  • Follow-up: Open a follow-up hardening issue to wrap the usize-laundering pattern in an unsafe newtype carrying the blocking-call contract at the type level, and add a doc-asserted invariant to block_on_worker that any caller using this pattern depends on its blocking semantics.
• Asset-lock funding orchestration (~690s worst-case latency budget) is not operator-tunable — submit_with_cl_height_retry (orchestration.rs) and fund_from_asset_lock.rs hard-code latency constants summing to ~690s in the worst case. Not a security defect, but synchronous use from a UI thread is operationally problematic and an IS-rejection-style adversary can push every transaction to that ceiling. This is new code arriving via the #3671 (platform-address funding) merge, not authored on this PR's branch.
  • Follow-up: Track operator-tunable timeouts (PutSettings-style) for CL_FALLBACK_TIMEOUT, CL_HEIGHT_RETRY_DELAY, CL_HEIGHT_RETRY_BUDGET in a separate issue against the asset-lock orchestration code.
• Upstream key_wallet::compute_wallet_id Debug/log hygiene (AR-7 / R3) — Acknowledged in the PR description as an accepted residual outside scope. The wallet_id is treated as a pseudonymous correlation key by this crate, but a careless {:?} upstream could spill it into logs. The PR's secrets_guard covers src/secrets/ only.
  • Follow-up: Audit key_wallet::compute_wallet_id and related types for Debug/Display/tracing exposure before wider rollout. Separate issue against the key-wallet crate.

[thepastaclaw]

🟡 Suggestion: [carried-forward, STILL VALID] permission validation has metadata-to-read TOCTOU gap

Verified at current head. The sequence fs::metadata(path) → check_perms(&meta) → fs::read(path) resolves the path twice with two independent syscalls. An attacker with write access in the vault directory (the very scenario check_perms is meant to defend against, and compounded by the unfixed directory-mode finding) who can rename a permissive-mode file over the path between those calls makes the code validate one inode's mode and then parse a different inode's bytes. Open the file once via OpenOptions::new().read(true).open(path), then call metadata() + permission check on the open File, then read_to_end from that same descriptor — that pins the inode for the operation. The AEAD tag will still reject foreign content, but InsecurePermissions is supposed to be a hard gate.

_{source: ['claude', 'codex']}

[thepastaclaw]

Code Review

Cumulative incremental review of PR #3672 at head 9884f22. The latest 4-commit delta (KdfDescriptor→KdfParams collapse, Header/VaultFile→Vault collapse, Entry/EntryRecord collapse, const-generic hex_array adapter) is a structural refactor that preserves the on-disk format and adds parse-time width validation; I verified it introduces no new defects. Status of the 6 prior findings against current head: all 6 are STILL VALID (none FIXED, OUTDATED, or INTENTIONALLY DEFERRED). The two blocking issues — rekey() updating the store-wide passphrase after rewriting only one wallet's vault, and put/delete read-modify-write without locking — remain the merge blockers; the four suggestion-tier defense-in-depth gaps (Windows check_perms stub, unhardened vault directory permissions, unbounded fs::read before authentication, metadata→read TOCTOU) also persist. No new latest-delta findings.

🔴 2 blocking | 🟡 4 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:270-314: [carried-forward, STILL VALID] rekey() switches the store-wide passphrase after rewriting only one wallet's vault
  Verified at head 9884f22725 against mod.rs:270-314. `EncryptedFileStoreInner` holds a single store-wide `passphrase: SecretString` (line 67) but `rekey(wallet_id, new_passphrase)` only re-encrypts the vault file at `self.vault_path(&wallet_id)` (lines 280-310) before unconditionally executing `self.passphrase = new_passphrase` (line 312). In any directory hosting vaults for wallets A and B opened under one store, `rekey(A, new)` leaves vault B's bytes encrypted under the old passphrase while every subsequent `read_vault`/`derive_and_verify` for B derives from the new one — vault B is bricked from this handle, surfacing as `WrongPassphrase` indistinguishable from real corruption. The missing-vault branch at lines 276-279 is worse: it sets `self.passphrase = new_passphrase` and returns `Ok(())` with no on-disk re-encryption, silently rotating the global passphrase against zero vaults and locking out every populated wallet in the directory. The newly added `Arc::get_mut`/`Busy` guard at line 96 only prevents the swap while live credentials hold the inner Arc — it does not bound the passphrase scope. Fix: either move passphrase ownership per-vault, or re-encrypt every `*.pwsvault` in `self.dir` atomically under the new passphrase, or refuse `rekey` when more than one vault file exists in `dir`.
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:316-389: [carried-forward, STILL VALID] put()/delete() lose concurrent updates via unsynchronized read-modify-write
  Verified at head 9884f22725 against mod.rs:317-389. `EncryptedFileStoreInner::put` (lines 317-336) and `::delete` (lines 373-389) both take `&self` and implement updates as `read_vault → mutate Vec<VaultEntry> → write_vault`. `CredentialStoreApi::build` (line 512) hands out multiple `EncryptedFileCredential`s that each clone the same `Arc<EncryptedFileStoreInner>`, and the public `put_bytes`/`delete_bytes` (lines 106-133) also take `&self`. There is no in-process mutex around the read-modify-write window and no inter-process advisory lock (e.g. `fcntl`/`flock`/`fs2::FileExt`). Two threads (or two processes opening the same directory) concurrently calling `put(W, "A", ...)` and `put(W, "B", ...)` each read the same baseline `vault.entries`, append their own entry, and the later `tempfile::persist` atomically replaces the file — the earlier write is silently lost. The same hazard applies to delete-vs-put and to concurrent rehydration vs key-import flows in the wallet app. The atomic rename gives torn-write protection but no compare-and-swap semantics. Fix: take a `Mutex`/`RwLock` (per dir, or per `(dir, wallet_id)`) spanning the entire read→write block in `EncryptedFileStoreInner`, plus an OS advisory file lock around the same window if multi-process use is supported.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:546-550: [carried-forward, STILL VALID] Windows skips the pre-existing vault permissions check entirely
  Verified at head 9884f22725. The non-Unix `check_perms` at mod.rs:547-550 is a stub returning `Ok(())`, with the explicit TODO at line 546 (`Windows ACL read-check is not yet implemented; tracked in PR #3672.`). The PR description and SECRETS.md threat model claim insecure pre-existing vault files are rejected, but on Windows a `.pwsvault` file with an overly broad DACL opens successfully — the documented invariant does not hold cross-platform. The companion `set_restrictive_perms` Windows stub (lines 559-562) is a no-op too, so at-rest hardening on Windows reduces to whatever ACL `tempfile` happens to apply. Either implement an ACL inspection on Windows (`GetSecurityInfo`/`GetEffectiveRightsFromAclW`) that approximates owner-only access, fail closed when the file backend is opened on non-Unix, or document the gap in SECRETS.md under accepted residuals (it is not currently listed).
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:73-79: [carried-forward, STILL VALID] Vault directory permissions are never hardened or validated
  Verified at head 9884f22725. `EncryptedFileStore::open` at mod.rs:73-79 calls `fs::create_dir_all(&dir)` with no permission tightening on a newly-created directory and no validation of an existing one. Per `create_dir_all`, the directory mode is governed by the process umask, so under the common umask 022 the parent can be world-readable / world-traversable. Even though the vault files themselves are 0600, listing the directory leaks the wallet-id hex filenames (which directly tag the user's wallets) and the per-file mtimes (which leak credential write activity to inotify or directory listings) — a real metadata side-channel for threat A1 (other local user). For consistency with the 0600 file invariant, the directory should be set to 0700 on creation and a pre-existing directory with any bit in 0o077 should at least surface a typed warning/error.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:216-226: [carried-forward, STILL VALID] read_vault() reads the entire file before enforcing any size bound
  Verified at head 9884f22725. `read_vault` at mod.rs:216-226 calls `fs::read(path)` (line 220) immediately after the permission check, with no explicit upper bound, no header sniff, no KDF validation, and no AEAD verification before allocating. `format::deserialize` then does `serde_json::from_slice` over the entire allocation and parses every entry. A malicious replacement or accidentally enormous `.pwsvault` (multi-GiB) forces an unbounded `Vec<u8>` allocation before any cryptographic check fires — a local DoS / OOM vector for any process that can write the vault directory or a hostile synced backup that can replace the file. The format-layer KDF ceiling (`ARGON2_MAX_M_KIB = 1 GiB`) bounds compute cost but not disk-read cost. Fix: check `meta.len()` against an explicit ceiling (e.g. 1-16 MiB — comfortably above any realistic vault) and error with a typed `MalformedVault`/`VaultTooLarge` before `fs::read`.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:217-220: [carried-forward, STILL VALID] permission validation has a metadata→read TOCTOU gap
  Verified at head 9884f22725. The sequence at mod.rs:217-220 is `fs::metadata(path) → check_perms(&meta) → fs::read(path)`, which resolves the path twice. An attacker able to swap the directory entry between those two syscalls (e.g. by swapping a symlink target) can have `check_perms` validate inode A's 0600 mode and `fs::read` consume bytes from a different inode whose mode was never checked — defeating the entire point of the `InsecurePermissions` invariant the doc-comment promises. Low practical exploitability under the documented threat model (an attacker who can rewrite the parent directory mid-call is already partially in control), but the mitigation is cheap and the invariant is otherwise hard. Fix: open once with `File::open(path)`, then take `File::metadata` on the open handle and read from that same handle so every check binds to one inode.

Out-of-scope follow-up suggestions (3)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• AR-7 / R3 — upstream key-wallet::compute_wallet_id Debug/log hygiene — Already disclosed in the PR's 'Known limitations and accepted risks'. Lives in the key-wallet crate, not in the secrets/ backend. Not introduced by this PR; flagged only so it is not lost in the downstream PR cascade.
  • Follow-up: Open a separate hardening issue against key-wallet to scrub Debug/log surfaces that could echo wallet-id-adjacent secret-derived data, before wider wallet rollout.
• Downstream PR cascade (#3692 / #3693) needs rebase to the new CredentialStoreApi seam — The PR description tracks this explicitly — the adapter referenced in downstream PRs needs to wrap the upstream CredentialStoreApi rather than the retired bespoke SecretStore trait. Not a defect in this PR.
  • Follow-up: Rebase #3692 and #3693 after this PR lands and update seed_provider_adapter.rs to the new seam.
• Operator-doc gap for headless-Linux fail-closed default — default_credential_store() returns NoDefaultStore on headless Linux, which is the correct fail-closed behavior. Operators deploying to mobile-on-server / CI without anticipating the headless case will see wallet rehydration fail at boot; this is a docs issue rather than a defect.
  • Follow-up: Add a SECRETS.md / runbook entry pointing operators at EncryptedFileStore for headless Linux.

[codecov]

Codecov Report

❌ Patch coverage is 91.25077% with 142 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.10%. Comparing base (64a85a8) to head (6ca1bc1).
⚠️ Report is 2 commits behind head on feat/platform-wallet-sqlite-persistor.

Files with missing lines	Patch %	Lines
...es/rs-platform-wallet-storage/src/secrets/store.rs	68.83%	48 Missing ⚠️
...rs-platform-wallet-storage/src/secrets/file/mod.rs	94.12%	44 Missing ⚠️
...s/rs-platform-wallet-storage/src/secrets/secret.rs	79.54%	36 Missing ⚠️
...-platform-wallet-storage/src/secrets/file/error.rs	91.78%	6 Missing ⚠️
packages/rs-platform-wallet-storage/src/lib.rs	0.00%	5 Missing ⚠️
.../rs-platform-wallet-storage/src/secrets/keyring.rs	84.61%	2 Missing ⚠️
...platform-wallet-storage/src/secrets/file/format.rs	99.56%	1 Missing ⚠️

Additional details and impacted files

@@                            Coverage Diff                            @@
##           feat/platform-wallet-sqlite-persistor    #3672      +/-   ##
=========================================================================
- Coverage                                  87.17%   87.10%   -0.07%     
=========================================================================
  Files                                       2607     2638      +31     
  Lines                                     319489   323964    +4475     
=========================================================================
+ Hits                                      278507   282203    +3696     
- Misses                                     40982    41761     +779     

Components	Coverage Δ
dpp	87.71% <ø> (-0.01%)	⬇️
drive	85.95% <ø> (ø)
drive-abci	89.60% <ø> (ø)
sdk	∅ <ø> (∅)
dapi-client	∅ <ø> (∅)
platform-version	∅ <ø> (∅)
platform-value	92.20% <ø> (+0.03%)	⬆️
platform-wallet	∅ <ø> (∅)
drive-proof-verifier	49.16% <ø> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[thepastaclaw]

Code Review

Cumulative incremental review at head f4a9a92 vs prior 9884f22. Prior finding statuses: #1 (rekey passphrase desync) STILL VALID; #2 (RMW races) FIXED via with_vault_lock (mod.rs:239-269); #3 (Windows perm-check) INTENTIONALLY DEFERRED with explicit marker + tracked issue #3754; #4 (dir perms) FIXED on Unix via set_restrictive_dir_perms (mod.rs:788-808); #5 (read_vault size bound) FIXED via pre-allocation cap check (mod.rs:336-344); #6 (metadata→read TOCTOU) FIXED via open_no_follow + fd-derived metadata (mod.rs:330-347, 760-767). One blocking carried-forward finding remains plus two small new latest-delta observations (with_vault_lock error collapsing all try_write errors to Busy; minor symlink-follow surfaces in helpers). REQUEST_CHANGES on the rekey bug.

🔴 1 blocking | 🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:410-452: [carried-forward, STILL VALID] rekey() swaps store-wide passphrase after re-encrypting only one wallet's vault
  Re-verified at head f4a9a92a (mod.rs:410-452). `EncryptedFileStoreInner` still owns a single store-wide `passphrase: SecretString` (mod.rs:87), used by `derive_and_verify` (mod.rs:306-318) for every vault in the directory. `rekey(wallet_id, new_passphrase)` re-encrypts only `self.vault_path(&wallet_id)` (lines 422-448) inside `with_vault_lock`, then unconditionally executes `self.passphrase = new_passphrase` at line 450 — outside the per-wallet lock and irrespective of how many other vault files share the directory.

  The module-level doc (mod.rs:3) and `vault_path` (mod.rs:205-207) confirm the intended layout is multiple `{wallet_id}.pwsvault` files in one directory under a single store handle. Two distinct exploitable shapes survive Waves 1–4:

    1. **Multi-wallet store lockout.** A directory holding `wallet_A.pwsvault` and `wallet_B.pwsvault`: caller invokes `rekey(A, new_pp)`. Only A is re-encrypted under `new_pp`; B's on-disk vault still carries the old salt sealed under the old passphrase. Every subsequent read/put/delete on B runs `derive_and_verify` with `new_pp`, fails the verify-token AEAD tag, and surfaces `WrongPassphrase` — silent denial-of-access to data the user actually owns.

    2. **Missing-target-vault passphrase swap.** The `None` arm at lines 424-428 returns `Ok(())` from the closure without touching disk, but control still falls through to `self.passphrase = new_passphrase`. So `rekey(unknown_wallet, new_pp)` silently re-passphrases the store handle without re-encrypting any existing sibling vaults.

  The in-line comment at lines 425-426 documents the swap as intentional, but the `passphrase` field is store-wide while the rekey is per-wallet — the contract is internally inconsistent for any directory containing more than one vault. Wave-1 locking and the outer `Arc::get_mut` gate only protect against concurrent writers; they do not change the per-wallet-rewrite vs store-wide-replace asymmetry. Fix options: (a) iterate every `*.pwsvault` in the directory under all locks and re-encrypt in lockstep; (b) move the passphrase to a per-wallet store (one wallet per directory); (c) enforce at `open()` that the dir contains no more than one vault file and document the single-wallet invariant.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:257-267: [new] with_vault_lock collapses every io::Error from try_write into Busy
  `with_vault_lock` (mod.rs:257-267) loops on `rw.try_write()` and matches `Err(_)` once for the retry branch and once for the terminal `FileStoreError::Busy` branch — every `io::Error` variant is treated as contention. `fd_lock::RwLock::try_write` returns `ErrorKind::WouldBlock` only for genuine lock contention; other errno classes (EBADF, EIO, kernel lock-table exhaustion, fs eviction) are real I/O faults. As written those faults are silently retried until the wait budget expires and then projected to `Busy`, hiding both the original errno and the appropriate `FileStoreError::Io` channel. The `Busy` retry hint is also misleading for non-contention faults. Inspect `err.kind()` and only treat `WouldBlock` as contention; surface anything else as `Err(FileStoreError::Io(err))` immediately. Diagnostic-quality issue, not a correctness one.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:731-740: [carried-forward, INTENTIONALLY DEFERRED] Windows skips pre-existing vault permissions check
  Re-verified at head f4a9a92a. The non-Unix `check_perms` (mod.rs:737-740) and `set_restrictive_dir_perms` (mod.rs:816-819) are still no-op stubs. The prior TODO has been promoted to explicit `INTENTIONAL(CMT-007)` / `INTENTIONAL(CMT-002)` markers (lines 731-736, 810-815) referencing tracked follow-up https://github.com/dashpay/platform/issues/3754, and the `EncryptedFileStore::open` doc comment now flags the Windows operator responsibility. Reclassified from STILL VALID to INTENTIONALLY DEFERRED for this review — not a regression, but the A1/A4/A6 at-rest guarantees in SECRETS.md remain Unix-only until #3754 lands. Surfacing for visibility, not blocking.

[thepastaclaw]

🔴 Blocking: [carried-forward, STILL VALID] rekey() swaps store-wide passphrase after re-encrypting only one wallet's vault

Re-verified at head f4a9a92 (mod.rs:410-452). EncryptedFileStoreInner still owns a single store-wide passphrase: SecretString (mod.rs:87), used by derive_and_verify (mod.rs:306-318) for every vault in the directory. rekey(wallet_id, new_passphrase) re-encrypts only self.vault_path(&wallet_id) (lines 422-448) inside with_vault_lock, then unconditionally executes self.passphrase = new_passphrase at line 450 — outside the per-wallet lock and irrespective of how many other vault files share the directory.

The module-level doc (mod.rs:3) and vault_path (mod.rs:205-207) confirm the intended layout is multiple {wallet_id}.pwsvault files in one directory under a single store handle. Two distinct exploitable shapes survive Waves 1–4:

1. Multi-wallet store lockout. A directory holding wallet_A.pwsvault and wallet_B.pwsvault: caller invokes rekey(A, new_pp). Only A is re-encrypted under new_pp; B's on-disk vault still carries the old salt sealed under the old passphrase. Every subsequent read/put/delete on B runs derive_and_verify with new_pp, fails the verify-token AEAD tag, and surfaces WrongPassphrase — silent denial-of-access to data the user actually owns.

2. Missing-target-vault passphrase swap. The None arm at lines 424-428 returns Ok(()) from the closure without touching disk, but control still falls through to self.passphrase = new_passphrase. So rekey(unknown_wallet, new_pp) silently re-passphrases the store handle without re-encrypting any existing sibling vaults.

The in-line comment at lines 425-426 documents the swap as intentional, but the passphrase field is store-wide while the rekey is per-wallet — the contract is internally inconsistent for any directory containing more than one vault. Wave-1 locking and the outer Arc::get_mut gate only protect against concurrent writers; they do not change the per-wallet-rewrite vs store-wide-replace asymmetry. Fix options: (a) iterate every *.pwsvault in the directory under all locks and re-encrypt in lockstep; (b) move the passphrase to a per-wallet store (one wallet per directory); (c) enforce at open() that the dir contains no more than one vault file and document the single-wallet invariant.

_{source: ['claude', 'codex']}

[thepastaclaw]

🟡 Suggestion: [new] with_vault_lock collapses every io::Error from try_write into Busy

with_vault_lock (mod.rs:257-267) loops on rw.try_write() and matches Err(_) once for the retry branch and once for the terminal FileStoreError::Busy branch — every io::Error variant is treated as contention. fd_lock::RwLock::try_write returns ErrorKind::WouldBlock only for genuine lock contention; other errno classes (EBADF, EIO, kernel lock-table exhaustion, fs eviction) are real I/O faults. As written those faults are silently retried until the wait budget expires and then projected to Busy, hiding both the original errno and the appropriate FileStoreError::Io channel. The Busy retry hint is also misleading for non-contention faults. Inspect err.kind() and only treat WouldBlock as contention; surface anything else as Err(FileStoreError::Io(err)) immediately. Diagnostic-quality issue, not a correctness one.

_{source: ['claude']}

[thepastaclaw]

🟡 Suggestion: [carried-forward, INTENTIONALLY DEFERRED] Windows skips pre-existing vault permissions check

Re-verified at head f4a9a92. The non-Unix check_perms (mod.rs:737-740) and set_restrictive_dir_perms (mod.rs:816-819) are still no-op stubs. The prior TODO has been promoted to explicit INTENTIONAL(CMT-007) / INTENTIONAL(CMT-002) markers (lines 731-736, 810-815) referencing tracked follow-up #3754, and the EncryptedFileStore::open doc comment now flags the Windows operator responsibility. Reclassified from STILL VALID to INTENTIONALLY DEFERRED for this review — not a regression, but the A1/A4/A6 at-rest guarantees in SECRETS.md remain Unix-only until #3754 lands. Surfacing for visibility, not blocking.

_{source: ['claude', 'codex']}

[thepastaclaw]

Code Review

Prior finding reconciliation for f0f69fa: carried forward STILL VALID prior findings: #2:with-vault-lock-collapses-io-errors-to-busy; fixed/outdated prior findings: #1:file-store-rekey-store-wide-passphrase-single-wallet; intentionally deferred prior findings: #3:windows-vault-permissions-intentionally-deferred. Latest-delta findings are listed separately when present.

Latest delta (f4a9a92 → f0f69fa) is a one-line Cargo.toml fix enabling the keychain feature on apple-native-keyring-store to satisfy the upstream crate's mandatory feature gate for macOS builds. No new defects introduced in the delta. Two prior in-scope findings remain valid at the current head and are carried forward: (1) rekey() still mutates the store-wide self.passphrase after re-encrypting only one wallet's vault, a data-loss bug for multi-wallet stores in this PR's new public API; (2) with_vault_lock still maps every try_write Err(_) to Busy, hiding genuine I/O failures. The Windows ACL gap is intentionally deferred to follow-up #3754.

🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:257-267: with_vault_lock collapses every try_write I/O error into Busy
  Verified at f0f69fa (mod.rs:259-267): the loop matches `Err(_) if Instant::now() < deadline` for retry and the bare `Err(_)` arm at line 265 returns `FileStoreError::Busy`. `fd_lock::RwLock::try_write` returns `io::Error` that may carry kinds other than `WouldBlock` (EIO, EBADF on a closed lock fd, EACCES after a permission flip on the lock file, ENOSPC during lock-state changes, etc.). All of those genuine failures get hidden under `Busy`, which the SPI maps to a transient/retryable signal — operators see a misleading 'someone else holds the lock' instead of the real fault, and consumers can spin on a permanent failure. Match `e.kind() == ErrorKind::WouldBlock` (optionally also `Interrupted`) for the retry/`Busy` path and propagate other `io::Error`s through the existing `FileStoreError::Io` path so the `tracing` and operator diagnostics added elsewhere in this PR aren't undone here.

Review details

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 257-267: with_vault_lock collapses every try_write I/O error into Busy
  Verified at f0f69fa (mod.rs:259-267): the loop matches `Err(_) if Instant::now() < deadline` for retry and the bare `Err(_)` arm at line 265 returns `FileStoreError::Busy`. `fd_lock::RwLock::try_write` returns `io::Error` that may carry kinds other than `WouldBlock` (EIO, EBADF on a closed lock fd, EACCES after a permission flip on the lock file, ENOSPC during lock-state changes, etc.). All of those genuine failures get hidden under `Busy`, which the SPI maps to a transient/retryable signal — operators see a misleading 'someone else holds the lock' instead of the real fault, and consumers can spin on a permanent failure. Match `e.kind() == ErrorKind::WouldBlock` (optionally also `Interrupted`) for the retry/`Busy` path and propagate other `io::Error`s through the existing `FileStoreError::Io` path so the `tracing` and operator diagnostics added elsewhere in this PR aren't undone here.

Reviewed commit: f0f69fae

[thepastaclaw]

🟡 Suggestion: with_vault_lock collapses every try_write I/O error into Busy

Verified at f0f69fa (mod.rs:259-267): the loop matches Err(_) if Instant::now() < deadline for retry and the bare Err(_) arm at line 265 returns FileStoreError::Busy. fd_lock::RwLock::try_write returns io::Error that may carry kinds other than WouldBlock (EIO, EBADF on a closed lock fd, EACCES after a permission flip on the lock file, ENOSPC during lock-state changes, etc.). All of those genuine failures get hidden under Busy, which the SPI maps to a transient/retryable signal — operators see a misleading 'someone else holds the lock' instead of the real fault, and consumers can spin on a permanent failure. Match e.kind() == ErrorKind::WouldBlock (optionally also Interrupted) for the retry/Busy path and propagate other io::Errors through the existing FileStoreError::Io path so the tracing and operator diagnostics added elsewhere in this PR aren't undone here.

Suggested change

	let mut rw = fd_lock::RwLock::new(lock_file);
	let deadline = Instant::now() + LOCK_WAIT_BUDGET;
	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(_) if Instant::now() < deadline => {
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(_) => return Err(FileStoreError::Busy),
	}
	};
	let mut rw = fd_lock::RwLock::new(lock_file);
	let deadline = Instant::now() + LOCK_WAIT_BUDGET;
	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(e) if e.kind() == std::io::ErrorKind::WouldBlock
	&& Instant::now() < deadline =>
	{
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {
	return Err(FileStoreError::Busy);
	}
	Err(e) => return Err(FileStoreError::Io(e)),
	}
	};

_{source: ['claude', 'codex']}

[thepastaclaw]

Code Review

Prior finding reconciliation for f0f69fa: carried forward STILL VALID prior findings: #1:file-store-rekey-store-wide-passphrase-single-wallet, #2:with-vault-lock-collapses-io-errors-to-busy; fixed/outdated prior findings: none; intentionally deferred prior findings: #3:windows-vault-permissions-intentionally-deferred.

Latest delta (f4a9a92 → f0f69fa) only enables the keychain feature on the macOS apple-native-keyring-store dependency, and I did not find a new defect in that delta. The unresolved prior rekey data-loss bug and the lock-error taxonomy issue are still present at this head and are carried forward; the Windows ACL gap remains explicitly deferred to #3754.

🔴 1 blocking | 🟡 1 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [BLOCKING] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:410-452: rekey() swaps the store-wide passphrase after re-encrypting only one wallet's vault
  Verified at f0f69fae (mod.rs:410-452). `EncryptedFileStoreInner` still owns a single store-wide `passphrase: SecretString` (line 87), and `derive_and_verify` uses it for every vault in the directory. `rekey(wallet_id, new_passphrase)` computes `path = self.vault_path(&wallet_id)`, re-encrypts only that wallet's vault under the new key, writes only that one path, then assigns `self.passphrase = new_passphrase`. Any other existing wallet vault in the same store remains sealed under the old passphrase but future operations now derive with the new one, so rekeying one wallet can make the rest of the store unreadable until reopened with the old passphrase. For the new multi-wallet SecretStore surface, that is a data-loss bug.
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:257-267: with_vault_lock collapses every try_write I/O error into Busy
  Verified at f0f69fae (mod.rs:259-267): the lock loop matches `Err(_) if Instant::now() < deadline` for retry and a bare `Err(_)` returns `FileStoreError::Busy`. `fd_lock::RwLock::try_write` can surface real `io::Error`s as well as `WouldBlock`; those failures are now hidden as a transient contention signal, which can make operators/consumers retry a permanent lock-file or filesystem fault instead of seeing the real error.

Out-of-scope follow-up suggestions (1)

These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.

• Windows skips the pre-existing vault permissions check — The non-Unix permission helpers are still no-op stubs, but the current head explicitly marks the gap as INTENTIONAL(CMT-007) / INTENTIONAL(CMT-002) and tracks it in follow-up issue #3754, so this is intentionally deferred rather than a fresh blocker for this push.
  • Follow-up: Handle via the referenced Windows ACL follow-up (#3754).

Reviewed commit: f0f69fae

Note: this supersedes my earlier same-SHA review body, which incorrectly classified prior rekey finding #1 as fixed while the current file still shows it is valid.

[thepastaclaw]

Code Review

Prior finding reconciliation for 48b0749: carried forward STILL VALID prior findings: #1:with-vault-lock-collapses-io-errors-to-busy; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.

Cumulative wallet-secret-storage PR (file vault + OS-keyring backends) remains cryptographically sound. The latest f0f69fa..48b0749 delta is a 6-line macOS-only fix moving Store under apple_native_keyring_store::keychain — verified correct, no new defects. Two carried/identified suggestions stand: the prior finding about with_vault_lock collapsing every fd_lock::try_write io::Error into Busy is STILL VALID and unchanged at mod.rs:259-265, and the OS-keyring build_os helper skips the crate's documented label allowlist before passing into native credential stores.

🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:259-266: with_vault_lock reports every try_write I/O error as transient Busy
  The retry loop matches `Err(_) if Instant::now() < deadline` and the terminal `Err(_) => return Err(FileStoreError::Busy)` arm catches every `io::Error` kind, not just `WouldBlock`. `fd_lock::RwLock::try_write` returns plain `io::Error`, so genuine non-contention failures — `EACCES` after a permission flip on the lock file, `EBADF` on a closed fd, `EIO`, `ENOSPC` — get reported as `FileStoreError::Busy`, which the SPI projection maps to `KeyringError::NoStorageAccess` (a retryable signal) and which the `FileStoreError::Busy` doc-comment explicitly limits to two recoverable causes (rekey-with-outstanding-cred and cross-process advisory-lock contention). Operators see a misleading 'someone else holds the lock' for permanent faults and consumers can spin on a non-retryable failure. Match only `WouldBlock` for retry/Busy and bubble every other `io::Error` through the existing `From<io::Error>` path.

In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/store.rs:123-130: build_os skips the label allowlist before passing into native keyring backends
  `validate.rs:1-6` documents the crate contract that `label` is reject-not-sanitize against `^[A-Za-z0-9._-]{1,64}$` *before any backend maps it to a filename or a keyring attribute*, and the file backend honors this everywhere (mod.rs:467, 495, 518, 595, 612, 626, 667). `build_os` does not: it forwards the caller-supplied `label` straight into `store.build(&svc, label, None)`, so non-ASCII, control characters, path separators, NULs, or overlong inputs reach the per-platform credential store and get accepted, normalized, or rejected differently depending on backend (Secret Service vs Keychain vs Credential Manager vs linux-keyutils). The result is cross-platform behavior drift and loss of the documented reject-before-mapping invariant. Validate at the seam, mirroring the file backend.

Review details

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 259-266: with_vault_lock reports every try_write I/O error as transient Busy
  The retry loop matches `Err(_) if Instant::now() < deadline` and the terminal `Err(_) => return Err(FileStoreError::Busy)` arm catches every `io::Error` kind, not just `WouldBlock`. `fd_lock::RwLock::try_write` returns plain `io::Error`, so genuine non-contention failures — `EACCES` after a permission flip on the lock file, `EBADF` on a closed fd, `EIO`, `ENOSPC` — get reported as `FileStoreError::Busy`, which the SPI projection maps to `KeyringError::NoStorageAccess` (a retryable signal) and which the `FileStoreError::Busy` doc-comment explicitly limits to two recoverable causes (rekey-with-outstanding-cred and cross-process advisory-lock contention). Operators see a misleading 'someone else holds the lock' for permanent faults and consumers can spin on a non-retryable failure. Match only `WouldBlock` for retry/Busy and bubble every other `io::Error` through the existing `From<io::Error>` path.

In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] lines 123-130: build_os skips the label allowlist before passing into native keyring backends
  `validate.rs:1-6` documents the crate contract that `label` is reject-not-sanitize against `^[A-Za-z0-9._-]{1,64}$` *before any backend maps it to a filename or a keyring attribute*, and the file backend honors this everywhere (mod.rs:467, 495, 518, 595, 612, 626, 667). `build_os` does not: it forwards the caller-supplied `label` straight into `store.build(&svc, label, None)`, so non-ASCII, control characters, path separators, NULs, or overlong inputs reach the per-platform credential store and get accepted, normalized, or rejected differently depending on backend (Secret Service vs Keychain vs Credential Manager vs linux-keyutils). The result is cross-platform behavior drift and loss of the documented reject-before-mapping invariant. Validate at the seam, mirroring the file backend.

Reviewed commit: 48b0749c

[thepastaclaw]

🟡 Suggestion: with_vault_lock reports every try_write I/O error as transient Busy

The retry loop matches Err(_) if Instant::now() < deadline and the terminal Err(_) => return Err(FileStoreError::Busy) arm catches every io::Error kind, not just WouldBlock. fd_lock::RwLock::try_write returns plain io::Error, so genuine non-contention failures — EACCES after a permission flip on the lock file, EBADF on a closed fd, EIO, ENOSPC — get reported as FileStoreError::Busy, which the SPI projection maps to KeyringError::NoStorageAccess (a retryable signal) and which the FileStoreError::Busy doc-comment explicitly limits to two recoverable causes (rekey-with-outstanding-cred and cross-process advisory-lock contention). Operators see a misleading 'someone else holds the lock' for permanent faults and consumers can spin on a non-retryable failure. Match only WouldBlock for retry/Busy and bubble every other io::Error through the existing From<io::Error> path.

Suggested change

	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(_) if Instant::now() < deadline => {
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(_) => return Err(FileStoreError::Busy),
	}
	let mut rw = fd_lock::RwLock::new(lock_file);
	let deadline = Instant::now() + LOCK_WAIT_BUDGET;
	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {
	if Instant::now() >= deadline {
	return Err(FileStoreError::Busy);
	}
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(e) => return Err(FileStoreError::from(e)),
	}
	};

_{source: ['claude', 'codex']}

[thepastaclaw]

🟡 Suggestion: build_os skips the label allowlist before passing into native keyring backends

validate.rs:1-6 documents the crate contract that label is reject-not-sanitize against ^[A-Za-z0-9._-]{1,64}$ before any backend maps it to a filename or a keyring attribute, and the file backend honors this everywhere (mod.rs:467, 495, 518, 595, 612, 626, 667). build_os does not: it forwards the caller-supplied label straight into store.build(&svc, label, None), so non-ASCII, control characters, path separators, NULs, or overlong inputs reach the per-platform credential store and get accepted, normalized, or rejected differently depending on backend (Secret Service vs Keychain vs Credential Manager vs linux-keyutils). The result is cross-platform behavior drift and loss of the documented reject-before-mapping invariant. Validate at the seam, mirroring the file backend.

Suggested change

	fn build_os(
	store: &Arc<dyn CredentialStoreApi + Send + Sync>,
	service: &WalletId,
	label: &str,
	) -> Result<Entry, FileStoreError> {
	let svc = format!("{SERVICE_PREFIX}{}", service.to_hex());
	store.build(&svc, label, None).map_err(map_spi)
	}
	fn build_os(
	store: &Arc<dyn CredentialStoreApi + Send + Sync>,
	service: &WalletId,
	label: &str,
	) -> Result<Entry, FileStoreError> {
	let label = super::validate::validated_label(label).map_err(FileStoreError::from)?;
	let svc = format!("{SERVICE_PREFIX}{}", service.to_hex());
	store.build(&svc, label, None).map_err(map_spi)
	}

_{source: ['codex']}

[thepastaclaw]

Code Review

Prior finding reconciliation for 6ca1bc1: carried forward STILL VALID prior findings: #1:with-vault-lock-reports-try-write-io-errors-as-busy, #2:build-os-skips-label-allowlist-before-native-keyring; fixed/outdated prior findings: none; intentionally deferred prior findings: none. Latest-delta findings are listed separately when present.

Latest delta (6ca1bc1) is a small, correct error-construction refactor in rs-platform-wallet-ffi/src/persistence.rs (drops From<String> lift in favor of explicit PersistenceError::backend(...)). No new defects introduced. Both prior findings remain STILL VALID at HEAD: with_vault_lock collapses every try_write io::Error to Busy, and build_os bypasses the documented label allowlist on the OS-keyring arm. Carried forward as suggestions; no blockers.

🟡 2 suggestion(s)

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/file/mod.rs:259-267: with_vault_lock collapses every try_write io::Error into Busy
  `fd_lock::RwLock::try_write` returns a plain `io::Error` whose `ErrorKind` distinguishes lock contention (`WouldBlock`) from real I/O faults (`PermissionDenied`, `BadFileDescriptor`, EIO, etc.). The current loop matches `Err(_)` in both the retry and terminal arms, so any non-contention failure is busy-spun until `LOCK_WAIT_BUDGET` expires and then surfaced to callers as `FileStoreError::Busy`. This violates the variant's documented contract ("another process is mid-write, retry") and hides real fs failures (permissions/quota/tampered sidecar) behind a transient retry signal, prompting operators to retry on errors that retry cannot fix. The `FileStoreError::Io(io::Error)` variant already exists via `#[from]`; only `ErrorKind::WouldBlock` should map to `Busy`, everything else should propagate as `Io`.

In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] packages/rs-platform-wallet-storage/src/secrets/store.rs:123-130: build_os bypasses the documented label allowlist on the OS-keyring arm
  The secrets module documents a reject-not-sanitize label contract (`^[A-Za-z0-9._-]{1,64}$`, enforced in `secrets/validate.rs`) intended to fire before any backend maps a label to a filename or keyring attribute (CWE-22/CWE-20). The File arm enforces it at every entry point (put_bytes/get_bytes/delete_bytes), but `build_os` — the sole entry path for `SecretStore::Os::{set,get,delete}` — hands the raw caller-supplied `label` directly to `store.build(&svc, label, None)`. Whatever validation happens after that is per-backend (macOS Keychain, Windows Credential Manager, libsecret, linux-keyutils all differ), so labels containing whitespace, control characters, path separators, or oversize strings that the File arm rejects may be accepted on some platforms. This breaks the cross-backend invariant that the same `(service, label)` resolves identically and weakens the OS-keyring trust boundary. Enforce `validated_label` in `build_os` so the policy is uniform across arms.

Review details

🤖 Prompt for all review comments with AI agents

These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

In `packages/rs-platform-wallet-storage/src/secrets/file/mod.rs`:
- [SUGGESTION] lines 259-267: with_vault_lock collapses every try_write io::Error into Busy
  `fd_lock::RwLock::try_write` returns a plain `io::Error` whose `ErrorKind` distinguishes lock contention (`WouldBlock`) from real I/O faults (`PermissionDenied`, `BadFileDescriptor`, EIO, etc.). The current loop matches `Err(_)` in both the retry and terminal arms, so any non-contention failure is busy-spun until `LOCK_WAIT_BUDGET` expires and then surfaced to callers as `FileStoreError::Busy`. This violates the variant's documented contract ("another process is mid-write, retry") and hides real fs failures (permissions/quota/tampered sidecar) behind a transient retry signal, prompting operators to retry on errors that retry cannot fix. The `FileStoreError::Io(io::Error)` variant already exists via `#[from]`; only `ErrorKind::WouldBlock` should map to `Busy`, everything else should propagate as `Io`.

In `packages/rs-platform-wallet-storage/src/secrets/store.rs`:
- [SUGGESTION] lines 123-130: build_os bypasses the documented label allowlist on the OS-keyring arm
  The secrets module documents a reject-not-sanitize label contract (`^[A-Za-z0-9._-]{1,64}$`, enforced in `secrets/validate.rs`) intended to fire before any backend maps a label to a filename or keyring attribute (CWE-22/CWE-20). The File arm enforces it at every entry point (put_bytes/get_bytes/delete_bytes), but `build_os` — the sole entry path for `SecretStore::Os::{set,get,delete}` — hands the raw caller-supplied `label` directly to `store.build(&svc, label, None)`. Whatever validation happens after that is per-backend (macOS Keychain, Windows Credential Manager, libsecret, linux-keyutils all differ), so labels containing whitespace, control characters, path separators, or oversize strings that the File arm rejects may be accepted on some platforms. This breaks the cross-backend invariant that the same `(service, label)` resolves identically and weakens the OS-keyring trust boundary. Enforce `validated_label` in `build_os` so the policy is uniform across arms.

Reviewed commit: 6ca1bc16

[thepastaclaw]

🟡 Suggestion: with_vault_lock collapses every try_write io::Error into Busy

fd_lock::RwLock::try_write returns a plain io::Error whose ErrorKind distinguishes lock contention (WouldBlock) from real I/O faults (PermissionDenied, BadFileDescriptor, EIO, etc.). The current loop matches Err(_) in both the retry and terminal arms, so any non-contention failure is busy-spun until LOCK_WAIT_BUDGET expires and then surfaced to callers as FileStoreError::Busy. This violates the variant's documented contract ("another process is mid-write, retry") and hides real fs failures (permissions/quota/tampered sidecar) behind a transient retry signal, prompting operators to retry on errors that retry cannot fix. The FileStoreError::Io(io::Error) variant already exists via #[from]; only ErrorKind::WouldBlock should map to Busy, everything else should propagate as Io.

Suggested change

	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(_) if Instant::now() < deadline => {
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(_) => return Err(FileStoreError::Busy),
	}
	};
	let _file_guard = loop {
	match rw.try_write() {
	Ok(guard) => break guard,
	Err(e) if e.kind() == std::io::ErrorKind::WouldBlock
	&& Instant::now() < deadline =>
	{
	std::thread::sleep(LOCK_POLL_INTERVAL);
	}
	Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => {
	return Err(FileStoreError::Busy)
	}
	Err(e) => return Err(FileStoreError::from(e)),
	}
	};

_{source: ['claude', 'codex']}

[thepastaclaw]

🟡 Suggestion: build_os bypasses the documented label allowlist on the OS-keyring arm

The secrets module documents a reject-not-sanitize label contract (^[A-Za-z0-9._-]{1,64}$, enforced in secrets/validate.rs) intended to fire before any backend maps a label to a filename or keyring attribute (CWE-22/CWE-20). The File arm enforces it at every entry point (put_bytes/get_bytes/delete_bytes), but build_os — the sole entry path for SecretStore::Os::{set,get,delete} — hands the raw caller-supplied label directly to store.build(&svc, label, None). Whatever validation happens after that is per-backend (macOS Keychain, Windows Credential Manager, libsecret, linux-keyutils all differ), so labels containing whitespace, control characters, path separators, or oversize strings that the File arm rejects may be accepted on some platforms. This breaks the cross-backend invariant that the same (service, label) resolves identically and weakens the OS-keyring trust boundary. Enforce validated_label in build_os so the policy is uniform across arms.

Suggested change

	fn build_os(
	store: &Arc<dyn CredentialStoreApi + Send + Sync>,
	service: &WalletId,
	label: &str,
	) -> Result<Entry, FileStoreError> {
	let svc = format!("{SERVICE_PREFIX}{}", service.to_hex());
	store.build(&svc, label, None).map_err(map_spi)
	}
	fn build_os(
	store: &Arc<dyn CredentialStoreApi + Send + Sync>,
	service: &WalletId,
	label: &str,
	) -> Result<Entry, FileStoreError> {
	let label = super::validate::validated_label(label).map_err(FileStoreError::from)?;
	let svc = format!("{SERVICE_PREFIX}{}", service.to_hex());
	store.build(&svc, label, None).map_err(map_spi)
	}

_{source: ['claude', 'codex']}