Merged
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughIntroduces comprehensive wallet backup export/import functionality across Android, iOS, and Rust. Adds password-protected backup flows with encryption (Argon2id + XChaCha20-Poly1305), file I/O, format validation, and wallet restoration. Integrates beta feature gating with authentication gates. Updates FFI bindings and keychain error handling for backup operations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Backup Export UI
participant Backend as Rust Backend
participant Crypto as Crypto Module
participant FileIO as File I/O
participant DB as Database/Keychain
User->>UI: Enter password & tap Export
UI->>UI: Validate password (32+ chars)
activate UI
UI->>Backend: export(password)
activate Backend
Backend->>Backend: Clean password
Backend->>DB: Gather wallets from all networks
Backend->>DB: Fetch wallet secrets (mnemonics/TapSignerBackup)
Backend->>DB: Gather settings (network, fiat, node config)
Backend->>Backend: Serialize to CBOR
Backend->>Crypto: compress(cbor_data)
activate Crypto
Crypto->>Crypto: Zstd compression
Crypto-->>Backend: compressed_data
deactivate Crypto
Backend->>Crypto: encrypt(compressed_data, password)
activate Crypto
Crypto->>Crypto: Generate salt + nonce
Crypto->>Crypto: Argon2id key derivation
Crypto->>Crypto: XChaCha20-Poly1305 encrypt
Crypto-->>Backend: encrypted_backup
deactivate Crypto
Backend-->>UI: BackupResult { data, filename, warnings }
deactivate Backend
UI->>FileIO: Write to user-selected URI
activate FileIO
FileIO-->>UI: Success
deactivate FileIO
UI->>FileIO: Share backup file
UI-->>User: Share sheet displayed
deactivate UI
sequenceDiagram
actor User
participant UI as Backup Import UI
participant FileIO as File I/O
participant Backend as Rust Backend
participant Crypto as Crypto Module
participant DB as Database/Keychain
User->>UI: Select backup file
UI->>FileIO: Read file data
activate FileIO
FileIO-->>UI: file_data
deactivate FileIO
UI->>Backend: validate_format(file_data)
activate Backend
Backend->>Crypto: validate_header(file_data)
Crypto-->>Backend: header_valid
Backend-->>UI: Format valid
deactivate Backend
User->>UI: Enter password & tap Import
UI->>UI: Validate password
UI->>Backend: import(file_data, password)
activate Backend
Backend->>Backend: Clean password
Backend->>Crypto: decrypt(file_data, password)
activate Crypto
Crypto->>Crypto: Parse header (salt, nonce)
Crypto->>Crypto: Argon2id key derivation
Crypto->>Crypto: XChaCha20-Poly1305 decrypt
Crypto-->>Backend: decrypted_data
deactivate Crypto
Backend->>Crypto: decompress(decrypted_data)
activate Crypto
Crypto->>Crypto: Zstd decompression
Crypto-->>Backend: cbor_data
deactivate Crypto
Backend->>Backend: Deserialize CBOR payload
Backend->>DB: Iterate wallets, check for duplicates
Backend->>DB: Restore mnemonic wallets → Keychain + DB
Backend->>DB: Restore descriptor wallets → Keychain + DB
Backend->>DB: Import labels per wallet
Backend->>DB: Restore settings (fiat, color scheme, nodes)
Backend-->>UI: BackupImportReport { imported, skipped, failed, warnings }
deactivate Backend
UI-->>User: Show import summary dialog
User->>UI: Confirm
UI->>UI: Emit AppAction::RefreshAfterImport
UI-->>User: Dismiss and refresh app state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift-480-484 (1)
480-484:⚠️ Potential issue | 🟡 MinorAvoid silent failure when persisting the beta feature flag.
Both enable/disable flows ignore thrown errors, so failed writes have no user feedback or diagnostics.
Suggested fix
- } catch {} + } catch { + Log.error("Failed to update beta feature flag: \(error.localizedDescription)") + }Also applies to: 496-500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift` around lines 480 - 484, The catch blocks around Database().globalFlag().set(key: .betaFeaturesEnabled, value: ...) silently swallow errors; update the error handling in the enable/disable flows (the try calls to Database().globalFlag().set and the surrounding code that sets isBetaEnabled and alertState) to catch the thrown Error, log it (e.g., via your app logger or print) and present user feedback by setting alertState to an error variant (or revert isBetaEnabled) so failures are visible; do the same fix for the corresponding block at the second occurrence (lines ~496-500) so both enable and disable flows handle errors consistently.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt-74-75 (1)
74-75:⚠️ Potential issue | 🟡 MinorClient-side password validation does not match backend whitespace rules.
Line [74] strips only
" ", while backend cleaning removes all whitespace. Tabs/newlines can pass UI checks and still fail export.💡 Proposed fix
- val strippedPassword = password.replace(" ", "") + val strippedPassword = password.filterNot { it.isWhitespace() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt` around lines 74 - 75, The UI currently strips only literal space characters when computing strippedPassword in BackupExportScreen (password.replace(" ", "")) so tabs/newlines remain and can make the client-side isPasswordValid check mismatch the backend; update the stripping logic used when computing strippedPassword to remove all whitespace characters (spaces, tabs, newlines) before measuring length and keep isPasswordValid unchanged so the client-side rule mirrors the backend cleaning.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt-251-255 (1)
251-255:⚠️ Potential issue | 🟡 MinorDo not silently ignore beta-flag persistence failures.
The empty catch at Line [254] can hide storage failures and leave UI state misleading.
💡 Proposed fix
- try { - Database().globalFlag().set(GlobalFlagKey.BETA_FEATURES_ENABLED, newValue) - isBetaEnabled = newValue - } catch (_: Exception) {} + try { + Database().globalFlag().set(GlobalFlagKey.BETA_FEATURES_ENABLED, newValue) + isBetaEnabled = newValue + } catch (e: Exception) { + Log.e("MainSettingsScreen", "Failed to persist beta feature flag", e) + return@TextButton + } showBetaDialog = false🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt` around lines 251 - 255, The empty catch around Database().globalFlag().set(GlobalFlagKey.BETA_FEATURES_ENABLED, newValue) silently hides persistence failures and can leave isBetaEnabled/showBetaDialog inconsistent; change the catch to capture the exception (e), log the error (e.g., Log.e or your app logger) with a clear message referencing GlobalFlagKey.BETA_FEATURES_ENABLED, and handle the failure by either reverting isBetaEnabled to the previous value and keeping showBetaDialog open or showing a user-facing error (Toast/Snackbar) so the UI reflects the real state; update the try/catch surrounding Database().globalFlag().set in MainSettingsScreen.kt accordingly.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt-187-189 (1)
187-189:⚠️ Potential issue | 🟡 MinorRemove unnecessary
Dispatchers.IOwrapper from suspend function call.Line 187:
importBackupis a suspend function that already handles async execution internally throughuniffiRustCallAsync. Wrapping it withwithContext(Dispatchers.IO)is redundant. Call it directly.Proposed fix
- val report = withContext(Dispatchers.IO) { - BackupManager().importBackup(data, password) - } + val report = BackupManager().importBackup(data, password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt` around lines 187 - 189, The call currently wraps the suspend method BackupManager().importBackup(data, password) inside withContext(Dispatchers.IO); remove that wrapper and invoke importBackup(data, password) directly since importBackup is already a suspend function and performs its own asynchronous work (via uniffiRustCallAsync). Update the code in BackupImportScreen to call BackupManager().importBackup(...) without using withContext(Dispatchers.IO).android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt-210-212 (1)
210-212:⚠️ Potential issue | 🟡 MinorRemove redundant
Dispatchers.IOwrapping from the suspend function call.
BackupManager().export()is already a suspend function that usesuniffiRustCallAsync()to manage async Rust operations internally. Wrapping it inwithContext(Dispatchers.IO)is unnecessary—call it directly from the coroutine scope.Proposed fix
- val result = withContext(Dispatchers.IO) { - BackupManager().export(password) - } + val result = BackupManager().export(password)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt` around lines 210 - 212, The call currently wraps the suspend export function in withContext(Dispatchers.IO); remove that redundant wrapper and call BackupManager().export(password) directly from the coroutine since export is already suspend and handles its own threading via uniffiRustCallAsync(); update the code to eliminate withContext(Dispatchers.IO) around BackupManager().export to avoid unnecessary context switches.rust/src/backup/crypto.rs-30-37 (1)
30-37:⚠️ Potential issue | 🟡 MinorEncryption key is not zeroized after use.
The derived key
[u8; 32]contains sensitive cryptographic material but is not zeroized when the function returns or when it goes out of scope. This leaves the key material in memory until the page is reused.🛡️ Proposed fix using zeroize
+use zeroize::Zeroize; + /// Derive a 32-byte encryption key from a password and salt using Argon2id -fn derive_key(password: &str, salt: &[u8; SALT_SIZE]) -> Result<[u8; 32], BackupError> { - let mut key = [0u8; 32]; +fn derive_key(password: &str, salt: &[u8; SALT_SIZE]) -> Result<zeroize::Zeroizing<[u8; 32]>, BackupError> { + let mut key = zeroize::Zeroizing::new([0u8; 32]); Argon2::default() - .hash_password_into(password.as_bytes(), salt, &mut key) + .hash_password_into(password.as_bytes(), salt, key.as_mut()) .map_err(|e| BackupError::Encryption(format!("key derivation failed: {e}")))?; Ok(key) }Then update callers to use
key.as_ref()or dereference as needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/crypto.rs` around lines 30 - 37, The derive_key function returns a plain [u8; 32] that leaves secret material in memory; change it to return a zeroizing wrapper (e.g., zeroize::Zeroizing<[u8; 32]>) so the buffer is automatically zeroed on drop, update the signature of derive_key to Result<Zeroizing<[u8; 32]>, BackupError>, perform Argon2::hash_password_into into the Zeroizing buffer, and update all callers of derive_key to use key.as_ref() or dereference the Zeroizing value where a &[u8] is needed; ensure you add the zeroize crate and import Zeroizing in the module.rust/src/backup/crypto.rs-33-35 (1)
33-35:⚠️ Potential issue | 🟡 MinorArgon2 defaults already meet OWASP minimum requirements; consider stronger parameters for wallet backup sensitivity.
Argon2::default()uses Argon2id with m=19 MiB, t=2, p=1, which meets OWASP's minimum security recommendations. However, for backup files protecting Bitcoin wallet mnemonics, stronger parameters (e.g., RFC 9106's m=64 MiB, t=3, p=4) could be considered to provide additional security margin beyond the minimum.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/crypto.rs` around lines 33 - 35, The code uses Argon2::default() in the key derivation call (hash_password_into), which only meets OWASP minima; replace it by explicitly constructing an Argon2 instance with stronger parameters per RFC 9106 (Argon2id, Version::V0x13, Params with memory=64 MiB, iterations=3, parallelism=4 and a hash length matching the key size) and then call hash_password_into(password.as_bytes(), salt, &mut key) on that instance; e.g., create Argon2::new(Algorithm::Argon2id, Version::V0x13, Params::new(65536, 3, 4, Some(key.len()))) and use it in place of Argon2::default().
🧹 Nitpick comments (4)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
25184-25196: Note:ByteArrayin data class has structural equality limitations.
BackupResultcontains aByteArrayfield. Kotlin data classes use reference equality for arrays in auto-generatedequals()/hashCode()/copy(). This means twoBackupResultinstances with identical byte content won't be considered equal.This is likely acceptable since these objects are primarily used for FFI transport rather than comparison. If equality checks are needed later, consider using
contentEquals()explicitly or wrapping in a custom type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt` around lines 25184 - 25196, The BackupResult data class uses a ByteArray field (`data`) which is compared by reference in the autogenerated equals()/hashCode()/copy(); override equals() and hashCode() in the BackupResult companion (or class) to use data.contentEquals(other.data) and data.contentHashCode(), and implement a custom copy() that copies the byte array (e.g., data.copyOf()) when producing a new instance; alternatively replace the ByteArray with an immutable structural-equality-friendly type (e.g., ByteBuffer or List<Byte>) if you prefer not to write custom equality/copy logic.rust/src/backup/model.rs (2)
17-32: Consider addingZeroizetoWalletBackupfor defense-in-depth.While
WalletSecretproperly derivesZeroizeandZeroizeOnDrop, the parent structWalletBackupdoes not. Although nestedWalletSecretfields will be zeroized on drop, ifWalletBackupinstances are ever cloned or moved in ways that leave copies in memory, those copies' secret fields could persist until the page is reused.For Bitcoin wallet security, consider deriving
ZeroizeonWalletBackupas well, ensuring all copies are explicitly zeroized when needed.🛡️ Proposed change for defense-in-depth
/// Per-wallet backup data -#[derive(Debug, Serialize, Deserialize)] +#[derive(Debug, Serialize, Deserialize, Zeroize, ZeroizeOnDrop)] pub struct WalletBackup { /// WalletMetadata serialized as JSON value for forward compatibility + #[zeroize(skip)] pub metadata: serde_json::Value, /// Secret key material, varies by wallet type pub secret: WalletSecret, /// External public descriptor string pub external_descriptor: Option<String>, /// Internal (change) public descriptor string pub internal_descriptor: Option<String>, /// Extended public key string pub xpub: Option<String>, /// BIP-329 labels as JSONL string pub labels_jsonl: Option<String>, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/model.rs` around lines 17 - 32, The WalletBackup struct should also derive Zeroize and ZeroizeOnDrop to defensively zero secret material when instances are dropped or explicitly zeroed; update the WalletBackup declaration to derive Zeroize and ZeroizeOnDrop (alongside existing Debug/Serialize/Deserialize) so the nested WalletSecret is covered and any copies are zeroized on drop, and ensure the zeroize crate is imported (and annotate any non-sensitive fields with #[zeroize(skip)] if you need to avoid zeroing them).
117-133: Consider using auseimport instead ofsuper::superpath navigation.The path
super::super::cryptoworks but is fragile and harder to read. A direct import would be clearer.♻️ Proposed refactor
#[cfg(test)] mod tests { use super::*; + use crate::backup::crypto; // ... in cbor_zstd_round_trip test: - let compressed = super::super::crypto::compress(&cbor).unwrap(); - let decompressed = super::super::crypto::decompress(&compressed).unwrap(); + let compressed = crypto::compress(&cbor).unwrap(); + let decompressed = crypto::decompress(&compressed).unwrap();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/model.rs` around lines 117 - 133, The test cbor_zstd_round_trip uses fragile super::super::crypto path navigation; replace that with a direct use import at the top of the module (e.g., use crate::crypto or use crate::path::to::crypto) and then call compress and decompress as crypto::compress/crypto::decompress (or call compress/decompress directly if you import the functions). Update references in the cbor_zstd_round_trip test to use the new imported symbol(s) (compress, decompress, or crypto) so the code is clearer and less brittle.rust/src/backup/crypto.rs (1)
131-139: Consider adding a maximum decompression size limit to prevent resource exhaustion.The
decompressfunction usesread_to_endwith no size limit. A malformed or malicious compressed payload could decompress to an arbitrarily large size, potentially causing out-of-memory conditions.Since backups contain wallet data and settings, a reasonable upper bound (e.g., 100 MiB) would protect against decompression bombs while accommodating legitimate use cases.
🛡️ Proposed fix with size limit
+const MAX_DECOMPRESSED_SIZE: usize = 100 * 1024 * 1024; // 100 MiB + /// Decompress zstd-compressed data pub fn decompress(data: &[u8]) -> Result<Vec<u8>, BackupError> { use std::io::Read as _; let mut decoder = ruzstd::decoding::StreamingDecoder::new(data) .map_err(|e| BackupError::Decompression(e.to_string()))?; - let mut output = Vec::new(); - decoder.read_to_end(&mut output).map_err(|e| BackupError::Decompression(e.to_string()))?; + let mut output = Vec::new(); + let mut limited = decoder.take(MAX_DECOMPRESSED_SIZE as u64 + 1); + limited.read_to_end(&mut output).map_err(|e| BackupError::Decompression(e.to_string()))?; + if output.len() > MAX_DECOMPRESSED_SIZE { + return Err(BackupError::Decompression("decompressed data exceeds size limit".into())); + } Ok(output) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/crypto.rs` around lines 131 - 139, The decompress function currently calls StreamingDecoder.read_to_end with no size bound; change it to enforce a maximum decompressed size (suggest 100 MiB) by reading in chunks from ruzstd::decoding::StreamingDecoder (e.g., a loop calling read into a fixed-size buffer) and accumulating into output while tracking total bytes, returning a BackupError::Decompression if the total exceeds the limit or on read error; keep using the same StreamingDecoder::new(...) error mapping and ensure the error message clearly indicates "decompressed size limit exceeded" when triggered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt`:
- Line 68: The password visibility flag is initialized to true, exposing the
backup password by default; change the initialization of isPasswordVisible (in
the BackupExportScreen composable where var isPasswordVisible by remember {
mutableStateOf(true) } appears, and the other occurrence around lines noted) to
false so the password is hidden by default, and verify any UI toggle or icon
still flips this boolean to reveal the password when the user explicitly
requests it.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt`:
- Line 73: The password visibility flag is initialized to true causing the
backup password to display in plaintext; change the default of isPasswordVisible
(and the other identical instance later in the file) from true to false so the
input is masked by default, and ensure any UI toggle logic that flips
isPasswordVisible remains unchanged so the eye icon still reveals the password
when explicitly tapped.
- Around line 84-95: The synchronous readBytes() in BackupImportScreen's URI
callback can block the UI and on failure leaves stale fileData/fileName; move
the file read and BackupManager().validateFormat(bytes) onto a background thread
(e.g. launch a coroutine with Dispatchers.IO) and use a safe InputStream.use {
... } pattern to close streams, then post results back on the main thread to set
fileData/fileName; in the catch block (or on validation failure) explicitly
clear/reset fileData and fileName (e.g. null and default label) before setting
errorMessage so a failed import cannot leave old data around.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt`:
- Around line 194-198: The current conditional treats any non-NONE Auth.type
(including AuthType.BIOMETRIC) as PIN-protected and sets showBackupExportAuth,
which blocks biometric-only users; change the branch logic in MainSettingsScreen
where Auth.type is checked so that only PIN-protected auth triggers
showBackupExportAuth (e.g., check for AuthType.PIN or explicitly exclude
AuthType.BIOMETRIC) and let AuthType.BIOMETRIC fall through to set
showBackupExport = true; also adjust the PIN pad routing code that references
Auth.type (the block around the PIN pad handling) to ensure it only activates
for PIN auth, not BIOMETRIC.
In `@ios/Cove/Flows/SettingsFlow/BackupExportView.swift`:
- Line 8: The password field in BackupExportView is currently visible by
default; change the `@State` property isPasswordVisible from true to false so the
backup password is masked by default, and update any other instances of this
state/initialization in the same file (the other occurrences around the password
field setup and toggle logic) to use the new default; ensure the UI toggle logic
in BackupExportView (the password TextField/SecureField switch and any
Button/Image that flips isPasswordVisible) still correctly shows/hides the
password when the user toggles it.
- Around line 13-19: Validation uses strippedPassword but the export path uses
the original password leading to inconsistency; change the export logic to use
the same normalized value and ensure isPasswordValid references the same
normalized variable. Specifically, in BackupExportView use strippedPassword (the
normalized password from the strippedPassword computed property) wherever the
plaintext password is validated or passed to encryption/export routines (instead
of raw password), and update any export/export button handler that currently
reads `password` to use `strippedPassword` so validation and encryption use the
identical input.
In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift`:
- Line 11: BackupImportView currently initializes `@State` private var
isPasswordVisible = true which shows the backup password in plaintext; change
the default to hide the password by setting isPasswordVisible to false and
ensure any other occurrences or toggles in BackupImportView (lines handling the
password TextField/SecureField logic) respect this default so the password
starts masked (also update related logic referenced around the password field
handling at the block covering lines ~44-50 to use SecureField when
isPasswordVisible is false).
- Around line 128-149: In handleFileSelection, avoid synchronous UI-blocking
file reads and prevent reuse of stale state by performing file
reading/validation off the main thread and only assigning fileData and fileName
on success, while clearing them on any failure; specifically, move
Data(contentsOf:) and BackupManager().validateFormat(data:) into a background
Task/DispatchQueue, keep url.startAccessingSecurityScopedResource()/defer
url.stopAccessingSecurityScopedResource(), set fileData = data and fileName =
url.lastPathComponent only after validation succeeds, and in all catch/failure
branches explicitly reset fileData = nil and fileName = "" (or appropriate empty
value) and update errorMessage on the main thread.
In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift`:
- Around line 232-236: The branch that sets sheetState to
.init(.backupExportAuth) when auth.type != .none can lock out biometric-only
users because .backupExportAuth only handles PIN; update the routing logic in
MainSettingsScreen so it checks auth.type (or the specific auth modes) and only
use .backupExportAuth for PIN-capable configs, otherwise route biometric-only
users to .init(.backupExport) or to a new biometric-capable sheet; specifically
modify the block that inspects auth.type (and the analogous block around lines
663-675) to detect biometric-only and choose .backupExport (or a biometric auth
sheet) instead of .backupExportAuth.
In `@ios/Cove/ShareSheet.swift`:
- Around line 43-45: The present(for:completion:) helper currently
unconditionally deletes the provided URL in its completion handler; change it to
only remove the file when it is actually a temporary export — e.g. check that
url.isFileURL and
url.path.hasPrefix(FileManager.default.temporaryDirectory.path) (or otherwise
detect your app's temp export location) before calling
FileManager.default.removeItem; apply the same guarded-delete change to the
other overloaded present(for:in:completion:) path so caller-provided
non-temporary files are never deleted.
In `@rust/src/backup/export.rs`:
- Around line 61-87: The export currently downgrades missing or errored private
key material to WalletSecret::None (in the match on metadata.wallet_type
handling WalletType::Hot and WalletType::Cold), producing backups that look
valid but cannot restore spendable wallets; change the logic in the export flow
to treat missing or failed retrievals from keychain.get_wallet_key and
keychain.get_tap_signer_backup as hard failures instead of continuing: return an
Err (or propagate the error) with a clear message including the wallet id when
get_wallet_key returns Ok(None) or Err(e), and when get_tap_signer_backup
returns None, instead of assigning WalletSecret::None, so the export function
fails fast and does not emit incomplete backups. Ensure the returned error type
matches the surrounding function's error handling.
- Around line 134-155: The current get closure (used with config.get and
ok().flatten) swallows DB errors; change it to propagate errors instead of
converting them to None so export fails on read errors. Replace the closure
around config.get(GlobalConfigKey) used in building AppSettings
(selected_network, selected_fiat_currency, color_scheme, auth_type and
SelectedNode loop) with one that returns a Result<Option<String>, _> or directly
matches config.get(key) and returns Err on Err(...) (bubbling the error from
config.get), then adjust the callers in the SelectedNode loop and when
constructing AppSettings to propagate that Result so the function returns an Err
when any DB read fails rather than silently ignoring it.
In `@rust/src/backup/import.rs`:
- Around line 46-49: The current duplicate check uses
collect_existing_fingerprints() only once and never updates it, so multiple
wallets in payload.wallets can share the same fingerprint and both be restored;
modify the import loop around restore_wallet to update the fingerprint set when
a wallet import succeeds (or alternatively maintain a local HashSet of seen
fingerprints initialized from collect_existing_fingerprints()) and check/insert
into that set before calling restore_wallet to prevent importing duplicate
fingerprints within the same payload; ensure the same fix is applied to the
other loop handling imports (the block around the second restore loop referenced
in the review).
- Around line 68-70: The restore_settings function currently swallows config.set
errors (uses let _ = ...) and always returns Ok, causing
report.settings_restored to be true even when writes fail; change
restore_settings to propagate errors from each config.set (use ? or map_err to
return Err) so callers can correctly set report.settings_restored based on the
Result; additionally ensure the exported "auth_type" key is actually applied
during import by invoking the appropriate config/set or authentication API for
"auth_type" (instead of ignoring it), and apply the same error-propagation and
auth_type-restoration fixes to the other import/restore code path that mirrors
this logic (the other restore_settings usage).
---
Minor comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt`:
- Around line 74-75: The UI currently strips only literal space characters when
computing strippedPassword in BackupExportScreen (password.replace(" ", "")) so
tabs/newlines remain and can make the client-side isPasswordValid check mismatch
the backend; update the stripping logic used when computing strippedPassword to
remove all whitespace characters (spaces, tabs, newlines) before measuring
length and keep isPasswordValid unchanged so the client-side rule mirrors the
backend cleaning.
- Around line 210-212: The call currently wraps the suspend export function in
withContext(Dispatchers.IO); remove that redundant wrapper and call
BackupManager().export(password) directly from the coroutine since export is
already suspend and handles its own threading via uniffiRustCallAsync(); update
the code to eliminate withContext(Dispatchers.IO) around BackupManager().export
to avoid unnecessary context switches.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt`:
- Around line 187-189: The call currently wraps the suspend method
BackupManager().importBackup(data, password) inside withContext(Dispatchers.IO);
remove that wrapper and invoke importBackup(data, password) directly since
importBackup is already a suspend function and performs its own asynchronous
work (via uniffiRustCallAsync). Update the code in BackupImportScreen to call
BackupManager().importBackup(...) without using withContext(Dispatchers.IO).
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt`:
- Around line 251-255: The empty catch around
Database().globalFlag().set(GlobalFlagKey.BETA_FEATURES_ENABLED, newValue)
silently hides persistence failures and can leave isBetaEnabled/showBetaDialog
inconsistent; change the catch to capture the exception (e), log the error
(e.g., Log.e or your app logger) with a clear message referencing
GlobalFlagKey.BETA_FEATURES_ENABLED, and handle the failure by either reverting
isBetaEnabled to the previous value and keeping showBetaDialog open or showing a
user-facing error (Toast/Snackbar) so the UI reflects the real state; update the
try/catch surrounding Database().globalFlag().set in MainSettingsScreen.kt
accordingly.
In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift`:
- Around line 480-484: The catch blocks around Database().globalFlag().set(key:
.betaFeaturesEnabled, value: ...) silently swallow errors; update the error
handling in the enable/disable flows (the try calls to
Database().globalFlag().set and the surrounding code that sets isBetaEnabled and
alertState) to catch the thrown Error, log it (e.g., via your app logger or
print) and present user feedback by setting alertState to an error variant (or
revert isBetaEnabled) so failures are visible; do the same fix for the
corresponding block at the second occurrence (lines ~496-500) so both enable and
disable flows handle errors consistently.
In `@rust/src/backup/crypto.rs`:
- Around line 30-37: The derive_key function returns a plain [u8; 32] that
leaves secret material in memory; change it to return a zeroizing wrapper (e.g.,
zeroize::Zeroizing<[u8; 32]>) so the buffer is automatically zeroed on drop,
update the signature of derive_key to Result<Zeroizing<[u8; 32]>, BackupError>,
perform Argon2::hash_password_into into the Zeroizing buffer, and update all
callers of derive_key to use key.as_ref() or dereference the Zeroizing value
where a &[u8] is needed; ensure you add the zeroize crate and import Zeroizing
in the module.
- Around line 33-35: The code uses Argon2::default() in the key derivation call
(hash_password_into), which only meets OWASP minima; replace it by explicitly
constructing an Argon2 instance with stronger parameters per RFC 9106 (Argon2id,
Version::V0x13, Params with memory=64 MiB, iterations=3, parallelism=4 and a
hash length matching the key size) and then call
hash_password_into(password.as_bytes(), salt, &mut key) on that instance; e.g.,
create Argon2::new(Algorithm::Argon2id, Version::V0x13, Params::new(65536, 3, 4,
Some(key.len()))) and use it in place of Argon2::default().
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 25184-25196: The BackupResult data class uses a ByteArray field
(`data`) which is compared by reference in the autogenerated
equals()/hashCode()/copy(); override equals() and hashCode() in the BackupResult
companion (or class) to use data.contentEquals(other.data) and
data.contentHashCode(), and implement a custom copy() that copies the byte array
(e.g., data.copyOf()) when producing a new instance; alternatively replace the
ByteArray with an immutable structural-equality-friendly type (e.g., ByteBuffer
or List<Byte>) if you prefer not to write custom equality/copy logic.
In `@rust/src/backup/crypto.rs`:
- Around line 131-139: The decompress function currently calls
StreamingDecoder.read_to_end with no size bound; change it to enforce a maximum
decompressed size (suggest 100 MiB) by reading in chunks from
ruzstd::decoding::StreamingDecoder (e.g., a loop calling read into a fixed-size
buffer) and accumulating into output while tracking total bytes, returning a
BackupError::Decompression if the total exceeds the limit or on read error; keep
using the same StreamingDecoder::new(...) error mapping and ensure the error
message clearly indicates "decompressed size limit exceeded" when triggered.
In `@rust/src/backup/model.rs`:
- Around line 17-32: The WalletBackup struct should also derive Zeroize and
ZeroizeOnDrop to defensively zero secret material when instances are dropped or
explicitly zeroed; update the WalletBackup declaration to derive Zeroize and
ZeroizeOnDrop (alongside existing Debug/Serialize/Deserialize) so the nested
WalletSecret is covered and any copies are zeroized on drop, and ensure the
zeroize crate is imported (and annotate any non-sensitive fields with
#[zeroize(skip)] if you need to avoid zeroing them).
- Around line 117-133: The test cbor_zstd_round_trip uses fragile
super::super::crypto path navigation; replace that with a direct use import at
the top of the module (e.g., use crate::crypto or use crate::path::to::crypto)
and then call compress and decompress as crypto::compress/crypto::decompress (or
call compress/decompress directly if you import the functions). Update
references in the cbor_zstd_round_trip test to use the new imported symbol(s)
(compress, decompress, or crypto) so the code is clearer and less brittle.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (19)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/BackupImportView.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftios/Cove/ShareSheet.swiftrust/Cargo.tomlrust/src/app.rsrust/src/backup.rsrust/src/backup/crypto.rsrust/src/backup/error.rsrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/lib.rs
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Outdated
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
ios/Cove/Flows/SettingsFlow/BackupExportView.swift (1)
13-19:⚠️ Potential issue | 🟠 MajorUse full-whitespace normalization and pass the normalized value to export.
Current validation strips only
" ", and export still uses rawpassword. This can desync validation vs actual backend input for tab/newline-containing strings.💡 Proposed fix
private var strippedPassword: String { - password.replacingOccurrences(of: " ", with: "") + password.filter { !$0.isWhitespace } } @@ - let result = try await BackupManager().export(password: password) + let result = try await BackupManager().export(password: strippedPassword)Also applies to: 105-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/BackupExportView.swift` around lines 13 - 19, The validation currently only removes literal spaces in strippedPassword and then validates using isPasswordValid while the export still uses the raw password; update strippedPassword to perform full whitespace normalization (remove all whitespace/newlines/tabs, e.g. using CharacterSet.whitespacesAndNewlines or a regex) and use that normalized value everywhere validation and export occur (replace usages of raw password in the export action with strippedPassword) so validation and actual backend input are consistent; ensure isPasswordValid references the normalized strippedPassword and the export method receives the normalized password.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt (1)
84-100:⚠️ Potential issue | 🟠 MajorReset file state on selection failure and close the stream safely.
On failure, previous
fileData/fileNamecan persist and be imported accidentally; alsoopenInputStream(...).readBytes()should useuse {}to close resources.💡 Proposed fix
) { uri -> uri?.let { scope.launch { + fileData = null + fileName = null try { val (bytes, name) = withContext(Dispatchers.IO) { - val bytes = context.contentResolver.openInputStream(uri)?.readBytes() + val bytes = context.contentResolver.openInputStream(uri)?.use { input -> + input.readBytes() + } ?: throw java.io.IOException("Failed to read file") BackupManager().validateFormat(bytes) bytes to (uri.lastPathSegment?.substringAfterLast('/') ?: "backup file") } fileData = bytes fileName = name } catch (e: Exception) { + fileData = null + fileName = null errorMessage = e.toString() } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt` around lines 84 - 100, When reading the selected URI in BackupImportScreen (inside the scope.launch/withContext block), ensure the InputStream is closed by using use { } around context.contentResolver.openInputStream(uri) and call readBytes() from inside that block, validateFormat(bytes) on the resulting bytes, and on any exception reset fileData and fileName to null/empty before setting errorMessage so stale values cannot be imported; update the code paths that set fileData and fileName after withContext to only run on success (and reference BackupManager().validateFormat, fileData, fileName, and uri in your changes).rust/src/backup/import.rs (1)
227-237:⚠️ Potential issue | 🟠 Major
auth_typeis still not restored during settings import.
AppSettings.auth_typeis exported but never applied inrestore_settings, so imported security settings can remain inconsistent.💡 Proposed fix
if let Some(scheme) = &settings.color_scheme { set(GlobalConfigKey::ColorScheme, scheme.clone())?; } + if let Some(auth_type) = &settings.auth_type { + set(GlobalConfigKey::AuthType, auth_type.clone())?; + } + for (network_str, node_json) in &settings.selected_nodes { if let Ok(network) = Network::try_from(network_str.as_str()) { set(GlobalConfigKey::SelectedNode(network), node_json.clone())?; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` around lines 227 - 237, The restore_settings path is missing applying AppSettings.auth_type; in restore_settings, detect settings.auth_type (e.g., if let Some(auth) = &settings.auth_type) and call set(GlobalConfigKey::AuthType, auth.clone())? (or the appropriate GlobalConfigKey variant) so the exported auth_type is persisted; update the restore_settings function to mirror the pattern used for selected_network/selected_fiat_currency/color_scheme by applying the value via set(...) and handling the Result with ?.ios/Cove/Flows/SettingsFlow/BackupImportView.swift (1)
128-149:⚠️ Potential issue | 🟠 MajorAvoid synchronous file loading here and clear stale selection on all failures.
The current path can block UI on large files and keep previous
fileData/fileNameafter a failed new selection.💡 Proposed fix
private func handleFileSelection(_ result: Result<URL, Error>) { switch result { case let .success(url): - guard url.startAccessingSecurityScopedResource() else { - errorMessage = "Unable to access the selected file" - return - } - defer { url.stopAccessingSecurityScopedResource() } - - do { - let data = try Data(contentsOf: url) - try BackupManager().validateFormat(data: data) - - fileData = data - fileName = url.lastPathComponent - } catch { - errorMessage = (error as? BackupError)?.description ?? error.localizedDescription - } + fileData = nil + fileName = nil + Task { + guard url.startAccessingSecurityScopedResource() else { + await MainActor.run { errorMessage = "Unable to access the selected file" } + return + } + defer { url.stopAccessingSecurityScopedResource() } + + do { + let data = try await Task.detached(priority: .userInitiated) { + try Data(contentsOf: url) + }.value + try BackupManager().validateFormat(data: data) + await MainActor.run { + fileData = data + fileName = url.lastPathComponent + } + } catch { + await MainActor.run { + fileData = nil + fileName = nil + errorMessage = (error as? BackupError)?.description ?? error.localizedDescription + } + } + } case let .failure(error): + fileData = nil + fileName = nil errorMessage = error.localizedDescription } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift` around lines 128 - 149, handleFileSelection currently reads the file synchronously (Data(contentsOf:)) which can block the UI and may leave stale fileData/fileName on failure; change it to perform file read and BackupManager().validateFormat(data:) off the main thread (e.g., Task or DispatchQueue.global) and update fileData/fileName on the main thread when successful, and ensure on any failure (including failing startAccessingSecurityScopedResource or validation errors) you clear/reset fileData and fileName and set errorMessage appropriately; keep the security-scoped access pattern (startAccessingSecurityScopedResource/defer stop) but move heavy IO/validation out of the main thread inside handleFileSelection to avoid blocking the UI.rust/src/backup/export.rs (2)
134-154:⚠️ Potential issue | 🟠 MajorSettings export still swallows configuration read failures.
Converting
config.geterrors toNonehides data-loss in exported settings.💡 Proposed fix
- let get = |key: GlobalConfigKey| -> Option<String> { - match config.get(key) { - Ok(value) => value, - Err(e) => { - warn!("failed to read config key: {e}"); - None - } - } - }; + let get = |key: GlobalConfigKey| -> Result<Option<String>, BackupError> { + config + .get(key) + .map_err(|e| BackupError::Database(format!("failed to read setting: {e}"))) + }; - let selected_network = get(GlobalConfigKey::SelectedNetwork); - let selected_fiat_currency = get(GlobalConfigKey::SelectedFiatCurrency); - let color_scheme = get(GlobalConfigKey::ColorScheme); - let auth_type = get(GlobalConfigKey::AuthType); + let selected_network = get(GlobalConfigKey::SelectedNetwork)?; + let selected_fiat_currency = get(GlobalConfigKey::SelectedFiatCurrency)?; + let color_scheme = get(GlobalConfigKey::ColorScheme)?; + let auth_type = get(GlobalConfigKey::AuthType)?; let mut selected_nodes = Vec::new(); for network in Network::iter() { - if let Some(node_json) = get(GlobalConfigKey::SelectedNode(network)) { + if let Some(node_json) = get(GlobalConfigKey::SelectedNode(network))? { selected_nodes.push((network.to_string(), node_json)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/export.rs` around lines 134 - 154, The current closure get converts all config.get errors to None, silently hiding read failures; change it to propagate errors instead of swallowing them: replace the closure get used around config.get(...) so it returns a Result<Option<String>, E> or Result<String, E> (e.g., Result<String, anyhow::Error>) and handle the Err cases where export code calls get (for SelectedNetwork, SelectedFiatCurrency, ColorScheme, AuthType and the SelectedNode loop) by either failing the export with a clear error or recording the error detail in the export metadata/log; ensure any use of get(GlobalConfigKey::SelectedNode(network)) and get(GlobalConfigKey::SelectedNetwork) checks the Result and does not convert Err to None silently so configuration read failures are surfaced.
75-83:⚠️ Potential issue | 🟠 MajorMissing TapSigner backup for cold wallets is still silently downgraded.
Returning
WalletSecret::Nonehere can produce a "successful" export that cannot fully restore a cold wallet.💡 Proposed fix
crate::wallet::metadata::WalletType::Cold => { match keychain.get_tap_signer_backup(id) { Some(backup) => WalletSecret::TapSignerBackup(backup), None => { - warn!( - "cold wallet {id} has no tap signer backup, backing up as None" - ); - WalletSecret::None + return Err(BackupError::Gather(format!( + "cold wallet {id} has no tap signer backup" + ))); } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/export.rs` around lines 75 - 83, The code silently downgrades cold wallets by returning WalletSecret::None when keychain.get_tap_signer_backup(id) is missing; instead, change the export logic for crate::wallet::metadata::WalletType::Cold (and the surrounding export function) to treat a missing TapSigner backup as an error: detect when keychain.get_tap_signer_backup(id) returns None and return a failure (or a Result::Err) with a clear message rather than producing WalletSecret::None, or alternatively propagate a RecoverableBackupMissing error; ensure you update callers to handle the Result and keep successful exports only when WalletSecret::TapSignerBackup(backup) is present.
🧹 Nitpick comments (2)
rust/src/backup/crypto.rs (1)
55-57: Zeroize derived keys after constructing the cipher.Derived keys remain in memory after use. Zeroizing reduces exposure of sensitive material.
💡 Proposed fix
+use zeroize::Zeroize as _; pub fn encrypt(plaintext: &[u8], password: &str) -> Result<Vec<u8>, BackupError> { @@ - let key = derive_key(password, &salt)?; - let cipher = XChaCha20Poly1305::new((&key).into()); + let mut key = derive_key(password, &salt)?; + let cipher = XChaCha20Poly1305::new((&key).into()); + key.zeroize(); @@ pub fn decrypt(data: &[u8], password: &str) -> Result<Vec<u8>, BackupError> { @@ - let key = derive_key(password, &salt)?; - let cipher = XChaCha20Poly1305::new((&key).into()); + let mut key = derive_key(password, &salt)?; + let cipher = XChaCha20Poly1305::new((&key).into()); + key.zeroize();Also applies to: 103-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/crypto.rs` around lines 55 - 57, Derived encryption key remains in memory after creating the cipher; after calling derive_key and passing key to XChaCha20Poly1305::new, zeroize the key material immediately (e.g., drop/overwrite securely or wrap in a Zeroizing<T>) so the derived bytes are cleared; update both occurrences where derive_key, key, and XChaCha20Poly1305::new are used (the block at the first snippet and the analogous block around lines 103-105) to zeroize the key after constructing the cipher and before returning/using nonce.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt (1)
1121-1121: KeyLaunchedEffectto actual dependencies instead ofUnitin backup auth flows.Both effects read values from their enclosing scope that can change:
- Line 1121: Should be keyed to
activityandauthTypeto re-run if activity becomes available after being null- Line 1152: Should be keyed to
activity,authType, andshowPinFallbackto re-run ifshowPinFallbackchanges back to falseUsing
Unitkeys causes these effects to run only once during composition and miss subsequent state changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt` at line 1121, The two LaunchedEffect blocks currently keyed to Unit must instead use the actual reactive dependencies so they re-run when relevant state changes: replace the first LaunchedEffect(Unit) with LaunchedEffect(activity, authType) so it re-executes if activity becomes available or authType changes, and replace the second LaunchedEffect(Unit) with LaunchedEffect(activity, authType, showPinFallback) so it re-executes when activity, authType, or showPinFallback toggles; locate the LaunchedEffect usages in MainSettingsScreen.kt and update their keys accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt`:
- Around line 78-79: The current password normalization in BackupImportScreen.kt
uses password.replace(" ", "") which only removes ASCII spaces; change the logic
that produces strippedPassword so it removes all Unicode whitespace (spaces,
tabs, newlines, etc.) before checking length (isPasswordValid). Implement this
by replacing the single-space replace call with a normalization that strips all
whitespace characters (for example using a regex that matches any whitespace or
filtering characters via Char.isWhitespace) so strippedPassword accurately
reflects the backend-cleaned password.
In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift`:
- Around line 17-23: Normalize the password the same way for validation and for
the import call: replace the current strippedPassword (which only replaces " ")
with a normalizedPassword that removes all whitespace characters (e.g., using
.components(separatedBy: .whitespacesAndNewlines).joined()), use
normalizedPassword in isPasswordValid (instead of strippedPassword) and pass
normalizedPassword to the import function so validation and import use identical
whitespace normalization; update references to strippedPassword and any places
using raw password for import to use normalizedPassword (e.g., in
BackupImportView's isPasswordValid and the import action).
In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift`:
- Around line 480-484: The empty catch blocks around
Database().globalFlag().set(key: .betaFeaturesEnabled, value: ...) in
MainSettingsScreen.swift are swallowing persistence errors; update the do-catch
so the catch captures the thrown error, logs it (using your app logger or
os_log), and surfaces user feedback by setting alertState to an error case (or
showing a failure alert) instead of silently continuing; also only set
isBetaEnabled = true after a successful write; make the same changes for the
other symmetric block that disables the flag (the second
Database().globalFlag().set call).
In `@rust/src/backup/crypto.rs`:
- Around line 143-149: The decompress function currently calls
decoder.read_to_end which allows untrusted input to expand arbitrarily and OOM;
modify decompress to enforce a maximum decompressed size (e.g., define
MAX_DECOMPRESSED_SIZE) and read from ruzstd::decoding::StreamingDecoder via a
bounded reader (use std::io::Read::take or manually read in chunks while summing
bytes) so you stop and return BackupError::Decompression if the limit is
exceeded; keep the existing error mapping (map_err(...)) and adjust variable
names in decompress to check the accumulated output.len() against
MAX_DECOMPRESSED_SIZE and fail deterministically instead of unboundedly growing
the Vec.
---
Duplicate comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt`:
- Around line 84-100: When reading the selected URI in BackupImportScreen
(inside the scope.launch/withContext block), ensure the InputStream is closed by
using use { } around context.contentResolver.openInputStream(uri) and call
readBytes() from inside that block, validateFormat(bytes) on the resulting
bytes, and on any exception reset fileData and fileName to null/empty before
setting errorMessage so stale values cannot be imported; update the code paths
that set fileData and fileName after withContext to only run on success (and
reference BackupManager().validateFormat, fileData, fileName, and uri in your
changes).
In `@ios/Cove/Flows/SettingsFlow/BackupExportView.swift`:
- Around line 13-19: The validation currently only removes literal spaces in
strippedPassword and then validates using isPasswordValid while the export still
uses the raw password; update strippedPassword to perform full whitespace
normalization (remove all whitespace/newlines/tabs, e.g. using
CharacterSet.whitespacesAndNewlines or a regex) and use that normalized value
everywhere validation and export occur (replace usages of raw password in the
export action with strippedPassword) so validation and actual backend input are
consistent; ensure isPasswordValid references the normalized strippedPassword
and the export method receives the normalized password.
In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift`:
- Around line 128-149: handleFileSelection currently reads the file
synchronously (Data(contentsOf:)) which can block the UI and may leave stale
fileData/fileName on failure; change it to perform file read and
BackupManager().validateFormat(data:) off the main thread (e.g., Task or
DispatchQueue.global) and update fileData/fileName on the main thread when
successful, and ensure on any failure (including failing
startAccessingSecurityScopedResource or validation errors) you clear/reset
fileData and fileName and set errorMessage appropriately; keep the
security-scoped access pattern (startAccessingSecurityScopedResource/defer stop)
but move heavy IO/validation out of the main thread inside handleFileSelection
to avoid blocking the UI.
In `@rust/src/backup/export.rs`:
- Around line 134-154: The current closure get converts all config.get errors to
None, silently hiding read failures; change it to propagate errors instead of
swallowing them: replace the closure get used around config.get(...) so it
returns a Result<Option<String>, E> or Result<String, E> (e.g., Result<String,
anyhow::Error>) and handle the Err cases where export code calls get (for
SelectedNetwork, SelectedFiatCurrency, ColorScheme, AuthType and the
SelectedNode loop) by either failing the export with a clear error or recording
the error detail in the export metadata/log; ensure any use of
get(GlobalConfigKey::SelectedNode(network)) and
get(GlobalConfigKey::SelectedNetwork) checks the Result and does not convert Err
to None silently so configuration read failures are surfaced.
- Around line 75-83: The code silently downgrades cold wallets by returning
WalletSecret::None when keychain.get_tap_signer_backup(id) is missing; instead,
change the export logic for crate::wallet::metadata::WalletType::Cold (and the
surrounding export function) to treat a missing TapSigner backup as an error:
detect when keychain.get_tap_signer_backup(id) returns None and return a failure
(or a Result::Err) with a clear message rather than producing
WalletSecret::None, or alternatively propagate a RecoverableBackupMissing error;
ensure you update callers to handle the Result and keep successful exports only
when WalletSecret::TapSignerBackup(backup) is present.
In `@rust/src/backup/import.rs`:
- Around line 227-237: The restore_settings path is missing applying
AppSettings.auth_type; in restore_settings, detect settings.auth_type (e.g., if
let Some(auth) = &settings.auth_type) and call set(GlobalConfigKey::AuthType,
auth.clone())? (or the appropriate GlobalConfigKey variant) so the exported
auth_type is persisted; update the restore_settings function to mirror the
pattern used for selected_network/selected_fiat_currency/color_scheme by
applying the value via set(...) and handling the Result with ?.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt`:
- Line 1121: The two LaunchedEffect blocks currently keyed to Unit must instead
use the actual reactive dependencies so they re-run when relevant state changes:
replace the first LaunchedEffect(Unit) with LaunchedEffect(activity, authType)
so it re-executes if activity becomes available or authType changes, and replace
the second LaunchedEffect(Unit) with LaunchedEffect(activity, authType,
showPinFallback) so it re-executes when activity, authType, or showPinFallback
toggles; locate the LaunchedEffect usages in MainSettingsScreen.kt and update
their keys accordingly.
In `@rust/src/backup/crypto.rs`:
- Around line 55-57: Derived encryption key remains in memory after creating the
cipher; after calling derive_key and passing key to XChaCha20Poly1305::new,
zeroize the key material immediately (e.g., drop/overwrite securely or wrap in a
Zeroizing<T>) so the derived bytes are cleared; update both occurrences where
derive_key, key, and XChaCha20Poly1305::new are used (the block at the first
snippet and the analogous block around lines 103-105) to zeroize the key after
constructing the cipher and before returning/using nonce.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/BackupImportView.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftrust/src/backup/crypto.rsrust/src/backup/export.rsrust/src/backup/import.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Outdated
Show resolved
Hide resolved
praveenperera
force-pushed
the
beta-full-import-export
branch
7 times, most recently
from
1db75ca to
9f19bfe
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rust/crates/cove-util/src/encryption.rs (1)
71-87:⚠️ Potential issue | 🟠 MajorPersisted
Cryptorcan still be reused for encryption after reload
try_from_stringinitializesused: false, so a serialized key+nonce can be reloaded and used to encrypt again. That re-opens nonce reuse across persistence boundaries, which defeats the one-time nonce guarantee in practice.🔐 Suggested hardening
pub fn try_from_string(string: &str) -> Result<Self, Error> { @@ - Ok(Self { key: key_bytes.into(), nonce: nonce_bytes.into(), used: false }) + // Deserialized cryptors should be treated as decrypt-only to prevent + // accidental nonce reuse after persistence round-trips. + Ok(Self { key: key_bytes.into(), nonce: nonce_bytes.into(), used: true }) }As per coding guidelines, "rust/**/*.rs: Check for potential security issues, make sure Bitcoin wallets are handled securely."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-util/src/encryption.rs` around lines 71 - 87, The deserialization in try_from_string currently sets used: false which allows a reloaded Cryptor to encrypt again and reuse the nonce; change try_from_string to initialize used: true (or otherwise mark the instance as decryption-only) so a persisted key+nonce cannot be used for new encryptions after reload, and add a brief comment in try_from_string/Cryptor noting that reloaded instances are intentionally disabled for encryption to prevent nonce reuse.rust/crates/cove-device/src/keychain.rs (1)
97-109:⚠️ Potential issue | 🔴 CriticalPrevent key/data mismatch on partial overwrite failures.
These save paths always generate and persist a new encryption key before persisting ciphertext. On overwrite, if the second save fails, existing ciphertext can become undecryptable (key rotated, data not updated), which is catastrophic for mnemonic/backup recovery.
Suggested fix (reuse existing encryption key on updates; only create/save a new one when absent)
@@ - // save encryption key first — orphaned key is harmless, but orphaned data without its key would be unrecoverable - let encryption_key_key = wallet_mnemonic_encryption_and_nonce_key_name(id); - let mut cryptor = Cryptor::new(); + let encryption_key_key = wallet_mnemonic_encryption_and_nonce_key_name(id); + let existing_encryption_key = self.0.get(encryption_key_key.clone()); + let mut cryptor = if let Some(serialized) = &existing_encryption_key { + Cryptor::try_from_string(serialized).map_err(|e| { + KeychainError::Decrypt(format!("wallet mnemonic encryption key: {e}")) + })? + } else { + Cryptor::new() + }; @@ - let encryption_key = cryptor.serialize_to_string(); - - self.0.save(encryption_key_key, encryption_key)?; + if existing_encryption_key.is_none() { + self.0 + .save(encryption_key_key, cryptor.serialize_to_string())?; + } self.0.save(key, encrypted_secret_key)?; @@ - let encryption_key_key = wallet_tap_signer_encryption_key_and_nonce_key_name(id); - let mut cryptor = Cryptor::new(); + let encryption_key_key = wallet_tap_signer_encryption_key_and_nonce_key_name(id); + let existing_encryption_key = self.0.get(encryption_key_key.clone()); + let mut cryptor = if let Some(serialized) = &existing_encryption_key { + Cryptor::try_from_string(serialized).map_err(|e| { + KeychainError::Decrypt(format!("tap signer encryption key: {e}")) + })? + } else { + Cryptor::new() + }; @@ - let encryption_key = cryptor.serialize_to_string(); - - // save encryption key first — orphaned key is harmless, but orphaned data without its key would be unrecoverable - self.0.save(encryption_key_key, encryption_key)?; + if existing_encryption_key.is_none() { + self.0 + .save(encryption_key_key, cryptor.serialize_to_string())?; + } self.0.save(backup_key, encrypted_backup)?;As per coding guidelines, "- Check for potential security issues, make sure Bitcoin wallets are handled securely." and "- Verify correct functionality and logic consistency."
Also applies to: 287-297
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-device/src/keychain.rs` around lines 97 - 109, Currently the code always generates a new Cryptor (Cryptor::new()) and saves its serialized encryption key (wallet_mnemonic_encryption_and_nonce_key_name) before saving the ciphertext (wallet_mnemonic_key_name), which can rotate the key and leave ciphertext undecryptable if the second save fails; change the flow to first attempt to load an existing encryption key via self.0.load(...) for wallet_mnemonic_encryption_and_nonce_key_name and, if present, initialize the Cryptor from that serialized key and reuse it to encrypt secret_key, only creating and saving a new encryption key if none exists; ensure you call self.0.save(encryption_key_key, ...) only when creating a new key and keep the order so ciphertext is saved after a successful encrypt using the current encryption key.
♻️ Duplicate comments (1)
ios/Cove/ShareSheet.swift (1)
76-83:⚠️ Potential issue | 🟠 MajorAvoid deleting caller-provided URLs in generic share path.
Line 78 currently removes any passed URL, which can delete non-temporary files if this API is reused outside temp-export flows.
Proposed fix
static func present(for url: URL) { - present(for: url) { _ in } + present(for: url, deleteAfterShare: false) { _ in } } `@MainActor` -static func present(for url: URL, completion: `@escaping` (Bool) -> Void) { +static func present( + for url: URL, + deleteAfterShare: Bool = false, + completion: `@escaping` (Bool) -> Void +) { @@ - activityViewController.completionWithItemsHandler = { _, completed, _, error in - do { - try FileManager.default.removeItem(at: url) - } catch let removeError as NSError where removeError.domain == NSCocoaErrorDomain && removeError.code == NSFileNoSuchFileError { - // already cleaned up - } catch { - Log.error("Failed to remove temporary backup file: \(error)") - } + activityViewController.completionWithItemsHandler = { _, completed, _, error in + if deleteAfterShare, + url.isFileURL, + url.path.hasPrefix(FileManager.default.temporaryDirectory.path) { + do { + try FileManager.default.removeItem(at: url) + } catch let removeError as NSError where removeError.domain == NSCocoaErrorDomain && removeError.code == NSFileNoSuchFileError { + // already cleaned up + } catch { + Log.error("Failed to remove temporary backup file: \(error)") + } + } if let error { Log.error("Share sheet error: \(error.localizedDescription)") completion(false) } else { completion(completed) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/ShareSheet.swift` around lines 76 - 83, The completion handler currently unconditionally calls FileManager.default.removeItem(at: url) which can delete caller-provided files; update activityViewController.completionWithItemsHandler so it only removes the file if we created it (e.g., the URL points into NSTemporaryDirectory() or matches a known temp prefix) or when an explicit "isTemporary" flag/marker is set, otherwise do not call FileManager.removeItem(at: url); keep the removal inside the existing do/catch and retain the existing error handling for removal failures.
🧹 Nitpick comments (2)
rust/src/app.rs (1)
357-374: Consider returningResultfor API consistency.
get_tap_signer_backupnow returnsResult<Option<Vec<u8>>, KeychainError>, butsave_tap_signer_backupreturnsbool. For API consistency and to allow callers to distinguish between "wallet not found" and "keychain save failed", consider returningResult<bool, KeychainError>orResult<(), SaveBackupError>.That said, the current boolean approach with logging is acceptable for simpler FFI consumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/app.rs` around lines 357 - 374, The method save_tap_signer_backup currently returns bool while get_tap_signer_backup returns Result; change save_tap_signer_backup to return a Result type (recommend Result<(), KeychainError> or Result<bool, KeychainError>) so callers can distinguish "wallet not found" from keychain save failures; locate save_tap_signer_backup and find_tap_signer_wallet, return an Err when find_tap_signer_wallet yields None (create or map to an appropriate error variant such as WalletNotFound) and propagate the error from Keychain::save_tap_signer_backup instead of swallowing it behind a boolean (you can keep logging but must return the error to the caller).android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt (1)
6651-6663: Static analysis warning:Balanceoverridesequals()withouthashCode()Detekt flags that
Balanceimplementsequals()(via UniFFI's RustEqtrait binding) without a correspondinghashCode()override. This violates the Kotlin/Java contract that equal objects must have equal hash codes, which could cause issues ifBalanceinstances are used in hash-based collections (HashMap,HashSet, etc.).However, since this file is auto-generated by UniFFI, manually adding
hashCode()would be overwritten on the next code generation. Consider either:
- Suppressing this warning for generated files in your detekt configuration
- Filing an issue upstream with UniFFI if
Balancewill be used in hash-based collections💡 Suggested detekt configuration to suppress for generated files
Add to your
detekt.yml:potential-bugs: EqualsWithHashCodeExist: excludes: - '**/cove_core/cove.kt'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt` around lines 6651 - 6663, The Balance class overrides equals (Balance.equals via UniFFI-generated binding) but lacks hashCode, triggering Detekt; do not edit the generated Balance class—either suppress the rule for generated UniFFI output by updating detekt configuration to exclude the generated file (e.g., add an excludes pattern for '**/cove_core/cove.kt' or the generated-files directory under potential-bugs.EqualsWithHashCodeExist) or file an upstream UniFFI issue requesting generation of a matching hashCode implementation for the Balance type so future codegen includes both equals and hashCode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/build.gradle.kts`:
- Line 113: Replace the dependency
implementation("androidx.documentfile:documentfile:1.0.1") with
implementation("androidx.documentfile:documentfile:1.1.0"); then ensure the
project uses Kotlin Gradle Plugin (KGP) >= 2.0.0 (upgrade your kotlin
plugin/toolchain declaration and Gradle plugin version if needed) and rebuild;
if new JSpecify-driven Kotlin nullability warnings appear, either fix the
affected code (address nullability in call sites) or adjust Kotlin compiler
options (e.g., add appropriate freeCompilerArgs or explicit nullability
annotations) to resolve or suppress the warnings.
In `@justfile`:
- Around line 378-379: The logcat recipe currently calls adb logcat --pid=$(adb
shell pidof org.bitcoinppl.cove) without checking the PID; update the logcat
justfile recipe to capture the pid into a variable (from adb shell pidof
org.bitcoinppl.cove), strip whitespace, check if the PID is empty, and if so
print a clear error and exit non‑zero; otherwise call adb logcat --pid=<pid>.
This ensures the recipe (logcat) validates the PID before invoking adb logcat
and gives a readable error when the app isn't running.
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 150-162: The paired-delete logic in delete_wallet_items (and the
similar block at lines referenced) computes has_data/has_key but still calls
self.0.delete on both keys, which can cause false failures if delete returns
false for missing entries; change the code to only invoke self.0.delete(key)
when has_data is true and only invoke self.0.delete(encryption_key_key) when
has_key is true, then combine the results (treat omitted deletes as success) so
the final return is true when all existing entries were removed; keep the
initial early-return when neither exists and preserve the order (delete data
before encryption key) to avoid orphaned data.
In `@rust/crates/cove-util/src/encryption.rs`:
- Around line 78-84: The length-conversion failures for the 32-byte key and
12-byte nonce should not map to Error::KeyAndNonceNotFound; update the two
try_into().map_err(...) calls so the key conversion maps to a dedicated
key-length error (e.g., Error::KeyInvalidLength or the repository's existing
key-invalid-format variant) and the nonce conversion maps to a dedicated
nonce-length error (e.g., Error::NonceInvalidLength or reuse
Error::NonceInvalidFormat if that's the project's convention), keeping the
BASE64_STANDARD.decode(...) mapping to Error::NonceInvalidFormat as-is; modify
the map_err targets in the key_bytes try_into() and the nonce_bytes try_into()
calls accordingly.
In `@rust/src/backup/model.rs`:
- Around line 157-158: In BackupImportReport::finalize(), remove the
panic-causing assert_eq! checks on failed_wallet_names vs failed_wallet_errors
and labels_failed_wallet_names vs labels_failed_errors and instead handle
mismatches safely: detect length mismatches, log or record an error (or return a
Result) and reconcile the vectors (e.g., truncate the longer vector or pad with
placeholder errors) so the method never panics; update the function signature to
return a Result if necessary and ensure callers handle the error path. Ensure
you reference and adjust failed_wallet_names, failed_wallet_errors,
labels_failed_wallet_names, and labels_failed_errors inside finalize() when
implementing the safe reconciliation.
---
Outside diff comments:
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 97-109: Currently the code always generates a new Cryptor
(Cryptor::new()) and saves its serialized encryption key
(wallet_mnemonic_encryption_and_nonce_key_name) before saving the ciphertext
(wallet_mnemonic_key_name), which can rotate the key and leave ciphertext
undecryptable if the second save fails; change the flow to first attempt to load
an existing encryption key via self.0.load(...) for
wallet_mnemonic_encryption_and_nonce_key_name and, if present, initialize the
Cryptor from that serialized key and reuse it to encrypt secret_key, only
creating and saving a new encryption key if none exists; ensure you call
self.0.save(encryption_key_key, ...) only when creating a new key and keep the
order so ciphertext is saved after a successful encrypt using the current
encryption key.
In `@rust/crates/cove-util/src/encryption.rs`:
- Around line 71-87: The deserialization in try_from_string currently sets used:
false which allows a reloaded Cryptor to encrypt again and reuse the nonce;
change try_from_string to initialize used: true (or otherwise mark the instance
as decryption-only) so a persisted key+nonce cannot be used for new encryptions
after reload, and add a brief comment in try_from_string/Cryptor noting that
reloaded instances are intentionally disabled for encryption to prevent nonce
reuse.
---
Duplicate comments:
In `@ios/Cove/ShareSheet.swift`:
- Around line 76-83: The completion handler currently unconditionally calls
FileManager.default.removeItem(at: url) which can delete caller-provided files;
update activityViewController.completionWithItemsHandler so it only removes the
file if we created it (e.g., the URL points into NSTemporaryDirectory() or
matches a known temp prefix) or when an explicit "isTemporary" flag/marker is
set, otherwise do not call FileManager.removeItem(at: url); keep the removal
inside the existing do/catch and retain the existing error handling for removal
failures.
---
Nitpick comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/cove.kt`:
- Around line 6651-6663: The Balance class overrides equals (Balance.equals via
UniFFI-generated binding) but lacks hashCode, triggering Detekt; do not edit the
generated Balance class—either suppress the rule for generated UniFFI output by
updating detekt configuration to exclude the generated file (e.g., add an
excludes pattern for '**/cove_core/cove.kt' or the generated-files directory
under potential-bugs.EqualsWithHashCodeExist) or file an upstream UniFFI issue
requesting generation of a matching hashCode implementation for the Balance type
so future codegen includes both equals and hashCode.
In `@rust/src/app.rs`:
- Around line 357-374: The method save_tap_signer_backup currently returns bool
while get_tap_signer_backup returns Result; change save_tap_signer_backup to
return a Result type (recommend Result<(), KeychainError> or Result<bool,
KeychainError>) so callers can distinguish "wallet not found" from keychain save
failures; locate save_tap_signer_backup and find_tap_signer_wallet, return an
Err when find_tap_signer_wallet yields None (create or map to an appropriate
error variant such as WalletNotFound) and propagate the error from
Keychain::save_tap_signer_backup instead of swallowing it behind a boolean (you
can keep logging but must return the error to the caller).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50334ed9-cba7-4580-9cb8-9804ba38493a
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (29)
android/app/build.gradle.ktsandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/WalletMoreOptionsSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/BackupImportView.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftios/Cove/ShareSheet.swiftjustfilerust/.cargo/config.tomlrust/Cargo.tomlrust/crates/cove-cspp/src/cspp.rsrust/crates/cove-device/src/keychain.rsrust/crates/cove-util/src/encryption.rsrust/src/app.rsrust/src/backup.rsrust/src/backup/crypto.rsrust/src/backup/error.rsrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- rust/Cargo.toml
- rust/src/database/global_flag.rs
- ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift
- rust/src/backup.rs
praveenperera
force-pushed
the
beta-full-import-export
branch
from
9f19bfe to
3adb423
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/database/global_config.rs (1)
328-344:⚠️ Potential issue | 🟠 MajorConstrain crate-wide raw config access to avoid bypassing sensitive-key invariants.
Line 328 and Line 344 make generic
get/setcrate-visible. This allows any crate module to read/write sensitive keys (for exampleHashedPinCode,WipeDataPin,DecoyPin) and bypass safeguards likeset_hashed_pin_codevalidation. Prefer a narrower crate-visible API for backup-safe keys only, and keep rawget/setprivate.Suggested refactor direction
- pub(crate) fn get(&self, key: GlobalConfigKey) -> Result<Option<String>> { + fn get(&self, key: GlobalConfigKey) -> Result<Option<String>> { ... } - pub(crate) fn set(&self, key: GlobalConfigKey, value: String) -> Result<()> { + fn set(&self, key: GlobalConfigKey, value: String) -> Result<()> { ... } + // Expose only backup-safe keys at crate scope + pub(crate) fn get_backup_setting(&self, key: BackupConfigKey) -> Result<Option<String>> { + self.get(key.into()) + } + + pub(crate) fn set_backup_setting(&self, key: BackupConfigKey, value: String) -> Result<()> { + self.set(key.into(), value) + }As per coding guidelines, "Check for potential security issues, make sure Bitcoin wallets are handled securely."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/global_config.rs` around lines 328 - 344, The current pub(crate) get and set (methods get and set) expose raw config access allowing bypass of sensitive-key invariants; make those raw get/set private (remove pub(crate)) and introduce a new crate-visible API (e.g., get_backup_safe/set_backup_safe or similar) that only accepts keys validated as backup-safe via a guard on GlobalConfigKey (e.g., an is_sensitive/is_backup_safe check) and returns/sets values accordingly; keep specialized setters like set_hashed_pin_code and set_wipe_data_pin private/public as before and ensure the new crate-visible methods explicitly reject sensitive keys and propagate a clear error when attempted.
♻️ Duplicate comments (3)
ios/Cove/ShareSheet.swift (1)
76-83:⚠️ Potential issue | 🟠 MajorAvoid unconditional deletion of caller-provided URLs (still unresolved).
Line [76]-Line [83] still removes
urlunconditionally in the genericpresent(for: URL, completion:)path. This can delete non-temporary files when this API is reused outside temp exports.Proposed fix
activityViewController.completionWithItemsHandler = { _, completed, _, error in - do { - try FileManager.default.removeItem(at: url) - } catch let removeError as NSError where removeError.domain == NSCocoaErrorDomain && removeError.code == NSFileNoSuchFileError { - // already cleaned up - } catch { - Log.error("Failed to remove temporary backup file: \(error)") - } + let tempPath = FileManager.default.temporaryDirectory.path + if url.isFileURL && url.path.hasPrefix(tempPath) { + do { + try FileManager.default.removeItem(at: url) + } catch let removeError as NSError where removeError.domain == NSCocoaErrorDomain && removeError.code == NSFileNoSuchFileError { + // already cleaned up + } catch { + Log.error("Failed to remove temporary backup file: \(error)") + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/ShareSheet.swift` around lines 76 - 83, The completion handler currently always attempts to delete the provided URL (activityViewController.completionWithItemsHandler), which can remove caller-owned files; change the API and logic so deletion is conditional: either add a removeAfterPresentation: Bool parameter to present(for: URL, completion:) (default false) or track whether the URL was created as a temp copy by this module and only remove when that flag is true; update activityViewController.completionWithItemsHandler to consult that flag/parameter before calling FileManager.default.removeItem(at: url) and keep the existing error handling for removal attempts.rust/crates/cove-device/src/keychain.rs (1)
150-162:⚠️ Potential issue | 🟠 MajorMake paired deletes truly idempotent by gating each delete by existence.
Line [150]-Line [162] and Line [339]-Line [351] still call
deleteon both entries even when one is already absent, so cleanup can incorrectly returnfalseafter successful removal of existing data.Proposed fix
- let data_ok = self.0.delete(key); - let key_ok = self.0.delete(encryption_key_key); + let data_ok = !has_data || self.0.delete(key); + let key_ok = !has_key || self.0.delete(encryption_key_key); data_ok && key_ok- let data_ok = self.0.delete(backup_key); - let key_ok = self.0.delete(encryption_key_key); + let data_ok = !has_data || self.0.delete(backup_key); + let key_ok = !has_key || self.0.delete(encryption_key_key); data_ok && key_okAlso applies to: 339-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-device/src/keychain.rs` around lines 150 - 162, The paired-delete logic in keychain.rs (around the block using has_data and has_key) currently calls self.0.delete(...) for both entries unconditionally, which can make the overall cleanup return false if one side is already absent; change the deletion to be gated by existence: compute has_data and has_key as you already do, then only call self.0.delete(key) if has_data (otherwise treat data_ok as true), and only call self.0.delete(encryption_key_key) if has_key (otherwise treat key_ok as true); apply the same pattern for the similar block later (lines 339-351) so both deletes are idempotent and the method returns true when all existing items were removed.ios/Cove/Flows/SettingsFlow/BackupImportView.swift (1)
140-171:⚠️ Potential issue | 🟠 MajorMove file parsing/validation off the main thread and clear stale file state on picker failure.
Line [149]-[159] performs synchronous file load + validation in the selection callback, which can freeze the UI for large files. Also, Line [169]-[170] does not clear
fileData/fileName, so a failed reselection can leave the previous file importable.💡 Suggested fix (state reset + async parsing direction)
private func handleFileSelection(_ result: Result<URL, Error>) { switch result { case let .success(url): + fileData = nil + fileName = nil guard url.startAccessingSecurityScopedResource() else { errorMessage = "Unable to access the selected file" return } defer { url.stopAccessingSecurityScopedResource() } - do { + do { let attrs = try url.resourceValues(forKeys: [.fileSizeKey]) if let size = attrs.fileSize, size > 50_000_000 { throw BackupError.FileTooLarge } - let data = try Data(contentsOf: url) + // Move heavy read/validation off main thread + let data = try Data(contentsOf: url) if data.count > 50_000_000 { throw BackupError.FileTooLarge } try backupManager.validateFormat(data: data) fileData = data fileName = url.lastPathComponent } catch { fileData = nil fileName = nil errorMessage = (error as? BackupError)?.description ?? error.localizedDescription } case let .failure(error): + fileData = nil + fileName = nil errorMessage = error.localizedDescription } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift` around lines 140 - 171, In handleFileSelection, move the synchronous file read/validation off the main thread: when .success(url) succeeds, immediately guard startAccessingSecurityScopedResource (if it fails clear fileData/fileName and set errorMessage), then dispatch the Data(contentsOf:) and backupManager.validateFormat(data:) to a background queue, and when done switch back to the main thread to set fileData/fileName or set errorMessage; ensure you call url.stopAccessingSecurityScopedResource() after the background work finishes (not before). Also, in the .failure(error) branch clear stale state by setting fileData = nil and fileName = nil in addition to setting errorMessage so a failed picker selection cannot leave a previous file importable.
🧹 Nitpick comments (3)
rust/crates/cove-util/src/encryption.rs (1)
86-86: Consider documenting the decryption-only intent for deserialized Cryptors.Setting
used: falseon deserialization allows encryption on a deserialized instance, which could cause nonce reuse if the same serialized key/nonce was previously used for encryption. The current design relies onserialize_to_stringconsumingselfto prevent immediate reuse, but if the serialized string is stored and later deserialized for encryption (rather than decryption), nonce reuse occurs.Consider either:
- Adding a doc comment warning that deserialized Cryptors should only be used for decryption
- Or tracking
usedstate in serialization format (breaking change)📝 Suggested documentation
/// Create a cryptor from a serialized string /// + /// Note: Deserialized Cryptors should only be used for decryption. + /// Using them for encryption may cause nonce reuse if the original + /// Cryptor was previously used to encrypt data. + /// /// # Errors /// Returns an error if the string is not in the correct format or contains invalid base64🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-util/src/encryption.rs` at line 86, Deserializing Cryptor currently sets used: false which permits encrypting with a deserialized instance and risks nonce reuse; update the API/docs to prevent misuse by adding clear documentation on the Cryptor type and its deserialization path (e.g., the Cryptor::deserialize / impl Deserialize implementation and the Cryptor struct doc comment) stating that deserialized instances are intended for decryption-only and must not be used to encrypt, or alternatively change the serialization format to persist the used flag (breaking change) if you prefer to preserve behavior; include a short migration note and consider adding a helper constructor name like Cryptor::from_serialized_for_decrypt to make intent explicit.ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift (1)
107-113: Reset both backup states before loading to avoid stale UI state.If a previous load succeeded/failed, Line [107]-Line [113] can keep stale
tapSignerBackup/tapSignerBackupErrorvalues on subsequent loads. Clear both before fetching.Proposed fix
func loadTapSignerBackup(_ t: TapSigner) { + tapSignerBackup = nil + tapSignerBackupError = nil do { tapSignerBackup = try app.getTapSignerBackup(t) } catch { tapSignerBackupError = error } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift` around lines 107 - 113, In loadTapSignerBackup, reset both state variables before attempting to fetch so stale values don't persist: set tapSignerBackup = nil and tapSignerBackupError = nil at the start of the loadTapSignerBackup(_ t: TapSigner) function, then call app.getTapSignerBackup(t) in the existing do/catch and assign the result to tapSignerBackup or the caught error to tapSignerBackupError.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt (1)
279-281: CallbackupManager.export(password)directly; avoid Dispatchers.IO for Rust FFI.Line [279]-Line [281] runs the Rust FFI export call inside
withContext(Dispatchers.IO). Keep file I/O on IO, but call Rust FFI directly in the coroutine context to follow project policy.Proposed fix
- val result = withContext(Dispatchers.IO) { - backupManager.export(password) - } + val result = backupManager.export(password)As per coding guidelines, "NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))" and "Rust FFI calls are SYNCHRONOUS and FAST - they complete in microseconds."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt` around lines 279 - 281, The code currently calls backupManager.export(password) inside withContext(Dispatchers.IO); remove that withContext wrapper and invoke backupManager.export(password) directly on the current coroutine context (i.e., call export synchronously), while keeping any surrounding file I/O portions in Dispatchers.IO if needed—locate the call to backupManager.export in BackupExportScreen.kt and replace the withContext(Dispatchers.IO) block so only actual file I/O uses Dispatchers.IO and the Rust FFI export call runs directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/WalletMoreOptionsSheet.kt`:
- Around line 215-218: The alert currently surfaces raw exception text
(e.message) in WalletMoreOptionsSheet's AppAlertState.General which is shown by
GlobalAlertDialog; instead, replace the user-facing message with a stable,
generic string like "Failed to retrieve backup" and move the detailed exception
to logs (use Log.e or the existing logger inside the same scope) so the detailed
error stays in logs for debugging while AppAlertState.General contains only the
generic text; update the code that constructs AppAlertState.General (the block
that references e.message) to remove e.message and add a logging call that logs
the exception and any context.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt`:
- Around line 92-97: The DisposableEffect(Unit) onDispose currently clears
password, pendingResult and closes backupManager but doesn't clear the
clipboard; update the onDispose block in BackupExportScreen's DisposableEffect
to also clear the clipboard by calling
clipboardManager.setText(AnnotatedString("")) (or the equivalent
ClipboardManager clear/set empty text) so copied password is removed if the
screen is disposed without handleDismiss(); ensure you reference the existing
clipboardManager instance used in the composable and perform this action
alongside password = "", pendingResult = null, and backupManager.close().
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt`:
- Around line 108-133: The Rust FFI calls backupManager.validateFormat(bytes)
and backupManager.importBackup(data, password) are being executed inside
withContext(Dispatchers.IO); move only the pure file I/O (reading bytes from
contentResolver / DocumentFile logic) into the Dispatchers.IO block and perform
backupManager.validateFormat(bytes) and backupManager.importBackup(...) on the
calling (UI) coroutine context instead—i.e., return the read ByteArray and
filename from the IO block, then call validateFormat and importBackup outside
that withContext so the FFI runs on the main/context thread.
In `@ios/Cove/Flows/SettingsFlow/BackupExportView.swift`:
- Around line 149-155: The completion handler passed to ShareSheet.present(for:
fileURL) redundantly attempts to delete the temp file (the try
FileManager.default.removeItem(at: fileURL) and its catch/print), but
ShareSheet.present already removes the file; remove the duplicate deletion and
warning log. In BackupExportView/BackupExportView.swift, inside the closure for
ShareSheet.present(for: fileURL) keep only the shareSheetPresented = false (and
any existing UI state updates) and delete the
FileManager.default.removeItem(...) try/catch block so no misleading warning is
emitted.
In `@rust/src/app.rs`:
- Around line 244-246: The two reconcile events are sent in the wrong order:
Updater::send_update(AppMessage::WalletsChanged) runs before
Updater::send_update(AppMessage::DatabaseUpdated), causing frontends reacting to
WalletsChanged to read a stale database; fix by sending
AppMessage::DatabaseUpdated before AppMessage::WalletsChanged (i.e., call
Updater::send_update(AppMessage::DatabaseUpdated) first, then
Updater::send_update(AppMessage::WalletsChanged)) so consumers read the updated
database state.
In `@rust/src/backup/crypto.rs`:
- Around line 155-170: The decompression cap constant MAX_DECOMPRESSED_SIZE used
by decompress() is set to 10 MB but the client accepts up to 50 MB, which can
reject valid backups; update MAX_DECOMPRESSED_SIZE to match the accepted backup
size (e.g., 50 * 1024 * 1024) and/or replace the hardcoded constant with a
single shared configuration entry referenced by decompress() so UI and Rust core
enforce the same limit (ensure the error message and the reader.take(...) use
the updated/shared value).
In `@rust/src/backup/export.rs`:
- Around line 86-107: The export currently allows both xpub and descriptors to
be None for a given wallet (values from self.keychain.get_wallet_xpub and
self.keychain.get_public_descriptor), producing backups that cannot be restored
by restore_descriptor_wallet; update the export logic (where xpub and
descriptors are obtained) to validate that at least one of xpub or descriptors
is present and return an Err(BackupError::Keychain(...)) with a clear message
identifying the wallet id when both are missing, rather than allowing None/None
to pass through.
---
Outside diff comments:
In `@rust/src/database/global_config.rs`:
- Around line 328-344: The current pub(crate) get and set (methods get and set)
expose raw config access allowing bypass of sensitive-key invariants; make those
raw get/set private (remove pub(crate)) and introduce a new crate-visible API
(e.g., get_backup_safe/set_backup_safe or similar) that only accepts keys
validated as backup-safe via a guard on GlobalConfigKey (e.g., an
is_sensitive/is_backup_safe check) and returns/sets values accordingly; keep
specialized setters like set_hashed_pin_code and set_wipe_data_pin
private/public as before and ensure the new crate-visible methods explicitly
reject sensitive keys and propagate a clear error when attempted.
---
Duplicate comments:
In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift`:
- Around line 140-171: In handleFileSelection, move the synchronous file
read/validation off the main thread: when .success(url) succeeds, immediately
guard startAccessingSecurityScopedResource (if it fails clear fileData/fileName
and set errorMessage), then dispatch the Data(contentsOf:) and
backupManager.validateFormat(data:) to a background queue, and when done switch
back to the main thread to set fileData/fileName or set errorMessage; ensure you
call url.stopAccessingSecurityScopedResource() after the background work
finishes (not before). Also, in the .failure(error) branch clear stale state by
setting fileData = nil and fileName = nil in addition to setting errorMessage so
a failed picker selection cannot leave a previous file importable.
In `@ios/Cove/ShareSheet.swift`:
- Around line 76-83: The completion handler currently always attempts to delete
the provided URL (activityViewController.completionWithItemsHandler), which can
remove caller-owned files; change the API and logic so deletion is conditional:
either add a removeAfterPresentation: Bool parameter to present(for: URL,
completion:) (default false) or track whether the URL was created as a temp copy
by this module and only remove when that flag is true; update
activityViewController.completionWithItemsHandler to consult that flag/parameter
before calling FileManager.default.removeItem(at: url) and keep the existing
error handling for removal attempts.
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 150-162: The paired-delete logic in keychain.rs (around the block
using has_data and has_key) currently calls self.0.delete(...) for both entries
unconditionally, which can make the overall cleanup return false if one side is
already absent; change the deletion to be gated by existence: compute has_data
and has_key as you already do, then only call self.0.delete(key) if has_data
(otherwise treat data_ok as true), and only call
self.0.delete(encryption_key_key) if has_key (otherwise treat key_ok as true);
apply the same pattern for the similar block later (lines 339-351) so both
deletes are idempotent and the method returns true when all existing items were
removed.
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt`:
- Around line 279-281: The code currently calls backupManager.export(password)
inside withContext(Dispatchers.IO); remove that withContext wrapper and invoke
backupManager.export(password) directly on the current coroutine context (i.e.,
call export synchronously), while keeping any surrounding file I/O portions in
Dispatchers.IO if needed—locate the call to backupManager.export in
BackupExportScreen.kt and replace the withContext(Dispatchers.IO) block so only
actual file I/O uses Dispatchers.IO and the Rust FFI export call runs directly.
In `@ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift`:
- Around line 107-113: In loadTapSignerBackup, reset both state variables before
attempting to fetch so stale values don't persist: set tapSignerBackup = nil and
tapSignerBackupError = nil at the start of the loadTapSignerBackup(_ t:
TapSigner) function, then call app.getTapSignerBackup(t) in the existing
do/catch and assign the result to tapSignerBackup or the caught error to
tapSignerBackupError.
In `@rust/crates/cove-util/src/encryption.rs`:
- Line 86: Deserializing Cryptor currently sets used: false which permits
encrypting with a deserialized instance and risks nonce reuse; update the
API/docs to prevent misuse by adding clear documentation on the Cryptor type and
its deserialization path (e.g., the Cryptor::deserialize / impl Deserialize
implementation and the Cryptor struct doc comment) stating that deserialized
instances are intended for decryption-only and must not be used to encrypt, or
alternatively change the serialization format to persist the used flag (breaking
change) if you prefer to preserve behavior; include a short migration note and
consider adding a helper constructor name like
Cryptor::from_serialized_for_decrypt to make intent explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a08d6d34-9850-4f20-96ae-b69f8aa647a7
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (26)
android/app/build.gradle.ktsandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/WalletMoreOptionsSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktios/Cove/AppManager.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/BackupImportView.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftios/Cove/ShareSheet.swiftrust/Cargo.tomlrust/crates/cove-device/src/keychain.rsrust/crates/cove-util/src/encryption.rsrust/src/app.rsrust/src/backup.rsrust/src/backup/crypto.rsrust/src/backup/error.rsrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- rust/src/database/global_flag.rs
- rust/src/lib.rs
- android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt
- ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift
- rust/Cargo.toml
...oid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/WalletMoreOptionsSheet.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Show resolved
Hide resolved
praveenperera
force-pushed
the
beta-full-import-export
branch
2 times, most recently
from
d228947 to
364e6bd
Compare
Contributor
Author
|
@greptile-apps re-review |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/database/global_config.rs (1)
328-344:⚠️ Potential issue | 🟠 MajorCrate-wide raw
get/setweakens safeguards for sensitive config keys.Line 328 and Line 344 make generic config I/O crate-visible, which allows direct read/write of keys like
HashedPinCode,WipeDataPin, andDecoyPinfrom any module, bypassing validation paths (e.g.,set_hashed_pin_code). In wallet code, this broadens risk unnecessarily.Prefer keeping raw
get/setprivate and exposing narrowpub(crate)backup/import APIs with an explicit allowlist of keys.Suggested direction
- pub(crate) fn get(&self, key: GlobalConfigKey) -> Result<Option<String>> { + fn get(&self, key: GlobalConfigKey) -> Result<Option<String>> { // ... } - pub(crate) fn set(&self, key: GlobalConfigKey, value: String) -> Result<()> { + fn set(&self, key: GlobalConfigKey, value: String) -> Result<()> { // ... } + + // Add scoped APIs for backup/import with an allowlisted key enum + pub(crate) fn backup_get(&self, key: BackupConfigKey) -> Result<Option<String>> { + self.get(key.into()) + } + + pub(crate) fn backup_set(&self, key: BackupConfigKey, value: String) -> Result<()> { + self.set(key.into(), value) + }As per coding guidelines,
rust/**/*.rs: "Check for potential security issues, make sure Bitcoin wallets are handled securely."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/database/global_config.rs` around lines 328 - 344, The crate-visible raw accessors get and set on GlobalConfigTable (functions named get and set) expose all GlobalConfigKey variants—including sensitive keys like HashedPinCode, WipeDataPin, and DecoyPin—so change get and set to private (remove pub(crate)) and create explicit pub(crate) backup/import APIs (e.g., backup_get/backup_set or import_read/import_write) that only allow an explicit allowlist of non-sensitive keys or are explicitly annotated to handle backups; implement the allowlist by matching on GlobalConfigKey variants inside the new functions and return an error for disallowed keys, and update internal call sites to use the new narrow APIs instead of the raw get/set.
♻️ Duplicate comments (3)
rust/crates/cove-device/src/keychain.rs (1)
339-351:⚠️ Potential issue | 🟠 MajorGate each delete by presence in
delete_tap_signer_backupLine 349 and Line 350 still delete both entries unconditionally after computing
has_data/has_key. This can cause false failures when one item is already missing and backenddeletereturnsfalsefor missing keys.Suggested fix
- let data_ok = self.0.delete(backup_key); - let key_ok = self.0.delete(encryption_key_key); + let data_ok = if has_data { self.0.delete(backup_key) } else { true }; + let key_ok = if has_key { self.0.delete(encryption_key_key) } else { true }; data_ok && key_okAs per coding guidelines, "rust/**/*.rs: - Verify correct functionality and logic consistency."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/crates/cove-device/src/keychain.rs` around lines 339 - 351, The delete logic in delete_tap_signer_backup computes has_data/has_key but then unconditionally calls self.0.delete(backup_key) and self.0.delete(encryption_key_key), which can return false for already-missing entries and incorrectly fail; change it to only call self.0.delete for each key when its corresponding has_data or has_key is true (e.g., set data_ok = true if !has_data else call delete(backup_key), and similarly for key_ok with encryption_key_key) and return data_ok && key_ok so missing items are treated as successful.rust/src/app.rs (1)
242-263:⚠️ Potential issue | 🟠 MajorEvent ordering:
WalletsChangedsent beforeDatabaseUpdated.Lines 244-245 send
WalletsChangedbeforeDatabaseUpdated. Frontends handlingWalletsChangedtypically reload wallets from the database, but the database instance may not have been refreshed yet ifDatabaseUpdatedhasn't been processed.Proposed fix
AppAction::RefreshAfterImport => { debug!("refreshing state after backup import"); - Updater::send_update(AppMessage::WalletsChanged); Updater::send_update(AppMessage::DatabaseUpdated); + Updater::send_update(AppMessage::WalletsChanged);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/app.rs` around lines 242 - 263, The event ordering in AppAction::RefreshAfterImport is wrong: Updater::send_update currently sends AppMessage::WalletsChanged before AppMessage::DatabaseUpdated, which can cause frontends to read stale DB state; change the order in the RefreshAfterImport match arm so that Updater::send_update(AppMessage::DatabaseUpdated) is sent before Updater::send_update(AppMessage::WalletsChanged), keeping the subsequent config-derived sends (SelectedNetworkChanged, ColorSchemeChanged, FiatCurrencyChanged, SelectedNodeChanged) unchanged and still using Database::global().global_config for config reads.rust/src/backup/export.rs (1)
86-107:⚠️ Potential issue | 🟡 MinorDescriptor-based wallets can export with neither xpub nor descriptors.
For
WalletType::Cold,WalletType::XpubOnly, andWalletType::WatchOnly, bothxpubanddescriptorscan beNoneafter Lines 86-107. The import path (restore_descriptor_wallet) explicitly rejects this combination (Lines 351-356 in import.rs), so such backups would be non-restorable.Consider adding validation here to fail fast during export rather than producing a backup that will fail on import.
🛡️ Proposed validation guard
let descriptors = match self.keychain.get_public_descriptor(id) { Ok(Some((ext, int))) => Some(DescriptorPair { external: ext.to_string(), internal: int.to_string(), }), Ok(None) => None, Err(e) => { return Err(BackupError::Keychain(format!( "failed to read descriptors for wallet {id}: {e}" ))); } }; + // Descriptor-based wallets need at least xpub or descriptors to be restorable + if matches!( + metadata.wallet_type, + WalletType::Cold | WalletType::XpubOnly | WalletType::WatchOnly + ) && xpub.is_none() && descriptors.is_none() + { + return Err(BackupError::Gather(format!( + "wallet {id} has no xpub or descriptors, export would be non-restorable" + ))); + }As per coding guidelines
rust/**/*.rs: "Verify correct functionality and logic consistency." and "Check for potential security issues, make sure Bitcoin wallets are handled securely."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/export.rs` around lines 86 - 107, After retrieving xpub (via get_wallet_xpub) and descriptors (via get_public_descriptor) in the export routine, add a validation that for WalletType::Cold, WalletType::XpubOnly, and WalletType::WatchOnly you must not allow both xpub and descriptors to be None; if both are None return an early Err (e.g., BackupError::Keychain or a new BackupError::InvalidBackup) with a clear message indicating the wallet id and that neither xpub nor descriptors are present and the wallet would be non-restorable (this check should live immediately after computing xpub and descriptors and before continuing to build the backup, referencing restore_descriptor_wallet semantics).
🧹 Nitpick comments (9)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt (1)
277-291: Keep Rust FFI call offDispatchers.IO.
backupManager.export(password)at line 280 is a Rust FFI call wrapped inwithContext(Dispatchers.IO). Per project policy, Rust FFI calls should run on the calling coroutine context since they complete quickly and the Rust side manages async internally.Proposed fix
scope.launch { try { - val result = withContext(Dispatchers.IO) { - backupManager.export(password) - } + val result = backupManager.export(password) pendingResult = result exportLauncher.launch(result.filename) } catch (e: CancellationException) {As per coding guidelines
android/app/src/main/java/org/bitcoinppl/cove/**/*.kt: "NEVER suggest moving Rust FFI calls to background threads (withContext(Dispatchers.IO))".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt` around lines 277 - 291, The Rust FFI call backupManager.export(password) is being forced onto Dispatchers.IO inside the scope.launch block; remove the withContext(Dispatchers.IO) wrapper and invoke backupManager.export(password) directly on the current coroutine context (keeping the surrounding try/catch and CancellationException handling intact), then assign pendingResult and call exportLauncher.launch(result.filename) as before so the call runs on the caller context per project FFI policy.android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt (1)
243-268: Consider not exposing raw exception message in beta error dialog.At line 256,
e.messageis interpolated into the user-visiblebetaErrordialog. While this is a beta feature toggle (lower risk), consider using a generic message for consistency with the rest of the codebase.Proposed fix
} catch (e: Exception) { - betaError = "Failed to update beta features: ${e.message}" + android.util.Log.e("MainSettings", "Failed to update beta features", e) + betaError = "Failed to update beta features. Please try again." }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt` around lines 243 - 268, The user-visible error message currently sets betaError to include the raw exception text (betaError = "Failed to update beta features: ${e.message}") inside MainSettingsScreen after Database().globalFlag().set(...) fails; change this to use a generic, non-sensitive message (e.g., "Failed to update beta features. Please try again.") while still logging the exception internally if needed (log e in the catch block or report to telemetry) and preserve setting showBetaDialog = false and isBetaEnabled behavior.ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift (1)
239-241: Import does not require authentication while export does.Export requires authentication (lines 232-236), but Import bypasses auth entirely. Consider whether importing backup data (which could overwrite wallet settings) should also require authentication for consistency and security.
Add auth gate for import
SettingsRow(title: "Import All", symbol: "square.and.arrow.down") { - sheetState = .init(.backupImport) + if auth.type != .none { + sheetState = .init(.backupImportAuth) + } else { + sheetState = .init(.backupImport) + } }This would require adding a
.backupImportAuthsheet state similar to.backupExportAuth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift` around lines 239 - 241, The Import button currently sets sheetState = .init(.backupImport) and bypasses authentication; add an auth gate mirroring the export flow by changing the action to set sheetState = .init(.backupImportAuth) (or route through the same auth flow used by .backupExportAuth), implement a new .backupImportAuth case in the SheetState enum/handler, and ensure the auth completion transitions to .backupImport so the import sheet only appears after successful authentication; update any handlers or switch statements that map sheetState to presented views to support the new .backupImportAuth state.ios/Cove/Flows/SettingsFlow/BackupImportView.swift (2)
174-191: Import task cancellation doesn't prevent completion handlers from running.When
importTaskis cancelled (line 96), the task'scatchblock may still execute and seterrorMessage. Consider checkingTask.isCancelledbefore updating UI state.Add cancellation check
private func importBackup() { guard let fileData else { return } isImporting = true importTask = Task { do { let report = try await backupManager.importBackup(data: fileData, password: password) + guard !Task.isCancelled else { return } await MainActor.run { isImporting = false importReport = report } } catch { + guard !Task.isCancelled else { return } await MainActor.run { isImporting = false errorMessage = (error as? BackupError)?.description ?? error.localizedDescription } } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift` around lines 174 - 191, The importBackup Task should avoid touching UI state if it was cancelled: inside importBackup's Task (the async block that calls backupManager.importBackup), check Task.isCancelled (or Task.currentPriority/Task.checkCancellation) before updating isImporting, importReport, or errorMessage—both after a successful await and inside the catch—so bail out early when cancelled; reference the importBackup function, importTask property, and the await call to backupManager.importBackup(data:password:) to locate where to add the cancellation checks and skip UI updates when cancelled.
150-158: Redundant file size validation.The file size is checked twice: first via
resourceValues(line 151) and again after reading the data (line 156). The second check is redundant sinceData(contentsOf:)would have already loaded the file. Consider removing one check.Simplify to single check
do { - let attrs = try url.resourceValues(forKeys: [.fileSizeKey]) - if let size = attrs.fileSize, size > 50_000_000 { - throw BackupError.FileTooLarge - } - let data = try Data(contentsOf: url) if data.count > 50_000_000 { throw BackupError.FileTooLarge }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift` around lines 150 - 158, The code currently checks file size twice (using url.resourceValues(forKeys:) and again with data.count). Update the logic in BackupImportView so you use the file size from resourceValues when available to enforce the 50_000_000 limit and only fall back to checking Data(contentsOf: url).count when resourceValues.fileSize is nil; remove the unconditional redundant data.count > 50_000_000 check and its throw for cases where attrs.fileSize was already validated. Ensure you still throw BackupError.FileTooLarge (the same symbol) when either check indicates the file is too large.rust/src/backup/import.rs (2)
196-204: Consider using a HashSet for fingerprint lookups.Line 197 uses
Vec::containsfor duplicate checking, which is O(n) per lookup. With many wallets, this could become slow. AHashSetwould provide O(1) lookups.♻️ Suggested optimization
-fn collect_existing_fingerprints() -> Result<Vec<(Fingerprint, Network, WalletMode)>, BackupError> { +fn collect_existing_fingerprints() -> Result<std::collections::HashSet<(Fingerprint, Network, WalletMode)>, BackupError> { let db = Database::global(); - let mut fingerprints = Vec::new(); + let mut fingerprints = std::collections::HashSet::new(); for network in Network::iter() { for mode in [WalletMode::Main, WalletMode::Decoy] { // ... for wallet in wallets { if let Some(fp) = &wallet.master_fingerprint { - fingerprints.push((**fp, network, mode)); + fingerprints.insert((**fp, network, mode)); } } } } Ok(fingerprints) }Then update the caller to use
fingerprints.contains(&(...))andfingerprints.insert(entry).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` around lines 196 - 204, The duplicate-fingerprint check currently uses a Vec lookup (existing_fingerprints.contains(...)) which is O(n); change existing_fingerprints to a HashSet of the same tuple type so lookups become O(1), update the code in import.rs where metadata.master_fingerprint is checked to use existing_fingerprints.contains(&(...)) as before, and ensure callers that populate existing_fingerprints switch from pushing to using existing_fingerprints.insert(entry) (maintain the same tuple shape used in the contains call); no behavioral changes to RestoreResult::Skipped are needed.
282-334: Mnemonic is cloned before being consumed, leaving unzeroized copy.Line 303 clones the mnemonic to extract descriptors before the original is consumed by
save_wallet_key. Thebip39::Mnemonictype doesn't implementZeroize, so this clone remains in memory until garbage collected.The code comment on Lines 232-234 acknowledges this limitation. Given that the upstream
bip39crate doesn't support zeroization and the exposure window is limited, this is an acceptable tradeoff, but worth documenting in security considerations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/import.rs` around lines 282 - 334, Mnemonic is cloned in restore_mnemonic_wallet_inner (mnemonic.clone().into_descriptors(...)) before the original is consumed by save_wallet_key, leaving an unzeroized copy in memory; update the function to document this tradeoff by adding a clear comment next to the mnemonic.clone().into_descriptors call explaining that bip39::Mnemonic does not implement Zeroize, that the clone is necessary to extract descriptors before consuming the original, and noting the security implications and that this is an accepted temporary compromise until upstream zeroization is available (also mention to revisit when bip39 supports Zeroize so the clone can be removed and the mnemonic securely zeroed after save_wallet_key).rust/src/backup/export.rs (2)
70-84: Cold wallet without TapSignerBackup exports successfully but may produce incomplete backup.Lines 72-76 allow a cold wallet to export with
WalletSecret::Nonewhenget_tap_signer_backupreturnsOk(None). While a warning is logged and added to the result, this could produce a backup that cannot fully restore the wallet's signing capability.Consider whether this graceful degradation is intentional or if you'd prefer to fail the export for cold wallets missing their backup material, similar to how hot wallets now fail when missing mnemonics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/export.rs` around lines 70 - 84, The current branch for WalletType::Cold treats Ok(None) from self.keychain.get_tap_signer_backup as a warning and returns WalletSecret::None; change this to fail the export instead: in the match arm handling Ok(None) for WalletType::Cold inside export.rs, return an Err(BackupError::Keychain(...)) with a clear message that the cold wallet {id} / {metadata.name} is missing its tap signer backup (include id and name), mirroring the error style used in the Err(e) branch; update any tests or callers expecting a successful export without backup accordingly.
149-158: Settings read failures are logged but not surfaced in result warnings.The
get_configmethod logs errors viatracing::warnand adds them toself.warnings, which is an improvement over silently ignoring them. However, the warning message on Line 154 says "using default" but the function returnsNone, not a default value. This could be misleading.📝 Suggested clarification
Err(e) => { warn!("failed to read config key {key:?}: {e}"); - self.warnings.push(format!("Failed to read a setting ({key:?}), using default")); + self.warnings.push(format!("Failed to read setting {key:?}: {e}")); None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rust/src/backup/export.rs` around lines 149 - 158, The log in get_config claims "using default" but the function returns None, which is misleading; either return a concrete default or update the warning to reflect that no value is available. Modify the get_config method (function name get_config, struct field self.warnings, and the warn! call) so that on Err(e) you either (A) produce and return a real default String for the given GlobalConfigKey, pushing a corresponding warning that indicates a default was used, or (B) change the warn! and self.warnings message to state that the value is unavailable/None (e.g. "failed to read config key {key:?}: {e}; using no value") so the log matches the actual return of None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.kt`:
- Around line 2366-2370: The KDoc for uniffiIsDestroyed is incorrect: it
currently claims the Rust-side reference is gone but the property just mirrors
wasDestroyed which flips when destroy() is requested; update the KDoc for
uniffiIsDestroyed (and the other identical occurrences) to state that it
indicates whether destroy() has been requested / the local wrapper marked
destroyed, not a guarantee that the Rust-side reference has been fully freed;
reference the wasDestroyed backing field and the destroy() method in the doc so
readers understand the actual semantics.
In `@ios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swift`:
- Around line 107-113: In loadTapSignerBackup, clear the previous state before
starting the fetch and ensure state is correctly set after completion: set
tapSignerBackup = nil and tapSignerBackupError = nil at the start of
loadTapSignerBackup, then call app.getTapSignerBackup(t) and on success assign
tapSignerBackup and ensure tapSignerBackupError is nil; on catch assign
tapSignerBackupError and set tapSignerBackup = nil to avoid leaving stale data
visible; reference loadTapSignerBackup, tapSignerBackup, tapSignerBackupError,
and app.getTapSignerBackup when making the edits.
In `@rust/src/backup/crypto.rs`:
- Around line 155-175: The server decompression cap (MAX_DECOMPRESSED_SIZE) is
10 MB but clients accept up to 50 MB, causing valid client uploads to be
rejected; update the constant MAX_DECOMPRESSED_SIZE to match the client-side 50
MB limit (set to 50 * 1024 * 1024) and ensure the decompress function/reader
logic (StreamingDecoder::new, .take, and the final length check in decompress)
uses that updated value; alternatively, if keeping 10 MB is intentional, add a
clear comment referencing the client 50 MB limit and the reason for the stricter
server cap so future reviewers see the intentional discrepancy.
---
Outside diff comments:
In `@rust/src/database/global_config.rs`:
- Around line 328-344: The crate-visible raw accessors get and set on
GlobalConfigTable (functions named get and set) expose all GlobalConfigKey
variants—including sensitive keys like HashedPinCode, WipeDataPin, and
DecoyPin—so change get and set to private (remove pub(crate)) and create
explicit pub(crate) backup/import APIs (e.g., backup_get/backup_set or
import_read/import_write) that only allow an explicit allowlist of non-sensitive
keys or are explicitly annotated to handle backups; implement the allowlist by
matching on GlobalConfigKey variants inside the new functions and return an
error for disallowed keys, and update internal call sites to use the new narrow
APIs instead of the raw get/set.
---
Duplicate comments:
In `@rust/crates/cove-device/src/keychain.rs`:
- Around line 339-351: The delete logic in delete_tap_signer_backup computes
has_data/has_key but then unconditionally calls self.0.delete(backup_key) and
self.0.delete(encryption_key_key), which can return false for already-missing
entries and incorrectly fail; change it to only call self.0.delete for each key
when its corresponding has_data or has_key is true (e.g., set data_ok = true if
!has_data else call delete(backup_key), and similarly for key_ok with
encryption_key_key) and return data_ok && key_ok so missing items are treated as
successful.
In `@rust/src/app.rs`:
- Around line 242-263: The event ordering in AppAction::RefreshAfterImport is
wrong: Updater::send_update currently sends AppMessage::WalletsChanged before
AppMessage::DatabaseUpdated, which can cause frontends to read stale DB state;
change the order in the RefreshAfterImport match arm so that
Updater::send_update(AppMessage::DatabaseUpdated) is sent before
Updater::send_update(AppMessage::WalletsChanged), keeping the subsequent
config-derived sends (SelectedNetworkChanged, ColorSchemeChanged,
FiatCurrencyChanged, SelectedNodeChanged) unchanged and still using
Database::global().global_config for config reads.
In `@rust/src/backup/export.rs`:
- Around line 86-107: After retrieving xpub (via get_wallet_xpub) and
descriptors (via get_public_descriptor) in the export routine, add a validation
that for WalletType::Cold, WalletType::XpubOnly, and WalletType::WatchOnly you
must not allow both xpub and descriptors to be None; if both are None return an
early Err (e.g., BackupError::Keychain or a new BackupError::InvalidBackup) with
a clear message indicating the wallet id and that neither xpub nor descriptors
are present and the wallet would be non-restorable (this check should live
immediately after computing xpub and descriptors and before continuing to build
the backup, referencing restore_descriptor_wallet semantics).
---
Nitpick comments:
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt`:
- Around line 277-291: The Rust FFI call backupManager.export(password) is being
forced onto Dispatchers.IO inside the scope.launch block; remove the
withContext(Dispatchers.IO) wrapper and invoke backupManager.export(password)
directly on the current coroutine context (keeping the surrounding try/catch and
CancellationException handling intact), then assign pendingResult and call
exportLauncher.launch(result.filename) as before so the call runs on the caller
context per project FFI policy.
In
`@android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt`:
- Around line 243-268: The user-visible error message currently sets betaError
to include the raw exception text (betaError = "Failed to update beta features:
${e.message}") inside MainSettingsScreen after Database().globalFlag().set(...)
fails; change this to use a generic, non-sensitive message (e.g., "Failed to
update beta features. Please try again.") while still logging the exception
internally if needed (log e in the catch block or report to telemetry) and
preserve setting showBetaDialog = false and isBetaEnabled behavior.
In `@ios/Cove/Flows/SettingsFlow/BackupImportView.swift`:
- Around line 174-191: The importBackup Task should avoid touching UI state if
it was cancelled: inside importBackup's Task (the async block that calls
backupManager.importBackup), check Task.isCancelled (or
Task.currentPriority/Task.checkCancellation) before updating isImporting,
importReport, or errorMessage—both after a successful await and inside the
catch—so bail out early when cancelled; reference the importBackup function,
importTask property, and the await call to
backupManager.importBackup(data:password:) to locate where to add the
cancellation checks and skip UI updates when cancelled.
- Around line 150-158: The code currently checks file size twice (using
url.resourceValues(forKeys:) and again with data.count). Update the logic in
BackupImportView so you use the file size from resourceValues when available to
enforce the 50_000_000 limit and only fall back to checking Data(contentsOf:
url).count when resourceValues.fileSize is nil; remove the unconditional
redundant data.count > 50_000_000 check and its throw for cases where
attrs.fileSize was already validated. Ensure you still throw
BackupError.FileTooLarge (the same symbol) when either check indicates the file
is too large.
In `@ios/Cove/Flows/SettingsFlow/MainSettingsScreen.swift`:
- Around line 239-241: The Import button currently sets sheetState =
.init(.backupImport) and bypasses authentication; add an auth gate mirroring the
export flow by changing the action to set sheetState = .init(.backupImportAuth)
(or route through the same auth flow used by .backupExportAuth), implement a new
.backupImportAuth case in the SheetState enum/handler, and ensure the auth
completion transitions to .backupImport so the import sheet only appears after
successful authentication; update any handlers or switch statements that map
sheetState to presented views to support the new .backupImportAuth state.
In `@rust/src/backup/export.rs`:
- Around line 70-84: The current branch for WalletType::Cold treats Ok(None)
from self.keychain.get_tap_signer_backup as a warning and returns
WalletSecret::None; change this to fail the export instead: in the match arm
handling Ok(None) for WalletType::Cold inside export.rs, return an
Err(BackupError::Keychain(...)) with a clear message that the cold wallet {id} /
{metadata.name} is missing its tap signer backup (include id and name),
mirroring the error style used in the Err(e) branch; update any tests or callers
expecting a successful export without backup accordingly.
- Around line 149-158: The log in get_config claims "using default" but the
function returns None, which is misleading; either return a concrete default or
update the warning to reflect that no value is available. Modify the get_config
method (function name get_config, struct field self.warnings, and the warn!
call) so that on Err(e) you either (A) produce and return a real default String
for the given GlobalConfigKey, pushing a corresponding warning that indicates a
default was used, or (B) change the warn! and self.warnings message to state
that the value is unavailable/None (e.g. "failed to read config key {key:?}:
{e}; using no value") so the log matches the actual return of None.
In `@rust/src/backup/import.rs`:
- Around line 196-204: The duplicate-fingerprint check currently uses a Vec
lookup (existing_fingerprints.contains(...)) which is O(n); change
existing_fingerprints to a HashSet of the same tuple type so lookups become
O(1), update the code in import.rs where metadata.master_fingerprint is checked
to use existing_fingerprints.contains(&(...)) as before, and ensure callers that
populate existing_fingerprints switch from pushing to using
existing_fingerprints.insert(entry) (maintain the same tuple shape used in the
contains call); no behavioral changes to RestoreResult::Skipped are needed.
- Around line 282-334: Mnemonic is cloned in restore_mnemonic_wallet_inner
(mnemonic.clone().into_descriptors(...)) before the original is consumed by
save_wallet_key, leaving an unzeroized copy in memory; update the function to
document this tradeoff by adding a clear comment next to the
mnemonic.clone().into_descriptors call explaining that bip39::Mnemonic does not
implement Zeroize, that the clone is necessary to extract descriptors before
consuming the original, and noting the security implications and that this is an
accepted temporary compromise until upstream zeroization is available (also
mention to revisit when bip39 supports Zeroize so the clone can be removed and
the mnemonic securely zeroed after save_wallet_key).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25bdc691-850d-4ee8-88a9-241ba526b19f
⛔ Files ignored due to path filters (2)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (33)
android/app/build.gradle.ktsandroid/app/src/main/java/org/bitcoinppl/cove/AppManager.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SelectedWalletFlow/WalletMoreOptionsSheet.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/cove.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/device/cove_device.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/nfc/cove_nfc.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/tapcard/cove_tap_card.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/types/cove_types.ktandroid/app/src/main/java/org/bitcoinppl/cove_core/ur/cove_ur.ktios/Cove/AppManager.swiftios/Cove/Flows/SelectedWalletFlow/MoreInfoPopover.swiftios/Cove/Flows/SettingsFlow/BackupExportView.swiftios/Cove/Flows/SettingsFlow/BackupImportView.swiftios/Cove/Flows/SettingsFlow/MainSettingsScreen.swiftios/Cove/ShareSheet.swiftjustfilerust/Cargo.tomlrust/crates/cove-cspp/src/cspp.rsrust/crates/cove-device/src/keychain.rsrust/crates/cove-util/src/encryption.rsrust/src/app.rsrust/src/backup.rsrust/src/backup/crypto.rsrust/src/backup/error.rsrust/src/backup/export.rsrust/src/backup/import.rsrust/src/backup/model.rsrust/src/database/global_config.rsrust/src/database/global_flag.rsrust/src/lib.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- rust/src/backup/error.rs
- justfile
- rust/src/database/global_flag.rs
- ios/Cove/Flows/SettingsFlow/BackupExportView.swift
- android/app/src/main/java/org/bitcoinppl/cove/AppManager.kt
- rust/Cargo.toml
praveenperera
force-pushed
the
beta-full-import-export
branch
from
364e6bd to
71f6e9c
Compare
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupExportScreen.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/MainSettingsScreen.kt
Show resolved
Hide resolved
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/BackupImportScreen.kt
Show resolved
Hide resolved
praveenperera
force-pushed
the
beta-full-import-export
branch
from
3f59660 to
2139231
Compare
praveenperera
force-pushed
the
beta-full-import-export
branch
from
2139231 to
20d79b5
Compare
praveenperera
force-pushed
the
beta-full-import-export
branch
from
20d79b5 to
43b172d
Compare
Fix Android crash from reqwest::Client::new() with rustls-no-provider by moving the TLS-configured client builder into its own crate. This keeps rustls and webpki-roots as deps of cove-http only, with just rustls remaining in the main crate for crypto provider install.
Beta mode activation now shows confirmation alert and pops back to settings. Backup import/export requires a separate toggle with a warning about experimental nature, gated behind BetaImportExportEnabled flag. Disabling beta features also resets the import/export toggle.
Contributor
Author
|
@greptile-apps re-review |
Greptile SummaryThis PR introduces a full password-protected wallet backup and restore system (export, import, and verify flows) for both iOS and Android, backed by a new Rust core with The Rust core is well-designed: cryptographic constants are pinned, all secret material is wrapped in A few issues remain on the platform (iOS/Android) side:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as iOS/Android UI
participant BM as BackupManager (FFI)
participant Export as export::export_all
participant Import as import::import_all
participant Crypto as crypto
participant KC as Keychain
participant DB as Database
Note over UI,DB: Export Flow
UI->>BM: export(password)
BM->>Export: export_all(password)
Export->>KC: get_wallet_key / get_wallet_xpub / get_public_descriptor
Export->>DB: wallets.get_all + global_config.get
Export->>Crypto: compress(json_payload)
Export->>Crypto: encrypt(compressed, password) [Argon2id + XChaCha20Poly1305]
Crypto-->>Export: .covb bytes
Export-->>UI: BackupResult{data, filename, warnings}
UI->>UI: write to temp file + ShareSheet
Note over UI,DB: Import Flow
UI->>BM: verifyBackup(data, password)
BM->>Crypto: decrypt + decompress
Crypto-->>BM: BackupVerifyReport
BM-->>UI: show wallet summary
UI->>BM: importBackup(data, password)
BM->>Import: import_all(data, password)
Import->>Crypto: decrypt + decompress
Import->>DB: check existing fingerprints (dedup)
loop each wallet
Import->>KC: save_wallet_key / save_wallet_xpub / save_public_descriptor
Import->>DB: save_new_wallet_metadata
Import->>DB: import labels (non-fatal)
end
Import->>DB: restore settings (fiat, color scheme, nodes)
Import-->>UI: BackupImportReport
UI->>BM: dispatch(RefreshAfterImport)
Last reviewed commit: 20f3045 |
Comment on lines
+183
to
+187
| let attrs = try url.resourceValues(forKeys: [.fileSizeKey]) | ||
| if let size = attrs.fileSize, size > 50_000_000 { | ||
| throw BackupError.FileTooLarge | ||
| } | ||
|
|
There was a problem hiding this comment.
File read blocks main actor
Data(contentsOf: url) is a synchronous, blocking I/O call and runs on the main actor here (SwiftUI's .fileImporter onCompletion closure is dispatched to the main actor). For files close to the 50 MB limit this can freeze the UI for several seconds and trigger watchdog jank.
Move the I/O to a background task — preserving the security-scoped resource access which must be released on the same thread it was acquired:
case let .success(url):
guard url.startAccessingSecurityScopedResource() else {
errorMessage = "Unable to access the selected file"
return
}
Task {
defer { url.stopAccessingSecurityScopedResource() }
do {
let attrs = try url.resourceValues(forKeys: [.fileSizeKey])
if let size = attrs.fileSize, size > 50_000_000 {
throw BackupError.FileTooLarge
}
let data = try await Task.detached(priority: .userInitiated) {
try Data(contentsOf: url)
}.value
if data.count > 50_000_000 { throw BackupError.FileTooLarge }
try backupManager.validateFormat(data: data)
await MainActor.run {
fileData = data
fileName = url.lastPathComponent
}
} catch {
await MainActor.run {
fileData = nil
fileName = nil
errorMessage = (error as? BackupError)?.description ?? error.localizedDescription
}
}
}
Comment on lines
+135
to
+146
| if isExporting { | ||
| HStack { | ||
| Spacer() | ||
| ProgressView() | ||
| Spacer() | ||
| } | ||
| } else { | ||
| HStack { | ||
| Spacer() | ||
| Text("Export Backup") | ||
| .fontWeight(.semibold) | ||
| Spacer() |
There was a problem hiding this comment.
Generated password not cleared on disappear
password = "" is correctly zeroed in onDisappear, but generatedPassword — which holds the same backup password value (the 12-word BIP39 mnemonic) — is never cleared. If the user navigates away after generating but before exporting, the plaintext backup password remains in memory as generatedPassword.
Suggested change
| if isExporting { | |
| HStack { | |
| Spacer() | |
| ProgressView() | |
| Spacer() | |
| } | |
| } else { | |
| HStack { | |
| Spacer() | |
| Text("Export Backup") | |
| .fontWeight(.semibold) | |
| Spacer() | |
| .onDisappear { | |
| exportTask?.cancel() | |
| password = "" | |
| generatedPassword = nil | |
| if let url = tempFileURL { | |
| do { | |
| try FileManager.default.removeItem(at: url) | |
| } catch { | |
| print("Warning: failed to delete temp backup file: \(error.localizedDescription)") | |
| } | |
| tempFileURL = nil | |
| } | |
| } |
Comment on lines
+264
to
+266
| lines.append("\(report.walletsSkipped) wallet(s) skipped: \(report.skippedWalletNames.joined(separator: ", "))") | ||
| } | ||
| if report.walletsFailed > 0 { |
There was a problem hiding this comment.
Report always shows "0 wallet(s) imported" when all wallets are skipped
lines.append(...) is unconditional. When a backup contains only wallets that already exist on the device (all skipped, none newly imported), the dialog reads "0 wallet(s) imported, N wallet(s) skipped: …", which is confusing — the import was actually successful and the right outcome is to show the skipped count prominently.
This mirrors the same issue flagged on the Android side. Consider conditionally showing the imported count:
Suggested change
| lines.append("\(report.walletsSkipped) wallet(s) skipped: \(report.skippedWalletNames.joined(separator: ", "))") | |
| } | |
| if report.walletsFailed > 0 { | |
| var lines: [String] = [] | |
| if report.walletsImported > 0 { | |
| lines.append("\(report.walletsImported) wallet(s) imported") | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes