From c6317b954c22d0c93f1a902cd8c4fcee400c7816 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 12:44:40 -0700 Subject: [PATCH 01/16] feat: DEV/TEST environments + OAuth scope step-up (DEVEX-719) Consolidate environment->endpoint resolution into one module and let internal developers target DEV/TEST at runtime without committing anything to this OSS repo, and wire the CLI to cli-engine 0.2.1's OAuth scope step-up. Environments (DEVEX-719): - New `environments` module resolves an env to {api_url, client_id, auth_url, token_url} from built-ins (ote/prod) -> ~/.config/gddy/environments.toml -> per-env _API_URL vars (built-ins overridable). Replaces the API-base-URL logic previously duplicated across env, auth, and client. No internal hostnames or client IDs are committed. - `--env` validates against known envs; `env list/get/info/set` route through the resolver. Scope step-up (consumes cli-engine 0.2.1): - GoDaddyAuthProvider forwards get_credential_for so the per-env PkceAuthProvider performs scope step-up; SCOPES -> DEFAULT_OAUTH_SCOPES. - `api call` merges the matched endpoint's scopes with --scope and requests them via credential_with_scopes; --scope now requires a value. Hardening from deep review: api_url_for_env never returns an empty base URL; documented the built-in-override token-redirect trade-off; added unit tests for the resolver, scope merge, and api_url_for_env. Co-Authored-By: Claude Opus 4.8 --- rust/Cargo.lock | 10 +- rust/Cargo.toml | 2 +- rust/src/api_explorer/mod.rs | 91 ++++++-- rust/src/application/client.rs | 29 ++- rust/src/auth.rs | 107 ++++----- rust/src/env/mod.rs | 90 ++++---- rust/src/environments/mod.rs | 409 +++++++++++++++++++++++++++++++++ rust/src/main.rs | 7 +- 8 files changed, 626 insertions(+), 119 deletions(-) create mode 100644 rust/src/environments/mod.rs diff --git a/rust/Cargo.lock b/rust/Cargo.lock index 0f0a8d9..2464036 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -536,9 +536,9 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "cli-engine" -version = "0.2.0" +version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d3c13c34cb4bce62d50adef63dff8b16abc53ac4ae5ddb4d24670307b0291390" +checksum = "5f4015806946ade71addfa8ae7a869dfd061df2536dac41371efb0489bed9760" dependencies = [ "async-trait", "base64 0.22.1", @@ -1290,7 +1290,7 @@ dependencies = [ "libc", "percent-encoding", "pin-project-lite", - "socket2 0.5.10", + "socket2 0.6.3", "tokio", "tower-service", "tracing", @@ -2020,7 +2020,7 @@ dependencies = [ "quinn-udp", "rustc-hash", "rustls", - "socket2 0.5.10", + "socket2 0.6.3", "thiserror 2.0.18", "tokio", "tracing", @@ -2057,7 +2057,7 @@ dependencies = [ "cfg_aliases", "libc", "once_cell", - "socket2 0.5.10", + "socket2 0.6.3", "tracing", "windows-sys 0.60.2", ] diff --git a/rust/Cargo.toml b/rust/Cargo.toml index bdca7b1..d40bd03 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -16,7 +16,7 @@ path = "src/main.rs" async-trait = "0.1" chrono = { version = "0.4", default-features = false, features = ["clock", "serde"] } clap = { version = "4.5", features = ["std", "string"] } -cli-engine = { features = ["pkce-auth"], version = "0.2" } +cli-engine = { features = ["pkce-auth"], version = "0.2.1" } dirs = "6" fancy-regex = "0.14" regex = { version = "1", features = ["std"] } diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 23227a2..334328a 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -136,6 +136,18 @@ fn find_endpoint<'a>(catalog: &'a [Domain], query: &str) -> Option<(&'a Domain, }) } +/// Union of user-supplied `--scope` flags and a matched endpoint's declared +/// scopes, order-preserving and de-duplicated (flags first). +fn merge_required_scopes(flag_scopes: Vec, endpoint_scopes: &[String]) -> Vec { + let mut required = flag_scopes; + for scope in endpoint_scopes { + if !required.contains(scope) { + required.push(scope.clone()); + } + } + required +} + fn search_endpoints<'a>(catalog: &'a [Domain], query: &str) -> Vec<(&'a Domain, &'a Endpoint)> { let q = query.to_lowercase(); catalog @@ -432,8 +444,10 @@ fn call_command() -> RuntimeCommandSpec { .long("scope") .short('s') .value_name("SCOPE") - .num_args(0..) - .help("Required OAuth scope(s); on 403 a re-auth hint is shown"), + // Require a value per occurrence so a bare `--scope` is + // rejected rather than silently contributing nothing. + .num_args(1..) + .help("Additional required OAuth scope(s), merged with the endpoint's"), ), |ctx| async move { let endpoint = ctx @@ -446,7 +460,26 @@ fn call_command() -> RuntimeCommandSpec { .get("method") .and_then(|v| v.as_str()) .unwrap_or("GET"); - let token = ctx.credential().await?.token; + // Required scopes = explicit --scope flags, plus the matched catalog + // endpoint's declared scopes (best-effort: a concrete request path + // may not match a templated catalog path, in which case only --scope + // contributes). These drive OAuth scope step-up at credential time. + let flag_scopes: Vec = ctx + .args + .get("scope") + .and_then(|v| v.as_array()) + .map(|arr| { + arr.iter() + .filter_map(|v| v.as_str().map(str::to_owned)) + .collect() + }) + .unwrap_or_default(); + let endpoint_scopes = find_endpoint(catalog(), endpoint) + .map(|(_, ep)| ep.scopes.as_slice()) + .unwrap_or(&[]); + let required = merge_required_scopes(flag_scopes, endpoint_scopes); + + let token = ctx.credential_with_scopes(&required).await?.token; let base_url = crate::application::client::api_url_for_env(&ctx.middleware.env); let url = format!("{base_url}{endpoint}"); @@ -524,23 +557,17 @@ fn call_command() -> RuntimeCommandSpec { None }; - let scopes: Vec = ctx - .args - .get("scope") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|v| v.as_str().map(str::to_owned)) - .collect() - }) - .unwrap_or_default(); - let body: Value = resp.json().await.unwrap_or(json!(null)); - if status == 403 && !scopes.is_empty() { + // Scope step-up already ran up front (the token was requested with + // `required`). A 403 here means the granted token still lacks a + // required scope — surface it rather than silently returning the body. + if status == 403 && !required.is_empty() { return Err(cli_engine::CliCoreError::message(format!( - "403 Forbidden — your token may be missing scope(s): {}\nRun `gddy auth login` to re-authenticate.", - scopes.join(", ") + "403 Forbidden — the authorized token is missing required scope(s): {}. \ + Re-run `gddy auth login --scope {}` and try again.", + required.join(", "), + required.join(" ") ))); } @@ -562,3 +589,33 @@ fn call_command() -> RuntimeCommandSpec { }, ) } + +#[cfg(test)] +mod tests { + use super::merge_required_scopes; + + fn v(items: &[&str]) -> Vec { + items.iter().map(|s| (*s).to_owned()).collect() + } + + #[test] + fn merge_flags_only_when_no_endpoint_scopes() { + assert_eq!(merge_required_scopes(v(&["a", "b"]), &[]), v(&["a", "b"])); + } + + #[test] + fn merge_endpoint_only_when_no_flags() { + assert_eq!( + merge_required_scopes(v(&[]), &v(&["x", "y"])), + v(&["x", "y"]) + ); + } + + #[test] + fn merge_unions_and_dedupes_flags_first() { + assert_eq!( + merge_required_scopes(v(&["a", "b"]), &v(&["b", "c"])), + v(&["a", "b", "c"]) + ); + } +} diff --git a/rust/src/application/client.rs b/rust/src/application/client.rs index 464f026..8311b21 100644 --- a/rust/src/application/client.rs +++ b/rust/src/application/client.rs @@ -186,9 +186,30 @@ impl ApplicationClient { } } -pub fn api_url_for_env(env: &str) -> &'static str { - match env { - "prod" => "https://api.godaddy.com", - _ => "https://api.ote-godaddy.com", +pub fn api_url_for_env(env: &str) -> String { + crate::environments::resolve(env) + .or_else(|_| crate::environments::resolve(crate::environments::DEFAULT_ENV)) + .map(|e| e.api_url) + // Never return an empty base URL (e.g. if a malformed local config makes + // even the default fail to resolve) — fall back to the built-in default. + .unwrap_or_else(|_| crate::environments::default_api_url().to_owned()) +} + +#[cfg(test)] +mod tests { + use super::api_url_for_env; + + #[test] + fn api_url_for_builtins() { + assert_eq!(api_url_for_env("prod"), "https://api.godaddy.com"); + assert_eq!(api_url_for_env("ote"), "https://api.ote-godaddy.com"); + } + + #[test] + fn api_url_for_unknown_env_falls_back_to_default_and_is_never_empty() { + // Unknown env resolves to the default environment's URL (never empty). + let url = api_url_for_env("definitely-not-a-real-env-xyz"); + assert!(!url.is_empty()); + assert!(url.starts_with("https://")); } } diff --git a/rust/src/auth.rs b/rust/src/auth.rs index b786de6..d09c0bf 100644 --- a/rust/src/auth.rs +++ b/rust/src/auth.rs @@ -1,64 +1,52 @@ use async_trait::async_trait; use cli_engine::{ - CliCoreError, Credential, Result, + CliCoreError, Credential, CredentialRequest, Result, auth::{AuthProvider, pkce::PkceAuthProvider}, }; -const OTE_API_URL: &str = "https://api.ote-godaddy.com"; -const PROD_API_URL: &str = "https://api.godaddy.com"; - -const OTE_CLIENT_ID: &str = "a502484b-d7b1-4509-aa88-08b391a54c28"; -const PROD_CLIENT_ID: &str = "39489dee-4103-4284-9aab-9f2452142bce"; - -const SCOPES: &[&str] = &["apps.app-registry:read", "apps.app-registry:write"]; +use crate::environments::{self, ResolvedEnv}; /// Single auth provider that dispatches to env-specific PKCE providers. /// -/// Env var overrides (per-env): -/// OTE: OTE_OAUTH_CLIENT_ID, OTE_OAUTH_AUTH_URL, OTE_OAUTH_TOKEN_URL -/// PROD: PROD_OAUTH_CLIENT_ID, PROD_OAUTH_AUTH_URL, PROD_OAUTH_TOKEN_URL -#[derive(Debug)] -pub struct GoDaddyAuthProvider { - ote: PkceAuthProvider, - prod: PkceAuthProvider, -} +/// Each env's provider is named after the env, so cli-engine's +/// `PkceAuthProvider` picks up its per-env overrides automatically: +/// `_OAUTH_CLIENT_ID`, `_OAUTH_AUTH_URL`, `_OAUTH_TOKEN_URL` +/// where `` is the env name uppercased with `-` replaced by `_` +/// (e.g. `OTE_OAUTH_CLIENT_ID`, `DEV_OAUTH_AUTH_URL`). The API base URL and the +/// per-env defaults come from [`crate::environments`], which also resolves +/// custom DEV/TEST environments from `~/.config/gddy/environments.toml`. +#[derive(Debug, Default)] +pub struct GoDaddyAuthProvider; impl GoDaddyAuthProvider { pub fn new() -> Self { - let ote = PkceAuthProvider::new( - "ote", - format!("{OTE_API_URL}/v2/oauth2/authorize"), - format!("{OTE_API_URL}/v2/oauth2/token"), - OTE_CLIENT_ID, - SCOPES, - ) - .with_app_id("gddy") - .with_redirect_uri("http://localhost:7443/callback"); - - let prod = PkceAuthProvider::new( - "prod", - format!("{PROD_API_URL}/v2/oauth2/authorize"), - format!("{PROD_API_URL}/v2/oauth2/token"), - PROD_CLIENT_ID, - SCOPES, - ) - .with_app_id("gddy") - .with_redirect_uri("http://localhost:7443/callback"); - - Self { ote, prod } + Self } - fn provider_for(&self, env: &str) -> Result<&PkceAuthProvider> { - match env { - "ote" => Ok(&self.ote), - "prod" => Ok(&self.prod), - _ => Err(CliCoreError::message(format!( - "unknown environment {env:?}; expected \"ote\" or \"prod\"" - ))), - } + /// Build a PKCE provider for the given env by resolving its endpoints. + /// + /// Providers are constructed on demand (tokens persist in the OS keychain, + /// so there is nothing to cache across a one-shot CLI invocation). Works for + /// built-ins as well as any custom env defined via env var or local config. + fn provider_for(&self, env: &str) -> Result { + let resolved = + environments::resolve(env).map_err(|e| CliCoreError::message(e.to_string()))?; + Ok(build_provider(&resolved)) } } +fn build_provider(env: &ResolvedEnv) -> PkceAuthProvider { + PkceAuthProvider::new( + env.name.clone(), + env.auth_url.clone(), + env.token_url.clone(), + env.client_id.clone(), + environments::DEFAULT_OAUTH_SCOPES, + ) + .with_app_id(environments::APP_ID) + .with_redirect_uri(environments::REDIRECT_URI) +} + #[async_trait] impl AuthProvider for GoDaddyAuthProvider { fn name(&self) -> &str { @@ -66,22 +54,37 @@ impl AuthProvider for GoDaddyAuthProvider { } async fn get_credential(&self, env: &str, command: &str, tier: &str) -> Result { - self.provider_for(env)? - .get_credential(env, command, tier) - .await + let provider = self.provider_for(env)?; + provider.get_credential(env, command, tier).await + } + + async fn get_credential_for(&self, req: &CredentialRequest<'_>) -> Result { + // Forward to the env's PKCE provider, which performs OAuth scope step-up + // when the cached token lacks the command's required scopes. + let provider = self.provider_for(req.env)?; + provider.get_credential_for(req).await } async fn status(&self, env: &str) -> Result { - self.provider_for(env)?.status(env).await + let provider = self.provider_for(env)?; + provider.status(env).await } async fn logout(&self, env: &str) -> Result<()> { - self.provider_for(env)?.logout(env).await + let provider = self.provider_for(env)?; + provider.logout(env).await } async fn list_environments(&self) -> Result> { - let mut envs = self.ote.list_environments().await.unwrap_or_default(); - envs.extend(self.prod.list_environments().await.unwrap_or_default()); + // Enumerate stored credentials across built-ins + locally-configured + // envs (env-var-only envs are excluded from `listable`, matching the + // `env list` contract). + let mut envs = Vec::new(); + for resolved in environments::listable().unwrap_or_default() { + let provider = build_provider(&resolved); + envs.extend(provider.list_environments().await.unwrap_or_default()); + } + envs.sort(); envs.dedup(); Ok(envs) } diff --git a/rust/src/env/mod.rs b/rust/src/env/mod.rs index 6d7407b..948f2ff 100644 --- a/rust/src/env/mod.rs +++ b/rust/src/env/mod.rs @@ -3,10 +3,7 @@ use cli_engine::{ }; use serde_json::json; -const OTE_API_URL: &str = "https://api.ote-godaddy.com"; -const PROD_API_URL: &str = "https://api.godaddy.com"; -const KNOWN_ENVS: &[&str] = &["ote", "prod"]; -pub const DEFAULT_ENV: &str = "prod"; +use crate::environments::{self, EnvError}; /// Resolve the path to the `.gdenv` state file in the user's home directory. /// @@ -31,8 +28,10 @@ pub fn get_env() -> Option { .map(|s| s.trim().to_owned()) // Ignore empty or unrecognized values (e.g. a hand-edited/corrupted // .gdenv) so callers fall back to DEFAULT_ENV instead of propagating an - // unknown environment that helpers would silently treat as prod. - .filter(|s| KNOWN_ENVS.contains(&s.as_str())) + // unknown environment. `is_known` accepts built-ins plus any env defined + // via env var or local config, so a persisted custom env (e.g. "dev") + // survives. + .filter(|s| !s.is_empty() && environments::is_known(s)) } pub fn set_env(env: &str) -> std::io::Result<()> { @@ -44,11 +43,12 @@ pub fn set_env(env: &str) -> std::io::Result<()> { }) } -fn api_url_for(env: &str) -> &'static str { - match env { - "ote" => OTE_API_URL, - _ => PROD_API_URL, - } +fn map_err(e: EnvError) -> cli_engine::CliCoreError { + cli_engine::CliCoreError::message(e.to_string()) +} + +fn active_env() -> String { + get_env().unwrap_or_else(|| environments::DEFAULT_ENV.to_owned()) } pub fn module() -> Module { @@ -60,14 +60,15 @@ pub fn module() -> Module { .with_tier(Tier::Read) .no_auth(true), |_cred, _args| async move { - let current = get_env().unwrap_or_else(|| DEFAULT_ENV.to_owned()); - let envs: Vec<_> = KNOWN_ENVS - .iter() - .map(|&e| { + let current = active_env(); + let envs: Vec<_> = environments::listable() + .map_err(map_err)? + .into_iter() + .map(|e| { json!({ - "name": e, - "active": e == current, - "apiUrl": api_url_for(e), + "name": e.name, + "active": e.name == current, + "apiUrl": e.api_url, }) }) .collect(); @@ -80,10 +81,11 @@ pub fn module() -> Module { .with_tier(Tier::Read) .no_auth(true), |_cred, _args| async move { - let env = get_env().unwrap_or_else(|| DEFAULT_ENV.to_owned()); + let env = active_env(); + let resolved = environments::resolve(&env).map_err(map_err)?; Ok(CommandResult::new(json!({ - "env": env, - "apiUrl": api_url_for(&env), + "env": resolved.name, + "apiUrl": resolved.api_url, }))) }, )) @@ -108,20 +110,17 @@ pub fn module() -> Module { .and_then(|v| v.as_str()) .unwrap_or("") .to_owned(); - if !KNOWN_ENVS.contains(&env.as_str()) { - return Err(cli_engine::CliCoreError::message(format!( - "unknown environment {env:?}; expected one of: {}", - KNOWN_ENVS.join(", ") - ))); - } + // Resolve up front: validates the env exists (built-in, env + // var, or local config) and yields its API URL for the reply. + let resolved = environments::resolve(&env).map_err(map_err)?; set_env(&env).map_err(|e| { cli_engine::CliCoreError::message(format!( "failed to write .gdenv state file: {e}" )) })?; Ok(CommandResult::new(json!({ - "env": env, - "apiUrl": api_url_for(&env), + "env": resolved.name, + "apiUrl": resolved.api_url, }))) }, )) @@ -131,11 +130,12 @@ pub fn module() -> Module { .with_tier(Tier::Read) .no_auth(true), |_cred, _args| async move { - let env = get_env().unwrap_or_else(|| DEFAULT_ENV.to_owned()); + let env = active_env(); + let resolved = environments::resolve(&env).map_err(map_err)?; Ok(CommandResult::new(json!({ - "env": env, - "apiUrl": api_url_for(&env), - "graphqlUrl": format!("{}/v1/applications/graphql", api_url_for(&env)), + "env": resolved.name, + "apiUrl": resolved.api_url, + "graphqlUrl": format!("{}/v1/applications/graphql", resolved.api_url), }))) }, )) @@ -164,11 +164,23 @@ mod tests { let json: serde_json::Value = serde_json::from_str(&output.rendered).expect("valid json output"); let envs = json["data"].as_array().expect("data array"); - assert!( - envs.iter() - .any(|e| e["name"] == "ote" || e["name"] == "prod"), - "expected known environments in output, got: {}", - output.rendered - ); + // Both built-ins are always listed, regardless of local config / env vars. + for name in ["ote", "prod"] { + let entry = envs + .iter() + .find(|e| e["name"] == name) + .unwrap_or_else(|| unreachable!("missing env {name} in: {}", output.rendered)); + // Output-shape contract: each entry carries name/active/apiUrl. + assert!( + entry["active"].is_boolean(), + "active should be bool: {entry}" + ); + assert!( + entry["apiUrl"] + .as_str() + .is_some_and(|u| u.starts_with("https://")), + "apiUrl should be an https URL: {entry}" + ); + } } } diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs new file mode 100644 index 0000000..b06ba35 --- /dev/null +++ b/rust/src/environments/mod.rs @@ -0,0 +1,409 @@ +//! Single source of truth for environment → endpoint resolution. +//! +//! Built-in, public-safe environments (`ote`, `prod`) are compiled in. Internal +//! DEV/TEST environments are supplied **at runtime** and never committed to this +//! (OSS) repo, via two override mechanisms: +//! +//! * **Per-env environment variable** — `_API_URL` overrides (or defines) +//! an environment's API base URL, where `` is the env name uppercased +//! with `-` replaced by `_` (e.g. `DEV_API_URL`, `OTE_API_URL`). This mirrors +//! cli-engine's `_OAUTH_CLIENT_ID` / `_AUTH_URL` / `_TOKEN_URL` naming, +//! which `PkceAuthProvider` reads automatically when a provider is named after +//! its environment. +//! * **Gitignored local config** — `~/.config/gddy/environments.toml` listing +//! custom environments. The file lives in the user's home directory, never in +//! the repo, so internal hostnames stay on the developer's machine. +//! +//! Resolution order (later layers win): built-in base → local config entry → +//! `_API_URL` env var. Built-ins may be overridden by either layer. +//! +//! Security note: because a built-in's `api_url` is overridable (and `auth_url` +//! /`token_url` derive from it), overriding e.g. `prod` redirects that env's +//! OAuth and bearer traffic to the new host while still presenting the built-in +//! client id — i.e. a real prod token could be sent to the overriding host. The +//! override surface (a process env var or a file under the user's home dir) +//! already implies local trust, so this is an accepted trade-off; only override +//! a built-in on a machine you control. + +use std::collections::BTreeMap; + +use serde::Deserialize; + +pub const DEFAULT_ENV: &str = "prod"; +/// Scopes requested at login by default. The authorization server may grant a +/// subset; commands needing more declare them and the provider steps up. +pub const DEFAULT_OAUTH_SCOPES: &[&str] = &["apps.app-registry:read", "apps.app-registry:write"]; +pub const REDIRECT_URI: &str = "http://localhost:7443/callback"; +pub const APP_ID: &str = "gddy"; + +struct Builtin { + name: &'static str, + api_url: &'static str, + client_id: &'static str, +} + +/// Public-safe, compiled-in environments. `api.ote-godaddy.com` is public, and +/// these client IDs are public OAuth identifiers (not secrets). +const BUILTINS: &[Builtin] = &[ + Builtin { + name: "ote", + api_url: "https://api.ote-godaddy.com", + client_id: "a502484b-d7b1-4509-aa88-08b391a54c28", + }, + Builtin { + name: "prod", + api_url: "https://api.godaddy.com", + client_id: "39489dee-4103-4284-9aab-9f2452142bce", + }, +]; + +/// A fully-resolved environment: everything needed to talk to it. +#[derive(Debug, Clone)] +pub struct ResolvedEnv { + pub name: String, + pub api_url: String, + pub client_id: String, + pub auth_url: String, + pub token_url: String, +} + +/// Schema of `~/.config/gddy/environments.toml`. +#[derive(Debug, Clone, Default, Deserialize)] +pub struct EnvironmentsFile { + #[serde(default)] + pub environments: BTreeMap, +} + +#[derive(Debug, Clone, Deserialize)] +pub struct EnvEntry { + pub api_url: String, + #[serde(default)] + pub client_id: Option, + #[serde(default)] + pub auth_url: Option, + #[serde(default)] + pub token_url: Option, +} + +#[derive(Debug, thiserror::Error)] +pub enum EnvError { + #[error("unknown environment {name:?}; known: {known}")] + Unknown { name: String, known: String }, + #[error("failed to read {path}: {source}")] + Io { + path: String, + source: std::io::Error, + }, + #[error("failed to parse {path}: {source}")] + Parse { + path: String, + source: toml::de::Error, + }, +} + +/// Environment-variable prefix for an env name, matching cli-engine's +/// `PkceAuthProvider` derivation (uppercase, `-` → `_`). +fn env_prefix(name: &str) -> String { + name.to_uppercase().replace('-', "_") +} + +fn derive_auth_url(api_url: &str) -> String { + format!("{}/v2/oauth2/authorize", api_url.trim_end_matches('/')) +} + +fn derive_token_url(api_url: &str) -> String { + format!("{}/v2/oauth2/token", api_url.trim_end_matches('/')) +} + +/// Path to the local environments config file, if a config dir can be resolved. +/// +/// Uses `dirs::config_dir()` which honors `XDG_CONFIG_HOME` (→ `~/.config`) on +/// Linux, matching cli-engine's own credential-store location logic. +pub fn environments_path() -> Option { + dirs::config_dir().map(|d| d.join("gddy").join("environments.toml")) +} + +/// Load the local environments file. A missing file is not an error. +fn load_file() -> Result { + let Some(path) = environments_path() else { + return Ok(EnvironmentsFile::default()); + }; + match std::fs::read_to_string(&path) { + Ok(contents) => toml::from_str(&contents).map_err(|source| EnvError::Parse { + path: path.display().to_string(), + source, + }), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(EnvironmentsFile::default()), + Err(source) => Err(EnvError::Io { + path: path.display().to_string(), + source, + }), + } +} + +fn builtin(name: &str) -> Option<&'static Builtin> { + BUILTINS.iter().find(|b| b.name == name) +} + +fn known_names(file: &EnvironmentsFile) -> String { + let mut names: Vec<&str> = BUILTINS.iter().map(|b| b.name).collect(); + names.extend(file.environments.keys().map(String::as_str)); + names.sort_unstable(); + names.dedup(); + names.join(", ") +} + +/// Resolve an environment from an explicit file + env-var getter. Pure: all +/// inputs are injected, so this is unit-testable without touching process state. +fn resolve_with( + name: &str, + file: &EnvironmentsFile, + var: impl Fn(&str) -> Option, +) -> Result { + // Layer 1: built-in base. + let (mut api_url, mut client_id) = match builtin(name) { + Some(b) => (Some(b.api_url.to_owned()), b.client_id.to_owned()), + None => (None, String::new()), + }; + let mut auth_url: Option = None; + let mut token_url: Option = None; + + // Layer 2: local config entry (overrides/defines). + if let Some(entry) = file.environments.get(name) { + api_url = Some(entry.api_url.clone()); + if let Some(cid) = &entry.client_id { + client_id = cid.clone(); + } + auth_url = entry.auth_url.clone(); + token_url = entry.token_url.clone(); + } + + // Layer 3: per-env `_API_URL` override (highest precedence). + if let Some(url) = var(&format!("{}_API_URL", env_prefix(name))) { + api_url = Some(url); + } + + let api_url = api_url.ok_or_else(|| EnvError::Unknown { + name: name.to_owned(), + known: known_names(file), + })?; + + let auth_url = auth_url.unwrap_or_else(|| derive_auth_url(&api_url)); + let token_url = token_url.unwrap_or_else(|| derive_token_url(&api_url)); + + Ok(ResolvedEnv { + name: name.to_owned(), + api_url, + client_id, + auth_url, + token_url, + }) +} + +/// Names listed by `env list`: built-ins + locally-configured entries only +/// (env-var-only environments are intentionally excluded). +fn listable_with( + file: &EnvironmentsFile, + var: impl Fn(&str) -> Option + Copy, +) -> Result, EnvError> { + let mut names: Vec = BUILTINS.iter().map(|b| b.name.to_owned()).collect(); + for key in file.environments.keys() { + if !names.iter().any(|n| n == key) { + names.push(key.clone()); + } + } + names.iter().map(|n| resolve_with(n, file, var)).collect() +} + +fn is_known_with( + name: &str, + file: &EnvironmentsFile, + var: impl Fn(&str) -> Option, +) -> bool { + builtin(name).is_some() + || file.environments.contains_key(name) + || var(&format!("{}_API_URL", env_prefix(name))).is_some() +} + +/// Resolve an environment by name (built-ins → local config → env var). +pub fn resolve(name: &str) -> Result { + let file = load_file()?; + resolve_with(name, &file, |k| std::env::var(k).ok()) +} + +/// The default environment's built-in API base URL. +/// +/// Infallible last-resort value (unlike [`resolve`], which can fail on a +/// malformed local config), so callers never end up with an empty base URL. +pub fn default_api_url() -> &'static str { + builtin(DEFAULT_ENV) + .map(|b| b.api_url) + .unwrap_or("https://api.godaddy.com") +} + +/// Environments to show in `env list`: built-ins + local-config entries. +pub fn listable() -> Result, EnvError> { + let file = load_file()?; + listable_with(&file, |k| std::env::var(k).ok()) +} + +/// Whether `name` is a usable environment (built-in, locally configured, or +/// defined via a `_API_URL` env var). +pub fn is_known(name: &str) -> bool { + match load_file() { + Ok(file) => is_known_with(name, &file, |k| std::env::var(k).ok()), + // If the config can't be read, fall back to built-ins + env vars so a + // broken file never hides the public environments. + Err(_) => { + builtin(name).is_some() + || std::env::var(format!("{}_API_URL", env_prefix(name))).is_ok() + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + fn entry(api_url: &str) -> EnvEntry { + EnvEntry { + api_url: api_url.to_owned(), + client_id: None, + auth_url: None, + token_url: None, + } + } + + fn no_vars(_: &str) -> Option { + None + } + + #[test] + fn builtin_resolves_with_derived_oauth_urls() { + let file = EnvironmentsFile::default(); + let env = resolve_with("prod", &file, no_vars).expect("prod resolves"); + assert_eq!(env.api_url, "https://api.godaddy.com"); + assert_eq!(env.client_id, "39489dee-4103-4284-9aab-9f2452142bce"); + assert_eq!(env.auth_url, "https://api.godaddy.com/v2/oauth2/authorize"); + assert_eq!(env.token_url, "https://api.godaddy.com/v2/oauth2/token"); + } + + #[test] + fn unknown_env_errors_with_known_list() { + let file = EnvironmentsFile::default(); + let err = resolve_with("nope", &file, no_vars).expect_err("unknown errors"); + let EnvError::Unknown { name, known } = err else { + unreachable!("expected Unknown variant"); + }; + assert_eq!(name, "nope"); + assert!(known.contains("ote") && known.contains("prod"), "{known}"); + } + + #[test] + fn env_var_defines_a_new_env() { + let file = EnvironmentsFile::default(); + let var = |k: &str| (k == "DEV_API_URL").then(|| "https://dev.example.test".to_owned()); + let env = resolve_with("dev", &file, var).expect("dev resolves from env var"); + assert_eq!(env.api_url, "https://dev.example.test"); + assert_eq!(env.auth_url, "https://dev.example.test/v2/oauth2/authorize"); + assert!(env.client_id.is_empty()); + } + + #[test] + fn env_var_overrides_a_builtin() { + let file = EnvironmentsFile::default(); + let var = + |k: &str| (k == "PROD_API_URL").then(|| "https://sandbox.example.test".to_owned()); + let env = resolve_with("prod", &file, var).expect("prod resolves"); + assert_eq!(env.api_url, "https://sandbox.example.test"); + // Client id is retained from the built-in. + assert_eq!(env.client_id, "39489dee-4103-4284-9aab-9f2452142bce"); + } + + #[test] + fn local_config_entry_resolves_and_respects_precedence() { + let mut file = EnvironmentsFile::default(); + file.environments + .insert("test".to_owned(), entry("https://test.example.invalid")); + // No env var: local config wins. + let env = resolve_with("test", &file, no_vars).expect("test resolves"); + assert_eq!(env.api_url, "https://test.example.invalid"); + // Env var present: overrides the local config api_url. + let var = + |k: &str| (k == "TEST_API_URL").then(|| "https://override.example.test".to_owned()); + let env = resolve_with("test", &file, var).expect("test resolves"); + assert_eq!(env.api_url, "https://override.example.test"); + } + + #[test] + fn explicit_oauth_urls_and_client_id_in_local_config() { + let mut file = EnvironmentsFile::default(); + file.environments.insert( + "dev".to_owned(), + EnvEntry { + api_url: "https://dev.example.invalid".to_owned(), + client_id: Some("custom-client".to_owned()), + auth_url: Some("https://auth.example.invalid/authorize".to_owned()), + token_url: Some("https://auth.example.invalid/token".to_owned()), + }, + ); + let env = resolve_with("dev", &file, no_vars).expect("dev resolves"); + assert_eq!(env.client_id, "custom-client"); + assert_eq!(env.auth_url, "https://auth.example.invalid/authorize"); + assert_eq!(env.token_url, "https://auth.example.invalid/token"); + } + + #[test] + fn oauth_urls_derive_from_custom_api_url_trimming_trailing_slash() { + let mut file = EnvironmentsFile::default(); + // Custom env, no explicit auth/token URLs, api_url has a trailing slash. + file.environments + .insert("dev".to_owned(), entry("https://dev.example.invalid/")); + let env = resolve_with("dev", &file, no_vars).expect("dev resolves"); + assert_eq!(env.api_url, "https://dev.example.invalid/"); + assert_eq!( + env.auth_url, + "https://dev.example.invalid/v2/oauth2/authorize" + ); + assert_eq!(env.token_url, "https://dev.example.invalid/v2/oauth2/token"); + } + + #[test] + fn default_api_url_is_the_builtin_prod_url() { + assert_eq!(default_api_url(), "https://api.godaddy.com"); + } + + #[test] + fn listable_includes_builtins_and_local_but_not_env_var_only() { + let mut file = EnvironmentsFile::default(); + file.environments + .insert("test".to_owned(), entry("https://test.example.invalid")); + // `foo` is defined only via an env var and must NOT appear in the list. + let var = |k: &str| (k == "FOO_API_URL").then(|| "https://foo.example.test".to_owned()); + let listed = listable_with(&file, var).expect("listable"); + let names: Vec<&str> = listed.iter().map(|e| e.name.as_str()).collect(); + assert!(names.contains(&"ote")); + assert!(names.contains(&"prod")); + assert!(names.contains(&"test")); + assert!(!names.contains(&"foo"), "env-var-only env leaked into list"); + } + + #[test] + fn is_known_covers_builtin_local_and_env_var() { + let mut file = EnvironmentsFile::default(); + file.environments + .insert("test".to_owned(), entry("https://test.example.invalid")); + let var = |k: &str| (k == "FOO_API_URL").then(|| "https://foo.example.test".to_owned()); + assert!(is_known_with("prod", &file, var)); // built-in + assert!(is_known_with("test", &file, var)); // local config + assert!(is_known_with("foo", &file, var)); // env var + assert!(!is_known_with("missing", &file, var)); + } + + #[test] + fn missing_config_file_is_not_an_error() { + // load_file resolves a real path; just assert it does not panic and that + // a default (empty) file resolves built-ins correctly via the public API. + assert!(is_known("prod")); + } +} diff --git a/rust/src/main.rs b/rust/src/main.rs index 1f76c00..a42f025 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -4,6 +4,7 @@ mod application; mod auth; mod config; mod env; +mod environments; mod extension; mod webhook; @@ -34,12 +35,16 @@ async fn main() -> ExitCode { .long("env") .global(true) .value_name("ENV") - .default_value(get_env().unwrap_or_else(|| env::DEFAULT_ENV.to_owned())) + .default_value( + get_env().unwrap_or_else(|| environments::DEFAULT_ENV.to_owned()), + ) .help("Target environment (ote|prod)"), ) })) .with_apply_flags(Arc::new(|matches, mw| { if let Some(env) = matches.get_one::("env") { + environments::resolve(env) + .map_err(|e| cli_engine::CliCoreError::message(e.to_string()))?; mw.env = env.clone(); } Ok(()) From c9267bd51243ac79d1b8f58eb152481d043c6b5b Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 13:02:41 -0700 Subject: [PATCH 02/16] fix: address Copilot review on environments + scope step-up - main.rs: validate --env only when explicitly passed, so a malformed ~/.config/gddy/environments.toml no longer fails every command (the default env comes from .gdenv/DEFAULT_ENV and is already filtered). - environments: normalize api_url (trim trailing slash) once during resolution so callers concatenating paths never produce '//'; update the test. - auth.rs: list_environments propagates a malformed-config error instead of silently reporting zero environments. Co-Authored-By: Claude Opus 4.8 --- rust/src/auth.rs | 6 +++++- rust/src/environments/mod.rs | 6 +++++- rust/src/main.rs | 10 ++++++++-- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/rust/src/auth.rs b/rust/src/auth.rs index d09c0bf..cba8e91 100644 --- a/rust/src/auth.rs +++ b/rust/src/auth.rs @@ -79,8 +79,12 @@ impl AuthProvider for GoDaddyAuthProvider { // Enumerate stored credentials across built-ins + locally-configured // envs (env-var-only envs are excluded from `listable`, matching the // `env list` contract). + // Surface a malformed local config rather than silently reporting zero + // environments (which would hide stored credentials, incl. built-ins). + let listable = + environments::listable().map_err(|e| CliCoreError::message(e.to_string()))?; let mut envs = Vec::new(); - for resolved in environments::listable().unwrap_or_default() { + for resolved in listable { let provider = build_provider(&resolved); envs.extend(provider.list_environments().await.unwrap_or_default()); } diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index b06ba35..3136884 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -187,6 +187,9 @@ fn resolve_with( name: name.to_owned(), known: known_names(file), })?; + // Normalize once so callers that concatenate (`{api_url}{endpoint}`, + // `{api_url}/v1/...`) never produce a `//` path segment. + let api_url = api_url.trim_end_matches('/').to_owned(); let auth_url = auth_url.unwrap_or_else(|| derive_auth_url(&api_url)); let token_url = token_url.unwrap_or_else(|| derive_token_url(&api_url)); @@ -360,7 +363,8 @@ mod tests { file.environments .insert("dev".to_owned(), entry("https://dev.example.invalid/")); let env = resolve_with("dev", &file, no_vars).expect("dev resolves"); - assert_eq!(env.api_url, "https://dev.example.invalid/"); + // api_url is normalized (trailing slash trimmed) so callers don't build `//`. + assert_eq!(env.api_url, "https://dev.example.invalid"); assert_eq!( env.auth_url, "https://dev.example.invalid/v2/oauth2/authorize" diff --git a/rust/src/main.rs b/rust/src/main.rs index a42f025..5a17245 100644 --- a/rust/src/main.rs +++ b/rust/src/main.rs @@ -43,8 +43,14 @@ async fn main() -> ExitCode { })) .with_apply_flags(Arc::new(|matches, mw| { if let Some(env) = matches.get_one::("env") { - environments::resolve(env) - .map_err(|e| cli_engine::CliCoreError::message(e.to_string()))?; + // Validate only an explicitly-provided `--env`. The default + // value comes from `.gdenv`/DEFAULT_ENV (already filtered), so + // validating it unconditionally would let a malformed local + // config fail *every* command — even ones not using `--env`. + if matches.value_source("env") == Some(clap::parser::ValueSource::CommandLine) { + environments::resolve(env) + .map_err(|e| cli_engine::CliCoreError::message(e.to_string()))?; + } mw.env = env.clone(); } Ok(()) From 89eece4e2a5fcec4e1d864cc56d2f919c783b7ed Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 13:11:25 -0700 Subject: [PATCH 03/16] fix: address Copilot round 2 on PR #57 - api call --scope: use ArgAction::Append (one value per occurrence) instead of num_args(1..), which could greedily consume the required ENDPOINT positional. - client.rs test: assert builtins resolve to a URL rather than exact hosts (built-ins are overridable; exact mapping covered in environments::tests). - env list test: accept any scheme (http/https) for apiUrl shape. Co-Authored-By: Claude Opus 4.8 --- rust/src/api_explorer/mod.rs | 7 ++++--- rust/src/application/client.rs | 12 +++++++++--- rust/src/env/mod.rs | 6 ++---- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 334328a..05e7c51 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -444,9 +444,10 @@ fn call_command() -> RuntimeCommandSpec { .long("scope") .short('s') .value_name("SCOPE") - // Require a value per occurrence so a bare `--scope` is - // rejected rather than silently contributing nothing. - .num_args(1..) + // One value per occurrence, repeatable (`--scope a --scope b`). + // Append (vs num_args(1..)) avoids greedily consuming the + // ENDPOINT positional and still rejects a bare `--scope`. + .action(clap::ArgAction::Append) .help("Additional required OAuth scope(s), merged with the endpoint's"), ), |ctx| async move { diff --git a/rust/src/application/client.rs b/rust/src/application/client.rs index 8311b21..6fb11b7 100644 --- a/rust/src/application/client.rs +++ b/rust/src/application/client.rs @@ -200,9 +200,15 @@ mod tests { use super::api_url_for_env; #[test] - fn api_url_for_builtins() { - assert_eq!(api_url_for_env("prod"), "https://api.godaddy.com"); - assert_eq!(api_url_for_env("ote"), "https://api.ote-godaddy.com"); + fn api_url_for_builtins_resolve_to_a_url() { + // The exact host mapping is covered deterministically in + // `environments::tests`. Here we only assert the built-ins resolve to a + // URL — a dev machine may legitimately override a built-in's URL via + // env var / local config, so don't hard-code the host. + for env in ["prod", "ote"] { + let url = api_url_for_env(env); + assert!(url.contains("://"), "{env} -> {url:?}"); + } } #[test] diff --git a/rust/src/env/mod.rs b/rust/src/env/mod.rs index 948f2ff..6fe50d3 100644 --- a/rust/src/env/mod.rs +++ b/rust/src/env/mod.rs @@ -176,10 +176,8 @@ mod tests { "active should be bool: {entry}" ); assert!( - entry["apiUrl"] - .as_str() - .is_some_and(|u| u.starts_with("https://")), - "apiUrl should be an https URL: {entry}" + entry["apiUrl"].as_str().is_some_and(|u| u.contains("://")), + "apiUrl should be a URL: {entry}" ); } } From 183463745fcf0ccebbf65636c919e943ca9144ae Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 13:19:29 -0700 Subject: [PATCH 04/16] fix: address Copilot round 3 on PR #57 - merge_required_scopes de-dupes repeated --scope flag values too (not just endpoint-vs-flags); added a test. - environments::resolve falls back to built-ins + _API_URL env vars when the optional local config is malformed/unreadable, so a broken file no longer bricks auth-required commands targeting prod/ote; the load error still surfaces for envs that actually need the file. - client.rs test asserts URL shape ('://') rather than the https scheme. Co-Authored-By: Claude Opus 4.8 --- rust/src/api_explorer/mod.rs | 20 ++++++++++++++++---- rust/src/application/client.rs | 4 +++- rust/src/environments/mod.rs | 13 +++++++++++-- 3 files changed, 30 insertions(+), 7 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 05e7c51..e6a02ce 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -139,10 +139,14 @@ fn find_endpoint<'a>(catalog: &'a [Domain], query: &str) -> Option<(&'a Domain, /// Union of user-supplied `--scope` flags and a matched endpoint's declared /// scopes, order-preserving and de-duplicated (flags first). fn merge_required_scopes(flag_scopes: Vec, endpoint_scopes: &[String]) -> Vec { - let mut required = flag_scopes; - for scope in endpoint_scopes { - if !required.contains(scope) { - required.push(scope.clone()); + let mut required: Vec = Vec::new(); + // De-dup across both sources (a user can repeat `--scope`), flags first. + for scope in flag_scopes + .into_iter() + .chain(endpoint_scopes.iter().cloned()) + { + if !required.contains(&scope) { + required.push(scope); } } required @@ -619,4 +623,12 @@ mod tests { v(&["a", "b", "c"]) ); } + + #[test] + fn merge_dedupes_repeated_flag_values() { + assert_eq!( + merge_required_scopes(v(&["a", "a", "b"]), &v(&["b"])), + v(&["a", "b"]) + ); + } } diff --git a/rust/src/application/client.rs b/rust/src/application/client.rs index 6fb11b7..9005b7b 100644 --- a/rust/src/application/client.rs +++ b/rust/src/application/client.rs @@ -216,6 +216,8 @@ mod tests { // Unknown env resolves to the default environment's URL (never empty). let url = api_url_for_env("definitely-not-a-real-env-xyz"); assert!(!url.is_empty()); - assert!(url.starts_with("https://")); + // Don't hard-code the scheme: a built-in's URL is overridable (a dev + // may point the default at an http:// local proxy). + assert!(url.contains("://"), "{url:?}"); } } diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 3136884..b40bb98 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -230,8 +230,17 @@ fn is_known_with( /// Resolve an environment by name (built-ins → local config → env var). pub fn resolve(name: &str) -> Result { - let file = load_file()?; - resolve_with(name, &file, |k| std::env::var(k).ok()) + match load_file() { + Ok(file) => resolve_with(name, &file, |k| std::env::var(k).ok()), + // The local config is optional; a malformed/unreadable file must not + // brick built-in or `_API_URL`-defined envs. Retry against an + // empty file, and only surface the load error if `name` actually needed + // the file to resolve. + Err(load_err) => { + let empty = EnvironmentsFile::default(); + resolve_with(name, &empty, |k| std::env::var(k).ok()).map_err(|_| load_err) + } + } } /// The default environment's built-in API base URL. From e9f28b513c03660f99c402db6829e86e136d5e49 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 13:31:03 -0700 Subject: [PATCH 05/16] fix: reject empty/whitespace api_url overrides (Copilot round 4) A blank api_url (from PROD_API_URL="" or a blank config entry) could clobber a built-in default and yield relative request/OAuth URLs, and made is_known accept an unusable env. Add clean_url (trim whitespace + trailing slash, None if empty); resolve ignores empty overrides; is_known (incl. the malformed-config fallback) requires a non-empty api_url. Added tests. Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 62 +++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 12 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index b40bb98..fe286d0 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -115,6 +115,14 @@ fn derive_token_url(api_url: &str) -> String { format!("{}/v2/oauth2/token", api_url.trim_end_matches('/')) } +/// Normalizes a candidate API base URL: trims surrounding whitespace and any +/// trailing slash. Returns `None` for an empty/whitespace value, so a blank +/// override never clobbers a built-in or yields relative request URLs. +fn clean_url(raw: &str) -> Option { + let trimmed = raw.trim().trim_end_matches('/'); + (!trimmed.is_empty()).then(|| trimmed.to_owned()) +} + /// Path to the local environments config file, if a config dir can be resolved. /// /// Uses `dirs::config_dir()` which honors `XDG_CONFIG_HOME` (→ `~/.config`) on @@ -168,28 +176,31 @@ fn resolve_with( let mut auth_url: Option = None; let mut token_url: Option = None; - // Layer 2: local config entry (overrides/defines). + // Layer 2: local config entry (overrides/defines). An empty/whitespace + // api_url is ignored so it can't clobber a built-in default. if let Some(entry) = file.environments.get(name) { - api_url = Some(entry.api_url.clone()); + if let Some(url) = clean_url(&entry.api_url) { + api_url = Some(url); + } if let Some(cid) = &entry.client_id { client_id = cid.clone(); } - auth_url = entry.auth_url.clone(); - token_url = entry.token_url.clone(); + auth_url = entry.auth_url.clone().filter(|u| !u.trim().is_empty()); + token_url = entry.token_url.clone().filter(|u| !u.trim().is_empty()); } - // Layer 3: per-env `_API_URL` override (highest precedence). - if let Some(url) = var(&format!("{}_API_URL", env_prefix(name))) { + // Layer 3: per-env `_API_URL` override (highest precedence). Empty + // values are ignored (handled by clean_url). + if let Some(url) = var(&format!("{}_API_URL", env_prefix(name))).and_then(|v| clean_url(&v)) { api_url = Some(url); } + // api_url is already trimmed/normalized by clean_url (and built-ins carry no + // trailing slash), so callers concatenating paths never produce `//`. let api_url = api_url.ok_or_else(|| EnvError::Unknown { name: name.to_owned(), known: known_names(file), })?; - // Normalize once so callers that concatenate (`{api_url}{endpoint}`, - // `{api_url}/v1/...`) never produce a `//` path segment. - let api_url = api_url.trim_end_matches('/').to_owned(); let auth_url = auth_url.unwrap_or_else(|| derive_auth_url(&api_url)); let token_url = token_url.unwrap_or_else(|| derive_token_url(&api_url)); @@ -224,8 +235,11 @@ fn is_known_with( var: impl Fn(&str) -> Option, ) -> bool { builtin(name).is_some() - || file.environments.contains_key(name) - || var(&format!("{}_API_URL", env_prefix(name))).is_some() + || file + .environments + .get(name) + .is_some_and(|e| !e.api_url.trim().is_empty()) + || var(&format!("{}_API_URL", env_prefix(name))).is_some_and(|v| !v.trim().is_empty()) } /// Resolve an environment by name (built-ins → local config → env var). @@ -268,7 +282,8 @@ pub fn is_known(name: &str) -> bool { // broken file never hides the public environments. Err(_) => { builtin(name).is_some() - || std::env::var(format!("{}_API_URL", env_prefix(name))).is_ok() + || std::env::var(format!("{}_API_URL", env_prefix(name))) + .is_ok_and(|v| !v.trim().is_empty()) } } } @@ -386,6 +401,29 @@ mod tests { assert_eq!(default_api_url(), "https://api.godaddy.com"); } + #[test] + fn empty_or_whitespace_override_does_not_clobber_builtin() { + let file = EnvironmentsFile::default(); + let var = |k: &str| (k == "PROD_API_URL").then(|| " ".to_owned()); + let env = resolve_with("prod", &file, var).expect("prod resolves"); + assert_eq!(env.api_url, "https://api.godaddy.com"); + } + + #[test] + fn is_known_rejects_empty_api_url_from_config_or_env() { + let mut file = EnvironmentsFile::default(); + file.environments.insert("blank".to_owned(), entry("")); + // Empty config api_url -> not usable -> not known. + assert!(!is_known_with("blank", &file, no_vars)); + // Empty env-var api_url -> not known either. + let blank_var = |k: &str| (k == "GHOST_API_URL").then(String::new); + assert!(!is_known_with( + "ghost", + &EnvironmentsFile::default(), + blank_var + )); + } + #[test] fn listable_includes_builtins_and_local_but_not_env_var_only() { let mut file = EnvironmentsFile::default(); From 28e584d200cb3a0e903a1a353aaf66c0e5058116 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 13:37:00 -0700 Subject: [PATCH 06/16] fix: exclude empty-api_url config entries from known list + env list (round 5) Consistency with the round-4 empty-api_url handling: known_names no longer advertises unusable entries, and listable_with skips them so one invalid entry can't fail env list / credential enumeration wholesale. Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index fe286d0..8f27bba 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -155,7 +155,14 @@ fn builtin(name: &str) -> Option<&'static Builtin> { fn known_names(file: &EnvironmentsFile) -> String { let mut names: Vec<&str> = BUILTINS.iter().map(|b| b.name).collect(); - names.extend(file.environments.keys().map(String::as_str)); + // Only usable config entries (non-empty api_url) — match `is_known`, so the + // "known: …" list never advertises an env that can't actually resolve. + names.extend( + file.environments + .iter() + .filter(|(_, e)| !e.api_url.trim().is_empty()) + .map(|(k, _)| k.as_str()), + ); names.sort_unstable(); names.dedup(); names.join(", ") @@ -221,8 +228,10 @@ fn listable_with( var: impl Fn(&str) -> Option + Copy, ) -> Result, EnvError> { let mut names: Vec = BUILTINS.iter().map(|b| b.name.to_owned()).collect(); - for key in file.environments.keys() { - if !names.iter().any(|n| n == key) { + for (key, entry) in &file.environments { + // Skip unusable entries (empty api_url): including one would make the + // whole `env list` / credential enumeration fail on a single bad entry. + if !entry.api_url.trim().is_empty() && !names.iter().any(|n| n == key) { names.push(key.clone()); } } From f8c599f4f63bf68fc1ca099870450b916bba204e Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 14:08:55 -0700 Subject: [PATCH 07/16] fix: require http(s):// scheme for api_url overrides (Copilot round 6) clean_url is now the single validity authority: it requires an http(s):// scheme with a non-empty host (reqwest needs absolute URLs). resolve, is_known, known_names, and listable_with all defer to it, so a present-but-schemeless api_url is treated as unusable consistently (rejected, not advertised, and never fails env list / enumeration wholesale). Added a test. Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 48 +++++++++++++++++++++++++++--------- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 8f27bba..82d23a5 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -115,12 +115,20 @@ fn derive_token_url(api_url: &str) -> String { format!("{}/v2/oauth2/token", api_url.trim_end_matches('/')) } -/// Normalizes a candidate API base URL: trims surrounding whitespace and any -/// trailing slash. Returns `None` for an empty/whitespace value, so a blank -/// override never clobbers a built-in or yields relative request URLs. +/// Validates and normalizes a candidate API base URL: trims surrounding +/// whitespace and any trailing slash, and requires an `http(s)://` scheme +/// (reqwest needs an absolute URL). Returns `None` for an empty/whitespace or +/// schemeless value, so a blank or malformed override never clobbers a built-in +/// or yields a relative/unusable request URL. This is the single authority for +/// "is this api_url usable?" — `resolve`, `is_known`, `known_names`, and +/// `listable` all defer to it. fn clean_url(raw: &str) -> Option { let trimmed = raw.trim().trim_end_matches('/'); - (!trimmed.is_empty()).then(|| trimmed.to_owned()) + // Require an http(s):// scheme followed by a non-empty host. + let host = trimmed + .strip_prefix("https://") + .or_else(|| trimmed.strip_prefix("http://"))?; + (!host.is_empty()).then(|| trimmed.to_owned()) } /// Path to the local environments config file, if a config dir can be resolved. @@ -160,7 +168,7 @@ fn known_names(file: &EnvironmentsFile) -> String { names.extend( file.environments .iter() - .filter(|(_, e)| !e.api_url.trim().is_empty()) + .filter(|(_, e)| clean_url(&e.api_url).is_some()) .map(|(k, _)| k.as_str()), ); names.sort_unstable(); @@ -229,9 +237,10 @@ fn listable_with( ) -> Result, EnvError> { let mut names: Vec = BUILTINS.iter().map(|b| b.name.to_owned()).collect(); for (key, entry) in &file.environments { - // Skip unusable entries (empty api_url): including one would make the - // whole `env list` / credential enumeration fail on a single bad entry. - if !entry.api_url.trim().is_empty() && !names.iter().any(|n| n == key) { + // Skip unusable entries (empty or schemeless api_url): including one + // would make the whole `env list` / credential enumeration fail on a + // single bad entry. + if clean_url(&entry.api_url).is_some() && !names.iter().any(|n| n == key) { names.push(key.clone()); } } @@ -247,8 +256,8 @@ fn is_known_with( || file .environments .get(name) - .is_some_and(|e| !e.api_url.trim().is_empty()) - || var(&format!("{}_API_URL", env_prefix(name))).is_some_and(|v| !v.trim().is_empty()) + .is_some_and(|e| clean_url(&e.api_url).is_some()) + || var(&format!("{}_API_URL", env_prefix(name))).is_some_and(|v| clean_url(&v).is_some()) } /// Resolve an environment by name (built-ins → local config → env var). @@ -292,7 +301,9 @@ pub fn is_known(name: &str) -> bool { Err(_) => { builtin(name).is_some() || std::env::var(format!("{}_API_URL", env_prefix(name))) - .is_ok_and(|v| !v.trim().is_empty()) + .ok() + .and_then(|v| clean_url(&v)) + .is_some() } } } @@ -433,6 +444,21 @@ mod tests { )); } + #[test] + fn schemeless_api_url_is_rejected_but_http_is_accepted() { + let mut file = EnvironmentsFile::default(); + // No http(s):// scheme -> unusable -> not known, and resolve errors. + file.environments + .insert("bare".to_owned(), entry("api.dev.invalid")); + assert!(!is_known_with("bare", &file, no_vars)); + assert!(resolve_with("bare", &file, no_vars).is_err()); + // http:// (e.g. a local proxy) is accepted. + file.environments + .insert("local".to_owned(), entry("http://localhost:8080")); + let env = resolve_with("local", &file, no_vars).expect("http accepted"); + assert_eq!(env.api_url, "http://localhost:8080"); + } + #[test] fn listable_includes_builtins_and_local_but_not_env_var_only() { let mut file = EnvironmentsFile::default(); From 717e5ad01ccd7900f1b740f30442c9a14b84edff Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 14:17:57 -0700 Subject: [PATCH 08/16] fix: validate auth/token overrides + show active env in list (round 7) - resolve runs auth_url/token_url config overrides through clean_url (scheme validation), so an invalid override falls back to the derived endpoints rather than failing later with an opaque reqwest error. - env list injects the active env when it's defined only via _API_URL (and thus excluded from listable), so the list always has an active:true entry. Co-Authored-By: Claude Opus 4.8 --- rust/src/env/mod.rs | 12 +++++++++-- rust/src/environments/mod.rs | 41 ++++++++++++++++++++++++++++-------- 2 files changed, 42 insertions(+), 11 deletions(-) diff --git a/rust/src/env/mod.rs b/rust/src/env/mod.rs index 6fe50d3..28c5a39 100644 --- a/rust/src/env/mod.rs +++ b/rust/src/env/mod.rs @@ -61,8 +61,16 @@ pub fn module() -> Module { .no_auth(true), |_cred, _args| async move { let current = active_env(); - let envs: Vec<_> = environments::listable() - .map_err(map_err)? + let mut resolved = environments::listable().map_err(map_err)?; + // If the active env is defined only via `_API_URL` (so it's + // excluded from `listable`), still show it — otherwise the list + // has no `active: true` entry and callers can't tell what's active. + if !resolved.iter().any(|e| e.name == current) + && let Ok(active) = environments::resolve(¤t) + { + resolved.push(active); + } + let envs: Vec<_> = resolved .into_iter() .map(|e| { json!({ diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 82d23a5..0ef19b3 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -115,13 +115,13 @@ fn derive_token_url(api_url: &str) -> String { format!("{}/v2/oauth2/token", api_url.trim_end_matches('/')) } -/// Validates and normalizes a candidate API base URL: trims surrounding -/// whitespace and any trailing slash, and requires an `http(s)://` scheme -/// (reqwest needs an absolute URL). Returns `None` for an empty/whitespace or -/// schemeless value, so a blank or malformed override never clobbers a built-in -/// or yields a relative/unusable request URL. This is the single authority for -/// "is this api_url usable?" — `resolve`, `is_known`, `known_names`, and -/// `listable` all defer to it. +/// Validates and normalizes a candidate URL (api/auth/token): trims surrounding +/// whitespace and any trailing slash, and requires an `http(s)://` scheme with a +/// non-empty host (reqwest needs an absolute URL). Returns `None` for an +/// empty/whitespace or schemeless value, so a blank or malformed override never +/// clobbers a built-in or yields a relative/unusable URL. This is the single +/// authority for "is this URL usable?" — `resolve`, `is_known`, `known_names`, +/// and `listable` all defer to it. fn clean_url(raw: &str) -> Option { let trimmed = raw.trim().trim_end_matches('/'); // Require an http(s):// scheme followed by a non-empty host. @@ -200,8 +200,10 @@ fn resolve_with( if let Some(cid) = &entry.client_id { client_id = cid.clone(); } - auth_url = entry.auth_url.clone().filter(|u| !u.trim().is_empty()); - token_url = entry.token_url.clone().filter(|u| !u.trim().is_empty()); + // Validate auth/token overrides the same way as api_url: a schemeless or + // empty value is ignored, falling back to the derived endpoints. + auth_url = entry.auth_url.as_deref().and_then(clean_url); + token_url = entry.token_url.as_deref().and_then(clean_url); } // Layer 3: per-env `_API_URL` override (highest precedence). Empty @@ -444,6 +446,27 @@ mod tests { )); } + #[test] + fn schemeless_auth_token_overrides_fall_back_to_derived() { + let mut file = EnvironmentsFile::default(); + file.environments.insert( + "dev".to_owned(), + EnvEntry { + api_url: "https://dev.example.invalid".to_owned(), + client_id: None, + auth_url: Some("auth.example.invalid/authorize".to_owned()), // no scheme + token_url: Some(" ".to_owned()), // blank + }, + ); + let env = resolve_with("dev", &file, no_vars).expect("dev resolves"); + // Invalid/blank overrides are ignored; endpoints derive from api_url. + assert_eq!( + env.auth_url, + "https://dev.example.invalid/v2/oauth2/authorize" + ); + assert_eq!(env.token_url, "https://dev.example.invalid/v2/oauth2/token"); + } + #[test] fn schemeless_api_url_is_rejected_but_http_is_accepted() { let mut file = EnvironmentsFile::default(); From 7735e765f2912e3874024cbb30cce3137b182573 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 15:11:48 -0700 Subject: [PATCH 09/16] fix: single --scope/--field value silently dropped (jgowdy review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BLOCKING: cli-engine serializes a single repeatable-arg occurrence to a scalar Value::String (only 2+ become Value::Array), so `api call --scope foo` matched .as_array()==None and dropped the scope — no step-up, omitted from the 403 hint. Add string_list() handling both shapes; use it for --scope and the same pre-existing --field defect. Regression test at the extraction layer. Also from the review: - listable() now falls back to built-ins (with a warning) on a malformed config, consistent with resolve/is_known (no longer bricks env list / auth for prod/ote). - clean_url requires a non-empty host (rejects https:///path), matching its doc. Co-Authored-By: Claude Opus 4.8 --- rust/src/api_explorer/mod.rs | 67 ++++++++++++++++++++++-------------- rust/src/environments/mod.rs | 29 ++++++++++++++-- 2 files changed, 68 insertions(+), 28 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index e6a02ce..312cdb0 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -136,6 +136,21 @@ fn find_endpoint<'a>(catalog: &'a [Domain], query: &str) -> Option<(&'a Domain, }) } +/// Reads a repeatable string argument, handling both shapes cli-engine +/// produces: a single occurrence is collapsed to a scalar `Value::String`, and +/// only two-or-more become a `Value::Array`. Matching only the array shape +/// silently drops a lone `--scope`/`--field` value, so handle both. +fn string_list(args: &serde_json::Map, key: &str) -> Vec { + match args.get(key) { + Some(Value::Array(arr)) => arr + .iter() + .filter_map(|v| v.as_str().map(str::to_owned)) + .collect(), + Some(Value::String(s)) => vec![s.clone()], + _ => Vec::new(), + } +} + /// Union of user-supplied `--scope` flags and a matched endpoint's declared /// scopes, order-preserving and de-duplicated (flags first). fn merge_required_scopes(flag_scopes: Vec, endpoint_scopes: &[String]) -> Vec { @@ -469,16 +484,7 @@ fn call_command() -> RuntimeCommandSpec { // endpoint's declared scopes (best-effort: a concrete request path // may not match a templated catalog path, in which case only --scope // contributes). These drive OAuth scope step-up at credential time. - let flag_scopes: Vec = ctx - .args - .get("scope") - .and_then(|v| v.as_array()) - .map(|arr| { - arr.iter() - .filter_map(|v| v.as_str().map(str::to_owned)) - .collect() - }) - .unwrap_or_default(); + let flag_scopes = string_list(&ctx.args, "scope"); let endpoint_scopes = find_endpoint(catalog(), endpoint) .map(|(_, ep)| ep.scopes.as_slice()) .unwrap_or(&[]); @@ -506,22 +512,19 @@ fn call_command() -> RuntimeCommandSpec { })?); } - if let Some(fields) = ctx.args.get("field").and_then(|v| v.as_array()) - && !fields.is_empty() - { + let fields = string_list(&ctx.args, "field"); + if !fields.is_empty() { let body = request_body.get_or_insert_with(|| json!({})); - for field in fields { - if let Some(s) = field.as_str() { - let eq = s.find('=').ok_or_else(|| { - cli_engine::CliCoreError::message(format!( - "invalid field format '{s}': expected key=value" - )) - })?; - let key = s[..eq].to_owned(); - let val = s[eq + 1..].to_owned(); - if let Some(obj) = body.as_object_mut() { - obj.insert(key, json!(val)); - } + for s in &fields { + let eq = s.find('=').ok_or_else(|| { + cli_engine::CliCoreError::message(format!( + "invalid field format '{s}': expected key=value" + )) + })?; + let key = s[..eq].to_owned(); + let val = s[eq + 1..].to_owned(); + if let Some(obj) = body.as_object_mut() { + obj.insert(key, json!(val)); } } } @@ -631,4 +634,18 @@ mod tests { v(&["a", "b"]) ); } + + #[test] + fn string_list_handles_scalar_array_and_missing() { + use serde_json::json; + // A single occurrence serializes to a scalar String (the bug case). + let mut args = serde_json::Map::new(); + args.insert("scope".to_owned(), json!("solo")); + assert_eq!(super::string_list(&args, "scope"), v(&["solo"])); + // Two-or-more serialize to an array. + args.insert("scope".to_owned(), json!(["a", "b"])); + assert_eq!(super::string_list(&args, "scope"), v(&["a", "b"])); + // Missing key. + assert!(super::string_list(&args, "absent").is_empty()); + } } diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 0ef19b3..17d35d1 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -124,10 +124,12 @@ fn derive_token_url(api_url: &str) -> String { /// and `listable` all defer to it. fn clean_url(raw: &str) -> Option { let trimmed = raw.trim().trim_end_matches('/'); - // Require an http(s):// scheme followed by a non-empty host. - let host = trimmed + // Require an http(s):// scheme followed by a non-empty host (the segment + // before the first '/'), so e.g. `https:///path` is rejected. + let after_scheme = trimmed .strip_prefix("https://") .or_else(|| trimmed.strip_prefix("http://"))?; + let host = after_scheme.split('/').next().unwrap_or(""); (!host.is_empty()).then(|| trimmed.to_owned()) } @@ -289,7 +291,16 @@ pub fn default_api_url() -> &'static str { /// Environments to show in `env list`: built-ins + local-config entries. pub fn listable() -> Result, EnvError> { - let file = load_file()?; + // Consistent with `resolve`/`is_known`: a malformed optional config must not + // brick `env list` / credential enumeration for built-ins. Fall back to + // built-ins only (warning) rather than failing wholesale. + let file = load_file().unwrap_or_else(|err| { + tracing::warn!( + error = %err, + "ignoring unreadable ~/.config/gddy/environments.toml; listing built-ins only" + ); + EnvironmentsFile::default() + }); listable_with(&file, |k| std::env::var(k).ok()) } @@ -482,6 +493,18 @@ mod tests { assert_eq!(env.api_url, "http://localhost:8080"); } + #[test] + fn clean_url_requires_a_non_empty_host() { + assert_eq!( + clean_url("https://api.example.test/"), + Some("https://api.example.test".to_owned()) + ); + assert!(clean_url("https:///path").is_none()); // empty host + assert!(clean_url("https://").is_none()); + assert!(clean_url("ftp://x").is_none()); // wrong scheme + assert!(clean_url("api.example.test").is_none()); // no scheme + } + #[test] fn listable_includes_builtins_and_local_but_not_env_var_only() { let mut file = EnvironmentsFile::default(); From 85a8db35854e81aa3c87a98918749568eb37e566 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 15:24:02 -0700 Subject: [PATCH 10/16] fix: clean_url rejects empty/query-only hosts (Copilot) Validate the host as the segment before any path/query/fragment and require it non-empty, so https:///path, https://, and https://?x are rejected. (Used a delimiter-aware string check rather than reqwest::Url, which accepted https:///path.) Added edge-case tests. Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 17d35d1..668bf4b 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -124,12 +124,13 @@ fn derive_token_url(api_url: &str) -> String { /// and `listable` all defer to it. fn clean_url(raw: &str) -> Option { let trimmed = raw.trim().trim_end_matches('/'); - // Require an http(s):// scheme followed by a non-empty host (the segment - // before the first '/'), so e.g. `https:///path` is rejected. + // Require an http(s):// scheme and a non-empty host. The host is the segment + // before any path/query/fragment, so this rejects `https:///path`, + // `https://`, and `https://?x` (which a lenient URL parser would accept). let after_scheme = trimmed .strip_prefix("https://") .or_else(|| trimmed.strip_prefix("http://"))?; - let host = after_scheme.split('/').next().unwrap_or(""); + let host = after_scheme.split(['/', '?', '#']).next().unwrap_or(""); (!host.is_empty()).then(|| trimmed.to_owned()) } @@ -501,8 +502,15 @@ mod tests { ); assert!(clean_url("https:///path").is_none()); // empty host assert!(clean_url("https://").is_none()); + assert!(clean_url("https://?x").is_none()); // query, no host assert!(clean_url("ftp://x").is_none()); // wrong scheme assert!(clean_url("api.example.test").is_none()); // no scheme + assert!(clean_url("not a url").is_none()); + // A path/port is preserved (trailing slash trimmed). + assert_eq!( + clean_url("http://localhost:8080/api/"), + Some("http://localhost:8080/api".to_owned()) + ); } #[test] From f894447ae9096d7b8ce95cc0d1f9683e223a589a Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 15:41:47 -0700 Subject: [PATCH 11/16] fix: don't hard-code config path in listable warning (Copilot) environments_path() resolves per-OS (XDG/%APPDATA%/macOS); the EnvError already carries the resolved path, so the warning no longer bakes in ~/.config/... Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 668bf4b..d1e0269 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -296,10 +296,9 @@ pub fn listable() -> Result, EnvError> { // brick `env list` / credential enumeration for built-ins. Fall back to // built-ins only (warning) rather than failing wholesale. let file = load_file().unwrap_or_else(|err| { - tracing::warn!( - error = %err, - "ignoring unreadable ~/.config/gddy/environments.toml; listing built-ins only" - ); + // `err` already includes the OS-resolved config path (Io/Parse carry it), + // so don't hard-code `~/.config/...` (wrong on Windows/macOS/XDG). + tracing::warn!(error = %err, "ignoring unreadable environments config; listing built-ins only"); EnvironmentsFile::default() }); listable_with(&file, |k| std::env::var(k).ok()) From 35b5544737adad2582e29c9c89418274198bbdd3 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 15:46:20 -0700 Subject: [PATCH 12/16] fix: 403 hint repeats --scope per scope (Copilot) auth login --scope is append-style (one value per flag), so the api call 403 hint now suggests `auth login --scope a --scope b` rather than space-joining. Co-Authored-By: Claude Opus 4.8 --- rust/src/api_explorer/mod.rs | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index 312cdb0..aeedac3 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -571,11 +571,17 @@ fn call_command() -> RuntimeCommandSpec { // `required`). A 403 here means the granted token still lacks a // required scope — surface it rather than silently returning the body. if status == 403 && !required.is_empty() { + // `auth login --scope` is append-style (one value per flag), so + // repeat the flag rather than space-joining. + let login_hint = required + .iter() + .map(|s| format!("--scope {s}")) + .collect::>() + .join(" "); return Err(cli_engine::CliCoreError::message(format!( "403 Forbidden — the authorized token is missing required scope(s): {}. \ - Re-run `gddy auth login --scope {}` and try again.", + Re-run `gddy auth login {login_hint}` and try again.", required.join(", "), - required.join(" ") ))); } From 399aa064d1f40aedd256931eaea01cd8281113ea Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 16:10:49 -0700 Subject: [PATCH 13/16] docs: fix stale list_environments comment (Copilot) listable() falls back to built-ins (with a warning) on a malformed config; the comment no longer claims the error is surfaced. Co-Authored-By: Claude Opus 4.8 --- rust/src/auth.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/rust/src/auth.rs b/rust/src/auth.rs index cba8e91..07f2f8a 100644 --- a/rust/src/auth.rs +++ b/rust/src/auth.rs @@ -78,9 +78,8 @@ impl AuthProvider for GoDaddyAuthProvider { async fn list_environments(&self) -> Result> { // Enumerate stored credentials across built-ins + locally-configured // envs (env-var-only envs are excluded from `listable`, matching the - // `env list` contract). - // Surface a malformed local config rather than silently reporting zero - // environments (which would hide stored credentials, incl. built-ins). + // `env list` contract). `listable` falls back to built-ins (logging a + // warning) on a malformed local config, so this never fails wholesale. let listable = environments::listable().map_err(|e| CliCoreError::message(e.to_string()))?; let mut envs = Vec::new(); From be682641d98ef43108b9629018ac9dfbe9610c1f Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 16:16:47 -0700 Subject: [PATCH 14/16] docs: describe env config path as platform-dependent (Copilot) Module/schema/auth doc comments no longer hard-code ~/.config/gddy; they reference dirs::config_dir()/environments_path() (XDG/%APPDATA%/macOS). Co-Authored-By: Claude Opus 4.8 --- rust/src/auth.rs | 3 ++- rust/src/environments/mod.rs | 10 ++++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/rust/src/auth.rs b/rust/src/auth.rs index 07f2f8a..cf823ff 100644 --- a/rust/src/auth.rs +++ b/rust/src/auth.rs @@ -14,7 +14,8 @@ use crate::environments::{self, ResolvedEnv}; /// where `` is the env name uppercased with `-` replaced by `_` /// (e.g. `OTE_OAUTH_CLIENT_ID`, `DEV_OAUTH_AUTH_URL`). The API base URL and the /// per-env defaults come from [`crate::environments`], which also resolves -/// custom DEV/TEST environments from `~/.config/gddy/environments.toml`. +/// custom DEV/TEST environments from the local config file (see +/// `crate::environments::environments_path`). #[derive(Debug, Default)] pub struct GoDaddyAuthProvider; diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index d1e0269..65365a6 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -10,9 +10,11 @@ //! cli-engine's `_OAUTH_CLIENT_ID` / `_AUTH_URL` / `_TOKEN_URL` naming, //! which `PkceAuthProvider` reads automatically when a provider is named after //! its environment. -//! * **Gitignored local config** — `~/.config/gddy/environments.toml` listing -//! custom environments. The file lives in the user's home directory, never in -//! the repo, so internal hostnames stay on the developer's machine. +//! * **Gitignored local config** — a `gddy/environments.toml` in the OS config +//! directory (`dirs::config_dir()`: `~/.config` on Linux/XDG, `%APPDATA%` on +//! Windows, `~/Library/Application Support` on macOS; see [`environments_path`]) +//! listing custom environments. The file lives outside the repo, so internal +//! hostnames stay on the developer's machine. //! //! Resolution order (later layers win): built-in base → local config entry → //! `_API_URL` env var. Built-ins may be overridden by either layer. @@ -67,7 +69,7 @@ pub struct ResolvedEnv { pub token_url: String, } -/// Schema of `~/.config/gddy/environments.toml`. +/// Schema of the local environments file (see [`environments_path`]). #[derive(Debug, Clone, Default, Deserialize)] pub struct EnvironmentsFile { #[serde(default)] From 277349e5c9312bef3f26def0b5afc46f9429004d Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 16:44:09 -0700 Subject: [PATCH 15/16] fix: case-insensitive URL scheme in clean_url (Copilot) URL schemes are case-insensitive (RFC 3986), but clean_url used case-sensitive strip_prefix, so a valid override like HTTPS://host was silently dropped. Match the scheme case-insensitively while preserving host case. Added a test. Co-Authored-By: Claude Opus 4.8 --- rust/src/environments/mod.rs | 28 +++++++++++++++++++++------- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 65365a6..817f6aa 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -126,13 +126,22 @@ fn derive_token_url(api_url: &str) -> String { /// and `listable` all defer to it. fn clean_url(raw: &str) -> Option { let trimmed = raw.trim().trim_end_matches('/'); - // Require an http(s):// scheme and a non-empty host. The host is the segment - // before any path/query/fragment, so this rejects `https:///path`, - // `https://`, and `https://?x` (which a lenient URL parser would accept). - let after_scheme = trimmed - .strip_prefix("https://") - .or_else(|| trimmed.strip_prefix("http://"))?; - let host = after_scheme.split(['/', '?', '#']).next().unwrap_or(""); + // Require an http(s):// scheme (case-insensitive per RFC 3986, so `HTTPS://` + // is valid) and a non-empty host. The host is the segment before any + // path/query/fragment, so this rejects `https:///path`, `https://`, and + // `https://?x` (which a lenient URL parser would accept). + let lower = trimmed.to_ascii_lowercase(); + let scheme_len = if lower.starts_with("https://") { + "https://".len() + } else if lower.starts_with("http://") { + "http://".len() + } else { + return None; + }; + let host = trimmed[scheme_len..] + .split(['/', '?', '#']) + .next() + .unwrap_or(""); (!host.is_empty()).then(|| trimmed.to_owned()) } @@ -507,6 +516,11 @@ mod tests { assert!(clean_url("ftp://x").is_none()); // wrong scheme assert!(clean_url("api.example.test").is_none()); // no scheme assert!(clean_url("not a url").is_none()); + // Scheme is case-insensitive (RFC 3986); host case is preserved. + assert_eq!( + clean_url("HTTPS://api.Example.test"), + Some("HTTPS://api.Example.test".to_owned()) + ); // A path/port is preserved (trailing slash trimmed). assert_eq!( clean_url("http://localhost:8080/api/"), From 2c08e03d9169629af847b1476876bc55d6a8c205 Mon Sep 17 00:00:00 2001 From: Jacob Page Date: Wed, 10 Jun 2026 16:49:34 -0700 Subject: [PATCH 16/16] docs: correct OAuth override var names + get_env survival caveat (Copilot) - Module doc spells the cli-engine override vars in full (_OAUTH_AUTH_URL / _OAUTH_TOKEN_URL), not the misleading _AUTH_URL. - get_env comment notes a config-only env is dropped if the config can't be read (is_known then falls back to built-ins + _API_URL). Co-Authored-By: Claude Opus 4.8 --- rust/src/env/mod.rs | 4 +++- rust/src/environments/mod.rs | 6 +++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/rust/src/env/mod.rs b/rust/src/env/mod.rs index 28c5a39..182bc35 100644 --- a/rust/src/env/mod.rs +++ b/rust/src/env/mod.rs @@ -30,7 +30,9 @@ pub fn get_env() -> Option { // .gdenv) so callers fall back to DEFAULT_ENV instead of propagating an // unknown environment. `is_known` accepts built-ins plus any env defined // via env var or local config, so a persisted custom env (e.g. "dev") - // survives. + // survives — except a config-only env is dropped if the config file + // can't be read, since `is_known` then falls back to built-ins + + // `_API_URL` only. .filter(|s| !s.is_empty() && environments::is_known(s)) } diff --git a/rust/src/environments/mod.rs b/rust/src/environments/mod.rs index 817f6aa..adf8eaa 100644 --- a/rust/src/environments/mod.rs +++ b/rust/src/environments/mod.rs @@ -7,9 +7,9 @@ //! * **Per-env environment variable** — `_API_URL` overrides (or defines) //! an environment's API base URL, where `` is the env name uppercased //! with `-` replaced by `_` (e.g. `DEV_API_URL`, `OTE_API_URL`). This mirrors -//! cli-engine's `_OAUTH_CLIENT_ID` / `_AUTH_URL` / `_TOKEN_URL` naming, -//! which `PkceAuthProvider` reads automatically when a provider is named after -//! its environment. +//! cli-engine's `_OAUTH_CLIENT_ID` / `_OAUTH_AUTH_URL` / +//! `_OAUTH_TOKEN_URL` naming, which `PkceAuthProvider` reads +//! automatically when a provider is named after its environment. //! * **Gitignored local config** — a `gddy/environments.toml` in the OS config //! directory (`dirs::config_dir()`: `~/.config` on Linux/XDG, `%APPDATA%` on //! Windows, `~/Library/Application Support` on macOS; see [`environments_path`])