feat: add sync diff view and secure password cache (plus fixes/ docs) #9
Conversation
feat: sync diff
feat: secure password cache
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds an encrypted, per-profile password cache with configurable TTL and file-based storage, a CLI Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as CLI/Task
participant Security
participant Cache as CacheFile
participant Age as Encryption
User->>CLI: Run operation needing password
CLI->>Security: get_or_prompt_password(profile)
Security->>Security: Load security config (security.json)
Security->>Cache: Check cache file (exists & TTL?)
alt Cache hit & valid
Cache-->>Security: Encrypted record
Security->>Age: Decrypt with identity
Age-->>Security: Plain password
Security-->>CLI: Return password
else Cache miss or expired
Security->>User: Prompt for password
User-->>Security: Enter password
Security->>Age: Encrypt password
Age->>Cache: Store encrypted record + timestamp
Security-->>CLI: Return password
end
CLI->>CLI: Continue operation
sequenceDiagram
participant User
participant CLI as CLI/SyncTask
participant Git
participant Output
User->>CLI: mntn sync --diff
CLI->>Git: git status
Git-->>CLI: Status
CLI->>Git: git diff (unstaged)
Git-->>CLI: Unstaged diff bytes
CLI->>Output: Print "Unstaged Changes"
CLI->>Git: git diff --staged (or --cached)
alt staged diff succeeds
Git-->>CLI: Staged diff bytes
else staged diff fails
Git-->>CLI: Error -> CLI retries with --cached
Git-->>CLI: Staged diff bytes
end
CLI->>Output: Print "Staged Changes"
Output-->>User: Display combined diffs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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. ✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/tasks/restore.rs (1)
321-331:⚠️ Potential issue | 🟠 MajorOn password error, the loop continues with the same wrong password, producing redundant warnings for all remaining entries.
After
invalidate_password_cacheon Line 324, the loop continues to the next entry which will also fail with the same already-obtained wrong password.validate.rscorrectly doesreturn errors;in the same scenario (Line 560). Consider breaking out of the loop here as well.Proposed fix
let err_msg = e.to_string(); if is_password_error(&err_msg) { invalidate_password_cache(profile); + log_warning(&format!( + "Failed to restore encrypted {}: incorrect password", + entry.name + )); + skipped_count += 1; + // Wrong password — skip remaining encrypted entries + skipped_count += enabled_entries.len() as u32 + - (restored_count + skipped_count); + return (restored_count, skipped_count); } - log_warning(&format!( - "Failed to restore encrypted {}: {}", - entry.name, e - )); - skipped_count += 1; + else { + log_warning(&format!( + "Failed to restore encrypted {}: {}", + entry.name, e + )); + skipped_count += 1; + }
🤖 Fix all issues with AI agents
In `@CHANGELOG.md`:
- Line 13: Change the likely-typo "skills ls -g" in the CHANGELOG entry to the
intended package manager command (e.g., replace "skills ls -g" with the actual
command like "npm ls -g" or "pnpm ls -g"), and update the surrounding text to
match that command (the entry labeled "Package Registry Output" that mentions
stripping ANSI escape codes should reference the corrected command name).
🧹 Nitpick comments (6)
src/utils/system.rs (1)
207-212: Test looks good, but consider adding an edge-case test for plain text passthrough.A test with no ANSI codes (e.g.,
"plain text") confirming the string is returned unchanged would strengthen coverage.README.md (1)
371-372: Implementation detail leaks into user-facing docs.The parenthetical "(uses
--cachedfallback for staged diffs)" describes an internal git implementation detail that may confuse users. Consider simplifying to just describe the user-visible behavior, e.g., "Show changes before syncing."src/encryption.rs (1)
84-91: Broad keyword matching could cause false positives.Terms like
"decrypt"and"identity"are generic and could match non-password errors (e.g.,"failed to decrypt configuration"due to corrupted data, or"identity not found"in a different context). In practice this is likely fine since callers are scoped toagedecryption, but consider adding a doc comment clarifying the intended use context so future callers don't misuse it.Suggested doc comment
+/// Heuristically checks whether an error message from the `age` encryption +/// library indicates a wrong-password / bad-identity failure. +/// Not intended for general-purpose error classification. pub fn is_password_error(message: &str) -> bool {src/tasks/sync.rs (1)
307-324: Redundantuse std::io::Write;— already imported at file scope (Line 8).The inner
use std::io::Write;at Line 308 shadows the file-level import at Line 8. Harmless but unnecessary.src/security.rs (2)
79-83:PasswordCacheRecord.passwordis a plainString— not zeroized on drop.After deserialization (Line 250) or before serialization (Line 293-296), the plaintext password exists as a regular
Stringin heap memory. When thePasswordCacheRecordis dropped, the memory is deallocated but not zeroed, leaving the password potentially recoverable from memory. Consider usingSecretString(or thezeroizecrate'sZeroizing<String>) for thepasswordfield inPasswordCacheRecord, or manually zeroize the record after use.This is a defense-in-depth concern rather than a practical exploit in most scenarios.
299-302: Minor TOCTOU in temp file path selection.The existence check on Line 300 and creation on Line 304 are not atomic. A concurrent process could create the same path between the check and the write. The risk is minimal (worst case: a failed write that's already handled by
?), but usingtempfile::NamedTempFilein the same directory would be more robust.
There was a problem hiding this comment.
Pull request overview
Adds a sync --diff view and introduces an encrypted, local password cache to reduce repeated prompts during encrypted backup/restore/validate flows. Also improves backup output stability by stripping ANSI escape sequences from captured package-manager command output.
Changes:
- Add
mntn sync --diffto display unstaged + staged diffs (with--cachedfallback). - Introduce
securitymodule withsecurity.jsonconfiguration and age-encrypted password caching under~/.mntn/.secrets/, plus cache invalidation on password errors. - Strip ANSI CSI sequences from package manager command output before persisting backups.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/system.rs | Adds ANSI escape stripping helper used to sanitize captured command output. |
| src/utils/paths.rs | Adds .secrets + security.json path constants and helpers. |
| src/tasks/validate.rs | Switches encrypted validation password flow to cached prompting and cache invalidation on password errors. |
| src/tasks/sync.rs | Adds --diff flag behavior and implementation to print git diffs. |
| src/tasks/restore.rs | Switches encrypted restore to cached prompting; invalidates cache on password errors. |
| src/tasks/backup.rs | Switches encrypted backup to cached prompting; strips ANSI escapes from package outputs before writing. |
| src/security.rs | New module implementing security config + encrypted password cache + tests. |
| src/main.rs | Registers the new security module. |
| src/encryption.rs | Adds is_password_error helper used by restore/validate flows. |
| src/cli.rs | Adds mntn sync --diff CLI flag. |
| README.md | Documents .secrets/, security.json, password cache behavior, and sync --diff. |
| Cargo.lock | Dependency lock updates (including regex, chrono, clap, etc.). |
| CHANGELOG.md | Documents v2.3.0 features/fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/utils/permissions.rs`:
- Around line 29-141: The variable `token` is declared as HANDLE (an isize) but
is initialized with ptr::null_mut() (a pointer), causing a type mismatch; change
the initialization of token to 0 (e.g., let mut token: HANDLE = 0;) and ensure
subsequent uses (OpenProcessToken, CloseHandle, GetTokenInformation) treat token
as the numeric HANDLE value returned by OpenProcessToken in set_owner_only_dacl;
update any casts/uses that assumed a pointer (e.g., passing &mut token into
OpenProcessToken stays the same) and remove pointer-specific initializations so
token is correctly typed for windows-sys 0.59.0.
🧹 Nitpick comments (5)
src/utils/permissions.rs (1)
84-85: Potential alignment issue when castingVec<u8>buffer toTOKEN_USER.
TOKEN_USERmay have alignment requirements stricter than 1 byte. Castingbuffer.as_ptr()(a*const u8, aligned to 1) to*const TOKEN_USERis technically undefined behavior if the alignment doesn't match. In practice,Vec<u8>may be over-aligned on Windows, but this is not guaranteed.A safer approach would be to use a buffer with guaranteed alignment (e.g.,
Vec<u64>or#[repr(align)]wrapper).src/utils/paths.rs (1)
90-100: Good env var override with defensive trimming.The
MNTN_SECRETS_KEY_DIRoverride pattern is clean and mirrors the existingMNTN_PROFILEpattern. Consider adding a test for the env var override path (similar totest_get_active_profile_name_from_env_var) to prevent regressions.src/security.rs (2)
386-401:fs::readfailure short-circuits the retry loop, unlike other steps.The
?onfs::read(line 390) causes an immediate return from the function, while all other error paths (Decryptor, decrypt, read_to_end, deserialize) use theif attempt == 0 { continue; }retry pattern. This inconsistency means a transient I/O error won't be retried.In practice the impact is low since a file read failure (e.g., file-not-found, permission denied) is unlikely to self-resolve on immediate retry. But the inconsistent control flow could confuse future maintainers.
♻️ Suggested consistency fix
for attempt in 0..2 { - let encrypted = fs::read(cache_path).map_err(|e| { - last_error = Some(format!("Failed to read password cache: {}", e)); - last_error.clone().unwrap() - })?; + let encrypted = match fs::read(cache_path) { + Ok(data) => data, + Err(e) => { + last_error = Some(format!("Failed to read password cache: {}", e)); + if attempt == 0 { + continue; + } + return Err(last_error.clone().unwrap()); + } + };
113-120:savedoes not lock down file permissions on the written config.
security.jsonisn't secret (no passwords), but it does reveal cache policy (enabled, TTL, profile names). If an attacker can write to it, they could disable caching to force password prompts (social engineering vector) or set TTL tonever. Consider applyinglock_down_fileafter write, similar to how cache and key files are protected.Cargo.toml (1)
43-44: Windows-specific dependency is properly scoped; consider updating to the latest version.The features align with the Win32 APIs used in
src/utils/permissions.rsand the dependency is correctly gated behindcfg(windows). However,windows-sys0.61.2 is now available—consider updating from 0.59.0 for the latest bug fixes and improvements.
| impl PasswordCacheTtl { | ||
| pub fn as_seconds(self) -> Option<i64> { | ||
| match self { | ||
| PasswordCacheTtl::FifteenMinutes => Some(15 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| pub fn as_seconds(self) -> Option<i64> { | ||
| match self { | ||
| PasswordCacheTtl::FifteenMinutes => Some(15 * 60), | ||
| PasswordCacheTtl::OneHour => Some(60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| pub fn as_seconds(self) -> Option<i64> { | ||
| match self { | ||
| PasswordCacheTtl::FifteenMinutes => Some(15 * 60), | ||
| PasswordCacheTtl::OneHour => Some(60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| match self { | ||
| PasswordCacheTtl::FifteenMinutes => Some(15 * 60), | ||
| PasswordCacheTtl::OneHour => Some(60 * 60), | ||
| PasswordCacheTtl::EightHours => Some(8 * 60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| PasswordCacheTtl::OneHour => Some(60 * 60), | ||
| PasswordCacheTtl::EightHours => Some(8 * 60 * 60), | ||
| PasswordCacheTtl::OneDay => Some(24 * 60 * 60), | ||
| PasswordCacheTtl::SevenDays => Some(7 * 24 * 60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| PasswordCacheTtl::OneHour => Some(60 * 60), | ||
| PasswordCacheTtl::EightHours => Some(8 * 60 * 60), | ||
| PasswordCacheTtl::OneDay => Some(24 * 60 * 60), | ||
| PasswordCacheTtl::SevenDays => Some(7 * 24 * 60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| PasswordCacheTtl::OneHour => Some(60 * 60), | ||
| PasswordCacheTtl::EightHours => Some(8 * 60 * 60), | ||
| PasswordCacheTtl::OneDay => Some(24 * 60 * 60), | ||
| PasswordCacheTtl::SevenDays => Some(7 * 24 * 60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| PasswordCacheTtl::OneHour => Some(60 * 60), | ||
| PasswordCacheTtl::EightHours => Some(8 * 60 * 60), | ||
| PasswordCacheTtl::OneDay => Some(24 * 60 * 60), | ||
| PasswordCacheTtl::SevenDays => Some(7 * 24 * 60 * 60), |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
| with_temp_home(|| { | ||
| let profile = ActiveProfile::with_profile("test-profile"); | ||
| let ttl_seconds = PasswordCacheTtl::OneHour.as_seconds().unwrap(); | ||
| let expired_at = Utc::now().timestamp() - ttl_seconds - 1; |
Check failure
Code scanning / CodeQL
Hard-coded cryptographic value Critical
Copilot Autofix
AI about 20 hours ago
Copilot could not generate an autofix suggestion
Copilot could not generate an autofix suggestion for this alert. Try pushing a new commit or if the problem persists contact support.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/security.rs`:
- Around line 272-320: store_cached_password currently creates tmp_path and
tmp_file and may return early via ? after partial work, leaving orphaned tmp
files; fix by ensuring tmp_path is removed on any error: wrap the
encryption/write/sync/lock_down/rename sequence in a scope that captures
tmp_path and collects the Result, and on Err(_) call let _ =
fs::remove_file(&tmp_path) before returning the error, or use a small RAII guard
(or tempfile::NamedTempFile) that removes the temp file on drop unless
explicitly persisted; update references inside store_cached_password (tmp_path,
tmp_file, encryptor, lock_down_file, fs::rename) so the temp file is always
cleaned up on failure and only preserved when fs::rename succeeds.
🧹 Nitpick comments (3)
src/security.rs (3)
113-120: Consider locking down the security config file after save.
savecreates the file but doesn't restrict permissions. While this file doesn't hold secrets directly, it does control security-sensitive settings (e.g., whether caching is enabled, TTL values). A tampered config could trick the system into caching passwords indefinitely or disabling cache expiry. Other write sites in this module (store_cached_password,get_or_create_cache_identity) consistently calllock_down_file.🛡️ Proposed fix
let content = serde_json::to_string_pretty(self)?; fs::write(path, content)?; + if let Err(e) = lock_down_file(path) { + log_warning(&format!("Failed to lock down security config file: {}", e)); + } Ok(())
380-438: Retry loop is ineffective — immediate re-read of the same file without delay.Two issues with this retry approach:
- The
?operator on line 390 causes an immediate return on I/O read errors, bypassing the retry logic entirely — inconsistent with the other error branches that usecontinue.- For crypto/parse errors, retrying immediately (no sleep/backoff) against the same file will almost certainly produce the same result, making the second attempt a no-op.
If the goal is to handle a narrow race with a concurrent writer, consider adding a small delay between attempts or removing the retry altogether and simply returning the error (the caller already handles failure gracefully by falling back to prompting).
♻️ Simplified version without retry
fn read_cached_password_record( cache_path: &Path, identity: &age::x25519::Identity, ) -> Result<PasswordCacheRecord, String> { - let mut last_error = None; - - for attempt in 0..2 { - let encrypted = fs::read(cache_path).map_err(|e| { - last_error = Some(format!("Failed to read password cache: {}", e)); - last_error.clone().unwrap() - })?; - - let decryptor = match Decryptor::new(&encrypted[..]) { - Ok(decryptor) => decryptor, - Err(e) => { - last_error = Some(format!("Failed to parse password cache: {}", e)); - if attempt == 0 { - continue; - } - return Err(last_error.clone().unwrap()); - } - }; - - let mut decrypted = Vec::new(); - let mut reader = match decryptor.decrypt(std::iter::once(identity as &dyn age::Identity)) { - Ok(reader) => reader, - Err(e) => { - last_error = Some(format!("Failed to decrypt password cache: {}", e)); - if attempt == 0 { - continue; - } - return Err(last_error.clone().unwrap()); - } - }; - if reader.read_to_end(&mut decrypted).is_err() { - last_error = Some("Failed to read decrypted password cache".to_string()); - if attempt == 0 { - continue; - } - return Err(last_error.clone().unwrap()); - } - - let decrypted = SecretBox::new(Box::new(decrypted)); - let record: PasswordCacheRecord = match serde_json::from_slice(decrypted.expose_secret()) { - Ok(record) => record, - Err(e) => { - last_error = Some(format!("Failed to parse password cache record: {}", e)); - if attempt == 0 { - continue; - } - return Err(last_error.clone().unwrap()); - } - }; - drop(decrypted); - return Ok(record); - } - - Err(last_error.unwrap_or_else(|| "Failed to read password cache".to_string())) + let encrypted = fs::read(cache_path) + .map_err(|e| format!("Failed to read password cache: {}", e))?; + + let decryptor = Decryptor::new(&encrypted[..]) + .map_err(|e| format!("Failed to parse password cache: {}", e))?; + + let mut decrypted = Vec::new(); + let mut reader = decryptor + .decrypt(std::iter::once(identity as &dyn age::Identity)) + .map_err(|e| format!("Failed to decrypt password cache: {}", e))?; + reader + .read_to_end(&mut decrypted) + .map_err(|_| "Failed to read decrypted password cache".to_string())?; + + let decrypted = SecretBox::new(Box::new(decrypted)); + let record: PasswordCacheRecord = serde_json::from_slice(decrypted.expose_secret()) + .map_err(|e| format!("Failed to parse password cache record: {}", e))?; + drop(decrypted); + Ok(record) }
293-298: Minor TOCTOU on temp file path selection.The existence check on line 296 and
File::createon line 300 are not atomic — another process could create the same temp file in between. Sincefs::File::createtruncates, this won't error, but it could cause data loss if two processes race. Consider usingcreate_new(true)(like you do for the key file on line 342) and retrying with the pid-suffixed path onAlreadyExists.♻️ Use create_new for atomicity
- let mut tmp_path = cache_path.with_extension("tmp"); - if tmp_path.exists() { - tmp_path = cache_path.with_extension(format!("tmp.{}", std::process::id())); - } - - let mut tmp_file = fs::File::create(&tmp_path)?; + let tmp_path = cache_path.with_extension(format!("tmp.{}", std::process::id())); + let mut tmp_file = fs::OpenOptions::new() + .create_new(true) + .write(true) + .open(&tmp_path) + .or_else(|_| { + // Fallback: truncate if our own PID file somehow exists + fs::File::create(&tmp_path) + })?;
1 participant
Summary by CodeRabbit
New Features
Security / Hardening
Bug Fixes
Documentation
Tests