diff --git a/rust/Cargo.lock b/rust/Cargo.lock index c818437b..c9ea489f 100644 --- a/rust/Cargo.lock +++ b/rust/Cargo.lock @@ -566,9 +566,9 @@ checksum = "c8d4a3bb8b1e0c1050499d1815f5ab16d04f0959b233085fb31653fbfc9d98f9" [[package]] name = "cli-engine" -version = "0.4.6" +version = "0.4.7" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "9aa5f4804af96b594f5903628dc3f12c04a9a0a27dc691c851ffa9a0fcf0477e" +checksum = "8f34184dcd279900e0822019ec221e3520ef92cca42025140f5ee5aa5f9578be" dependencies = [ "async-trait", "base64 0.22.1", @@ -1632,7 +1632,7 @@ dependencies = [ "libc", "percent-encoding", "pin-project-lite", - "socket2 0.6.3", + "socket2 0.5.10", "tokio", "tower-service", "tracing", @@ -2587,7 +2587,7 @@ dependencies = [ "quinn-udp", "rustc-hash", "rustls", - "socket2 0.6.3", + "socket2 0.5.10", "thiserror 2.0.18", "tokio", "tracing", @@ -2624,7 +2624,7 @@ dependencies = [ "cfg_aliases", "libc", "once_cell", - "socket2 0.6.3", + "socket2 0.5.10", "tracing", "windows-sys 0.60.2", ] diff --git a/rust/Cargo.toml b/rust/Cargo.toml index dd4f9ca8..0f4f9488 100644 --- a/rust/Cargo.toml +++ b/rust/Cargo.toml @@ -20,7 +20,7 @@ async-trait = "0.1" bytes = "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.4.6" } +cli-engine = { features = ["pkce-auth"], version = "0.4.7" } dirs = "6" domains-client = { path = "domains-client" } fancy-regex = "0.14" diff --git a/rust/src/api_explorer/mod.rs b/rust/src/api_explorer/mod.rs index d0910e5f..a6f8fe48 100644 --- a/rust/src/api_explorer/mod.rs +++ b/rust/src/api_explorer/mod.rs @@ -204,6 +204,14 @@ fn split_header(raw: &str) -> Option<(&str, &str)> { raw.split_once(':').map(|(k, v)| (k.trim(), v.trim())) } +/// Whether an HTTP method mutates server state, for `--dry-run` gating. +/// `call`'s tier is fixed at spec-build time, but the method is a runtime +/// arg — this decides per-invocation whether `--dry-run` should actually +/// short-circuit the request. Case-insensitive. +fn is_mutating_method(method: &str) -> bool { + !(method.eq_ignore_ascii_case("GET") || method.eq_ignore_ascii_case("HEAD")) +} + /// Parse a response body as JSON when possible, otherwise preserve it as raw /// UTF-8 text. A non-JSON body (plain text / HTML error page) must not be /// silently dropped to `null` — only a truly empty or binary body becomes null. @@ -525,6 +533,12 @@ fn call_command() -> RuntimeCommandSpec { ) .with_system("api") .with_tier(Tier::Mutate) + .handles_dry_run(true) + // The dry-run-for-mutating-methods branch never needs a token, so + // resolution is deferred to the handler (which only calls + // `credential_with_scopes` on the path that actually sends a + // request) instead of the engine resolving eagerly beforehand. + .auth_optional() .with_arg( clap::Arg::new("endpoint") .value_name("ENDPOINT") @@ -598,6 +612,28 @@ fn call_command() -> RuntimeCommandSpec { .get("method") .and_then(|v| v.as_str()) .unwrap_or("GET"); + // Validate the method unconditionally, including under + // `--dry-run` — otherwise a garbage/malformed method (e.g. + // "POST " with trailing whitespace) would return a successful + // dry-run preview even though a real run would reject it here. + let parsed_method: reqwest::Method = method.parse().map_err(|_| { + cli_engine::CliCoreError::message(format!("invalid HTTP method: {method}")) + })?; + + // `--dry-run` is statically tagged `Tier::Mutate` since the method + // is only known at runtime, but a GET/HEAD is safe to actually run + // (and more useful previewed as real data than as a generic + // string) — only short-circuit for methods that would mutate. + if ctx.dry_run() && is_mutating_method(method) { + return Ok(CommandResult::new(json!({ + "command": "api:call", + "action": "dry-run: would execute", + "method": method, + "endpoint": endpoint, + })) + .with_dry_run()); + } + // 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 @@ -649,12 +685,7 @@ fn call_command() -> RuntimeCommandSpec { let client = crate::application::client::make_http_client(); let mut req = client - .request( - method.parse().map_err(|_| { - cli_engine::CliCoreError::message(format!("invalid HTTP method: {method}")) - })?, - &url, - ) + .request(parsed_method, &url) .bearer_auth(&token) .header("x-request-id", uuid::Uuid::new_v4().to_string()); @@ -776,6 +807,8 @@ fn call_command() -> RuntimeCommandSpec { #[cfg(test)] mod tests { + use cli_engine::{Cli, CliConfig}; + use super::merge_required_scopes; fn v(items: &[&str]) -> Vec { @@ -861,4 +894,104 @@ mod tests { // Missing key. assert!(super::string_list(&args, "absent").is_empty()); } + + #[test] + fn is_mutating_method_treats_only_get_and_head_as_safe() { + assert!(!super::is_mutating_method("GET")); + assert!(!super::is_mutating_method("get")); + assert!(!super::is_mutating_method("HEAD")); + assert!(super::is_mutating_method("POST")); + assert!(super::is_mutating_method("PUT")); + assert!(super::is_mutating_method("PATCH")); + assert!(super::is_mutating_method("DELETE")); + } + + /// `call` opted into handler-driven `--dry-run` so a GET/HEAD can execute + /// for real under `--dry-run` — but it still requires `Required` auth + /// (no `.no_auth()`), so it must stay fail-closed regardless of method. + #[tokio::test] + async fn call_dry_run_get_still_requires_auth() { + const AUTH_FAILURE_EXIT: i32 = 2; + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy") + .with_default_auth_provider("godaddy") + .with_module(super::module()), + ); + let output = cli + .run([ + "gddy", + "api", + "call", + "/v1/example", + "--method", + "GET", + "--dry-run", + "--output", + "json", + ]) + .await; + assert_eq!( + output.exit_code, AUTH_FAILURE_EXIT, + "a GET falls through to the real request even under --dry-run, so it still needs \ + auth, got: {}", + output.rendered + ); + } + + /// `call` is `auth_optional()` specifically so the dry-run-for-mutating- + /// methods branch never triggers credential resolution (previously it did, + /// via the engine's `Required`-auth eager resolution before the handler + /// even ran) — a POST preview must succeed with no auth provider at all. + #[tokio::test] + async fn call_dry_run_mutating_method_needs_no_auth() { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(super::module()), + ); + let output = cli + .run([ + "gddy", + "api", + "call", + "/v1/example", + "--method", + "POST", + "--dry-run", + "--output", + "json", + ]) + .await; + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = + serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["data"]["action"], "dry-run: would execute"); + } + + /// A malformed method must still be rejected under `--dry-run`, matching + /// what the real path's `method.parse()` would do — a garbage method + /// must not previously have returned a fake "would execute" success. + #[tokio::test] + async fn call_dry_run_rejects_a_malformed_method() { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(super::module()), + ); + let output = cli + .run([ + "gddy", + "api", + "call", + "/v1/example", + "--method", + "POST ", + "--dry-run", + "--output", + "json", + ]) + .await; + assert_ne!(output.exit_code, 0, "{}", output.rendered); + assert!( + output.rendered.contains("invalid HTTP method"), + "{}", + output.rendered + ); + } } diff --git a/rust/src/dns/delete.rs b/rust/src/dns/delete.rs index 180472d3..d538bad0 100644 --- a/rust/src/dns/delete.rs +++ b/rust/src/dns/delete.rs @@ -7,6 +7,8 @@ use crate::domain::{api_error, make_client}; use crate::output_schema::output_schema; use crate::scopes::DOMAINS_DNS_UPDATE; +use domains_client::types; + use super::records::{arg_str, fetch_records, parse_write_type_arg, verify_with_list_action}; output_schema!(DnsDeleteResult { @@ -16,6 +18,7 @@ output_schema!(DnsDeleteResult { "deleted": "number"; "failed": "number"; "action": "string"; + "records": "[]object", optional; }); /// Build the `delete` result from per-record delete outcomes (each matched @@ -60,6 +63,47 @@ fn summarize_delete_outcomes( })) } +/// Builds the `dns delete --dry-run` preview from the same matched records the +/// real execution would delete, so the preview can never drift from what +/// `delete` would actually do. +fn dry_run_delete_preview( + domain: &str, + record_type: &str, + name: &str, + existing: &[types::DnsRecord], +) -> Value { + // Mirror the real execution's per-record split: a record without a + // recordId can't actually be deleted (see the handler below), so the + // preview must not claim it as a delete. + let would_delete = existing.iter().filter(|r| r.record_id.is_some()).count(); + let would_fail = existing.len() - would_delete; + let records: Vec = existing + .iter() + .map(|rec| { + let status = if rec.record_id.is_some() { + "would delete" + } else { + "would fail (missing recordId)" + }; + json!({"recordId": rec.record_id, "data": rec.data, "status": status}) + }) + .collect(); + + json!({ + "domain": domain, + "type": record_type, + "name": name, + // Reuse DnsDeleteResult's own field names (rather than inventing + // "wouldDelete"/"wouldFail") so the preview survives the command's + // `with_default_fields` projection instead of being silently + // stripped down to just domain/type/name. + "deleted": would_delete, + "failed": would_fail, + "records": records, + "action": "would delete", + }) +} + pub(super) fn command() -> RuntimeCommandSpec { RuntimeCommandSpec::new_with_context( CommandSpec::new( @@ -75,7 +119,8 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .with_system("domain") .with_tier(Tier::Destructive) - .with_default_fields("domain,type,name,deleted,failed") + .handles_dry_run(true) + .with_default_fields("domain,type,name,deleted,failed,action,records") .with_output_schema::() .with_scopes(&[DOMAINS_DNS_UPDATE]) .with_arg( @@ -117,6 +162,16 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .await?; + if ctx.dry_run() { + return Ok(CommandResult::new(dry_run_delete_preview( + &domain, + &record_type, + &name, + &existing, + )) + .with_dry_run()); + } + let mut outcomes = Vec::with_capacity(existing.len()); for rec in &existing { // A record with no server id can't be targeted — report it as a @@ -189,4 +244,79 @@ mod tests { "{err}" ); } + + fn test_record(record_id: &str, data: &str) -> types::DnsRecord { + types::DnsRecord { + data: data.to_owned(), + flag: None, + name: "www".to_owned(), + port: None, + priority: None, + protocol: None, + record_id: Some(record_id.to_owned()), + service: None, + tag: None, + ttl: 3600, + type_: types::DnsRecordType("A".to_owned()), + weight: None, + } + } + + fn test_record_without_id(data: &str) -> types::DnsRecord { + types::DnsRecord { + record_id: None, + ..test_record("unused", data) + } + } + + #[test] + fn dry_run_delete_preview_lists_every_matched_record_without_deleting() { + let existing = vec![test_record("r1", "1.2.3.4"), test_record("r2", "5.6.7.8")]; + let preview = dry_run_delete_preview("example.com", "A", "www", &existing); + assert_eq!(preview["deleted"], 2); + assert_eq!(preview["failed"], 0); + assert_eq!(preview["action"], "would delete"); + let records = preview["records"].as_array().expect("records array"); + assert_eq!(records.len(), 2); + assert_eq!(records[0]["data"], "1.2.3.4"); + assert_eq!(records[0]["status"], "would delete"); + } + + /// A record without a recordId can't actually be deleted (the real + /// handler reports it as a failure) — the preview must not claim it as + /// a delete either. + #[test] + fn dry_run_delete_preview_flags_records_without_a_recordid_as_would_fail() { + let existing = vec![ + test_record("r1", "1.2.3.4"), + test_record_without_id("5.6.7.8"), + ]; + let preview = dry_run_delete_preview("example.com", "A", "www", &existing); + assert_eq!(preview["deleted"], 1); + assert_eq!(preview["failed"], 1); + let records = preview["records"].as_array().expect("records array"); + assert_eq!(records[1]["status"], "would fail (missing recordId)"); + } + + /// The command's `--output json` path always projects through + /// `default_fields` when the user doesn't pass `--fields` — a preview + /// field that isn't in that list is silently stripped and never reaches + /// the user. Prove the preview's summary fields actually survive it + /// (this is exactly the gap a pure call to `dry_run_delete_preview` can't + /// catch, since it never goes through the projection). + #[test] + fn dry_run_delete_preview_survives_default_field_projection() { + let existing = vec![test_record("r1", "1.2.3.4")]; + let preview = dry_run_delete_preview("example.com", "A", "www", &existing); + let default_fields = "domain,type,name,deleted,failed,action,records"; + let projected = cli_engine::output::filter_fields(&preview, default_fields); + for field in [ + "domain", "type", "name", "deleted", "failed", "action", "records", + ] { + assert!( + !projected[field].is_null(), + "{field:?} was stripped by default_fields; preview: {projected}" + ); + } + } } diff --git a/rust/src/dns/mod.rs b/rust/src/dns/mod.rs index 35b8acc7..44474566 100644 --- a/rust/src/dns/mod.rs +++ b/rust/src/dns/mod.rs @@ -174,4 +174,56 @@ mod tests { ); } } + + /// `set`/`delete` opted into handler-driven `--dry-run` (to build a real + /// preview via `fetch_records`), which means the handler now runs under + /// `--dry-run` too — unlike the generic short-circuit, that must not skip + /// the fail-closed `Required`-auth check. + #[tokio::test] + async fn dns_set_and_delete_dry_run_still_require_auth() { + const AUTH_FAILURE_EXIT: i32 = 2; + let cases: [&[&str]; 2] = [ + &[ + "gddy", + "dns", + "set", + "example.com", + "--type", + "A", + "--name", + "www", + "--data", + "5.6.7.8", + "--dry-run", + "--output", + "json", + ], + &[ + "gddy", + "dns", + "delete", + "example.com", + "--type", + "A", + "--name", + "www", + "--dry-run", + "--output", + "json", + ], + ]; + for args in cases { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy") + .with_default_auth_provider("godaddy") + .with_module(module()), + ); + let output = cli.run(args.iter().copied()).await; + assert_eq!( + output.exit_code, AUTH_FAILURE_EXIT, + "{args:?} must still fail closed at auth resolution under --dry-run, got: {}", + output.rendered + ); + } + } } diff --git a/rust/src/dns/set.rs b/rust/src/dns/set.rs index 9a040acf..8adf48e5 100644 --- a/rust/src/dns/set.rs +++ b/rust/src/dns/set.rs @@ -27,6 +27,7 @@ output_schema!(DnsSetResult { "created": "number"; "deleted": "number"; "action": "string"; + "plan": "[]object", optional; }); /// One reconcile action for `dns set` over v3's per-record endpoints. @@ -132,6 +133,41 @@ fn summarize_set_outcomes( })) } +/// Builds the `dns set --dry-run` preview directly from the same reconcile +/// plan the real execution would apply, so the preview can never drift from +/// what `set` would actually do. Doesn't attempt to predict a name-exclusivity +/// conflict (that's only known once a write is actually attempted) — the +/// preview shows the reconcile plan, not whether `--replace-conflicting-types` +/// would end up mattering. +fn dry_run_set_preview(domain: &str, record_type: &str, name: &str, plan: &[SetAction]) -> Value { + let count = |predicate: fn(&SetAction) -> bool| plan.iter().filter(|a| predicate(a)).count(); + let plan_json: Vec = plan + .iter() + .map(|action| match action { + SetAction::Replace { record_id, data } => { + json!({"action": "replace", "recordId": record_id, "data": data}) + } + SetAction::Create { data } => json!({"action": "create", "data": data}), + SetAction::Delete { record_id } => json!({"action": "delete", "recordId": record_id}), + }) + .collect(); + + json!({ + "domain": domain, + "type": record_type, + "name": name, + // Reuse DnsSetResult's own field names (rather than inventing + // "wouldReplace" etc.) so the preview survives the command's + // `with_default_fields` projection instead of being silently + // stripped down to just domain/type/name. + "replaced": count(|a| matches!(a, SetAction::Replace { .. })), + "created": count(|a| matches!(a, SetAction::Create { .. })), + "deleted": count(|a| matches!(a, SetAction::Delete { .. })), + "plan": plan_json, + "action": "would set", + }) +} + /// Delete each conflicting record — used only by `set --replace-conflicting-types` /// to auto-resolve a name-exclusivity conflict before retrying the write. /// Returns one `SetOutcome` per record (rather than bailing on the first @@ -328,7 +364,8 @@ pub(super) fn command() -> RuntimeCommandSpec { ) .with_system("domain") .with_tier(Tier::Destructive) - .with_default_fields("domain,type,name,replaced,created,deleted") + .handles_dry_run(true) + .with_default_fields("domain,type,name,replaced,created,deleted,action,plan") .with_output_schema::() .with_scopes(&[DOMAINS_DNS_UPDATE]), ) @@ -380,8 +417,19 @@ pub(super) fn command() -> RuntimeCommandSpec { .filter_map(|r| r.record_id.clone()) .collect(); + let plan = plan_set(&existing_ids, &data); + if ctx.dry_run() { + return Ok(CommandResult::new(dry_run_set_preview( + &domain, + &record_type, + &name, + &plan, + )) + .with_dry_run()); + } + let mut outcomes = Vec::new(); - for action in plan_set(&existing_ids, &data) { + for action in plan { match action { SetAction::Replace { record_id, @@ -528,6 +576,42 @@ mod tests { assert!(err.contains("✗ created 8.8.8.8 — 422 bad"), "{err}"); } + #[test] + fn dry_run_set_preview_matches_the_plan_it_would_execute() { + let plan = plan_set( + &["r1".to_string(), "r2".to_string(), "r3".to_string()], + &["9.9.9.9".to_string(), "8.8.8.8".to_string()], + ); + let preview = dry_run_set_preview("example.com", "A", "www", &plan); + assert_eq!(preview["replaced"], 2); + assert_eq!(preview["created"], 0); + assert_eq!(preview["deleted"], 1); + assert_eq!(preview["action"], "would set"); + assert_eq!(preview["plan"].as_array().expect("plan array").len(), 3); + } + + /// The command's `--output json` path always projects through + /// `default_fields` when the user doesn't pass `--fields` — a preview + /// field that isn't in that list is silently stripped and never reaches + /// the user. Prove the preview's summary fields actually survive it + /// (this is exactly the gap a pure call to `dry_run_set_preview` can't + /// catch, since it never goes through the projection). + #[test] + fn dry_run_set_preview_survives_default_field_projection() { + let plan = plan_set(&["r1".to_string()], &["9.9.9.9".to_string()]); + let preview = dry_run_set_preview("example.com", "A", "www", &plan); + let default_fields = "domain,type,name,replaced,created,deleted,action,plan"; + let projected = cli_engine::output::filter_fields(&preview, default_fields); + for field in [ + "domain", "type", "name", "replaced", "created", "deleted", "action", "plan", + ] { + assert!( + !projected[field].is_null(), + "{field:?} was stripped by default_fields; preview: {projected}" + ); + } + } + // --- write_with_conflict_handling --------------------------------------- // // These exercise the DUPLICATE_RECORD branching against a mock server: the diff --git a/rust/src/domain/contacts.rs b/rust/src/domain/contacts.rs index 1a213b24..7e36eb56 100644 --- a/rust/src/domain/contacts.rs +++ b/rust/src/domain/contacts.rs @@ -35,6 +35,7 @@ pub(super) fn group() -> RuntimeGroupSpec { .with_system("domain") .with_tier(Tier::Mutate) .mutates(true) + .handles_dry_run(true) .no_auth(true) .with_default_fields("path,action") .with_arg( @@ -53,16 +54,26 @@ pub(super) fn group() -> RuntimeGroupSpec { .and_then(|v| v.as_bool()) .unwrap_or(false); let existed = path.exists(); - if existed && !force { - return Err(CliCoreError::message(format!( - "{} already exists; pass --force to overwrite", - path.display() - ))); + // Runs unconditionally, including under `--dry-run`, so a missing + // `--force` against an existing file is still reported as an error + // rather than a silent "would execute". + let action = init_action(&path, existed, force)?; + if ctx.dry_run() { + let preview_action = if existed { + "would overwrite" + } else { + "would create" + }; + return Ok(CommandResult::new(json!({ + "path": path.display().to_string(), + "action": preview_action, + })) + .with_dry_run()); } cli_engine::fs::write_string_atomic(&path, contacts::sample_toml())?; Ok(CommandResult::new(json!({ "path": path.display().to_string(), - "action": if existed { "overwritten" } else { "created" }, + "action": action, })) .with_next_actions(vec![next_action( "guide domain-purchase", @@ -71,3 +82,91 @@ pub(super) fn group() -> RuntimeGroupSpec { }, )) } + +/// Decides what `contacts init` would report, or the error it would return, +/// given whether the file exists and `--force`. Shared by the real execution +/// and dry-run paths so both agree on when this command would fail. +fn init_action( + path: &std::path::Path, + existed: bool, + force: bool, +) -> Result<&'static str, CliCoreError> { + if existed && !force { + return Err(CliCoreError::message(format!( + "{} already exists; pass --force to overwrite", + path.display() + ))); + } + Ok(if existed { "overwritten" } else { "created" }) +} + +#[cfg(test)] +mod tests { + use std::path::Path; + + use cli_engine::{Cli, CliConfig}; + + use super::init_action; + + #[test] + fn errors_when_file_exists_and_force_is_not_passed() { + let err = init_action(Path::new("/tmp/contacts.toml"), true, false) + .expect_err("should refuse to overwrite without --force"); + assert!(err.to_string().contains("--force")); + } + + #[test] + fn overwrites_when_file_exists_and_force_is_passed() { + assert_eq!( + init_action(Path::new("/tmp/contacts.toml"), true, true).expect("should succeed"), + "overwritten" + ); + } + + #[test] + fn creates_when_file_does_not_exist_regardless_of_force() { + assert_eq!( + init_action(Path::new("/tmp/contacts.toml"), false, false).expect("should succeed"), + "created" + ); + assert_eq!( + init_action(Path::new("/tmp/contacts.toml"), false, true).expect("should succeed"), + "created" + ); + } + + /// `--dry-run` never reaches `write_string_atomic` (it returns before that + /// call), so this is safe to run against the real config dir: it can only + /// read `path.exists()`, never write. + #[tokio::test] + async fn init_dry_run_previews_without_writing() { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy") + .with_module(crate::domain::module()), + ); + // `--force` so the assertion holds regardless of whether this + // machine already has a contacts.toml (the case this test exists to + // cover in the first place: DEVEX-890). + let output = cli + .run([ + "gddy", + "domain", + "contacts", + "init", + "--force", + "--dry-run", + "--output", + "json", + ]) + .await; + + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = + serde_json::from_str(&output.rendered).expect("valid json"); + let action = rendered["data"]["action"].as_str().unwrap_or_default(); + assert!( + action == "would create" || action == "would overwrite", + "unexpected action: {rendered}" + ); + } +} diff --git a/rust/src/env/mod.rs b/rust/src/env/mod.rs index 2c91148a..a0443c63 100644 --- a/rust/src/env/mod.rs +++ b/rust/src/env/mod.rs @@ -17,6 +17,16 @@ output_schema!(EnvActive { "apiUrl": "string"; }); +// `env set` always returns an "action" ("activated" for real, "would +// activate" under --dry-run) — a dedicated schema, rather than adding +// `action` to `EnvActive`, since `EnvActive` is shared with `env get`, which +// never has an action concept. +output_schema!(EnvSetResult { + "env": "string"; + "apiUrl": "string"; + "action": "string"; +}); + output_schema!(EnvInfo { "env": "string"; "apiUrl": "string"; @@ -147,7 +157,8 @@ pub fn module() -> Module { ) .with_system("env") .with_tier(Tier::Mutate) - .with_output_schema::() + .handles_dry_run(true) + .with_output_schema::() .no_auth(true) .with_arg( // Distinct id from the global `--env` flag (also id "env"); @@ -167,7 +178,16 @@ pub fn module() -> Module { .to_owned(); // Resolve up front: validates the env exists (built-in, env // var, or local config) and yields its API URL for the reply. + // Runs unconditionally, including under `--dry-run`. let resolved = environments::resolve(&env).map_err(map_err)?; + if ctx.dry_run() { + return Ok(CommandResult::new(json!({ + "env": resolved.name, + "apiUrl": resolved.api_url, + "action": "would activate", + })) + .with_dry_run()); + } set_env(&env).map_err(|e| { cli_engine::CliCoreError::message(format!( "failed to write .gdenv state file: {e}" @@ -176,6 +196,7 @@ pub fn module() -> Module { Ok(CommandResult::new(json!({ "env": resolved.name, "apiUrl": resolved.api_url, + "action": "activated", }))) }, )) @@ -243,4 +264,44 @@ mod tests { ); } } + + /// `env set --dry-run` never calls `set_env`, so this is safe to run + /// without touching the real `~/.gdenv` state file. + #[tokio::test] + async fn env_set_dry_run_previews_a_valid_environment_without_persisting() { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(super::module()), + ); + + let output = cli + .run(["gddy", "env", "set", "ote", "--dry-run", "--output", "json"]) + .await; + + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let json: serde_json::Value = + serde_json::from_str(&output.rendered).expect("valid json output"); + assert_eq!(json["data"]["env"], "ote"); + assert_eq!(json["data"]["action"], "would activate"); + } + + #[tokio::test] + async fn env_set_dry_run_still_rejects_an_unknown_environment() { + let cli = Cli::new( + CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(super::module()), + ); + + let output = cli + .run([ + "gddy", + "env", + "set", + "not-a-real-env", + "--dry-run", + "--output", + "json", + ]) + .await; + + assert_ne!(output.exit_code, 0, "{}", output.rendered); + } } diff --git a/rust/src/pat/mod.rs b/rust/src/pat/mod.rs index 4367a80c..2770eeb2 100644 --- a/rust/src/pat/mod.rs +++ b/rust/src/pat/mod.rs @@ -36,6 +36,7 @@ output_schema!(PatAddResult { "name": "string"; "lastFour": "string"; "path": "string"; + "action": "string"; }); output_schema!(PatRemoveResult { @@ -249,6 +250,15 @@ pub async fn delete_pat(env: &str) -> Result { Ok(existed) } +/// Returns whether a PAT is currently stored for `env`, without removing it. +/// Used to preview `pat remove --dry-run` without mutating the registry. +pub fn has_pat(env: &str) -> Result { + let Some(path) = registry_path() else { + return Ok(false); + }; + Ok(load_registry(&path)?.tokens.contains_key(env)) +} + /// List environments that have a PAT in the registry. pub async fn list_pats() -> Result, CliCoreError> { let Some(path) = registry_path() else { @@ -311,9 +321,10 @@ fn add_command() -> RuntimeCommandSpec { .with_system("pat") .with_tier(Tier::Mutate) .mutates(true) + .handles_dry_run(true) .no_auth(true) .with_output_schema::() - .with_default_fields("env,name,lastFour,path") + .with_default_fields("env,name,lastFour,path,action") .with_arg( clap::Arg::new("env") .long("env") @@ -336,7 +347,8 @@ fn add_command() -> RuntimeCommandSpec { |ctx| async move { let env = string_arg(&ctx.args, "env"); // Validate the environment up front so a typo does not stash a PAT - // for a non-existent env. + // for a non-existent env. Runs unconditionally, including under + // `--dry-run`, so `--dry-run` can be used to pre-validate a PAT. environments::resolve(&env).map_err(|e| CliCoreError::message(e.to_string()))?; let name = string_arg(&ctx.args, "name"); @@ -347,6 +359,24 @@ fn add_command() -> RuntimeCommandSpec { )); } + if ctx.dry_run() { + // Resolve and load the registry too, so a dry-run can't + // report success in a state (no config dir, or an existing + // pat.toml that fails to parse) where a real run — which + // calls save_pat, which loads the registry before writing — + // would immediately fail before ever getting this far. + let path = registry_path_err()?; + load_registry(&path)?; + return Ok(CommandResult::new(json!({ + "env": env, + "name": name, + "lastFour": last_four(&token), + "path": path.display().to_string(), + "action": "would store", + })) + .with_dry_run()); + } + let entry = PatEntry { token, name }; save_pat(&env, entry.clone()).await?; @@ -356,6 +386,7 @@ fn add_command() -> RuntimeCommandSpec { "name": entry.name, "lastFour": last_four(&entry.token), "path": path.display().to_string(), + "action": "stored", })) .with_next_actions(vec![next_action("guide auth", "Learn about PAT auth")])) }, @@ -403,6 +434,7 @@ fn remove_command() -> RuntimeCommandSpec { .with_system("pat") .with_tier(Tier::Mutate) .mutates(true) + .handles_dry_run(true) .no_auth(true) .with_output_schema::() .with_default_fields("env,status") @@ -418,6 +450,18 @@ fn remove_command() -> RuntimeCommandSpec { // Validate the environment up front so a typo produces a clear error // instead of silently reporting "not found". environments::resolve(&env).map_err(|e| CliCoreError::message(e.to_string()))?; + + if ctx.dry_run() { + let status = if has_pat(&env)? { + "would remove" + } else { + "not found" + }; + return Ok( + CommandResult::new(json!({ "env": env, "status": status })).with_dry_run() + ); + } + let existed = delete_pat(&env).await?; Ok(CommandResult::new(json!({ "env": env, @@ -473,6 +517,8 @@ async fn resolve_token_arg( #[cfg(test)] mod tests { + use cli_engine::{Cli, CliConfig}; + use super::*; #[test] @@ -569,6 +615,18 @@ mod tests { assert_eq!(loaded.tokens["prod"].token, "gd_pat_abc123_1234abcd"); } + /// `pat add --dry-run` calls this same function so it can't report + /// success in a state where a real run (which loads the registry via + /// `save_pat`) would immediately fail parsing an existing malformed file. + #[test] + fn load_registry_rejects_malformed_toml() { + let tmp = tempfile::tempdir().expect("tempdir"); + let path = tmp.path().join("pat.toml"); + std::fs::write(&path, "this is not valid toml {{{").expect("write"); + let err = load_registry(&path).expect_err("malformed TOML should be rejected"); + assert!(err.to_string().contains("pat.toml"), "{err}"); + } + fn test_registry_with(env: &str, token: &str) -> PatRegistry { let mut registry = PatRegistry::default(); registry.tokens.insert( @@ -658,4 +716,113 @@ mod tests { assert_eq!(env_var_for("prod"), "GDDY_PAT_PROD"); assert_eq!(env_var_for("ote"), "GDDY_PAT_OTE"); } + + /// DEVEX-889: the message should point at `gddy guide auth` and not imply + /// the user could type/guess a valid PAT by hand. + #[tokio::test] + async fn invalid_pat_message_points_to_guide_auth() { + let cli = + Cli::new(CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(module())); + let output = cli + .run([ + "gddy", "pat", "add", "--env", "ote", "--token", "garbage", "test", + ]) + .await; + + assert_ne!(output.exit_code, 0, "{}", output.rendered); + assert!( + output.rendered.contains("guide auth"), + "{}", + output.rendered + ); + assert!( + !output.rendered.contains("expected `gd_pat_...`"), + "should not imply a literal typed format: {}", + output.rendered + ); + } + + /// GDDEVPLAT-81: `--dry-run` must still reject a malformed token instead of + /// unconditionally reporting the generic "would execute". + #[tokio::test] + async fn add_dry_run_still_rejects_a_malformed_token() { + let cli = + Cli::new(CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(module())); + let output = cli + .run([ + "gddy", + "pat", + "add", + "--env", + "ote", + "--token", + "garbage", + "test", + "--dry-run", + ]) + .await; + + assert_ne!(output.exit_code, 0, "{}", output.rendered); + assert!( + !output.rendered.contains("would execute") && !output.rendered.contains("would store"), + "a malformed token must be rejected, not previewed: {}", + output.rendered + ); + } + + /// GDDEVPLAT-81: a well-formed token previews without being stored. + #[tokio::test] + async fn add_dry_run_previews_a_valid_token_without_storing() { + let cli = + Cli::new(CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(module())); + let output = cli + .run([ + "gddy", + "pat", + "add", + "--env", + "ote", + "--token", + "gd_pat_abc123_1234abcd", + "test", + "--dry-run", + "--output", + "json", + ]) + .await; + + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = + serde_json::from_str(&output.rendered).expect("valid json"); + assert_eq!(rendered["data"]["action"], "would store"); + assert_eq!(rendered["data"]["lastFour"], "abcd"); + } + + /// `pat remove --dry-run` previews existence without deleting. + #[tokio::test] + async fn remove_dry_run_reports_not_found_without_error() { + let cli = + Cli::new(CliConfig::new("gddy", "GoDaddy developer CLI", "gddy").with_module(module())); + let output = cli + .run([ + "gddy", + "pat", + "remove", + "--env", + "ote", + "--dry-run", + "--output", + "json", + ]) + .await; + + assert_eq!(output.exit_code, 0, "{}", output.rendered); + let rendered: serde_json::Value = + serde_json::from_str(&output.rendered).expect("valid json"); + let status = rendered["data"]["status"].as_str().unwrap_or_default(); + assert!( + status == "would remove" || status == "not found", + "unexpected status: {rendered}" + ); + } }