feat(vault): add credential history and harden vault workflows#71
Open
gajendraxdev wants to merge 1 commit into
Open
feat(vault): add credential history and harden vault workflows#71gajendraxdev wants to merge 1 commit into
gajendraxdev wants to merge 1 commit into
Conversation
Add credential revision history and restore support across the vault backend, IPC layer, and vault workspace UI. Modularize vault workspace actions into focused hooks, tighten credential modal validation, improve restore/error handling, and preserve vault credential references across sync/import flows. Update docs, changelog, CI checks, regression tests, and small UI/build-warning polish.
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vault/syncIpc.ts (1)
53-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't discard the
sync_connectpayload.
sync_connectstill returnsSyncProviderStatus, including the connected account email, but this path ignores it and immediately refetches viasync_status().sync_status()currently returnsemail: Nonefor Google, so the UI loses the identity it just obtained after a successful connect.🤖 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/syncIpc.ts` around lines 53 - 63, The connect method is throwing away the payload returned by invoke('sync_connect') which contains the SyncProviderStatus (including Google email); change connect in syncIpc.ts to capture the invoke result (await invoke<SyncProviderStatus>('sync_connect', { provider })) and use that as latestStatus (and pass it to notifySyncStatusChanged and return it) instead of immediately calling syncIpc.status(provider); if you still want a fallback, only call syncIpc.status(provider) when the invoke response is missing required fields and use fallbackStatus(provider, { connected: true }, error) on errors as currently done.
🧹 Nitpick comments (3)
src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts (2)
44-48: ⚡ Quick winSimplify error message extraction.
The error message extraction uses an overly complex type assertion pattern that can be simplified.
♻️ Suggested simplification
} catch (error: unknown) { console.warn('[Vault] Failed to load item for rotation:', error); - const msg = String((error as { message?: unknown } | null | undefined)?.message ?? error); + const msg = error instanceof Error ? error.message : String(error); showToast('error', `Failed to load vault item notes: ${msg}`); }This pattern is clearer and handles the common case of
Errorinstances more idiomatically.🤖 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/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts` around lines 44 - 48, In the catch block inside useRotateCredentialModal, replace the complex type assertion used to build the error text with a simpler, idiomatic check (e.g., use instanceof Error to get error.message, otherwise String(error)); update the const msg assignment and leave the existing console.warn and showToast calls untouched so showToast('error', `Failed to load vault item notes: ${msg}`) receives the simplified message.
103-108: ⚡ Quick winSimplify error message extraction (duplicate pattern).
Same complex type assertion pattern as in the
open()method.♻️ Suggested simplification
} catch (e: unknown) { - const msg = - e && typeof e === 'object' && 'message' in e - ? String((e as { message: unknown }).message) - : String(e); + const msg = e instanceof Error ? e.message : String(e); showToast('error', `Failed to rotate credential: ${msg}`); } finally {🤖 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/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts` around lines 103 - 108, The catch block in useRotateCredentialModal repeats the complex error-extraction logic (see the local variable msg and the showToast call), duplicating the same pattern used in open(); refactor by extracting that logic into a single helper (e.g., extractErrorMessage) either in this file or imported from a shared util, then replace the inline type-assertion block in the rotate handler and the open() method with a call to extractErrorMessage(error) and pass its result into showToast('error', `Failed to rotate credential: ${msg}`) (or the corresponding open() toast) to remove duplication and centralize error parsing.src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts (1)
64-82: ⚡ Quick winConsider simplifying error message extraction.
The error message extraction pattern can be simplified for better readability.
♻️ Suggested simplification
} catch (e: unknown) { - const msg = String((e as { message?: unknown } | null | undefined)?.message ?? e); + const msg = e instanceof Error ? e.message : String(e); showToast('error', `Failed to add credential: ${msg}`); } finally {This pattern is more idiomatic and handles the common
Errorinstance case clearly.🤖 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/components/settings/tabs/vault/hooks/useAddCredentialModal.ts` around lines 64 - 82, The current catch block in useAddCredentialModal's create flow extracts the error message via a verbose cast; simplify it by replacing the msg computation in the catch of the async create handler with an idiomatic check like using "e instanceof Error ? e.message : String(e)" so the showToast('error', ...) receives a clear string; keep the surrounding setIsCreating, vaultIpc.itemCreate, onCreated, reset, and setIsOpen logic unchanged.
🤖 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 `@src-tauri/src/vault/secure_to_vault.rs`:
- Around line 133-145: The secret format varies based on conn.password which
breaks deduplication because derive_secret_fingerprint currently fingerprints
the whole secret string; fix by normalizing secrets and fingerprinting: always
serialize secrets as JSON with explicit "key" and "passphrase" fields when
creating secret in the PreparedSecureItem (change the secret construction around
key_content and conn.password in secure_to_vault.rs), update
derive_secret_fingerprint (crypto.rs) to compute the fingerprint from the
canonical "key" value (or parse legacy raw secrets into the canonical JSON
shape), and update the deduplication lookup logic that uses the fingerprint to
accept both legacy raw and new JSON forms by normalizing stored items before
comparison; touch the secret creation site, derive_secret_fingerprint function,
and deduplication lookup to implement this consistent normalization/migration
path.
In `@src/components/dashboard/WelcomeScreen.tsx`:
- Line 324: The Tailwind class bg-size-[18px_18px] is non-standard; in the
WelcomeScreen component update the className on the absolute background element
to use Tailwind arbitrary property syntax for CSS properties (replace
bg-size-[18px_18px] with [background-size:18px_18px]) so the background-size is
applied correctly; keep the rest of the classes (absolute inset-0 -z-10
bg-app-bg
bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)]
opacity-40) unchanged.
In `@src/components/settings/tabs/vault/hooks/useAssignCredentialModal.ts`:
- Around line 71-76: The modal never closes because close() returns early while
isAssigning is true; either clear isAssigning before calling close or let close
bypass the guard when intended. Edit submit() (and the other call sites at lines
~122-124) so they call setIsAssigning(false) before invoking close(), or modify
close(itemId?, {force}) / remove the early return so close() will still clear
state (setItemId(null), setSearch(''), setSelectedConnectionIds([])) when a
successful submit finishes; reference the close, submit, isAssigning,
setIsAssigning, setItemId and setSelectedConnectionIds symbols to locate and
update the logic.
In `@src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts`:
- Line 66: The code sets baseSecret using the untrimmed secret for non-SSH keys
causing saved credentials to include whitespace; update the assignment in
useRotateCredentialModal (baseSecret) to use trimmedSecret for all kinds
(instead of secret) so secretToSave uses the already-trimmed value; ensure this
change affects the logic where secretToSave is derived (including the branch
that checks for passphrase) so non-SSH credentials are trimmed before saving.
In `@src/components/settings/tabs/vault/hooks/useVaultPanelActions.ts`:
- Around line 96-98: The mutating actions call only onRefresh(), which updates
vault status but not the item list; update the code paths (including the block
around useVaultPanelActions.ts lines where onLoadConnections(), onRefresh(),
loadSecurePreview() are called and the similar block at the other location
referenced) to call the item-reload routine after item mutations—either call
onLoadConnections() (or the specific item reload function used elsewhere) after
onRefresh(), or replace onRefresh() with a call sequence that ensures both
status and items are reloaded so the UI reflects secure/delete changes
immediately.
- Around line 461-473: The code marks a vault as backfilled before the IPC call
completes, preventing retries if backfill fails; move the
backfilledVaultIdsRef.current.add(vaultId) call so it only runs after
vaultIpc.backfillConnectionRefs() succeeds (e.g., after checking result.updated
> 0 or after a successful try block), and keep the early-return check (if
(backfilledVaultIdsRef.current.has(vaultId)) return;) intact; update the logic
in useVaultPanelActions around backfilledVaultIdsRef,
vaultIpc.backfillConnectionRefs, onLoadConnections, and the showToast call so
the vault ID is only added when the backfill completes without throwing.
- Around line 133-152: The toast incorrectly claims all vault-backed sessions
were disconnected even when some onDisconnectConnection calls failed; use the
Promise.allSettled results from disconnectResults to compute how many succeeded
(count result.status === 'fulfilled' for entries corresponding to
vaultBackedIds) and use that count in the showToast message instead of
vaultBackedConnectionIds.size, and if any were rejected, add a separate
warning/error toast (or include in the message) indicating partial failure and
referencing the failed vaultBackedIds by index from disconnectResults so the
user knows some sessions remain connected; keep the existing await onLocked()
call sequence (call onLocked before showing toasts) and update messages
accordingly.
In `@src/components/vault/VaultUnlockModal.tsx`:
- Around line 8-9: The frontend-only 12-character passphrase check
(PASSPHRASE_MIN_LENGTH in VaultUnlockModal.tsx) must be mirrored in the backend:
update VaultService::initialize() to import/use the same PASSPHRASE_MIN_LENGTH
constant and validate the provided passphrase length, returning/throwing a clear
error (e.g., InvalidPassphraseLength) when passphrase.length <
PASSPHRASE_MIN_LENGTH so IPC callers cannot bypass the rule; update any
callers/tests to handle the new error path and ensure the error propagates back
over IPC for user-facing display.
In `@src/lib/tauri-ipc.ts`:
- Line 174: The new channel mapping 'ssh:disconnectVaultBacked' ->
'ssh_disconnect_vault_backed' must be treated as a no-arg command so it
serializes to an empty payload instead of { args: [] }; update the payload
builder (where channel keys are translated to payloads) to add an explicit
branch for 'ssh_disconnect_vault_backed' that returns {} (empty object) rather
than the default args array handling so command deserialization succeeds.
In `@src/store/connectionSlice.ts`:
- Around line 694-696: The current assignment lets non-vault tabs keep a valid
vaultProfileId; change the logic so you first check s.tabType === 'vault' and
only then validate/or default the id: for example set vaultProfileId to
undefined for any non-'vault' tab, and for 'vault' tabs use
isVaultProfileId(s.vaultProfileId) ? s.vaultProfileId :
DEFAULT_VAULT_PROFILE_ID; update the expression containing vaultProfileId,
isVaultProfileId, s.tabType and DEFAULT_VAULT_PROFILE_ID accordingly.
---
Outside diff comments:
In `@src/vault/syncIpc.ts`:
- Around line 53-63: The connect method is throwing away the payload returned by
invoke('sync_connect') which contains the SyncProviderStatus (including Google
email); change connect in syncIpc.ts to capture the invoke result (await
invoke<SyncProviderStatus>('sync_connect', { provider })) and use that as
latestStatus (and pass it to notifySyncStatusChanged and return it) instead of
immediately calling syncIpc.status(provider); if you still want a fallback, only
call syncIpc.status(provider) when the invoke response is missing required
fields and use fallbackStatus(provider, { connected: true }, error) on errors as
currently done.
---
Nitpick comments:
In `@src/components/settings/tabs/vault/hooks/useAddCredentialModal.ts`:
- Around line 64-82: The current catch block in useAddCredentialModal's create
flow extracts the error message via a verbose cast; simplify it by replacing the
msg computation in the catch of the async create handler with an idiomatic check
like using "e instanceof Error ? e.message : String(e)" so the
showToast('error', ...) receives a clear string; keep the surrounding
setIsCreating, vaultIpc.itemCreate, onCreated, reset, and setIsOpen logic
unchanged.
In `@src/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts`:
- Around line 44-48: In the catch block inside useRotateCredentialModal, replace
the complex type assertion used to build the error text with a simpler,
idiomatic check (e.g., use instanceof Error to get error.message, otherwise
String(error)); update the const msg assignment and leave the existing
console.warn and showToast calls untouched so showToast('error', `Failed to load
vault item notes: ${msg}`) receives the simplified message.
- Around line 103-108: The catch block in useRotateCredentialModal repeats the
complex error-extraction logic (see the local variable msg and the showToast
call), duplicating the same pattern used in open(); refactor by extracting that
logic into a single helper (e.g., extractErrorMessage) either in this file or
imported from a shared util, then replace the inline type-assertion block in the
rotate handler and the open() method with a call to extractErrorMessage(error)
and pass its result into showToast('error', `Failed to rotate credential:
${msg}`) (or the corresponding open() toast) to remove duplication and
centralize error parsing.
🪄 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: e53d9265-a4f6-4eac-a4ba-78937fcb91ed
📒 Files selected for processing (38)
.github/workflows/ci.ymlCHANGELOG.mddocs/VAULT_AND_SYNC_ARCHITECTURE.mdsrc-tauri/build.rssrc-tauri/src/lib.rssrc-tauri/src/sync/commands.rssrc-tauri/src/vault/commands.rssrc-tauri/src/vault/schema.rssrc-tauri/src/vault/secure_to_vault.rssrc-tauri/src/vault/store.rssrc-tauri/src/vault/types.rssrc/components/dashboard/WelcomeScreen.tsxsrc/components/layout/Sidebar.tsxsrc/components/layout/sidebar/VaultNavSection.tsxsrc/components/modals/AddTunnelModal.tsxsrc/components/modals/useAutoVault.tssrc/components/settings/tabs/VaultTab.tsxsrc/components/settings/tabs/vault/CredentialHistoryModal.tsxsrc/components/settings/tabs/vault/VaultItemsPanel.tsxsrc/components/settings/tabs/vault/VaultStatusCard.tsxsrc/components/settings/tabs/vault/hooks/useAddCredentialModal.tssrc/components/settings/tabs/vault/hooks/useAssignCredentialModal.tssrc/components/settings/tabs/vault/hooks/useHistoryModal.tssrc/components/settings/tabs/vault/hooks/useRotateCredentialModal.tssrc/components/settings/tabs/vault/hooks/useVaultPanelActions.tssrc/components/ui/Modal.tsxsrc/components/vault/RecoveryKeyModal.tsxsrc/components/vault/VaultUnlockModal.tsxsrc/components/vault/VaultWorkspacePanel.tsxsrc/features/connections/domain/merge.tssrc/features/connections/infrastructure/connectionIpc.tssrc/lib/tauri-ipc.tssrc/store/connectionSlice.tssrc/vault/ipc.tssrc/vault/syncIpc.tssrc/vault/useVaultStore.tstests/sessionPersistence.test.mjstests/vaultNavState.test.mjs
Comment on lines
+133
to
145
| let secret = conn.password | ||
| .as_deref() | ||
| .filter(|p| !p.is_empty()) | ||
| .map(|passphrase| { | ||
| serde_json::json!({ "key": key_content, "passphrase": passphrase }).to_string() | ||
| }) | ||
| .unwrap_or(key_content); | ||
| prepared.push(PreparedSecureItem { | ||
| index, | ||
| label, | ||
| kind: CredentialItemKind::SshPrivateKey, | ||
| secret: serde_json::json!({ | ||
| "key": key_content, | ||
| "passphrase": conn.password.as_ref(), | ||
| }) | ||
| .to_string(), | ||
| secret, | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for code that deserializes/reads SSH private key secrets from vault
rg -nP --type=rust -C5 'SshPrivateKey.*secret|secret.*SshPrivateKey' \
-g '!**/secure_to_vault.rs'Repository: zync-sh/zync
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find fingerprinting implementation
ast-grep --pattern $'fn secret_fingerprint($$$) {
$$$
}'Repository: zync-sh/zync
Length of output: 38
🏁 Script executed:
# Search for SshPrivateKey usage more broadly
rg -n "SshPrivateKey" --type=rustRepository: zync-sh/zync
Length of output: 534
🏁 Script executed:
# Search for fingerprinting/deduplication logic
rg -n "fingerprint|deduplicate" --type=rust -iRepository: zync-sh/zync
Length of output: 3327
🏁 Script executed:
# Look at the context around lines 206-207 in secure_to_vault.rs
sed -n '200,220p' src-tauri/src/vault/secure_to_vault.rsRepository: zync-sh/zync
Length of output: 901
🏁 Script executed:
# Check the full structure of secure_to_vault.rs to understand the change
wc -l src-tauri/src/vault/secure_to_vault.rsRepository: zync-sh/zync
Length of output: 99
🏁 Script executed:
# Look at the match statement for SshPrivateKey at line 251
sed -n '245,280p' src-tauri/src/vault/secure_to_vault.rsRepository: zync-sh/zync
Length of output: 1170
🏁 Script executed:
# Look at the full context of lines 133-145
sed -n '125,155p' src-tauri/src/vault/secure_to_vault.rsRepository: zync-sh/zync
Length of output: 1213
🏁 Script executed:
# Check the derive_secret_fingerprint implementation
sed -n '141,160p' src-tauri/src/vault/crypto.rsRepository: zync-sh/zync
Length of output: 870
🏁 Script executed:
# Look at the fingerprinting logic in store.rs to understand what it fingerprints
sed -n '255,275p' src-tauri/src/vault/store.rsRepository: zync-sh/zync
Length of output: 866
🏁 Script executed:
# Find where vault items are retrieved/read
rg -n "item_get|read_record|retrieve.*secret" --type=rust src-tauri/src/vault/Repository: zync-sh/zync
Length of output: 1187
🏁 Script executed:
# Search for where credentials are retrieved for use
rg -n "get_credential|fetch.*secret" --type=rust src-tauri/src/Repository: zync-sh/zync
Length of output: 38
🏁 Script executed:
# Check git history to see what format was used before this change
git log -p --follow -S 'passphrase' -- src-tauri/src/vault/secure_to_vault.rs | head -200Repository: zync-sh/zync
Length of output: 38
🏁 Script executed:
# Look for any test or documentation about secret format
rg -n "SshPrivateKey|passphrase.*secret|secret.*passphrase" --type=rust src-tauri/src/vault/ -B2 -A2Repository: zync-sh/zync
Length of output: 2056
🏁 Script executed:
# Look at vault_item_get command to see what happens with the secret
sed -n '151,175p' src-tauri/src/vault/commands.rsRepository: zync-sh/zync
Length of output: 591
🏁 Script executed:
# Search for where SSH credentials are applied/used
rg -n "SshAuth|private_key_path|apply.*credential|use.*secret" --type=rust src-tauri/src/ -A3 -B3Repository: zync-sh/zync
Length of output: 9302
🏁 Script executed:
# Look for any logic that parses JSON from secret or handles multiple formats
rg -n "from_str|parse.*secret|serde_json" --type=rust src-tauri/src/vault/ | head -20Repository: zync-sh/zync
Length of output: 2296
🏁 Script executed:
# Check if there are any TODOs, FIXMEs, or comments about format handling
rg -n "TODO|FIXME|backward|compat|format" --type=rust src-tauri/src/vault/secure_to_vault.rs -iRepository: zync-sh/zync
Length of output: 1258
🏁 Script executed:
# Search for code that uses vault credentials, especially SshAuth purpose
rg -n "CredentialPurpose|SshAuth|apply.*credential|use_credential" --type=rust src-tauri/src/ -A5Repository: zync-sh/zync
Length of output: 1703
🏁 Script executed:
# Look for any handlers that might retrieve and use SSH secrets
rg -n "secret_fingerprint|item_get.*SshPrivateKey|retrieve.*credential" --type=rust src-tauri/src/ -B2 -A8Repository: zync-sh/zync
Length of output: 10787
🏁 Script executed:
# Check the Credential struct to see how secrets are passed along
rg -n "struct Credential|pub struct.*Credential" --type=rust src-tauri/src/ -A10Repository: zync-sh/zync
Length of output: 759
🏁 Script executed:
# Search for any recent changes or comments about the secret format
git log --oneline -20 -- src-tauri/src/vault/secure_to_vault.rsRepository: zync-sh/zync
Length of output: 127
Address fingerprinting and deduplication issues with conditional secret format.
The code stores SSH private key secrets in different formats based on passphrase presence:
- With passphrase: JSON
{"key": "...", "passphrase": "..."} - Without passphrase: raw key string
This causes backward compatibility problems:
-
Fingerprinting breaks deduplication: Since
derive_secret_fingerprint(crypto.rs:142) fingerprints the entire secret string, the same key stored before/after this change will produce different fingerprints. The deduplication lookup at lines 206-207 uses exact fingerprint match, so existing items won't be recognized as duplicates. -
No migration path: Existing vault items stored in raw format won't match new items with the same key stored in JSON format during deduplication.
To preserve backward compatibility, consider:
- Normalizing the format (always JSON, or always raw + separate passphrase field)
- Updating deduplication logic to handle both formats
- Providing migration for existing items
🤖 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/secure_to_vault.rs` around lines 133 - 145, The secret
format varies based on conn.password which breaks deduplication because
derive_secret_fingerprint currently fingerprints the whole secret string; fix by
normalizing secrets and fingerprinting: always serialize secrets as JSON with
explicit "key" and "passphrase" fields when creating secret in the
PreparedSecureItem (change the secret construction around key_content and
conn.password in secure_to_vault.rs), update derive_secret_fingerprint
(crypto.rs) to compute the fingerprint from the canonical "key" value (or parse
legacy raw secrets into the canonical JSON shape), and update the deduplication
lookup logic that uses the fingerprint to accept both legacy raw and new JSON
forms by normalizing stored items before comparison; touch the secret creation
site, derive_secret_fingerprint function, and deduplication lookup to implement
this consistent normalization/migration path.
| <div | ||
| aria-hidden="true" | ||
| className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(var(--color-app-border)_1px,transparent_1px)] bg-size-[18px_18px] mask-[radial-gradient(ellipse_70%_60%_at_50%_50%,black_30%,transparent_100%)] opacity-40" | ||
| className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)] bg-size-[18px_18px] opacity-40" |
Contributor
There was a problem hiding this comment.
Use standard arbitrary property syntax for background-size.
The bg-size-[18px_18px] syntax is non-standard. Tailwind doesn't provide a bg-size utility, so this may not render correctly.
🔧 Proposed fix using arbitrary property syntax
- className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)] bg-size-[18px_18px] opacity-40"
+ className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)] [background-size:18px_18px] opacity-40"📝 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
| className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)] bg-size-[18px_18px] opacity-40" | |
| className="absolute inset-0 -z-10 bg-app-bg bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)] [background-size:18px_18px] opacity-40" |
🤖 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/components/dashboard/WelcomeScreen.tsx` at line 324, The Tailwind class
bg-size-[18px_18px] is non-standard; in the WelcomeScreen component update the
className on the absolute background element to use Tailwind arbitrary property
syntax for CSS properties (replace bg-size-[18px_18px] with
[background-size:18px_18px]) so the background-size is applied correctly; keep
the rest of the classes (absolute inset-0 -z-10 bg-app-bg
bg-[radial-gradient(circle_at_center,var(--color-app-border)_1px,transparent_1px)]
opacity-40) unchanged.
Comment on lines
+71
to
+76
| const close = () => { | ||
| if (isAssigning) return; | ||
| setItemId(null); | ||
| setSearch(''); | ||
| setSelectedConnectionIds([]); | ||
| }; |
Contributor
There was a problem hiding this comment.
Successful submits never close the modal.
submit() calls close() while isAssigning is still true, and close() returns early in that state. The assignment is saved, but itemId stays set so the modal remains open and stale selections stick around.
Also applies to: 122-124
🤖 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/components/settings/tabs/vault/hooks/useAssignCredentialModal.ts` around
lines 71 - 76, The modal never closes because close() returns early while
isAssigning is true; either clear isAssigning before calling close or let close
bypass the guard when intended. Edit submit() (and the other call sites at lines
~122-124) so they call setIsAssigning(false) before invoking close(), or modify
close(itemId?, {force}) / remove the early return so close() will still clear
state (setItemId(null), setSearch(''), setSelectedConnectionIds([])) when a
successful submit finishes; reference the close, submit, isAssigning,
setIsAssigning, setItemId and setSelectedConnectionIds symbols to locate and
update the logic.
|
|
||
| const trimmedLabel = label.trim(); | ||
| const trimmedSecret = secret.trim(); | ||
| const baseSecret = item.kind === 'ssh-private-key' ? trimmedSecret : secret; |
Contributor
There was a problem hiding this comment.
Fix incorrect secret trimming for non-SSH-key types.
Line 66 assigns secret (untrimmed) to baseSecret for non-SSH-key kinds, but this untrimmed value is then used as secretToSave when there's no passphrase (line 81). This means SSH passwords and other credential types won't be trimmed before saving, potentially including unwanted whitespace.
🔧 Proposed fix
- const baseSecret = item.kind === 'ssh-private-key' ? trimmedSecret : secret;
+ const baseSecret = trimmedSecret;Since trimmedSecret is already computed on line 65, use it for all credential types.
📝 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
| const baseSecret = item.kind === 'ssh-private-key' ? trimmedSecret : secret; | |
| const baseSecret = trimmedSecret; |
🤖 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/components/settings/tabs/vault/hooks/useRotateCredentialModal.ts` at line
66, The code sets baseSecret using the untrimmed secret for non-SSH keys causing
saved credentials to include whitespace; update the assignment in
useRotateCredentialModal (baseSecret) to use trimmedSecret for all kinds
(instead of secret) so secretToSave uses the already-trimmed value; ensure this
change affects the logic where secretToSave is derived (including the branch
that checks for passphrase) so non-SSH credentials are trimmed before saving.
Comment on lines
+96
to
+98
| await onLoadConnections(); | ||
| await onRefresh(); | ||
| await loadSecurePreview(); |
Contributor
There was a problem hiding this comment.
Refresh vault items after item-mutating actions, not only vault status.
Both paths mutate the vault's item set, but they only call onRefresh(). In VaultTab, onRefresh is the status refresh path; it does not re-run the item reload effect while the vault stays unlocked, so the list can remain stale after securing credentials or deleting an item.
Proposed fix
- await onLoadConnections();
- await onRefresh();
+ await onLoadConnections();
+ await Promise.all([onRefresh(), onRefreshItems()]);
await loadSecurePreview();
@@
- await vaultIpc.itemDelete(itemId);
- await onRefresh();
+ await vaultIpc.itemDelete(itemId);
+ await onRefreshItems();
showToast('success', `Deleted "${label}".`);Also applies to: 374-376
🤖 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/components/settings/tabs/vault/hooks/useVaultPanelActions.ts` around
lines 96 - 98, The mutating actions call only onRefresh(), which updates vault
status but not the item list; update the code paths (including the block around
useVaultPanelActions.ts lines where onLoadConnections(), onRefresh(),
loadSecurePreview() are called and the similar block at the other location
referenced) to call the item-reload routine after item mutations—either call
onLoadConnections() (or the specific item reload function used elsewhere) after
onRefresh(), or replace onRefresh() with a call sequence that ensures both
status and items are reloaded so the UI reflects secure/delete changes
immediately.
Comment on lines
+133
to
+152
| const disconnectResults = await Promise.allSettled( | ||
| vaultBackedIds.map(id => onDisconnectConnection(id)), | ||
| ); | ||
| disconnectResults.forEach((result, index) => { | ||
| if (result.status === 'rejected') { | ||
| console.error( | ||
| '[Vault] Failed to disconnect vault-backed connection:', | ||
| vaultBackedIds[index], | ||
| result.reason, | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| await onLocked(); | ||
| showToast('info', 'Vault locked.'); | ||
| if (vaultBackedConnectionIds.size > 0) { | ||
| showToast( | ||
| 'info', | ||
| `Disconnected ${vaultBackedConnectionIds.size} vault-backed connection${vaultBackedConnectionIds.size > 1 ? 's' : ''} for security.`, | ||
| ); |
Contributor
There was a problem hiding this comment.
Don't report all vault-backed sessions as disconnected after partial failure.
Rejected disconnects are only logged, but the toast on Line 148 still reports vaultBackedConnectionIds.size as if every session was closed. In this security-sensitive flow, that can tell the user everything was disconnected when some sessions were not.
Proposed fix
const disconnectResults = await Promise.allSettled(
vaultBackedIds.map(id => onDisconnectConnection(id)),
);
+ const failedDisconnects = disconnectResults.filter(
+ result => result.status === 'rejected',
+ ).length;
+ const succeededDisconnects = vaultBackedIds.length - failedDisconnects;
+
disconnectResults.forEach((result, index) => {
if (result.status === 'rejected') {
console.error(
'[Vault] Failed to disconnect vault-backed connection:',
vaultBackedIds[index],
@@
await onLocked();
showToast('info', 'Vault locked.');
- if (vaultBackedConnectionIds.size > 0) {
+ if (succeededDisconnects > 0) {
showToast(
'info',
- `Disconnected ${vaultBackedConnectionIds.size} vault-backed connection${vaultBackedConnectionIds.size > 1 ? 's' : ''} for security.`,
+ `Disconnected ${succeededDisconnects} vault-backed connection${succeededDisconnects > 1 ? 's' : ''} for security.`,
);
}
+ if (failedDisconnects > 0) {
+ showToast(
+ 'error',
+ `${failedDisconnects} vault-backed connection${failedDisconnects > 1 ? 's could not be' : ' could not be'} disconnected.`,
+ );
+ }📝 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
| const disconnectResults = await Promise.allSettled( | |
| vaultBackedIds.map(id => onDisconnectConnection(id)), | |
| ); | |
| disconnectResults.forEach((result, index) => { | |
| if (result.status === 'rejected') { | |
| console.error( | |
| '[Vault] Failed to disconnect vault-backed connection:', | |
| vaultBackedIds[index], | |
| result.reason, | |
| ); | |
| } | |
| }); | |
| await onLocked(); | |
| showToast('info', 'Vault locked.'); | |
| if (vaultBackedConnectionIds.size > 0) { | |
| showToast( | |
| 'info', | |
| `Disconnected ${vaultBackedConnectionIds.size} vault-backed connection${vaultBackedConnectionIds.size > 1 ? 's' : ''} for security.`, | |
| ); | |
| const disconnectResults = await Promise.allSettled( | |
| vaultBackedIds.map(id => onDisconnectConnection(id)), | |
| ); | |
| const failedDisconnects = disconnectResults.filter( | |
| result => result.status === 'rejected', | |
| ).length; | |
| const succeededDisconnects = vaultBackedIds.length - failedDisconnects; | |
| disconnectResults.forEach((result, index) => { | |
| if (result.status === 'rejected') { | |
| console.error( | |
| '[Vault] Failed to disconnect vault-backed connection:', | |
| vaultBackedIds[index], | |
| result.reason, | |
| ); | |
| } | |
| }); | |
| await onLocked(); | |
| showToast('info', 'Vault locked.'); | |
| if (succeededDisconnects > 0) { | |
| showToast( | |
| 'info', | |
| `Disconnected ${succeededDisconnects} vault-backed connection${succeededDisconnects > 1 ? 's' : ''} for security.`, | |
| ); | |
| } | |
| if (failedDisconnects > 0) { | |
| showToast( | |
| 'error', | |
| `${failedDisconnects} vault-backed connection${failedDisconnects > 1 ? 's could not be' : ' could not be'} disconnected.`, | |
| ); | |
| } |
🤖 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/components/settings/tabs/vault/hooks/useVaultPanelActions.ts` around
lines 133 - 152, The toast incorrectly claims all vault-backed sessions were
disconnected even when some onDisconnectConnection calls failed; use the
Promise.allSettled results from disconnectResults to compute how many succeeded
(count result.status === 'fulfilled' for entries corresponding to
vaultBackedIds) and use that count in the showToast message instead of
vaultBackedConnectionIds.size, and if any were rejected, add a separate
warning/error toast (or include in the message) indicating partial failure and
referencing the failed vaultBackedIds by index from disconnectResults so the
user knows some sessions remain connected; keep the existing await onLocked()
call sequence (call onLocked before showing toasts) and update messages
accordingly.
Comment on lines
+461
to
+473
| if (backfilledVaultIdsRef.current.has(vaultId)) return; | ||
| backfilledVaultIdsRef.current.add(vaultId); | ||
| try { | ||
| const result = await vaultIpc.backfillConnectionRefs(); | ||
| if (result.updated > 0) { | ||
| await onLoadConnections(); | ||
| showToast( | ||
| 'info', | ||
| `Backfilled stable credential IDs for ${result.updated} connection${result.updated > 1 ? 's' : ''}.`, | ||
| ); | ||
| } | ||
| } catch (error) { | ||
| console.warn('[Vault] Failed to backfill credential ids:', error); |
Contributor
There was a problem hiding this comment.
Only mark a vault as backfilled after the backfill succeeds.
backfilledVaultIdsRef.current.add(vaultId) runs before the IPC call. If backfillConnectionRefs() fails once, later unlocks of the same vault in this mounted tab will never retry, so stale refs persist until the user manually repairs them or remounts the view.
Proposed fix
async (vaultId: string) => {
if (backfilledVaultIdsRef.current.has(vaultId)) return;
- backfilledVaultIdsRef.current.add(vaultId);
try {
const result = await vaultIpc.backfillConnectionRefs();
+ backfilledVaultIdsRef.current.add(vaultId);
if (result.updated > 0) {
await onLoadConnections();
showToast(
'info',
`Backfilled stable credential IDs for ${result.updated} connection${result.updated > 1 ? 's' : ''}.`,📝 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
| if (backfilledVaultIdsRef.current.has(vaultId)) return; | |
| backfilledVaultIdsRef.current.add(vaultId); | |
| try { | |
| const result = await vaultIpc.backfillConnectionRefs(); | |
| if (result.updated > 0) { | |
| await onLoadConnections(); | |
| showToast( | |
| 'info', | |
| `Backfilled stable credential IDs for ${result.updated} connection${result.updated > 1 ? 's' : ''}.`, | |
| ); | |
| } | |
| } catch (error) { | |
| console.warn('[Vault] Failed to backfill credential ids:', error); | |
| if (backfilledVaultIdsRef.current.has(vaultId)) return; | |
| try { | |
| const result = await vaultIpc.backfillConnectionRefs(); | |
| backfilledVaultIdsRef.current.add(vaultId); | |
| if (result.updated > 0) { | |
| await onLoadConnections(); | |
| showToast( | |
| 'info', | |
| `Backfilled stable credential IDs for ${result.updated} connection${result.updated > 1 ? 's' : ''}.`, | |
| ); | |
| } | |
| } catch (error) { | |
| console.warn('[Vault] Failed to backfill credential ids:', error); |
🤖 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/components/settings/tabs/vault/hooks/useVaultPanelActions.ts` around
lines 461 - 473, The code marks a vault as backfilled before the IPC call
completes, preventing retries if backfill fails; move the
backfilledVaultIdsRef.current.add(vaultId) call so it only runs after
vaultIpc.backfillConnectionRefs() succeeds (e.g., after checking result.updated
> 0 or after a successful try block), and keep the early-return check (if
(backfilledVaultIdsRef.current.has(vaultId)) return;) intact; update the logic
in useVaultPanelActions around backfilledVaultIdsRef,
vaultIpc.backfillConnectionRefs, onLoadConnections, and the showToast call so
the vault ID is only added when the backfill completes without throwing.
Comment on lines
+8
to
+9
| /** Minimum passphrase length enforced at vault creation time. */ | ||
| export const PASSPHRASE_MIN_LENGTH = 12; |
Contributor
There was a problem hiding this comment.
Enforce the passphrase floor in the backend too.
This 12-character rule only exists in the modal right now; VaultService::initialize() still accepts shorter passphrases. Any non-UI IPC caller can bypass the guarantee this comment implies.
Also applies to: 54-57
🤖 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/components/vault/VaultUnlockModal.tsx` around lines 8 - 9, The
frontend-only 12-character passphrase check (PASSPHRASE_MIN_LENGTH in
VaultUnlockModal.tsx) must be mirrored in the backend: update
VaultService::initialize() to import/use the same PASSPHRASE_MIN_LENGTH constant
and validate the provided passphrase length, returning/throwing a clear error
(e.g., InvalidPassphraseLength) when passphrase.length < PASSPHRASE_MIN_LENGTH
so IPC callers cannot bypass the rule; update any callers/tests to handle the
new error path and ensure the error propagates back over IPC for user-facing
display.
| 'ssh:importConfigBySource': 'ssh_import_config_by_source', | ||
| 'ssh:readConfig': 'ssh_import_config', | ||
| 'ssh:internalize-connections': 'ssh_internalize_connections', | ||
| 'ssh:disconnectVaultBacked': 'ssh_disconnect_vault_backed', |
Contributor
There was a problem hiding this comment.
Handle ssh_disconnect_vault_backed as a no-arg command payload.
This new channel currently falls through to the default payload builder and sends { args: [] } for a no-arg call, which can break command deserialization. Add explicit payload mapping to {} for this command.
Proposed fix
} else if (tauriCommand === 'ssh_migrate_all_keys') {
payload = {};
+ } else if (tauriCommand === 'ssh_disconnect_vault_backed') {
+ payload = {};
} else if (tauriCommand === 'connections_export_to_file' || tauriCommand === 'connections_import_from_file') {
payload = { request: args[0] };
}🤖 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/lib/tauri-ipc.ts` at line 174, The new channel mapping
'ssh:disconnectVaultBacked' -> 'ssh_disconnect_vault_backed' must be treated as
a no-arg command so it serializes to an empty payload instead of { args: [] };
update the payload builder (where channel keys are translated to payloads) to
add an explicit branch for 'ssh_disconnect_vault_backed' that returns {} (empty
object) rather than the default args array handling so command deserialization
succeeds.
Comment on lines
694
to
+696
| vaultProfileId: isVaultProfileId(s.vaultProfileId) | ||
| ? s.vaultProfileId | ||
| : undefined, | ||
| : s.tabType === 'vault' ? DEFAULT_VAULT_PROFILE_ID : undefined, |
Contributor
There was a problem hiding this comment.
Gate vaultProfileId by tab type before validation.
Line 694 currently preserves a valid vaultProfileId even for non-vault tabs. That allows non-vault snapshots to retain vault metadata unintentionally.
Suggested fix
- vaultProfileId: isVaultProfileId(s.vaultProfileId)
- ? s.vaultProfileId
- : s.tabType === 'vault' ? DEFAULT_VAULT_PROFILE_ID : undefined,
+ vaultProfileId: s.tabType === 'vault'
+ ? (isVaultProfileId(s.vaultProfileId) ? s.vaultProfileId : DEFAULT_VAULT_PROFILE_ID)
+ : undefined,📝 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
| vaultProfileId: isVaultProfileId(s.vaultProfileId) | |
| ? s.vaultProfileId | |
| : undefined, | |
| : s.tabType === 'vault' ? DEFAULT_VAULT_PROFILE_ID : undefined, | |
| vaultProfileId: s.tabType === 'vault' | |
| ? (isVaultProfileId(s.vaultProfileId) ? s.vaultProfileId : DEFAULT_VAULT_PROFILE_ID) | |
| : undefined, |
🤖 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/store/connectionSlice.ts` around lines 694 - 696, The current assignment
lets non-vault tabs keep a valid vaultProfileId; change the logic so you first
check s.tabType === 'vault' and only then validate/or default the id: for
example set vaultProfileId to undefined for any non-'vault' tab, and for 'vault'
tabs use isVaultProfileId(s.vaultProfileId) ? s.vaultProfileId :
DEFAULT_VAULT_PROFILE_ID; update the expression containing vaultProfileId,
isVaultProfileId, s.tabType and DEFAULT_VAULT_PROFILE_ID accordingly.
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.
Add credential revision history and restore support across the vault backend, IPC layer, and vault workspace UI.
Modularize vault workspace actions into focused hooks, tighten credential modal validation, improve restore/error handling, and preserve vault credential references across sync/import flows.
Update docs, changelog, CI checks, regression tests, and small UI/build-warning polish.
Summary by CodeRabbit
New Features
Bug Fixes
Improvements