From 72bfb0162b71db1c9ebc098c9019ca2174f8e954 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:23:04 -0400 Subject: [PATCH 1/4] Describe profiles in --json without their credentials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `imogen profiles --json` serialised the whole `Config`, so `access_token` and `refresh_token` went to stdout on any logged-in machine — into a pipe, a CI log, a shell history, an agent transcript. The table beside it showed name, server, auth kind and scopes and no secret, so the two audiences disagreed about what was safe to print, and the machine one got strictly more. `--json` now answers with a `ProfilesView` naming the same four things the table does, plus `expires_at` where there is one. A view struct rather than `#[serde(skip_serializing)]` on the token fields: the attribute has to be remembered by whoever adds the next credential to `Config`, while a struct that must be extended on purpose cannot leak one by omission. The table's "signed in via" and its notion of which profile is current now come from the same two helpers the view uses, so the audiences cannot drift apart again. Closes #33 Co-Authored-By: Claude Opus 5 --- src/commands/session.rs | 178 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 170 insertions(+), 8 deletions(-) diff --git a/src/commands/session.rs b/src/commands/session.rs index ffe024c..1d41138 100644 --- a/src/commands/session.rs +++ b/src/commands/session.rs @@ -1,7 +1,10 @@ //! Signing in and out, and the saved logins. +use std::collections::BTreeMap; + use anyhow::{bail, Result}; use imogen_sdk::OAuthClient; +use serde::Serialize; use serde_json::json; use crate::auth; @@ -150,29 +153,25 @@ pub fn profiles(global: &GlobalArgs, args: &ProfilesArgs) -> Result<()> { } if out.is_json() { - return out.json(&config); + return out.json(&profiles_view(&config)); } if config.profiles.is_empty() { out.note("No saved logins. Run: imogen login --server https://photos.example.com"); return Ok(()); } - let current = config.default_profile_name(); + let current = current_profile(&config); let rows: Vec> = config .profiles .iter() .map(|(name, profile)| { vec![ - if *name == current { + if Some(name.as_str()) == current { out.paint(name, YELLOW) } else { name.clone() }, profile.server.clone(), - if profile.client_id.is_some() { - "browser".into() - } else { - "token".into() - }, + signed_in_via(profile).to_string(), profile.scope.clone(), ] }) @@ -180,3 +179,166 @@ pub fn profiles(global: &GlobalArgs, args: &ProfilesArgs) -> Result<()> { out.table(&["PROFILE", "SERVER", "SIGNED IN VIA", "SCOPES"], &rows); Ok(()) } + +/// What `--json` says about the saved logins: the four things the table already shows, +/// and nothing else. +/// +/// A view struct rather than `#[serde(skip_serializing)]` on `Profile`'s token fields, +/// because this one names everything it prints. A skip attribute has to be remembered by +/// whoever adds the next credential to the config; a struct that has to be extended on +/// purpose cannot leak one by omission. +#[derive(Serialize)] +struct ProfilesView<'a> { + current: Option<&'a str>, + profiles: BTreeMap<&'a str, ProfileView<'a>>, +} + +#[derive(Serialize)] +struct ProfileView<'a> { + server: &'a str, + signed_in_via: &'static str, + scope: &'a str, + /// Unix milliseconds. Absent for a pasted token, which carries no expiry at all. + #[serde(skip_serializing_if = "Option::is_none")] + expires_at: Option, +} + +fn profiles_view(config: &Config) -> ProfilesView<'_> { + ProfilesView { + current: current_profile(config), + profiles: config + .profiles + .iter() + .map(|(name, profile)| { + ( + name.as_str(), + ProfileView { + server: &profile.server, + signed_in_via: signed_in_via(profile), + scope: &profile.scope, + expires_at: expires_at(profile), + }, + ) + }) + .collect(), + } +} + +/// The profile a command would reach for today — the one the table paints yellow. +/// `Config::default_profile_name` invents “default” for an empty config, which is the +/// right answer for naming a profile about to be written and the wrong one for describing +/// what is saved. +fn current_profile(config: &Config) -> Option<&str> { + let named = config.current.as_deref(); + config + .profiles + .keys() + .map(String::as_str) + .find(|name| Some(*name) == named) + .or_else(|| config.profiles.keys().next().map(String::as_str)) +} + +fn signed_in_via(profile: &Profile) -> &'static str { + if profile.client_id.is_some() { + "browser" + } else { + "token" + } +} + +fn expires_at(profile: &Profile) -> Option { + if profile.expires_in <= 0 || profile.obtained_at == 0 { + return None; + } + Some(profile.obtained_at + profile.expires_in as u128 * 1000) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn browser_profile() -> Profile { + Profile { + server: "https://photos.example.com".into(), + client_id: Some("client-1".into()), + access_token: Some("at-SECRET-ACCESS".into()), + refresh_token: Some("rt-SECRET-REFRESH".into()), + expires_in: 3600, + obtained_at: 1_700_000_000_000, + scope: "library:read".into(), + } + } + + fn saved() -> Config { + let mut config = Config::default(); + config.set("home", browser_profile()); + config.set( + "family", + Profile::from_token( + "https://family.example.com".into(), + "at-SECRET-PASTED".into(), + ), + ); + config + } + + /// A grep of the serialised bytes rather than a field-by-field check: a credential + /// added to `Profile` later would reintroduce the leak silently, and only looking at + /// the whole string catches it. + #[test] + fn the_json_answer_carries_no_credentials() { + let config = saved(); + let json = serde_json::to_string(&profiles_view(&config)).unwrap(); + + for secret in ["at-SECRET-ACCESS", "rt-SECRET-REFRESH", "at-SECRET-PASTED"] { + assert!(!json.contains(secret), "{secret} reached stdout: {json}"); + } + assert!( + !json.contains("token\":"), + "no credential field at all: {json}" + ); + } + + #[test] + fn the_json_answer_describes_what_the_table_shows() { + let config = saved(); + let json: serde_json::Value = serde_json::to_value(profiles_view(&config)).unwrap(); + + assert_eq!( + json["current"], "home", + "the first profile saved is the default" + ); + assert_eq!( + json["profiles"]["home"]["server"], + "https://photos.example.com" + ); + assert_eq!(json["profiles"]["home"]["signed_in_via"], "browser"); + assert_eq!(json["profiles"]["home"]["scope"], "library:read"); + assert_eq!( + json["profiles"]["home"]["expires_at"], + 1_700_000_000_000u64 + 3_600_000 + ); + + assert_eq!(json["profiles"]["family"]["signed_in_via"], "token"); + assert!( + json["profiles"]["family"]["expires_at"].is_null(), + "a pasted token has no expiry to report" + ); + } + + #[test] + fn nothing_is_current_until_something_is_saved() { + let json = serde_json::to_value(profiles_view(&Config::default())).unwrap(); + assert!(json["current"].is_null()); + } + + /// `current` naming a profile that was removed would send automation to a login that + /// is not there; the answer falls back the same way the commands themselves do. + #[test] + fn a_stale_current_falls_back_to_a_profile_that_exists() { + let mut config = saved(); + config.current = Some("gone".into()); + let json = serde_json::to_value(profiles_view(&config)).unwrap(); + assert_eq!(json["current"], "family", "the first profile by name"); + } +} From 9ab972e40e21104185b6f8c968c1ba22d818426a Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:30:22 -0400 Subject: [PATCH 2/4] Report a stale current profile as the commands will read it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding. `current_profile` skipped a `current` naming a profile that is no longer saved and fell to the first key, while every other command resolves through `Config::default_profile_name`, which returns it verbatim. With `current = "gone"`, `profiles` reported and painted `family` while `ls` failed on "gone" — the two disagreeing about which login is in use, which is the thing this change set out to stop. It now gives the same answer as `default_profile_name`, departing only for the empty config, where that function invents “default”: the right name for a profile about to be written, and the wrong description of what exists. The tests assert the two agree rather than asserting the view alone. Co-Authored-By: Claude Opus 5 --- src/commands/session.rs | 47 +++++++++++++++++++++++++++++------------ 1 file changed, 34 insertions(+), 13 deletions(-) diff --git a/src/commands/session.rs b/src/commands/session.rs index 1d41138..d2729dd 100644 --- a/src/commands/session.rs +++ b/src/commands/session.rs @@ -224,17 +224,23 @@ fn profiles_view(config: &Config) -> ProfilesView<'_> { } } -/// The profile a command would reach for today — the one the table paints yellow. -/// `Config::default_profile_name` invents “default” for an empty config, which is the -/// right answer for naming a profile about to be written and the wrong one for describing -/// what is saved. +/// The profile a command would reach for, which is what both audiences are told. +/// +/// The same answer as `Config::default_profile_name`, on purpose: a `current` naming a +/// profile that is no longer saved is reported as it stands rather than quietly repaired, +/// because that is the name every other command will try and fail on. Repairing it here +/// would have `profiles` point at one login while `ls` reached for another. +/// +/// The one departure is the empty config, where `default_profile_name` invents “default”: +/// that is the right name for a profile about to be written and the wrong description of +/// what exists. fn current_profile(config: &Config) -> Option<&str> { - let named = config.current.as_deref(); + if config.profiles.is_empty() { + return None; + } config - .profiles - .keys() - .map(String::as_str) - .find(|name| Some(*name) == named) + .current + .as_deref() .or_else(|| config.profiles.keys().next().map(String::as_str)) } @@ -332,13 +338,28 @@ mod tests { assert!(json["current"].is_null()); } - /// `current` naming a profile that was removed would send automation to a login that - /// is not there; the answer falls back the same way the commands themselves do. + /// A `current` naming a profile that is no longer saved — a hand-edited file, or one + /// written by an older version — is reported as it stands, because that is the name + /// every other command will try. Reporting a repaired one would say `profiles` and + /// `ls` agree when they do not. #[test] - fn a_stale_current_falls_back_to_a_profile_that_exists() { + fn a_stale_current_is_reported_as_the_commands_will_read_it() { let mut config = saved(); config.current = Some("gone".into()); + + assert_eq!(config.default_profile_name(), "gone"); + let json = serde_json::to_value(profiles_view(&config)).unwrap(); + assert_eq!(json["current"], "gone"); + } + + /// With no `current` recorded, both audiences fall to the first profile by name. + #[test] + fn with_nothing_chosen_the_first_profile_by_name_is_current() { + let mut config = saved(); + config.current = None; + + assert_eq!(config.default_profile_name(), "family"); let json = serde_json::to_value(profiles_view(&config)).unwrap(); - assert_eq!(json["current"], "family", "the first profile by name"); + assert_eq!(json["current"], "family"); } } From 1ec9f074234a1dd14c981c4c824cec7784295b05 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:32:22 -0400 Subject: [PATCH 3/4] Name the new --json keys the way every other command names them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding. `signed_in_via` and `expires_at` were snake_case because the fields they replace wear that casing on disk, but every key this program synthesises is camelCase — `capturedAt`, `faceIds`, `loggedOut` — as are the SDK's own models. A script that reads `.loggedOut` from one command and `.signed_in_via` from this one has no way to predict which it will get. This PR redefines the shape of `profiles --json` anyway, so aligning costs nothing now and would be a second break later. Co-Authored-By: Claude Opus 5 --- src/commands/session.rs | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/commands/session.rs b/src/commands/session.rs index d2729dd..f49c55e 100644 --- a/src/commands/session.rs +++ b/src/commands/session.rs @@ -193,7 +193,10 @@ struct ProfilesView<'a> { profiles: BTreeMap<&'a str, ProfileView<'a>>, } +/// camelCase like every other key this program synthesises — `capturedAt`, `faceIds`, +/// `loggedOut` — rather than the snake_case these fields happen to wear on disk. #[derive(Serialize)] +#[serde(rename_all = "camelCase")] struct ProfileView<'a> { server: &'a str, signed_in_via: &'static str, @@ -318,16 +321,16 @@ mod tests { json["profiles"]["home"]["server"], "https://photos.example.com" ); - assert_eq!(json["profiles"]["home"]["signed_in_via"], "browser"); + assert_eq!(json["profiles"]["home"]["signedInVia"], "browser"); assert_eq!(json["profiles"]["home"]["scope"], "library:read"); assert_eq!( - json["profiles"]["home"]["expires_at"], + json["profiles"]["home"]["expiresAt"], 1_700_000_000_000u64 + 3_600_000 ); - assert_eq!(json["profiles"]["family"]["signed_in_via"], "token"); + assert_eq!(json["profiles"]["family"]["signedInVia"], "token"); assert!( - json["profiles"]["family"]["expires_at"].is_null(), + json["profiles"]["family"]["expiresAt"].is_null(), "a pasted token has no expiry to report" ); } From 1ef6ab3eb06d0a01fe79a7c5c16b193b24fc1cc1 Mon Sep 17 00:00:00 2001 From: Jim Phillips <5315024+ergofobe@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:34:49 -0400 Subject: [PATCH 4/4] Make the regression guard survive the next credential's name MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings, both low, both about a claim the change makes being wider than the code behind it. The structural half of the credential test grepped for `token":`, which the camelCase rename two commits ago had already defeated: a field added later as `access_token` serialises as `accessToken` and would have sailed past the one assertion advertised as catching it. It now walks the keys of the serialised view case-insensitively — keys rather than bytes, because “token” is a legitimate *value* of `signedInVia`. Proven by adding an `access_token` field with a harmless value and watching it fail. `current_profile`'s empty-profiles early return also swallowed a recorded `current`, so a truncated config would have had `profiles` report null while every other command still resolved and failed on the name. Dropping the early return leaves exactly the one departure the comment claims. Co-Authored-By: Claude Opus 5 --- src/commands/session.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/src/commands/session.rs b/src/commands/session.rs index f49c55e..7ea9055 100644 --- a/src/commands/session.rs +++ b/src/commands/session.rs @@ -238,9 +238,6 @@ fn profiles_view(config: &Config) -> ProfilesView<'_> { /// that is the right name for a profile about to be written and the wrong description of /// what exists. fn current_profile(config: &Config) -> Option<&str> { - if config.profiles.is_empty() { - return None; - } config .current .as_deref() @@ -302,10 +299,27 @@ mod tests { for secret in ["at-SECRET-ACCESS", "rt-SECRET-REFRESH", "at-SECRET-PASTED"] { assert!(!json.contains(secret), "{secret} reached stdout: {json}"); } - assert!( - !json.contains("token\":"), - "no credential field at all: {json}" - ); + // Every key, not every byte: “token” is a legitimate *value* of `signedInVia`. + // Case-insensitive because `rename_all = "camelCase"` means the next credential + // would arrive as `accessToken` rather than the `access_token` it wears on disk. + let view = serde_json::to_value(profiles_view(&config)).unwrap(); + for key in keys(&view) { + let key = key.to_lowercase(); + for word in ["token", "secret", "password", "credential"] { + assert!(!key.contains(word), "“{key}” is a credential field: {json}"); + } + } + } + + fn keys(value: &serde_json::Value) -> Vec { + match value { + serde_json::Value::Object(map) => map + .iter() + .flat_map(|(key, nested)| std::iter::once(key.clone()).chain(keys(nested))) + .collect(), + serde_json::Value::Array(items) => items.iter().flat_map(keys).collect(), + _ => Vec::new(), + } } #[test]