From 3b2bc5bf9d52bcaac3c468178aceb5a92cfe6544 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 7 May 2026 16:11:08 -0700 Subject: [PATCH 01/11] feat(auth): add org_uuid to SessionEntry with serde-default backward compat Carries the authoritative `dd_oid` returned by the OAuth issuer alongside the existing free-form `org` label. `#[serde(default)]` lets sessions.json files written before this field existed deserialize cleanly, and `skip_serializing_if = "Option::is_none"` keeps the on-disk shape byte-stable for sessions that were never tagged with a UUID. Adds `find_session(site, org)` so login can recall a previously-stored UUID without searching the registry by hand. Existing callers of `save_session` pass `None` for the new third arg; the upcoming login-path change will populate it from the callback. --- src/auth/storage.rs | 81 +++++++++++++++++++++++++++++++++++++------- src/commands/auth.rs | 10 +++--- src/config.rs | 10 +++--- 3 files changed, 78 insertions(+), 23 deletions(-) diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 8ac9a7f0..81df5895 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -11,6 +11,11 @@ use super::types::{ClientCredentials, TokenSet}; pub struct SessionEntry { pub site: String, pub org: Option, + /// Authoritative org UUID (`dd_oid`) returned by the OAuth issuer. + /// `#[serde(default)]` lets sessions.json files written before this field + /// existed deserialize cleanly (treated as None). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub org_uuid: Option, } // --------------------------------------------------------------------------- @@ -880,11 +885,12 @@ pub fn list_sessions() -> Result> { /// Upsert a session entry into the registry. #[cfg(not(target_arch = "wasm32"))] -pub fn save_session(site: &str, org: Option<&str>) -> Result<()> { +pub fn save_session(site: &str, org: Option<&str>, org_uuid: Option<&str>) -> Result<()> { let mut sessions = list_sessions()?; let entry = SessionEntry { site: site.to_string(), org: org.map(String::from), + org_uuid: org_uuid.map(String::from), }; // Dedup: remove any existing entry with same site+org, then append sessions.retain(|s| !(s.site == entry.site && s.org == entry.org)); @@ -900,6 +906,16 @@ pub fn remove_session(site: &str, org: Option<&str>) -> Result<()> { write_sessions(&sessions) } +/// Look up a single session entry by `(site, org)`. +#[cfg(not(target_arch = "wasm32"))] +#[allow(dead_code)] +pub fn find_session(site: &str, org: Option<&str>) -> Option { + list_sessions() + .ok()? + .into_iter() + .find(|s| s.site == site && s.org.as_deref() == org) +} + /// Look up the site for a named org session. Returns None if no session exists /// for that org, or if multiple sessions share the same org name on different /// sites (ambiguous — caller must pass DD_SITE explicitly). On the ambiguous @@ -1241,6 +1257,45 @@ mod tests { .is_none()); } + // --- SessionEntry serde ------------------------------------------------- + + #[test] + fn test_session_entry_legacy_json_deserializes_with_no_org_uuid() { + // sessions.json files written before the org_uuid field existed must + // continue to deserialize. #[serde(default)] gives them None. + let legacy = r#"[{"site":"datadoghq.com","org":"prod-child"}]"#; + let parsed: Vec = serde_json::from_str(legacy).unwrap(); + assert_eq!(parsed.len(), 1); + assert_eq!(parsed[0].site, "datadoghq.com"); + assert_eq!(parsed[0].org.as_deref(), Some("prod-child")); + assert!(parsed[0].org_uuid.is_none()); + } + + #[test] + fn test_session_entry_roundtrip_with_uuid() { + let entry = SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091".into()), + }; + let json = serde_json::to_string(&entry).unwrap(); + let parsed: SessionEntry = serde_json::from_str(&json).unwrap(); + assert_eq!(parsed, entry); + } + + #[test] + fn test_session_entry_omits_uuid_when_none() { + // skip_serializing_if keeps existing on-disk shapes byte-stable for + // sessions that were never tagged with a UUID. + let entry = SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }; + let json = serde_json::to_string(&entry).unwrap(); + assert!(!json.contains("org_uuid"), "got: {json}"); + } + // --- Session registry --------------------------------------------------- #[test] @@ -1259,8 +1314,8 @@ mod tests { let tmp = TempDir::new("sess_save"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", None).unwrap(); - save_session("datadoghq.com", Some("prod-child")).unwrap(); + save_session("datadoghq.com", None, None).unwrap(); + save_session("datadoghq.com", Some("prod-child"), None).unwrap(); let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1279,8 +1334,8 @@ mod tests { let tmp = TempDir::new("sess_dedup"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", Some("prod")).unwrap(); - save_session("datadoghq.com", Some("prod")).unwrap(); // duplicate + save_session("datadoghq.com", Some("prod"), None).unwrap(); + save_session("datadoghq.com", Some("prod"), None).unwrap(); // duplicate let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1293,8 +1348,8 @@ mod tests { let tmp = TempDir::new("sess_remove"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", None).unwrap(); - save_session("datadoghq.com", Some("prod")).unwrap(); + save_session("datadoghq.com", None, None).unwrap(); + save_session("datadoghq.com", Some("prod"), None).unwrap(); remove_session("datadoghq.com", Some("prod")).unwrap(); let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1319,8 +1374,8 @@ mod tests { let tmp = TempDir::new("find_sess_unique"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("custom.datadoghq.com", Some("prod-child")).unwrap(); - save_session("datadoghq.com", None).unwrap(); + save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); + save_session("datadoghq.com", None, None).unwrap(); let result = find_session_site("prod-child"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1333,7 +1388,7 @@ mod tests { let tmp = TempDir::new("find_sess_none"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", Some("prod-child")).unwrap(); + save_session("datadoghq.com", Some("prod-child"), None).unwrap(); let result = find_session_site("nonexistent"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1348,8 +1403,8 @@ mod tests { // Same org name registered against two different sites → caller must // disambiguate via DD_SITE rather than us picking one. - save_session("datadoghq.com", Some("shared-name")).unwrap(); - save_session("datadoghq.eu", Some("shared-name")).unwrap(); + save_session("datadoghq.com", Some("shared-name"), None).unwrap(); + save_session("datadoghq.eu", Some("shared-name"), None).unwrap(); let result = find_session_site("shared-name"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1363,7 +1418,7 @@ mod tests { std::env::set_var("PUP_CONFIG_DIR", tmp.path()); // The unnamed (org=None) session must not match any --org lookup. - save_session("datadoghq.eu", None).unwrap(); + save_session("datadoghq.eu", None, None).unwrap(); let result = find_session_site("anything"); std::env::remove_var("PUP_CONFIG_DIR"); diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 15116ff7..2f1f2455 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -151,7 +151,7 @@ pub async fn login( })?; // Register this session in the session registry - storage::save_session(site, org)?; + storage::save_session(site, org, None)?; let expires_at = chrono::DateTime::from_timestamp(tokens.issued_at + tokens.expires_in, 0) .map(|dt| dt.with_timezone(&chrono::Local).to_rfc3339()) @@ -506,8 +506,8 @@ mod tests { std::env::set_var("PUP_CONFIG_DIR", tmp.path()); std::env::set_var("DD_TOKEN_STORAGE", "file"); - storage::save_session("datadoghq.com", None).unwrap(); - storage::save_session("datadoghq.com", Some("prod-child")).unwrap(); + storage::save_session("datadoghq.com", None, None).unwrap(); + storage::save_session("datadoghq.com", Some("prod-child"), None).unwrap(); let cfg = base_config(); let result = list(&cfg); @@ -603,8 +603,8 @@ mod tests { std::env::set_var("DD_TOKEN_STORAGE", "file"); let site = "logout-session.example.invalid"; - storage::save_session(site, None).unwrap(); - storage::save_session(site, Some("keep-me")).unwrap(); + storage::save_session(site, None, None).unwrap(); + storage::save_session(site, Some("keep-me"), None).unwrap(); let mut cfg = base_config(); cfg.site = site.into(); diff --git a/src/config.rs b/src/config.rs index 188c0e27..1f1c405c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -937,7 +937,7 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child")).unwrap(); + crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); std::env::set_var("DD_ORG", "prod-child"); let cfg = Config::from_env().unwrap(); @@ -967,7 +967,7 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child")).unwrap(); + crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); std::env::set_var("DD_ORG", "prod-child"); std::env::set_var("DD_SITE", "datadoghq.eu"); @@ -998,8 +998,8 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("a.datadoghq.com", Some("org-a")).unwrap(); - crate::auth::storage::save_session("b.datadoghq.com", Some("org-b")).unwrap(); + crate::auth::storage::save_session("a.datadoghq.com", Some("org-a"), None).unwrap(); + crate::auth::storage::save_session("b.datadoghq.com", Some("org-b"), None).unwrap(); // Simulate the post-from_env state where we resolved org-a's site via // the registry (site_explicit=false because the user did not set DD_SITE). @@ -1040,7 +1040,7 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("session.datadoghq.com", Some("org-a")).unwrap(); + crate::auth::storage::save_session("session.datadoghq.com", Some("org-a"), None).unwrap(); let mut cfg = Config { api_key: None, From a65ee9be66866cb3196b5edba3ad03b8be78acd6 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 7 May 2026 16:14:56 -0700 Subject: [PATCH 02/11] feat(auth): plumb --org-uuid through dcr + callback parsing Adds a `--org-uuid ` flag on `pup auth login` that emits the value as `dd_oid` in the OAuth authorize URL. The hint lets the authorize endpoint skip the org switcher when the existing browser session already matches, and pre-routes SAML/SSO routing for first-time logins. Resolution order for the effective UUID: 1. CLI flag `--org-uuid` if provided 2. Stored `SessionEntry.org_uuid` from the session registry 3. None (no hint) After token exchange, the callback's authoritative `dd_oid` is persisted on the session record so subsequent invocations re-emit it without the flag. The callback parser now also captures `dd_org_name` for use in the mismatch path (next commit). Empty UUID strings are coerced to "no hint" (mirrors the empty-subdomain behaviour) so an empty `--org-uuid ""` falls back rather than emitting a malformed `dd_oid=`. --- src/auth/callback.rs | 39 ++++++++++++++++++++++++ src/auth/dcr.rs | 71 ++++++++++++++++++++++++++++++++++++++++++-- src/commands/auth.rs | 26 ++++++++++++++-- src/main.rs | 17 ++++++++++- 4 files changed, 147 insertions(+), 6 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index 2a491b41..c88eba19 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -15,6 +15,15 @@ pub struct CallbackResult { pub state: String, pub error: Option, pub error_description: Option, + /// Authoritative org UUID returned by the OAuth server. Present when the + /// authorize response carries `dd_oid` — true for Datadog OAuth today on + /// both the org-switcher and direct-consent paths. + pub dd_oid: Option, + /// Display name of the org the user actually authenticated against, + /// returned alongside `dd_oid` as `dd_org_name`. Used as the saved-token + /// label when the user-supplied org/UUID didn't match the actual one. + #[allow(dead_code)] + pub dd_org_name: Option, } #[cfg(not(target_arch = "wasm32"))] @@ -162,6 +171,8 @@ async fn accept_loop( let state = params.get("state").cloned().unwrap_or_default(); let error = params.get("error").cloned(); let error_description = params.get("error_description").cloned(); + let dd_oid = params.get("dd_oid").cloned(); + let dd_org_name = params.get("dd_org_name").cloned(); let (status, body) = if error.is_some() { ("400 Bad Request", error_page(&error, &error_description)) @@ -179,6 +190,8 @@ async fn accept_loop( state, error, error_description, + dd_oid, + dd_org_name, }; if let Some(tx) = result_tx.lock().unwrap().take() { let _ = tx.send(result); @@ -224,12 +237,16 @@ fn parse_callback_url(input: &str) -> Result { let mut state = None; let mut error = None; let mut error_description = None; + let mut dd_oid = None; + let mut dd_org_name = None; for (k, v) in url.query_pairs() { match k.as_ref() { "code" => code = Some(v.into_owned()), "state" => state = Some(v.into_owned()), "error" => error = Some(v.into_owned()), "error_description" => error_description = Some(v.into_owned()), + "dd_oid" => dd_oid = Some(v.into_owned()), + "dd_org_name" => dd_org_name = Some(v.into_owned()), _ => {} } } @@ -241,6 +258,8 @@ fn parse_callback_url(input: &str) -> Result { state: state.unwrap_or_default(), error, error_description, + dd_oid, + dd_org_name, }) } @@ -290,6 +309,26 @@ mod tests { assert_eq!(r.state, "xyz789"); assert!(r.error.is_none()); assert!(r.error_description.is_none()); + assert!(r.dd_oid.is_none()); + assert!(r.dd_org_name.is_none()); + } + + #[test] + fn parse_callback_url_extracts_dd_oid_and_org_name() { + // Real callback shape from a Datadog OAuth flow — the issuer appends + // dd_oid and (URL-encoded) dd_org_name alongside code and state. + let r = parse_callback_url( + "http://127.0.0.1:8000/oauth/callback\ + ?code=abc&state=xyz\ + &dd_oid=8dee7c38-00cb-11ea-a77b-8b5a08d3b091\ + &dd_org_name=Datadog+HQ", + ) + .unwrap(); + assert_eq!( + r.dd_oid.as_deref(), + Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091") + ); + assert_eq!(r.dd_org_name.as_deref(), Some("Datadog HQ")); } #[test] diff --git a/src/auth/dcr.rs b/src/auth/dcr.rs index 1c66a472..2c9ba715 100644 --- a/src/auth/dcr.rs +++ b/src/auth/dcr.rs @@ -190,6 +190,11 @@ impl DcrClient { } /// Build the authorization URL for the browser. + /// + /// `org_uuid` (when set) is sent as the `dd_oid` query param. The + /// authorize endpoint uses it to skip the org switcher when the existing + /// browser session already matches, and to pre-route SAML/SSO routing + /// for first-time logins. Empty strings are coerced to `None`. pub fn build_authorization_url( &self, client_id: &str, @@ -198,17 +203,22 @@ impl DcrClient { challenge: &super::pkce::PkceChallenge, scopes: &[&str], subdomain: Option<&str>, + org_uuid: Option<&str>, ) -> String { let scope = scopes.join(" "); - let params = url::form_urlencoded::Serializer::new(String::new()) + let mut serializer = url::form_urlencoded::Serializer::new(String::new()); + serializer .append_pair("response_type", "code") .append_pair("client_id", client_id) .append_pair("redirect_uri", redirect_uri) .append_pair("state", state) .append_pair("scope", &scope) .append_pair("code_challenge", &challenge.challenge) - .append_pair("code_challenge_method", &challenge.method) - .finish(); + .append_pair("code_challenge_method", &challenge.method); + if let Some(uuid) = org_uuid.filter(|u| !u.is_empty()) { + serializer.append_pair("dd_oid", uuid); + } + let params = serializer.finish(); // Use custom subdomain for SAML/SSO auth, otherwise use standard app.{site}. // The subdomain replaces the `app` prefix on whichever site is in play, so a @@ -248,6 +258,7 @@ mod tests { &challenge(), &["dashboards_read"], None, + None, ); assert!( url.starts_with("https://app.datadoghq.com/oauth2/v1/authorize?"), @@ -268,6 +279,7 @@ mod tests { &challenge(), &["dashboards_read"], Some("dd"), + None, ); assert!( url.starts_with("https://dd.datad0g.com/oauth2/v1/authorize?"), @@ -289,6 +301,7 @@ mod tests { &challenge(), &["dashboards_read"], Some("acme"), + None, ); assert!( url.starts_with("https://acme.datadoghq.eu/oauth2/v1/authorize?"), @@ -310,6 +323,7 @@ mod tests { &challenge(), &["dashboards_read"], Some(""), + None, ); assert!( url.starts_with("https://app.datadoghq.com/oauth2/v1/authorize?"), @@ -327,6 +341,7 @@ mod tests { &challenge(), &["dashboards_read", "metrics_read"], None, + None, ); assert!(url.contains("response_type=code")); assert!(url.contains("client_id=client123")); @@ -336,4 +351,54 @@ mod tests { // Scopes are joined with a space, then URL-encoded as `+` or `%20`. assert!(url.contains("scope=dashboards_read") && url.contains("metrics_read")); } + + #[test] + fn build_authorization_url_appends_dd_oid_when_org_uuid_set() { + let client = DcrClient::new("datadoghq.com"); + let url = client.build_authorization_url( + "client123", + "http://127.0.0.1:8000/oauth/callback", + "state", + &challenge(), + &["dashboards_read"], + None, + Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091"), + ); + assert!( + url.contains("dd_oid=8dee7c38-00cb-11ea-a77b-8b5a08d3b091"), + "expected dd_oid query param, got: {url}" + ); + } + + #[test] + fn build_authorization_url_omits_dd_oid_when_unset() { + let client = DcrClient::new("datadoghq.com"); + let url = client.build_authorization_url( + "client123", + "http://127.0.0.1:8000/oauth/callback", + "state", + &challenge(), + &["dashboards_read"], + None, + None, + ); + assert!(!url.contains("dd_oid"), "got: {url}"); + } + + #[test] + fn build_authorization_url_treats_empty_org_uuid_as_unset() { + // Symmetric with the empty-subdomain handling: an empty UUID flag + // value falls back to "no hint" rather than emitting `dd_oid=`. + let client = DcrClient::new("datadoghq.com"); + let url = client.build_authorization_url( + "client123", + "http://127.0.0.1:8000/oauth/callback", + "state", + &challenge(), + &["dashboards_read"], + None, + Some(""), + ); + assert!(!url.contains("dd_oid"), "got: {url}"); + } } diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 2f1f2455..4d0036f1 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -20,12 +20,21 @@ pub async fn login( scopes: Vec, subdomain: Option<&str>, callback_port: Option, + org_uuid: Option<&str>, ) -> Result<()> { use crate::auth::{dcr, pkce}; let site = &cfg.site; let org = cfg.org.as_deref(); + // Resolve effective org_uuid: CLI flag wins; otherwise recall from any + // saved session so re-auth keeps emitting `dd_oid` without re-passing the + // flag. + let stored_session = storage::find_session(site, org); + let effective_org_uuid: Option = org_uuid + .map(String::from) + .or_else(|| stored_session.as_ref().and_then(|s| s.org_uuid.clone())); + // 1. Start callback server. `callback_port` (from --callback-port or // PUP_OAUTH_CALLBACK_PORT) pins the port deterministically for SSH // port-forwarded workflows; otherwise we scan the DCR allowlist. @@ -92,7 +101,11 @@ pub async fn login( &challenge, &scope_strs, subdomain, + effective_org_uuid.as_deref(), ); + if let Some(uuid) = effective_org_uuid.as_deref() { + eprintln!("šŸŽÆ Hinting org UUID (dd_oid): {uuid}"); + } // 5. Open browser eprintln!("\n🌐 Opening browser for authentication..."); @@ -150,8 +163,16 @@ pub async fn login( Ok(store.storage_location()) })?; - // Register this session in the session registry - storage::save_session(site, org, None)?; + // Register this session in the session registry, tagged with the + // authoritative `dd_oid` from the callback so re-auth without the flag + // keeps hinting the right org. The callback's UUID is preferred over the + // CLI/stored value because it reflects the org the user actually + // consented for. + let saved_org_uuid = result + .dd_oid + .as_deref() + .or(effective_org_uuid.as_deref()); + storage::save_session(site, org, saved_org_uuid)?; let expires_at = chrono::DateTime::from_timestamp(tokens.issued_at + tokens.expires_in, 0) .map(|dt| dt.with_timezone(&chrono::Local).to_rfc3339()) @@ -170,6 +191,7 @@ pub async fn login( _scopes: Vec, _subdomain: Option<&str>, _callback_port: Option, + _org_uuid: Option<&str>, ) -> Result<()> { bail!( "OAuth login is not available in WASM builds.\n\ diff --git a/src/main.rs b/src/main.rs index 2eb191ae..9bd4fbca 100644 --- a/src/main.rs +++ b/src/main.rs @@ -9239,6 +9239,13 @@ enum AuthActions { /// PUP_OAUTH_CALLBACK_PORT if unset. #[arg(long, value_name = "PORT")] callback_port: Option, + /// Hint the target org by UUID (sent as `dd_oid`). Skips the org switcher when the + /// existing browser session already matches and pre-routes SAML/SSO routing for + /// first-time logins. The value is persisted with the session so subsequent + /// `pup auth login` invocations re-emit it automatically. The server validates + /// the UUID; malformed values are rejected there. + #[arg(long, value_name = "UUID")] + org_uuid: Option, }, /// Logout and clear tokens Logout, @@ -13991,6 +13998,7 @@ async fn main_inner() -> anyhow::Result<()> { site, subdomain, callback_port, + org_uuid, } => { if let Some(s) = site { cfg.set_site_explicit(s); @@ -14002,7 +14010,14 @@ async fn main_inner() -> anyhow::Result<()> { let resolved = resolve_login_scopes(scopes.as_deref(), cfg.org.as_deref(), is_read_only); let resolved_port = resolve_callback_port(callback_port)?; - commands::auth::login(&cfg, resolved, subdomain.as_deref(), resolved_port).await? + commands::auth::login( + &cfg, + resolved, + subdomain.as_deref(), + resolved_port, + org_uuid.as_deref(), + ) + .await? } AuthActions::Logout => commands::auth::logout(&cfg).await?, AuthActions::Status { site } => { From beeb9d455149635b687bfbc3194d83d8714b36f4 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Thu, 7 May 2026 16:21:03 -0700 Subject: [PATCH 03/11] feat(auth): warn and save under actual org on dd_oid mismatch When the user hints a UUID via --org-uuid (or via a stored session) but the OAuth server returns a different `dd_oid` (the user picked a different org in the switcher, or hit an SSO routing override), saving the resulting token under the user-supplied --org label would mislabel it. Refusing to save would also throw away the user's click since authorization codes are single-use. The chosen middle ground: warn on stderr and switch the save target to the actual org's name (`dd_org_name` from the callback, falling back to the first 8 chars of the UUID). The user-supplied --org entry is left alone, so a future re-auth against the requested org still gets a clean slot. The selection logic lives in a pure helper (`resolve_save_target`) so the unit tests can pin the four cases (no hint, match, mismatch with name, mismatch without name) without touching env state or storage. --- src/auth/callback.rs | 1 - src/auth/dcr.rs | 1 + src/auth/storage.rs | 1 - src/commands/auth.rs | 140 ++++++++++++++++++++++++++++++++++++++++--- src/config.rs | 6 +- 5 files changed, 138 insertions(+), 11 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index c88eba19..9237fe27 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -22,7 +22,6 @@ pub struct CallbackResult { /// Display name of the org the user actually authenticated against, /// returned alongside `dd_oid` as `dd_org_name`. Used as the saved-token /// label when the user-supplied org/UUID didn't match the actual one. - #[allow(dead_code)] pub dd_org_name: Option, } diff --git a/src/auth/dcr.rs b/src/auth/dcr.rs index 2c9ba715..c7d92cbc 100644 --- a/src/auth/dcr.rs +++ b/src/auth/dcr.rs @@ -195,6 +195,7 @@ impl DcrClient { /// authorize endpoint uses it to skip the org switcher when the existing /// browser session already matches, and to pre-route SAML/SSO routing /// for first-time logins. Empty strings are coerced to `None`. + #[allow(clippy::too_many_arguments)] pub fn build_authorization_url( &self, client_id: &str, diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 81df5895..2cce626e 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -908,7 +908,6 @@ pub fn remove_session(site: &str, org: Option<&str>) -> Result<()> { /// Look up a single session entry by `(site, org)`. #[cfg(not(target_arch = "wasm32"))] -#[allow(dead_code)] pub fn find_session(site: &str, org: Option<&str>) -> Option { list_sessions() .ok()? diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 4d0036f1..564da740 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -158,8 +158,27 @@ pub async fn login( .exchange_code(&result.code, &redirect_uri, &challenge.verifier, &creds) .await?; + // 8. Resolve the save target. By default we save under the user-supplied + // (site, org) label. But if the user hinted a UUID and the callback + // returned a different `dd_oid`, it would be misleading to label the + // resulting token with the requested org name — so we switch the label + // to the one the OAuth server reports (`dd_org_name`, falling back to a + // shortened UUID) and warn on stderr. The OAuth code is single-use, so + // refusing to save would throw away the user's click; mislabeling is the + // bigger risk and this side-steps it. + let save_target = resolve_save_target( + org, + effective_org_uuid.as_deref(), + result.dd_oid.as_deref(), + result.dd_org_name.as_deref(), + ); + let saved_org = save_target.org.as_deref(); + let saved_org_label = saved_org + .map(|o| format!(" (org: {o})")) + .unwrap_or_default(); + let location = with_storage(|store| { - store.save_tokens(site, org, &tokens)?; + store.save_tokens(site, saved_org, &tokens)?; Ok(store.storage_location()) })?; @@ -168,23 +187,70 @@ pub async fn login( // keeps hinting the right org. The callback's UUID is preferred over the // CLI/stored value because it reflects the org the user actually // consented for. - let saved_org_uuid = result - .dd_oid - .as_deref() - .or(effective_org_uuid.as_deref()); - storage::save_session(site, org, saved_org_uuid)?; + let saved_org_uuid = result.dd_oid.as_deref().or(effective_org_uuid.as_deref()); + storage::save_session(site, saved_org, saved_org_uuid)?; let expires_at = chrono::DateTime::from_timestamp(tokens.issued_at + tokens.expires_in, 0) .map(|dt| dt.with_timezone(&chrono::Local).to_rfc3339()) .unwrap_or_else(|| format!("in {} hours", tokens.expires_in / 3600)); - eprintln!("\nāœ… Login successful{org_label}!"); + eprintln!("\nāœ… Login successful{saved_org_label}!"); eprintln!(" Access token expires: {expires_at}"); eprintln!(" Token stored in: {location}"); Ok(()) } +#[cfg(not(target_arch = "wasm32"))] +struct SaveTarget { + org: Option, +} + +/// Pick the `(site, org)` save target for a finished login. +/// +/// The default is the user-supplied `--org` label. If the user hinted a UUID +/// but the OAuth server returned a different one, we switch the label to +/// `dd_org_name` (or a shortened UUID) and warn on stderr — saving under the +/// requested label would mislabel the token. Any other case (no hint, hint +/// matches, server didn't return `dd_oid`) keeps the requested label. +#[cfg(not(target_arch = "wasm32"))] +fn resolve_save_target( + requested_org: Option<&str>, + requested_uuid: Option<&str>, + actual_uuid: Option<&str>, + actual_org_name: Option<&str>, +) -> SaveTarget { + let mismatch = match (requested_uuid, actual_uuid) { + (Some(req), Some(actual)) => !req.eq_ignore_ascii_case(actual), + _ => false, + }; + if !mismatch { + return SaveTarget { + org: requested_org.map(String::from), + }; + } + // Mismatch: swap to the actual org. Prefer the human-readable + // `dd_org_name`; if that's absent, fall back to the UUID's first 8 chars + // so the saved label is still distinguishable on disk. + let actual_label = actual_org_name + .map(String::from) + .or_else(|| actual_uuid.map(|u| u.chars().take(8).collect::())); + eprintln!( + "āš ļø Requested org UUID {} but OAuth returned {}{}. \ + Saving token under {} instead of the requested label.", + requested_uuid.unwrap_or("?"), + actual_uuid.unwrap_or("?"), + actual_org_name + .map(|n| format!(" ({n})")) + .unwrap_or_default(), + actual_label + .as_deref() + .map(|l| format!("\"{l}\"")) + .unwrap_or_else(|| "the default session".to_string()), + ); + SaveTarget { org: actual_label } +} + #[cfg(target_arch = "wasm32")] pub async fn login( _cfg: &Config, @@ -613,6 +679,66 @@ mod tests { ); } + // ------------------------------------------------------------------ + // resolve_save_target — pure function, no env / storage state. + // ------------------------------------------------------------------ + + #[test] + fn resolve_save_target_no_hint_keeps_requested_org() { + let t = resolve_save_target(Some("prod-child"), None, None, None); + assert_eq!(t.org.as_deref(), Some("prod-child")); + } + + #[test] + fn resolve_save_target_hint_matches_keeps_requested_org() { + let uuid = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let t = resolve_save_target( + Some("prod-child"), + Some(uuid), + Some(uuid), + Some("Datadog HQ"), + ); + assert_eq!(t.org.as_deref(), Some("prod-child")); + } + + #[test] + fn resolve_save_target_hint_matches_case_insensitive() { + // UUIDs are hex; normalise case so a mismatch in the issuer's + // canonicalisation doesn't trip the warn-and-relabel path. + let upper = "8DEE7C38-00CB-11EA-A77B-8B5A08D3B091"; + let lower = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let t = resolve_save_target(Some("prod-child"), Some(upper), Some(lower), None); + assert_eq!(t.org.as_deref(), Some("prod-child")); + } + + #[test] + fn resolve_save_target_mismatch_uses_dd_org_name() { + let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let act = "11111111-2222-3333-4444-555555555555"; + let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), Some("Other Org")); + assert_eq!(t.org.as_deref(), Some("Other Org")); + } + + #[test] + fn resolve_save_target_mismatch_falls_back_to_uuid_prefix() { + // No dd_org_name in the callback (older issuer or unusual flow): use + // the first 8 chars of the actual UUID as a stable, distinguishable + // label rather than reusing the wrong --org name. + let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let act = "11111111-2222-3333-4444-555555555555"; + let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), None); + assert_eq!(t.org.as_deref(), Some("11111111")); + } + + #[test] + fn resolve_save_target_actual_uuid_missing_keeps_requested() { + // If the issuer didn't echo dd_oid, we have no comparison to make; + // trust the user's --org label and the in-flight UUID we sent. + let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let t = resolve_save_target(Some("prod-child"), Some(req), None, None); + assert_eq!(t.org.as_deref(), Some("prod-child")); + } + #[tokio::test] async fn test_logout_with_org_removes_session_entry() { // save a session, then logout should remove just that org's entry diff --git a/src/config.rs b/src/config.rs index 1f1c405c..09c5f45c 100644 --- a/src/config.rs +++ b/src/config.rs @@ -937,7 +937,8 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); + crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None) + .unwrap(); std::env::set_var("DD_ORG", "prod-child"); let cfg = Config::from_env().unwrap(); @@ -967,7 +968,8 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); + crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None) + .unwrap(); std::env::set_var("DD_ORG", "prod-child"); std::env::set_var("DD_SITE", "datadoghq.eu"); From 484ea08e245365504e9ed71ea84f0da67b8a8525 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 08:54:28 -0700 Subject: [PATCH 04/11] feat(auth): show org name + UUID on the success page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The local listener's success page used to say only "Authentication Successful" — handy as a "you can close this tab" signal but offered no way to confirm the user consented for the org they intended. With the dd_oid hint flow, the consented org can diverge silently (logged-out login redirect drops the hint, SSO override, switcher pick), so showing the org name + UUID alongside the success message lets users catch the divergence at the moment of consent rather than later. Tests stub `success_page` directly with synthetic inputs (hostile strings included to verify HTML escaping). Real UUIDs that snuck into unit tests during the prior commits are swapped for placeholder values. --- src/auth/callback.rs | 81 +++++++++++++++++++++++++++++++++++++++----- src/auth/dcr.rs | 4 +-- src/auth/storage.rs | 2 +- src/commands/auth.rs | 12 +++---- 4 files changed, 81 insertions(+), 18 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index 9237fe27..25b31011 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -176,7 +176,10 @@ async fn accept_loop( let (status, body) = if error.is_some() { ("400 Bad Request", error_page(&error, &error_description)) } else { - ("200 OK", success_page()) + ( + "200 OK", + success_page(dd_org_name.as_deref(), dd_oid.as_deref()), + ) }; let response = format!( "HTTP/1.1 {status}\r\nContent-Type: text/html\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", @@ -200,14 +203,44 @@ async fn accept_loop( } #[cfg(not(target_arch = "wasm32"))] -fn success_page() -> String { - r#" +fn success_page(org_name: Option<&str>, org_uuid: Option<&str>) -> String { + // The org name + UUID help users confirm they consented for the org they + // intended — important when the dd_oid hint silently routes through a + // different one (logged-out login redirect, SSO override, switcher pick). + let detail = match (org_name, org_uuid) { + (Some(name), Some(uuid)) => format!( + "

Authenticated as {} ({}).

", + html_escape(name), + html_escape(uuid), + ), + (Some(name), None) => format!( + "

Authenticated as {}.

", + html_escape(name), + ), + (None, Some(uuid)) => format!("

Org UUID: {}.

", html_escape(uuid)), + (None, None) => String::new(), + }; + format!( + r#" Pup - Authentication Successful - +

Authentication Successful

-

You can close this window and return to pup.

"#.to_string() +{detail}

You can close this window and return to pup.

"# + ) +} + +/// Minimal HTML-escape for the few attribute-free text contexts we render +/// values into — org names from the OAuth issuer aren't expected to contain +/// markup, but we don't want to take that on trust. +#[cfg(not(target_arch = "wasm32"))] +fn html_escape(s: &str) -> String { + s.replace('&', "&") + .replace('<', "<") + .replace('>', ">") + .replace('"', """) + .replace('\'', "'") } #[cfg(not(target_arch = "wasm32"))] @@ -319,13 +352,13 @@ mod tests { let r = parse_callback_url( "http://127.0.0.1:8000/oauth/callback\ ?code=abc&state=xyz\ - &dd_oid=8dee7c38-00cb-11ea-a77b-8b5a08d3b091\ + &dd_oid=00000000-1111-2222-3333-444444444444\ &dd_org_name=Datadog+HQ", ) .unwrap(); assert_eq!( r.dd_oid.as_deref(), - Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091") + Some("00000000-1111-2222-3333-444444444444") ); assert_eq!(r.dd_org_name.as_deref(), Some("Datadog HQ")); } @@ -498,6 +531,36 @@ mod tests { ); } + #[test] + fn success_page_renders_org_name_and_uuid() { + let html = success_page( + Some("Datadog HQ"), + Some("00000000-1111-2222-3333-444444444444"), + ); + assert!(html.contains("Authentication Successful")); + assert!(html.contains("Datadog HQ")); + assert!(html.contains("00000000-1111-2222-3333-444444444444")); + } + + #[test] + fn success_page_omits_detail_when_callback_lacks_metadata() { + // Older issuers (or unusual flows) may not emit dd_org_name/dd_oid; the + // page should still render cleanly without dangling labels. + let html = success_page(None, None); + assert!(html.contains("Authentication Successful")); + assert!(!html.contains("Authenticated as")); + assert!(!html.contains("Org UUID:")); + } + + #[test] + fn success_page_escapes_org_name() { + // Defense in depth: a hostile dd_org_name shouldn't be able to inject + // script tags into the success page rendered by the local listener. + let html = success_page(Some(""), None); + assert!(!html.contains("")); + assert!(html.contains("<script>alert(1)</script>")); + } + #[tokio::test] async fn read_callback_url_passes_through_oauth_error_redirect() { let r = read_callback_url_from_reader(reader( diff --git a/src/auth/dcr.rs b/src/auth/dcr.rs index c7d92cbc..16a75a34 100644 --- a/src/auth/dcr.rs +++ b/src/auth/dcr.rs @@ -363,10 +363,10 @@ mod tests { &challenge(), &["dashboards_read"], None, - Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091"), + Some("00000000-1111-2222-3333-444444444444"), ); assert!( - url.contains("dd_oid=8dee7c38-00cb-11ea-a77b-8b5a08d3b091"), + url.contains("dd_oid=00000000-1111-2222-3333-444444444444"), "expected dd_oid query param, got: {url}" ); } diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 2cce626e..83cee7fb 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -1275,7 +1275,7 @@ mod tests { let entry = SessionEntry { site: "datadoghq.com".into(), org: Some("prod-child".into()), - org_uuid: Some("8dee7c38-00cb-11ea-a77b-8b5a08d3b091".into()), + org_uuid: Some("00000000-1111-2222-3333-444444444444".into()), }; let json = serde_json::to_string(&entry).unwrap(); let parsed: SessionEntry = serde_json::from_str(&json).unwrap(); diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 564da740..48318700 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -691,7 +691,7 @@ mod tests { #[test] fn resolve_save_target_hint_matches_keeps_requested_org() { - let uuid = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let uuid = "00000000-1111-2222-3333-444444444444"; let t = resolve_save_target( Some("prod-child"), Some(uuid), @@ -705,15 +705,15 @@ mod tests { fn resolve_save_target_hint_matches_case_insensitive() { // UUIDs are hex; normalise case so a mismatch in the issuer's // canonicalisation doesn't trip the warn-and-relabel path. - let upper = "8DEE7C38-00CB-11EA-A77B-8B5A08D3B091"; - let lower = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let upper = "AABBCCDD-1111-2222-3333-EEFFAABBCCDD"; + let lower = "aabbccdd-1111-2222-3333-eeffaabbccdd"; let t = resolve_save_target(Some("prod-child"), Some(upper), Some(lower), None); assert_eq!(t.org.as_deref(), Some("prod-child")); } #[test] fn resolve_save_target_mismatch_uses_dd_org_name() { - let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), Some("Other Org")); assert_eq!(t.org.as_deref(), Some("Other Org")); @@ -724,7 +724,7 @@ mod tests { // No dd_org_name in the callback (older issuer or unusual flow): use // the first 8 chars of the actual UUID as a stable, distinguishable // label rather than reusing the wrong --org name. - let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), None); assert_eq!(t.org.as_deref(), Some("11111111")); @@ -734,7 +734,7 @@ mod tests { fn resolve_save_target_actual_uuid_missing_keeps_requested() { // If the issuer didn't echo dd_oid, we have no comparison to make; // trust the user's --org label and the in-flight UUID we sent. - let req = "8dee7c38-00cb-11ea-a77b-8b5a08d3b091"; + let req = "00000000-1111-2222-3333-444444444444"; let t = resolve_save_target(Some("prod-child"), Some(req), None, None); assert_eq!(t.org.as_deref(), Some("prod-child")); } From f70e7884700c04cdd291458484f787e62f6631b6 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 09:22:41 -0700 Subject: [PATCH 05/11] feat(auth): refine success page wording, link, and UUID treatment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Iterating on the success page based on live preview feedback: - Lead line is now "Connected to Datadog (datadoghq.com)" — names the OAuth provider explicitly and links the site URL extracted from the callback's `site=` param. Avoids the dual "Authenticated"/"Authenticated to" repetition the prior copy had. - Org line reads "in org " instead of "as " — the org-as- context framing parses better than person-as-org. - UUID is now always-visible fine-print on its own line, wrapped in parens so it reads as supplementary annotation. Fine-print styling (small, gray, monospaced) demotes it visually but keeps it selectable and copy-pasteable. The earlier
/disclosure variant felt cluttered; hover-tooltip alternatives weren't copyable. - Defensive: the link href is only emitted when `site` is an https URL, and the displayed text uses the bare `domain` to avoid surfacing a full URL into the rendered text. Test fixtures use Star Wars placeholders (Mos Eisley Cantina, tatooine.example, Rebel Alliance, deathstar.tatooine.example) so no real Datadog org names or hosts leak into the codebase. --- src/auth/callback.rs | 148 +++++++++++++++++++++++++++++++++++-------- 1 file changed, 120 insertions(+), 28 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index 25b31011..55dc8b83 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -172,13 +172,24 @@ async fn accept_loop( let error_description = params.get("error_description").cloned(); let dd_oid = params.get("dd_oid").cloned(); let dd_org_name = params.get("dd_org_name").cloned(); + // `site` (full URL) and `domain` (bare host) are emitted by Datadog + // alongside the OAuth response so the success page can show users + // exactly which Datadog site they consented for. Both are optional — + // older issuers may omit them. + let site = params.get("site").cloned(); + let domain = params.get("domain").cloned(); let (status, body) = if error.is_some() { ("400 Bad Request", error_page(&error, &error_description)) } else { ( "200 OK", - success_page(dd_org_name.as_deref(), dd_oid.as_deref()), + success_page( + site.as_deref(), + domain.as_deref(), + dd_org_name.as_deref(), + dd_oid.as_deref(), + ), ) }; let response = format!( @@ -203,31 +214,71 @@ async fn accept_loop( } #[cfg(not(target_arch = "wasm32"))] -fn success_page(org_name: Option<&str>, org_uuid: Option<&str>) -> String { - // The org name + UUID help users confirm they consented for the org they - // intended — important when the dd_oid hint silently routes through a - // different one (logged-out login redirect, SSO override, switcher pick). - let detail = match (org_name, org_uuid) { - (Some(name), Some(uuid)) => format!( - "

Authenticated as {} ({}).

", - html_escape(name), - html_escape(uuid), +fn success_page( + site: Option<&str>, + domain: Option<&str>, + org_name: Option<&str>, + org_uuid: Option<&str>, +) -> String { + // Show users exactly what they just consented for. The dd_oid hint can + // silently route to a different org than the user expected (logged-out + // login redirect, SSO override, switcher pick), and naming the OAuth + // provider explicitly ("Datadog") helps users with multiple OAuth flows + // open in their browser keep them straight. + // + // The user's email is intentionally not shown here — pup's success page + // is rendered before token exchange completes, so we don't yet have a + // bearer token to call userinfo. Surfacing identity is a follow-up, + // either by reading userinfo after exchange or by emitting it in pup's + // terminal output instead of the browser page. + + // Only accept a same-origin https site URL as the link href — + // defense-in-depth against an unexpected scheme finding its way into the + // callback query string. The displayed text comes from `domain` (bare + // host) when available, so a missing or rejected `site` URL still + // renders "Datadog (datadoghq.com)" without an opaque link. + let provider_html = match (site.filter(|s| s.starts_with("https://")), domain) { + (Some(href), Some(host)) => format!( + r#"Datadog ({})"#, + html_escape(href), + html_escape(host), ), - (Some(name), None) => format!( - "

Authenticated as {}.

", - html_escape(name), - ), - (None, Some(uuid)) => format!("

Org UUID: {}.

", html_escape(uuid)), - (None, None) => String::new(), + (Some(href), None) => format!(r#"{0}"#, html_escape(href)), + (None, Some(host)) => format!("Datadog ({})", html_escape(host)), + (None, None) => "Datadog".to_string(), }; + + let org_line = org_name + .map(|n| { + format!( + r#"

in org {}

"#, + html_escape(n) + ) + }) + .unwrap_or_default(); + // Always render the UUID as fine-print mono text — visually demoted so it + // doesn't compete with the org name, but selectable and copy-pasteable + // when an operator wants to verify or store it. Parens nudge it further + // into "annotation" territory so it reads as supplementary info. + let uuid_line = org_uuid + .map(|u| format!(r#"

({})

"#, html_escape(u))) + .unwrap_or_default(); + format!( r#" Pup - Authentication Successful - +

Authentication Successful

-{detail}

You can close this window and return to pup.

"# +

Connected to {provider_html}

+{org_line}{uuid_line}

You can close this window and return to pup.

"# ) } @@ -534,33 +585,74 @@ mod tests { #[test] fn success_page_renders_org_name_and_uuid() { let html = success_page( - Some("Datadog HQ"), + Some("https://app.tatooine.example"), + Some("tatooine.example"), + Some("Mos Eisley Cantina"), Some("00000000-1111-2222-3333-444444444444"), ); assert!(html.contains("Authentication Successful")); - assert!(html.contains("Datadog HQ")); + assert!(html.contains("Connected to Datadog")); + assert!(html.contains(r#"href="https://app.tatooine.example""#)); + assert!(html.contains("Mos Eisley Cantina")); + assert!(html.contains("in org")); + // UUID is rendered inline as fine-print so operators can read or copy + // it without any interaction. + assert!(html.contains(r#"

no 'in org' line"); + assert!( + !html.contains(r#"

no fine-print line" + ); } #[test] fn success_page_escapes_org_name() { // Defense in depth: a hostile dd_org_name shouldn't be able to inject // script tags into the success page rendered by the local listener. - let html = success_page(Some(""), None); + let html = success_page(None, None, Some(""), None); assert!(!html.contains("")); assert!(html.contains("<script>alert(1)</script>")); } + #[test] + fn success_page_rejects_non_https_site_as_link() { + // An unexpected `site` value (anything other than https://...) must not + // become a clickable link. The bare host falls back to plain text. + let html = success_page( + Some("javascript:alert(1)"), + Some("deathstar.tatooine.example"), + None, + None, + ); + assert!(!html.contains("javascript:alert(1)")); + assert!(!html.contains("href=\"javascript")); + assert!(html.contains("deathstar.tatooine.example")); + } + + #[test] + fn success_page_uses_vanity_subdomain_in_link() { + // SAML/SSO vanity subdomains route through their own host; the link + // should reflect whatever site the issuer reported, not always app.. + let html = success_page( + Some("https://yavin4.tatooine.example"), + Some("tatooine.example"), + Some("Rebel Alliance"), + None, + ); + assert!(html.contains(r#"href="https://yavin4.tatooine.example""#)); + assert!(html.contains("Rebel Alliance")); + } + #[tokio::test] async fn read_callback_url_passes_through_oauth_error_redirect() { let r = read_callback_url_from_reader(reader( From 414b334b294ee050c92ab6ae794942e7a3454045 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 09:36:00 -0700 Subject: [PATCH 06/11] refactor(auth): simplifications from /simplify review - Extract `org_suffix(Option<&str>) -> String` helper to replace the `org.map(|o| format!(" (org: {o})")).unwrap_or_default()` boilerplate that was duplicated across `login`, `logout`, `status`, `refresh`, and the new mismatch path. - Replace the single-field `SaveTarget { org }` struct with an enum carrying both branches and pull the eprintln! out of the helper into the caller, so `resolve_save_target` becomes genuinely pure (matching the docstring) and the warning text stays testable separately if needed. - `save_session(&SessionEntry)` instead of three loose Option-shaped args: a slip between `org` and `org_uuid` would have been silent before; the struct field names make the call sites clearer. - Move the empty-`--org-uuid`-string coercion from `dcr::build_authorization_url` up to `main.rs` where the raw clap value lives. Drops the `.filter(|u| !u.is_empty())` from the URL-builder API and the now-redundant dcr test. - Trim narration comments that restated what the code did or what was changing in this PR. Kept genuine WHY comments (security defense, OAuth-code-single-use rationale). --- src/auth/callback.rs | 41 ++------- src/auth/dcr.rs | 28 +----- src/auth/storage.rs | 97 +++++++++++++++----- src/commands/auth.rs | 208 ++++++++++++++++++++++++++++--------------- src/config.rs | 38 ++++++-- src/main.rs | 5 +- 6 files changed, 256 insertions(+), 161 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index 55dc8b83..73bb31f6 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -15,13 +15,9 @@ pub struct CallbackResult { pub state: String, pub error: Option, pub error_description: Option, - /// Authoritative org UUID returned by the OAuth server. Present when the - /// authorize response carries `dd_oid` — true for Datadog OAuth today on - /// both the org-switcher and direct-consent paths. + /// Authoritative org UUID from the OAuth server (`dd_oid`). pub dd_oid: Option, - /// Display name of the org the user actually authenticated against, - /// returned alongside `dd_oid` as `dd_org_name`. Used as the saved-token - /// label when the user-supplied org/UUID didn't match the actual one. + /// Display name of the consented org (`dd_org_name`). pub dd_org_name: Option, } @@ -172,10 +168,8 @@ async fn accept_loop( let error_description = params.get("error_description").cloned(); let dd_oid = params.get("dd_oid").cloned(); let dd_org_name = params.get("dd_org_name").cloned(); - // `site` (full URL) and `domain` (bare host) are emitted by Datadog - // alongside the OAuth response so the success page can show users - // exactly which Datadog site they consented for. Both are optional — - // older issuers may omit them. + // `site` (full URL) and `domain` (bare host) are sent alongside the + // OAuth response so the success page can render a link back. let site = params.get("site").cloned(); let domain = params.get("domain").cloned(); @@ -220,23 +214,9 @@ fn success_page( org_name: Option<&str>, org_uuid: Option<&str>, ) -> String { - // Show users exactly what they just consented for. The dd_oid hint can - // silently route to a different org than the user expected (logged-out - // login redirect, SSO override, switcher pick), and naming the OAuth - // provider explicitly ("Datadog") helps users with multiple OAuth flows - // open in their browser keep them straight. - // - // The user's email is intentionally not shown here — pup's success page - // is rendered before token exchange completes, so we don't yet have a - // bearer token to call userinfo. Surfacing identity is a follow-up, - // either by reading userinfo after exchange or by emitting it in pup's - // terminal output instead of the browser page. - - // Only accept a same-origin https site URL as the link href — - // defense-in-depth against an unexpected scheme finding its way into the - // callback query string. The displayed text comes from `domain` (bare - // host) when available, so a missing or rejected `site` URL still - // renders "Datadog (datadoghq.com)" without an opaque link. + // Only accept https `site` as the link href; anything else falls back to + // plain text so a malformed or hostile callback can't inject a different + // scheme into the rendered HTML. let provider_html = match (site.filter(|s| s.starts_with("https://")), domain) { (Some(href), Some(host)) => format!( r#"Datadog ({})"#, @@ -256,10 +236,6 @@ fn success_page( ) }) .unwrap_or_default(); - // Always render the UUID as fine-print mono text — visually demoted so it - // doesn't compete with the org name, but selectable and copy-pasteable - // when an operator wants to verify or store it. Parens nudge it further - // into "annotation" territory so it reads as supplementary info. let uuid_line = org_uuid .map(|u| format!(r#"

({})

"#, html_escape(u))) .unwrap_or_default(); @@ -282,9 +258,6 @@ a{{color:#632ca6}} ) } -/// Minimal HTML-escape for the few attribute-free text contexts we render -/// values into — org names from the OAuth issuer aren't expected to contain -/// markup, but we don't want to take that on trust. #[cfg(not(target_arch = "wasm32"))] fn html_escape(s: &str) -> String { s.replace('&', "&") diff --git a/src/auth/dcr.rs b/src/auth/dcr.rs index 16a75a34..088fed55 100644 --- a/src/auth/dcr.rs +++ b/src/auth/dcr.rs @@ -189,12 +189,9 @@ impl DcrClient { }) } - /// Build the authorization URL for the browser. - /// - /// `org_uuid` (when set) is sent as the `dd_oid` query param. The - /// authorize endpoint uses it to skip the org switcher when the existing - /// browser session already matches, and to pre-route SAML/SSO routing - /// for first-time logins. Empty strings are coerced to `None`. + /// Build the authorization URL for the browser. `org_uuid` is appended as + /// `dd_oid` when set; callers should coerce empty strings to `None` + /// upstream so this function doesn't have to second-guess them. #[allow(clippy::too_many_arguments)] pub fn build_authorization_url( &self, @@ -216,7 +213,7 @@ impl DcrClient { .append_pair("scope", &scope) .append_pair("code_challenge", &challenge.challenge) .append_pair("code_challenge_method", &challenge.method); - if let Some(uuid) = org_uuid.filter(|u| !u.is_empty()) { + if let Some(uuid) = org_uuid { serializer.append_pair("dd_oid", uuid); } let params = serializer.finish(); @@ -385,21 +382,4 @@ mod tests { ); assert!(!url.contains("dd_oid"), "got: {url}"); } - - #[test] - fn build_authorization_url_treats_empty_org_uuid_as_unset() { - // Symmetric with the empty-subdomain handling: an empty UUID flag - // value falls back to "no hint" rather than emitting `dd_oid=`. - let client = DcrClient::new("datadoghq.com"); - let url = client.build_authorization_url( - "client123", - "http://127.0.0.1:8000/oauth/callback", - "state", - &challenge(), - &["dashboards_read"], - None, - Some(""), - ); - assert!(!url.contains("dd_oid"), "got: {url}"); - } } diff --git a/src/auth/storage.rs b/src/auth/storage.rs index 83cee7fb..25a6bda8 100644 --- a/src/auth/storage.rs +++ b/src/auth/storage.rs @@ -883,18 +883,13 @@ pub fn list_sessions() -> Result> { } } -/// Upsert a session entry into the registry. +/// Upsert a session entry into the registry. Dedups on `(site, org)`; the +/// new entry's `org_uuid` wins so re-auth refreshes the stored UUID. #[cfg(not(target_arch = "wasm32"))] -pub fn save_session(site: &str, org: Option<&str>, org_uuid: Option<&str>) -> Result<()> { +pub fn save_session(entry: &SessionEntry) -> Result<()> { let mut sessions = list_sessions()?; - let entry = SessionEntry { - site: site.to_string(), - org: org.map(String::from), - org_uuid: org_uuid.map(String::from), - }; - // Dedup: remove any existing entry with same site+org, then append sessions.retain(|s| !(s.site == entry.site && s.org == entry.org)); - sessions.push(entry); + sessions.push(entry.clone()); write_sessions(&sessions) } @@ -1313,8 +1308,18 @@ mod tests { let tmp = TempDir::new("sess_save"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", None, None).unwrap(); - save_session("datadoghq.com", Some("prod-child"), None).unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }) + .unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1333,8 +1338,18 @@ mod tests { let tmp = TempDir::new("sess_dedup"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", Some("prod"), None).unwrap(); - save_session("datadoghq.com", Some("prod"), None).unwrap(); // duplicate + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod".into()), + org_uuid: None, + }) + .unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod".into()), + org_uuid: None, + }) + .unwrap(); // duplicate let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1347,8 +1362,18 @@ mod tests { let tmp = TempDir::new("sess_remove"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", None, None).unwrap(); - save_session("datadoghq.com", Some("prod"), None).unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }) + .unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod".into()), + org_uuid: None, + }) + .unwrap(); remove_session("datadoghq.com", Some("prod")).unwrap(); let sessions = list_sessions().unwrap(); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1373,8 +1398,18 @@ mod tests { let tmp = TempDir::new("find_sess_unique"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("custom.datadoghq.com", Some("prod-child"), None).unwrap(); - save_session("datadoghq.com", None, None).unwrap(); + save_session(&SessionEntry { + site: "custom.datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }) + .unwrap(); let result = find_session_site("prod-child"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1387,7 +1422,12 @@ mod tests { let tmp = TempDir::new("find_sess_none"); std::env::set_var("PUP_CONFIG_DIR", tmp.path()); - save_session("datadoghq.com", Some("prod-child"), None).unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); let result = find_session_site("nonexistent"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1402,8 +1442,18 @@ mod tests { // Same org name registered against two different sites → caller must // disambiguate via DD_SITE rather than us picking one. - save_session("datadoghq.com", Some("shared-name"), None).unwrap(); - save_session("datadoghq.eu", Some("shared-name"), None).unwrap(); + save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("shared-name".into()), + org_uuid: None, + }) + .unwrap(); + save_session(&SessionEntry { + site: "datadoghq.eu".into(), + org: Some("shared-name".into()), + org_uuid: None, + }) + .unwrap(); let result = find_session_site("shared-name"); std::env::remove_var("PUP_CONFIG_DIR"); @@ -1417,7 +1467,12 @@ mod tests { std::env::set_var("PUP_CONFIG_DIR", tmp.path()); // The unnamed (org=None) session must not match any --org lookup. - save_session("datadoghq.eu", None, None).unwrap(); + save_session(&SessionEntry { + site: "datadoghq.eu".into(), + org: None, + org_uuid: None, + }) + .unwrap(); let result = find_session_site("anything"); std::env::remove_var("PUP_CONFIG_DIR"); diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 48318700..6a972486 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -14,6 +14,13 @@ where f(&mut **store) } +/// Format the trailing " (org: )" segment used in user-facing log lines. +/// Returns an empty string when there's no org so the surrounding messages stay +/// clean for the default-org case. +fn org_suffix(org: Option<&str>) -> String { + org.map(|o| format!(" (org: {o})")).unwrap_or_default() +} + #[cfg(not(target_arch = "wasm32"))] pub async fn login( cfg: &Config, @@ -40,7 +47,7 @@ pub async fn login( // port-forwarded workflows; otherwise we scan the DCR allowlist. let mut server = crate::auth::callback::CallbackServer::new(callback_port).await?; let redirect_uri = server.redirect_uri(); - let org_label = org.map(|o| format!(" (org: {o})")).unwrap_or_default(); + let org_label = org_suffix(org); eprintln!("\nšŸ” Starting OAuth2 login for site: {site}{org_label}\n"); if let Some(sub) = subdomain { // Compose against the actual site, not a hardcoded prod host. Mirrors @@ -158,37 +165,58 @@ pub async fn login( .exchange_code(&result.code, &redirect_uri, &challenge.verifier, &creds) .await?; - // 8. Resolve the save target. By default we save under the user-supplied - // (site, org) label. But if the user hinted a UUID and the callback - // returned a different `dd_oid`, it would be misleading to label the - // resulting token with the requested org name — so we switch the label - // to the one the OAuth server reports (`dd_org_name`, falling back to a - // shortened UUID) and warn on stderr. The OAuth code is single-use, so - // refusing to save would throw away the user's click; mislabeling is the - // bigger risk and this side-steps it. - let save_target = resolve_save_target( + // 8. Resolve the save target — relabel under the actual returned org on + // dd_oid mismatch. The OAuth code is single-use, so refusing to save + // would throw away the user's click. + let saved_org_owned: Option = match resolve_save_target( org, effective_org_uuid.as_deref(), result.dd_oid.as_deref(), result.dd_org_name.as_deref(), - ); - let saved_org = save_target.org.as_deref(); - let saved_org_label = saved_org - .map(|o| format!(" (org: {o})")) - .unwrap_or_default(); + ) { + SaveTarget::Requested(label) => label, + SaveTarget::Reassigned { + label, + requested_uuid, + actual_uuid, + actual_name, + } => { + let actual_name_suffix = actual_name + .as_deref() + .map(|n| format!(" ({n})")) + .unwrap_or_default(); + let label_display = label + .as_deref() + .map(|l| format!("\"{l}\"")) + .unwrap_or_else(|| "the default session".to_string()); + eprintln!( + "āš ļø Requested org UUID {requested_uuid} but OAuth returned \ + {actual_uuid}{actual_name_suffix}. Saving token under \ + {label_display} instead of the requested label." + ); + label + } + }; + let saved_org = saved_org_owned.as_deref(); + let saved_org_label = org_suffix(saved_org); let location = with_storage(|store| { store.save_tokens(site, saved_org, &tokens)?; Ok(store.storage_location()) })?; - // Register this session in the session registry, tagged with the - // authoritative `dd_oid` from the callback so re-auth without the flag - // keeps hinting the right org. The callback's UUID is preferred over the - // CLI/stored value because it reflects the org the user actually - // consented for. - let saved_org_uuid = result.dd_oid.as_deref().or(effective_org_uuid.as_deref()); - storage::save_session(site, saved_org, saved_org_uuid)?; + // Prefer the callback's `dd_oid` over the CLI/stored hint — it reflects + // the org the user actually consented for. + let saved_org_uuid = result + .dd_oid + .as_deref() + .or(effective_org_uuid.as_deref()) + .map(String::from); + storage::save_session(&storage::SessionEntry { + site: site.clone(), + org: saved_org.map(String::from), + org_uuid: saved_org_uuid, + })?; let expires_at = chrono::DateTime::from_timestamp(tokens.issued_at + tokens.expires_in, 0) .map(|dt| dt.with_timezone(&chrono::Local).to_rfc3339()) @@ -201,18 +229,26 @@ pub async fn login( Ok(()) } +/// What `resolve_save_target` decided about where to save the new token. #[cfg(not(target_arch = "wasm32"))] -struct SaveTarget { - org: Option, +#[derive(Debug)] +enum SaveTarget { + /// dd_oid hint matched (or wasn't sent) — save under the requested label. + Requested(Option), + /// dd_oid mismatch — save under `label` and warn the user. Carries the + /// requested vs actual UUIDs so the caller can render a useful warning. + Reassigned { + label: Option, + requested_uuid: String, + actual_uuid: String, + actual_name: Option, + }, } -/// Pick the `(site, org)` save target for a finished login. -/// -/// The default is the user-supplied `--org` label. If the user hinted a UUID -/// but the OAuth server returned a different one, we switch the label to -/// `dd_org_name` (or a shortened UUID) and warn on stderr — saving under the -/// requested label would mislabel the token. Any other case (no hint, hint -/// matches, server didn't return `dd_oid`) keeps the requested label. +/// Pure: pick the `(site, org)` save target for a finished login. Mismatch +/// detection compares hinted vs callback UUIDs case-insensitively (UUIDs are +/// hex). The mismatch fallback prefers `dd_org_name`, then a UUID prefix, so +/// the saved label is always distinguishable from the requested one. #[cfg(not(target_arch = "wasm32"))] fn resolve_save_target( requested_org: Option<&str>, @@ -220,35 +256,17 @@ fn resolve_save_target( actual_uuid: Option<&str>, actual_org_name: Option<&str>, ) -> SaveTarget { - let mismatch = match (requested_uuid, actual_uuid) { - (Some(req), Some(actual)) => !req.eq_ignore_ascii_case(actual), - _ => false, - }; - if !mismatch { - return SaveTarget { - org: requested_org.map(String::from), - }; + match (requested_uuid, actual_uuid) { + (Some(req), Some(actual)) if !req.eq_ignore_ascii_case(actual) => SaveTarget::Reassigned { + label: actual_org_name + .map(String::from) + .or_else(|| Some(actual.chars().take(8).collect())), + requested_uuid: req.to_string(), + actual_uuid: actual.to_string(), + actual_name: actual_org_name.map(String::from), + }, + _ => SaveTarget::Requested(requested_org.map(String::from)), } - // Mismatch: swap to the actual org. Prefer the human-readable - // `dd_org_name`; if that's absent, fall back to the UUID's first 8 chars - // so the saved label is still distinguishable on disk. - let actual_label = actual_org_name - .map(String::from) - .or_else(|| actual_uuid.map(|u| u.chars().take(8).collect::())); - eprintln!( - "āš ļø Requested org UUID {} but OAuth returned {}{}. \ - Saving token under {} instead of the requested label.", - requested_uuid.unwrap_or("?"), - actual_uuid.unwrap_or("?"), - actual_org_name - .map(|n| format!(" ({n})")) - .unwrap_or_default(), - actual_label - .as_deref() - .map(|l| format!("\"{l}\"")) - .unwrap_or_else(|| "the default session".to_string()), - ); - SaveTarget { org: actual_label } } #[cfg(target_arch = "wasm32")] @@ -280,7 +298,7 @@ pub async fn logout(cfg: &Config) -> Result<()> { Ok(()) })?; storage::remove_session(site, org)?; - let org_label = org.map(|o| format!(" (org: {o})")).unwrap_or_default(); + let org_label = org_suffix(org); eprintln!("Logged out from {site}{org_label}. Tokens removed."); Ok(()) } @@ -313,7 +331,7 @@ pub fn status(cfg: &Config) -> Result<()> { ("valid".to_string(), format!("{mins}m{secs}s")) }; - let org_label = org.map(|o| format!(" (org: {o})")).unwrap_or_default(); + let org_label = org_suffix(org); if tokens.is_expired() { eprintln!("āš ļø Token expired for site: {site}{org_label}"); } else { @@ -344,7 +362,7 @@ pub fn status(cfg: &Config) -> Result<()> { println!("{}", serde_json::to_string_pretty(&json).unwrap()); } None => { - let org_label = org.map(|o| format!(" (org: {o})")).unwrap_or_default(); + let org_label = org_suffix(org); eprintln!("āŒ Not authenticated for site: {site}{org_label}"); let json = serde_json::json!({ "authenticated": false, @@ -399,7 +417,7 @@ pub async fn refresh(cfg: &Config) -> Result<()> { anyhow::anyhow!("no client credentials found for site {site} — run 'pup auth login' first") })?; - let org_label = org.map(|o| format!(" (org: {o})")).unwrap_or_default(); + let org_label = org_suffix(org); eprintln!("šŸ”„ Refreshing access token for site: {site}{org_label}..."); let dcr_client = dcr::DcrClient::new(site); @@ -486,6 +504,7 @@ pub fn list(_cfg: &Config) -> Result<()> { #[cfg(all(test, not(target_arch = "wasm32")))] mod tests { use super::*; + use crate::auth::storage::SessionEntry; use crate::config::{Config, OutputFormat}; /// Temporary directory that removes itself on drop. Mirrors the helper @@ -594,8 +613,18 @@ mod tests { std::env::set_var("PUP_CONFIG_DIR", tmp.path()); std::env::set_var("DD_TOKEN_STORAGE", "file"); - storage::save_session("datadoghq.com", None, None).unwrap(); - storage::save_session("datadoghq.com", Some("prod-child"), None).unwrap(); + storage::save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: None, + org_uuid: None, + }) + .unwrap(); + storage::save_session(&SessionEntry { + site: "datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); let cfg = base_config(); let result = list(&cfg); @@ -686,7 +715,7 @@ mod tests { #[test] fn resolve_save_target_no_hint_keeps_requested_org() { let t = resolve_save_target(Some("prod-child"), None, None, None); - assert_eq!(t.org.as_deref(), Some("prod-child")); + assert!(matches!(t, SaveTarget::Requested(Some(s)) if s == "prod-child")); } #[test] @@ -696,9 +725,9 @@ mod tests { Some("prod-child"), Some(uuid), Some(uuid), - Some("Datadog HQ"), + Some("Mos Eisley Cantina"), ); - assert_eq!(t.org.as_deref(), Some("prod-child")); + assert!(matches!(t, SaveTarget::Requested(Some(s)) if s == "prod-child")); } #[test] @@ -708,7 +737,7 @@ mod tests { let upper = "AABBCCDD-1111-2222-3333-EEFFAABBCCDD"; let lower = "aabbccdd-1111-2222-3333-eeffaabbccdd"; let t = resolve_save_target(Some("prod-child"), Some(upper), Some(lower), None); - assert_eq!(t.org.as_deref(), Some("prod-child")); + assert!(matches!(t, SaveTarget::Requested(Some(s)) if s == "prod-child")); } #[test] @@ -716,7 +745,20 @@ mod tests { let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), Some("Other Org")); - assert_eq!(t.org.as_deref(), Some("Other Org")); + match t { + SaveTarget::Reassigned { + label, + requested_uuid, + actual_uuid, + actual_name, + } => { + assert_eq!(label.as_deref(), Some("Other Org")); + assert_eq!(requested_uuid, req); + assert_eq!(actual_uuid, act); + assert_eq!(actual_name.as_deref(), Some("Other Org")); + } + other => panic!("expected Reassigned, got {other:?}"), + } } #[test] @@ -727,7 +769,15 @@ mod tests { let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), None); - assert_eq!(t.org.as_deref(), Some("11111111")); + match t { + SaveTarget::Reassigned { + label, actual_name, .. + } => { + assert_eq!(label.as_deref(), Some("11111111")); + assert!(actual_name.is_none()); + } + other => panic!("expected Reassigned, got {other:?}"), + } } #[test] @@ -736,7 +786,7 @@ mod tests { // trust the user's --org label and the in-flight UUID we sent. let req = "00000000-1111-2222-3333-444444444444"; let t = resolve_save_target(Some("prod-child"), Some(req), None, None); - assert_eq!(t.org.as_deref(), Some("prod-child")); + assert!(matches!(t, SaveTarget::Requested(Some(s)) if s == "prod-child")); } #[tokio::test] @@ -751,8 +801,18 @@ mod tests { std::env::set_var("DD_TOKEN_STORAGE", "file"); let site = "logout-session.example.invalid"; - storage::save_session(site, None, None).unwrap(); - storage::save_session(site, Some("keep-me"), None).unwrap(); + storage::save_session(&SessionEntry { + site: site.into(), + org: None, + org_uuid: None, + }) + .unwrap(); + storage::save_session(&SessionEntry { + site: site.into(), + org: Some("keep-me".into()), + org_uuid: None, + }) + .unwrap(); let mut cfg = base_config(); cfg.site = site.into(); diff --git a/src/config.rs b/src/config.rs index 09c5f45c..48c462a0 100644 --- a/src/config.rs +++ b/src/config.rs @@ -482,6 +482,7 @@ fn env_bool(key: &str) -> bool { #[cfg(test)] mod tests { use super::*; + use crate::auth::storage::SessionEntry; use crate::test_utils::ENV_LOCK; fn make_cfg(api_key: Option<&str>, app_key: Option<&str>, token: Option<&str>) -> Config { @@ -937,8 +938,12 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None) - .unwrap(); + crate::auth::storage::save_session(&SessionEntry { + site: "custom.datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); std::env::set_var("DD_ORG", "prod-child"); let cfg = Config::from_env().unwrap(); @@ -968,8 +973,12 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("custom.datadoghq.com", Some("prod-child"), None) - .unwrap(); + crate::auth::storage::save_session(&SessionEntry { + site: "custom.datadoghq.com".into(), + org: Some("prod-child".into()), + org_uuid: None, + }) + .unwrap(); std::env::set_var("DD_ORG", "prod-child"); std::env::set_var("DD_SITE", "datadoghq.eu"); @@ -1000,8 +1009,18 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("a.datadoghq.com", Some("org-a"), None).unwrap(); - crate::auth::storage::save_session("b.datadoghq.com", Some("org-b"), None).unwrap(); + crate::auth::storage::save_session(&SessionEntry { + site: "a.datadoghq.com".into(), + org: Some("org-a".into()), + org_uuid: None, + }) + .unwrap(); + crate::auth::storage::save_session(&SessionEntry { + site: "b.datadoghq.com".into(), + org: Some("org-b".into()), + org_uuid: None, + }) + .unwrap(); // Simulate the post-from_env state where we resolved org-a's site via // the registry (site_explicit=false because the user did not set DD_SITE). @@ -1042,7 +1061,12 @@ mod tests { std::fs::create_dir_all(&tmp).unwrap(); std::env::set_var("PUP_CONFIG_DIR", &tmp); - crate::auth::storage::save_session("session.datadoghq.com", Some("org-a"), None).unwrap(); + crate::auth::storage::save_session(&SessionEntry { + site: "session.datadoghq.com".into(), + org: Some("org-a".into()), + org_uuid: None, + }) + .unwrap(); let mut cfg = Config { api_key: None, diff --git a/src/main.rs b/src/main.rs index 9bd4fbca..d3a22604 100644 --- a/src/main.rs +++ b/src/main.rs @@ -14010,12 +14010,15 @@ async fn main_inner() -> anyhow::Result<()> { let resolved = resolve_login_scopes(scopes.as_deref(), cfg.org.as_deref(), is_read_only); let resolved_port = resolve_callback_port(callback_port)?; + // Coerce empty `--org-uuid ""` to no-hint so callees treat + // it the same as omitting the flag, not as `dd_oid=`. + let org_uuid_hint = org_uuid.as_deref().filter(|s| !s.is_empty()); commands::auth::login( &cfg, resolved, subdomain.as_deref(), resolved_port, - org_uuid.as_deref(), + org_uuid_hint, ) .await? } From 62e2735148ae0b2324837c19cb39c9e48b0523e5 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 09:46:15 -0700 Subject: [PATCH 07/11] fix(auth): disambiguate relabeled session + cleanup hint on dd_oid mismatch Codex review flagged two real risks in the previous mismatch handling: 1. **Relabel could overwrite an unrelated existing session**. Saving under just `dd_org_name` meant a fresh login that resolved to "Foo HQ" would silently replace any existing `(site, "Foo HQ")` session on that site, even if that session belonged to a different underlying org UUID. Org display names are not stable identifiers, so the relabel now always embeds the actual UUID's 8-char prefix: `"Foo HQ [11111111]"`. Collisions become impossible. 2. **The user's pre-existing `--org foo` session was left untouched silently**. The warning told users their token was being saved somewhere else but didn't mention that the requested-label session was preserved as-is and may serve stale tokens on future commands. The warning now suggests `pup auth logout --org foo` so users can actively clear the prior session if it's stale. Also tightens `SaveTarget::Reassigned.label` from `Option` to `String` since the resolution path is now total, and drops a dead `code{...}` CSS rule on the success page that wasn't matching any rendered element. --- src/auth/callback.rs | 1 - src/commands/auth.rs | 75 ++++++++++++++++++++++++++++++-------------- 2 files changed, 51 insertions(+), 25 deletions(-) diff --git a/src/auth/callback.rs b/src/auth/callback.rs index 73bb31f6..3eaa9969 100644 --- a/src/auth/callback.rs +++ b/src/auth/callback.rs @@ -249,7 +249,6 @@ h1{{color:#632ca6;margin:0 0 .5rem}} p{{color:#555;margin:.4rem 0}} .org{{margin-top:.2rem}} .uuid{{margin-top:.75rem;color:#999;font-size:.75em;font-family:ui-monospace,SFMono-Regular,Menlo,Consolas,monospace;letter-spacing:.01em;word-break:break-all}} -code{{background:#f0f0f0;padding:.1em .35em;border-radius:3px;font-family:ui-monospace,SFMono-Regular,Menlo,Consolas,monospace}} a{{color:#632ca6}} .close{{margin-top:1.25rem;color:#777;font-size:.9em}}

Authentication Successful

diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 6a972486..7ea111a5 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -185,16 +185,19 @@ pub async fn login( .as_deref() .map(|n| format!(" ({n})")) .unwrap_or_default(); - let label_display = label - .as_deref() - .map(|l| format!("\"{l}\"")) - .unwrap_or_else(|| "the default session".to_string()); + let cleanup_hint = match org { + Some(o) => format!( + "\n Your existing '{o}' session is unchanged. \ + Run `pup auth logout --org {o}` to clear it if it's stale." + ), + None => String::new(), + }; eprintln!( "āš ļø Requested org UUID {requested_uuid} but OAuth returned \ {actual_uuid}{actual_name_suffix}. Saving token under \ - {label_display} instead of the requested label." + \"{label}\" instead of the requested label.{cleanup_hint}" ); - label + Some(label) } }; let saved_org = saved_org_owned.as_deref(); @@ -238,7 +241,7 @@ enum SaveTarget { /// dd_oid mismatch — save under `label` and warn the user. Carries the /// requested vs actual UUIDs so the caller can render a useful warning. Reassigned { - label: Option, + label: String, requested_uuid: String, actual_uuid: String, actual_name: Option, @@ -247,8 +250,9 @@ enum SaveTarget { /// Pure: pick the `(site, org)` save target for a finished login. Mismatch /// detection compares hinted vs callback UUIDs case-insensitively (UUIDs are -/// hex). The mismatch fallback prefers `dd_org_name`, then a UUID prefix, so -/// the saved label is always distinguishable from the requested one. +/// hex). On mismatch, the relabel always embeds an 8-char UUID prefix so the +/// stored entry is unambiguous even if the issuer's display name collides +/// with another existing session on the same site. #[cfg(not(target_arch = "wasm32"))] fn resolve_save_target( requested_org: Option<&str>, @@ -257,14 +261,19 @@ fn resolve_save_target( actual_org_name: Option<&str>, ) -> SaveTarget { match (requested_uuid, actual_uuid) { - (Some(req), Some(actual)) if !req.eq_ignore_ascii_case(actual) => SaveTarget::Reassigned { - label: actual_org_name - .map(String::from) - .or_else(|| Some(actual.chars().take(8).collect())), - requested_uuid: req.to_string(), - actual_uuid: actual.to_string(), - actual_name: actual_org_name.map(String::from), - }, + (Some(req), Some(actual)) if !req.eq_ignore_ascii_case(actual) => { + let prefix: String = actual.chars().take(8).collect(); + let label = match actual_org_name { + Some(name) if !name.is_empty() => format!("{name} [{prefix}]"), + _ => prefix, + }; + SaveTarget::Reassigned { + label, + requested_uuid: req.to_string(), + actual_uuid: actual.to_string(), + actual_name: actual_org_name.map(String::from), + } + } _ => SaveTarget::Requested(requested_org.map(String::from)), } } @@ -741,7 +750,10 @@ mod tests { } #[test] - fn resolve_save_target_mismatch_uses_dd_org_name() { + fn resolve_save_target_mismatch_combines_name_and_uuid_prefix() { + // The relabel embeds the UUID prefix so a save under (site, "Other Org") + // can never silently overwrite an unrelated existing "Other Org" session + // on the same site that has a different underlying UUID. let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), Some("Other Org")); @@ -752,7 +764,7 @@ mod tests { actual_uuid, actual_name, } => { - assert_eq!(label.as_deref(), Some("Other Org")); + assert_eq!(label, "Other Org [11111111]"); assert_eq!(requested_uuid, req); assert_eq!(actual_uuid, act); assert_eq!(actual_name.as_deref(), Some("Other Org")); @@ -762,10 +774,10 @@ mod tests { } #[test] - fn resolve_save_target_mismatch_falls_back_to_uuid_prefix() { - // No dd_org_name in the callback (older issuer or unusual flow): use - // the first 8 chars of the actual UUID as a stable, distinguishable - // label rather than reusing the wrong --org name. + fn resolve_save_target_mismatch_falls_back_to_uuid_prefix_only() { + // No dd_org_name in the callback (older issuer or unusual flow): the + // 8-char UUID prefix becomes the entire label so it's still stable + // and distinguishable. let req = "00000000-1111-2222-3333-444444444444"; let act = "11111111-2222-3333-4444-555555555555"; let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), None); @@ -773,13 +785,28 @@ mod tests { SaveTarget::Reassigned { label, actual_name, .. } => { - assert_eq!(label.as_deref(), Some("11111111")); + assert_eq!(label, "11111111"); assert!(actual_name.is_none()); } other => panic!("expected Reassigned, got {other:?}"), } } + #[test] + fn resolve_save_target_mismatch_treats_empty_org_name_as_missing() { + // An empty `dd_org_name=""` shouldn't produce labels like " [11111111]" + // with a leading space — fall back to the UUID-only label instead. + let req = "00000000-1111-2222-3333-444444444444"; + let act = "11111111-2222-3333-4444-555555555555"; + let t = resolve_save_target(Some("prod-child"), Some(req), Some(act), Some("")); + match t { + SaveTarget::Reassigned { label, .. } => { + assert_eq!(label, "11111111"); + } + other => panic!("expected Reassigned, got {other:?}"), + } + } + #[test] fn resolve_save_target_actual_uuid_missing_keeps_requested() { // If the issuer didn't echo dd_oid, we have no comparison to make; From b55f0e62dadad60bf38a49faf1b6259ecdaa613c Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 09:55:04 -0700 Subject: [PATCH 08/11] fix(auth): preserve default-session idempotency + validate callback dd_oid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more issues caught in the iter-2 review pass: - **Default-org reauth was silently leaving the unnamed session stale** on dd_oid mismatch. `pup auth login` (no `--org`) is supposed to be idempotent — refresh the default session. Previously, a recalled stored UUID that no longer matched the issuer's response would route through `Reassigned`, save under a phantom labeled slot, and leave the unnamed session pointing at old credentials. Future commands without `--org` would still load the stale token. `resolve_save_target` now only relabels when `--org` was explicitly set; when it wasn't, the default session is overwritten in place with the new UUID, so the user gets the idempotent refresh they expected. - **Callback `dd_oid` is now shape-validated** before being persisted. In the stdin-paste fallback path the user pastes a URL whose query parameters could carry an arbitrary string. Anything that doesn't match the canonical `8-4-4-4-12` hex-with-dashes UUID shape is treated as missing, so a tampered URL can't seed garbage into `sessions.json` or rendered hints. Also filters empty `dd_org_name` out of the warning text so we don't render `OAuth returned ()` with a dangling empty paren. --- src/commands/auth.rs | 85 +++++++++++++++++++++++++++++++++++++------- 1 file changed, 73 insertions(+), 12 deletions(-) diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 7ea111a5..54836d51 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -168,10 +168,15 @@ pub async fn login( // 8. Resolve the save target — relabel under the actual returned org on // dd_oid mismatch. The OAuth code is single-use, so refusing to save // would throw away the user's click. + // + // Filter callback `dd_oid` through a shape check so a tampered URL + // pasted into the stdin fallback path can't seed an arbitrary string + // into the session registry. + let validated_dd_oid = result.dd_oid.as_deref().filter(|u| looks_like_uuid(u)); let saved_org_owned: Option = match resolve_save_target( org, effective_org_uuid.as_deref(), - result.dd_oid.as_deref(), + validated_dd_oid, result.dd_org_name.as_deref(), ) { SaveTarget::Requested(label) => label, @@ -209,10 +214,9 @@ pub async fn login( })?; // Prefer the callback's `dd_oid` over the CLI/stored hint — it reflects - // the org the user actually consented for. - let saved_org_uuid = result - .dd_oid - .as_deref() + // the org the user actually consented for. Skip values that don't look + // like a UUID so we don't persist garbage from a tampered callback URL. + let saved_org_uuid = validated_dd_oid .or(effective_org_uuid.as_deref()) .map(String::from); storage::save_session(&storage::SessionEntry { @@ -232,14 +236,31 @@ pub async fn login( Ok(()) } +/// Loose UUID shape check (8-4-4-4-12 hex with dashes, ASCII only). The +/// callback's `dd_oid` is unsigned metadata — in the stdin-paste fallback +/// path a tampered URL could carry an arbitrary string. Rejecting anything +/// that doesn't look like a UUID keeps such values from being persisted into +/// `sessions.json` or rendered as a future hint. +#[cfg(not(target_arch = "wasm32"))] +fn looks_like_uuid(s: &str) -> bool { + s.len() == 36 + && s.chars().enumerate().all(|(i, c)| match i { + 8 | 13 | 18 | 23 => c == '-', + _ => c.is_ascii_hexdigit(), + }) +} + /// What `resolve_save_target` decided about where to save the new token. #[cfg(not(target_arch = "wasm32"))] #[derive(Debug)] enum SaveTarget { - /// dd_oid hint matched (or wasn't sent) — save under the requested label. + /// dd_oid hint matched, no hint was sent, or there's no `--org` to + /// relabel away from — save under the requested label (or the default + /// session when `--org` was unset). Requested(Option), - /// dd_oid mismatch — save under `label` and warn the user. Carries the - /// requested vs actual UUIDs so the caller can render a useful warning. + /// dd_oid mismatch on a named-org login — save under `label` and warn + /// the user. Carries the requested vs actual UUIDs so the caller can + /// render a useful warning. Reassigned { label: String, requested_uuid: String, @@ -250,7 +271,11 @@ enum SaveTarget { /// Pure: pick the `(site, org)` save target for a finished login. Mismatch /// detection compares hinted vs callback UUIDs case-insensitively (UUIDs are -/// hex). On mismatch, the relabel always embeds an 8-char UUID prefix so the +/// hex). Relabel only triggers for named-org logins — when `--org` is unset +/// the default session is the source of truth and gets overwritten with the +/// new UUID directly, so plain `pup auth login` stays idempotent. +/// +/// On relabel, the new label always embeds an 8-char UUID prefix so the /// stored entry is unambiguous even if the issuer's display name collides /// with another existing session on the same site. #[cfg(not(target_arch = "wasm32"))] @@ -260,8 +285,8 @@ fn resolve_save_target( actual_uuid: Option<&str>, actual_org_name: Option<&str>, ) -> SaveTarget { - match (requested_uuid, actual_uuid) { - (Some(req), Some(actual)) if !req.eq_ignore_ascii_case(actual) => { + match (requested_org, requested_uuid, actual_uuid) { + (Some(_), Some(req), Some(actual)) if !req.eq_ignore_ascii_case(actual) => { let prefix: String = actual.chars().take(8).collect(); let label = match actual_org_name { Some(name) if !name.is_empty() => format!("{name} [{prefix}]"), @@ -271,7 +296,7 @@ fn resolve_save_target( label, requested_uuid: req.to_string(), actual_uuid: actual.to_string(), - actual_name: actual_org_name.map(String::from), + actual_name: actual_org_name.filter(|n| !n.is_empty()).map(String::from), } } _ => SaveTarget::Requested(requested_org.map(String::from)), @@ -816,6 +841,42 @@ mod tests { assert!(matches!(t, SaveTarget::Requested(Some(s)) if s == "prod-child")); } + #[test] + fn resolve_save_target_default_org_mismatch_stays_default() { + // Default-org login with a stale stored UUID hint that no longer + // matches the issuer's response: `pup auth login` is supposed to be + // idempotent for the default session, so we don't relabel onto a + // phantom slot. The default session gets refreshed in place; the + // stored UUID hint is updated by the caller from the callback. + let req = "00000000-1111-2222-3333-444444444444"; + let act = "11111111-2222-3333-4444-555555555555"; + let t = resolve_save_target(None, Some(req), Some(act), Some("Other Org")); + assert!(matches!(t, SaveTarget::Requested(None))); + } + + // looks_like_uuid ----------------------------------------------------------- + + #[test] + fn looks_like_uuid_accepts_canonical_form() { + assert!(looks_like_uuid("00000000-1111-2222-3333-444444444444")); + assert!(looks_like_uuid("aabbccdd-1111-2222-3333-eeffaabbccdd")); + assert!(looks_like_uuid("AABBCCDD-1111-2222-3333-EEFFAABBCCDD")); + } + + #[test] + fn looks_like_uuid_rejects_garbage() { + assert!(!looks_like_uuid("")); + assert!(!looks_like_uuid("not-a-uuid")); + // Wrong length + assert!(!looks_like_uuid("00000000-1111-2222-3333-44444444444")); + // Non-hex character + assert!(!looks_like_uuid("0000000g-1111-2222-3333-444444444444")); + // Missing dash positions + assert!(!looks_like_uuid("000000001111-2222-3333-44444444444444")); + // Hostile injection attempt + assert!(!looks_like_uuid(" ")); + } + #[tokio::test] async fn test_logout_with_org_removes_session_entry() { // save a session, then logout should remove just that org's entry From 2559f4c9f9588e7ebb0cb9d4f9e6458653dd8a93 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 10:04:14 -0700 Subject: [PATCH 09/11] fix(auth): drop recalled UUID hint when the callback can't confirm it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex flagged that on a successful login where the issuer omitted (or sent a malformed) `dd_oid`, the previous code carried the already-recalled UUID forward into the saved session. That meant a user could authenticate into a different org but `sessions.json` would still be tagged with the old org UUID, and subsequent logins would re-emit that stale hint. The fallback chain is now: callback-validated `dd_oid` → user's explicit `--org-uuid` flag → None. The recalled-from-storage value is only used to construct the in-flight authorize URL; it is no longer written back unless the callback confirms it. --- src/commands/auth.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 54836d51..1d5141c9 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -213,12 +213,15 @@ pub async fn login( Ok(store.storage_location()) })?; - // Prefer the callback's `dd_oid` over the CLI/stored hint — it reflects - // the org the user actually consented for. Skip values that don't look - // like a UUID so we don't persist garbage from a tampered callback URL. + // Persist the callback's confirmed `dd_oid` if we have one. Otherwise + // fall back only to the user's explicit `--org-uuid` flag — never to a + // previously-recalled hint, since carrying that forward unconfirmed + // would let a stale UUID survive a login that actually landed in a + // different org. Users who want to re-establish a hint can pass the + // flag again on the next login. let saved_org_uuid = validated_dd_oid - .or(effective_org_uuid.as_deref()) - .map(String::from); + .map(String::from) + .or_else(|| org_uuid.map(String::from)); storage::save_session(&storage::SessionEntry { site: site.clone(), org: saved_org.map(String::from), From 0ea26727508cac506fbf1140adca797939211344 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 11:03:55 -0700 Subject: [PATCH 10/11] docs(auth): tighten 'any saved session' comment to reflect (site, org) keying --- src/commands/auth.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 1d5141c9..2f2123b3 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -34,9 +34,9 @@ pub async fn login( let site = &cfg.site; let org = cfg.org.as_deref(); - // Resolve effective org_uuid: CLI flag wins; otherwise recall from any - // saved session so re-auth keeps emitting `dd_oid` without re-passing the - // flag. + // Resolve effective org_uuid: CLI flag wins; otherwise recall the UUID + // stored on the matching `(site, org)` session so re-auth keeps emitting + // `dd_oid` without re-passing the flag. let stored_session = storage::find_session(site, org); let effective_org_uuid: Option = org_uuid .map(String::from) From bbf4bbd19b60bc9f69472a2c4e4f0c989e4e7196 Mon Sep 17 00:00:00 2001 From: Stephen Rosenthal Date: Fri, 8 May 2026 11:21:09 -0700 Subject: [PATCH 11/11] feat(auth): surface org_uuid in pup auth list output --- src/commands/auth.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/commands/auth.rs b/src/commands/auth.rs index 2f2123b3..b597ecce 100644 --- a/src/commands/auth.rs +++ b/src/commands/auth.rs @@ -513,6 +513,7 @@ pub fn list(cfg: &Config) -> Result<()> { "expires_at": expires_at, "has_refresh": !t.refresh_token.is_empty(), "org": s.org, + "org_uuid": s.org_uuid, "scopes": scopes, "site": s.site, "status": status, @@ -522,6 +523,7 @@ pub fn list(cfg: &Config) -> Result<()> { "expires_at": null, "has_refresh": false, "org": s.org, + "org_uuid": s.org_uuid, "scopes": [], "site": s.site, "status": "no token",