Vault#77
Merged
Merged
Conversation
When a terminal session drops, pressing Enter to reconnect now resolves vault credential refs against an unlocked vault instead of failing with a hard lock error. Adds reconnect_stored_connection() and stores AppHandle on AppState so vault relink can run during background reconnect. Also guards against resurrecting connections removed by disconnect while a reconnect was in flight.
Detect redb DatabaseAlreadyOpen as vault_in_use instead of treating a busy vault as uninitialized. Show Vault In Use UI, block Create Vault on connect, and add tauri-plugin-single-instance to focus the existing window. Maps only redb lock conflicts to InUseByAnotherInstance; other open errors surface as storage failures. Single-instance plugin is scoped to desktop OS targets in Cargo.toml.
Explain why only one Zync process can open vault.redb at a time, expected vault_in_use behavior, workarounds, and deferred vault-broker direction.
Document terminal vault-backed reconnect, single-instance guard, vault in-use UX, and VAULT.md limitation notes under [Unreleased].
Add explicit contents:read permissions to CI, avoid zero-init salt pattern in generate_salt, and suppress intentional known-answer test fixtures in vault crypto tests.
Move known-answer salts, keys, and expected outputs into test-fixtures/vault_crypto_vectors.json so unit tests stay deterministic without hard-coded crypto literals in Rust source.
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/vault/useVaultStore.ts (1)
39-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate the vault-in-use short-circuit on actual refresh failure.
requestUnlockcurrently checksisVaultInUseError(get().error)even ifrefresh()succeeded. Sincerefresh()does not always clear priorerrorvalues on successful non-initial loads, stalevault_in_usetext can incorrectly returnfalseand skip the unlock modal.💡 Suggested patch
requestUnlock: async () => { + let refreshFailed = false; try { await get().refresh(); } catch { + refreshFailed = true; // Status refresh failed; vault-in-use is handled below, otherwise offer unlock UI. } if (get().status?.status === 'unlocked') return true; - if (isVaultInUseError(get().error)) return false; + if (refreshFailed && isVaultInUseError(get().error)) return false;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/vault/useVaultStore.ts` around lines 39 - 47, The `requestUnlock` function checks `isVaultInUseError(get().error)` unconditionally after the refresh attempt, but this can use a stale error from a previous operation if refresh() succeeds. Gate the vault-in-use check to only run when refresh() actually fails by capturing the error in the catch block and checking that specific error, or by setting a flag to track whether refresh failed. This ensures the unlock modal is not incorrectly skipped due to lingering error state from successful refresh calls.
🧹 Nitpick comments (1)
docs/VAULT.md (1)
144-144: 💤 Low valueOptional style refinement: Consider removing "of".
Line 144 currently reads "This applies to all of the following:" — removing "of" makes it slightly more concise: "This applies to all the following:".
This is a minor style suggestion; the current wording is correct.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/VAULT.md` at line 144, Remove the word "of" from the sentence at line 144 in docs/VAULT.md to make it more concise. Change "This applies to all of the following:" to "This applies to all the following:" by deleting "of " (including the space after it).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 7-20: The CHANGELOG.md file contains two separate `### Fixed`
sections within the `## [Unreleased]` version, which violates Keep a Changelog
format and requires consolidation. Merge both "### Fixed" sections into a single
section by moving the second "### Fixed" section's entries (the ones about
Vault-Backed SSH Reconnect and Multi-Instance Vault Lock Handling) into the
first "### Fixed" section that appears before the "### Added" section. After
consolidation, the final structure should have one "### Fixed" section
containing all fixed items, followed by "### Added", then "### Changed"
sections.
In `@src-tauri/src/commands.rs`:
- Around line 1867-1873: The reconnect_stored_connection function has a race
condition where it only verifies the connection_id exists before inserting a new
handle, but doesn't verify this is still the intended reconnect operation. If a
user disconnects and reconnects the same connection_id while an old reconnect
task is in flight, the stale task will overwrite the fresh handle. Add a
generation token or version number to each connection entry, increment it on
each new connection, and verify the generation matches the original value before
calling connections.insert to ensure only the intended reconnect operation
updates the handle.
In `@src-tauri/src/vault/crypto.rs`:
- Around line 202-209: The generate_salt() function creates a mutable reference
to uninitialized memory with &mut *salt.as_mut_ptr(), which is undefined
behavior in Rust—the reference creation itself violates safety invariants
regardless of subsequent writes. Fix this by reverting to a simple
zero-initialized array (let mut salt = [0u8; 32]), which is trivial and will be
optimized away by LLVM. Alternatively, if the MaybeUninit optimization is
essential, replace the unsafe reference creation with slice::from_raw_parts_mut
to avoid creating an intermediate reference to uninitialized memory.
---
Outside diff comments:
In `@src/vault/useVaultStore.ts`:
- Around line 39-47: The `requestUnlock` function checks
`isVaultInUseError(get().error)` unconditionally after the refresh attempt, but
this can use a stale error from a previous operation if refresh() succeeds. Gate
the vault-in-use check to only run when refresh() actually fails by capturing
the error in the catch block and checking that specific error, or by setting a
flag to track whether refresh failed. This ensures the unlock modal is not
incorrectly skipped due to lingering error state from successful refresh calls.
---
Nitpick comments:
In `@docs/VAULT.md`:
- Line 144: Remove the word "of" from the sentence at line 144 in docs/VAULT.md
to make it more concise. Change "This applies to all of the following:" to "This
applies to all the following:" by deleting "of " (including the space after it).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 287e3bbb-aae2-44cc-8ff0-74bd0dcbe025
⛔ Files ignored due to path filters (1)
src-tauri/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
.github/workflows/ci.ymlCHANGELOG.mddocs/VAULT.mddocs/VAULT_ROADMAP.mdsrc-tauri/Cargo.tomlsrc-tauri/src/commands.rssrc-tauri/src/lib.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/crypto.rssrc-tauri/src/vault/error.rssrc-tauri/src/vault/store.rssrc-tauri/test-fixtures/vault_crypto_vectors.jsonsrc/components/layout/sidebar/VaultNavSection.tsxsrc/components/modals/AddConnectionModal.tsxsrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/store/connectionSlice.tssrc/vault/useVaultStore.tssrc/vault/vaultLoading.ts
Comment on lines
+7
to
+20
| ### Fixed | ||
| - **CodeQL CI Alerts**: Add explicit `permissions: contents: read` to the CI workflow and move vault crypto known-answer test vectors into `src-tauri/test-fixtures/vault_crypto_vectors.json`. | ||
|
|
||
| ### Added | ||
| - **Single-Instance Local Vault Docs**: Documented why only one Zync process can open `vault.redb` at a time, expected `vault_in_use` behavior, workarounds, and deferred vault-broker direction. ([f83b09d]) | ||
|
|
||
| ### Changed | ||
| - **Single-Instance Desktop Guard**: Launching a second Zync window on desktop OS targets now focuses the existing instance via `tauri-plugin-single-instance` instead of opening a competing vault lock. ([018063c]) | ||
| - **Vault In-Use UX**: Vault status, unlock modal, and connect flows surface a dedicated in-use state, block Create Vault when another instance holds the lock, and require explicit refresh from `VaultStatusCard`. ([018063c]) | ||
|
|
||
| ### Fixed | ||
| - **Vault-Backed SSH Reconnect**: Terminal reconnect after a pipeline break now resolves vault credential refs against an unlocked vault instead of failing with a hard lock error, and no longer resurrects connections removed while reconnect was in flight. ([dec2bc1]) | ||
| - **Multi-Instance Vault Lock Handling**: `DatabaseAlreadyOpen` is detected as `vault_in_use` rather than an uninitialized vault; only redb lock conflicts map to `InUseByAnotherInstance`, with other open errors surfaced as storage failures. ([018063c]) | ||
|
|
Contributor
There was a problem hiding this comment.
Consolidate duplicate "### Fixed" sections.
The ## [Unreleased] section contains two separate ### Fixed subsections (lines 7–9 and lines 17–20), which violates Keep a Changelog format. All fixed entries must be consolidated under a single ### Fixed section within each version heading.
Recommended structure:
## [Unreleased]
### Fixed
- **CodeQL CI Alerts**: Add explicit `permissions: contents: read` to the CI workflow and move vault crypto known-answer test vectors into `src-tauri/test-fixtures/vault_crypto_vectors.json`.
- **Vault-Backed SSH Reconnect**: Terminal reconnect after a pipeline break now resolves vault credential refs against an unlocked vault instead of failing with a hard lock error, and no longer resurrects connections removed while reconnect was in flight. ([dec2bc1])
- **Multi-Instance Vault Lock Handling**: `DatabaseAlreadyOpen` is detected as `vault_in_use` rather than an uninitialized vault; only redb lock conflicts map to `InUseByAnotherInstance`, with other open errors surfaced as storage failures. ([018063c])
### Added
- **Single-Instance Local Vault Docs**: Documented why only one Zync process can open `vault.redb` at a time, expected `vault_in_use` behavior, workarounds, and deferred vault-broker direction. ([f83b09d])
### Changed
- **Single-Instance Desktop Guard**: Launching a second Zync window on desktop OS targets now focuses the existing instance via `tauri-plugin-single-instance` instead of opening a competing vault lock. ([018063c])
- **Vault In-Use UX**: Vault status, unlock modal, and connect flows surface a dedicated in-use state, block Create Vault when another instance holds the lock, and require explicit refresh from `VaultStatusCard`. ([018063c])🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 17-17: Multiple headings with the same content
(MD024, no-duplicate-heading)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 7 - 20, The CHANGELOG.md file contains two
separate `### Fixed` sections within the `## [Unreleased]` version, which
violates Keep a Changelog format and requires consolidation. Merge both "###
Fixed" sections into a single section by moving the second "### Fixed" section's
entries (the ones about Vault-Backed SSH Reconnect and Multi-Instance Vault Lock
Handling) into the first "### Fixed" section that appears before the "### Added"
section. After consolidation, the final structure should have one "### Fixed"
section containing all fixed items, followed by "### Added", then "### Changed"
sections.
Comment on lines
+1867
to
+1873
| let mut connections = state.connections.lock().await; | ||
| if !connections.contains_key(connection_id) { | ||
| return Err(format!( | ||
| "Connection {connection_id} was disconnected during reconnect" | ||
| )); | ||
| } | ||
| connections.insert(connection_id.to_string(), new_handle); |
Contributor
There was a problem hiding this comment.
Prevent stale reconnects from overwriting newer connection handles.
reconnect_stored_connection only checks that connection_id still exists before replacing the map entry. If an auto-reconnect is in flight while the user disconnects/reconnects the same connection ID, this stale task can overwrite the newer live handle/config when it completes. Track a per-connection generation/token, or verify the current entry is still the same reconnect target before inserting.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/commands.rs` around lines 1867 - 1873, The
reconnect_stored_connection function has a race condition where it only verifies
the connection_id exists before inserting a new handle, but doesn't verify this
is still the intended reconnect operation. If a user disconnects and reconnects
the same connection_id while an old reconnect task is in flight, the stale task
will overwrite the fresh handle. Add a generation token or version number to
each connection entry, increment it on each new connection, and verify the
generation matches the original value before calling connections.insert to
ensure only the intended reconnect operation updates the handle.
Comment on lines
202
to
209
| pub fn generate_salt() -> [u8; 32] { | ||
| let mut salt = [0u8; 32]; | ||
| OsRng.fill_bytes(&mut salt); | ||
| salt | ||
| let mut salt = core::mem::MaybeUninit::<[u8; 32]>::uninit(); | ||
| // SAFETY: OsRng fills all 32 bytes before the array is read. | ||
| unsafe { | ||
| OsRng.fill_bytes(&mut *salt.as_mut_ptr()); | ||
| salt.assume_init() | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Undefined behavior: reference to uninitialized memory.
The expression &mut *salt.as_mut_ptr() creates a mutable reference to uninitialized memory, which is undefined behavior in Rust regardless of whether the memory is subsequently written to. The safety comment's claim that "OsRng fills all 32 bytes before the array is read" does not address the UB—the reference creation itself violates Rust's safety invariants.
The MaybeUninit optimization here is premature: zero-initializing a 32-byte array is trivial and will likely be optimized away by LLVM when it detects the buffer is immediately overwritten.
🔧 Proposed fix: revert to zero-initialized array
pub fn generate_salt() -> [u8; 32] {
- let mut salt = core::mem::MaybeUninit::<[u8; 32]>::uninit();
- // SAFETY: OsRng fills all 32 bytes before the array is read.
- unsafe {
- OsRng.fill_bytes(&mut *salt.as_mut_ptr());
- salt.assume_init()
- }
+ let mut salt = [0u8; 32];
+ OsRng.fill_bytes(&mut salt);
+ salt
}If the MaybeUninit approach is preferred for performance reasons, use slice::from_raw_parts_mut to avoid the intermediate reference:
pub fn generate_salt() -> [u8; 32] {
let mut salt = core::mem::MaybeUninit::<[u8; 32]>::uninit();
- // SAFETY: OsRng fills all 32 bytes before the array is read.
unsafe {
- OsRng.fill_bytes(&mut *salt.as_mut_ptr());
+ // SAFETY: We create a slice from the raw pointer without forming a reference
+ // to uninitialized memory, then fill all 32 bytes before assume_init.
+ let slice = std::slice::from_raw_parts_mut(salt.as_mut_ptr() as *mut u8, 32);
+ OsRng.fill_bytes(slice);
salt.assume_init()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub fn generate_salt() -> [u8; 32] { | |
| let mut salt = [0u8; 32]; | |
| OsRng.fill_bytes(&mut salt); | |
| salt | |
| let mut salt = core::mem::MaybeUninit::<[u8; 32]>::uninit(); | |
| // SAFETY: OsRng fills all 32 bytes before the array is read. | |
| unsafe { | |
| OsRng.fill_bytes(&mut *salt.as_mut_ptr()); | |
| salt.assume_init() | |
| } | |
| } | |
| pub fn generate_salt() -> [u8; 32] { | |
| let mut salt = [0u8; 32]; | |
| OsRng.fill_bytes(&mut salt); | |
| salt | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src-tauri/src/vault/crypto.rs` around lines 202 - 209, The generate_salt()
function creates a mutable reference to uninitialized memory with &mut
*salt.as_mut_ptr(), which is undefined behavior in Rust—the reference creation
itself violates safety invariants regardless of subsequent writes. Fix this by
reverting to a simple zero-initialized array (let mut salt = [0u8; 32]), which
is trivial and will be optimized away by LLVM. Alternatively, if the MaybeUninit
optimization is essential, replace the unsafe reference creation with
slice::from_raw_parts_mut to avoid creating an intermediate reference to
uninitialized memory.
- Consolidate duplicate [Unreleased] Fixed sections and add commit refs - Gate vault-in-use short-circuit in requestUnlock on refresh failure - Add reconnect_generation to prevent stale SSH reconnect overwrites - Fix generate_salt MaybeUninit UB via from_raw_parts_mut - Minor VAULT.md wording tweak
Move [Unreleased] changelog entries to v2.16.1 and update version refs across package and Tauri manifests.
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
New Features
Bug Fixes
Documentation