Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
f283b16
feat(cli): graceful fallback for keyring failures
sheikhlimon Nov 19, 2025
f22473d
CLI continuation after keyring fallback
sheikhlimon Nov 20, 2025
66fba65
fix: improve keyring fallback
sheikhlimon Nov 20, 2025
670f18c
refactor: repetitive keyring fallback code
sheikhlimon Nov 20, 2025
cc3265a
refactor: keyring and file storage logic to reduce duplication
sheikhlimon Nov 21, 2025
685602a
refactor: simplify keyring error handling in config storage
sheikhlimon Nov 21, 2025
2c1af23
refactor: remove duplicate secret storage error handling
sheikhlimon Nov 21, 2025
4a83e5a
fix: resolve all clippy warnings in secret storage
sheikhlimon Nov 21, 2025
194933b
rename store_secret_with_fallback to store_secret
sheikhlimon Dec 16, 2025
72b0bac
remove success logging from store_secret
sheikhlimon Dec 16, 2025
e3972ef
simplify keyring fallback warning and auto-disable
sheikhlimon Dec 16, 2025
3aaa013
ensure file storage actually writes secrets and creates directories
sheikhlimon Dec 16, 2025
8f7c2f5
replace Value::String with plain strings in set_param calls
sheikhlimon Dec 16, 2025
d0b511f
add one-time keyring fallback warning to backend
sheikhlimon Dec 16, 2025
e358cdb
remove frontend keyring fallback, restore original error message
sheikhlimon Dec 16, 2025
0cb267e
consolidate GOOSE_DISABLE_KEYRING setting to single location, fix clippy
sheikhlimon Dec 16, 2025
ad2ef73
eliminate duplicate keyring error handling and file path logic
sheikhlimon Dec 16, 2025
cc37d3b
Merge branch 'main' into feat/graceful-keyring-fallback
sheikhlimon Jan 5, 2026
98808fa
Merge branch 'main' into feat/graceful-keyring-fallback
sheikhlimon Jan 6, 2026
01986c9
handle FallbackToFileStorage when setting provider secrets for first …
sheikhlimon Jan 6, 2026
2cbda18
remove misleading comment about env var checking
sheikhlimon Jan 6, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions crates/goose-cli/src/commands/configure.rs
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,7 @@ fn select_model_from_list(
fn try_store_secret(config: &Config, key_name: &str, value: String) -> anyhow::Result<bool> {
match config.set_secret(key_name, &value) {
Ok(_) => Ok(true),
Err(ConfigError::FallbackToFileStorage) => Ok(true),
Err(e) => {
cliclack::outro(style(format!(
"Failed to store {} securely: {}. Please ensure your system's secure storage is accessible. Alternatively you can run with GOOSE_DISABLE_KEYRING=true or set the key in your environment variables",
Expand Down Expand Up @@ -500,7 +501,6 @@ pub async fn configure_provider_dialog() -> anyhow::Result<bool> {
}
}
None => {
// No env var, check config/secret storage
let existing: Result<String, _> = if key.secret {
config.get_secret(&key.name)
} else {
Expand Down Expand Up @@ -565,7 +565,9 @@ pub async fn configure_provider_dialog() -> anyhow::Result<bool> {
};

if key.secret {
config.set_secret(&key.name, &value)?;
if !try_store_secret(config, &key.name, value)? {
return Ok(false);
}
} else {
config.set_param(&key.name, &value)?;
}
Expand Down Expand Up @@ -734,7 +736,7 @@ fn prompt_extension_name(placeholder: &str) -> anyhow::Result<String> {
}

fn collect_env_vars() -> anyhow::Result<(HashMap<String, String>, Vec<String>)> {
let mut envs = HashMap::new();
let envs = HashMap::new();
let mut env_keys = Vec::new();
let config = Config::global();

Expand All @@ -751,12 +753,10 @@ fn collect_env_vars() -> anyhow::Result<(HashMap<String, String>, Vec<String>)>
.mask('▪')
.interact()?;

match config.set_secret(&key, &value) {
Ok(_) => env_keys.push(key),
Err(_) => {
envs.insert(key, value);
}
if !try_store_secret(config, &key, value)? {
return Err(anyhow::anyhow!("Failed to store secret"));
}
env_keys.push(key);

if !cliclack::confirm("Add another environment variable?").interact()? {
break;
Expand Down
11 changes: 9 additions & 2 deletions crates/goose-cli/src/recipes/recipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,15 @@ pub fn collect_missing_secrets(requirements: &[SecretRequirement]) -> Result<()>
.unwrap_or_else(|_| String::new());

if !value.trim().is_empty() {
config.set_secret(&req.key, &value)?;
println!("✅ Secret stored securely for {}", req.extension_name);
if let Err(e) = config.set_secret(&req.key, &value) {
println!("⚠️ Failed to store secret in secure storage: {}. Secret available for this session only.", e);
println!(
" Consider setting {} as an environment variable for future use.",
req.key
);
} else {
println!("✅ Secret stored securely for {}", req.extension_name);
}
Comment thread
sheikhlimon marked this conversation as resolved.
} else {
println!("⏭️ Skipped {} for {}", req.key, req.extension_name);
}
Expand Down
135 changes: 114 additions & 21 deletions crates/goose/src/config/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@ pub enum ConfigError {
KeyringError(String),
#[error("Failed to lock config file: {0}")]
LockError(String),
#[error("Secret stored using file-based fallback")]
FallbackToFileStorage,
Comment thread
sheikhlimon marked this conversation as resolved.
}

impl From<serde_json::Error> for ConfigError {
Expand Down Expand Up @@ -540,30 +542,25 @@ impl Config {
pub fn all_secrets(&self) -> Result<HashMap<String, Value>, ConfigError> {
match &self.secrets {
SecretStorage::Keyring { service } => {
let entry = Entry::new(service, KEYRING_USERNAME)?;
let result =
self.handle_keyring_operation(|entry| entry.get_password(), service, None);

match entry.get_password() {
match result {
Ok(content) => {
let values: HashMap<String, Value> = serde_json::from_str(&content)?;
Ok(values)
}
Err(keyring::Error::NoEntry) => Ok(HashMap::new()),
Err(e) => Err(ConfigError::KeyringError(e.to_string())),
}
}
SecretStorage::File { path } => {
if path.exists() {
let file_content = std::fs::read_to_string(path)?;
let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
let json_value: Value = serde_json::to_value(yaml_value)?;
match json_value {
Value::Object(map) => Ok(map.into_iter().collect()),
_ => Ok(HashMap::new()),
Err(ConfigError::FallbackToFileStorage) => self.fallback_to_file_storage(),
Err(ConfigError::KeyringError(msg))
if msg.contains("No entry found")
|| msg.contains("No matching entry found") =>
{
Ok(HashMap::new())
}
} else {
Ok(HashMap::new())
Err(e) => Err(e),
}
}
SecretStorage::File { path } => self.read_secrets_from_file(path),
}
}

Expand Down Expand Up @@ -778,8 +775,11 @@ impl Config {
match &self.secrets {
SecretStorage::Keyring { service } => {
let json_value = serde_json::to_string(&values)?;
let entry = Entry::new(service, KEYRING_USERNAME)?;
entry.set_password(&json_value)?;
self.handle_keyring_operation(
|entry| entry.set_password(&json_value),
service,
Some(&values),
)?;
}
SecretStorage::File { path } => {
let yaml_value = serde_yaml::to_string(&values)?;
Expand Down Expand Up @@ -809,8 +809,11 @@ impl Config {
match &self.secrets {
SecretStorage::Keyring { service } => {
let json_value = serde_json::to_string(&values)?;
let entry = Entry::new(service, KEYRING_USERNAME)?;
entry.set_password(&json_value)?;
self.handle_keyring_operation(
|entry| entry.set_password(&json_value),
service,
Some(&values),
)?;
}
SecretStorage::File { path } => {
let yaml_value = serde_yaml::to_string(&values)?;
Expand All @@ -819,6 +822,97 @@ impl Config {
};
Ok(())
}

/// Read secrets from a YAML file
fn read_secrets_from_file(&self, path: &Path) -> Result<HashMap<String, Value>, ConfigError> {
if path.exists() {
let file_content = std::fs::read_to_string(path)?;
let yaml_value: serde_yaml::Value = serde_yaml::from_str(&file_content)?;
let json_value: Value = serde_json::to_value(yaml_value)?;
match json_value {
Value::Object(map) => Ok(map.into_iter().collect()),
_ => Ok(HashMap::new()),
}
} else {
Ok(HashMap::new())
}
}

/// Get the path to the secrets storage file
fn secrets_file_path() -> PathBuf {
Paths::config_dir().join("secrets.yaml")
}

/// Perform fallback to file storage when keyring is unavailable
fn fallback_to_file_storage(&self) -> Result<HashMap<String, Value>, ConfigError> {
let path = Self::secrets_file_path();
self.read_secrets_from_file(&path)
}

/// Write secrets to file storage (used for fallback)
fn write_secrets_to_file(&self, values: &HashMap<String, Value>) -> Result<(), ConfigError> {
std::fs::create_dir_all(Paths::config_dir())?;
let path = Self::secrets_file_path();
let yaml_value = serde_yaml::to_string(values)?;
std::fs::write(path, yaml_value)?;
Ok(())
}

/// Check if an error string indicates a keyring availability issue that should trigger fallback
fn is_keyring_availability_error(&self, error_str: &str) -> bool {
error_str.contains("keyring")
|| error_str.contains("DBus error")
|| error_str.contains("org.freedesktop.secrets")
|| error_str.contains("couldn't access platform secure storage")
}

/// Get a keyring entry for the specified service
fn get_keyring_entry(service: &str) -> Result<keyring::Entry, keyring::Error> {
Entry::new(service, KEYRING_USERNAME)
}

/// Handle keyring errors with automatic fallback to file storage
fn handle_keyring_fallback_error<T>(
&self,
keyring_err: &keyring::Error,
fallback_values: Option<&HashMap<String, Value>>,
) -> Result<T, ConfigError> {
if self.is_keyring_availability_error(&keyring_err.to_string()) {
std::env::set_var("GOOSE_DISABLE_KEYRING", "1");
tracing::warn!("Keyring unavailable. Using file storage for secrets.");

if let Some(values) = fallback_values {
self.write_secrets_to_file(values)?;
Err(ConfigError::FallbackToFileStorage)
} else {
Err(ConfigError::FallbackToFileStorage)
}
} else {
Err(ConfigError::KeyringError(keyring_err.to_string()))
}
}

/// Handle keyring operation with automatic fallback to file storage
fn handle_keyring_operation<T>(
&self,
operation: impl FnOnce(keyring::Entry) -> Result<T, keyring::Error>,
service: &str,
fallback_values: Option<&HashMap<String, Value>>,
) -> Result<T, ConfigError> {
// Try to get the keyring entry and perform the operation
let entry = match Self::get_keyring_entry(service) {
Ok(entry) => entry,
Err(keyring_err) => {
return self.handle_keyring_fallback_error(&keyring_err, fallback_values);
}
};

// Perform the operation
match operation(entry) {
Ok(result) => Ok(result),
Err(keyring_err) => self.handle_keyring_fallback_error(&keyring_err, fallback_values),
}
}
}

config_value!(CLAUDE_CODE_COMMAND, OsString, "claude");
Expand Down Expand Up @@ -878,7 +972,6 @@ mod tests {
use super::*;
use serial_test::serial;
use tempfile::NamedTempFile;

#[test]
fn test_basic_config() -> Result<(), ConfigError> {
let config = new_test_config();
Expand Down