From e13fe56e56e0a977e40ab0f846b690e1a32be8c3 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 12 Jun 2026 08:10:09 -0700 Subject: [PATCH 1/4] feat: injectable credential storage + per-CLI config file Decouple credential storage from PkceAuthProvider behind a `CredentialStorage` trait with keyring/file/auto backends. The mode is selectable via a config-file key, the `${PREFIX}_CREDENTIAL_STORE` env var, and a `--credential-store` global flag (precedence: explicit builder > flag > env > config > default `keyring`), giving WSL/headless users a way to disable the system keychain. Default behavior is unchanged; `with_file_fallback` is deprecated in favor of `with_credential_store`. Add a general per-application TOML config file (`ConfigFile`) that the engine shares with consumers: engine settings live in reserved top-level tables (`[credentials]`), consumers own every other table and read them via `CommandContext::config()` / `ModuleContext::config()`. An opt-in built-in `config get/set/path/list` command group reads and writes it (format-preserving via toml_edit; `config set` is dry-run aware and validates engine keys). Factor shared filesystem/path primitives (`config_base_dir`, `is_safe_path_component`, atomic `write_string_atomic`) into a new `fs` module reused by both config and credential storage. Co-Authored-By: Claude Opus 4.8 --- Cargo.lock | 71 ++- Cargo.toml | 2 + docs/auth.md | 29 +- docs/concepts.md | 68 ++- src/auth/mod.rs | 5 + src/auth/pkce.rs | 855 +++++++++---------------------- src/auth/storage.rs | 534 +++++++++++++++++++ src/cli.rs | 61 +++ src/command.rs | 12 + src/config.rs | 788 ++++++++++++++++++++++++++++ src/config_commands.rs | 104 ++++ src/flags.rs | 37 +- src/fs.rs | 249 +++++++++ src/lib.rs | 18 +- src/middleware.rs | 8 + src/module.rs | 8 + tests/config_file.rs | 199 +++++++ tests/credential_store_config.rs | 239 +++++++++ tests/exhaustive_public_api.rs | 3 + tests/foundation.rs | 1 + 20 files changed, 2666 insertions(+), 625 deletions(-) create mode 100644 src/auth/storage.rs create mode 100644 src/config.rs create mode 100644 src/config_commands.rs create mode 100644 src/fs.rs create mode 100644 tests/config_file.rs create mode 100644 tests/credential_store_config.rs diff --git a/Cargo.lock b/Cargo.lock index 30f832e..81378d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -417,6 +417,8 @@ dependencies = [ "tempfile", "thiserror", "tokio", + "toml", + "toml_edit 0.22.27", "tracing", "url", "zeroize", @@ -1457,7 +1459,7 @@ version = "3.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e67ba7e9b2b56446f1d419b1d807906278ffa1a658a8a5d8a39dcb1f5a78614f" dependencies = [ - "toml_edit", + "toml_edit 0.25.8+spec-1.1.0", ] [[package]] @@ -1924,6 +1926,15 @@ dependencies = [ "syn", ] +[[package]] +name = "serde_spanned" +version = "0.6.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bf41e0cfaf7226dca15e8197172c295a782857fcb97fad1808a166870dee75a3" +dependencies = [ + "serde", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -2157,6 +2168,27 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "0.8.23" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dc1beb996b9d83529a9e75c17a1686767d148d70663143c7854d8b4a09ced362" +dependencies = [ + "serde", + "serde_spanned", + "toml_datetime 0.6.11", + "toml_edit 0.22.27", +] + +[[package]] +name = "toml_datetime" +version = "0.6.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "22cddaf88f4fbc13c51aebbf5f8eceb5c7c5a9da2ac40a13519eb5b0a0e8f11c" +dependencies = [ + "serde", +] + [[package]] name = "toml_datetime" version = "1.1.1+spec-1.1.0" @@ -2166,6 +2198,20 @@ dependencies = [ "serde_core", ] +[[package]] +name = "toml_edit" +version = "0.22.27" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "41fe8c660ae4257887cf66394862d21dbca4a6ddd26f04a3560410406a2f819a" +dependencies = [ + "indexmap", + "serde", + "serde_spanned", + "toml_datetime 0.6.11", + "toml_write", + "winnow 0.7.15", +] + [[package]] name = "toml_edit" version = "0.25.8+spec-1.1.0" @@ -2173,9 +2219,9 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "16bff38f1d86c47f9ff0647e6838d7bb362522bdf44006c7068c2b1e606f1f3c" dependencies = [ "indexmap", - "toml_datetime", + "toml_datetime 1.1.1+spec-1.1.0", "toml_parser", - "winnow", + "winnow 1.0.3", ] [[package]] @@ -2184,9 +2230,15 @@ version = "1.1.0+spec-1.1.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2334f11ee363607eb04df9b8fc8a13ca1715a72ba8662a26ac285c98aabb4011" dependencies = [ - "winnow", + "winnow 1.0.3", ] +[[package]] +name = "toml_write" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5d99f8c9a7727884afe522e9bd5edbfc91a3312b36a77b5fb8926e4c31a41801" + [[package]] name = "tower" version = "0.5.3" @@ -2283,7 +2335,7 @@ checksum = "f2f6fb2847f6742cd76af783a2a2c49e9375d0a111c7bef6f71cd9e738c72d6e" dependencies = [ "memoffset", "tempfile", - "windows-sys 0.60.2", + "windows-sys 0.61.2", ] [[package]] @@ -2715,6 +2767,15 @@ version = "0.53.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" +[[package]] +name = "winnow" +version = "0.7.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "df79d97927682d2fd8adb29682d1140b343be4ac0f08fd68b7765d9c059d3945" +dependencies = [ + "memchr", +] + [[package]] name = "winnow" version = "1.0.3" diff --git a/Cargo.toml b/Cargo.toml index 1e96f02..773b097 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,6 +34,8 @@ schemars = { version = "1.2.1", features = ["derive"] } serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.145" thiserror = "2.0.17" +toml = "0.8" +toml_edit = "0.22" tokio = { version = "1.48.0", features = ["fs", "io-std", "io-util", "macros", "net", "process", "rt-multi-thread", "signal", "sync", "time"] } tracing = "0.1.43" diff --git a/docs/auth.md b/docs/auth.md index 0214aef..a93402e 100644 --- a/docs/auth.md +++ b/docs/auth.md @@ -189,8 +189,7 @@ The provider manages the full browser-based login flow: 2. Starts a local TCP server on `127.0.0.1` to receive the OAuth callback. 3. Opens the system browser to the authorization endpoint. 4. Exchanges the returned code for tokens at the token endpoint. -5. Stores tokens in the system keychain via the `keyring` crate (macOS Keychain, Linux Secret - Service, or the platform's encrypted file fallback). +5. Persists tokens through a `CredentialStorage` backend (the system keychain by default; see [Credential Storage](#credential-storage)). 6. Refreshes expired tokens automatically using the stored refresh token. ```rust @@ -232,6 +231,32 @@ values. The prefix is the provider name uppercased with hyphens replaced by unde For a provider named `"primary"`, the variables are `PRIMARY_OAUTH_CLIENT_ID`, `PRIMARY_OAUTH_AUTH_URL`, and `PRIMARY_OAUTH_TOKEN_URL`. +## Credential Storage + +Tokens are persisted through the injectable `CredentialStorage` trait rather than a hard-wired keychain. Each credential is keyed by `CredentialKey { app_id, provider, env }`. Three built-in backends correspond to the `CredentialStore` modes: + +- **`keyring`** (`KeyringStorage`, default) — system keychain only (macOS Keychain, Linux Secret Service, Windows Credential Manager). A keychain failure is a hard error; no file is written. +- **`auto`** (`AutoStorage`) — try the keychain, and transparently fall back to an unencrypted file when the keychain backend is unavailable. +- **`file`** (`FileStorage`) — never contact the keychain. Tokens are written as **unencrypted JSON** to `//credentials/-.json` (`0600` on Unix), where `` is `$XDG_CONFIG_HOME`, `$HOME/.config`, or `%APPDATA%`. + +### Selecting a mode + +`file` is the recommended escape hatch on **WSL and headless Linux**, where a Secret Service daemon is often missing or awkward to run. Operators can disable the keychain without code changes, with this precedence (highest first): + +1. `PkceAuthProvider::with_storage(...)` — inject a custom backend, or `PkceAuthProvider::with_credential_store(CredentialStore::File)` — force a built-in mode. +2. `--credential-store auto|keyring|file` global flag. +3. `${PREFIX}_CREDENTIAL_STORE` env var (e.g. `MY_CLI_CREDENTIAL_STORE=file`), where `${PREFIX}` is the **app id** uppercased with non-alphanumerics replaced by `_`. +4. `[credentials].store` in `//config.toml`: + ```toml + [credentials] + store = "file" + ``` +5. Default: `keyring`. + +> The escape-hatch trade-off: `file`/`auto` write credentials to disk **unencrypted** (owner-only permissions on Unix). Prefer the keychain where one is available. + +`with_file_fallback(bool)` is deprecated: `true` maps to `CredentialStore::Auto` and `false` to `CredentialStore::Keyring`. Use `with_credential_store` instead. + ## Dispatcher [`Dispatcher`](../src/auth/dispatcher.rs) routes auth calls by provider name: diff --git a/docs/concepts.md b/docs/concepts.md index 16b5b09..80c04fe 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -318,10 +318,7 @@ Auth providers implement the `AuthProvider` trait. Providers expose credential r status, logout, and environment-listing behavior. The framework includes: - `ExecProvider`, which invokes an external provider command using JSON stdin/stdout. -- `PkceAuthProvider` (requires the `pkce-auth` feature), a built-in browser-based OAuth 2.0 PKCE - flow that manages the local callback server, opens the system browser, and persists tokens in the - system keychain via the `keyring` crate. Auth URL, token URL, and client ID can be overridden via - environment variables at runtime. +- `PkceAuthProvider` (requires the `pkce-auth` feature), a built-in browser-based OAuth 2.0 PKCE flow that manages the local callback server, opens the system browser, and persists tokens through a pluggable credential-storage backend (system keychain by default). Auth URL, token URL, and client ID can be overridden via environment variables at runtime. - A `Dispatcher` that routes auth calls by provider name. Single-provider facades created from the dispatcher remain live views of the dispatcher, so transport injectors observe later provider registration or replacement. @@ -331,6 +328,69 @@ Command handlers receive `Option`. No-auth commands receive `None`. Provider process contracts and request injectors are described in [Authentication and Transport](auth.md). +## Credential Storage + +Auth providers persist credentials through the injectable `CredentialStorage` trait (`auth::storage`), keyed by `CredentialKey { app_id, provider, env }`. Three built-in backends map to the `CredentialStore` modes: + +| Mode | Backend | Behavior | +| --- | --- | --- | +| `keyring` (default) | `KeyringStorage` | System keychain only; failure is a hard error. | +| `auto` | `AutoStorage` | Keychain, with a transparent unencrypted-file fallback when the keychain backend is unavailable. | +| `file` | `FileStorage` | Never contacts the keychain; stores unencrypted JSON under the config base directory. | + +`File` is the escape hatch for environments where the system keychain is unavailable or impractical (headless Linux, WSL). The selected mode is resolved with the precedence: + +```text +PkceAuthProvider::with_storage / with_credential_store (explicit, highest) + > --credential-store flag + > ${PREFIX}_CREDENTIAL_STORE env var + > [credentials].store in config.toml + > keyring (default) +``` + +where `${PREFIX}` is the app id uppercased with non-alphanumerics replaced by `_` (e.g. `godaddy` → `GODADDY_CREDENTIAL_STORE`). Providers resolve their backend lazily, so `--schema` and `--dry-run` build no storage and never touch the keychain. A custom backend (for example an in-memory store in tests, or a remote secret manager) can be injected with `PkceAuthProvider::with_storage`. + +## Configuration File + +cli-engine provides a single per-application TOML config file that **consumer CLIs share with the engine**. It lives at `//config.toml`, where `` is `$XDG_CONFIG_HOME`, `$HOME/.config`, or `%APPDATA%`. Loading is best-effort: a missing/unreadable/malformed file yields an empty config (a warning is logged for malformed) rather than failing the +command. + +Engine-reserved settings live in documented top-level tables (today just `[credentials]`); the consumer CLI owns **every other top-level table**: + +```toml +[credentials] # engine-reserved +store = "file" # "auto" | "keyring" | "file" + +[deploy] # consumer-owned +region = "us-west" +``` + +### Reading config + +The loaded file is exposed as a `ConfigFile` and surfaced everywhere it's useful — `toml` stays an internal detail, so access is typed: + +- In command handlers: `ctx.config().section::("deploy")?` +- In module registration: `module_ctx.config().section::(...)` +- Engine-reserved view: `ConfigFile::engine() -> EngineConfig` +- Whole-file into a consumer root type: `ConfigFile::deserialize::()` + +The file is loaded once at startup and cloned into each run's middleware, so reads are cheap. + +### Writing config (`config` command group) + +`CliConfig::with_config_commands()` mounts a built-in `config` group (filed under the admin help category), opt-in so it never collides with a consumer's own `config` noun: + +```text +mycli config path # print the file path +mycli config get deploy.region # read a dotted key +mycli config set deploy.region us-east # set + save (mutating; --dry-run aware) +mycli config list # print the whole file +``` + +`config set` is dry-run aware, parses the value as a bool/int/float when it looks like one (else a string), preserves existing comments and formatting (backed by `toml_edit`), and validates the engine-reserved `credentials.store` key. Programmatically, `ConfigFile::set` + `ConfigFile::save` do the same. + +The `config` module also exposes `load`, `resolve_credential_store`, and the pure `resolve_credential_store_with` for testing credential-store precedence without touching process state. + ## Authorization Authorization is provided by an `Authorizer` attached to middleware. The authorizer receives: diff --git a/src/auth/mod.rs b/src/auth/mod.rs index 7bcc347..befb684 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -19,6 +19,8 @@ pub mod exec; /// OAuth 2.0 PKCE auth provider (requires the `pkce-auth` feature). #[cfg(feature = "pkce-auth")] pub mod pkce; +/// Pluggable credential storage backends (keychain, file, auto). +pub mod storage; use async_trait::async_trait; @@ -32,6 +34,9 @@ pub use exec::{ ACTION_AUTHENTICATE, ACTION_LIST_ENVIRONMENTS, ACTION_LIST_REALMS, ACTION_LOGOUT, ACTION_STATUS, AuthnRequest, EnvironmentsResponse, ExecProvider, }; +#[cfg(feature = "pkce-auth")] +pub use storage::{AutoStorage, KeyringStorage}; +pub use storage::{CredentialKey, CredentialStorage, FileStorage, default_storage, storage_for}; use crate::Result; use crate::middleware::CommandMeta; diff --git a/src/auth/pkce.rs b/src/auth/pkce.rs index e16e9cf..4a743e2 100644 --- a/src/auth/pkce.rs +++ b/src/auth/pkce.rs @@ -1,15 +1,26 @@ //! OAuth 2.0 PKCE authentication provider. //! //! Implements the browser-based Authorization Code + PKCE flow (RFC 7636). -//! Tokens are stored in the system keychain via the `keyring` crate. -//! On headless or WSL environments where a keychain daemon is unavailable, an -//! opt-in file fallback can be enabled with `PkceAuthProvider::with_file_fallback`; -//! tokens are then written as **unencrypted JSON** to -//! `//credentials/-.json` (at most `0600` on Unix; the -//! process umask may make the mode more restrictive), -//! where `` is `$XDG_CONFIG_HOME`, `$HOME/.config`, or `%APPDATA%`. -//! Only enable the fallback when the deployment environment lacks a reliable -//! keychain and the security tradeoff is acceptable. +//! Tokens are persisted through a pluggable [`CredentialStorage`] backend +//! (see [`crate::auth::storage`]) rather than a hard-wired keychain. By default +//! the backend is resolved from configuration — the `--credential-store` flag, +//! the `${PREFIX}_CREDENTIAL_STORE` env var, the engine config file, or the +//! `keyring` default — so an operator can disable the system keychain on +//! environments where it is unavailable (headless Linux, WSL) without code +//! changes. The three modes are: +//! +//! - `Keyring` (default): system keychain only. +//! - `Auto`: keychain with a transparent unencrypted-file fallback when the +//! keychain backend is unavailable. +//! - `File`: never contact the keychain; store unencrypted JSON under +//! `//credentials/-.json`, where +//! `` is `$XDG_CONFIG_HOME`, `$HOME/.config`, or `%APPDATA%`. +//! +//! See [`CredentialStore`](crate::config::CredentialStore). A backend can also be +//! injected directly with +//! [`PkceAuthProvider::with_storage`](crate::auth::pkce::PkceAuthProvider::with_storage) +//! or forced with +//! [`PkceAuthProvider::with_credential_store`](crate::auth::pkce::PkceAuthProvider::with_credential_store). //! //! # Setup //! @@ -55,7 +66,14 @@ use sha2::{Digest, Sha256}; use tokio::sync::RwLock; use zeroize::{Zeroize, ZeroizeOnDrop}; -use crate::{Credential, Result, auth::AuthProvider, auth::CredentialRequest, error::CliCoreError}; +use crate::{ + Credential, Result, + auth::AuthProvider, + auth::CredentialRequest, + auth::storage::{CredentialKey, CredentialStorage, default_storage, storage_for}, + config::CredentialStore, + error::CliCoreError, +}; const REDIRECT_PORT_DEFAULT: u16 = 7443; const TOKEN_EXPIRY_BUFFER_SECS: i64 = 30; @@ -121,7 +139,15 @@ pub struct PkceAuthProvider { redirect_uri: Option, app_id: String, env_prefix: String, - allow_file_fallback: bool, + /// Explicit storage backend injected via [`PkceAuthProvider::with_storage`]. + /// Wins over `store_mode` and the config-driven default. + storage_override: Option>, + /// Explicit storage mode from [`PkceAuthProvider::with_credential_store`]. + /// Forces a built-in backend, bypassing flag/env/config resolution. + store_mode: Option, + /// Lazily-resolved storage backend. Built on first use so `--schema` / + /// `--dry-run` (which never resolve a credential) touch no keychain/config. + storage: tokio::sync::OnceCell>, /// Prioritized JWT claim names used to derive `Credential.identity` from the /// decoded access-token payload. First non-empty string claim wins. identity_claims: Vec, @@ -161,7 +187,9 @@ impl PkceAuthProvider { redirect_uri: None, app_id: String::new(), env_prefix, - allow_file_fallback: false, + storage_override: None, + store_mode: None, + storage: tokio::sync::OnceCell::new(), identity_claims: DEFAULT_IDENTITY_CLAIMS .iter() .map(|claim| (*claim).to_owned()) @@ -204,16 +232,48 @@ impl PkceAuthProvider { self } + /// Injects a custom credential storage backend. + /// + /// Takes precedence over [`with_credential_store`](Self::with_credential_store) + /// and the config-driven default. Use this to plug in a bespoke + /// [`CredentialStorage`] (for example an in-memory store in tests, or a + /// remote secret manager). + #[must_use] + pub fn with_storage(mut self, storage: Arc) -> Self { + self.storage_override = Some(storage); + self + } + + /// Forces a built-in credential storage mode, bypassing the + /// flag/env/config resolution. + /// + /// Use [`CredentialStore::File`] to skip the system keychain entirely (the + /// escape hatch for headless Linux / WSL), [`CredentialStore::Auto`] for a + /// keychain-with-file-fallback, or [`CredentialStore::Keyring`] for + /// keychain-only. When unset, the mode is resolved per + /// [`crate::config::resolve_credential_store`]. + #[must_use] + pub fn with_credential_store(mut self, mode: CredentialStore) -> Self { + self.store_mode = Some(mode); + self + } + /// Enables a file-based fallback when the system keychain is unavailable /// (e.g. headless Linux / WSL without a running secret-service daemon). /// - /// Disabled by default. The original TypeScript CLI had no file fallback; - /// enable only when you have confirmed the deployment environment lacks a - /// reliable keychain and you accept unencrypted credentials on disk. + /// `true` maps to [`CredentialStore::Auto`] and `false` to + /// [`CredentialStore::Keyring`]. #[must_use] - pub fn with_file_fallback(mut self, enabled: bool) -> Self { - self.allow_file_fallback = enabled; - self + #[deprecated( + since = "0.3.0", + note = "use with_credential_store(CredentialStore::Auto) or (CredentialStore::Keyring)" + )] + pub fn with_file_fallback(self, enabled: bool) -> Self { + self.with_credential_store(if enabled { + CredentialStore::Auto + } else { + CredentialStore::Keyring + }) } /// Overrides the prioritized JWT claim names used to derive @@ -293,222 +353,61 @@ impl PkceAuthProvider { Ok((port, path)) } - fn keychain_service(&self, env: &str) -> String { - if self.app_id.is_empty() { - format!("{}/{}", self.name, env) - } else { - format!("{}/{}/{}", self.app_id, self.name, env) - } + /// Builds the storage key for this provider and `env`. + fn credential_key<'key>(&'key self, env: &'key str) -> CredentialKey<'key> { + CredentialKey::new(&self.app_id, &self.name, env) } - fn keychain_user() -> &'static str { - "token" - } - - /// Returns the path to the fallback credential file for this provider/env. + /// Returns the credential storage backend, resolving and caching it on first + /// use. Precedence: an injected [`with_storage`](Self::with_storage) backend, + /// then a forced [`with_credential_store`](Self::with_credential_store) mode, + /// then the config-driven [`default_storage`]. /// - /// Used when the system keychain is unavailable (e.g. WSL, headless Linux). - fn credential_file_path(&self, env: &str) -> Option { - let app = if self.app_id.is_empty() { - &self.name - } else { - &self.app_id - }; - if !is_safe_path_component(app) - || !is_safe_path_component(&self.name) - || !is_safe_path_component(env) - { - tracing::warn!( - app, - name = self.name, - env, - "refusing credential path with unsafe component" - ); - return None; - } - let base = config_base_dir()?; - Some( - base.join(app) - .join("credentials") - .join(format!("{}-{}.json", self.name, env)), - ) + /// Resolution is lazy so paths that never resolve a credential (`--schema`, + /// `--dry-run`) build no storage and touch neither the keychain nor config. + async fn storage(&self) -> &Arc { + self.storage + .get_or_init(async || { + if let Some(storage) = &self.storage_override { + storage.clone() + } else if let Some(mode) = self.store_mode { + storage_for(mode) + } else { + default_storage(&self.app_id) + } + }) + .await } - async fn load_token_from_keychain(&self, env: &str) -> Option { - let service = self.keychain_service(env); - let user = Self::keychain_user(); - - // None = backend error/unavailable; Some(None) = working but no entry. - let keychain_result = match tokio::task::spawn_blocking({ - let service = service.clone(); - move || keychain_read_blocking(&service, user) - }) - .await - { - Ok(result) => result, + /// Loads and deserializes the stored token for `env`, if present. + /// + /// On a corrupt/undecodable blob, best-effort deletes it (self-heal) and + /// returns `None` so the caller re-authenticates rather than looping on the + /// bad entry. + async fn load_stored(&self, env: &str) -> Option { + let key = self.credential_key(env); + let raw = self.storage().await.load(&key).await?; + match serde_json::from_str::(&raw) { + Ok(token) => Some(token), Err(e) => { - let reason = if e.is_cancelled() { - "cancelled" - } else { - "panicked" - }; - tracing::warn!(service, error = %e, reason, "keychain read task failed"); + tracing::warn!(env, error = %e, "stored token JSON invalid; clearing"); + self.storage().await.delete(&key).await; None } - }; - - match keychain_result { - Some(Some(ref json)) => { - match serde_json::from_str::(json) { - Ok(token) => return Some(token), - Err(e) => { - tracing::warn!(service, error = %e, "keychain token JSON invalid"); - // Best-effort delete the corrupt entry so subsequent runs - // don't repeat the warning. The keychain was reachable, so - // skip the file fallback and force re-auth. - let svc = service.clone(); - let usr = Self::keychain_user(); - if let Err(e) = tokio::task::spawn_blocking(move || { - if let Ok(entry) = keyring::Entry::new(&svc, usr) - && let Err(e) = entry.delete_credential() - && !matches!(e, keyring::Error::NoEntry) - { - tracing::warn!(service = %svc, error = %e, "failed to self-heal corrupt keychain entry"); - } - }) - .await - { - let reason = if e.is_cancelled() { "cancelled" } else { "panicked" }; - tracing::warn!(service, error = %e, reason, "keychain self-heal task failed"); - } - return None; - } - } - } - Some(None) => { - // Keychain is reachable but has no entry. The file is stale or absent; - // skip the file fallback and let the caller trigger a new login. - return None; - } - None => { - // Keychain backend unavailable — fall through to the file fallback. - } } - - if !self.allow_file_fallback { - return None; - } - let path = self.credential_file_path(env)?; - load_token_from_file(&path).await } - async fn save_token_to_keychain(&self, env: &str, token: &StoredToken) -> Result<()> { + /// Serializes and persists `token` for `env` via the storage backend. + async fn save_stored(&self, env: &str, token: &StoredToken) -> Result<()> { let json = serde_json::to_string(token).map_err(CliCoreError::from)?; - let service = self.keychain_service(env); - let user = Self::keychain_user(); - - let (keychain_saved, json) = match tokio::task::spawn_blocking({ - let service = service.clone(); - move || { - let saved = keychain_write_blocking(&service, user, &json); - (saved, json) - } - }) - .await - { - Ok(result) => result, - Err(e) => { - let reason = if e.is_cancelled() { - "cancelled" - } else { - "panicked" - }; - tracing::warn!(service, error = %e, reason, "keychain write task failed"); - // Re-serialize on task panic/cancel so the file-fallback path still has json. - let json = serde_json::to_string(token).map_err(CliCoreError::from)?; - (false, json) - } - }; - - if keychain_saved { - // Best-effort: remove any stale file-fallback token now that the - // keychain is working. Ignore NotFound; the file may never have existed. - if let Some(path) = self.credential_file_path(env) { - match tokio::fs::remove_file(&path).await { - Ok(()) => { - tracing::debug!(path = %path.display(), "removed stale file fallback after keychain write"); - } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => { - tracing::debug!(path = %path.display(), error = %e, "could not remove stale file fallback"); - } - } - } - return Ok(()); - } - if !self.allow_file_fallback { - return Err(CliCoreError::message( - "failed to save token to keychain and file fallback is disabled — \ - check logs for the underlying error, or ensure your system keychain \ - (e.g. gnome-keyring, macOS Keychain) is running and unlocked", - )); - } - let path = self - .credential_file_path(env) - .ok_or_else(|| CliCoreError::message("could not determine credential file path"))?; - tokio::task::spawn_blocking({ - let path = path.clone(); - move || write_token_file_blocking(path, json) - }) - .await - .map_err(|e| { - CliCoreError::message(format!( - "credential file write task {}: {e}", - if e.is_cancelled() { - "cancelled" - } else { - "panicked" - } - )) - })??; - tracing::debug!(path = %path.display(), "token saved to file fallback"); - Ok(()) + let key = self.credential_key(env); + self.storage().await.save(&key, &json).await } - async fn delete_token_from_keychain(&self, env: &str) { - let service = self.keychain_service(env); - let user = Self::keychain_user(); - let service_for_warn = service.clone(); - if let Err(e) = - tokio::task::spawn_blocking(move || match keyring::Entry::new(&service, user) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed on delete"); - } - Ok(entry) => match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) => { - tracing::warn!(service, error = %e, "keychain delete failed"); - } - }, - }) - .await - { - let reason = if e.is_cancelled() { - "cancelled" - } else { - "panicked" - }; - tracing::warn!(service = service_for_warn, error = %e, reason, "keychain delete task failed"); - } - if let Some(path) = self.credential_file_path(env) { - match tokio::fs::remove_file(&path).await { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "failed to delete credential file"); - } - } - } + /// Removes any stored token for `env` via the storage backend. + async fn delete_stored(&self, env: &str) { + let key = self.credential_key(env); + self.storage().await.delete(&key).await; } async fn cached_token(&self, env: &str) -> Option { @@ -536,7 +435,7 @@ impl PkceAuthProvider { if let Some(token) = self.cached_token(env).await { return Ok(Some(token)); } - if let Some(token) = self.load_token_from_keychain(env).await { + if let Some(token) = self.load_stored(env).await { if token.is_valid() { self.store_cached_token(env, token.clone()).await; return Ok(Some(token)); @@ -549,7 +448,7 @@ impl PkceAuthProvider { if refreshed.refresh_token.is_none() { refreshed.refresh_token = Some(refresh_token.to_owned()); } - self.save_token_to_keychain(env, &refreshed).await?; + self.save_stored(env, &refreshed).await?; self.store_cached_token(env, refreshed.clone()).await; return Ok(Some(refreshed)); } @@ -565,7 +464,7 @@ impl PkceAuthProvider { // this env — and only update the in-memory cache after a successful // save. This avoids destroying a still-valid token if the save fails // (e.g. keychain unavailable and file fallback disabled). - self.save_token_to_keychain(env, &token).await?; + self.save_stored(env, &token).await?; self.store_cached_token(env, token.clone()).await; Ok(token) } @@ -727,7 +626,7 @@ impl AuthProvider for PkceAuthProvider { } async fn status(&self, env: &str) -> Result { - let Some(token) = self.load_token_from_keychain(env).await else { + let Some(token) = self.load_stored(env).await else { return Err(CliCoreError::message(format!( "not logged in for environment {env:?}" ))); @@ -736,7 +635,7 @@ impl AuthProvider for PkceAuthProvider { } async fn logout(&self, env: &str) -> Result<()> { - self.delete_token_from_keychain(env).await; + self.delete_stored(env).await; let mut cache = self.cache.write().await; cache.remove(env); Ok(()) @@ -766,226 +665,6 @@ fn random_state() -> String { URL_SAFE_NO_PAD.encode(bytes) } -/// Resolves the base config directory from environment variables. -fn config_base_dir() -> Option { - std::env::var("XDG_CONFIG_HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - // On Windows prefer APPDATA over HOME/.config: HOME is often set by - // Git Bash/MSYS shells and would place credentials in a non-standard - // location. On all other platforms prefer XDG-conventional HOME/.config, - // falling back to APPDATA as a last resort if HOME is unset. - #[cfg(windows)] - { - std::env::var("APPDATA") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - .or_else(|| { - std::env::var("HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(|h| std::path::PathBuf::from(h).join(".config")) - }) - } - #[cfg(not(windows))] - { - std::env::var("HOME") - .ok() - .filter(|v| !v.is_empty()) - .map(|h| std::path::PathBuf::from(h).join(".config")) - .or_else(|| { - std::env::var("APPDATA") - .ok() - .filter(|v| !v.is_empty()) - .map(std::path::PathBuf::from) - }) - } - }) - // Reject relative paths: a relative XDG_CONFIG_HOME/APPDATA/HOME would - // silently place credentials relative to the current working directory. - .filter(|p| p.is_absolute()) -} - -/// Reads a token JSON string from the system keychain. Sync; call inside `spawn_blocking`. -/// -/// Returns `Some(Some(json))` when a credential is found, `Some(None)` when the keychain -/// is reachable but has no entry, and `None` when the keychain backend is unavailable or -/// returns an unexpected error. Callers use `None` to decide whether to try the file fallback. -fn keychain_read_blocking(service: &str, user: &str) -> Option> { - match keyring::Entry::new(service, user) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - None - } - Ok(entry) => match entry.get_password() { - Err(keyring::Error::NoEntry) => { - tracing::debug!(service, "no stored token in keychain"); - Some(None) - } - Err(e) => { - tracing::warn!(service, error = %e, "keychain read failed"); - None - } - Ok(json) => Some(Some(json)), - }, - } -} - -/// Writes a token JSON string to the system keychain. Sync; call inside `spawn_blocking`. -fn keychain_write_blocking(service: &str, user: &str, json: &str) -> bool { - match keyring::Entry::new(service, user) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain entry creation failed"); - false - } - Ok(entry) => match entry.set_password(json) { - Err(e) => { - tracing::warn!(service, error = %e, "keychain write failed"); - false - } - Ok(()) => { - tracing::debug!(service, "token saved to keychain"); - true - } - }, - } -} - -/// Reads and parses a [`StoredToken`] from the file fallback path. -async fn load_token_from_file(path: &std::path::Path) -> Option { - let json = match tokio::fs::read_to_string(path).await { - Ok(s) => s, - Err(e) if e.kind() == std::io::ErrorKind::NotFound => return None, - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "file fallback read failed"); - return None; - } - }; - match serde_json::from_str(&json) { - Ok(token) => { - tracing::debug!(path = %path.display(), "loaded token from file fallback"); - Some(token) - } - Err(e) => { - tracing::warn!(path = %path.display(), error = %e, "file fallback token JSON invalid"); - // Best-effort delete: a permanently corrupt file causes repeated - // warnings and PKCE flows on every run until manually removed. - tokio::fs::remove_file(path).await.ok(); - None - } - } -} - -/// Writes `json` to `path` via a uniquely-named temp file then renames it into place. -/// On Unix the rename is atomic. On Windows it is best-effort (`MoveFileExW` with -/// `MOVEFILE_REPLACE_EXISTING`): it replaces an existing destination but is not -/// crash-atomic. Sync; call inside `spawn_blocking`. -fn write_token_file_blocking(path: std::path::PathBuf, json: String) -> Result<()> { - if let Some(parent) = path.parent() { - std::fs::create_dir_all(parent).map_err(|e| { - CliCoreError::message(format!("failed to create credential directory: {e}")) - })?; - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt as _; - if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) - { - // The credential file itself is always 0o600; failing to - // restrict the parent directory is a defence-in-depth miss, - // not a confidentiality breach. - tracing::debug!( - path = %parent.display(), - error = %e, - "could not restrict credential directory permissions" - ); - } - } - } - let rand_id = rand::random::(); - let tmp_path = path.with_file_name(format!( - "{}.{rand_id:08x}.tmp", - path.file_stem().and_then(|s| s.to_str()).unwrap_or("cred"), - )); - write_token_tmp(&tmp_path, &json)?; - if let Err(e) = std::fs::rename(&tmp_path, &path) { - std::fs::remove_file(&tmp_path).ok(); - return Err(CliCoreError::message(format!( - "failed to finalize credential file {}: {e}", - path.display() - ))); - } - Ok(()) -} - -/// Opens `tmp_path` with `O_CREAT|O_EXCL` and writes `json`. -/// Sets mode `0o600` on Unix so credentials are never world-readable. -fn write_token_tmp(tmp_path: &std::path::Path, json: &str) -> Result<()> { - use std::io::Write as _; - let mut opts = std::fs::OpenOptions::new(); - opts.write(true).create_new(true); - #[cfg(unix)] - { - use std::os::unix::fs::OpenOptionsExt as _; - opts.mode(0o600); - } - let mut file = opts.open(tmp_path).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - tmp_path.display() - )) - })?; - file.write_all(json.as_bytes()).map_err(|e| { - CliCoreError::message(format!( - "failed to write credentials to {}: {e}", - tmp_path.display() - )) - }) -} - -/// Returns true only when `s` is a single, non-traversal path component that is -/// valid on all supported platforms. -/// -/// Rejects: -/// - empty strings, `.`, and `..` -/// - strings containing `/` or `\` (path separators on any platform) -/// - Windows-forbidden filename characters: `: * ? " < > |` -/// - ASCII control characters (bytes 0x00–0x1F) and the DEL character (0x7F) -/// - leading or trailing space (leading space is invisible in directory listings) -/// - trailing `.` (valid on Unix but rejected by Windows) -/// - Windows reserved device names (`CON`, `NUL`, `COM1`, etc.) with or without extension -fn is_safe_path_component(s: &str) -> bool { - // '/' is listed explicitly because Path::components() silently strips trailing - // slashes — "prod/" parses as a single Normal("prod") component and would - // otherwise pass the components check below. - const FORBIDDEN: &[char] = &['/', '\\', ':', '*', '?', '"', '<', '>', '|']; - if s.contains(FORBIDDEN) || s.bytes().any(|b| b < 0x20 || b == 0x7F) { - return false; - } - if s.starts_with(' ') || s.ends_with('.') || s.ends_with(' ') { - return false; - } - // Windows treats these device names as special regardless of extension, - // e.g. opening "NUL.json" writes to the null device, not a file. - const RESERVED: &[&str] = &[ - "CON", "PRN", "AUX", "NUL", "COM0", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", - "COM8", "COM9", "LPT0", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", - "LPT9", - ]; - let stem = std::path::Path::new(s) - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or(s); - if RESERVED.iter().any(|r| stem.eq_ignore_ascii_case(r)) { - return false; - } - let mut components = std::path::Path::new(s).components(); - matches!(components.next(), Some(std::path::Component::Normal(_))) - && components.next().is_none() -} - /// Waits for the OAuth callback on the given listener, validates state and path. /// /// Accepts connections in a loop so that stray connections (port scanners, @@ -1303,49 +982,11 @@ async fn parse_token_response( } #[cfg(test)] -// set_var/remove_var are unsafe in Rust 2024 edition. The XDG_MUTEX in this -// module serialises all access so usage here is data-race-free. -#[allow(unsafe_code)] mod tests { use serde_json::json; use super::*; - /// Serialises access to XDG_CONFIG_HOME (and restores it) so env-var tests - /// cannot race each other when the test runner spawns multiple threads. - static XDG_MUTEX: std::sync::Mutex<()> = std::sync::Mutex::new(()); - - /// RAII guard that restores an env var when dropped, including on panic. - struct EnvVarGuard { - key: &'static str, - prev: Option, - } - - impl Drop for EnvVarGuard { - fn drop(&mut self) { - // SAFETY: XDG_MUTEX is held for the duration of this guard's lifetime, - // ensuring no other thread modifies these variables concurrently. - unsafe { - match self.prev.take() { - Some(v) => std::env::set_var(self.key, v), - None => std::env::remove_var(self.key), - } - } - } - } - - fn with_xdg_config_home(value: &std::path::Path, f: F) { - let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); - let prev = std::env::var("XDG_CONFIG_HOME").ok(); - // SAFETY: same as EnvVarGuard::drop — mutex held for the duration. - unsafe { std::env::set_var("XDG_CONFIG_HOME", value) }; - let _restore = EnvVarGuard { - key: "XDG_CONFIG_HOME", - prev, - }; - f(); - } - fn test_provider() -> PkceAuthProvider { PkceAuthProvider::new( "test", @@ -1662,130 +1303,6 @@ mod tests { ); } - #[test] - fn credential_file_path_uses_xdg_config_home() { - let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce"); - with_xdg_config_home(&dir, || { - let path = test_provider().credential_file_path("prod"); - assert_eq!( - path, - Some(dir.join("test").join("credentials").join("test-prod.json")) - ); - }); - } - - #[test] - fn credential_file_path_with_app_id_uses_app_id_as_dir() { - let dir = std::env::temp_dir().join("cli-engine-test-xdg-pkce-appid"); - with_xdg_config_home(&dir, || { - let path = test_provider() - .with_app_id("myapp") - .credential_file_path("prod"); - assert_eq!( - path, - Some(dir.join("myapp").join("credentials").join("test-prod.json")) - ); - }); - } - - #[test] - fn credential_file_path_rejects_traversal_in_env() { - let dir = std::env::temp_dir().join("cli-engine-test-xdg-traversal"); - with_xdg_config_home(&dir, || { - assert_eq!( - test_provider().credential_file_path("../../etc/passwd"), - None - ); - assert_eq!(test_provider().credential_file_path("dev/subdir"), None); - assert_eq!(test_provider().credential_file_path("dev\\subdir"), None); - assert_eq!(test_provider().credential_file_path(".."), None); - }); - } - - #[test] - fn is_safe_path_component_rejects_windows_reserved_names() { - for name in &[ - "CON", "con", "NUL", "nul", "COM1", "LPT9", "CON.txt", "NUL.json", - ] { - assert!( - !is_safe_path_component(name), - "{name:?} should be rejected as a Windows reserved name" - ); - } - } - - #[test] - fn is_safe_path_component_rejects_empty_string() { - assert!(!is_safe_path_component("")); - } - - #[test] - fn is_safe_path_component_rejects_leading_space_and_del() { - assert!( - !is_safe_path_component(" prod"), - "leading space should be rejected" - ); - assert!( - !is_safe_path_component("prod\x7f"), - "DEL byte should be rejected" - ); - } - - #[test] - fn is_safe_path_component_rejects_trailing_dot_and_space() { - assert!(!is_safe_path_component("prod.")); - assert!(!is_safe_path_component("prod ")); - } - - #[test] - fn is_safe_path_component_accepts_normal_values() { - for name in &["dev", "prod", "staging", "my-app", "my_app", "app.v2"] { - assert!(is_safe_path_component(name), "{name:?} should be accepted"); - } - } - - #[test] - fn credential_file_path_rejects_relative_base_dir() { - let _lock = XDG_MUTEX.lock().unwrap_or_else(|e| e.into_inner()); - let prev = std::env::var("XDG_CONFIG_HOME").ok(); - // SAFETY: mutex held for the duration. - unsafe { std::env::set_var("XDG_CONFIG_HOME", ".") }; - let _restore = EnvVarGuard { - key: "XDG_CONFIG_HOME", - prev, - }; - assert_eq!( - test_provider().credential_file_path("prod"), - None, - "relative XDG_CONFIG_HOME should be rejected" - ); - } - - #[tokio::test] - async fn file_fallback_round_trip_write_then_read() { - let dir = tempfile::tempdir().expect("tempdir"); - let path = dir.path().join("test-prod.json"); - let token = valid_token("file-token"); - let json = serde_json::to_string(&token).expect("serialize"); - - write_token_file_blocking(path.clone(), json).expect("write"); - - let loaded = load_token_from_file(&path).await; - assert_eq!(loaded.expect("token present").access_token, "file-token"); - } - - #[tokio::test] - async fn file_fallback_invalid_json_returns_none() { - let dir = tempfile::tempdir().expect("tempdir"); - let path = dir.path().join("bad.json"); - std::fs::write(&path, b"not-valid-json").expect("write"); - - assert!( - load_token_from_file(&path).await.is_none(), - "invalid JSON should return None" - ); - } - /// resolve_token must return a pre-seeded in-memory token without /// triggering the PKCE browser flow (which would require a port and browser). /// This also exercises the cache-hit path that follows token persistence. @@ -1905,4 +1422,128 @@ mod tests { let credential = provider.build_credential("prod", &token); assert_eq!(credential.identity, "picked"); } + + /// In-memory [`CredentialStorage`] double: lets us assert the provider + /// delegates load/save/delete without any real keychain or filesystem. + #[derive(Debug, Default)] + struct MemoryStorage { + entries: std::sync::Mutex>, + } + + impl MemoryStorage { + fn entry_key(key: &CredentialKey<'_>) -> String { + format!("{}/{}/{}", key.app_id, key.provider, key.env) + } + } + + #[async_trait] + impl CredentialStorage for MemoryStorage { + async fn load(&self, key: &CredentialKey<'_>) -> Option { + self.entries + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(&Self::entry_key(key)) + .cloned() + } + + async fn save(&self, key: &CredentialKey<'_>, value: &str) -> Result<()> { + self.entries + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(Self::entry_key(key), value.to_owned()); + Ok(()) + } + + async fn delete(&self, key: &CredentialKey<'_>) { + self.entries + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&Self::entry_key(key)); + } + } + + #[test] + #[allow(deprecated)] + fn with_file_fallback_maps_to_store_modes() { + assert_eq!( + test_provider().with_file_fallback(true).store_mode, + Some(CredentialStore::Auto) + ); + assert_eq!( + test_provider().with_file_fallback(false).store_mode, + Some(CredentialStore::Keyring) + ); + } + + #[test] + fn builders_record_storage_selection() { + assert_eq!( + test_provider() + .with_credential_store(CredentialStore::File) + .store_mode, + Some(CredentialStore::File) + ); + let provider = test_provider().with_storage(Arc::new(MemoryStorage::default())); + assert!(provider.storage_override.is_some()); + } + + #[tokio::test] + async fn provider_delegates_to_injected_storage() { + let mem = Arc::new(MemoryStorage::default()); + let provider = test_provider().with_app_id("app").with_storage(mem.clone()); + + // No entry yet: status reports not-logged-in. + assert!(provider.status("dev").await.is_err()); + + // Saving routes through the injected store. + provider + .save_stored("dev", &valid_token("tok")) + .await + .expect("save"); + let key = CredentialKey::new("app", "test", "dev"); + assert!(mem.load(&key).await.is_some(), "token reached the store"); + + // And status reads it back. + let cred = provider.status("dev").await.expect("status"); + assert_eq!(cred.token, "tok"); + + // Logout clears it from the store. + provider.logout("dev").await.expect("logout"); + assert!(mem.load(&key).await.is_none(), "token removed on logout"); + } + + #[tokio::test] + async fn corrupt_stored_blob_self_heals() { + let mem = Arc::new(MemoryStorage::default()); + let key = CredentialKey::new("app", "test", "dev"); + mem.save(&key, "not-valid-json").await.expect("seed"); + + let provider = test_provider().with_app_id("app").with_storage(mem.clone()); + assert!(provider.load_stored("dev").await.is_none()); + assert!( + mem.load(&key).await.is_none(), + "corrupt blob should be deleted (self-heal)" + ); + } + + #[tokio::test] + // The guard is intentionally held across awaits to serialize env mutation. + #[allow(clippy::await_holding_lock)] + async fn file_store_round_trips_without_keyring() { + let dir = tempfile::tempdir().expect("tempdir"); + // Hold the shared lock + env guard across the awaits. + let _lock = crate::config::test_env::lock(); + let _env = crate::config::test_env::EnvVarGuard::set("XDG_CONFIG_HOME", Some(dir.path())); + + let provider = test_provider() + .with_app_id("app") + .with_credential_store(CredentialStore::File); + assert!(provider.status("dev").await.is_err()); + provider + .save_stored("dev", &valid_token("filetok")) + .await + .expect("save"); + let cred = provider.status("dev").await.expect("status"); + assert_eq!(cred.token, "filetok"); + } } diff --git a/src/auth/storage.rs b/src/auth/storage.rs new file mode 100644 index 0000000..3cf0fde --- /dev/null +++ b/src/auth/storage.rs @@ -0,0 +1,534 @@ +//! Injectable credential storage backends. +//! +//! Auth providers persist credentials through the [`CredentialStorage`] trait +//! rather than talking to a keychain or the filesystem directly. This decouples +//! *what* is stored (a provider's serialized token) from *where* it is stored, +//! so a single storage backend can be shared across providers and swapped out — +//! for tests, or to disable the system keychain on machines where it is +//! unavailable (headless Linux, WSL). +//! +//! Backends map one-to-one onto [`CredentialStore`](crate::config::CredentialStore) modes: +//! +//! - [`FileStorage`] — unencrypted JSON under the config base directory. Always +//! available; needs no system dependencies. +//! - `KeyringStorage` — the system keychain only (requires the `pkce-auth` +//! feature and the `keyring` crate). +//! - `AutoStorage` — keychain with a transparent file fallback when the +//! keychain backend is unavailable (requires `pkce-auth`). +//! +//! [`default_storage`] picks a backend from the resolved +//! [`CredentialStore`](crate::config::CredentialStore) mode (CLI flag, env var, +//! config file, or the `Keyring` default); see [`crate::config`]. + +use std::sync::Arc; + +use async_trait::async_trait; + +use crate::Result; +use crate::config::CredentialStore; +use crate::fs::{config_base_dir, is_safe_path_component}; + +/// Identifies a single stored credential. +/// +/// Backends derive their storage location from this key: the keychain service +/// name and the file path are both functions of `(app_id, provider, env)`. +#[derive(Clone, Copy, Debug)] +pub struct CredentialKey<'key> { + /// Application id; namespaces credentials across CLIs sharing a keychain. + pub app_id: &'key str, + /// Auth provider name. + pub provider: &'key str, + /// Target environment name. + pub env: &'key str, +} + +impl<'key> CredentialKey<'key> { + /// Creates a key from its parts. + #[must_use] + pub fn new(app_id: &'key str, provider: &'key str, env: &'key str) -> Self { + Self { + app_id, + provider, + env, + } + } +} + +/// A pluggable place to persist a provider's serialized credential. +/// +/// Values are opaque strings (typically JSON); the backend never interprets +/// them, so it stays independent of any provider's token shape. Callers own +/// (de)serialization and any validity/expiry checks. +#[async_trait] +pub trait CredentialStorage: Send + Sync + std::fmt::Debug { + /// Loads the stored blob for `key`, or `None` when absent or unreadable. + /// + /// Backends own the policy for distinguishing "no entry" from "store + /// unavailable" (see `AutoStorage`); both collapse to `None` here. + async fn load(&self, key: &CredentialKey<'_>) -> Option; + + /// Persists `value` for `key`, replacing any existing value. + /// + /// # Errors + /// Returns an error when the backend cannot durably store the value. + async fn save(&self, key: &CredentialKey<'_>, value: &str) -> Result<()>; + + /// Removes the blob for `key`. Best-effort: absence is not an error. + async fn delete(&self, key: &CredentialKey<'_>); + + /// Lists environment names with stored credentials, if the backend supports + /// enumeration. The default returns an empty list (the keychain cannot be + /// enumerated by service prefix). + /// + /// # Errors + /// Returns an error when enumeration is attempted but fails. + async fn list(&self) -> Result> { + Ok(Vec::new()) + } +} + +/// Unencrypted file-based credential storage. +/// +/// Stores each credential as JSON at +/// `//credentials/-.json`, where `` is the +/// key's `app_id` (or `provider` when `app_id` is empty) and `` is +/// resolved by [`config_base_dir`]. On Unix the file is created `0600` and the +/// parent directory is best-effort restricted to `0700`. +/// +/// Credentials are written in clear text, so prefer the system keychain where +/// one is available. +#[derive(Clone, Copy, Debug, Default)] +pub struct FileStorage; + +impl FileStorage { + /// Creates a file-storage backend. + #[must_use] + pub fn new() -> Self { + Self + } + + /// Resolves the on-disk path for `key`, or `None` when the config base + /// directory is unavailable or any key component is unsafe as a path + /// segment. + fn path_for(key: &CredentialKey<'_>) -> Option { + let app = if key.app_id.is_empty() { + key.provider + } else { + key.app_id + }; + if !is_safe_path_component(app) + || !is_safe_path_component(key.provider) + || !is_safe_path_component(key.env) + { + tracing::warn!( + app, + provider = key.provider, + env = key.env, + "refusing credential path with unsafe component" + ); + return None; + } + let base = config_base_dir()?; + Some( + base.join(app) + .join("credentials") + .join(format!("{}-{}.json", key.provider, key.env)), + ) + } +} + +#[async_trait] +impl CredentialStorage for FileStorage { + async fn load(&self, key: &CredentialKey<'_>) -> Option { + let path = Self::path_for(key)?; + match tokio::fs::read_to_string(&path).await { + Ok(s) => Some(s), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => None, + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "credential file read failed"); + None + } + } + } + + async fn save(&self, key: &CredentialKey<'_>, value: &str) -> Result<()> { + let path = Self::path_for(key).ok_or_else(|| { + crate::error::CliCoreError::message("could not determine credential file path") + })?; + let value = value.to_owned(); + tokio::task::spawn_blocking(move || crate::fs::write_string_atomic(&path, &value)) + .await + .map_err(|e| { + crate::error::CliCoreError::message(format!( + "credential file write task {}: {e}", + if e.is_cancelled() { + "cancelled" + } else { + "panicked" + } + )) + })? + } + + async fn delete(&self, key: &CredentialKey<'_>) { + let Some(path) = Self::path_for(key) else { + return; + }; + match tokio::fs::remove_file(&path).await { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => { + tracing::warn!(path = %path.display(), error = %e, "failed to delete credential file"); + } + } + } +} + +#[cfg(feature = "pkce-auth")] +pub use keychain::{AutoStorage, KeyringStorage}; + +#[cfg(feature = "pkce-auth")] +mod keychain { + use super::{CredentialKey, CredentialStorage, FileStorage, Result, async_trait}; + + const KEYCHAIN_USER: &str = "token"; + + /// Derives the keychain service name for `key`: + /// `//`, or `/` when `app_id` is empty. + fn keychain_service(key: &CredentialKey<'_>) -> String { + if key.app_id.is_empty() { + format!("{}/{}", key.provider, key.env) + } else { + format!("{}/{}/{}", key.app_id, key.provider, key.env) + } + } + + /// System-keychain credential storage. + /// + /// Reads and writes the OS keychain only. A `load` returns `None` both when + /// no entry exists and when the keychain backend is unavailable; a `save` + /// failure is a hard error. No file is ever written — use [`AutoStorage`] + /// for a file fallback or [`super::FileStorage`] to skip the keychain. + #[derive(Clone, Copy, Debug, Default)] + pub struct KeyringStorage; + + impl KeyringStorage { + /// Creates a keychain-storage backend. + #[must_use] + pub fn new() -> Self { + Self + } + + /// Three-state keychain read used by [`AutoStorage`] to decide whether + /// to consult a file fallback. + /// + /// `Some(Some(json))` = entry found; `Some(None)` = keychain reachable + /// but empty; `None` = keychain backend unavailable. + pub(super) async fn read_three_state( + &self, + key: &CredentialKey<'_>, + ) -> Option> { + let service = keychain_service(key); + match tokio::task::spawn_blocking({ + let service = service.clone(); + move || keychain_read_blocking(&service, KEYCHAIN_USER) + }) + .await + { + Ok(result) => result, + Err(e) => { + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(service, error = %e, reason, "keychain read task failed"); + None + } + } + } + + /// Writes to the keychain, returning whether the write succeeded. + pub(super) async fn write_raw(&self, key: &CredentialKey<'_>, value: &str) -> bool { + let service = keychain_service(key); + let value = value.to_owned(); + match tokio::task::spawn_blocking({ + let service = service.clone(); + move || keychain_write_blocking(&service, KEYCHAIN_USER, &value) + }) + .await + { + Ok(saved) => saved, + Err(e) => { + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(service, error = %e, reason, "keychain write task failed"); + false + } + } + } + + /// Best-effort keychain entry deletion. + pub(super) async fn delete_entry(&self, key: &CredentialKey<'_>) { + let service = keychain_service(key); + let service_for_warn = service.clone(); + if let Err(e) = + tokio::task::spawn_blocking(move || match keyring::Entry::new(&service, KEYCHAIN_USER) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed on delete"); + } + Ok(entry) => match entry.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => {} + Err(e) => { + tracing::warn!(service, error = %e, "keychain delete failed"); + } + }, + }) + .await + { + let reason = if e.is_cancelled() { + "cancelled" + } else { + "panicked" + }; + tracing::warn!(service = service_for_warn, error = %e, reason, "keychain delete task failed"); + } + } + } + + #[async_trait] + impl CredentialStorage for KeyringStorage { + async fn load(&self, key: &CredentialKey<'_>) -> Option { + // Collapse "no entry" and "unavailable" to None: keychain-only mode + // never falls back to a file. + self.read_three_state(key).await.flatten() + } + + async fn save(&self, key: &CredentialKey<'_>, value: &str) -> Result<()> { + if self.write_raw(key, value).await { + Ok(()) + } else { + Err(crate::error::CliCoreError::message( + "failed to save token to keychain — check logs for the underlying error, \ + ensure your system keychain (e.g. gnome-keyring, macOS Keychain) is running \ + and unlocked, or select file storage (credential store \"file\" or \"auto\")", + )) + } + } + + async fn delete(&self, key: &CredentialKey<'_>) { + self.delete_entry(key).await; + } + } + + /// Keychain storage with a transparent unencrypted-file fallback. + /// + /// Preferred when a keychain is usually present but may be missing (WSL, + /// headless sessions). Behavior: + /// - `load`: keychain entry present → use it; keychain reachable but empty → + /// `None` (the file is stale or absent, force re-auth); keychain + /// unavailable → read the file. + /// - `save`: try the keychain; on success remove any stale file; on failure + /// write the file. + /// - `delete`: remove from both the keychain and the file. + #[derive(Clone, Copy, Debug, Default)] + pub struct AutoStorage { + file: FileStorage, + keyring: KeyringStorage, + } + + impl AutoStorage { + /// Creates an auto (keychain-with-file-fallback) backend. + #[must_use] + pub fn new() -> Self { + Self { + file: FileStorage::new(), + keyring: KeyringStorage::new(), + } + } + } + + #[async_trait] + impl CredentialStorage for AutoStorage { + async fn load(&self, key: &CredentialKey<'_>) -> Option { + match self.keyring.read_three_state(key).await { + // Keychain has the entry. + Some(Some(json)) => Some(json), + // Keychain is reachable but empty: skip the file and force login. + Some(None) => None, + // Keychain backend unavailable: fall back to the file. + None => self.file.load(key).await, + } + } + + async fn save(&self, key: &CredentialKey<'_>, value: &str) -> Result<()> { + if self.keyring.write_raw(key, value).await { + // Keychain is the source of truth now; drop any stale file copy. + self.file.delete(key).await; + return Ok(()); + } + self.file.save(key, value).await + } + + async fn delete(&self, key: &CredentialKey<'_>) { + self.keyring.delete(key).await; + self.file.delete(key).await; + } + } + + /// Reads a token JSON string from the system keychain. Sync; call inside + /// `spawn_blocking`. + fn keychain_read_blocking(service: &str, user: &str) -> Option> { + match keyring::Entry::new(service, user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + None + } + Ok(entry) => match entry.get_password() { + Err(keyring::Error::NoEntry) => { + tracing::debug!(service, "no stored token in keychain"); + Some(None) + } + Err(e) => { + tracing::warn!(service, error = %e, "keychain read failed"); + None + } + Ok(json) => Some(Some(json)), + }, + } + } + + /// Writes a token JSON string to the system keychain. Sync; call inside + /// `spawn_blocking`. + fn keychain_write_blocking(service: &str, user: &str, json: &str) -> bool { + match keyring::Entry::new(service, user) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain entry creation failed"); + false + } + Ok(entry) => match entry.set_password(json) { + Err(e) => { + tracing::warn!(service, error = %e, "keychain write failed"); + false + } + Ok(()) => { + tracing::debug!(service, "token saved to keychain"); + true + } + }, + } + } +} + +/// Builds the credential storage backend for `mode`. +/// +/// `File` always yields a [`FileStorage`]. `Keyring`/`Auto` yield the keychain +/// backends when the `pkce-auth` feature is enabled; without it they log a +/// warning and degrade to [`FileStorage`], since no keychain backend is +/// compiled in. +#[must_use] +pub fn storage_for(mode: CredentialStore) -> Arc { + match mode { + CredentialStore::File => Arc::new(FileStorage::new()), + #[cfg(feature = "pkce-auth")] + CredentialStore::Keyring => Arc::new(KeyringStorage::new()), + #[cfg(feature = "pkce-auth")] + CredentialStore::Auto => Arc::new(AutoStorage::new()), + #[cfg(not(feature = "pkce-auth"))] + mode => { + tracing::warn!( + %mode, + "keyring backends unavailable (pkce-auth feature disabled); using file storage" + ); + Arc::new(FileStorage::new()) + } + } +} + +/// Resolves the configured [`CredentialStore`] for `app_id` and builds the +/// matching backend. +/// +/// Resolution consults the CLI flag, the `${PREFIX}_CREDENTIAL_STORE` env var, +/// and the config file, defaulting to [`CredentialStore::Keyring`]; see +/// [`crate::config::resolve_credential_store`]. +#[must_use] +pub fn default_storage(app_id: &str) -> Arc { + let mode = crate::config::resolve_credential_store(app_id, |k| std::env::var(k).ok()); + storage_for(mode) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::test_env::{EnvVarGuard, lock, with_xdg_config_home}; + + #[test] + fn file_path_uses_app_id_and_provider() { + let dir = std::env::temp_dir().join("cli-engine-storage-test-xdg"); + with_xdg_config_home(&dir, || { + let key = CredentialKey::new("myapp", "prov", "prod"); + assert_eq!( + FileStorage::path_for(&key), + Some(dir.join("myapp").join("credentials").join("prov-prod.json")) + ); + // empty app_id falls back to provider as the dir + let key2 = CredentialKey::new("", "prov", "prod"); + assert_eq!( + FileStorage::path_for(&key2), + Some(dir.join("prov").join("credentials").join("prov-prod.json")) + ); + }); + } + + #[test] + fn file_path_rejects_unsafe_components() { + for env in ["../../etc/passwd", "dev/subdir", "dev\\subdir", ".."] { + let key = CredentialKey::new("app", "prov", env); + assert_eq!( + FileStorage::path_for(&key), + None, + "{env:?} should be rejected" + ); + } + } + + #[test] + fn file_path_rejects_relative_base_dir() { + with_xdg_config_home(std::path::Path::new("."), || { + let key = CredentialKey::new("app", "prov", "dev"); + assert_eq!(FileStorage::path_for(&key), None); + }); + } + + #[tokio::test] + // The guard is intentionally held across awaits to serialize env mutation. + #[allow(clippy::await_holding_lock)] + async fn file_storage_round_trip() { + let dir = tempfile::tempdir().expect("tempdir"); + // Hold the shared lock + env guard across the awaits (tokio::test uses a + // current-thread runtime, so the non-Send guard is fine). + let _lock = lock(); + let _env = EnvVarGuard::set("XDG_CONFIG_HOME", Some(dir.path())); + + let store = FileStorage::new(); + let key = CredentialKey::new("app", "prov", "dev"); + assert_eq!(store.load(&key).await, None); + store.save(&key, "{\"token\":\"abc\"}").await.expect("save"); + assert_eq!( + store.load(&key).await.as_deref(), + Some("{\"token\":\"abc\"}") + ); + store.delete(&key).await; + assert_eq!(store.load(&key).await, None); + } + + #[test] + fn storage_for_file_is_always_available() { + // Just assert it constructs without panicking; behavior covered above. + let store = storage_for(CredentialStore::File); + assert!(format!("{store:?}").contains("FileStorage")); + } +} diff --git a/src/cli.rs b/src/cli.rs index b93f327..d738cdf 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -237,6 +237,11 @@ pub struct CliConfig { /// admin modules (e.g. godaddy's `env`). When unset, defaults to `"Admin"`; /// set it to match a consumer's own taxonomy (e.g. gdx's "Administration"). pub admin_category: Option, + /// Whether to mount the built-in `config` command group (`config + /// get`/`set`/`path`/`list`). Off by default to avoid colliding with a + /// consumer's own `config` noun. Enable via + /// [`CliConfig::with_config_commands`]. + pub config_commands: bool, /// Alternative `argv[0]` names this binary may be invoked as, mapped to the /// behavior the engine should take (busybox/git-style multi-call dispatch). /// @@ -419,6 +424,17 @@ impl CliConfig { self } + /// Mounts the built-in `config` command group (`config get`/`set`/`path`/ + /// `list`) for reading and writing the per-application config file. + /// + /// Off by default so it never collides with a consumer's own `config` noun; + /// the group is filed under the admin help category when enabled. + #[must_use] + pub fn with_config_commands(mut self) -> Self { + self.config_commands = true; + self + } + /// Registers an alternative `argv[0]` name that acts as a shortcut to a /// command path on this same CLI. /// @@ -683,6 +699,9 @@ impl Cli { let mut middleware = Middleware::new(); middleware.app_id = config.app_id.clone(); + // Load the per-application config file once at startup; cloned into each + // per-run middleware so handlers and module registration share it. + middleware.config = Arc::new(crate::config::ConfigFile::load(&config.app_id)); middleware.default_auth_provider = config.default_auth_provider.clone().unwrap_or_default(); middleware.authz = config.authz.clone(); middleware.auditor = config.auditor.clone(); @@ -731,6 +750,9 @@ impl Cli { for command in commands { cli.add_command(command); } + if cli.config.config_commands { + cli.ensure_config_command(); + } cli } @@ -1249,6 +1271,9 @@ impl Cli { let default_format = default_output_format(&self.config.app_id); let flags = global_flags_from_matches(&matches, &default_format); + // Publish the --credential-store override so auth providers resolving + // their storage backend see it at the top of the precedence chain. + crate::config::set_credential_store_flag(flags.credential_store); let command_timeout = match parse_command_timeout(&flags.timeout) { Ok(timeout) => timeout, Err(err) => { @@ -1731,6 +1756,42 @@ impl Cli { self.register_auth_help_entry(); } + /// Mounts the built-in `config` command group and files it under the admin + /// help category. Idempotent and yields to a consumer-defined `config` + /// subcommand if one already exists. + fn ensure_config_command(&mut self) { + if has_subcommand(&self.root, "config") { + return; + } + let group = crate::config_commands::config_command_group(); + let mut prefix = Vec::new(); + group.register_commands(&mut prefix, &mut self.commands); + let mut prefix = Vec::new(); + let clap_group = runtime_group_clap_command_with_schema_help( + &group, + &mut prefix, + &self.middleware.schema_registry, + ); + self.root = self.root.clone().subcommand(clap_group); + let category = self + .config + .admin_category + .clone() + .unwrap_or_else(|| DEFAULT_ADMIN_CATEGORY.to_owned()); + if !self + .module_entries + .iter() + .any(|entry| entry.name == "config") + { + self.module_entries.push(ModuleHelpEntry { + category, + name: "config".to_owned(), + short: "Read and write the CLI config file".to_owned(), + }); + } + self.refresh_root_long(); + } + fn default_auth_provider(&self) -> String { if !self.middleware.default_auth_provider.is_empty() { return self.middleware.default_auth_provider.clone(); diff --git a/src/command.rs b/src/command.rs index 145e658..334cdcf 100644 --- a/src/command.rs +++ b/src/command.rs @@ -107,6 +107,18 @@ pub struct CommandContext { } impl CommandContext { + /// Returns the loaded per-application config file. + /// + /// Read a consumer-owned section with + /// [`ConfigFile::section`](crate::config::ConfigFile::section), for example + /// `ctx.config().section::("deploy")?`. Engine-reserved + /// settings are available via + /// [`ConfigFile::engine`](crate::config::ConfigFile::engine). + #[must_use] + pub fn config(&self) -> &crate::config::ConfigFile { + &self.middleware.config + } + /// Deserializes the raw argument matches into a typed args struct. /// /// Use this with `#[derive(clap::Args)]` structs to get type-safe access diff --git a/src/config.rs b/src/config.rs new file mode 100644 index 0000000..132a88b --- /dev/null +++ b/src/config.rs @@ -0,0 +1,788 @@ +//! Engine configuration file and credential-storage selection. +//! +//! cli-engine reads an optional per-application TOML config file at +//! `//config.toml`, where `` is +//! `$XDG_CONFIG_HOME`, `$HOME/.config`, or `%APPDATA%` (see +//! [`config_base_dir`](crate::fs::config_base_dir)). +//! Loading is best-effort: a missing file yields defaults, and a malformed file +//! logs a warning and falls back to defaults rather than failing the command. +//! +//! The primary setting today selects where credentials are stored — see +//! [`CredentialStore`]. The effective mode is resolved with the precedence +//! +//! ```text +//! --credential-store flag > ${PREFIX}_CREDENTIAL_STORE env > config file > default (Keyring) +//! ``` +//! +//! where `${PREFIX}` is the app id sanitized by +//! [`app_id_env_prefix`](crate::flags::app_id_env_prefix). See +//! [`resolve_credential_store`] and the pure [`resolve_credential_store_with`]. + +use std::path::{Path, PathBuf}; +use std::str::FromStr; +use std::sync::atomic::{AtomicU8, Ordering}; + +use serde::de::DeserializeOwned; +use serde::{Deserialize, Deserializer}; +use toml_edit::DocumentMut; + +use crate::error::CliCoreError; + +/// Where an auth provider stores credentials. +/// +/// The variant selects a concrete storage backend +/// (see [`crate::auth::storage`]). `Keyring` is the default and preserves the +/// historical behavior (system keychain only, hard error when unavailable); +/// `File` is the escape hatch for environments without a working keychain +/// (headless Linux, WSL). +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +#[non_exhaustive] +pub enum CredentialStore { + /// Try the system keychain; transparently fall back to an unencrypted file + /// when the keychain backend is unavailable. + Auto, + /// System keychain only. A keychain failure is a hard error and no file is + /// ever written. This is the default. + #[default] + Keyring, + /// File only: never contact the system keychain. Credentials are written as + /// unencrypted JSON under the config base directory. + File, +} + +impl CredentialStore { + /// Returns the lowercase canonical name (`auto`, `keyring`, or `file`). + #[must_use] + pub fn as_str(self) -> &'static str { + match self { + CredentialStore::Auto => "auto", + CredentialStore::Keyring => "keyring", + CredentialStore::File => "file", + } + } +} + +impl std::fmt::Display for CredentialStore { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.as_str()) + } +} + +/// Error returned when a string does not name a [`CredentialStore`] variant. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ParseCredentialStoreError(String); + +impl std::fmt::Display for ParseCredentialStoreError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "invalid credential store {:?} (expected one of: auto, keyring, file)", + self.0 + ) + } +} + +impl std::error::Error for ParseCredentialStoreError {} + +impl FromStr for CredentialStore { + type Err = ParseCredentialStoreError; + + fn from_str(s: &str) -> Result { + match s.trim().to_ascii_lowercase().as_str() { + "auto" => Ok(CredentialStore::Auto), + // `keychain` is accepted as an alias for the keychain-only mode. + "keyring" | "keychain" => Ok(CredentialStore::Keyring), + "file" => Ok(CredentialStore::File), + _ => Err(ParseCredentialStoreError(s.to_owned())), + } + } +} + +impl<'de> Deserialize<'de> for CredentialStore { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let raw = String::deserialize(deserializer)?; + raw.parse().map_err(serde::de::Error::custom) + } +} + +/// Top-level engine configuration parsed from `config.toml`. +/// +/// Unknown keys are ignored so older binaries tolerate config written for newer +/// ones. New sections can be added as additional fields over time. +#[derive(Clone, Debug, Default, Deserialize)] +#[serde(default)] +pub struct EngineConfig { + /// Credential-storage settings (`[credentials]` table). + pub credentials: CredentialsConfig, +} + +/// The `[credentials]` table of the engine config file. +#[derive(Clone, Debug, Default, Deserialize)] +#[serde(default)] +pub struct CredentialsConfig { + /// Selected credential store, or `None` when the key is absent. + pub store: Option, +} + +/// Process-wide override set from the `--credential-store` global flag. +/// +/// Sits at the top of the resolution precedence. Encoded as a byte so it can be +/// updated lock-free: `0` = unset, `1` = `Auto`, `2` = `Keyring`, `3` = `File`. +/// Overwritable (not set-once) so each [`crate::cli::Cli::run`] re-establishes +/// the flag for that run rather than the first run pinning it for the process. +static CREDENTIAL_STORE_FLAG: AtomicU8 = AtomicU8::new(0); + +fn encode_store(store: Option) -> u8 { + match store { + None => 0, + Some(CredentialStore::Auto) => 1, + Some(CredentialStore::Keyring) => 2, + Some(CredentialStore::File) => 3, + } +} + +fn decode_store(byte: u8) -> Option { + match byte { + 1 => Some(CredentialStore::Auto), + 2 => Some(CredentialStore::Keyring), + 3 => Some(CredentialStore::File), + _ => None, + } +} + +/// Records the value of the `--credential-store` flag for later resolution. +/// +/// Called at the start of each CLI run with the parsed flag value (`None` when +/// the flag was not supplied), overwriting any previous value. +pub fn set_credential_store_flag(store: Option) { + CREDENTIAL_STORE_FLAG.store(encode_store(store), Ordering::Relaxed); +} + +/// Returns the flag override recorded by [`set_credential_store_flag`], if any. +#[must_use] +pub fn credential_store_flag() -> Option { + decode_store(CREDENTIAL_STORE_FLAG.load(Ordering::Relaxed)) +} + +/// Derives the credential-store override env var from an app id, e.g. +/// `godaddy` -> `GODADDY_CREDENTIAL_STORE`. +#[must_use] +pub fn credential_store_env_var(app_id: &str) -> String { + format!( + "{}_CREDENTIAL_STORE", + crate::flags::app_id_env_prefix(app_id) + ) +} + +/// Returns the path to the engine config file for `app_id`, if a base config +/// directory can be resolved and `app_id` is a safe single path component. +#[must_use] +pub fn config_file_path(app_id: &str) -> Option { + if !crate::fs::is_safe_path_component(app_id) { + tracing::warn!(app_id, "refusing config path with unsafe app id"); + return None; + } + crate::fs::config_base_dir().map(|base| base.join(app_id).join("config.toml")) +} + +/// Loads the engine-reserved config for `app_id`. +/// +/// Convenience wrapper over [`ConfigFile::load`] + [`ConfigFile::engine`]. +/// Best-effort: a missing/unreadable/malformed file yields +/// [`EngineConfig::default`], so a broken config file cannot take the CLI down. +#[must_use] +pub fn load(app_id: &str) -> EngineConfig { + ConfigFile::load(app_id).engine() +} + +/// A loaded per-application config file. +/// +/// cli-engine reads a single TOML file at `//config.toml` +/// (see [`config_file_path`]). Engine-reserved settings live in documented +/// top-level tables (today just `[credentials]`, see [`EngineConfig`]); consumer +/// CLIs own every other top-level table and read them with [`section`] or +/// [`deserialize`]. The file is also surfaced to command handlers via +/// [`CommandContext::config`](crate::command::CommandContext::config) and to +/// module registration via +/// [`ModuleContext::config`](crate::module::ModuleContext::config). +/// +/// Edits made with [`set`] preserve existing comments and formatting (backed by +/// `toml_edit`) and are persisted with [`save`]. +/// +/// [`section`]: ConfigFile::section +/// [`deserialize`]: ConfigFile::deserialize +/// [`set`]: ConfigFile::set +/// [`save`]: ConfigFile::save +#[derive(Clone, Debug)] +pub struct ConfigFile { + path: Option, + doc: DocumentMut, +} + +impl Default for ConfigFile { + fn default() -> Self { + Self { + path: None, + doc: DocumentMut::new(), + } + } +} + +impl ConfigFile { + /// Loads the config file for `app_id`. + /// + /// Best-effort: a missing file, unresolvable config directory, or malformed + /// TOML yields an empty document (a warning is logged for the malformed + /// case). The resolved path is retained for [`save`](ConfigFile::save) even + /// when the file does not yet exist. + #[must_use] + pub fn load(app_id: &str) -> Self { + let path = config_file_path(app_id); + let doc = match &path { + None => DocumentMut::new(), + Some(p) => match std::fs::read_to_string(p) { + Ok(contents) => contents.parse::().unwrap_or_else(|e| { + tracing::warn!(path = %p.display(), error = %e, "ignoring malformed config file"); + DocumentMut::new() + }), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => DocumentMut::new(), + Err(e) => { + tracing::warn!(path = %p.display(), error = %e, "could not read config file"); + DocumentMut::new() + } + }, + }; + Self { path, doc } + } + + /// Returns the resolved config file path, if a config directory was + /// available. `None` means neither `XDG_CONFIG_HOME`/`HOME` nor `APPDATA` + /// resolved to an absolute path (so nothing can be loaded or saved). + #[must_use] + pub fn path(&self) -> Option<&Path> { + self.path.as_deref() + } + + /// Deserializes the engine-reserved sections into an [`EngineConfig`]. + /// + /// Lenient: any deserialization error (for example an invalid + /// `[credentials].store`) yields [`EngineConfig::default`]. + #[must_use] + pub fn engine(&self) -> EngineConfig { + self.deserialize().unwrap_or_default() + } + + /// Deserializes a single top-level table `name` into `T`, or `Ok(None)` when + /// the key is absent. + /// + /// Use this to read a consumer-owned section such as `[deploy]`: + /// `cfg.section::("deploy")?`. + /// + /// # Errors + /// Returns an error when the table is present but does not deserialize into + /// `T`. + pub fn section(&self, name: &str) -> crate::Result> { + let table: toml::Table = toml::from_str(&self.doc.to_string()) + .map_err(|e| CliCoreError::message(format!("config parse error: {e}")))?; + match table.get(name) { + None => Ok(None), + Some(value) => value + .clone() + .try_into() + .map(Some) + .map_err(|e| CliCoreError::message(format!("config section {name:?}: {e}"))), + } + } + + /// Deserializes the entire config file into a consumer root type `T`. + /// + /// The root type may include the engine-reserved sections alongside its own; + /// unknown keys are tolerated when `T` allows them. + /// + /// # Errors + /// Returns an error when the document does not deserialize into `T`. + pub fn deserialize(&self) -> crate::Result { + toml::from_str(&self.doc.to_string()) + .map_err(|e| CliCoreError::message(format!("config deserialize error: {e}"))) + } + + /// Returns the string form of the value at a dotted key (for example + /// `credentials.store` or `deploy.region`), or `None` when absent. + /// + /// Scalars render without quotes; a table renders as its TOML fragment. + #[must_use] + pub fn get(&self, dotted_key: &str) -> Option { + let mut item = self.doc.as_item(); + for segment in dotted_key.split('.') { + item = item.as_table_like()?.get(segment)?; + } + match item.as_value() { + Some(toml_edit::Value::String(s)) => Some(s.value().clone()), + Some(other) => Some(other.to_string().trim().to_owned()), + None => Some(item.to_string()), + } + } + + /// Sets the value at a dotted key, creating intermediate tables as needed. + /// + /// `value` is parsed as a TOML scalar when it looks like a bool/integer/float + /// and stored as a string otherwise. The engine-reserved `credentials.store` + /// key is validated against [`CredentialStore`] and rejected when invalid. + /// Existing comments and formatting elsewhere in the file are preserved. + /// Call [`save`](ConfigFile::save) to persist. + /// + /// # Errors + /// Returns an error for an empty/invalid key, an invalid engine value, or a + /// key whose parent path is not a table. + pub fn set(&mut self, dotted_key: &str, value: &str) -> crate::Result<()> { + if dotted_key == "credentials.store" { + value + .parse::() + .map_err(|e| CliCoreError::message(e.to_string()))?; + } + let segments: Vec<&str> = dotted_key.split('.').collect(); + if segments.iter().any(|s| s.is_empty()) { + return Err(CliCoreError::message(format!( + "invalid config key {dotted_key:?}" + ))); + } + let Some((last, parents)) = segments.split_last() else { + return Err(CliCoreError::message("empty config key")); + }; + let mut table = self.doc.as_table_mut(); + for segment in parents { + let entry = table + .entry(segment) + .or_insert(toml_edit::Item::Table(toml_edit::Table::new())); + table = entry.as_table_mut().ok_or_else(|| { + CliCoreError::message(format!("config key {segment:?} is not a table")) + })?; + } + table[last] = toml_edit::Item::Value(infer_toml_value(value)); + Ok(()) + } + + /// Renders the whole config document back to a TOML string (preserving + /// comments and formatting). + #[must_use] + pub fn to_toml_string(&self) -> String { + self.doc.to_string() + } + + /// Persists the document to its config path via an atomic write. + /// + /// # Errors + /// Returns an error when no config path is available (no resolvable config + /// directory) or the write fails. + pub fn save(&self) -> crate::Result<()> { + let path = self.path.as_ref().ok_or_else(|| { + CliCoreError::message( + "no config path available (set XDG_CONFIG_HOME or HOME to a directory)", + ) + })?; + crate::fs::write_string_atomic(path, &self.doc.to_string()) + } +} + +/// Parses `value` as a TOML bool/integer/float when possible, else a string. +fn infer_toml_value(value: &str) -> toml_edit::Value { + if let Ok(b) = value.parse::() { + return b.into(); + } + if let Ok(i) = value.parse::() { + return i.into(); + } + if let Ok(f) = value.parse::() { + return f.into(); + } + value.into() +} + +/// Resolves the effective [`CredentialStore`] from explicit inputs. +/// +/// Pure and side-effect free so the precedence is unit-testable without touching +/// process state. Precedence (highest first): CLI `flag`, then `env` (an invalid +/// value is logged and ignored, falling through), then the config `file`, then +/// the default [`CredentialStore::Keyring`]. +#[must_use] +pub fn resolve_credential_store_with( + flag: Option, + env: Option<&str>, + file: &EngineConfig, +) -> CredentialStore { + if let Some(store) = flag { + return store; + } + if let Some(raw) = env { + match raw.parse::() { + Ok(store) => return store, + Err(e) => tracing::warn!(error = %e, "ignoring invalid credential-store env var"), + } + } + if let Some(store) = file.credentials.store { + return store; + } + CredentialStore::default() +} + +/// Resolves the effective [`CredentialStore`] for `app_id` against process state. +/// +/// Reads the CLI-flag override ([`credential_store_flag`]), the +/// `${PREFIX}_CREDENTIAL_STORE` env var via the injected `var` getter, and the +/// config file ([`load`]), then applies [`resolve_credential_store_with`]. The +/// `var` getter is injected so callers/tests can supply environment lookups +/// without mutating the process environment. +pub fn resolve_credential_store( + app_id: &str, + var: impl Fn(&str) -> Option, +) -> CredentialStore { + let env = var(&credential_store_env_var(app_id)); + let file = load(app_id); + resolve_credential_store_with(credential_store_flag(), env.as_deref(), &file) +} + +/// Test-only helpers for serializing and mutating `XDG_CONFIG_HOME`. +/// +/// `set_var`/`remove_var` are `unsafe` in the Rust 2024 edition; [`XDG_TEST_MUTEX`] +/// serializes all access so usage here is data-race-free. Shared crate-wide so +/// every test that mutates `XDG_CONFIG_HOME` (in `config`, `auth::storage`, and +/// `auth::pkce`) contends on the *same* lock rather than racing across modules. +#[cfg(test)] +#[allow(unsafe_code, dead_code)] +pub(crate) mod test_env { + use std::path::Path; + use std::sync::{Mutex, MutexGuard}; + + /// Serializes access to `XDG_CONFIG_HOME` across all crate tests. + pub(crate) static XDG_TEST_MUTEX: Mutex<()> = Mutex::new(()); + + /// Acquires the shared lock (poison-tolerant). Hold it for the entire span + /// during which `XDG_CONFIG_HOME` is mutated — including across `.await` + /// points in async tests (`#[tokio::test]` uses a current-thread runtime, + /// so the non-`Send` guard is fine). + pub(crate) fn lock() -> MutexGuard<'static, ()> { + XDG_TEST_MUTEX.lock().unwrap_or_else(|e| e.into_inner()) + } + + /// RAII guard that restores an env var to its prior value when dropped, + /// including on panic. The caller must hold [`lock`] for the guard's life. + pub(crate) struct EnvVarGuard { + key: &'static str, + prev: Option, + } + + impl EnvVarGuard { + /// Sets `key` to `value` (or removes it when `None`), capturing the + /// prior value for restoration on drop. Caller must hold [`lock`]. + pub(crate) fn set(key: &'static str, value: Option<&Path>) -> Self { + let prev = std::env::var(key).ok(); + // SAFETY: caller holds XDG_TEST_MUTEX, serializing all mutation. + unsafe { + match value { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + Self { key, prev } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + // SAFETY: callers hold XDG_TEST_MUTEX for the guard's lifetime. + unsafe { + match self.prev.take() { + Some(v) => std::env::set_var(self.key, v), + None => std::env::remove_var(self.key), + } + } + } + } + + /// Runs `f` with `XDG_CONFIG_HOME` set to `value`, holding the shared lock + /// and restoring the previous value afterward. + pub(crate) fn with_xdg_config_home R, R>(value: &Path, f: F) -> R { + let _lock = lock(); + let _restore = EnvVarGuard::set("XDG_CONFIG_HOME", Some(value)); + f() + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parses_known_variants_case_insensitively() { + assert_eq!("auto".parse(), Ok(CredentialStore::Auto)); + assert_eq!("Keyring".parse(), Ok(CredentialStore::Keyring)); + assert_eq!("KEYCHAIN".parse(), Ok(CredentialStore::Keyring)); + assert_eq!(" file ".parse(), Ok(CredentialStore::File)); + } + + #[test] + fn rejects_unknown_variant() { + let err = "vault" + .parse::() + .expect_err("should reject"); + assert!(err.to_string().contains("vault")); + } + + #[test] + fn display_round_trips_through_from_str() { + for store in [ + CredentialStore::Auto, + CredentialStore::Keyring, + CredentialStore::File, + ] { + assert_eq!(store.to_string().parse(), Ok(store)); + } + } + + #[test] + fn env_var_name_is_derived_from_app_id() { + assert_eq!( + credential_store_env_var("godaddy"), + "GODADDY_CREDENTIAL_STORE" + ); + assert_eq!( + credential_store_env_var("my-cli"), + "MY_CLI_CREDENTIAL_STORE" + ); + } + + #[test] + fn deserializes_store_from_toml() { + let config: EngineConfig = + toml::from_str("[credentials]\nstore = \"file\"\n").expect("valid toml"); + assert_eq!(config.credentials.store, Some(CredentialStore::File)); + } + + #[test] + fn deserialize_rejects_bad_store_value() { + let result = toml::from_str::("[credentials]\nstore = \"nope\"\n"); + assert!(result.is_err(), "bad store value should fail to parse"); + } + + #[test] + fn unknown_keys_are_ignored() { + let config: EngineConfig = + toml::from_str("future_section = true\n[credentials]\nstore = \"auto\"\n") + .expect("unknown keys tolerated"); + assert_eq!(config.credentials.store, Some(CredentialStore::Auto)); + } + + #[test] + fn resolution_precedence_flag_beats_env_beats_file() { + let file = EngineConfig { + credentials: CredentialsConfig { + store: Some(CredentialStore::Keyring), + }, + }; + // flag wins over everything + assert_eq!( + resolve_credential_store_with(Some(CredentialStore::Auto), Some("file"), &file), + CredentialStore::Auto + ); + // env wins over file + assert_eq!( + resolve_credential_store_with(None, Some("file"), &file), + CredentialStore::File + ); + // file wins over default + assert_eq!( + resolve_credential_store_with(None, None, &file), + CredentialStore::Keyring + ); + } + + #[test] + fn resolution_defaults_to_keyring() { + assert_eq!( + resolve_credential_store_with(None, None, &EngineConfig::default()), + CredentialStore::Keyring + ); + } + + #[test] + fn resolution_ignores_invalid_env_and_falls_through() { + let file = EngineConfig { + credentials: CredentialsConfig { + store: Some(CredentialStore::File), + }, + }; + // invalid env is ignored, so the file value applies + assert_eq!( + resolve_credential_store_with(None, Some("garbage"), &file), + CredentialStore::File + ); + // invalid env with no file falls through to the default + assert_eq!( + resolve_credential_store_with(None, Some("garbage"), &EngineConfig::default()), + CredentialStore::Keyring + ); + } + + #[test] + fn config_file_path_rejects_unsafe_app_id() { + assert_eq!(config_file_path("../evil"), None); + assert_eq!(config_file_path("a/b"), None); + } + + #[test] + fn credential_store_flag_encodes_round_trips() { + for store in [ + None, + Some(CredentialStore::Auto), + Some(CredentialStore::Keyring), + Some(CredentialStore::File), + ] { + assert_eq!(decode_store(encode_store(store)), store); + } + } + + #[test] + fn config_file_path_uses_xdg_config_home() { + let dir = std::env::temp_dir().join("cli-engine-config-path-test"); + test_env::with_xdg_config_home(&dir, || { + assert_eq!( + config_file_path("myapp"), + Some(dir.join("myapp").join("config.toml")) + ); + }); + } + + #[derive(Debug, Deserialize, PartialEq)] + struct Deploy { + region: String, + replicas: u32, + } + + fn doc_config(toml: &str) -> ConfigFile { + ConfigFile { + path: None, + doc: toml.parse().expect("valid toml"), + } + } + + #[test] + fn section_reads_consumer_table() { + let cfg = doc_config("[deploy]\nregion = \"us-west\"\nreplicas = 3\n"); + let deploy: Deploy = cfg.section("deploy").expect("ok").expect("present"); + assert_eq!( + deploy, + Deploy { + region: "us-west".to_owned(), + replicas: 3 + } + ); + assert!(cfg.section::("absent").expect("ok").is_none()); + } + + #[test] + fn engine_and_consumer_sections_coexist() { + let cfg = doc_config( + "[credentials]\nstore = \"file\"\n[deploy]\nregion = \"eu\"\nreplicas = 1\n", + ); + assert_eq!(cfg.engine().credentials.store, Some(CredentialStore::File)); + assert_eq!( + cfg.section::("deploy") + .expect("ok") + .expect("present") + .region, + "eu" + ); + } + + #[test] + fn get_reads_dotted_scalar() { + let cfg = doc_config("[credentials]\nstore = \"file\"\n[deploy]\nreplicas = 3\n"); + assert_eq!(cfg.get("credentials.store").as_deref(), Some("file")); + assert_eq!(cfg.get("deploy.replicas").as_deref(), Some("3")); + assert_eq!(cfg.get("deploy.missing"), None); + assert_eq!(cfg.get("nope.at.all"), None); + } + + #[test] + fn set_infers_scalar_types() { + let mut cfg = ConfigFile::default(); + cfg.set("telemetry.enabled", "true").expect("set bool"); + cfg.set("deploy.replicas", "5").expect("set int"); + cfg.set("deploy.region", "us-west").expect("set str"); + assert_eq!(cfg.get("telemetry.enabled").as_deref(), Some("true")); + assert_eq!(cfg.get("deploy.replicas").as_deref(), Some("5")); + assert_eq!(cfg.get("deploy.region").as_deref(), Some("us-west")); + // bool/int stored as scalars, not quoted strings + assert!(cfg.doc.to_string().contains("enabled = true")); + assert!(cfg.doc.to_string().contains("replicas = 5")); + } + + #[test] + fn set_validates_engine_store_key() { + let mut cfg = ConfigFile::default(); + assert!(cfg.set("credentials.store", "bogus").is_err()); + assert!(cfg.set("credentials.store", "file").is_ok()); + assert_eq!(cfg.engine().credentials.store, Some(CredentialStore::File)); + } + + #[test] + fn set_rejects_empty_key_segments() { + let mut cfg = ConfigFile::default(); + assert!(cfg.set("a..b", "x").is_err()); + assert!(cfg.set("", "x").is_err()); + } + + #[test] + fn set_preserves_comments_and_other_tables() { + let mut cfg = + doc_config("# keep me\n[credentials]\nstore = \"file\"\n\n[deploy]\nregion = \"us\"\n"); + cfg.set("deploy.region", "eu").expect("set"); + let rendered = cfg.doc.to_string(); + assert!( + rendered.contains("# keep me"), + "comment preserved: {rendered}" + ); + assert!( + rendered.contains("store = \"file\""), + "other table preserved" + ); + assert!(rendered.contains("region = \"eu\""), "value updated"); + } + + #[test] + fn load_and_save_round_trip() { + let dir = tempfile::tempdir().expect("tempdir"); + test_env::with_xdg_config_home(dir.path(), || { + let mut cfg = ConfigFile::load("roundtrip"); + assert!(cfg.path().is_some()); + cfg.set("deploy.region", "us-west").expect("set"); + cfg.save().expect("save"); + // Reload from disk and confirm persistence. + let reloaded = ConfigFile::load("roundtrip"); + assert_eq!(reloaded.get("deploy.region").as_deref(), Some("us-west")); + }); + } + + #[test] + fn malformed_file_loads_as_empty() { + let dir = tempfile::tempdir().expect("tempdir"); + test_env::with_xdg_config_home(dir.path(), || { + let path = config_file_path("broken").expect("path"); + std::fs::create_dir_all(path.parent().expect("parent")).expect("mkdir"); + std::fs::write(&path, "not = valid = toml").expect("write"); + let cfg = ConfigFile::load("broken"); + assert_eq!(cfg.engine().credentials.store, None); + assert_eq!(cfg.get("anything"), None); + }); + } + + #[test] + fn default_config_has_no_path_and_save_errors() { + let cfg = ConfigFile::default(); + assert!(cfg.path().is_none()); + assert!(cfg.save().is_err(), "save without a path should error"); + } +} diff --git a/src/config_commands.rs b/src/config_commands.rs new file mode 100644 index 0000000..18080a0 --- /dev/null +++ b/src/config_commands.rs @@ -0,0 +1,104 @@ +//! Built-in `config` command group for reading and writing the per-application +//! [config file](crate::config::ConfigFile). +//! +//! Mount it on a CLI with +//! [`CliConfig::with_config_commands`](crate::cli::CliConfig::with_config_commands). +//! The group exposes: +//! +//! - `config path` — print the resolved config file path. +//! - `config get ` — print the value at a dotted key (e.g. `deploy.region`). +//! - `config set ` — set a value and persist (mutating; dry-run aware). +//! - `config list` — print the full config file contents. + +use clap::Arg; +use serde_json::{Value, json}; + +use crate::config::{ConfigFile, config_file_path}; +use crate::{CommandResult, CommandSpec, GroupSpec, RuntimeCommandSpec, RuntimeGroupSpec, Tier}; + +/// Builds the built-in runtime `config` command group. +#[must_use] +pub fn config_command_group() -> RuntimeGroupSpec { + RuntimeGroupSpec::new(GroupSpec::new( + "config", + "Read and write the CLI config file", + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("path", "Print the config file path") + .with_system("config") + .no_auth(true), + async |context| { + let path = + config_file_path(&context.middleware.app_id).map(|p| p.display().to_string()); + Ok(CommandResult::new(json!({ "path": path }))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("get", "Print a config value by dotted key") + .with_system("config") + .no_auth(true) + .with_arg( + Arg::new("key") + .value_name("KEY") + .required(true) + .help("Dotted key, e.g. credentials.store or deploy.region"), + ), + async |context| { + let key = string_arg(&context.args, "key"); + let value = context.config().get(&key); + Ok(CommandResult::new(json!({ "key": key, "value": value }))) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("set", "Set a config value and save") + .with_system("config") + .with_tier(Tier::Mutate) + .mutates(true) + .no_auth(true) + .with_arg( + Arg::new("key") + .value_name("KEY") + .required(true) + .help("Dotted key, e.g. credentials.store or deploy.region"), + ) + .with_arg( + Arg::new("value") + .value_name("VALUE") + .required(true) + .help("Value (parsed as bool/int/float when possible, else string)"), + ), + async |context| { + let key = string_arg(&context.args, "key"); + let value = string_arg(&context.args, "value"); + // Load fresh from disk (not the startup snapshot) so a concurrent + // external edit is not clobbered, then set + save. + let mut config = ConfigFile::load(&context.middleware.app_id); + config.set(&key, &value)?; + config.save()?; + let path = config.path().map(|p| p.display().to_string()); + Ok(CommandResult::new( + json!({ "key": key, "value": value, "path": path }), + )) + }, + )) + .with_command(RuntimeCommandSpec::new_with_context( + CommandSpec::new("list", "Print the full config file contents") + .with_system("config") + .no_auth(true), + async |context| { + let path = context.config().path().map(|p| p.display().to_string()); + Ok(CommandResult::new(json!({ + "path": path, + "contents": context.config().to_toml_string(), + }))) + }, + )) +} + +/// Reads a required string argument, defaulting to empty when absent. +fn string_arg(args: &serde_json::Map, name: &str) -> String { + args.get(name) + .and_then(Value::as_str) + .unwrap_or_default() + .to_owned() +} diff --git a/src/flags.rs b/src/flags.rs index eab67a1..068a157 100644 --- a/src/flags.rs +++ b/src/flags.rs @@ -35,6 +35,8 @@ pub struct GlobalFlags { pub debug: String, /// Search query. pub search: String, + /// Credential storage override from `--credential-store`, if supplied. + pub credential_store: Option, } impl Default for GlobalFlags { @@ -53,6 +55,7 @@ impl Default for GlobalFlags { timeout: "0s".to_owned(), debug: String::new(), search: String::new(), + credential_store: None, } } } @@ -171,6 +174,14 @@ pub fn register_global_flags(command: Command) -> Command { .value_name("KEYWORD") .help("Search commands and guides by keyword"), ) + .arg( + Arg::new("credential-store") + .long("credential-store") + .global(true) + .value_name("MODE") + .value_parser(|s: &str| s.parse::()) + .help("Credential storage: auto|keyring|file (overrides env and config)"), + ) .arg( Arg::new("json") .long("json") @@ -214,11 +225,16 @@ pub fn resolve_default_output_format(env_override: Option<&str>, is_tty: bool) - if is_tty { "human" } else { "json" }.to_owned() } -/// Derives the per-application output-format override env var from an app id, -/// e.g. `godaddy` -> `GODADDY_OUTPUT`, `gdx` -> `GDX_OUTPUT`. +/// Sanitizes an app id into an environment-variable prefix: ASCII alphanumerics +/// are uppercased and every other character becomes `_`, e.g. `godaddy` -> +/// `GODADDY`, `my-cli` -> `MY_CLI`. +/// +/// Shared by the framework's app-scoped env vars (for example +/// [`output_env_var`] and `${PREFIX}_CREDENTIAL_STORE`) so they derive the same +/// prefix from a given app id. #[must_use] -pub fn output_env_var(app_id: &str) -> String { - let sanitized: String = app_id +pub fn app_id_env_prefix(app_id: &str) -> String { + app_id .chars() .map(|c| { if c.is_ascii_alphanumeric() { @@ -227,8 +243,14 @@ pub fn output_env_var(app_id: &str) -> String { '_' } }) - .collect(); - format!("{sanitized}_OUTPUT") + .collect() +} + +/// Derives the per-application output-format override env var from an app id, +/// e.g. `godaddy` -> `GODADDY_OUTPUT`, `gdx` -> `GDX_OUTPUT`. +#[must_use] +pub fn output_env_var(app_id: &str) -> String { + format!("{}_OUTPUT", app_id_env_prefix(app_id)) } /// Computes the default output format for `app_id`, consulting the @@ -298,6 +320,9 @@ pub fn global_flags_from_matches(matches: &ArgMatches, default_format: &str) -> .get_one::("search") .cloned() .unwrap_or_default(), + credential_store: matches + .get_one::("credential-store") + .copied(), } } diff --git a/src/fs.rs b/src/fs.rs new file mode 100644 index 0000000..30dd69c --- /dev/null +++ b/src/fs.rs @@ -0,0 +1,249 @@ +//! Filesystem and path utilities shared across the engine. +//! +//! These primitives back both the engine [config file](crate::config) and +//! [credential storage](crate::auth::storage): resolving the per-user base +//! directory, validating untrusted path components, and writing files +//! atomically with restrictive permissions. They are domain-agnostic so callers +//! that persist their own files can reuse them rather than re-implementing the +//! same path safety and atomic-write logic. + +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicU64, Ordering}; + +use crate::error::CliCoreError; + +/// Reads `key` from the environment as a non-empty path, or `None`. +fn env_path(key: &str) -> Option { + std::env::var(key) + .ok() + .filter(|v| !v.is_empty()) + .map(PathBuf::from) +} + +/// XDG-conventional `$HOME/.config`, if `HOME` is set. +fn home_config_dir() -> Option { + env_path("HOME").map(|home| home.join(".config")) +} + +/// Resolves the per-user base directory for an app's config and data files. +/// +/// Returns `$XDG_CONFIG_HOME` when set, else `$HOME/.config` (or `%APPDATA%` on +/// Windows). Only absolute paths are accepted; a relative value is rejected so +/// files never land relative to the current working directory. +#[must_use] +pub fn config_base_dir() -> Option { + env_path("XDG_CONFIG_HOME") + .or_else(|| { + // On Windows prefer APPDATA over HOME/.config: HOME is often set by + // Git Bash/MSYS shells and would place files in a non-standard + // location. On all other platforms prefer XDG-conventional + // HOME/.config, falling back to APPDATA only as a last resort. + // `cfg!(windows)` keeps both branches compiled (and type-checked) + // on every platform. + if cfg!(windows) { + env_path("APPDATA").or_else(home_config_dir) + } else { + home_config_dir().or_else(|| env_path("APPDATA")) + } + }) + // Reject relative paths: a relative XDG_CONFIG_HOME/APPDATA/HOME would + // silently place files relative to the current working directory. + .filter(|p| p.is_absolute()) +} + +/// Returns true only when `s` is a single, non-traversal path component that is +/// valid on all supported platforms. +/// +/// Use this to validate untrusted segments (app ids, environment names, etc.) +/// before joining them into a path. +/// +/// Rejects: +/// - empty strings, `.`, and `..` +/// - strings containing `/` or `\` (path separators on any platform) +/// - Windows-forbidden filename characters: `: * ? " < > |` +/// - ASCII control characters (bytes 0x00–0x1F) and the DEL character (0x7F) +/// - leading or trailing space (leading space is invisible in directory listings) +/// - trailing `.` (valid on Unix but rejected by Windows) +/// - Windows reserved device names (`CON`, `NUL`, `COM1`, etc.) with or without extension +#[must_use] +pub fn is_safe_path_component(s: &str) -> bool { + // '/' is listed explicitly because Path::components() silently strips trailing + // slashes — "prod/" parses as a single Normal("prod") component and would + // otherwise pass the components check below. + const FORBIDDEN: &[char] = &['/', '\\', ':', '*', '?', '"', '<', '>', '|']; + if s.contains(FORBIDDEN) || s.bytes().any(|b| b < 0x20 || b == 0x7F) { + return false; + } + if s.starts_with(' ') || s.ends_with('.') || s.ends_with(' ') { + return false; + } + // Windows treats these device names as special regardless of extension, + // e.g. opening "NUL.json" writes to the null device, not a file. + const RESERVED: &[&str] = &[ + "CON", "PRN", "AUX", "NUL", "COM0", "COM1", "COM2", "COM3", "COM4", "COM5", "COM6", "COM7", + "COM8", "COM9", "LPT0", "LPT1", "LPT2", "LPT3", "LPT4", "LPT5", "LPT6", "LPT7", "LPT8", + "LPT9", + ]; + let stem = Path::new(s) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or(s); + if RESERVED.iter().any(|r| stem.eq_ignore_ascii_case(r)) { + return false; + } + let mut components = Path::new(s).components(); + matches!(components.next(), Some(std::path::Component::Normal(_))) + && components.next().is_none() +} + +/// Writes `contents` to `path` via a uniquely-named temp file then renames it +/// into place. On Unix the rename is atomic, the file is created `0600`, and the +/// parent directory is best-effort restricted to `0700`. On Windows the rename +/// replaces an existing destination but is not crash-atomic. +/// +/// # Errors +/// Returns an error when the directory cannot be created or the write/rename +/// fails. +pub fn write_string_atomic(path: &Path, contents: &str) -> crate::Result<()> { + if let Some(parent) = path.parent() { + std::fs::create_dir_all(parent) + .map_err(|e| CliCoreError::message(format!("failed to create directory: {e}")))?; + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + if let Err(e) = std::fs::set_permissions(parent, std::fs::Permissions::from_mode(0o700)) + { + tracing::debug!( + path = %parent.display(), + error = %e, + "could not restrict directory permissions" + ); + } + } + } + // Unique temp name without pulling in `rand`: pid plus a monotonic counter is + // unique within a process, and the pid differs across processes. + static TMP_COUNTER: AtomicU64 = AtomicU64::new(0); + let unique = TMP_COUNTER.fetch_add(1, Ordering::Relaxed); + let pid = std::process::id(); + let tmp_path = path.with_file_name(format!( + "{}.{pid:x}.{unique:x}.tmp", + path.file_name().and_then(|s| s.to_str()).unwrap_or("tmp"), + )); + write_tmp_file(&tmp_path, contents)?; + if let Err(e) = std::fs::rename(&tmp_path, path) { + std::fs::remove_file(&tmp_path).ok(); + return Err(CliCoreError::message(format!( + "failed to finalize {}: {e}", + path.display() + ))); + } + Ok(()) +} + +/// Opens `tmp_path` with `O_CREAT|O_EXCL` and writes `contents`, mode `0600` on +/// Unix so files are never world-readable. +fn write_tmp_file(tmp_path: &Path, contents: &str) -> crate::Result<()> { + use std::io::Write as _; + let mut opts = std::fs::OpenOptions::new(); + opts.write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + opts.mode(0o600); + } + let mut file = opts.open(tmp_path).map_err(|e| { + CliCoreError::message(format!("failed to write {}: {e}", tmp_path.display())) + })?; + file.write_all(contents.as_bytes()) + .map_err(|e| CliCoreError::message(format!("failed to write {}: {e}", tmp_path.display()))) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::config::test_env::with_xdg_config_home; + + #[test] + fn safe_path_component_basic() { + assert!(is_safe_path_component("godaddy")); + assert!(!is_safe_path_component("..")); + assert!(!is_safe_path_component("")); + assert!(!is_safe_path_component("a/b")); + assert!(!is_safe_path_component("NUL")); + } + + #[test] + fn safe_path_component_rejects_windows_reserved_names() { + for name in &[ + "CON", "con", "NUL", "nul", "COM1", "LPT9", "CON.txt", "NUL.json", + ] { + assert!( + !is_safe_path_component(name), + "{name:?} should be rejected as a Windows reserved name" + ); + } + } + + #[test] + fn safe_path_component_rejects_control_and_space_edges() { + assert!(!is_safe_path_component(" prod"), "leading space"); + assert!(!is_safe_path_component("prod\x7f"), "DEL byte"); + assert!(!is_safe_path_component("prod."), "trailing dot"); + assert!(!is_safe_path_component("prod "), "trailing space"); + } + + #[test] + fn safe_path_component_accepts_normal_values() { + for name in &["dev", "prod", "staging", "my-app", "my_app", "app.v2"] { + assert!(is_safe_path_component(name), "{name:?} should be accepted"); + } + } + + #[test] + fn config_base_dir_rejects_relative_xdg() { + with_xdg_config_home(Path::new("."), || { + assert!( + config_base_dir().is_none(), + "relative XDG_CONFIG_HOME should be rejected" + ); + }); + } + + #[test] + fn config_base_dir_honors_xdg() { + let dir = std::env::temp_dir().join("cli-engine-fs-base-test"); + with_xdg_config_home(&dir, || { + assert_eq!(config_base_dir(), Some(dir.clone())); + }); + } + + #[tokio::test] + async fn write_string_atomic_round_trip_creates_dirs() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("nested").join("file.txt"); + write_string_atomic(&path, "hello").expect("write"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "hello"); + // Overwrite replaces the contents. + write_string_atomic(&path, "world").expect("rewrite"); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "world"); + // No stray temp files remain alongside the target. + let strays: Vec<_> = std::fs::read_dir(path.parent().expect("parent")) + .expect("read_dir") + .filter_map(|e| e.ok()) + .filter(|e| e.file_name().to_string_lossy().ends_with(".tmp")) + .collect(); + assert!(strays.is_empty(), "temp files should be renamed away"); + } + + #[cfg(unix)] + #[tokio::test] + async fn write_string_atomic_sets_owner_only_mode() { + use std::os::unix::fs::PermissionsExt as _; + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("secret.txt"); + write_string_atomic(&path, "s3cr3t").expect("write"); + let mode = std::fs::metadata(&path).expect("meta").permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "file should be owner read/write only"); + } +} diff --git a/src/lib.rs b/src/lib.rs index b20330f..ffad838 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -67,10 +67,16 @@ pub mod auth; pub mod cli; /// Command and command-group specifications. pub mod command; +/// Engine config file and credential-storage selection. +pub mod config; +/// Built-in `config` command group. +pub mod config_commands; /// Shared error type and error traits. pub mod error; /// Global framework flags and flag-extraction helpers. pub mod flags; +/// Filesystem and path utilities (base dir, path-component safety, atomic write). +pub mod fs; /// Embedded or file-backed guide parsing. pub mod guide; /// Cross-cutting command execution middleware. @@ -88,6 +94,11 @@ pub mod transport; /// Command tree data model and human rendering. pub mod tree; +#[cfg(feature = "pkce-auth")] +pub use auth::storage::{AutoStorage, KeyringStorage}; +pub use auth::storage::{ + CredentialKey, CredentialStorage, FileStorage, default_storage, storage_for, +}; pub use auth::{ AuthLoginResult, AuthProvider, AuthStatusEntry, CACHE_TTL, Credential, CredentialRequest, Dispatcher, SingleProvider, StatusEntry, auth_command_group, login_and_build, @@ -104,11 +115,16 @@ pub use command::{ StreamingCommandFuture, StreamingCommandHandler, command_args_from_matches, command_path_from_matches, command_path_from_parts, leaf_matches, }; +pub use config::{ + ConfigFile, CredentialStore, CredentialsConfig, EngineConfig, ParseCredentialStoreError, + credential_store_env_var, resolve_credential_store, resolve_credential_store_with, +}; +pub use config_commands::config_command_group; pub use error::{ CliCoreError, DetailedError, ExitCoder, Result, exit_code_for_error, exit_code_for_exit_coder, }; pub use flags::{ - GlobalFlags, default_output_format, derive_bool_flags, derive_value_flags, + GlobalFlags, app_id_env_prefix, default_output_format, derive_bool_flags, derive_value_flags, extract_command_path, extract_output_format, extract_search_query, global_flags_from_matches, has_true_schema_flag, output_env_var, register_global_flags, resolve_default_output_format, }; diff --git a/src/middleware.rs b/src/middleware.rs index 59888a1..168e701 100644 --- a/src/middleware.rs +++ b/src/middleware.rs @@ -506,6 +506,14 @@ pub struct Middleware { pub schema_registry: SchemaRegistry, /// Human output view registry. pub human_views: HumanViewRegistry, + /// Loaded per-application config file, shared across the run. + /// + /// Populated once at startup from `//config.toml`. + /// Command handlers read it via + /// [`CommandContext::config`](crate::command::CommandContext::config) and + /// module registration via + /// [`ModuleContext::config`](crate::module::ModuleContext::config). + pub config: Arc, } /// Rendered result produced by middleware. diff --git a/src/module.rs b/src/module.rs index 2dfae6a..98a851b 100644 --- a/src/module.rs +++ b/src/module.rs @@ -155,6 +155,14 @@ impl<'middleware> ModuleContext<'middleware> { self.middleware } + /// Returns the loaded per-application config file for registration-time use. + /// + /// Read a consumer-owned section with + /// [`ConfigFile::section`](crate::config::ConfigFile::section). + pub fn config(&self) -> &crate::config::ConfigFile { + &self.middleware.config + } + /// Returns the schema registry for direct registration. pub fn schema_registry(&mut self) -> &mut SchemaRegistry { &mut self.middleware.schema_registry diff --git a/tests/config_file.rs b/tests/config_file.rs new file mode 100644 index 0000000..e557716 --- /dev/null +++ b/tests/config_file.rs @@ -0,0 +1,199 @@ +//! End-to-end coverage for the per-CLI config file: a consumer command reading +//! its own section, and the built-in `config get/set/path` group. +//! +//! Each test serializes on a shared lock and points `XDG_CONFIG_HOME` at a temp +//! dir. A fresh `Cli` is built per `run` so each invocation reloads the file from +//! disk, matching how a real one-shot CLI process behaves. +#![allow(unsafe_code)] +#![allow(clippy::await_holding_lock)] + +use std::path::Path; +use std::sync::{Mutex, MutexGuard}; + +use cli_engine::{ + Cli, CliConfig, CommandResult, CommandSpec, GroupSpec, Module, RuntimeCommandSpec, + RuntimeGroupSpec, +}; +use serde::Deserialize; +use serde_json::json; + +const APP_ID: &str = "cfgtest"; + +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +fn lock() -> MutexGuard<'static, ()> { + ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} + +struct EnvGuard { + prev: Option, +} + +impl EnvGuard { + fn set(path: &Path) -> Self { + let prev = std::env::var("XDG_CONFIG_HOME").ok(); + // SAFETY: caller holds ENV_LOCK for the guard's lifetime. + unsafe { std::env::set_var("XDG_CONFIG_HOME", path) }; + Self { prev } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: caller holds ENV_LOCK for the guard's lifetime. + unsafe { + match &self.prev { + Some(v) => std::env::set_var("XDG_CONFIG_HOME", v), + None => std::env::remove_var("XDG_CONFIG_HOME"), + } + } + } +} + +#[derive(Debug, Deserialize)] +struct Deploy { + region: String, +} + +fn write_config(xdg: &Path, contents: &str) { + let dir = xdg.join(APP_ID); + std::fs::create_dir_all(&dir).expect("mkdir"); + std::fs::write(dir.join("config.toml"), contents).expect("write config"); +} + +fn config_contents(xdg: &Path) -> Option { + std::fs::read_to_string(xdg.join(APP_ID).join("config.toml")).ok() +} + +/// Builds a fresh CLI (reloading config from disk) with a consumer `deploy show` +/// command and the built-in `config` group mounted. +fn build_cli() -> Cli { + let module = Module::new("Deploy", |_ctx| { + RuntimeGroupSpec::new(GroupSpec::new("deploy", "Deploy things")).with_command( + RuntimeCommandSpec::new_with_context( + CommandSpec::new("show", "Show configured region") + .with_system("deploy") + .no_auth(true), + async |ctx| { + let region = ctx + .config() + .section::("deploy")? + .map(|d| d.region) + .unwrap_or_else(|| "".to_owned()); + Ok(CommandResult::new(json!({ "region": region }))) + }, + ), + ) + }); + Cli::new( + CliConfig::new(APP_ID, "Config test CLI", APP_ID) + .with_config_commands() + .with_module(module), + ) +} + +#[tokio::test] +async fn handler_reads_consumer_section() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _env = EnvGuard::set(dir.path()); + write_config(dir.path(), "[deploy]\nregion = \"us-west\"\n"); + + let out = build_cli().run(["cfgtest", "deploy", "show"]).await; + assert_eq!(out.exit_code, 0, "{}", out.rendered); + assert!( + out.rendered.contains("us-west"), + "handler should read [deploy].region: {}", + out.rendered + ); +} + +#[tokio::test] +async fn config_set_then_get_roundtrips_and_preserves_other_tables() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _env = EnvGuard::set(dir.path()); + write_config( + dir.path(), + "# keep this comment\n[credentials]\nstore = \"file\"\n", + ); + + let set = build_cli() + .run(["cfgtest", "config", "set", "deploy.region", "eu-west"]) + .await; + assert_eq!(set.exit_code, 0, "{}", set.rendered); + + // Fresh CLI reloads from disk and sees the new value. + let get = build_cli() + .run(["cfgtest", "config", "get", "deploy.region"]) + .await; + assert_eq!(get.exit_code, 0, "{}", get.rendered); + assert!(get.rendered.contains("eu-west"), "{}", get.rendered); + + // The engine table and the comment survived the write. + let on_disk = config_contents(dir.path()).expect("file exists"); + assert!(on_disk.contains("store = \"file\""), "{on_disk}"); + assert!(on_disk.contains("# keep this comment"), "{on_disk}"); + + // Engine still reads its reserved key. + let store = build_cli() + .run(["cfgtest", "config", "get", "credentials.store"]) + .await; + assert!(store.rendered.contains("file"), "{}", store.rendered); +} + +#[tokio::test] +async fn config_set_dry_run_does_not_write() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _env = EnvGuard::set(dir.path()); + // No config file exists yet. + + let out = build_cli() + .run([ + "cfgtest", + "config", + "set", + "deploy.region", + "eu-west", + "--dry-run", + ]) + .await; + assert_eq!(out.exit_code, 0, "{}", out.rendered); + assert!( + config_contents(dir.path()).is_none(), + "dry-run must not create the config file" + ); +} + +#[tokio::test] +async fn config_path_prints_path() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _env = EnvGuard::set(dir.path()); + + let out = build_cli().run(["cfgtest", "config", "path"]).await; + assert_eq!(out.exit_code, 0, "{}", out.rendered); + assert!(out.rendered.contains("cfgtest"), "{}", out.rendered); + assert!(out.rendered.contains("config.toml"), "{}", out.rendered); +} + +#[tokio::test] +async fn config_set_rejects_invalid_engine_value() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _env = EnvGuard::set(dir.path()); + + let out = build_cli() + .run(["cfgtest", "config", "set", "credentials.store", "bogus"]) + .await; + assert_ne!( + out.exit_code, 0, + "invalid engine value should fail: {}", + out.rendered + ); + assert!( + config_contents(dir.path()).is_none(), + "rejected set must not write the file" + ); +} diff --git a/tests/credential_store_config.rs b/tests/credential_store_config.rs new file mode 100644 index 0000000..31eb638 --- /dev/null +++ b/tests/credential_store_config.rs @@ -0,0 +1,239 @@ +//! End-to-end coverage for credential-store selection via config file, env var, +//! and the `--credential-store` flag, exercised through `Cli::run` with a real +//! `PkceAuthProvider`. +//! +//! These tests mutate process-global state (`XDG_CONFIG_HOME`, the +//! `ITEST_CREDENTIAL_STORE` env var, and the `--credential-store` flag latch), +//! so they serialize on a shared lock. The file-storage backend is the seam we +//! assert against: a credential file is read only in `file`/`auto` modes, never +//! in the default `keyring` mode — so "status shows logged in" cleanly +//! distinguishes which backend the engine selected without needing a keychain +//! daemon or a browser login. +#![cfg(feature = "pkce-auth")] +#![allow(unsafe_code)] +// These tests serialize on a std Mutex and hold the guard across `.await` to keep +// process-global env mutations race-free; that is the intent, not a bug. +#![allow(clippy::await_holding_lock)] + +use std::path::Path; +use std::sync::{Arc, Mutex, MutexGuard}; +use std::time::{SystemTime, UNIX_EPOCH}; + +use cli_engine::auth::pkce::PkceAuthProvider; +use cli_engine::{Cli, CliConfig}; + +const APP_ID: &str = "itest"; +const ENV_VAR: &str = "ITEST_CREDENTIAL_STORE"; + +/// Serializes the process-global mutations these tests perform. +static ENV_LOCK: Mutex<()> = Mutex::new(()); + +fn lock() -> MutexGuard<'static, ()> { + ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()) +} + +/// RAII guard that restores an env var when dropped. Caller must hold [`lock`]. +struct EnvGuard { + key: String, + prev: Option, +} + +impl EnvGuard { + fn set(key: &str, value: Option<&str>) -> Self { + let prev = std::env::var(key).ok(); + // SAFETY: caller holds ENV_LOCK for the guard's lifetime. + unsafe { + match value { + Some(v) => std::env::set_var(key, v), + None => std::env::remove_var(key), + } + } + Self { + key: key.to_owned(), + prev, + } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + // SAFETY: caller holds ENV_LOCK for the guard's lifetime. + unsafe { + match &self.prev { + Some(v) => std::env::set_var(&self.key, v), + None => std::env::remove_var(&self.key), + } + } + } +} + +/// Writes a non-expired credential file at the path `FileStorage` reads. +fn seed_credential_file(xdg: &Path) { + let dir = xdg.join(APP_ID).join("credentials"); + std::fs::create_dir_all(&dir).expect("create credentials dir"); + let expires_at = SystemTime::now() + .duration_since(UNIX_EPOCH) + .expect("clock") + .as_secs() + + 3600; + let json = format!( + "{{\"access_token\":\"itok\",\"expires_at\":{expires_at},\"refresh_token\":null,\"scopes\":[]}}" + ); + std::fs::write(dir.join("primary-dev.json"), json).expect("write credential file"); +} + +/// Writes `//config.toml` with the given `[credentials].store`. +fn write_config(xdg: &Path, store: &str) { + let dir = xdg.join(APP_ID); + std::fs::create_dir_all(&dir).expect("create config dir"); + std::fs::write( + dir.join("config.toml"), + format!("[credentials]\nstore = \"{store}\"\n"), + ) + .expect("write config"); +} + +fn build_cli() -> Cli { + let provider = Arc::new( + PkceAuthProvider::new( + "primary", + "https://example.com/auth", + "https://example.com/token", + "client-id", + &["openid"], + ) + .with_app_id(APP_ID), + ); + Cli::new( + CliConfig::new(APP_ID, "Integration CLI", APP_ID) + .with_auth_provider(provider) + .with_default_auth_provider("primary"), + ) +} + +#[tokio::test] +async fn config_file_selects_file_store() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _xdg = EnvGuard::set("XDG_CONFIG_HOME", Some(&dir.path().to_string_lossy())); + let _env = EnvGuard::set(ENV_VAR, None); + write_config(dir.path(), "file"); + seed_credential_file(dir.path()); + + let out = build_cli() + .run(["itest", "auth", "status", "--env", "dev"]) + .await; + + assert_eq!(out.exit_code, 0, "expected success, got: {}", out.rendered); + assert!( + out.rendered.contains("dev"), + "status should report the env: {}", + out.rendered + ); + assert!( + !out.rendered.contains("not logged in"), + "file store should find the seeded credential: {}", + out.rendered + ); +} + +#[tokio::test] +async fn default_keyring_mode_ignores_credential_file() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _xdg = EnvGuard::set("XDG_CONFIG_HOME", Some(&dir.path().to_string_lossy())); + let _env = EnvGuard::set(ENV_VAR, None); + // No config file => default Keyring mode. A credential file exists but must + // be ignored (keyring-only never reads the file). + seed_credential_file(dir.path()); + + let out = build_cli() + .run(["itest", "auth", "status", "--env", "dev"]) + .await; + + assert_ne!(out.exit_code, 0, "expected not-logged-in: {}", out.rendered); + assert!( + out.rendered.contains("not logged in"), + "keyring mode must not read the credential file: {}", + out.rendered + ); +} + +#[tokio::test] +async fn env_var_overrides_config() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _xdg = EnvGuard::set("XDG_CONFIG_HOME", Some(&dir.path().to_string_lossy())); + // Config says keyring, env says file: env wins, so the file is read. + write_config(dir.path(), "keyring"); + let _env = EnvGuard::set(ENV_VAR, Some("file")); + seed_credential_file(dir.path()); + + let out = build_cli() + .run(["itest", "auth", "status", "--env", "dev"]) + .await; + + assert_eq!( + out.exit_code, 0, + "env override should win: {}", + out.rendered + ); + assert!(!out.rendered.contains("not logged in"), "{}", out.rendered); +} + +#[tokio::test] +async fn flag_overrides_env() { + let _guard = lock(); + let dir = tempfile::tempdir().expect("tempdir"); + let _xdg = EnvGuard::set("XDG_CONFIG_HOME", Some(&dir.path().to_string_lossy())); + // Env says keyring, flag says file: flag wins. + let _env = EnvGuard::set(ENV_VAR, Some("keyring")); + seed_credential_file(dir.path()); + + let out = build_cli() + .run([ + "itest", + "--credential-store", + "file", + "auth", + "status", + "--env", + "dev", + ]) + .await; + + assert_eq!( + out.exit_code, 0, + "flag override should win: {}", + out.rendered + ); + assert!(!out.rendered.contains("not logged in"), "{}", out.rendered); + + // Reset the flag latch so later tests in this binary see no flag. + let reset = build_cli() + .run(["itest", "auth", "status", "--env", "dev"]) + .await; + assert_ne!(reset.exit_code, 0, "{}", reset.rendered); +} + +#[tokio::test] +async fn invalid_credential_store_flag_is_rejected() { + let _guard = lock(); + let out = build_cli() + .run([ + "itest", + "--credential-store", + "vault", + "auth", + "status", + "--env", + "dev", + ]) + .await; + assert_ne!(out.exit_code, 0, "invalid value should be a usage error"); + // Reset the flag latch for subsequent tests. + let reset = build_cli() + .run(["itest", "auth", "status", "--env", "dev"]) + .await; + assert_ne!(reset.exit_code, 0, "{}", reset.rendered); +} diff --git a/tests/exhaustive_public_api.rs b/tests/exhaustive_public_api.rs index dada90e..fed88b9 100644 --- a/tests/exhaustive_public_api.rs +++ b/tests/exhaustive_public_api.rs @@ -131,6 +131,8 @@ fn parsed_global_flags_cover_defaults_short_aliases_and_optional_values() { "5m", "--search", "projects", + "--credential-store", + "file", "project", "list", "--team", @@ -159,6 +161,7 @@ fn parsed_global_flags_cover_defaults_short_aliases_and_optional_values() { timeout: "5m".to_owned(), debug: "transport".to_owned(), search: "projects".to_owned(), + credential_store: Some(cli_engine::CredentialStore::File), } ); } diff --git a/tests/foundation.rs b/tests/foundation.rs index c9700b2..d96d202 100644 --- a/tests/foundation.rs +++ b/tests/foundation.rs @@ -3744,6 +3744,7 @@ fn global_flag_defaults_and_derived_flag_classes_cover_common_clap_actions() { timeout: "0s".to_owned(), debug: String::new(), search: String::new(), + credential_store: None, } ); From 41bd38ab3504ca6ba0881dfc41aa5d25a989f9ce Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Fri, 12 Jun 2026 08:36:50 -0700 Subject: [PATCH 2/4] refactor(config): address Copilot review - ConfigFile caches a parsed read-model at load (refreshed on set) instead of reparsing the whole document on every section/deserialize/engine call. - Restrict the process-global credential-store flag accessors (set_credential_store_flag / credential_store_flag) to pub(crate). - save() error message now mentions %APPDATA% alongside XDG_CONFIG_HOME/HOME. Co-Authored-By: Claude Opus 4.8 --- src/config.rs | 58 +++++++++++++++++++++++++++++++++++---------------- 1 file changed, 40 insertions(+), 18 deletions(-) diff --git a/src/config.rs b/src/config.rs index 132a88b..ae593a7 100644 --- a/src/config.rs +++ b/src/config.rs @@ -156,14 +156,17 @@ fn decode_store(byte: u8) -> Option { /// Records the value of the `--credential-store` flag for later resolution. /// /// Called at the start of each CLI run with the parsed flag value (`None` when -/// the flag was not supplied), overwriting any previous value. -pub fn set_credential_store_flag(store: Option) { +/// the flag was not supplied), overwriting any previous value. Crate-internal: +/// only the engine publishes per-run flag state, so library consumers cannot +/// mutate this process-global latch. +pub(crate) fn set_credential_store_flag(store: Option) { CREDENTIAL_STORE_FLAG.store(encode_store(store), Ordering::Relaxed); } /// Returns the flag override recorded by [`set_credential_store_flag`], if any. +/// Crate-internal accessor for the process-global flag latch. #[must_use] -pub fn credential_store_flag() -> Option { +pub(crate) fn credential_store_flag() -> Option { decode_store(CREDENTIAL_STORE_FLAG.load(Ordering::Relaxed)) } @@ -220,18 +223,31 @@ pub fn load(app_id: &str) -> EngineConfig { pub struct ConfigFile { path: Option, doc: DocumentMut, + /// Parsed read-model for typed access, kept in sync with `doc`. Avoids + /// reparsing the whole document on every `section`/`deserialize`/`engine` + /// call; rebuilt only when the document is mutated via `set`. + read_model: toml::Table, } impl Default for ConfigFile { fn default() -> Self { - Self { - path: None, - doc: DocumentMut::new(), - } + Self::from_doc(None, DocumentMut::new()) } } impl ConfigFile { + /// Builds a `ConfigFile` from a path + document, parsing the typed + /// read-model once. The document is the source of truth; the read-model is + /// a serde-friendly view derived from it. + fn from_doc(path: Option, doc: DocumentMut) -> Self { + let read_model = parse_read_model(&doc); + Self { + path, + doc, + read_model, + } + } + /// Loads the config file for `app_id`. /// /// Best-effort: a missing file, unresolvable config directory, or malformed @@ -255,7 +271,7 @@ impl ConfigFile { } }, }; - Self { path, doc } + Self::from_doc(path, doc) } /// Returns the resolved config file path, if a config directory was @@ -285,9 +301,7 @@ impl ConfigFile { /// Returns an error when the table is present but does not deserialize into /// `T`. pub fn section(&self, name: &str) -> crate::Result> { - let table: toml::Table = toml::from_str(&self.doc.to_string()) - .map_err(|e| CliCoreError::message(format!("config parse error: {e}")))?; - match table.get(name) { + match self.read_model.get(name) { None => Ok(None), Some(value) => value .clone() @@ -305,7 +319,8 @@ impl ConfigFile { /// # Errors /// Returns an error when the document does not deserialize into `T`. pub fn deserialize(&self) -> crate::Result { - toml::from_str(&self.doc.to_string()) + toml::Value::Table(self.read_model.clone()) + .try_into() .map_err(|e| CliCoreError::message(format!("config deserialize error: {e}"))) } @@ -362,6 +377,8 @@ impl ConfigFile { })?; } table[last] = toml_edit::Item::Value(infer_toml_value(value)); + // Keep the typed read-model in sync with the mutated document. + self.read_model = parse_read_model(&self.doc); Ok(()) } @@ -380,13 +397,21 @@ impl ConfigFile { pub fn save(&self) -> crate::Result<()> { let path = self.path.as_ref().ok_or_else(|| { CliCoreError::message( - "no config path available (set XDG_CONFIG_HOME or HOME to a directory)", + "no config path available (set XDG_CONFIG_HOME, HOME, or %APPDATA% \ + to a directory)", ) })?; crate::fs::write_string_atomic(path, &self.doc.to_string()) } } +/// Builds the serde read-model from a document. A document that fails to +/// re-parse (which should not happen for one we constructed) yields an empty +/// table rather than panicking. +fn parse_read_model(doc: &DocumentMut) -> toml::Table { + toml::from_str(&doc.to_string()).unwrap_or_default() +} + /// Parses `value` as a TOML bool/integer/float when possible, else a string. fn infer_toml_value(value: &str) -> toml_edit::Value { if let Ok(b) = value.parse::() { @@ -430,7 +455,7 @@ pub fn resolve_credential_store_with( /// Resolves the effective [`CredentialStore`] for `app_id` against process state. /// -/// Reads the CLI-flag override ([`credential_store_flag`]), the +/// Reads the CLI-flag override (`credential_store_flag`), the /// `${PREFIX}_CREDENTIAL_STORE` env var via the injected `var` getter, and the /// config file ([`load`]), then applies [`resolve_credential_store_with`]. The /// `var` getter is injected so callers/tests can supply environment lookups @@ -662,10 +687,7 @@ mod tests { } fn doc_config(toml: &str) -> ConfigFile { - ConfigFile { - path: None, - doc: toml.parse().expect("valid toml"), - } + ConfigFile::from_doc(None, toml.parse().expect("valid toml")) } #[test] From ed3cf5cf76822bd7e25293fb75f6facdac854339 Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Fri, 12 Jun 2026 11:32:44 -0700 Subject: [PATCH 3/4] =?UTF-8?q?fix:=20address=20review=20findings=20?= =?UTF-8?q?=E2=80=94=20dual=20TOML=20dep,=20global=20flag=20race,=20valida?= =?UTF-8?q?tion=20gaps?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocking fixes: 1. Remove dual TOML dependency (toml 0.8 + toml_edit 0.22). The previous ConfigFile kept a `read_model: toml::Table` synced from the document via a string round-trip (toml_edit → string → toml::from_str). Two separate crates with the same purpose could version-drift and the round-trip silently yielded an empty config on failure. Replace with direct toml_edit::de::from_document and toml_edit::de::from_str; remove the read_model field and parse_read_model. Enable the "serde" feature on toml_edit = "0.22" in Cargo.toml. 2. Replace CREDENTIAL_STORE_FLAG AtomicU8 process-global with thread_local! Cell. The global AtomicU8 could leak the --credential-store flag across concurrent Cli::run calls on different OS threads; a thread_local scopes the flag per thread so concurrent runs on separate threads are safe. Also add clear_credential_store_flag() and call it from finish_run() so the flag does not persist across sequential runs on the same thread. Non-blocking fixes: 3. Document that default_storage reads thread-local state and is therefore non-deterministic when called outside a Cli::run execution context. 4. Clarify KeyringStorage::load docstring: both "no entry" and "keychain unavailable" collapse to None; callers cannot distinguish the two states. Point readers to read_three_state for the three-state variant. 5. Document infer_toml_value coercion rules in ConfigFile::set docstring and config set help text: bool → int → float (including 1e5/inf/nan) → string. 6. Expand engine-reserved key validation in ConfigFile::set from the single key "credentials.store" to the entire [credentials] table. Unknown keys in that table now return a clear error rather than silently persisting a no-op value. Add a test covering the rejection of unknown credentials.* keys. 7. Document snapshot semantics on CommandContext::config() and ModuleContext::config(): returns the startup snapshot, not updated by config set within the same process. Add a note to config set that the startup snapshot in context.config() is not updated by the write. 8. Document write_string_atomic and ConfigFile::load as performing blocking I/O, with a note to use spawn_blocking when called from async contexts. --- Cargo.lock | 13 ---- Cargo.toml | 3 +- src/auth/storage.rs | 26 +++++-- src/cli.rs | 3 + src/command.rs | 8 +- src/config.rs | 170 ++++++++++++++++++++++++++--------------- src/config_commands.rs | 18 +++-- src/fs.rs | 4 + src/module.rs | 7 +- 9 files changed, 159 insertions(+), 93 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 81378d9..734025e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -417,7 +417,6 @@ dependencies = [ "tempfile", "thiserror", "tokio", - "toml", "toml_edit 0.22.27", "tracing", "url", @@ -2168,18 +2167,6 @@ dependencies = [ "tokio", ] -[[package]] -name = "toml" -version = "0.8.23" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "dc1beb996b9d83529a9e75c17a1686767d148d70663143c7854d8b4a09ced362" -dependencies = [ - "serde", - "serde_spanned", - "toml_datetime 0.6.11", - "toml_edit 0.22.27", -] - [[package]] name = "toml_datetime" version = "0.6.11" diff --git a/Cargo.toml b/Cargo.toml index 773b097..b254329 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -34,8 +34,7 @@ schemars = { version = "1.2.1", features = ["derive"] } serde = { version = "1.0.228", features = ["derive"] } serde_json = "1.0.145" thiserror = "2.0.17" -toml = "0.8" -toml_edit = "0.22" +toml_edit = { version = "0.22", features = ["serde"] } tokio = { version = "1.48.0", features = ["fs", "io-std", "io-util", "macros", "net", "process", "rt-multi-thread", "signal", "sync", "time"] } tracing = "0.1.43" diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 3cf0fde..f2cb61d 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -205,10 +205,15 @@ mod keychain { /// System-keychain credential storage. /// - /// Reads and writes the OS keychain only. A `load` returns `None` both when - /// no entry exists and when the keychain backend is unavailable; a `save` - /// failure is a hard error. No file is ever written — use [`AutoStorage`] - /// for a file fallback or [`super::FileStorage`] to skip the keychain. + /// Reads and writes the OS keychain only. `load` returns `None` for both + /// "no entry" and "keychain backend unavailable" — callers cannot + /// distinguish the two states. `save` is a hard error when the write + /// fails. No file is ever written — use [`AutoStorage`] for a file + /// fallback or [`super::FileStorage`] to skip the keychain entirely. + /// + /// If you need the three-state distinction (entry present / reachable but + /// empty / backend unavailable), use [`KeyringStorage::read_three_state`] + /// directly (as [`AutoStorage`] does). #[derive(Clone, Copy, Debug, Default)] pub struct KeyringStorage; @@ -451,9 +456,16 @@ pub fn storage_for(mode: CredentialStore) -> Arc { /// Resolves the configured [`CredentialStore`] for `app_id` and builds the /// matching backend. /// -/// Resolution consults the CLI flag, the `${PREFIX}_CREDENTIAL_STORE` env var, -/// and the config file, defaulting to [`CredentialStore::Keyring`]; see -/// [`crate::config::resolve_credential_store`]. +/// Resolution consults (in priority order): the `--credential-store` CLI flag +/// stored in a per-thread latch by [`crate::cli::Cli::run`]; the +/// `${PREFIX}_CREDENTIAL_STORE` env var; the config file; and finally the +/// default [`CredentialStore::Keyring`]. +/// +/// **Note**: this function reads thread-local state set by `Cli::run`. Calling +/// it outside of a `Cli::run` execution (e.g. in a standalone binary without a +/// `Cli`) always sees `None` for the flag and relies solely on the env var and +/// config file. Prefer [`storage_for`] with an explicit [`CredentialStore`] +/// when you don't need the full resolution chain. #[must_use] pub fn default_storage(app_id: &str) -> Arc { let mode = crate::config::resolve_credential_store(app_id, |k| std::env::var(k).ok()); diff --git a/src/cli.rs b/src/cli.rs index d738cdf..f3202a5 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1851,6 +1851,9 @@ impl Cli { } fn finish_run(&self, output: CliRunOutput) -> CliRunOutput { + // Clear the per-thread credential-store flag so it does not leak into + // subsequent sequential runs on the same thread. + crate::config::clear_credential_store_flag(); if let Some(on_shutdown) = &self.on_shutdown { on_shutdown(); } diff --git a/src/command.rs b/src/command.rs index 334cdcf..6f6f547 100644 --- a/src/command.rs +++ b/src/command.rs @@ -107,13 +107,19 @@ pub struct CommandContext { } impl CommandContext { - /// Returns the loaded per-application config file. + /// Returns the per-application config file as loaded at startup. /// /// Read a consumer-owned section with /// [`ConfigFile::section`](crate::config::ConfigFile::section), for example /// `ctx.config().section::("deploy")?`. Engine-reserved /// settings are available via /// [`ConfigFile::engine`](crate::config::ConfigFile::engine). + /// + /// **Snapshot semantics**: this is the config loaded once when + /// [`crate::cli::Cli::new`] was called. Changes made by `config set` during the same process + /// invocation (e.g. from a previous `Cli::run`) are not reflected here; + /// restart the CLI (a new `Cli::new`) to pick them up. For a one-shot CLI + /// process this is always the current on-disk state. #[must_use] pub fn config(&self) -> &crate::config::ConfigFile { &self.middleware.config diff --git a/src/config.rs b/src/config.rs index ae593a7..16021d5 100644 --- a/src/config.rs +++ b/src/config.rs @@ -18,9 +18,9 @@ //! [`app_id_env_prefix`](crate::flags::app_id_env_prefix). See //! [`resolve_credential_store`] and the pure [`resolve_credential_store_with`]. +use std::cell::Cell; use std::path::{Path, PathBuf}; use std::str::FromStr; -use std::sync::atomic::{AtomicU8, Ordering}; use serde::de::DeserializeOwned; use serde::{Deserialize, Deserializer}; @@ -127,13 +127,20 @@ pub struct CredentialsConfig { pub store: Option, } -/// Process-wide override set from the `--credential-store` global flag. -/// -/// Sits at the top of the resolution precedence. Encoded as a byte so it can be -/// updated lock-free: `0` = unset, `1` = `Auto`, `2` = `Keyring`, `3` = `File`. -/// Overwritable (not set-once) so each [`crate::cli::Cli::run`] re-establishes -/// the flag for that run rather than the first run pinning it for the process. -static CREDENTIAL_STORE_FLAG: AtomicU8 = AtomicU8::new(0); +// Per-thread override from the `--credential-store` global flag. +// +// Stored in a `thread_local!` `Cell` so concurrent `Cli::run` calls on +// different OS threads cannot interfere with each other. Each thread writes +// its own flag at the start of `Cli::run` (via `set_credential_store_flag`) +// and clears it at the end (via `clear_credential_store_flag`). +// +// Limitation: concurrent `Cli::run` calls sharing the same OS thread (e.g. +// concurrent tokio tasks on a single-threaded runtime) are not supported — +// the second call will observe the first run's flag. This scenario is atypical +// for a CLI library. +thread_local! { + static CREDENTIAL_STORE_FLAG: Cell = const { Cell::new(0) }; +} fn encode_store(store: Option) -> u8 { match store { @@ -153,21 +160,28 @@ fn decode_store(byte: u8) -> Option { } } -/// Records the value of the `--credential-store` flag for later resolution. +/// Records the value of the `--credential-store` flag for the current thread. /// /// Called at the start of each CLI run with the parsed flag value (`None` when -/// the flag was not supplied), overwriting any previous value. Crate-internal: -/// only the engine publishes per-run flag state, so library consumers cannot -/// mutate this process-global latch. +/// the flag was not supplied). Crate-internal: only the engine publishes +/// per-run flag state, so library consumers cannot mutate this latch. pub(crate) fn set_credential_store_flag(store: Option) { - CREDENTIAL_STORE_FLAG.store(encode_store(store), Ordering::Relaxed); + CREDENTIAL_STORE_FLAG.with(|f| f.set(encode_store(store))); +} + +/// Clears the thread-local flag set by [`set_credential_store_flag`]. +/// +/// Called at the end of each `Cli::run` so the flag does not leak into +/// subsequent sequential runs on the same thread. +pub(crate) fn clear_credential_store_flag() { + CREDENTIAL_STORE_FLAG.with(|f| f.set(0)); } /// Returns the flag override recorded by [`set_credential_store_flag`], if any. -/// Crate-internal accessor for the process-global flag latch. +/// Crate-internal accessor for the per-thread flag latch. #[must_use] pub(crate) fn credential_store_flag() -> Option { - decode_store(CREDENTIAL_STORE_FLAG.load(Ordering::Relaxed)) + CREDENTIAL_STORE_FLAG.with(|f| decode_store(f.get())) } /// Derives the credential-store override env var from an app id, e.g. @@ -223,10 +237,6 @@ pub fn load(app_id: &str) -> EngineConfig { pub struct ConfigFile { path: Option, doc: DocumentMut, - /// Parsed read-model for typed access, kept in sync with `doc`. Avoids - /// reparsing the whole document on every `section`/`deserialize`/`engine` - /// call; rebuilt only when the document is mutated via `set`. - read_model: toml::Table, } impl Default for ConfigFile { @@ -236,16 +246,8 @@ impl Default for ConfigFile { } impl ConfigFile { - /// Builds a `ConfigFile` from a path + document, parsing the typed - /// read-model once. The document is the source of truth; the read-model is - /// a serde-friendly view derived from it. fn from_doc(path: Option, doc: DocumentMut) -> Self { - let read_model = parse_read_model(&doc); - Self { - path, - doc, - read_model, - } + Self { path, doc } } /// Loads the config file for `app_id`. @@ -254,6 +256,11 @@ impl ConfigFile { /// TOML yields an empty document (a warning is logged for the malformed /// case). The resolved path is retained for [`save`](ConfigFile::save) even /// when the file does not yet exist. + /// + /// **Blocking**: this function performs synchronous filesystem I/O. The + /// engine calls it once at `Cli::new` time (outside of command execution), + /// which is acceptable for a one-shot CLI. Avoid calling it from hot paths + /// or from within an async executor without `spawn_blocking`. #[must_use] pub fn load(app_id: &str) -> Self { let path = config_file_path(app_id); @@ -301,14 +308,21 @@ impl ConfigFile { /// Returns an error when the table is present but does not deserialize into /// `T`. pub fn section(&self, name: &str) -> crate::Result> { - match self.read_model.get(name) { - None => Ok(None), - Some(value) => value - .clone() - .try_into() - .map(Some) - .map_err(|e| CliCoreError::message(format!("config section {name:?}: {e}"))), + let item = match self.doc.get(name) { + None => return Ok(None), + Some(item) => item, + }; + // Extract the section's key-value pairs into a temporary root-level + // document so `from_document` sees a plain `T`-shaped struct. + let mut tmp = DocumentMut::new(); + if let Some(tbl) = item.as_table_like() { + for (k, v) in tbl.iter() { + tmp[k] = v.clone(); + } } + toml_edit::de::from_document::(tmp) + .map(Some) + .map_err(|e| CliCoreError::message(format!("config section {name:?}: {e}"))) } /// Deserializes the entire config file into a consumer root type `T`. @@ -319,8 +333,7 @@ impl ConfigFile { /// # Errors /// Returns an error when the document does not deserialize into `T`. pub fn deserialize(&self) -> crate::Result { - toml::Value::Table(self.read_model.clone()) - .try_into() + toml_edit::de::from_document::(self.doc.clone()) .map_err(|e| CliCoreError::message(format!("config deserialize error: {e}"))) } @@ -343,20 +356,47 @@ impl ConfigFile { /// Sets the value at a dotted key, creating intermediate tables as needed. /// - /// `value` is parsed as a TOML scalar when it looks like a bool/integer/float - /// and stored as a string otherwise. The engine-reserved `credentials.store` - /// key is validated against [`CredentialStore`] and rejected when invalid. - /// Existing comments and formatting elsewhere in the file are preserved. - /// Call [`save`](ConfigFile::save) to persist. + /// `value` is coerced to a TOML scalar type using these rules (in order): + /// 1. `"true"` / `"false"` (case-sensitive) → TOML boolean. + /// 2. Any string parseable as an `i64` → TOML integer. + /// 3. Any string parseable as an `f64` (including `"1e5"`, `"inf"`, + /// `"nan"`) → TOML float. + /// 4. Everything else → TOML string. + /// + /// To force a value to be stored as a string when it looks numeric (e.g. + /// a version like `"1.0"`), this API does not currently support quoting — + /// wrap the value in the config file by hand. + /// + /// The engine-reserved `[credentials]` table is validated: only the known + /// key `credentials.store` is accepted; unknown keys in that table are + /// rejected. Existing comments and formatting elsewhere in the file are + /// preserved. Call [`save`](ConfigFile::save) to persist. /// /// # Errors - /// Returns an error for an empty/invalid key, an invalid engine value, or a - /// key whose parent path is not a table. + /// Returns an error for an empty/invalid key, an unknown engine-reserved + /// key, an invalid engine value, or a key whose parent path is not a + /// table. pub fn set(&mut self, dotted_key: &str, value: &str) -> crate::Result<()> { - if dotted_key == "credentials.store" { - value - .parse::() - .map_err(|e| CliCoreError::message(e.to_string()))?; + // Validate engine-reserved keys under [credentials]. + // Only the documented key `credentials.store` is accepted; any other + // key in that table is rejected to prevent silently writing unknown + // engine config that would be ignored (and confuse the user). + const ENGINE_RESERVED_TABLES: &[&str] = &["credentials"]; + let first_segment = dotted_key.split('.').next().unwrap_or(""); + if ENGINE_RESERVED_TABLES.contains(&first_segment) { + match dotted_key { + "credentials.store" => { + value + .parse::() + .map_err(|e| CliCoreError::message(e.to_string()))?; + } + other => { + return Err(CliCoreError::message(format!( + "unknown engine-reserved key {other:?}; \ + the only supported key in [credentials] is \"credentials.store\"" + ))); + } + } } let segments: Vec<&str> = dotted_key.split('.').collect(); if segments.iter().any(|s| s.is_empty()) { @@ -377,8 +417,6 @@ impl ConfigFile { })?; } table[last] = toml_edit::Item::Value(infer_toml_value(value)); - // Keep the typed read-model in sync with the mutated document. - self.read_model = parse_read_model(&self.doc); Ok(()) } @@ -405,13 +443,6 @@ impl ConfigFile { } } -/// Builds the serde read-model from a document. A document that fails to -/// re-parse (which should not happen for one we constructed) yields an empty -/// table rather than panicking. -fn parse_read_model(doc: &DocumentMut) -> toml::Table { - toml::from_str(&doc.to_string()).unwrap_or_default() -} - /// Parses `value` as a TOML bool/integer/float when possible, else a string. fn infer_toml_value(value: &str) -> toml_edit::Value { if let Ok(b) = value.parse::() { @@ -582,20 +613,20 @@ mod tests { #[test] fn deserializes_store_from_toml() { let config: EngineConfig = - toml::from_str("[credentials]\nstore = \"file\"\n").expect("valid toml"); + toml_edit::de::from_str("[credentials]\nstore = \"file\"\n").expect("valid toml"); assert_eq!(config.credentials.store, Some(CredentialStore::File)); } #[test] fn deserialize_rejects_bad_store_value() { - let result = toml::from_str::("[credentials]\nstore = \"nope\"\n"); + let result = toml_edit::de::from_str::("[credentials]\nstore = \"nope\"\n"); assert!(result.is_err(), "bad store value should fail to parse"); } #[test] fn unknown_keys_are_ignored() { let config: EngineConfig = - toml::from_str("future_section = true\n[credentials]\nstore = \"auto\"\n") + toml_edit::de::from_str("future_section = true\n[credentials]\nstore = \"auto\"\n") .expect("unknown keys tolerated"); assert_eq!(config.credentials.store, Some(CredentialStore::Auto)); } @@ -750,6 +781,25 @@ mod tests { assert_eq!(cfg.engine().credentials.store, Some(CredentialStore::File)); } + #[test] + fn set_rejects_unknown_engine_reserved_keys() { + let mut cfg = ConfigFile::default(); + // Unknown keys in [credentials] are rejected to prevent silent no-ops. + assert!( + cfg.set("credentials.unknown_future_key", "foo").is_err(), + "unknown credentials key should be rejected" + ); + assert!( + cfg.set("credentials.timeout", "30").is_err(), + "unknown credentials.timeout should be rejected" + ); + // Consumer-owned tables are unrestricted. + assert!( + cfg.set("deploy.region", "us-west").is_ok(), + "consumer-owned keys should be accepted" + ); + } + #[test] fn set_rejects_empty_key_segments() { let mut cfg = ConfigFile::default(); diff --git a/src/config_commands.rs b/src/config_commands.rs index 18080a0..25bfe1d 100644 --- a/src/config_commands.rs +++ b/src/config_commands.rs @@ -61,17 +61,19 @@ pub fn config_command_group() -> RuntimeGroupSpec { .required(true) .help("Dotted key, e.g. credentials.store or deploy.region"), ) - .with_arg( - Arg::new("value") - .value_name("VALUE") - .required(true) - .help("Value (parsed as bool/int/float when possible, else string)"), - ), + .with_arg(Arg::new("value").value_name("VALUE").required(true).help( + "Value to set. Type is inferred: \"true\"/\"false\" → bool, \ + digits → int, float syntax (\"1.5\", \"1e5\") → float, \ + everything else → string.", + )), async |context| { let key = string_arg(&context.args, "key"); let value = string_arg(&context.args, "value"); - // Load fresh from disk (not the startup snapshot) so a concurrent - // external edit is not clobbered, then set + save. + // Load fresh from disk (not the startup snapshot) to avoid + // clobbering a concurrent external edit. The startup snapshot in + // `context.config()` is NOT updated by this write; a subsequent + // `config get` in the same process will still read the old value + // until the CLI is restarted. let mut config = ConfigFile::load(&context.middleware.app_id); config.set(&key, &value)?; config.save()?; diff --git a/src/fs.rs b/src/fs.rs index 30dd69c..0d3dfb0 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -101,6 +101,10 @@ pub fn is_safe_path_component(s: &str) -> bool { /// parent directory is best-effort restricted to `0700`. On Windows the rename /// replaces an existing destination but is not crash-atomic. /// +/// **Blocking**: this function uses synchronous filesystem I/O. Call it from +/// within [`tokio::task::spawn_blocking`] when used in an async context to +/// avoid stalling the executor. +/// /// # Errors /// Returns an error when the directory cannot be created or the write/rename /// fails. diff --git a/src/module.rs b/src/module.rs index 98a851b..2a383b0 100644 --- a/src/module.rs +++ b/src/module.rs @@ -155,10 +155,13 @@ impl<'middleware> ModuleContext<'middleware> { self.middleware } - /// Returns the loaded per-application config file for registration-time use. + /// Returns the per-application config file as loaded at startup. /// /// Read a consumer-owned section with - /// [`ConfigFile::section`](crate::config::ConfigFile::section). + /// [`ConfigFile::section`](crate::config::ConfigFile::section). This is + /// the same startup snapshot surfaced via + /// [`CommandContext::config`](crate::command::CommandContext::config); see + /// its documentation for snapshot-semantics caveats. pub fn config(&self) -> &crate::config::ConfigFile { &self.middleware.config } From 20bda9497a17a6716834aedd253c29d1879fe4c8 Mon Sep 17 00:00:00 2001 From: Jay Gowdy Date: Fri, 12 Jun 2026 11:42:25 -0700 Subject: [PATCH 4/4] fix(docs): remove private intra-doc link from KeyringStorage The read_three_state method is pub(super) so rustdoc rejects the link with -D warnings when built with --features pkce-auth. --- src/auth/storage.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/auth/storage.rs b/src/auth/storage.rs index f2cb61d..c918d9b 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -212,8 +212,8 @@ mod keychain { /// fallback or [`super::FileStorage`] to skip the keychain entirely. /// /// If you need the three-state distinction (entry present / reachable but - /// empty / backend unavailable), use [`KeyringStorage::read_three_state`] - /// directly (as [`AutoStorage`] does). + /// empty / backend unavailable), use the crate-internal `read_three_state` + /// helper (as [`AutoStorage`] does internally). #[derive(Clone, Copy, Debug, Default)] pub struct KeyringStorage;