From 38471080bfbda504c37807806706da7fea47e9ed Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:46:19 +0100 Subject: [PATCH 1/6] Report token store read failures instead of hiding them list_profiles() turned any index read error or corrupt JSON into an empty list. After a denied macOS keychain prompt, the next login rewrote the index to hold only the new profile, orphaning the rest, and `auth logout --all` reported "Logged out from 0 profile(s)" as a success. resolve_token likewise mapped every get_token error to "Not authenticated" (exit 3), sending a user with a locked keychain to log in again. get_token now returns Ok(None) for an absent token (and when the keyring is disabled); only that is "Not authenticated". Any other store error propagates as KEYRING_ERROR (exit 10) with its cause, and keychain read errors add a hint to unlock it or set TEAMS_CLI_TOKEN_STORE=file. A Windows token with a missing chunk is now exit 10 too. list_profiles returns Result: an absent index is empty, an unreadable or corrupt one is an error, and add/remove never write after a failed read. `auth list` and `logout --all` surface it. No on-disk or keychain format change. Tested with unit tests on in-memory and always-failing stores (corrupt or unreadable index is never rewritten) and file-store CLI tests: corrupt or unreadable index makes `auth list` / `logout` exit 10 with the index unchanged; absent token exits 3; unreadable or unparseable token exits 10 with the cause. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 5 + docs/troubleshooting.md | 3 + src/auth/keyring.rs | 230 +++++++++++++++++++++++++++++++--------- src/auth/mod.rs | 11 +- src/cli/auth.rs | 9 +- tests/cli.rs | 116 +++++++++++++++++++- 6 files changed, 315 insertions(+), 59 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 58c2b46..c66617d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +### Fixed + +- A token store that cannot be read (a locked keychain, a denied prompt, an SSH session) is reported as `KEYRING_ERROR` (exit code 10) with its cause, instead of "Not authenticated" (exit code 3). Only a profile with no stored token is "Not authenticated". +- A profile index that cannot be read or parsed is an error (exit code 10) instead of an empty list. `auth login` no longer rewrites it to hold only the new profile, orphaning the rest; `auth list` and `auth logout --all` no longer report no profiles as a success. + ## v0.9.0 - 2026-10-04 ### Added diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 846ce2a..bcb51d6 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -140,6 +140,9 @@ Actions: - Expect one prompt per profile after upgrading or rebuilding; approve it and the grant holds until the binary changes again. +- A denied prompt or a locked keychain (common over SSH) fails with + `KEYRING_ERROR` (exit code 10), not "Not authenticated". Unlock the + keychain (`security unlock-keychain`) or use `TEAMS_CLI_TOKEN_STORE=file`. - An unattended process (a daemon, a scheduled job) cannot answer the prompt and waits on it. Set `TEAMS_CLI_TOKEN_STORE=file` for that process and sign in once with it set; tokens then live in `0600` files under the config diff --git a/src/auth/keyring.rs b/src/auth/keyring.rs index de20026..c9ef739 100644 --- a/src/auth/keyring.rs +++ b/src/auth/keyring.rs @@ -79,9 +79,8 @@ fn parse_store_kind(value: Option<&str>) -> Result { } } -/// Refuse a mistyped `TEAMS_CLI_TOKEN_STORE` before any command runs. Token -/// reads report every store error as "not signed in", so without this a typo -/// would send the caller to log in again instead of naming the variable. +/// Refuse a mistyped `TEAMS_CLI_TOKEN_STORE` before any command runs, naming +/// the variable rather than failing on the first token read. pub fn check_token_store() -> Result<()> { store_kind().map(|_| ()) } @@ -133,9 +132,21 @@ fn absent_as_none(result: ::keyring::Result, what: &str) -> Result(result: ::keyring::Result) -> Result> { + absent_as_none(result, "retrieve token").map_err(|e| match e { + TeamsError::KeyringError(message) => TeamsError::KeyringError(format!( + "{message}. If the keychain is locked or this is an SSH session, unlock it \ + or set {TOKEN_STORE_ENV}=file" + )), + other => other, + }) +} + impl SecretStore for OsKeyring { fn get_password(&self, key: &str) -> Result> { - absent_as_none(Self::entry(key)?.get_password(), "retrieve token") + read_keyring(Self::entry(key)?.get_password()) } fn set_password(&self, key: &str, value: &str) -> Result<()> { @@ -148,7 +159,7 @@ impl SecretStore for OsKeyring { } fn get_secret(&self, key: &str) -> Result>> { - absent_as_none(Self::entry(key)?.get_secret(), "retrieve token") + read_keyring(Self::entry(key)?.get_secret()) } fn set_secret(&self, key: &str, value: &[u8]) -> Result<()> { @@ -241,8 +252,8 @@ fn sync_dir(dir: &Path) { } /// The store writes every file `0600`, so a token readable by the group or by -/// others was copied or loosened by hand. Say so on stderr rather than refuse: -/// a refused read would surface as "not signed in", which hides the cause. +/// others was copied or loosened by hand. Say so on stderr rather than refuse, +/// which would lock the owner out of their own token. #[cfg(unix)] fn warn_if_exposed(path: &Path) { use std::os::unix::fs::PermissionsExt; @@ -322,9 +333,11 @@ pub fn store_token(profile: &str, token: &TokenInfo) -> Result<()> { store_token_in(&*store()?, profile, token, chunk_bytes_for(store_kind()?)) } -pub fn get_token(profile: &str) -> Result { +/// The stored token for `profile`, or `Ok(None)` when none is stored. A +/// disabled keyring stores nothing, so it reads as absent. +pub fn get_token(profile: &str) -> Result> { if disabled() { - return Err(TeamsError::KeyringError("Keyring is disabled".into())); + return Ok(None); } get_token_from(&*store()?, profile) } @@ -386,17 +399,16 @@ fn stored_chunk_count(store: &dyn SecretStore, profile: &str) -> Result { .map_or(0, |header| header.chunks)) } -fn get_token_from(store: &dyn SecretStore, profile: &str) -> Result { - let primary = store.get_password(&entry_key(profile))?.ok_or_else(|| { - TeamsError::KeyringError(format!( - "Failed to retrieve token: no stored token for profile `{profile}`" - )) - })?; +fn get_token_from(store: &dyn SecretStore, profile: &str) -> Result> { + let Some(primary) = store.get_password(&entry_key(profile))? else { + return Ok(None); + }; let json = match serde_json::from_str::(&primary) { Ok(header) => read_chunks(store, profile, header.chunks)?, Err(_) => primary, }; serde_json::from_str(&json) + .map(Some) .map_err(|e| TeamsError::KeyringError(format!("Failed to parse stored token: {e}"))) } @@ -449,46 +461,64 @@ fn delete_chunks( } } -pub fn list_profiles() -> Vec { - if disabled() { - return vec![]; - } +const PROFILE_INDEX_KEY: &str = "profile-index"; - // Keyring doesn't support enumeration natively. - // We maintain a separate index entry. - match store().and_then(|s| s.get_password("profile-index")) { - Ok(Some(json)) => serde_json::from_str(&json).unwrap_or_default(), - _ => vec![], +/// The indexed profiles; empty when there is no index. An index that cannot be +/// read or parsed is an error, never an empty list, so no caller rewrites it +/// with the other profiles dropped. +pub fn list_profiles() -> Result> { + if disabled() { + return Ok(vec![]); } + list_profiles_in(&*store()?) } pub fn add_profile_to_index(profile: &str) -> Result<()> { if disabled() { return Ok(()); } - - let mut profiles = list_profiles(); - if !profiles.contains(&profile.to_string()) { - profiles.push(profile.to_string()); - } - write_profile_index(&profiles) + add_profile_to_index_in(&*store()?, profile) } pub fn remove_profile_from_index(profile: &str) -> Result<()> { if disabled() { return Ok(()); } + remove_profile_from_index_in(&*store()?, profile) +} + +// Keyring doesn't support enumeration natively. +// We maintain a separate index entry. +fn list_profiles_in(store: &dyn SecretStore) -> Result> { + let json = store + .get_password(PROFILE_INDEX_KEY) + .map_err(|e| TeamsError::KeyringError(format!("Failed to read profile index: {e}")))?; + match json { + None => Ok(vec![]), + Some(json) => serde_json::from_str(&json) + .map_err(|e| TeamsError::KeyringError(format!("Stored profile index is corrupt: {e}"))), + } +} + +fn add_profile_to_index_in(store: &dyn SecretStore, profile: &str) -> Result<()> { + let mut profiles = list_profiles_in(store)?; + if !profiles.contains(&profile.to_string()) { + profiles.push(profile.to_string()); + } + write_profile_index(store, &profiles) +} - let mut profiles = list_profiles(); +fn remove_profile_from_index_in(store: &dyn SecretStore, profile: &str) -> Result<()> { + let mut profiles = list_profiles_in(store)?; profiles.retain(|p| p != profile); - write_profile_index(&profiles) + write_profile_index(store, &profiles) } -fn write_profile_index(profiles: &[String]) -> Result<()> { +fn write_profile_index(store: &dyn SecretStore, profiles: &[String]) -> Result<()> { let json = serde_json::to_string(profiles) .map_err(|e| TeamsError::KeyringError(format!("Failed to serialize index: {e}")))?; - store()? - .set_password("profile-index", &json) + store + .set_password(PROFILE_INDEX_KEY, &json) .map_err(|e| TeamsError::KeyringError(format!("Failed to store index: {e}"))) } @@ -585,12 +615,14 @@ mod tests { single.is_err(), "Credential Manager accepted an oversized blob; the premise of chunking is gone" ); - let loaded = loaded.expect("chunked store and load"); + let loaded = loaded + .expect("chunked store and load") + .expect("token stored"); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); assert_eq!(loaded.scope, original.scope); swept.expect("logout sweep"); - assert!(after.is_err(), "logout left the token readable"); + assert!(matches!(after, Ok(None)), "logout left the token readable"); } fn token(access_token: &str) -> TokenInfo { @@ -635,7 +667,7 @@ mod tests { 0o600 ); } - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); @@ -646,7 +678,7 @@ mod tests { ); delete_token_from(&store, "work", false).unwrap(); - assert!(get_token_from(&store, "work").is_err()); + assert!(get_token_from(&store, "work").unwrap().is_none()); assert!( !store.delete("work:token").unwrap(), "a second delete finds nothing" @@ -693,7 +725,7 @@ mod tests { store_token_in(&store, "work", &original, None).unwrap(); assert_eq!(store.keys(), vec!["work:token"]); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); } @@ -718,7 +750,7 @@ mod tests { format!("{{\"chunks\":{expected_chunks}}}") ); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.access_token, original.access_token); assert_eq!(loaded.refresh_token, original.refresh_token); assert_eq!(loaded.scope, original.scope); @@ -737,7 +769,13 @@ mod tests { "{\"chunks\":1}" ); assert_eq!(store.keys(), vec!["work:token", "work:token:0"]); - assert_eq!(get_token_from(&store, "work").unwrap().access_token, "x"); + assert_eq!( + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, + "x" + ); } #[test] @@ -753,7 +791,10 @@ mod tests { let expected_chunks = serialized_len(&token("short")).div_ceil(64); assert_eq!(after.len(), expected_chunks + 1); assert_eq!( - get_token_from(&store, "work").unwrap().access_token, + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, "short" ); } @@ -767,7 +808,10 @@ mod tests { .unwrap(); assert_eq!( - get_token_from(&store, "work").unwrap().access_token, + get_token_from(&store, "work") + .unwrap() + .unwrap() + .access_token, "legacy" ); } @@ -785,9 +829,96 @@ mod tests { } #[test] - fn missing_primary_entry_is_an_error() { + fn missing_primary_entry_reads_as_absent() { + let store = MemoryStore::default(); + assert!(get_token_from(&store, "work").unwrap().is_none()); + } + + /// Fails every read, as a locked keychain or a denied prompt does, and + /// records every write. + #[derive(Default)] + struct UnreadableStore { + writes: Mutex>, + } + + impl SecretStore for UnreadableStore { + fn get_password(&self, _key: &str) -> Result> { + Err(TeamsError::KeyringError( + "Failed to retrieve token: denied".into(), + )) + } + + fn set_password(&self, key: &str, _value: &str) -> Result<()> { + self.writes.lock().unwrap().push(key.to_string()); + Ok(()) + } + + fn get_secret(&self, key: &str) -> Result>> { + self.get_password(key).map(|v| v.map(String::into_bytes)) + } + + fn set_secret(&self, key: &str, value: &[u8]) -> Result<()> { + self.set_password(key, std::str::from_utf8(value).unwrap()) + } + + fn delete(&self, _key: &str) -> Result { + Ok(false) + } + } + + #[test] + fn unreadable_store_is_an_error_not_an_absent_token() { + let err = get_token_from(&UnreadableStore::default(), "work").unwrap_err(); + assert!(err.to_string().contains("denied"), "{err}"); + } + + #[test] + fn absent_index_lists_no_profiles_and_is_created_on_add() { + let store = MemoryStore::default(); + assert!(list_profiles_in(&store).unwrap().is_empty()); + + add_profile_to_index_in(&store, "work").unwrap(); + add_profile_to_index_in(&store, "work").unwrap(); + add_profile_to_index_in(&store, "home").unwrap(); + assert_eq!(list_profiles_in(&store).unwrap(), ["work", "home"]); + + remove_profile_from_index_in(&store, "work").unwrap(); + assert_eq!(list_profiles_in(&store).unwrap(), ["home"]); + } + + /// The failure that orphaned profiles: a corrupt index read as empty, so + /// the next login rewrote it to hold only the profile just signed in. + #[test] + fn corrupt_index_is_an_error_and_never_rewritten() { let store = MemoryStore::default(); - assert!(get_token_from(&store, "work").is_err()); + store.set_password(PROFILE_INDEX_KEY, "[\"work\",").unwrap(); + + let err = list_profiles_in(&store).unwrap_err(); + assert!( + err.to_string().contains("profile index is corrupt"), + "{err}" + ); + assert_eq!(err.exit_code(), 10); + assert!(add_profile_to_index_in(&store, "home").is_err()); + assert!(remove_profile_from_index_in(&store, "work").is_err()); + + assert_eq!( + store.get_password(PROFILE_INDEX_KEY).unwrap().as_deref(), + Some("[\"work\",") + ); + } + + #[test] + fn unreadable_index_is_an_error_and_never_rewritten() { + let store = UnreadableStore::default(); + + let err = list_profiles_in(&store).unwrap_err(); + assert!(err.to_string().contains("profile index"), "{err}"); + assert!(err.to_string().contains("denied"), "{err}"); + assert!(add_profile_to_index_in(&store, "home").is_err()); + assert!(remove_profile_from_index_in(&store, "work").is_err()); + + assert!(store.writes.lock().unwrap().is_empty()); } #[test] @@ -803,7 +934,10 @@ mod tests { .iter() .all(|key| key.starts_with("other:token"))); assert_eq!( - get_token_from(&store, "other").unwrap().access_token, + get_token_from(&store, "other") + .unwrap() + .unwrap() + .access_token, "keep" ); } @@ -878,7 +1012,7 @@ mod tests { for size in 1..=5 { store_token_in(&store, "work", &original, Some(size)).unwrap(); - let loaded = get_token_from(&store, "work").unwrap(); + let loaded = get_token_from(&store, "work").unwrap().unwrap(); assert_eq!(loaded.scope, original.scope, "chunk size {size}"); assert_eq!(loaded.profile, original.profile, "chunk size {size}"); } diff --git a/src/auth/mod.rs b/src/auth/mod.rs index e46e702..a3355f2 100644 --- a/src/auth/mod.rs +++ b/src/auth/mod.rs @@ -19,7 +19,8 @@ const EXPIRY_SKEW_SECS: i64 = 60; /// 1. TEAMS_CLI_ACCESS_TOKEN env var (used verbatim, never refreshed) /// 2. Keyring: if the stored token is (near) expired, silently redeem its /// refresh token; otherwise return it as-is -/// 3. Error +/// 3. Error: not authenticated when no token is stored; the store's own +/// error when it cannot be read pub async fn resolve_token(profile: &str) -> Result { // 1. Direct env var override if let Ok(token) = std::env::var("TEAMS_CLI_ACCESS_TOKEN") { @@ -34,8 +35,8 @@ pub async fn resolve_token(profile: &str) -> Result { } // 2. Keyring - match keyring::get_token(profile) { - Ok(info) => { + match keyring::get_token(profile)? { + Some(info) => { if !needs_refresh(&info) { return Ok(info); } @@ -48,7 +49,7 @@ pub async fn resolve_token(profile: &str) -> Result { Err(e) => handle_refresh_failure(&info, e), } } - Err(_) => Err(not_authenticated()), + None => Err(not_authenticated()), } } @@ -122,7 +123,7 @@ pub async fn refresh_token_with_scopes( profile: &str, scope: Option<&str>, ) -> Result<(TokenInfo, String)> { - let info = keyring::get_token(profile).map_err(|_| not_authenticated())?; + let info = keyring::get_token(profile)?.ok_or_else(not_authenticated)?; if info.refresh_token.is_none() { return Err(TeamsError::AuthError( diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 93999a0..358f57d 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -511,9 +511,12 @@ pub async fn run( AuthCommand::List => { let start = Instant::now(); - let profiles: Vec = auth::keyring::list_profiles() + let profiles: Vec = auth::keyring::list_profiles()? .iter() - .map(|name| summarize_profile(name, auth::keyring::get_token(name).ok().as_ref())) + .map(|name| { + let token = auth::keyring::get_token(name).ok().flatten(); + summarize_profile(name, token.as_ref()) + }) .collect(); let msg = serde_json::json!({ "profiles": profiles, @@ -556,7 +559,7 @@ pub async fn run( } => { let start = Instant::now(); if all { - let profiles = auth::keyring::list_profiles(); + let profiles = auth::keyring::list_profiles()?; for p in &profiles { auth::keyring::delete_token(p)?; auth::keyring::remove_profile_from_index(p)?; diff --git a/tests/cli.rs b/tests/cli.rs index a204250..a463e79 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1328,9 +1328,7 @@ fn message_update_requires_channel_alongside_team() { } /// A mistyped `TEAMS_CLI_TOKEN_STORE` is refused before any command runs, in -/// the error envelope with exit code 2. Token reads treat every store error as -/// "not signed in", so without the check a typo would read as exit code 3 and -/// send the caller to log in again. +/// the error envelope with exit code 2, naming the variable. #[test] fn unknown_token_store_is_invalid_input_not_an_auth_error() { for args in [["chat", "list"], ["auth", "list"]] { @@ -1493,3 +1491,115 @@ fn a_failed_login_does_not_save_the_given_ids() { .stdout(predicate::str::contains("Client secret is required")); assert!(!config_path.exists()); } + +/// A `teams` command factory using the file token store under `home`, and +/// that store's `tokens/` directory, created. Not on Windows, whose config +/// directory ignores `HOME`. +#[cfg(not(windows))] +fn file_store_in(home: &std::path::Path) -> (impl Fn() -> Command + '_, std::path::PathBuf) { + let output = teams() + .env("HOME", home) + .env("XDG_CONFIG_HOME", home) + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let config_path = std::path::PathBuf::from(data["data"]["path"].as_str().unwrap()); + assert!(config_path.starts_with(home)); + let tokens = config_path.parent().unwrap().join("tokens"); + fs::create_dir_all(&tokens).unwrap(); + let cmd = move || { + let mut cmd = teams(); + cmd.env_remove("TEAMS_CLI_DISABLE_KEYRING") + .env("TEAMS_CLI_TOKEN_STORE", "file") + .env("HOME", home) + .env("XDG_CONFIG_HOME", home); + cmd + }; + (cmd, tokens) +} + +/// A corrupt profile index is an error (exit code 10), never an empty list: +/// `auth list` must not report no profiles, `logout --all` must not report +/// "0 profile(s)" as a success, and none of them may rewrite the index. +#[cfg(not(windows))] +#[test] +fn corrupt_profile_index_is_a_storage_error_and_left_untouched() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + let index = tokens.join("profile-index"); + fs::write(&index, "not json").unwrap(); + + for args in [ + &["auth", "list"][..], + &["auth", "logout", "--all"], + &["auth", "logout", "--profile", "work"], + ] { + file_store() + .args(args) + .args(["--output", "json"]) + .assert() + .code(10) + .stdout( + predicate::str::contains("KEYRING_ERROR") + .and(predicate::str::contains("profile index")), + ); + assert_eq!(fs::read_to_string(&index).unwrap(), "not json", "{args:?}"); + } +} + +/// An index that exists but cannot be read is an error too, not "no profiles". +#[cfg(not(windows))] +#[test] +fn unreadable_profile_index_is_a_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + // a directory where the index file belongs: reading it fails, but not as + // "not found" + fs::create_dir(tokens.join("profile-index")).unwrap(); + + for args in [&["auth", "list"][..], &["auth", "logout", "--all"]] { + file_store() + .args(args) + .args(["--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("KEYRING_ERROR")); + } +} + +/// Only a token that is not stored at all means "not authenticated" (exit +/// code 3). A token the store holds but cannot read is a storage error (exit +/// code 10) naming the cause, so the caller is not sent to log in again. +#[cfg(not(windows))] +#[test] +fn absent_token_is_auth_error_but_unreadable_token_is_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + let token = tokens.join("work.token"); + + file_store() + .args(["--profile", "work", "user", "me", "--output", "json"]) + .assert() + .code(3) + .stdout(predicate::str::contains("Not authenticated.")); + + fs::create_dir(&token).unwrap(); + file_store() + .args(["--profile", "work", "user", "me", "--output", "json"]) + .assert() + .code(10) + .stdout( + predicate::str::contains("KEYRING_ERROR") + .and(predicate::str::contains("Failed to retrieve token")) + .and(predicate::str::contains("Not authenticated").not()), + ); + + fs::remove_dir(&token).unwrap(); + fs::write(&token, "not json").unwrap(); + file_store() + .args(["--profile", "work", "auth", "refresh", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("Failed to parse stored token")); +} From ba2454eb2dd8a8a1de1c1b39e073ecfcbc4444a9 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:46:47 +0100 Subject: [PATCH 2/6] Honour --config in auth switch and config path; save atomically `auth switch` saved with `save_config(.., None)`, so with `--config customer.toml` it wrote customer.toml's contents plus the new default profile over the default config file and left customer.toml unchanged. A failed save was only a tracing warning beside a success. `config path` always reported the default path, ignoring `--config`. `save_config` truncated the file and wrote it in place, so a failed write (full disk, crash) could leave a partial config. `auth switch` now saves to the `--config` path, and a failed save is a config error (exit 10). `config path` reports the `--config` file when given and adds `source`: `flag` or `default`. `save_config` goes through `write_config_text`, the temp-file-and-rename writer login already uses (0600, writes through symlinks), so `config set`, `config init` and `auth switch` keep the old file on failure. No cross-process locking. Tests: switch with `--config` updates that file and creates no default config under a temp HOME; a switch whose save fails exits 10; `config path` with and without `--config`; `save_config` into a read-only directory fails and leaves the original file intact. All four fail without the fix. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 3 ++ src/cli/auth.rs | 4 +-- src/cli/config_cmd.rs | 6 +++- src/config.rs | 46 +++++++++++++------------- tests/cli.rs | 77 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 26 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c66617d..7ce9850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,9 @@ - A token store that cannot be read (a locked keychain, a denied prompt, an SSH session) is reported as `KEYRING_ERROR` (exit code 10) with its cause, instead of "Not authenticated" (exit code 3). Only a profile with no stored token is "Not authenticated". - A profile index that cannot be read or parsed is an error (exit code 10) instead of an empty list. `auth login` no longer rewrites it to hold only the new profile, orphaning the rest; `auth list` and `auth logout --all` no longer report no profiles as a success. +- `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. +- `teams config path` ignored `--config` and always reported the default path. It now reports the file in use, with `source` set to `flag` or `default`. +- Config saves (`config set`, `config init`, `auth switch`) truncated the file and wrote it in place, so a failed write could leave a partial config. They now write a temporary file and rename it over the original, as `auth login` already did. ## v0.9.0 - 2026-10-04 diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 358f57d..11d97a8 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -541,9 +541,7 @@ pub async fn run( // Update config to set default profile let mut updated_config = config.clone(); updated_config.default.profile = Some(name.clone()); - if let Err(e) = config::save_config(&updated_config, None) { - tracing::warn!("Could not save profile switch to config: {e}"); - } + config::save_config(&updated_config, config_path)?; let msg = serde_json::json!({ "message": format!("Switched to profile '{name}'"), diff --git a/src/cli/config_cmd.rs b/src/cli/config_cmd.rs index 498230e..c6f434e 100644 --- a/src/cli/config_cmd.rs +++ b/src/cli/config_cmd.rs @@ -112,9 +112,13 @@ pub async fn run( ConfigCommand::Path => { let start = Instant::now(); - let path = config::default_config_path()?; + let (path, source) = match config_path { + Some(path) => (PathBuf::from(path), "flag"), + None => (config::default_config_path()?, "default"), + }; let msg = serde_json::json!({ "path": path.display().to_string(), + "source": source, "exists": path.exists(), "namespace": config::NAMESPACE, }); diff --git a/src/config.rs b/src/config.rs index 5b4f68b..89cebf0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -216,30 +216,9 @@ pub fn save_config(config: &ConfigFile, path: Option<&str>) -> Result<()> { Some(p) => PathBuf::from(p), None => default_config_path()?, }; - - if let Some(parent) = config_path.parent() { - fs::create_dir_all(parent) - .map_err(|e| TeamsError::ConfigError(format!("Failed to create config dir: {e}")))?; - } - let content = toml::to_string_pretty(config) .map_err(|e| TeamsError::ConfigError(format!("Failed to serialize config: {e}")))?; - - fs::write(&config_path, &content).map_err(|e| { - TeamsError::ConfigError(format!( - "Failed to write config to {}: {e}", - config_path.display() - )) - })?; - - #[cfg(unix)] - { - use std::os::unix::fs::PermissionsExt; - let perms = std::fs::Permissions::from_mode(0o600); - fs::set_permissions(&config_path, perms).ok(); - } - - Ok(()) + write_config_text(&config_path, &content) } /// Resolve the active profile: an explicit `--profile` flag or @@ -1082,6 +1061,29 @@ client_id = \"other-app\" assert_eq!(fs::read_to_string(&target).unwrap(), "a = 2\n"); } + /// `save_config` replaces the file rather than truncating it, so a write + /// that fails, here for want of a writable directory, keeps the old file. + #[cfg(unix)] + #[test] + fn save_config_failure_keeps_the_old_file() { + use std::os::unix::fs::PermissionsExt; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("config.toml"); + fs::write(&path, "[default]\nprofile = \"old\"\n").unwrap(); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o500)).unwrap(); + + let mut config = ConfigFile::default(); + config.default.profile = Some("new".into()); + let saved = save_config(&config, path.to_str()); + fs::set_permissions(dir.path(), fs::Permissions::from_mode(0o700)).unwrap(); + + assert!(matches!(saved, Err(TeamsError::ConfigError(_)))); + assert_eq!( + fs::read_to_string(&path).unwrap(), + "[default]\nprofile = \"old\"\n" + ); + } + #[test] fn id_source_follows_the_resolution_order() { assert_eq!(IdSource::of(true, true, true), IdSource::Flag); diff --git a/tests/cli.rs b/tests/cli.rs index a463e79..8e97587 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1603,3 +1603,80 @@ fn absent_token_is_auth_error_but_unreadable_token_is_storage_error() { .code(10) .stdout(predicate::str::contains("Failed to parse stored token")); } + +/// The default config path under `home`, as `config path` reports it. +#[cfg(not(windows))] +fn default_config_path_in(home: &std::path::Path) -> std::path::PathBuf { + let output = teams() + .env("HOME", home) + .env("XDG_CONFIG_HOME", home) + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["source"], "default"); + std::path::PathBuf::from(data["data"]["path"].as_str().unwrap()) +} + +/// `config path` reports the `--config` file when one is given. +#[test] +fn config_path_reports_the_config_flag() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("customer.toml"); + let result = teams() + .args(["--config", path.to_str().unwrap()]) + .args(["config", "path", "--output", "json"]) + .assert() + .success(); + let data: serde_json::Value = serde_json::from_slice(&result.get_output().stdout).unwrap(); + assert_eq!(data["data"]["path"], path.to_str().unwrap()); + assert_eq!(data["data"]["source"], "flag"); + assert_eq!(data["data"]["exists"], false); +} + +/// `auth switch --config F` records the profile in F and leaves the default +/// config alone. Not on Windows, whose config directory ignores `HOME`. +#[cfg(not(windows))] +#[test] +fn auth_switch_saves_to_the_config_flag() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let default_path = default_config_path_in(home.path()); + assert!(default_path.starts_with(home.path())); + let dir = tempfile::tempdir().unwrap(); + let path = write_customer_profile_config(&dir); + + teams() + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()) + .env("TEAMS_CLI_ACCESS_TOKEN", "not-a-jwt") + .args(["--config", path.to_str().unwrap()]) + .args(["auth", "switch", "customer", "--output", "json"]) + .assert() + .success(); + + let saved = fs::read_to_string(&path).unwrap(); + assert!(saved.contains("profile = \"customer\""), "{saved}"); + assert!(saved.contains("11111111-1111-1111-1111-111111111111")); + assert!(!default_path.exists()); +} + +/// A switch that cannot be saved fails with a config error instead of +/// reporting success. +#[test] +fn auth_switch_fails_when_the_config_cannot_be_saved() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let dir = tempfile::tempdir().unwrap(); + let not_a_dir = dir.path().join("file"); + fs::write(¬_a_dir, "").unwrap(); + let path = not_a_dir.join("config.toml"); + + teams() + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()) + .env("TEAMS_CLI_ACCESS_TOKEN", "not-a-jwt") + .args(["--config", path.to_str().unwrap()]) + .args(["auth", "switch", "customer", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("\"success\": false")); +} From 6626b11481b75888bf32838fc5745a41e2f40d65 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 16:45:31 +0100 Subject: [PATCH 3/6] Limit new profile names to a file-safe character set With TEAMS_CLI_TOKEN_STORE=file, token file names map ':', '/' and '\' to '.', so profiles `customer/a` and `customer.a` shared one token file and a command could run as the wrong account. A new profile's name must now be 1-64 of A-Z a-z 0-9 . _ - and not `.` or `..`. `auth login` (via --profile, TEAMS_CLI_PROFILE or default.profile), `config set profiles....` and `config set default.profile` refuse other names with exit code 2. A name already in the config or the token index is accepted, so existing profiles keep working. The only name the CLI derives itself, `default`, conforms. The file store's name encoding is unchanged. Tests: unit tests for the name check, the `default` fallback and the existing-profile exemption; CLI tests for each refusal path, valid names, and legacy names in the config and in the file store index. An invalid name is checked against the token index; if that index cannot be read, the storage error (exit 10) is reported rather than read as no profiles. Valid names never read the index. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 1 + docs/command-reference.md | 2 +- docs/man/teams.1 | 7 ++ src/cli/auth.rs | 1 + src/cli/config_cmd.rs | 5 ++ src/config.rs | 88 +++++++++++++++++++++++++ tests/cli.rs | 131 ++++++++++++++++++++++++++++++++++++++ 7 files changed, 234 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ce9850..a234c73 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ - `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. - `teams config path` ignored `--config` and always reported the default path. It now reports the file in use, with `source` set to `flag` or `default`. - Config saves (`config set`, `config init`, `auth switch`) truncated the file and wrote it in place, so a failed write could leave a partial config. They now write a temporary file and rename it over the original, as `auth login` already did. +- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. ## v0.9.0 - 2026-10-04 diff --git a/docs/command-reference.md b/docs/command-reference.md index f86c631..0832240 100644 --- a/docs/command-reference.md +++ b/docs/command-reference.md @@ -15,7 +15,7 @@ teams [OPTIONS] | `-v, --verbose` | Increase stderr logging. Repeat for debug/trace. | | `--no-color` | Disable ANSI colors. | | `--config ` | Use an explicit config path. | -| `--profile ` | Use a named credential profile. | +| `--profile ` | Use a named credential profile. New names: 1-64 of `A-Z a-z 0-9 . _ -`. | | `--timeout ` | Override request timeout. | | `--retry ` | Override retry attempts. | | `--page-size ` | Page size for paginated list operations. | diff --git a/docs/man/teams.1 b/docs/man/teams.1 index d100036..9799c26 100644 --- a/docs/man/teams.1 +++ b/docs/man/teams.1 @@ -39,6 +39,13 @@ then the configured default profile, then .B default. An explicit NAME always wins, even when it is the literal .B default. +A new profile's name is 1 to 64 of +.B A-Z a-z 0-9 . _ - +(not +.B . +or +.B ..); +existing profiles keep their names. .TP .B --timeout SECONDS Override Graph request timeout. diff --git a/src/cli/auth.rs b/src/cli/auth.rs index 11d97a8..f6716b0 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -336,6 +336,7 @@ async fn login( format: OutputFormat, ) -> Result<()> { let start = Instant::now(); + config::check_new_profile_name(profile, config)?; let (client_id, tenant_id) = login_registration(&args, config, profile)?; let client_secret = if args.client_credentials { Some( diff --git a/src/cli/config_cmd.rs b/src/cli/config_cmd.rs index c6f434e..5d4ea2b 100644 --- a/src/cli/config_cmd.rs +++ b/src/cli/config_cmd.rs @@ -87,6 +87,11 @@ pub async fn run( .map_err(|e| TeamsError::ConfigError(format!("Serialization error: {e}")))?; let parts: Vec<&str> = key.split('.').collect(); + match parts.as_slice() { + ["profiles", name, ..] => config::check_new_profile_name(name, config)?, + ["default", "profile"] => config::check_new_profile_name(&value, config)?, + _ => {} + } let mut current = &mut config_json; for (i, part) in parts.iter().enumerate() { if i == parts.len() - 1 { diff --git a/src/config.rs b/src/config.rs index 89cebf0..c2b85b2 100644 --- a/src/config.rs +++ b/src/config.rs @@ -232,6 +232,44 @@ pub fn resolve_profile<'a>(cli_profile: Option<&'a str>, config: &'a ConfigFile) } } +/// Longest name a new profile may have. +pub const MAX_PROFILE_NAME_LEN: usize = 64; + +/// Whether `name` may name a new profile: `A-Z a-z 0-9 . _ -`, at most +/// `MAX_PROFILE_NAME_LEN` long, not `.` or `..`. The file token store maps +/// `:`, `/` and `\` to `.`, so wider names could share a token file. +pub fn is_valid_profile_name(name: &str) -> bool { + !name.is_empty() + && name.len() <= MAX_PROFILE_NAME_LEN + && name != "." + && name != ".." + && name + .bytes() + .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-')) +} + +/// Refuse to create a profile under an invalid name. A profile that already +/// exists, in the config or the token index, keeps its name. +pub fn check_new_profile_name(name: &str, config: &ConfigFile) -> Result<()> { + check_new_profile_name_with(name, config, crate::auth::keyring::list_profiles) +} + +fn check_new_profile_name_with( + name: &str, + config: &ConfigFile, + index: impl FnOnce() -> Result>, +) -> Result<()> { + if is_valid_profile_name(name) + || config.profiles.contains_key(name) + || index()?.iter().any(|p| p == name) + { + return Ok(()); + } + Err(TeamsError::InvalidInput(format!( + "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" + ))) +} + pub fn resolve_client_id( cli_flag: Option<&str>, profile: &str, @@ -615,6 +653,56 @@ scopes = "User.Read People.Read offline_access" assert_eq!(resolve_profile(None, &config), "default"); } + #[test] + fn profile_names_are_limited_to_a_file_safe_set() { + for name in ["default", "work", "customer.a", "a_b-C9", &"x".repeat(64)] { + assert!(is_valid_profile_name(name), "{name}"); + } + for name in [ + "", + ".", + "..", + "customer/a", + "a\\b", + "a:b", + "a b", + "ñ", + &"x".repeat(65), + ] { + assert!(!is_valid_profile_name(name), "{name}"); + } + } + + /// The name the CLI falls back to must itself be a valid new name. + #[test] + fn the_default_profile_name_is_valid() { + let config = ConfigFile::default(); + assert!(is_valid_profile_name(resolve_profile(None, &config))); + } + + #[test] + fn check_new_profile_name_keeps_existing_profiles() { + let mut config = ConfigFile::default(); + let err = + check_new_profile_name_with("customer/a", &config, || Ok(Vec::new())).unwrap_err(); + assert_eq!(err.exit_code(), 2); + check_new_profile_name_with("customer/a", &config, || Ok(vec!["customer/a".into()])) + .unwrap(); + config + .profiles + .insert("customer/a".into(), ProfileConfig::default()); + check_new_profile_name_with("customer/a", &config, || Ok(Vec::new())).unwrap(); + } + + #[test] + fn check_new_profile_name_reports_an_unreadable_index() { + let config = ConfigFile::default(); + let unreadable = || Err(TeamsError::KeyringError("denied".into())); + let err = check_new_profile_name_with("other/b", &config, unreadable).unwrap_err(); + assert_eq!(err.exit_code(), 10); + check_new_profile_name_with("valid", &config, || panic!("index read")).unwrap(); + } + #[test] fn resolve_profile_explicit_default_beats_config_default() { // An explicit --profile default must address the profile named diff --git a/tests/cli.rs b/tests/cli.rs index 8e97587..f19e780 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1397,6 +1397,137 @@ fn file_token_store_lists_and_logs_out_a_profile() { ); } +/// A new profile name is limited to `A-Z a-z 0-9 . _ -`: the file token +/// store maps `/` to `.`, so `customer/a` would share `customer.a`'s token. +/// Each way a name can be created refuses it as invalid input and writes +/// nothing. +#[test] +fn an_invalid_new_profile_name_is_refused() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + let refused = |cmd: &mut Command| { + cmd.args(["--config", config, "--output", "json"]) + .assert() + .code(2) + .stdout( + predicate::str::contains("INVALID_INPUT") + .and(predicate::str::contains("Invalid profile name")), + ); + }; + let login = ["auth", "login", "--client-credentials"]; + let long = "x".repeat(65); + for name in ["customer/a", "a:b", "..", "", long.as_str()] { + refused(teams().args(["--profile", name]).args(login)); + } + refused(teams().env("TEAMS_CLI_PROFILE", "customer/a").args(login)); + refused(teams().args(["config", "set", "profiles.customer/a.tenant_id", "t"])); + refused(teams().args(["config", "set", "default.profile", "customer/a"])); + assert!(!config_path.exists()); +} + +/// A valid name gets past the name check; this login then stops for want of +/// a client secret, before any network call. +#[test] +fn a_valid_new_profile_name_is_accepted() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + for name in ["customer.a", "work_2-B"] { + teams() + .args(["--config", config, "--output", "json", "--profile", name]) + .args(["auth", "login", "--client-credentials"]) + .args(["--client-id", "app-1", "--tenant-id", "tenant-1"]) + .assert() + .code(2) + .stdout(predicate::str::contains("Client secret is required")); + } + teams() + .args(["--config", config, "--output", "json"]) + .args(["config", "set", "profiles.customer-a.tenant_id", "t"]) + .assert() + .success(); +} + +/// A profile created before names were limited keeps working: it loads from +/// the config, can be edited and signed in to again. +#[test] +fn a_legacy_profile_name_in_the_config_keeps_working() { + let dir = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let config_path = dir.path().join("config.toml"); + let config = config_path.to_str().unwrap(); + fs::write( + &config_path, + r#"[default] +profile = "customer/a" + +[profiles."customer/a"] +client_id = "app-1" +tenant_id = "tenant-1" +"#, + ) + .unwrap(); + teams() + .args(["--config", config, "--output", "json", "config", "profiles"]) + .assert() + .success() + .stdout(predicate::str::contains("customer/a")); + teams() + .args(["--config", config, "--output", "json"]) + .args(["config", "set", "profiles.customer/a.tenant_id", "tenant-2"]) + .assert() + .success(); + // Resolved from `default.profile`, and again given explicitly. + for profile in [&[][..], &["--profile", "customer/a"][..]] { + teams() + .args(["--config", config, "--output", "json"]) + .args(profile) + .args(["auth", "login", "--client-credentials"]) + .assert() + .code(2) + .stdout(predicate::str::contains("Client secret is required")); + } +} + +/// A legacy name known only from the file store's profile index also keeps +/// working. Not on Windows, whose config directory ignores `HOME`. +#[cfg(not(windows))] +#[test] +fn a_legacy_profile_name_in_the_token_index_keeps_working() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let file_store = || { + let mut cmd = teams(); + cmd.env_remove("TEAMS_CLI_DISABLE_KEYRING") + .env("TEAMS_CLI_TOKEN_STORE", "file") + .env("HOME", home.path()) + .env("XDG_CONFIG_HOME", home.path()); + cmd + }; + let output = file_store() + .args(["config", "path", "--output", "json"]) + .output() + .unwrap(); + let output: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + let config_path = std::path::PathBuf::from(output["data"]["path"].as_str().unwrap()); + assert!(config_path.starts_with(home.path())); + let tokens = config_path.parent().unwrap().join("tokens"); + fs::create_dir_all(&tokens).unwrap(); + fs::write(tokens.join("profile-index"), r#"["customer/a"]"#).unwrap(); + + for (name, expected) in [ + ("customer/a", "Client secret is required"), + ("customer/b", "Invalid profile name"), + ] { + file_store() + .args(["--profile", name, "--output", "json"]) + .args(["auth", "login", "--client-credentials"]) + .args(["--client-id", "app-1", "--tenant-id", "tenant-1"]) + .assert() + .code(2) + .stdout(predicate::str::contains(expected)); + } +} + /// `--help` names the environment variable a client secret can come from, but /// never prints the secret itself, which would put it in a terminal /// scrollback, a pasted bug report or an agent transcript. From ca0b0eb3b7e54a6148a3b7228549405eeb049d5e Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:18:13 +0100 Subject: [PATCH 4/6] Refuse a new profile name that shares an existing token file The file token store maps ':', '/' and '\' to '.', and existing profiles keep names outside the new set. So a legacy 'customer/a' and a new, valid 'customer.a' still mapped to one token file: logging in to the new one overwrote the old one's credentials. A new name is now also refused (exit 2) when its token file name matches any existing profile's, in the config or the token index. The mapping is shared with the file store, so the two cannot drift. New valid names now read the index; an unreadable index is still reported (exit 10), not taken as no profiles. Tests: unit cases for an indexed and a configured legacy name, and the CLI login case with 'customer/a' in the file store index; the CLI case fails against the previous check. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 2 +- src/auth/keyring.rs | 9 ++++++- src/config.rs | 58 +++++++++++++++++++++++++++++++++++---------- tests/cli.rs | 4 ++++ 4 files changed, 59 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a234c73..38b4127 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. - `teams config path` ignored `--config` and always reported the default path. It now reports the file in use, with `source` set to `flag` or `default`. - Config saves (`config set`, `config init`, `auth switch`) truncated the file and wrote it in place, so a failed write could leave a partial config. They now write a temporary file and rename it over the original, as `auth login` already did. -- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. +- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. A new name whose token file name matches an existing profile's is refused too. ## v0.9.0 - 2026-10-04 diff --git a/src/auth/keyring.rs b/src/auth/keyring.rs index c9ef739..b47b852 100644 --- a/src/auth/keyring.rs +++ b/src/auth/keyring.rs @@ -181,13 +181,20 @@ struct FileStore { dir: PathBuf, } +/// The file name the file token store uses for `key`. It maps `:`, `/` and +/// `\\` to `.`, so distinct keys can share a name; new profile names are +/// checked against it (`config::check_new_profile_name`). +pub(crate) fn file_store_name(key: &str) -> String { + key.replace([':', '/', '\\'], ".") +} + impl FileStore { fn new(dir: PathBuf) -> Self { Self { dir } } fn path(&self, key: &str) -> PathBuf { - self.dir.join(key.replace([':', '/', '\\'], ".")) + self.dir.join(file_store_name(key)) } fn ensure_dir(&self) -> Result<()> { diff --git a/src/config.rs b/src/config.rs index c2b85b2..494effd 100644 --- a/src/config.rs +++ b/src/config.rs @@ -248,8 +248,9 @@ pub fn is_valid_profile_name(name: &str) -> bool { .all(|b| b.is_ascii_alphanumeric() || matches!(b, b'.' | b'_' | b'-')) } -/// Refuse to create a profile under an invalid name. A profile that already -/// exists, in the config or the token index, keeps its name. +/// Refuse to create a profile under an invalid name, or under one whose token +/// file name matches an existing profile's. A profile that already exists, in +/// the config or the token index, keeps its name. pub fn check_new_profile_name(name: &str, config: &ConfigFile) -> Result<()> { check_new_profile_name_with(name, config, crate::auth::keyring::list_profiles) } @@ -259,15 +260,30 @@ fn check_new_profile_name_with( config: &ConfigFile, index: impl FnOnce() -> Result>, ) -> Result<()> { - if is_valid_profile_name(name) - || config.profiles.contains_key(name) - || index()?.iter().any(|p| p == name) - { + if config.profiles.contains_key(name) { + return Ok(()); + } + let indexed = index()?; + if indexed.iter().any(|p| p == name) { return Ok(()); } - Err(TeamsError::InvalidInput(format!( - "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" - ))) + if !is_valid_profile_name(name) { + return Err(TeamsError::InvalidInput(format!( + "Invalid profile name '{name}': use A-Z a-z 0-9 . _ - (max {MAX_PROFILE_NAME_LEN}, not . or ..)" + ))); + } + let file_name = crate::auth::keyring::file_store_name(name); + if let Some(other) = config + .profiles + .keys() + .chain(&indexed) + .find(|p| crate::auth::keyring::file_store_name(p) == file_name) + { + return Err(TeamsError::InvalidInput(format!( + "Profile name '{name}' would share a token file with profile '{other}'; choose another" + ))); + } + Ok(()) } pub fn resolve_client_id( @@ -698,9 +714,27 @@ scopes = "User.Read People.Read offline_access" fn check_new_profile_name_reports_an_unreadable_index() { let config = ConfigFile::default(); let unreadable = || Err(TeamsError::KeyringError("denied".into())); - let err = check_new_profile_name_with("other/b", &config, unreadable).unwrap_err(); - assert_eq!(err.exit_code(), 10); - check_new_profile_name_with("valid", &config, || panic!("index read")).unwrap(); + for name in ["other/b", "valid"] { + let err = check_new_profile_name_with(name, &config, unreadable).unwrap_err(); + assert_eq!(err.exit_code(), 10); + } + } + + #[test] + fn check_new_profile_name_refuses_a_shared_token_file() { + let mut config = ConfigFile::default(); + let legacy = || Ok(vec!["customer/a".to_string()]); + let err = check_new_profile_name_with("customer.a", &config, legacy).unwrap_err(); + assert_eq!(err.exit_code(), 2); + assert!(err.to_string().contains("customer/a"), "{err}"); + check_new_profile_name_with("customer.b", &config, legacy).unwrap(); + + config + .profiles + .insert("customer:a".into(), ProfileConfig::default()); + let err = + check_new_profile_name_with("customer.a", &config, || Ok(Vec::new())).unwrap_err(); + assert!(err.to_string().contains("customer:a"), "{err}"); } #[test] diff --git a/tests/cli.rs b/tests/cli.rs index f19e780..eb2b600 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1517,6 +1517,10 @@ fn a_legacy_profile_name_in_the_token_index_keeps_working() { for (name, expected) in [ ("customer/a", "Client secret is required"), ("customer/b", "Invalid profile name"), + ( + "customer.a", + "would share a token file with profile 'customer/a'", + ), ] { file_store() .args(["--profile", name, "--output", "json"]) From 0c15e50979a93be507e07db2c698f812ceb21ea6 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 17:21:05 +0100 Subject: [PATCH 5/6] Compare profile token file names without regard to case macOS and Windows file systems ignore case by default, so 'Work' and 'work' address one token file. The collision check compared names exactly, letting a new 'work' replace an existing 'Work' profile's credentials. Compare them ignoring ASCII case. Test: an existing 'Work' refuses a new 'work'; fails with an exact comparison. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 2 +- src/config.rs | 8 ++++++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 38b4127..2b91740 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,7 @@ - `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. - `teams config path` ignored `--config` and always reported the default path. It now reports the file in use, with `source` set to `flag` or `default`. - Config saves (`config set`, `config init`, `auth switch`) truncated the file and wrote it in place, so a failed write could leave a partial config. They now write a temporary file and rename it over the original, as `auth login` already did. -- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. A new name whose token file name matches an existing profile's is refused too. +- A new profile's name is limited to 1-64 of `A-Z a-z 0-9 . _ -`, not `.` or `..`. `TEAMS_CLI_TOKEN_STORE=file` maps `:`, `/` and `\` to `.` in token file names, so `customer/a` and `customer.a` shared one token and could run as the wrong account. `auth login`, `config set profiles....` and `config set default.profile` refuse another name as invalid input (exit code 2). Profiles already in the config or the token index keep working under their names. A new name whose token file name matches an existing profile's, ignoring case, is refused too. ## v0.9.0 - 2026-10-04 diff --git a/src/config.rs b/src/config.rs index 494effd..3aa023a 100644 --- a/src/config.rs +++ b/src/config.rs @@ -249,7 +249,8 @@ pub fn is_valid_profile_name(name: &str) -> bool { } /// Refuse to create a profile under an invalid name, or under one whose token -/// file name matches an existing profile's. A profile that already exists, in +/// file name matches an existing profile's, ignoring case as macOS and Windows +/// file systems do by default. A profile that already exists, in /// the config or the token index, keeps its name. pub fn check_new_profile_name(name: &str, config: &ConfigFile) -> Result<()> { check_new_profile_name_with(name, config, crate::auth::keyring::list_profiles) @@ -277,7 +278,7 @@ fn check_new_profile_name_with( .profiles .keys() .chain(&indexed) - .find(|p| crate::auth::keyring::file_store_name(p) == file_name) + .find(|p| crate::auth::keyring::file_store_name(p).eq_ignore_ascii_case(&file_name)) { return Err(TeamsError::InvalidInput(format!( "Profile name '{name}' would share a token file with profile '{other}'; choose another" @@ -728,6 +729,9 @@ scopes = "User.Read People.Read offline_access" assert_eq!(err.exit_code(), 2); assert!(err.to_string().contains("customer/a"), "{err}"); check_new_profile_name_with("customer.b", &config, legacy).unwrap(); + let existing = || Ok(vec!["Work".to_string()]); + let err = check_new_profile_name_with("work", &config, existing).unwrap_err(); + assert!(err.to_string().contains("'Work'"), "{err}"); config .profiles From f0a83750e46cbcdbb95cebc2eababad75f63c980 Mon Sep 17 00:00:00 2001 From: Abraham Ingersoll <586805+aberoham@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:16:30 +0100 Subject: [PATCH 6/6] Report user, tenant and store errors in auth status auth status put the signed-in user only inside token_diagnostics, so .data.user read null although auth list shows it; and any failure to load a token, a locked or denied keychain included, came out as authenticated: false with exit 1, the "signed out" answer. Report user and tenant_id at the top level as well. A token store that cannot be read is now KEYRING_ERROR (exit 10), as for every other command; any other failure stays authenticated: false (exit 1) and says why in reason. Tests: top-level user and tenant from a token's claims; a missing token is exit 1 with a reason, an unreadable store is exit 10. Both fail against the previous handler. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Hd5XXx9UkpLZpAAkvPPyUg --- CHANGELOG.md | 1 + src/cli/auth.rs | 8 ++++++- tests/cli.rs | 59 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 67 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b91740..60ce7ab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- `teams auth status` reports `user` and `tenant_id` at the top level, as `auth list` does, not only inside `token_diagnostics`. A token store that cannot be read is now `KEYRING_ERROR` (exit code 10) instead of `authenticated: false`, and a signed-out profile says why in `reason`. - A token store that cannot be read (a locked keychain, a denied prompt, an SSH session) is reported as `KEYRING_ERROR` (exit code 10) with its cause, instead of "Not authenticated" (exit code 3). Only a profile with no stored token is "Not authenticated". - A profile index that cannot be read or parsed is an error (exit code 10) instead of an empty list. `auth login` no longer rewrites it to hold only the new profile, orphaning the rest; `auth list` and `auth logout --all` no longer report no profiles as a success. - `teams auth switch` with `--config` wrote the selected profile to the default config file, overwriting it with the `--config` file's contents and leaving that file unchanged. It now saves to the `--config` file, and a failed save is a config error (exit code 10) instead of a warning beside a success. diff --git a/src/cli/auth.rs b/src/cli/auth.rs index f6716b0..f4b8185 100644 --- a/src/cli/auth.rs +++ b/src/cli/auth.rs @@ -491,6 +491,8 @@ pub async fn run( let msg = serde_json::json!({ "authenticated": true, "profile": profile, + "user": claims.as_ref().and_then(|c| c.user()), + "tenant_id": claims.as_ref().and_then(|c| c.tid.clone()), "expires_at": token.expires_at.map(|e| e.to_rfc3339()), "scope": token.scope, "token_diagnostics": token_diagnostics(claims.as_ref()), @@ -499,10 +501,14 @@ pub async fn run( output::print_success(format, &msg, start); Ok(()) } - Err(_) => { + // A token store that cannot be read is not "signed out"; report it as + // every other command does (exit 10). + Err(e @ TeamsError::KeyringError(_)) => Err(e), + Err(e) => { let msg = serde_json::json!({ "authenticated": false, "profile": profile, + "reason": e.to_string(), }); output::print_success(format, &msg, start); std::process::exit(1); diff --git a/tests/cli.rs b/tests/cli.rs index eb2b600..780c2ee 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -1815,3 +1815,62 @@ fn auth_switch_fails_when_the_config_cannot_be_saved() { .code(10) .stdout(predicate::str::contains("\"success\": false")); } + +/// `auth status` names the signed-in user and tenant at the top level, as +/// `auth list` does, not only inside `token_diagnostics`. +#[test] +fn auth_status_reports_user_and_tenant() { + use base64::Engine as _; + let b64 = |v: serde_json::Value| { + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(v.to_string()) + }; + let jwt = format!( + "{}.{}.sig", + b64(serde_json::json!({ "alg": "none" })), + b64(serde_json::json!({ + "preferred_username": "a@contoso.com", + "tid": "tenant-1", + "aud": "https://graph.microsoft.com", + "exp": 4_102_444_800u64 + })) + ); + let output = teams() + .env("TEAMS_CLI_ACCESS_TOKEN", jwt) + .args(["auth", "status", "--output", "json"]) + .output() + .unwrap(); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["user"], "a@contoso.com", "{data}"); + assert_eq!(data["data"]["tenant_id"], "tenant-1"); +} + +/// `auth status` reports an unreadable token store as a storage error (exit +/// 10), and only a profile with no token as signed out (exit 1). +#[cfg(not(windows))] +#[test] +fn auth_status_reports_an_unreadable_store_as_a_storage_error() { + let home = tempfile::tempdir_in(env!("CARGO_TARGET_TMPDIR")).unwrap(); + let (file_store, tokens) = file_store_in(home.path()); + + let output = file_store() + .args(["auth", "status", "--output", "json"]) + .output() + .unwrap(); + assert_eq!(output.status.code(), Some(1)); + let data: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(data["data"]["authenticated"], false); + assert!( + data["data"]["reason"] + .as_str() + .unwrap() + .contains("Not authenticated"), + "{data}" + ); + + fs::create_dir_all(tokens.join("default.token")).unwrap(); + file_store() + .args(["auth", "status", "--output", "json"]) + .assert() + .code(10) + .stdout(predicate::str::contains("KEYRING_ERROR")); +}