feat: Implement multi-device shared storage and address review findings - #14
Merged
Conversation
A multi-dimensional review of the feature surfaced correctness, security, and CI issues. Fix all confirmed findings: - clippy -D warnings (CI gate): use inspect instead of map-for-side-effect; take the recovered key out of the MutexGuard before awaiting - device id path traversal: reject "."/".." in sanitize_device_id and assert a single normal path segment in device_data_dir() - cache the multi_device decision (OnceLock) so a transient config.toml parse failure can no longer flip storage mode mid-run (also fixes a destructive forget that could delete the shared cache) - enroll this device's Hello/native slot after a PIN unlock so a late-joining device is not stuck PIN-only forever - atomic owner-only writes (tmp + rename) for local-key-cache.json and unlock-slots.json via a shared write_owner_only_file helper - case-insensitive home-relative path match on Windows; warn on fallback - gate the manual-sync PIN prompt to multi-device mode (no single-device regression) - device-only forget clears key_names unconditionally - keep the in-memory cache when a shared reload finds the file absent - add tests for sanitize_device_id and home_relative_path; doc fixes Gates green: cargo fmt --check, clippy --all-targets -D warnings, test --workspace (56 passed).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughSSHWarden gains opt-in multi-device storage mode separating shared config artifacts from device-local runtime and unlock state. The PR restructures directory resolution, adds device ID resolution with sanitization, implements home-relative SSH config path styling, refactors cache persistence and unlock-slot handling, updates the forget command with a ChangesMulti-Device Storage Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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}
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 (1)
crates/sshwarden-config/src/session.rs (1)
116-121:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSession file writes are not atomic, unlike the shared cache.
session.rsstill uses a localwrite_owner_only_filethat does a non-atomicstd::fs::writefollowed by a permission change. Meanwhile,cache.rswas updated to usecrate::write_owner_only_filewhich writes to a temp file, sets permissions before rename, and handles Windows rename failures with backup/rollback.The session file contains encrypted refresh tokens. A crash or cloud-sync collision during write could truncate it. Consider switching to the crate-level atomic helper:
Proposed fix
pub fn save(&self) -> anyhow::Result<()> { let path = Self::path()?; if let Some(parent) = path.parent() { std::fs::create_dir_all(parent).with_context(|| { format!("Failed to create session directory: {}", parent.display()) })?; } let content = serde_json::to_string_pretty(self).context("Failed to serialize session file")?; - write_owner_only_file(&path, content) + crate::write_owner_only_file(&path, content) .with_context(|| format!("Failed to write session file: {}", path.display()))?; Ok(()) } ... -fn write_owner_only_file(path: &Path, content: impl AsRef<[u8]>) -> anyhow::Result<()> { - std::fs::write(path, content)?; - #[cfg(unix)] - std::fs::set_permissions(path, std::fs::Permissions::from_mode(0o600))?; - Ok(()) -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/sshwarden-config/src/session.rs` around lines 116 - 121, The local non-atomic write_owner_only_file in session.rs should be replaced with the crate-level atomic helper used by cache.rs: stop calling std::fs::write + set_permissions and instead call crate::write_owner_only_file so the file is written to a temp file, permissions are set before rename, and Windows rename failures are handled with backup/rollback; update any references to the local function (write_owner_only_file) and remove the local implementation so session file writes become atomic and consistent with cache.rs behavior.
🤖 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 `@crates/sshwarden-config/src/ssh_config.rs`:
- Around line 69-74: line_matches_sshwarden_include is triggering tracing::warn!
indirectly because include_line_with_style(...,
SshConfigPathStyle::HomeRelative) calls path_arg_with_style which logs when the
path is outside the home; change matching to use a non-logging helper for
home-relative rewriting or a cached precomputed pattern instead of calling
path_arg_with_style here: extract the path normalization/backing logic used by
path_arg_with_style into a new function (e.g., path_arg_normalized_no_log) and
have include_line_with_style call that plus emit logs only on the write path, or
compute include_line/include_line_with_style/SshConfigPathStyle::HomeRelative
variants once at startup and reuse them in line_matches_sshwarden_include so no
tracing::warn! is emitted during parsing.
In `@crates/sshwarden-config/src/unlock_slots.rs`:
- Line 15: The UnlockSlots struct exposes pub version and save() currently
writes whatever self.version contains while load() rejects unsupported versions;
update save() to validate self.version against the supported version set (the
same check used in load()) and return an error instead of persisting an
unsupported version; specifically, in UnlockSlots::save() add the same version
validation logic used in UnlockSlots::load() (or call a shared helper/constant)
to guard against writing unreadable files (also apply this guard to any other
save-like methods handling version, as noted around the code region covering
lines 62-75).
- Around line 44-45: The current code does a check-then-act using path.exists()
which can race if the file is removed between the check and the I/O; change both
code paths that check path.exists() (the block using path and the later
remove/read block) to remove the pre-check and instead perform the I/O (e.g.,
fs::read / fs::read_to_string and fs::remove_file) and match the resulting
Result; if the error kind() is std::io::ErrorKind::NotFound return Ok(None) or
otherwise ignore/continue as appropriate, and for other errors propagate them
(using ?); reference the same local variable path and the read/remove calls in
unlock_slots.rs when updating the code.
In `@src/main.rs`:
- Around line 4597-4602: After a successful PIN auto-unlock in
handle_ui_request(), call
enroll_device_platform_slots_if_missing(&local_cache_key) the same way the
explicit "unlock-pin" control path does; locate the PIN-dialog/SSH-request
unlock branch in handle_ui_request() and insert the enrollment call immediately
after the code that marks the unlock as successful so newly-joined multi-device
peers get their platform slots seeded (the function is a no-op in single-device
mode or if a slot already exists).
- Around line 2591-2603: current_hello_challenge_b64() fails in single-device
mode because it only checks load_device_unlock_slots() and falls back to
VaultFile::load(), but after set-pin/envelope migration the Hello challenge is
stored in the envelope cache (local-key-cache.json) not vault.enc; update
current_hello_challenge_b64 to, when multi_device_mode() is false, try the
envelope cache reader (the code/path that loads the local key cache / envelope
cache) before or instead of calling sshwarden_config::vault::VaultFile::load(),
so it returns the migrated hello_challenge and restores Hello-related flows
(refer to current_hello_challenge_b64, load_device_unlock_slots,
multi_device_mode, and VaultFile::load to locate where to insert the
envelope-cache lookup).
---
Outside diff comments:
In `@crates/sshwarden-config/src/session.rs`:
- Around line 116-121: The local non-atomic write_owner_only_file in session.rs
should be replaced with the crate-level atomic helper used by cache.rs: stop
calling std::fs::write + set_permissions and instead call
crate::write_owner_only_file so the file is written to a temp file, permissions
are set before rename, and Windows rename failures are handled with
backup/rollback; update any references to the local function
(write_owner_only_file) and remove the local implementation so session file
writes become atomic and consistent with cache.rs behavior.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ea4b87f1-cb08-418e-977d-7a7fbbcb8d45
📒 Files selected for processing (9)
config.toml.examplecrates/sshwarden-agent/src/control.rscrates/sshwarden-config/src/cache.rscrates/sshwarden-config/src/lib.rscrates/sshwarden-config/src/session.rscrates/sshwarden-config/src/ssh_config.rscrates/sshwarden-config/src/unlock_slots.rsdocs/adr/0024-multi-device-shared-storage-with-device-runtime.mdsrc/main.rs
Address a second review pass over the previous fix commit: - ssh_config: stop line_matches_sshwarden_include from emitting a home_relative warning on every ~/.ssh/config comparison; warn only on the write path via a new no-log path_arg_with_style_quiet - unlock_slots: validate version in save() (shared helper with load()) so an unreadable file is never persisted; drop the exists()-then-IO TOCTOU in load()/delete() in favor of matching ErrorKind::NotFound - session: use the crate-level atomic write_owner_only_file instead of a local non-atomic write; generalize its tmp/backup naming to append a suffix so .enc files keep their extension (session-x.enc.tmp) - main: current_hello_challenge_b64 reads the envelope cache (local-key-cache.json) before vault.enc in single-device mode, fixing Hello token restore after envelope migration - main: seed this device's platform unlock slot after the SSH-request PIN auto-unlock in handle_ui_request, matching the control-path behavior Gates green: cargo fmt --check, clippy --all-targets -D warnings, test --workspace (56 passed).
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.
This pull request implements a new multi-device shared storage mode for SSHWarden, enabling multiple devices to share a common configuration and key cache while keeping runtime/session data isolated per device. It introduces new configuration options, updates the storage layout logic, and refactors path handling throughout the codebase to support this feature. Additionally, it improves file writing safety and path normalization.
Multi-device shared storage support:
[storage] multi_deviceanddevice_idoptions toconfig.tomland parsing logic, allowing users to enable a shared storage mode suitable for syncing via OneDrive/Dropbox, with per-device runtime/session isolation. (config.toml.example,crates/sshwarden-config/src/lib.rs[1] [2]shared_data_dir,device_data_dir), and logic to sanitize and auto-generate device IDs. (crates/sshwarden-config/src/lib.rs[1] [2]Path handling and config changes:
crates/sshwarden-config/src/lib.rs[1] [2]SshConfigPathStyleto allow generated SSH config to use home-relative (~/...) paths for better cross-device compatibility. (crates/sshwarden-config/src/lib.rscrates/sshwarden-config/src/lib.rsR264-R288)File writing and safety:
write_owner_only_filewith a new, more robust version that atomically writes files with owner-only permissions and includes backup/rollback logic for Windows. (crates/sshwarden-config/src/cache.rs,crates/sshwarden-config/src/lib.rs[1] [2] [3] [4]Control command updates:
Forgetcontrol action to optionally remove shared cache files in multi-device mode, and added a newforget-sharedcommand. (crates/sshwarden-agent/src/control.rs[1] [2]Testing and validation:
crates/sshwarden-config/src/lib.rscrates/sshwarden-config/src/lib.rsR822-R845)These changes collectively enable safe and robust multi-device usage scenarios, particularly for users syncing their SSHWarden data via cloud storage.
Summary by CodeRabbit
New Features
forgetgains a shared-cache option to remove shared vs device-only dataDocumentation