Conversation
…annel tokens, GitHub auth Replaces the sqlite-backed gateway_secrets store with flare-vault's envelope-encrypted local vault (Argon2id KEK, AES-256-GCM DEK, OS-keyring session cache, global/project scoping). channels::send_message, cli::gateway secret, github::auth, and mcp_server's gateway-secret resolution now go through crate::vault instead of crate::gateway_secrets. Fixes found in review: - vault::get_secret was swallowing unseal errors (wrong/missing passphrase) into Ok(None), indistinguishable from "no secret configured" -- exactly the failure mode mcp_server::resolve_gateway_secrets was trying to surface. Now propagates the error. - No migration path existed from the old gateway_secrets sqlite table to the new vault, so upgrading would silently drop already-configured secrets (github_token, telegram_bot_token, etc). Added a one-time, best-effort migration on first unlock/unseal. - Deduplicated the encrypted-DEK byte layout (magic+nonce+ciphertext) into EncryptedBlob::to_bytes/from_bytes instead of hand-packing it in both create_vault and open_vault. - Fixed mod declaration order in main.rs (vault was sorted before update).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds the ChangesVault crate and integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant VaultCli
participant AppVault
participant FlareVault
participant Session
VaultCli->>AppVault: unlock(passphrase)
AppVault->>FlareVault: create_vault or open_vault
FlareVault-->>AppVault: VaultDek
AppVault->>Session: store_session(app_name, vault_path, dek)
VaultCli->>AppVault: get_secret(name)
AppVault->>FlareVault: read_vault_body and decrypt value
FlareVault-->>VaultCli: secret value
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/channels.rs (1)
291-295: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep this test isolated from the user's vault and network.
If
telegram_bot_tokenexists in the test runner's vault, this invokes the real Telegram send path; otherwise a locked/corrupt vault changes the expected error. Inject or fixture the secret lookup so this test deterministically verifiesOk(None)before any HTTP request.🤖 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/channels.rs` around lines 291 - 295, Update the send_message test around the Telegram path to inject or fixture the secret lookup instead of reading the user’s vault. Make the fixture deterministically return Ok(None) for telegram_bot_token, and assert that missing-secret behavior occurs before any HTTP request or real Telegram send path is invoked.
🧹 Nitpick comments (1)
crates/flare-vault/src/crypto/aead.rs (1)
40-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate framing logic between
EncryptedBlobandencrypt_value/decrypt_value.
EncryptedBlob::to_bytes/from_bytesalready implement the exactmagic + nonce + ciphertextlayout used again manually inencrypt_value/decrypt_value. Any future change to the blob format (e.g. versioning, longer nonce) needs to be updated in two places consistently.♻️ Reuse `EncryptedBlob` for value encryption
pub fn encrypt_value(plaintext: &[u8], dek: &[u8; 32]) -> Result<Vec<u8>, String> { let mut nonce_bytes = [0u8; NONCE_SIZE]; OsRng.fill_bytes(&mut nonce_bytes); let nonce = Nonce::from_slice(&nonce_bytes); let cipher = Aes256Gcm::new(Key::<Aes256Gcm>::from_slice(dek)); let ciphertext = cipher .encrypt(nonce, plaintext) .map_err(|e| format!("AES-GCM encrypt value: {e}"))?; - let mut out = MAGIC.to_vec(); - out.extend_from_slice(&nonce_bytes); - out.extend(ciphertext); - Ok(out) + Ok(EncryptedBlob { + magic: *b"FLVT", + nonce: nonce_bytes, + ciphertext, + } + .to_bytes()) } pub fn decrypt_value(data: &[u8], dek: &[u8; 32]) -> Result<Vec<u8>, String> { - if data.len() < MAGIC.len() + NONCE_SIZE + 16 { - return Err("data too short".into()); - } - if &data[..MAGIC.len()] != MAGIC { - return Err("invalid magic".into()); - } - let nonce = Nonce::from_slice(&data[MAGIC.len()..MAGIC.len() + NONCE_SIZE]); - let ciphertext = &data[MAGIC.len() + NONCE_SIZE..]; + let blob = EncryptedBlob::from_bytes(data)?; + if &blob.magic != MAGIC { + return Err("invalid magic".into()); + } + let nonce = Nonce::from_slice(&blob.nonce); let cipher = Aes256Gcm::new(Key::<Aes256Gcm>::from_slice(dek)); cipher - .decrypt(nonce, ciphertext) + .decrypt(nonce, blob.ciphertext.as_slice()) .map_err(|_| "AES-GCM decrypt failed".into()) }🤖 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/flare-vault/src/crypto/aead.rs` around lines 40 - 93, Refactor encrypt_value and decrypt_value to reuse EncryptedBlob::to_bytes and EncryptedBlob::from_bytes for the shared magic, nonce, and ciphertext framing. Preserve the existing AES-GCM encryption/decryption behavior and error handling while removing the duplicated manual layout and validation logic.
🤖 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/flare-vault/src/inject.rs`:
- Around line 50-63: The load_from_global_vault test currently only checks
success without verifying injection. Store the DEK using the exact vault path
before calling load_vault_env, then assert that the returned environment map
contains MY_SECRET with the value test-value, using the existing
open_vault_with_dek/session-DEK flow.
- Around line 15-38: Update the vault-loading logic around the global and
project calls to propagate unexpected errors through the enclosing function’s
VaultResult return type. Continue treating absent or locked vaults as skippable,
but return I/O, parse, and decryption failures from open_vault_with_dek,
read_vault_body, and get_secret_value instead of discarding them; preserve the
existing environment insertion and project-vault handling for successful loads.
In `@crates/flare-vault/src/session/file_cache.rs`:
- Around line 4-8: Update cache_dir to return an error when dirs::home_dir() is
unavailable instead of falling back to the current directory, and propagate that
error through its callers. In the cache creation and write flow using
create_dir_all and fs::write, enforce owner-only directory access and owner
read/write file permissions, or equivalent platform ACLs.
In `@crates/flare-vault/src/session/mod.rs`:
- Around line 24-34: Update store_session and clear_session to return a Result
and propagate each cache backend’s outcome instead of unconditionally reporting
success. Permit store_session to succeed when at least one backend stores the
DEK successfully, while clear_session must fail when an existing credential or
cache file cannot be removed and treat missing entries as successful cleanup.
Update callers to handle the new results.
In `@crates/flare-vault/src/vault/manager.rs`:
- Around line 73-100: Protect the entire destination-file operation with a
shared exclusive lock: update create_vault and the read-modify-write flow around
write_vault_body so locking occurs before vault_file_exists or read_vault_file
and remains held through write_vault_file’s completed rename. Use the
destination path or a stable sidecar lock shared by all callers, rather than the
uniquely named temporary file, and release it only after persistence finishes.
- Around line 116-121: In the DEK-loading flow containing decrypt_dek, zeroize
the intermediate dek_bytes Vec after copying it into the fixed-size dek array
and before returning VaultDek. Use the existing zeroization mechanism already
applied to nearby sensitive types, while preserving the current WrongPassphrase
mapping and VaultDek construction.
- Around line 159-175: Update set_secret_value to check whether body already
contains name before encrypting or inserting; return
VaultError::SecretAlreadyExists for duplicates so callers such as set_secret
cannot silently overwrite existing secrets. Preserve the current insertion
behavior for new names.
In `@src/cli/vault.rs`:
- Around line 33-44: Update run_unlock’s stdin passphrase handling to remove
only the trailing line ending, not leading or trailing passphrase whitespace.
Preserve all meaningful whitespace while keeping the interactive prompt and
vault creation passphrase behavior consistent.
In `@src/vault.rs`:
- Around line 176-180: Update list_secrets to detect a missing vault.json while
reading through read_vault_body and return Ok(Vec::new()) for that case,
matching get_secret’s absent-vault behavior. Preserve existing error propagation
for other read failures and continue using list_secret_names when the vault
exists.
- Around line 191-193: Update vault_env to return a Result rather than using
unwrap_or_default, propagating load_vault_env failures to cli_run and vault env.
Preserve an empty environment only when the vault is explicitly absent, while
allowing other errors to surface to callers.
---
Outside diff comments:
In `@src/channels.rs`:
- Around line 291-295: Update the send_message test around the Telegram path to
inject or fixture the secret lookup instead of reading the user’s vault. Make
the fixture deterministically return Ok(None) for telegram_bot_token, and assert
that missing-secret behavior occurs before any HTTP request or real Telegram
send path is invoked.
---
Nitpick comments:
In `@crates/flare-vault/src/crypto/aead.rs`:
- Around line 40-93: Refactor encrypt_value and decrypt_value to reuse
EncryptedBlob::to_bytes and EncryptedBlob::from_bytes for the shared magic,
nonce, and ciphertext framing. Preserve the existing AES-GCM
encryption/decryption behavior and error handling while removing the duplicated
manual layout and validation logic.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3529e0dd-d032-4b18-b7d4-550dbd6c3d50
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
CHANGELOG.mdCargo.tomlcrates/flare-vault/Cargo.tomlcrates/flare-vault/src/crypto/aead.rscrates/flare-vault/src/crypto/kdf.rscrates/flare-vault/src/crypto/mod.rscrates/flare-vault/src/error.rscrates/flare-vault/src/inject.rscrates/flare-vault/src/lib.rscrates/flare-vault/src/session/file_cache.rscrates/flare-vault/src/session/keyring_cache.rscrates/flare-vault/src/session/mod.rscrates/flare-vault/src/vault/file.rscrates/flare-vault/src/vault/manager.rscrates/flare-vault/src/vault/mod.rscrates/flare-vault/src/vault/model.rssrc/agents.rssrc/channels.rssrc/cli/channel.rssrc/cli/gateway.rssrc/cli/mod.rssrc/cli/vault.rssrc/gateway_secrets.rssrc/github/auth.rssrc/github/init_auth.rssrc/main.rssrc/mcp_server.rssrc/vault.rs
Comment on lines
+4
to
+8
| fn cache_dir(app_name: &str) -> PathBuf { | ||
| let home = dirs::home_dir().unwrap_or_else(|| PathBuf::from(".")); | ||
| home.join(format!(".{app_name}")) | ||
| .join("cache") | ||
| .join("vault-sessions") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict fallback-cache placement and permissions.
The DEK fallback is recoverable from predictable machine/user metadata. create_dir_all and fs::write rely on the umask, so a traversable home can expose a readable .session file to other local users; falling back to . can place it in an arbitrary shared directory. Return an error when no home directory exists, create the cache directory as owner-only, and write cache files as owner-read/write only (or apply equivalent platform ACLs).
Also applies to: 51-58
🤖 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/flare-vault/src/session/file_cache.rs` around lines 4 - 8, Update
cache_dir to return an error when dirs::home_dir() is unavailable instead of
falling back to the current directory, and propagate that error through its
callers. In the cache creation and write flow using create_dir_all and
fs::write, enforce owner-only directory access and owner read/write file
permissions, or equivalent platform ACLs.
Comment on lines
+24
to
+34
| pub fn store_session(app_name: &str, vault_path: &Path, dek: &[u8; 32]) { | ||
| let entry_key = vault_path_hash(vault_path); | ||
|
|
||
| keyring_cache::store_in_keyring(app_name, &entry_key, dek); | ||
| file_cache::store_in_file_cache(app_name, &entry_key, dek); | ||
| } | ||
|
|
||
| pub fn clear_session(app_name: &str, vault_path: &Path) { | ||
| let entry_key = vault_path_hash(vault_path); | ||
| keyring_cache::clear_keyring(app_name, &entry_key); | ||
| file_cache::clear_file_cache(app_name, &entry_key); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not report unlock/lock success when cache operations fail.
store_session and clear_session cannot surface failures from either backend. Consequently, unlock may not persist a session, while lock may leave a DEK accessible. Return a result: allow unlock when at least one cache backend stores successfully, but fail lock if an existing credential/cache file cannot be removed (while treating “not found” as success).
🤖 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/flare-vault/src/session/mod.rs` around lines 24 - 34, Update
store_session and clear_session to return a Result and propagate each cache
backend’s outcome instead of unconditionally reporting success. Permit
store_session to succeed when at least one backend stores the DEK successfully,
while clear_session must fail when an existing credential or cache file cannot
be removed and treat missing entries as successful cleanup. Update callers to
handle the new results.
Comment on lines
+73
to
+100
| pub fn create_vault(path: &Path, passphrase: &str) -> VaultResult<()> { | ||
| if vault_file_exists(path) { | ||
| return Err(VaultError::AlreadyInitialized(path.display().to_string())); | ||
| } | ||
|
|
||
| let mut salt = vec![0u8; SALT_SIZE]; | ||
| rand::rngs::OsRng.fill_bytes(&mut salt); | ||
|
|
||
| let params = KdfParams { | ||
| salt: salt.clone(), | ||
| ..Default::default() | ||
| }; | ||
|
|
||
| let kek = derive_kek(passphrase, ¶ms).map_err(VaultError::Crypto)?; | ||
|
|
||
| let mut dek = [0u8; DEK_SIZE]; | ||
| rand::rngs::OsRng.fill_bytes(&mut dek); | ||
|
|
||
| let blob = encrypt_dek(&dek, &kek.key).map_err(VaultError::Crypto)?; | ||
| let vault = VaultFile::new(blob.to_bytes(), salt); | ||
|
|
||
| if let Some(parent) = path.parent() { | ||
| std::fs::create_dir_all(parent)?; | ||
| } | ||
|
|
||
| let json = serde_json::to_string_pretty(&vault)?; | ||
| write_vault_file(path, json.as_bytes()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Lost-update race: no lock held across the destination file's read-modify-write / check-then-act.
create_vault's vault_file_exists check (line 74) and write_vault_body's read_vault_file (line 137) are followed later by a separate write_vault_file call, with no lock held across the whole sequence. write_vault_file only locks its own private, uniquely-named temp file (see file.rs review), which no other process can ever contend for — so it provides no real mutual exclusion on the destination vault.json. Two concurrent callers (e.g. a CLI vault command and the MCP server resolving/persisting gateway secrets) can both read the same old body, mutate independently, and each persist — the later writer silently discards the earlier caller's change (or, for create_vault, the earlier caller's salt/DEK).
Fixing this requires holding an exclusive lock across the entire read-modify-write (e.g. lock the destination path itself, or a sidecar lock file, before reading and release only after the rename completes), not just around the temp-file write step.
Also applies to: 136-142
🤖 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/flare-vault/src/vault/manager.rs` around lines 73 - 100, Protect the
entire destination-file operation with a shared exclusive lock: update
create_vault and the read-modify-write flow around write_vault_body so locking
occurs before vault_file_exists or read_vault_file and remains held through
write_vault_file’s completed rename. Use the destination path or a stable
sidecar lock shared by all callers, rather than the uniquely named temporary
file, and release it only after persistence finishes.
Comment on lines
+159
to
+175
| pub fn set_secret_value( | ||
| body: &mut VaultBody, | ||
| dek: &[u8; 32], | ||
| name: &str, | ||
| value: &str, | ||
| ) -> VaultResult<()> { | ||
| let encrypted = encrypt_value(value.as_bytes(), dek).map_err(VaultError::Crypto)?; | ||
| body.insert( | ||
| name.to_string(), | ||
| SecretEntry { | ||
| value: encrypted, | ||
| added_at: Utc::now(), | ||
| rotated_at: None, | ||
| }, | ||
| ); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
# Check whether the agentflare wrapper checks existence before calling set_secret_value,
# and whether SecretAlreadyExists is constructed anywhere in the codebase.
ast-grep run --pattern 'set_secret_value($$$)' --lang rust src
ast-grep run --pattern 'VaultError::SecretAlreadyExists($$$)' --lang rust src crates/flare-vaultRepository: getappz/agentflare
Length of output: 347
🏁 Script executed:
sed -n '90,135p' src/vault.rs
printf '\n---\n'
sed -n '155,190p' src/vault.rs
printf '\n---\n'
rg -n "SecretAlreadyExists|set_secret_value\(" crates/flare-vault srcRepository: getappz/agentflare
Length of output: 3762
🏁 Script executed:
sed -n '1,80p' crates/flare-vault/src/error.rs
printf '\n---\n'
sed -n '150,185p' crates/flare-vault/src/vault/manager.rs
printf '\n---\n'
sed -n '1,120p' crates/flare-vault/src/inject.rsRepository: getappz/agentflare
Length of output: 4249
set_secret_value overwrites existing secrets silently.
VaultError::SecretAlreadyExists is never returned, and src/vault.rs::set_secret calls this helper directly. If duplicate names should be rejected, add an existence check here; otherwise remove the unused error variant.
🤖 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/flare-vault/src/vault/manager.rs` around lines 159 - 175, Update
set_secret_value to check whether body already contains name before encrypting
or inserting; return VaultError::SecretAlreadyExists for duplicates so callers
such as set_secret cannot silently overwrite existing secrets. Preserve the
current insertion behavior for new names.
Comment on lines
+191
to
+193
| pub fn vault_env(working_dir: &Path) -> HashMap<String, String> { | ||
| load_vault_env(APP_NAME, working_dir).unwrap_or_default() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/vault.rs ==\n'
nl -ba src/vault.rs | sed -n '1,280p'
printf '\n== src/agents.rs ==\n'
nl -ba src/agents.rs | sed -n '150,260p'
printf '\n== search load_vault_env / vault_env usages ==\n'
rg -n "load_vault_env|vault_env\(" -S srcRepository: getappz/agentflare
Length of output: 216
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== src/vault.rs outline ==\n'
ast-grep outline src/vault.rs --view expanded || true
printf '\n== src/agents.rs outline ==\n'
ast-grep outline src/agents.rs --view expanded || trueRepository: getappz/agentflare
Length of output: 2608
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for p in [Path('src/vault.rs'), Path('src/agents.rs')]:
print(f"\n== {p} ==")
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 'vault_env' in line or 'load_vault_env' in line or 'list_secrets' in line or 'get_secret' in line:
start = max(1, i-8)
end = min(len(text.splitlines()), i+25)
for j in range(start, end+1):
print(f"{j:4}: {text.splitlines()[j-1]}")
print()
break
PYRepository: getappz/agentflare
Length of output: 2420
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "vault_env\(|load_vault_env|list_secrets|get_secret|APP_NAME" src -SRepository: getappz/agentflare
Length of output: 2360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
files = [
Path('src/cli/vault.rs'),
Path('src/mcp_server.rs'),
Path('src/vault.rs'),
]
for p in files:
print(f"\n== {p} ==")
lines = p.read_text().splitlines()
# print all occurrences of relevant terms with context
targets = {'vault_env', 'load_vault_env', 'list_secrets', 'get_secret', 'cli_run', 'run_launch_env'}
hit_any = False
for i, line in enumerate(lines, 1):
if any(t in line for t in targets):
hit_any = True
start = max(1, i-8)
end = min(len(lines), i+22)
for j in range(start, end+1):
print(f"{j:4}: {lines[j-1]}")
print()
if not hit_any:
print("(no relevant lines)")
PYRepository: getappz/agentflare
Length of output: 11383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline src/cli/vault.rs --view expanded || true
ast-grep outline src/mcp_server.rs --view expanded || trueRepository: getappz/agentflare
Length of output: 6928
Propagate vault load errors from vault_env.
unwrap_or_default() turns every vault-load failure into “no secrets,” so cli_run and vault env can continue with missing credentials instead of surfacing the problem. Return a Result, and only treat an explicitly absent vault as empty.
🤖 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.rs` around lines 191 - 193, Update vault_env to return a Result
rather than using unwrap_or_default, propagating load_vault_env failures to
cli_run and vault env. Preserve an empty environment only when the vault is
explicitly absent, while allowing other errors to surface to callers.
…ion tests Addresses findings from the PR review: - Zeroize passphrase and decrypted-secret memory. PASSPHRASE_CACHE and vault::get_passphrase() now hold a Zeroizing<String> instead of a plain String (previously the longest-lived unprotected copy -- cached in a static until an explicit `lock()`). get_secret_value/vault::get_secret now return Zeroizing<String> too, so decrypted plaintext is wiped as soon as its holder drops it instead of lingering in the heap. Callers that must hand off an owned String to an unavoidable final sink (env var HashMaps, gh/HTTP calls) convert at that boundary. - Fix flare-vault bypassing this codebase's home-dir test-isolation convention. VaultPaths::global/project and the session file-cache both called dirs::home_dir() directly, ignoring the AGENTFLARE_HOME_OVERRIDE escape hatch src/paths.rs documents was added after a sandboxed test run once wrote to a live ~/.claude/settings.json. Confirmed the same class of bug here: running `cargo test -p flare-vault` wrote real files under ~/.flare-vault-test on this machine. Added flare-vault's own FLARE_VAULT_HOME_OVERRIDE (crates/flare-vault/src/paths.rs) and routed every home_dir() call through it; the main crate's with_temp_home() test helper now sets both overrides together. This also fixed a vacuous test (inject.rs's load_from_global_vault was asserting against a vault it never actually read, since VaultPaths::project resolved the real home instead of the test's temp dir). - Add regression tests for src/vault.rs, which previously had none despite containing the session-fallback, migration, and passphrase-caching logic: unlock/set/get roundtrip, get_secret propagating a real unseal error instead of Ok(None), migration pulling in a legacy gateway_secrets value on first unlock, and migration not clobbering an existing vault secret. - Document the remaining known trade-offs raised in review: the read-modify-write race window in vault/file.rs's locking, no session TTL by design, and why vault::get_passphrase() doesn't reuse auth_crypt::get_passphrase() despite sharing the same env var.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/vault.rs (1)
110-117: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winZeroize the legacy plaintext during migration.
src/vault.rs:110-117materializes the decrypted legacy secret as a plainString; wrap it inZeroizing<String>and passvalue.as_str()so the temporary plaintext is wiped on drop.🤖 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.rs` around lines 110 - 117, Update the legacy migration flow around set_secret_value to store the decrypted UTF-8 secret in Zeroizing<String> instead of a plain String, then pass value.as_str() when setting the secret. Preserve the existing continue behavior for decryption or UTF-8 failures while ensuring the temporary plaintext is wiped on drop.
🤖 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/flare-vault/src/paths.rs`:
- Around line 7-11: Update home_dir so it no longer falls back to
PathBuf::from(".") when dirs::home_dir() returns None. Preserve
FLARE_VAULT_HOME_OVERRIDE handling, and change the API and callers as needed to
propagate an explicit home-resolution error for unavailable home directories.
In `@src/paths.rs`:
- Around line 110-123: Update the cleanup test associated with
ResetHomeOverrideOnDrop to assert that FLARE_VAULT_HOME_OVERRIDE is also unset
after cleanup, alongside the existing AGENTFLARE_HOME_OVERRIDE assertion.
Preserve the current assertions and test behavior.
---
Outside diff comments:
In `@src/vault.rs`:
- Around line 110-117: Update the legacy migration flow around set_secret_value
to store the decrypted UTF-8 secret in Zeroizing<String> instead of a plain
String, then pass value.as_str() when setting the secret. Preserve the existing
continue behavior for decryption or UTF-8 failures while ensuring the temporary
plaintext is wiped on drop.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df4376a7-8658-4aab-be7b-db0c366ffddd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
Cargo.tomlcrates/flare-vault/src/inject.rscrates/flare-vault/src/lib.rscrates/flare-vault/src/paths.rscrates/flare-vault/src/session/file_cache.rscrates/flare-vault/src/session/mod.rscrates/flare-vault/src/vault/file.rscrates/flare-vault/src/vault/manager.rssrc/github/auth.rssrc/mcp_server.rssrc/paths.rssrc/vault.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/flare-vault/src/lib.rs
- Cargo.toml
- src/github/auth.rs
- crates/flare-vault/src/session/mod.rs
- crates/flare-vault/src/session/file_cache.rs
- crates/flare-vault/src/vault/file.rs
- crates/flare-vault/src/vault/manager.rs
Comment on lines
+7
to
+11
| pub fn home_dir() -> PathBuf { | ||
| if let Ok(p) = std::env::var("FLARE_VAULT_HOME_OVERRIDE") { | ||
| return PathBuf::from(p); | ||
| } | ||
| dirs::home_dir().unwrap_or_else(|| PathBuf::from(".")) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target file with line numbers.
sed -n '1,220p' crates/flare-vault/src/paths.rs | cat -n
printf '\n---\n'
# Find references to home_dir and the override env var.
rg -n "home_dir\(|FLARE_VAULT_HOME_OVERRIDE|vault state|file-cache|cache" crates/flare-vault -SRepository: getappz/agentflare
Length of output: 6514
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' crates/flare-vault/src/session/file_cache.rs | cat -n
printf '\n---\n'
sed -n '1,200p' crates/flare-vault/src/vault/manager.rs | cat -n
printf '\n---\n'
sed -n '1,180p' crates/flare-vault/src/vault/file.rs | cat -nRepository: getappz/agentflare
Length of output: 14508
Avoid falling back to . for vault storage. If dirs::home_dir() is unavailable, the global vault and session cache end up under the current working directory, which can place secrets in a repo or shared workspace. Return an explicit home-resolution error instead.
🤖 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/flare-vault/src/paths.rs` around lines 7 - 11, Update home_dir so it
no longer falls back to PathBuf::from(".") when dirs::home_dir() returns None.
Preserve FLARE_VAULT_HOME_OVERRIDE handling, and change the API and callers as
needed to propagate an explicit home-resolution error for unavailable home
directories.
- Isolate the channels.rs "no token configured" test from the developer's real vault (with_temp_home). Without it, a configured+unlocked telegram_bot_token would make this test send a real Telegram message. - run_unlock --stdin now strips only the trailing line ending from the piped passphrase instead of trimming all surrounding whitespace, so a passphrase with meaningful leading/trailing characters isn't silently altered. - Zeroize the intermediate DEK byte buffer in open_vault, and the decrypted legacy secret in migrate_legacy_secrets -- both were plain, unwiped copies of key material/plaintext. - Reuse EncryptedBlob::to_bytes/from_bytes in encrypt_value/decrypt_value instead of hand-packing the same magic+nonce+ciphertext layout again. - inject::load_vault_env now propagates real read/decrypt failures instead of silently treating them the same as "not unlocked" -- a vault with no cached session is still a normal skip, but a corrupt file or decrypt failure once we do have a DEK now surfaces. vault_env logs a warning on failure instead of swallowing it via unwrap_or_default. - list_secrets() now returns Ok(vec![]) for a vault that doesn't exist yet, matching get_secret's existing "absent vault" handling, instead of erroring on every gateway-secret listing before first unlock. - with_temp_home's own cleanup test now asserts FLARE_VAULT_HOME_OVERRIDE is cleared too, not just AGENTFLARE_HOME_OVERRIDE. Not applied, with reasoning: - set_secret_value rejecting overwrites: would break `gateway secret set` used for rotation/updates, which needs update semantics. - store_session/clear_session returning Result: best-effort dual-backend caching (keyring + file) is intentional -- either succeeding is enough to unseal later, so partial-failure propagation doesn't add value here. - home_dir()/cache_dir() falling back to "." instead of erroring when dirs::home_dir() is unavailable: pre-existing pattern from before this PR's changes in multiple places; fixing it properly means threading Result through VaultPaths/session file-cache construction, a bigger API change for an extremely rare failure mode. Left as a follow-up.
Merged
3 tasks
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
flare-vault, a standalone crate implementing a local secrets vault: Argon2id-derived KEK, AES-256-GCM envelope encryption of a per-vault DEK, OS-keyring session caching (with an obfuscated file-cache fallback), and global/project vault scoping.channels::send_message,cli gateway secret,github::auth/init_auth, andmcp_server's gateway-secret resolution now go throughcrate::vaultinstead of the sqlite-backedgateway_secretsstore. Newagentflare vault unlock|lock|envCLI.Review fixes applied on top of the original change
vault::get_secretwas converting unseal failures (wrong/missing passphrase) intoOk(None), indistinguishable from "no secret configured" — the exact problemmcp_server::resolve_gateway_secrets's error handling was written to surface, but never actually could because the error was discarded one layer down. Now propagates the real error.gateway_secretssqlite table to the new vault, so any user with a previously-configuredgithub_token/telegram_bot_token/etc. would have it silently vanish after upgrading. Added a one-time, best-effort migration that runs on first unlock/unseal, using the already-known passphrase to decrypt legacy rows and re-encrypt them into the vault (never overwrites a name already present in the vault).create_vault/open_vaulthand-packed and hand-parsed themagic+nonce+ciphertextencrypted-DEK layout inline; consolidated intoEncryptedBlob::to_bytes/from_bytes.moddeclaration order inmain.rs(vaultwas sorted beforeupdate, breaking the file's otherwise-alphabetical ordering).Test plan
cargo build --workspace --all-featurescargo clippy --workspace --all-features -- -D warnings(clean)cargo test -p flare-vault(21/21 pass)cargo test --bin agentflare gateway_secrets/channels::(targeted, pass)cargo test --workspace --all-features(931 passed / 1 failed / 1 ignored — the failure,mcp_server::tests::action_tests::item_claim_response_includes_worktree_path, is unrelated to this change (git-worktree fixture test) and passes in isolation on both this branch and an unrelated clean-master worktree; pre-existing parallel-execution flakiness, not a regression)Summary by CodeRabbit