From 024651c21e78c563ffe8c2dfb218145cbc800859 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 17:33:42 +0800 Subject: [PATCH 1/5] feat: add mutation plans and confirmation --- README.md | 38 +++- README.zh.md | 22 ++- src/cli.rs | 2 +- src/command.rs | 473 +++++++++++++++++++++++++++++++++++++++++------ src/error.rs | 35 +++- src/lib.rs | 1 + src/mutation.rs | 348 ++++++++++++++++++++++++++++++++++ src/output.rs | 42 ++++- tests/cli.rs | 6 +- tests/openapi.rs | 159 +++++++++++++++- 10 files changed, 1047 insertions(+), 79 deletions(-) create mode 100644 src/mutation.rs diff --git a/README.md b/README.md index de8b3f4..2f78348 100644 --- a/README.md +++ b/README.md @@ -84,6 +84,9 @@ The current scaffold parses these global flags before subcommands: - `--output json|table` - `--yes` +`--yes` explicitly approves a mutating OpenAPI request without an interactive prompt. It does not +skip target-plan construction, validation, redaction, or operation reporting. + ## Guided setup Use `apollo init` for first-time setup. It creates a profile, writes non-secret profile metadata to @@ -274,7 +277,8 @@ Structured JSON errors include: - `code`: stable error code - `category`: stable category - `message`: human-readable message -- optional non-sensitive details such as `command`, `profile`, `path`, or `follow_up_issue` +- optional non-sensitive details such as `command`, `profile`, `path`, `operation`, or + `follow_up_issue` Current error categories: @@ -289,6 +293,34 @@ Current error categories: - `confirmation_required` - `unsupported_operation` +Process exit statuses are stable at the following level: + +- `0`: success +- `1`: runtime or operation failure, including authentication, validation, network/server, and + confirmation failures +- `2`: command-line parse or usage failure + +Use the structured JSON `error.code` and `error.category` fields when automation needs a more +specific failure reason than the process exit status. + +## Mutation safety + +Before a built-in namespace, config, release, or raw API mutation, the CLI constructs a redacted +operation plan from the selected profile/server and the command target. Plans include the fields +available for that operation, such as app, env, cluster, namespace, config key/count, release IDs, +or a sanitized raw OpenAPI method and path. Config values, request bodies, query values, tokens, and +Authorization headers are not included. + +In interactive table mode, a mutation without `--yes` writes the plan and a `[y/N]` prompt to +stderr. Only `y` or `yes` executes the request; `n`, `no`, blank input, or EOF rejects it. In +non-interactive mode and in JSON mode, mutations require `--yes`; otherwise the CLI returns a +`confirmation_required` error whose `operation` field contains the redacted plan. Rejection occurs +before any OpenAPI request is sent. + +With `--yes`, table mode still writes the plan before the request. A successful JSON response stays +one valid JSON document and preserves the existing top-level `status` and `data` fields while adding +the top-level `operation` plan. + ## OpenAPI behavior The first v0 implementation uses a small generic HTTP client instead of a generated SDK. This keeps @@ -309,8 +341,8 @@ Path and payload mapping follows the current Apollo Portal OpenAPI contract, inc - `POST /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/releases` - `PUT /openapi/v1/envs/{env}/releases/{releaseId}/rollback` -Mutating commands require `--yes`. Without it, the CLI returns `confirmation_required` before -opening a network connection. +Mutating command confirmation and operation-plan behavior are described in +[Mutation safety](#mutation-safety). ## Binary releases diff --git a/README.zh.md b/README.zh.md index d3c8dd2..697ca48 100644 --- a/README.zh.md +++ b/README.zh.md @@ -73,6 +73,8 @@ apollo api post /openapi/v1/apps --body '{"app":{"appId":"sample-app"}}' --yes - `--output json|table` - `--yes` +`--yes` 表示在不显示交互提示的情况下显式批准一次 OpenAPI 变更请求。它不会跳过目标计划构建、参数校验、脱敏或操作信息输出。 + ## 引导式初始化 首次使用时推荐执行 `apollo init`。它会创建 profile，将非敏感 profile 元数据写入 `config.toml`，并且可以通过凭据存储抽象保存 Apollo OpenAPI token。 @@ -242,7 +244,7 @@ apollo --profile dev auth capabilities - `code`：稳定错误码 - `category`：稳定错误分类 - `message`：人类可读错误信息 -- 可选的非敏感详情，例如 `command`、`profile`、`path` 或 `follow_up_issue` +- 可选的非敏感详情，例如 `command`、`profile`、`path`、`operation` 或 `follow_up_issue` 当前错误分类： @@ -257,6 +259,22 @@ apollo --profile dev auth capabilities - `confirmation_required` - `unsupported_operation` +进程退出状态在以下层级保持稳定： + +- `0`：成功 +- `1`：运行期或操作失败，包括鉴权、校验、网络/服务端和确认失败 +- `2`：命令行解析或用法错误 + +当自动化调用方需要比进程退出状态更具体的失败原因时，应使用结构化 JSON 中的 `error.code` 和 `error.category`。 + +## 变更安全 + +在执行内置 namespace、config、release 或 raw API 变更前，CLI 会根据选中的 profile/server 和命令目标构建一份脱敏操作计划。计划会按操作类型包含可用字段，例如 app、env、cluster、namespace、配置 key/数量、release ID，或经过净化的 raw OpenAPI method 和 path。计划不会包含配置值、请求 body、query 值、token 或 Authorization header。 + +在交互式 table 模式中，未传 `--yes` 的变更会把计划和默认拒绝的 `[y/N]` 提示写到 stderr。只有输入 `y` 或 `yes` 才会执行；输入 `n`、`no`、空行或遇到 EOF 都会拒绝。在非交互模式和 JSON 模式中，变更必须显式传入 `--yes`；否则 CLI 返回 `confirmation_required`，其 `operation` 字段包含脱敏计划。拒绝发生在任何 OpenAPI 请求发送之前。 + +传入 `--yes` 时，table 模式仍会在请求前输出计划。成功的 JSON 输出仍是一个完整 JSON 文档，并保留现有顶层 `status` 和 `data` 字段，同时新增顶层 `operation` 计划。 + ## OpenAPI 行为 第一版 v0 实现使用一个小型通用 HTTP client，而不是生成式 SDK。这样可以让 CLI 与 Apollo 服务端仓库解耦，同时仍然保证所有内置资源命令都限定在 `/openapi/v1/*`。 @@ -275,7 +293,7 @@ apollo --profile dev auth capabilities - `POST /openapi/v1/envs/{env}/apps/{appId}/clusters/{clusterName}/namespaces/{namespaceName}/releases` - `PUT /openapi/v1/envs/{env}/releases/{releaseId}/rollback` -变更类命令要求传 `--yes`。如果没有传，CLI 会在建立网络连接之前返回 `confirmation_required`。 +变更类命令的确认和操作计划行为见[变更安全](#变更安全)。 ## 可执行文件发布 diff --git a/src/cli.rs b/src/cli.rs index 026454d..f1bbbc5 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -32,7 +32,7 @@ pub struct GlobalOptions { #[arg( long, global = true, - help = "Skip confirmation prompts for mutating OpenAPI requests" + help = "Approve mutating OpenAPI requests without an interactive prompt" )] pub yes: bool, } diff --git a/src/command.rs b/src/command.rs index c82f763..f24d856 100644 --- a/src/command.rs +++ b/src/command.rs @@ -17,8 +17,9 @@ use crate::config::{ use crate::credential; use crate::error::CliError; use crate::http::{OpenApiClient, OpenApiResponse, append_query, encode_path_segment}; +use crate::mutation::{MutationPlan, MutationScope}; use crate::output::{OutputWriter, RenderedOutput}; -use crate::redaction::Sensitive; +use crate::redaction::{Redactor, Sensitive}; const DEFAULT_PAGE: u32 = 0; const DEFAULT_PAGE_SIZE: u32 = 20; @@ -549,9 +550,9 @@ fn execute_namespace( cli: &Cli, output: OutputFormat, ) -> Result { - if matches!(command, NamespaceCommand::Create { .. }) { - require_yes_for_openapi(cli, output)?; - } + let mutation_plan = namespace_mutation_plan(&command) + .map(|plan| prepare_mutation(cli, output, plan)) + .transpose()?; let openapi = openapi_context(cli, output)?; match command { NamespaceCommand::List { scope } => { @@ -587,6 +588,9 @@ fn execute_namespace( comment, append_namespace_prefix, } => { + let mutation_plan = mutation_plan + .as_ref() + .expect("namespace create mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -619,7 +623,7 @@ fn execute_namespace( "clusterName": &scope.cluster, "appNamespaceName": &app_namespace.name, }]); - match openapi.request("POST", &path, Some(body)) { + match openapi.mutation_request(mutation_plan, "POST", &path, Some(body)) { Ok(output) => Ok(output), Err(error) if is_namespace_create_reported_failed(&error) => { match openapi @@ -628,8 +632,9 @@ fn execute_namespace( { Ok(response) => { let data = redact_nested_item_values(response.data.clone()); - Ok(render_openapi_response_with_data( + Ok(render_mutation_response_with_data( &openapi.writer, + mutation_plan, &response, data, )) @@ -936,9 +941,9 @@ fn execute_config( cli: &Cli, output: OutputFormat, ) -> Result { - if config_command_requires_confirmation(&command) { - require_yes_for_openapi(cli, output)?; - } + let mutation_plan = config_mutation_plan(&command) + .map(|plan| prepare_mutation(cli, output, plan)) + .transpose()?; let openapi = openapi_context(cli, output)?; match command { ConfigCommand::List { scope, page, size } => { @@ -967,6 +972,7 @@ fn execute_config( comment, operator, } => { + let mutation_plan = mutation_plan.as_ref().expect("config set mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -995,10 +1001,18 @@ fn execute_config( .client .request("PUT", &update_path, Some(body.clone())) { - Ok(response) => Ok(render_openapi_response(&openapi.writer, &response)), + Ok(response) => Ok(render_mutation_response( + &openapi.writer, + mutation_plan, + &response, + )), Err(error) if error.http_status_code() == Some(404) => { let response = openapi.client.request("POST", &create_path, Some(body))?; - Ok(render_openapi_response(&openapi.writer, &response)) + Ok(render_mutation_response( + &openapi.writer, + mutation_plan, + &response, + )) } Err(error) => Err(error), } @@ -1008,6 +1022,7 @@ fn execute_config( key, operator, } => { + let mutation_plan = mutation_plan.as_ref().expect("config delete mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1018,7 +1033,7 @@ fn execute_config( "operator", operator.as_deref(), ); - openapi.request("DELETE", &path, None) + openapi.mutation_request(mutation_plan, "DELETE", &path, None) } ConfigCommand::Diff { scope, @@ -1050,6 +1065,7 @@ fn execute_config( target_namespace, operator, } => { + let mutation_plan = mutation_plan.as_ref().expect("config apply mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1076,7 +1092,7 @@ fn execute_config( "operator", operator.as_deref(), ); - openapi.request("POST", &path, Some(body)) + openapi.mutation_request(mutation_plan, "POST", &path, Some(body)) } } } @@ -1086,9 +1102,9 @@ fn execute_release( cli: &Cli, output: OutputFormat, ) -> Result { - if release_command_requires_confirmation(&command) { - require_yes_for_openapi(cli, output)?; - } + let mutation_plan = release_mutation_plan(&command) + .map(|plan| prepare_mutation(cli, output, plan)) + .transpose()?; let openapi = openapi_context(cli, output)?; match command { ReleaseCommand::List { scope, page, size } => { @@ -1111,6 +1127,9 @@ fn execute_release( emergency, operator, } => { + let mutation_plan = mutation_plan + .as_ref() + .expect("release create mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1127,8 +1146,9 @@ fn execute_release( } let response = openapi.client.request("POST", &path, Some(body))?; let data = redact_release_configurations(response.data.clone()); - Ok(render_openapi_response_with_data( + Ok(render_mutation_response_with_data( &openapi.writer, + mutation_plan, &response, data, )) @@ -1139,6 +1159,9 @@ fn execute_release( to_release_id, operator, } => { + let mutation_plan = mutation_plan + .as_ref() + .expect("release rollback mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1156,15 +1179,15 @@ fn execute_release( if let Some(to_release_id) = to_release_id { path = append_query(path, "toReleaseId", &to_release_id.to_string()); } - openapi.request("PUT", &path, None) + openapi.mutation_request(mutation_plan, "PUT", &path, None) } } } fn execute_api(args: ApiArgs, cli: &Cli, output: OutputFormat) -> Result { - if http_method_requires_confirmation(args.method) { - require_yes_for_openapi(cli, output)?; - } + let mutation_plan = api_mutation_plan(&args) + .map(|plan| prepare_mutation(cli, output, plan)) + .transpose()?; let openapi = openapi_context(cli, output)?; let body = match args.body { Some(body) => Some(serde_json::from_str::(&body).map_err(|error| { @@ -1172,7 +1195,12 @@ fn execute_api(args: ApiArgs, cli: &Cli, output: OutputFormat) -> Result None, }; - openapi.request(args.method.as_str(), &args.path, body) + match mutation_plan.as_ref() { + Some(mutation_plan) => { + openapi.mutation_request(mutation_plan, args.method.as_str(), &args.path, body) + } + None => openapi.request(args.method.as_str(), &args.path, body), + } } fn execute_auth_self_check( @@ -1298,6 +1326,12 @@ struct OpenApiCommandContext { client: OpenApiClient, } +struct MutationRuntimeContext { + profile: Option, + server: String, + output: OutputFormat, +} + impl OpenApiCommandContext { fn request( &self, @@ -1308,6 +1342,59 @@ impl OpenApiCommandContext { let response = self.client.request(method, path, body)?; Ok(render_openapi_response(&self.writer, &response)) } + + fn mutation_request( + &self, + operation: &MutationPlan, + method: &str, + path: &str, + body: Option, + ) -> Result { + let response = self.client.request(method, path, body)?; + Ok(render_mutation_response(&self.writer, operation, &response)) + } +} + +fn mutation_runtime_context( + cli: &Cli, + output: OutputFormat, +) -> Result { + if env_token_is_set() + && let Some(server) = explicit_server(cli) + { + let loaded = load_config(output).ok(); + let writer_output = loaded + .as_ref() + .and_then(|loaded| resolve_output(cli, loaded, output).ok()) + .unwrap_or_else(|| output_from_flags_or_env(cli).unwrap_or(output)); + let selected_profile = cli + .global + .profile + .clone() + .and_then(non_blank) + .or_else(|| std::env::var("APOLLO_PROFILE").ok().and_then(non_blank)) + .or_else(|| { + loaded + .as_ref() + .and_then(|loaded| loaded.config.active_profile.clone()) + .and_then(non_blank) + }); + return Ok(MutationRuntimeContext { + profile: selected_profile, + server, + output: writer_output, + }); + } + + let loaded = load_config(output)?; + let writer_output = resolve_output(cli, &loaded, output)?; + let context = resolve_context(cli, &loaded, writer_output)?; + let server = required_server(&context, writer_output)?; + Ok(MutationRuntimeContext { + profile: context.profile, + server, + output: writer_output, + }) } fn openapi_context(cli: &Cli, output: OutputFormat) -> Result { @@ -1449,6 +1536,14 @@ fn render_openapi_response(writer: &OutputWriter, response: &OpenApiResponse) -> writer.render_success(response, response.render_table()) } +fn render_mutation_response( + writer: &OutputWriter, + operation: &MutationPlan, + response: &OpenApiResponse, +) -> RenderedOutput { + writer.render_mutation_success(operation, response, response.render_table()) +} + fn render_openapi_response_with_data( writer: &OutputWriter, response: &OpenApiResponse, @@ -1458,6 +1553,16 @@ fn render_openapi_response_with_data( render_openapi_response(writer, &response) } +fn render_mutation_response_with_data( + writer: &OutputWriter, + operation: &MutationPlan, + response: &OpenApiResponse, + data: Value, +) -> RenderedOutput { + let response = response.with_data(data); + render_mutation_response(writer, operation, &response) +} + fn filter_apps_by_ids(data: Value, app_ids: &str) -> Value { let selected = app_ids .split(',') @@ -1655,43 +1760,188 @@ fn redact_release_configurations_in_value(value: &mut Value) { } } -fn config_command_requires_confirmation(command: &ConfigCommand) -> bool { - matches!( - command, - ConfigCommand::Set { .. } | ConfigCommand::Delete { .. } | ConfigCommand::Apply { .. } - ) +fn namespace_mutation_plan(command: &NamespaceCommand) -> Option { + match command { + NamespaceCommand::Create { + scope, + name, + public_namespace, + append_namespace_prefix, + .. + } => Some( + MutationPlan::new("namespace.create") + .with_target(MutationScope::namespace( + &scope.app, + &scope.env, + &scope.cluster, + name, + )) + .with_namespace_kind(*public_namespace, *append_namespace_prefix), + ), + NamespaceCommand::List { .. } | NamespaceCommand::Get { .. } => None, + } } -fn release_command_requires_confirmation(command: &ReleaseCommand) -> bool { - matches!( - command, - ReleaseCommand::Create { .. } | ReleaseCommand::Rollback { .. } - ) +fn config_mutation_plan(command: &ConfigCommand) -> Option { + match command { + ConfigCommand::Set { scope, key, .. } => Some( + MutationPlan::new("config.set") + .with_target(namespace_mutation_scope(scope)) + .with_key(key), + ), + ConfigCommand::Delete { scope, key, .. } => Some( + MutationPlan::new("config.delete") + .with_target(namespace_mutation_scope(scope)) + .with_key(key), + ), + ConfigCommand::Apply { + scope, + target_env, + target_cluster, + target_namespace, + .. + } => Some( + MutationPlan::new("config.apply") + .with_source(namespace_mutation_scope(scope)) + .with_target(MutationScope::namespace( + &scope.cluster_scope.app, + target_env, + target_cluster, + target_namespace.as_deref().unwrap_or(&scope.namespace), + )), + ), + ConfigCommand::List { .. } | ConfigCommand::Get { .. } | ConfigCommand::Diff { .. } => None, + } +} + +fn release_mutation_plan(command: &ReleaseCommand) -> Option { + match command { + ReleaseCommand::Create { + scope, + title, + emergency, + .. + } => Some( + MutationPlan::new("release.create") + .with_target(namespace_mutation_scope(scope)) + .with_release(title, *emergency), + ), + ReleaseCommand::Rollback { + env, + release_id, + to_release_id, + .. + } => Some( + MutationPlan::new("release.rollback") + .with_target(MutationScope::environment(env)) + .with_release_ids(*release_id, *to_release_id), + ), + ReleaseCommand::List { .. } => None, + } } -fn http_method_requires_confirmation(method: crate::cli::HttpMethod) -> bool { +fn api_mutation_plan(args: &ApiArgs) -> Option { matches!( - method, + args.method, crate::cli::HttpMethod::Post | crate::cli::HttpMethod::Put | crate::cli::HttpMethod::Patch | crate::cli::HttpMethod::Delete ) + .then(|| { + MutationPlan::new(format!("api.{}", args.method.as_str().to_ascii_lowercase())) + .with_request(args.method.as_str(), &args.path) + }) } -fn require_yes_for_openapi(cli: &Cli, output: OutputFormat) -> Result<(), CliError> { - require_yes(cli, output_for_confirmation(cli, output)) +fn namespace_mutation_scope(scope: &NamespaceScopeArgs) -> MutationScope { + MutationScope::namespace( + &scope.cluster_scope.app, + &scope.cluster_scope.env, + &scope.cluster_scope.cluster, + &scope.namespace, + ) } -fn output_for_confirmation(cli: &Cli, output: OutputFormat) -> OutputFormat { - if let Some(output) = output_from_flags_or_env(cli) { - return output; +fn prepare_mutation( + cli: &Cli, + output: OutputFormat, + plan: MutationPlan, +) -> Result { + let context = mutation_runtime_context(cli, output)?; + let plan = plan.with_context(context.profile, Some(&context.server)); + confirm_mutation(cli, &plan, context.output)?; + Ok(plan) +} + +fn confirm_mutation(cli: &Cli, plan: &MutationPlan, output: OutputFormat) -> Result<(), CliError> { + let interactive = output == OutputFormat::Table && is_interactive_terminal(); + let stdin = io::stdin(); + let stderr = io::stderr(); + confirm_mutation_with_io( + cli.global.yes, + interactive, + plan, + output, + &mut stdin.lock(), + &mut stderr.lock(), + ) +} + +fn confirm_mutation_with_io( + assume_yes: bool, + interactive: bool, + plan: &MutationPlan, + output: OutputFormat, + reader: &mut R, + writer: &mut W, +) -> Result<(), CliError> { + if output == OutputFormat::Table && (assume_yes || interactive) { + let summary = Redactor.redact_text(&plan.render_table()); + writeln!(writer, "{summary}") + .map_err(|error| CliError::invalid_input(&error.to_string(), output))?; } - load_config(output) - .ok() - .and_then(|loaded| resolve_output(cli, &loaded, output).ok()) - .unwrap_or(output) + if assume_yes { + return Ok(()); + } + + if !interactive { + return Err(CliError::confirmation_required_with_plan( + "This command mutates Apollo state. Re-run with --yes in non-interactive or JSON mode.", + plan.clone(), + output, + )); + } + + loop { + write!(writer, "Proceed with this mutation? [y/N] ") + .and_then(|_| writer.flush()) + .map_err(|error| CliError::invalid_input(&error.to_string(), output))?; + let mut line = String::new(); + let bytes_read = reader + .read_line(&mut line) + .map_err(|error| CliError::invalid_input(&error.to_string(), output))?; + if bytes_read == 0 { + return Err(CliError::confirmation_required( + "Mutation cancelled because confirmation input ended; no changes were made.", + output, + )); + } + match line.trim().to_ascii_lowercase().as_str() { + "y" | "yes" => return Ok(()), + "" | "n" | "no" => { + return Err(CliError::confirmation_required( + "Mutation cancelled; no changes were made.", + output, + )); + } + _ => { + writeln!(writer, "Please answer y or n.") + .map_err(|error| CliError::invalid_input(&error.to_string(), output))?; + } + } + } } fn explicit_server(cli: &Cli) -> Option { @@ -1768,17 +2018,6 @@ fn append_optional_query(path: String, key: &str, value: Option<&str>) -> String } } -fn require_yes(cli: &Cli, output: OutputFormat) -> Result<(), CliError> { - if cli.global.yes { - Ok(()) - } else { - Err(CliError::confirmation_required( - "This command mutates Apollo state. Re-run with --yes to confirm.", - output, - )) - } -} - fn cluster_namespaces_path(env: &str, app: &str, cluster: &str) -> String { format!( "/openapi/v1/envs/{}/apps/{}/clusters/{}/namespaces", @@ -2638,6 +2877,19 @@ mod tests { use crate::cli::OutputFormat; use crate::config::{CredentialRef, ProfileConfig}; + use crate::mutation::{MutationPlan, MutationScope}; + + fn mutation_plan() -> MutationPlan { + MutationPlan::new("config.set") + .with_context(Some("dev".to_owned()), Some("https://apollo.example.com")) + .with_target(MutationScope::namespace( + "demo", + "PROD", + "default", + "application", + )) + .with_key("timeout") + } #[test] fn read_prompt_line_reports_eof_as_aborted_input() { @@ -2648,6 +2900,119 @@ mod tests { assert_eq!(error.exit_code(), 1); } + #[test] + fn tty_mutation_confirmation_accepts_yes_after_showing_plan() { + let mut reader = Cursor::new(b"yes\n".to_vec()); + let mut writer = Vec::new(); + + super::confirm_mutation_with_io( + false, + true, + &mutation_plan(), + OutputFormat::Table, + &mut reader, + &mut writer, + ) + .expect("yes should confirm"); + + let output = String::from_utf8(writer).expect("utf8 output"); + assert!(output.contains("Mutation plan:")); + assert!(output.contains("Operation: config.set")); + assert!(output.contains("Target: app=demo env=PROD")); + assert!(output.contains("Proceed with this mutation? [y/N]")); + } + + #[test] + fn tty_mutation_confirmation_rejects_no_blank_and_eof() { + for input in [b"no\n".as_slice(), b"\n".as_slice(), b"".as_slice()] { + let mut reader = Cursor::new(input.to_vec()); + let mut writer = Vec::new(); + let error = super::confirm_mutation_with_io( + false, + true, + &mutation_plan(), + OutputFormat::Table, + &mut reader, + &mut writer, + ) + .expect_err("confirmation should be rejected"); + + let rendered = error.render(); + assert!(rendered.body.contains("no changes were made")); + assert!( + String::from_utf8(writer) + .expect("utf8 output") + .contains("[y/N]") + ); + } + } + + #[test] + fn tty_mutation_confirmation_reprompts_invalid_input() { + let mut reader = Cursor::new(b"maybe\ny\n".to_vec()); + let mut writer = Vec::new(); + + super::confirm_mutation_with_io( + false, + true, + &mutation_plan(), + OutputFormat::Table, + &mut reader, + &mut writer, + ) + .expect("eventual yes should confirm"); + + let output = String::from_utf8(writer).expect("utf8 output"); + assert!(output.contains("Please answer y or n.")); + assert_eq!(output.matches("Proceed with this mutation?").count(), 2); + } + + #[test] + fn noninteractive_confirmation_returns_json_plan() { + let mut reader = Cursor::new(Vec::::new()); + let mut writer = Vec::new(); + let error = super::confirm_mutation_with_io( + false, + false, + &mutation_plan(), + OutputFormat::Json, + &mut reader, + &mut writer, + ) + .expect_err("non-interactive mutation should require --yes"); + + let rendered = error.render(); + let json: serde_json::Value = + serde_json::from_str(&rendered.body).expect("confirmation json"); + assert_eq!(json["error"]["code"], "confirmation_required"); + assert_eq!(json["error"]["operation"]["operation"], "config.set"); + assert!(writer.is_empty()); + } + + #[test] + fn yes_in_table_mode_prints_a_redacted_plan_without_prompting() { + let plan = MutationPlan::new("release.create") + .with_context(None, Some("https://apollo.example.com")) + .with_release("apollo_pat_secret_title", false); + let mut reader = Cursor::new(Vec::::new()); + let mut writer = Vec::new(); + + super::confirm_mutation_with_io( + true, + false, + &plan, + OutputFormat::Table, + &mut reader, + &mut writer, + ) + .expect("--yes should confirm"); + + let output = String::from_utf8(writer).expect("utf8 output"); + assert!(output.contains("Release title: [REDACTED]")); + assert!(!output.contains("apollo_pat_secret_title")); + assert!(!output.contains("Proceed with this mutation?")); + } + #[test] fn replaced_credential_to_delete_uses_implicit_native_for_legacy_profiles() { let existing = ProfileConfig { diff --git a/src/error.rs b/src/error.rs index 3ae1c50..cbd98a6 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,4 +1,5 @@ use crate::cli::OutputFormat; +use crate::mutation::MutationPlan; use crate::output::{OutputWriter, RenderedOutput, StructuredError}; #[derive(Debug)] @@ -15,6 +16,7 @@ pub enum CliErrorKind { }, ConfirmationRequired { message: String, + operation: Option>, }, CredentialStoreUnavailable { message: String, @@ -99,6 +101,21 @@ impl CliError { Self { kind: CliErrorKind::ConfirmationRequired { message: message.to_owned(), + operation: None, + }, + format, + } + } + + pub fn confirmation_required_with_plan( + message: &str, + operation: MutationPlan, + format: OutputFormat, + ) -> Self { + Self { + kind: CliErrorKind::ConfirmationRequired { + message: message.to_owned(), + operation: Some(Box::new(operation)), }, format, } @@ -189,6 +206,7 @@ impl CliError { code: "parse_error", category: "invalid_input", message: message.clone(), + operation: None, command: None, follow_up_issue: Some(5631), path: None, @@ -200,6 +218,7 @@ impl CliError { code: "invalid_config", category: "invalid_input", message: format!("Invalid Apollo CLI config at {}: {}", path, message), + operation: None, command: None, follow_up_issue: None, path: Some(path.clone()), @@ -210,26 +229,30 @@ impl CliError { code: "missing_config_base", category: "invalid_input", message: format!("Cannot resolve Apollo CLI config path: {}", message), + operation: None, command: None, follow_up_issue: None, path: None, profile: None, }), - CliErrorKind::ConfirmationRequired { message } => OutputWriter::new(self.format) - .render_error(&StructuredError { + CliErrorKind::ConfirmationRequired { message, operation } => { + OutputWriter::new(self.format).render_error(&StructuredError { code: "confirmation_required", category: "confirmation_required", message: message.clone(), + operation: operation.as_deref().cloned(), command: None, follow_up_issue: Some(5626), path: None, profile: None, - }), + }) + } CliErrorKind::CredentialStoreUnavailable { message } => OutputWriter::new(self.format) .render_error(&StructuredError { code: "credential_store_unavailable", category: "unsupported_operation", message: message.clone(), + operation: None, command: Some("auth".to_owned()), follow_up_issue: Some(5630), path: None, @@ -240,6 +263,7 @@ impl CliError { code: "invalid_input", category: "invalid_input", message: message.clone(), + operation: None, command: None, follow_up_issue: None, path: None, @@ -251,6 +275,7 @@ impl CliError { code: "authentication_failed", category: "authentication_failed", message: message.clone(), + operation: None, command: Some("auth".to_owned()), follow_up_issue: Some(5630), path: None, @@ -261,6 +286,7 @@ impl CliError { code: "network_error", category: "network", message: format!("OpenAPI request to {} failed: {}", path, message), + operation: None, command: None, follow_up_issue: None, path: Some(path.clone()), @@ -280,6 +306,7 @@ impl CliError { "OpenAPI request to {} returned HTTP {}: {}", path, status, message ), + operation: None, command: None, follow_up_issue: None, path: Some(path.clone()), @@ -291,6 +318,7 @@ impl CliError { code: "profile_not_found", category: "not_found", message: format!("Profile '{}' was not found.", profile), + operation: None, command: Some("profile".to_owned()), follow_up_issue: Some(5629), path: None, @@ -304,6 +332,7 @@ impl CliError { "Profile '{}' already exists. Re-run with --overwrite to replace it.", profile ), + operation: None, command: Some(command.clone()), follow_up_issue: None, path: None, diff --git a/src/lib.rs b/src/lib.rs index b457612..4eb4379 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -4,6 +4,7 @@ mod config; mod credential; mod error; mod http; +mod mutation; mod output; pub mod redaction; diff --git a/src/mutation.rs b/src/mutation.rs new file mode 100644 index 0000000..7ef5dea --- /dev/null +++ b/src/mutation.rs @@ -0,0 +1,348 @@ +use serde::Serialize; + +const MAX_TABLE_VALUE_CHARS: usize = 200; + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct MutationPlan { + pub operation: String, + #[serde(skip_serializing_if = "Option::is_none")] + pub profile: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub server: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub source: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub target: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub key: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub key_count: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub release_title: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub emergency: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub release_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub to_release_id: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub public_namespace: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub append_namespace_prefix: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub request: Option, +} + +impl MutationPlan { + pub fn new(operation: impl Into) -> Self { + Self { + operation: operation.into(), + profile: None, + server: None, + source: None, + target: None, + key: None, + key_count: None, + release_title: None, + emergency: None, + release_id: None, + to_release_id: None, + public_namespace: None, + append_namespace_prefix: None, + request: None, + } + } + + pub fn with_context(mut self, profile: Option, server: Option<&str>) -> Self { + self.profile = profile; + self.server = server.map(sanitize_server); + self + } + + pub fn with_source(mut self, source: MutationScope) -> Self { + self.source = Some(source); + self + } + + pub fn with_target(mut self, target: MutationScope) -> Self { + self.target = Some(target); + self + } + + pub fn with_key(mut self, key: impl Into) -> Self { + self.key = Some(key.into()); + self.key_count = Some(1); + self + } + + pub fn with_release(mut self, title: impl Into, emergency: bool) -> Self { + self.release_title = Some(title.into()); + self.emergency = Some(emergency); + self + } + + pub fn with_release_ids(mut self, release_id: i64, to_release_id: Option) -> Self { + self.release_id = Some(release_id); + self.to_release_id = to_release_id; + self + } + + pub fn with_namespace_kind(mut self, public: bool, append_prefix: bool) -> Self { + self.public_namespace = Some(public); + self.append_namespace_prefix = Some(append_prefix); + self + } + + pub fn with_request(mut self, method: impl Into, path: &str) -> Self { + self.request = Some(MutationRequest::new(method, path)); + self + } + + pub fn render_table(&self) -> String { + let mut lines = vec![ + "Mutation plan:".to_owned(), + format!("Operation: {}", table_value(&self.operation)), + ]; + push_optional(&mut lines, "Profile", self.profile.as_deref()); + push_optional(&mut lines, "Server", self.server.as_deref()); + if let Some(source) = &self.source { + lines.push(format!("Source: {}", source.render_table())); + } + if let Some(target) = &self.target { + lines.push(format!("Target: {}", target.render_table())); + } + push_optional(&mut lines, "Key", self.key.as_deref()); + if let Some(key_count) = self.key_count { + lines.push(format!("Key count: {key_count}")); + } + push_optional(&mut lines, "Release title", self.release_title.as_deref()); + if let Some(emergency) = self.emergency { + lines.push(format!("Emergency: {emergency}")); + } + if let Some(release_id) = self.release_id { + lines.push(format!("Release ID: {release_id}")); + } + if let Some(to_release_id) = self.to_release_id { + lines.push(format!("Target release ID: {to_release_id}")); + } + if let Some(public_namespace) = self.public_namespace { + lines.push(format!("Public namespace: {public_namespace}")); + } + if let Some(append_namespace_prefix) = self.append_namespace_prefix { + lines.push(format!( + "Append namespace prefix: {append_namespace_prefix}" + )); + } + if let Some(request) = &self.request { + lines.push(format!("Method: {}", table_value(&request.method))); + lines.push(format!("Path: {}", table_value(&request.path))); + if !request.query_parameters.is_empty() { + lines.push(format!( + "Query parameters: {}", + request + .query_parameters + .iter() + .map(|value| table_value(value)) + .collect::>() + .join(", ") + )); + } + } + lines.join("\n") + } +} + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct MutationScope { + #[serde(skip_serializing_if = "Option::is_none")] + pub app: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub env: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub cluster: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub namespace: Option, +} + +impl MutationScope { + pub fn namespace( + app: impl Into, + env: impl Into, + cluster: impl Into, + namespace: impl Into, + ) -> Self { + Self { + app: Some(app.into()), + env: Some(env.into()), + cluster: Some(cluster.into()), + namespace: Some(namespace.into()), + } + } + + pub fn environment(env: impl Into) -> Self { + Self { + app: None, + env: Some(env.into()), + cluster: None, + namespace: None, + } + } + + fn render_table(&self) -> String { + let fields = [ + ("app", self.app.as_deref()), + ("env", self.env.as_deref()), + ("cluster", self.cluster.as_deref()), + ("namespace", self.namespace.as_deref()), + ]; + fields + .into_iter() + .filter_map(|(name, value)| value.map(|value| format!("{name}={}", table_value(value)))) + .collect::>() + .join(" ") + } +} + +#[derive(Clone, Debug, Eq, PartialEq, Serialize)] +#[serde(rename_all = "camelCase")] +pub struct MutationRequest { + pub method: String, + pub path: String, + #[serde(skip_serializing_if = "Vec::is_empty")] + pub query_parameters: Vec, +} + +impl MutationRequest { + fn new(method: impl Into, path: &str) -> Self { + let path = path.split('#').next().unwrap_or_default(); + let (path, query) = path.split_once('?').unwrap_or((path, "")); + let query_parameters = query + .split('&') + .filter_map(|pair| pair.split('=').next()) + .map(str::trim) + .filter(|name| !name.is_empty()) + .map(ToOwned::to_owned) + .collect(); + Self { + method: method.into(), + path: sanitize_path(path), + query_parameters, + } + } +} + +fn sanitize_server(server: &str) -> String { + let without_fragment = server.split('#').next().unwrap_or_default(); + let without_query = without_fragment.split('?').next().unwrap_or_default(); + let Some((scheme, remainder)) = without_query.split_once("://") else { + return without_query.to_owned(); + }; + let authority_end = remainder.find('/').unwrap_or(remainder.len()); + let (authority, suffix) = remainder.split_at(authority_end); + let authority = authority + .rsplit_once('@') + .map_or(authority, |(_, host)| host); + format!("{scheme}://{authority}{}", sanitize_path(suffix)) +} + +fn sanitize_path(path: &str) -> String { + let mut redact_next = false; + path.split('/') + .map(|segment| { + if redact_next { + redact_next = false; + return "[REDACTED]".to_owned(); + } + let lowercase = segment.to_ascii_lowercase(); + redact_next = lowercase.contains("token") + || lowercase.contains("authorization") + || lowercase.contains("password") + || lowercase.contains("secret"); + segment.to_owned() + }) + .collect::>() + .join("/") +} + +fn push_optional(lines: &mut Vec, label: &str, value: Option<&str>) { + if let Some(value) = value { + lines.push(format!("{label}: {}", table_value(value))); + } +} + +fn table_value(value: &str) -> String { + let mut truncated = false; + let value = value + .chars() + .filter_map(|character| { + if character == '\n' || character == '\r' || character == '\t' { + Some(' ') + } else if character.is_control() { + None + } else { + Some(character) + } + }) + .take(MAX_TABLE_VALUE_CHARS + 1) + .enumerate() + .filter_map(|(index, character)| { + if index == MAX_TABLE_VALUE_CHARS { + truncated = true; + None + } else { + Some(character) + } + }) + .collect::(); + if truncated { + format!("{value}...") + } else { + value + } +} + +#[cfg(test)] +mod tests { + use super::{MutationPlan, MutationScope}; + + #[test] + fn request_plan_omits_query_values_and_sensitive_path_segments() { + let plan = MutationPlan::new("api.post") + .with_context( + Some("dev".to_owned()), + Some("https://user:password@example.com/tokens/server-secret?token=secret"), + ) + .with_request( + "POST", + "/openapi/v1/tokens/consumer-secret/apps?operator=alice&token=secret", + ); + let json = serde_json::to_value(&plan).expect("plan json"); + + assert_eq!(json["server"], "https://example.com/tokens/[REDACTED]"); + assert_eq!( + json["request"]["path"], + "/openapi/v1/tokens/[REDACTED]/apps" + ); + assert_eq!(json["request"]["queryParameters"][0], "operator"); + assert_eq!(json["request"]["queryParameters"][1], "token"); + assert!(!json.to_string().contains("consumer-secret")); + } + + #[test] + fn table_plan_replaces_control_characters() { + let plan = MutationPlan::new("config.set") + .with_target(MutationScope::namespace( + "demo", + "PROD", + "default", + "application", + )) + .with_key("safe\nInjected: value"); + + let table = plan.render_table(); + assert!(table.contains("Key: safe Injected: value")); + assert!(!table.contains("safe\nInjected")); + } +} diff --git a/src/output.rs b/src/output.rs index 5a0044a..3338303 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,7 +1,8 @@ use serde::Serialize; -use serde_json::json; +use serde_json::{Value, json}; use crate::cli::OutputFormat; +use crate::mutation::MutationPlan; use crate::redaction::Redactor; #[derive(Debug, Eq, PartialEq)] @@ -48,7 +49,10 @@ impl OutputWriter { } } OutputFormat::Table => { - let mut lines = vec![redactor.redact_text(&error.message)]; + let mut lines = vec![error.message.clone()]; + if let Some(operation) = &error.operation { + lines.push(operation.render_table()); + } if let Some(command) = &error.command { lines.push(format!("Command: {}", command)); } @@ -57,7 +61,7 @@ impl OutputWriter { } RenderedOutput { stream: OutputStream::Stderr, - body: ensure_trailing_newline(lines.join("\n")), + body: ensure_trailing_newline(redactor.redact_text(&lines.join("\n"))), } } } @@ -78,6 +82,36 @@ impl OutputWriter { OutputFormat::Table => RenderedOutput::stdout(redactor.redact_text(&table_body)), } } + + pub fn render_mutation_success( + &self, + operation: &MutationPlan, + value: &T, + table_body: String, + ) -> RenderedOutput { + if self.format == OutputFormat::Table { + return self.render_success(value, table_body); + } + + let mut value = serde_json::to_value(value).expect("structured success json serialization"); + let operation = serde_json::to_value(operation).expect("mutation plan json serialization"); + match &mut value { + Value::Object(fields) => { + fields.insert("operation".to_owned(), operation); + } + value => { + *value = json!({ + "operation": operation, + "data": value.take(), + }); + } + } + let value = Redactor.redact_json(value); + RenderedOutput::stdout( + serde_json::to_string_pretty(&value) + .expect("structured mutation success json serialization"), + ) + } } #[derive(Clone, Debug, Eq, PartialEq, Serialize)] @@ -86,6 +120,8 @@ pub struct StructuredError { pub category: &'static str, pub message: String, #[serde(skip_serializing_if = "Option::is_none")] + pub operation: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub command: Option, #[serde(skip_serializing_if = "Option::is_none")] pub follow_up_issue: Option, diff --git a/tests/cli.rs b/tests/cli.rs index c662c75..8280f85 100644 --- a/tests/cli.rs +++ b/tests/cli.rs @@ -41,7 +41,7 @@ fn help_lists_v0_command_groups_and_global_flags() { )) .stdout(predicate::str::contains("Render output as json or table")) .stdout(predicate::str::contains( - "Skip confirmation prompts for mutating OpenAPI requests", + "Approve mutating OpenAPI requests without an interactive prompt", )); } @@ -58,7 +58,7 @@ fn openapi_command_without_token_returns_structured_json_error() { "list", ]) .assert() - .failure(); + .code(1); assert!(assert.get_output().stdout.is_empty()); let output = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 output"); @@ -82,7 +82,7 @@ fn parse_errors_redact_token_like_arguments_and_honor_json_output() { "apollo_pat_secret_token", ]) .assert() - .failure(); + .code(2); assert!(assert.get_output().stdout.is_empty()); let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); diff --git a/tests/openapi.rs b/tests/openapi.rs index 9d3aa4a..d3677f7 100644 --- a/tests/openapi.rs +++ b/tests/openapi.rs @@ -793,8 +793,9 @@ fn single_read_commands_redact_broad_config_values() { #[test] fn mutating_commands_require_yes_before_network_call() { + let server = TestServer::empty(); let home = temp_home(); - write_config(&home, &profile_config("http://127.0.0.1:9")); + write_config(&home, &profile_config(&server.url())); let assert = base_command(&home) .env("APOLLO_TOKEN", "consumer-token") @@ -802,12 +803,98 @@ fn mutating_commands_require_yes_before_network_call() { "--output", "json", "config", "set", "--env", "DEV", "--app", "demo", "timeout", "3000", ]) .assert() - .failure(); + .code(1); assert!(assert.get_output().stdout.is_empty()); let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); let json: Value = serde_json::from_str(&stderr).expect("json stderr"); assert_eq!(json["error"]["code"], "confirmation_required"); + assert_eq!(json["error"]["operation"]["operation"], "config.set"); + assert_eq!(json["error"]["operation"]["profile"], "dev"); + assert_eq!(json["error"]["operation"]["server"], server.url()); + assert_eq!(json["error"]["operation"]["target"]["app"], "demo"); + assert_eq!(json["error"]["operation"]["target"]["env"], "DEV"); + assert_eq!(json["error"]["operation"]["key"], "timeout"); + assert_eq!(json["error"]["operation"]["keyCount"], 1); + server.assert_no_request(); +} + +#[test] +fn yes_in_table_mode_prints_target_summary_before_mutation() { + let server = TestServer::json(r#"{"key":"timeout","value":"3000"}"#); + let home = temp_home(); + write_config(&home, &profile_config(&server.url())); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "consumer-secret") + .args([ + "--yes", + "config", + "set", + "--env", + "PROD", + "--app", + "demo", + "db.password", + "s3cr3t", + ]) + .assert() + .success(); + + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + assert!(stderr.contains("Mutation plan:")); + assert!(stderr.contains("Operation: config.set")); + assert!(stderr.contains("Target: app=demo env=PROD cluster=default namespace=application")); + assert!(stderr.contains("Key: db.password")); + assert!(!stderr.contains("s3cr3t")); + assert!(!stderr.contains("consumer-secret")); + + let request = server.request(); + assert!(request.body.contains("s3cr3t")); +} + +#[test] +fn api_mutation_json_plan_sanitizes_path_query_and_body() { + let server = TestServer::empty(); + let home = temp_home(); + let path = "/openapi/v1/tokens/consumer-secret/apps?operator=alice&token=consumer-secret"; + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "consumer-secret") + .args([ + "--server", + &server.url(), + "--yes", + "--output", + "json", + "api", + "post", + path, + "--body", + r#"{"password":"s3cr3t"}"#, + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("json stdout"); + assert_eq!(json["operation"]["operation"], "api.post"); + assert_eq!(json["operation"]["request"]["method"], "POST"); + assert_eq!( + json["operation"]["request"]["path"], + "/openapi/v1/tokens/[REDACTED]/apps" + ); + assert_eq!( + json["operation"]["request"]["queryParameters"], + serde_json::json!(["operator", "token"]) + ); + assert!(!stdout.contains("consumer-secret")); + assert!(!stdout.contains("s3cr3t")); + + let request = server.request(); + assert_eq!(request.method, "POST"); + assert_eq!(request.path, path); + assert!(request.body.contains("s3cr3t")); } #[test] @@ -815,7 +902,7 @@ fn user_token_config_set_does_not_require_or_send_operator() { let server = TestServer::json(r#"{"key":"timeout","value":"3000"}"#); let home = temp_home(); - base_command(&home) + let assert = base_command(&home) .env("APOLLO_TOKEN", "apollo_pat_test_token") .args([ "--server", @@ -835,6 +922,14 @@ fn user_token_config_set_does_not_require_or_send_operator() { .assert() .success(); + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("json stdout"); + assert_eq!(json["status"], 200); + assert_eq!(json["operation"]["operation"], "config.set"); + assert_eq!(json["operation"]["target"]["namespace"], "application"); + assert_eq!(json["operation"]["key"], "timeout"); + assert_eq!(json["data"]["value"], "3000"); + let request = server.request(); assert_eq!( request.path, @@ -1183,7 +1278,7 @@ fn config_item_commands_use_encoded_items_for_path_sensitive_keys() { &home, &profile_config_with_operator(&delete_server.url(), "apollo-bot"), ); - base_command(&home) + let delete_assert = base_command(&home) .env("APOLLO_TOKEN", "consumer-token") .args([ "--yes", @@ -1199,6 +1294,10 @@ fn config_item_commands_use_encoded_items_for_path_sensitive_keys() { ]) .assert() .success(); + let stdout = String::from_utf8(delete_assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("delete json"); + assert_eq!(json["operation"]["operation"], "config.delete"); + assert_eq!(json["operation"]["key"], "logging/level"); assert_eq!( delete_server.request().path, "/openapi/v1/envs/DEV/apps/demo/clusters/default/namespaces/application/encodedItems/bG9nZ2luZy9sZXZlbA?operator=apollo-bot" @@ -1262,7 +1361,7 @@ fn namespace_create_with_yes_sends_namespace_instance_payload() { &profile_config_with_operator(&server.url(), "apollo-bot"), ); - base_command(&home) + let assert = base_command(&home) .env("APOLLO_TOKEN", "consumer-token") .args([ "--yes", @@ -1279,6 +1378,14 @@ fn namespace_create_with_yes_sends_namespace_instance_payload() { .assert() .success(); + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("namespace create json"); + assert_eq!(json["operation"]["operation"], "namespace.create"); + assert_eq!(json["operation"]["target"]["app"], "demo"); + assert_eq!(json["operation"]["target"]["namespace"], "settings.json"); + assert_eq!(json["operation"]["publicNamespace"], false); + assert_eq!(json["operation"]["appendNamespacePrefix"], true); + let requests = server.requests(3); assert_eq!(requests[0].method, "GET"); assert_eq!( @@ -2194,7 +2301,7 @@ fn user_token_release_writes_omit_operator_fields() { ); write_file_credential(&home, "dev", "apollo_pat_stored_token"); - base_command(&home) + let create_assert = base_command(&home) .args([ "--yes", "--output", "json", "release", "create", "--env", "DEV", "--app", "demo", "--title", "release", @@ -2202,13 +2309,38 @@ fn user_token_release_writes_omit_operator_fields() { .assert() .success(); - base_command(&home) + let stdout = String::from_utf8(create_assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("create json"); + assert_eq!(json["operation"]["operation"], "release.create"); + assert_eq!(json["operation"]["target"]["app"], "demo"); + assert_eq!(json["operation"]["target"]["env"], "DEV"); + assert_eq!(json["operation"]["releaseTitle"], "release"); + assert_eq!(json["operation"]["emergency"], false); + + let rollback_assert = base_command(&home) .args([ - "--yes", "--output", "json", "release", "rollback", "--env", "DEV", "42", + "--yes", + "--output", + "json", + "release", + "rollback", + "--env", + "DEV", + "42", + "--to-release-id", + "40", ]) .assert() .success(); + let stdout = + String::from_utf8(rollback_assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("rollback json"); + assert_eq!(json["operation"]["operation"], "release.rollback"); + assert_eq!(json["operation"]["target"]["env"], "DEV"); + assert_eq!(json["operation"]["releaseId"], 42); + assert_eq!(json["operation"]["toReleaseId"], 40); + let requests = server.requests(2); assert_eq!(requests[0].method, "POST"); assert_eq!( @@ -2221,7 +2353,7 @@ fn user_token_release_writes_omit_operator_fields() { assert_eq!(requests[1].method, "PUT"); assert_eq!( requests[1].path, - "/openapi/v1/envs/DEV/releases/42/rollback" + "/openapi/v1/envs/DEV/releases/42/rollback?toReleaseId=40" ); } @@ -2304,7 +2436,7 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { &profile_config_with_auth_mode(&server.url(), "user-token"), ); - base_command(&home) + let assert = base_command(&home) .env("APOLLO_TOKEN", "apollo_pat_test_token") .args([ "--yes", @@ -2322,6 +2454,13 @@ fn config_apply_with_yes_uses_synchronize_endpoint() { .assert() .success(); + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("apply json"); + assert_eq!(json["operation"]["operation"], "config.apply"); + assert_eq!(json["operation"]["source"]["env"], "DEV"); + assert_eq!(json["operation"]["target"]["env"], "FAT"); + assert_eq!(json["operation"]["target"]["namespace"], "application"); + let requests = server.requests(2); assert_eq!(requests[0].method, "GET"); assert_eq!( From 6874e4af3af456182dc3bd326298b66617c62079 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 18:24:59 +0800 Subject: [PATCH 2/5] fix: harden mutation feedback and plans --- src/command.rs | 211 ++++++++++++++++++++++++++++++++++------------- src/error.rs | 2 +- src/mutation.rs | 41 ++++++++- src/output.rs | 3 +- tests/openapi.rs | 111 ++++++++++++++++++++++--- 5 files changed, 297 insertions(+), 71 deletions(-) diff --git a/src/command.rs b/src/command.rs index f24d856..f6b41af 100644 --- a/src/command.rs +++ b/src/command.rs @@ -6,9 +6,9 @@ use serde::Serialize; use serde_json::{Value, json}; use crate::cli::{ - ApiArgs, AppCommand, AuthCommand, AuthMode, Cli, Commands, ConfigCommand, EnvCommand, InitArgs, - NamespaceCommand, NamespaceScopeArgs, OutputFormat, ProfileCommand, ReleaseCommand, - USER_TOKEN_PREFIX, + ApiArgs, AppCommand, AuthCommand, AuthMode, Cli, ClusterScopeArgs, Commands, ConfigCommand, + EnvCommand, InitArgs, NamespaceCommand, NamespaceScopeArgs, OutputFormat, ProfileCommand, + ReleaseCommand, USER_TOKEN_PREFIX, }; use crate::config::{ CredentialRef, LoadedConfig, ProfileConfig, RuntimeContext, load_config, read_env_output, @@ -550,9 +550,6 @@ fn execute_namespace( cli: &Cli, output: OutputFormat, ) -> Result { - let mutation_plan = namespace_mutation_plan(&command) - .map(|plan| prepare_mutation(cli, output, plan)) - .transpose()?; let openapi = openapi_context(cli, output)?; match command { NamespaceCommand::List { scope } => { @@ -588,19 +585,33 @@ fn execute_namespace( comment, append_namespace_prefix, } => { - let mutation_plan = mutation_plan - .as_ref() - .expect("namespace create mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, openapi.context.output, )?; - let app_namespace = register_app_namespace( + let app_namespace_plan = prepare_app_namespace_registration( &openapi, &scope.app, &name, public_namespace, + append_namespace_prefix, + )?; + let mutation_plan = prepare_mutation_with_openapi_context( + cli, + &openapi, + namespace_create_mutation_plan( + &scope, + &app_namespace_plan.name, + public_namespace, + append_namespace_prefix, + ), + )?; + let app_namespace = register_app_namespace( + &openapi, + &scope.app, + app_namespace_plan, + public_namespace, comment.as_deref(), append_namespace_prefix, operator.as_deref(), @@ -623,7 +634,7 @@ fn execute_namespace( "clusterName": &scope.cluster, "appNamespaceName": &app_namespace.name, }]); - match openapi.mutation_request(mutation_plan, "POST", &path, Some(body)) { + match openapi.mutation_request(&mutation_plan, "POST", &path, Some(body)) { Ok(output) => Ok(output), Err(error) if is_namespace_create_reported_failed(&error) => { match openapi @@ -634,7 +645,7 @@ fn execute_namespace( let data = redact_nested_item_values(response.data.clone()); Ok(render_mutation_response_with_data( &openapi.writer, - mutation_plan, + &mutation_plan, &response, data, )) @@ -687,15 +698,19 @@ struct RegisteredAppNamespace { created: bool, } -fn register_app_namespace( +struct PreparedAppNamespaceRegistration { + registration: AppNamespaceRegistration, + name: String, + should_register: bool, +} + +fn prepare_app_namespace_registration( openapi: &OpenApiCommandContext, app_id: &str, namespace_name: &str, public_namespace: bool, - comment: Option<&str>, append_namespace_prefix: bool, - operator: Option<&str>, -) -> Result { +) -> Result { let registration = app_namespace_registration(namespace_name); let mut checked_prefixed_public = false; match find_app_namespace(openapi, app_id, namespace_name)? { @@ -706,9 +721,10 @@ fn register_app_namespace( if let Some(existing) = find_prefixed_public_app_namespace(openapi, app_id, ®istration)? { - return Ok(RegisteredAppNamespace { + return Ok(PreparedAppNamespaceRegistration { + registration, name: existing.name, - created: false, + should_register: false, }); } } else { @@ -728,16 +744,18 @@ fn register_app_namespace( && append_namespace_prefix && matches!(existing.is_public, Some(false))) { - return Ok(RegisteredAppNamespace { + return Ok(PreparedAppNamespaceRegistration { + registration, name: existing.name, - created: false, + should_register: false, }); }; } AppNamespaceLookup::UnknownReadDenied => { - return Ok(RegisteredAppNamespace { + return Ok(PreparedAppNamespaceRegistration { + registration, name: namespace_name.to_owned(), - created: false, + should_register: false, }); } AppNamespaceLookup::Missing => {} @@ -747,8 +765,37 @@ fn register_app_namespace( && !checked_prefixed_public && let Some(existing) = find_prefixed_public_app_namespace(openapi, app_id, ®istration)? { - return Ok(RegisteredAppNamespace { + return Ok(PreparedAppNamespaceRegistration { + registration, name: existing.name, + should_register: false, + }); + } + + let name = if public_namespace && append_namespace_prefix { + prefixed_public_app_namespace_name(openapi, app_id, ®istration)? + } else { + app_namespace_registration_name(®istration) + }; + Ok(PreparedAppNamespaceRegistration { + registration, + name, + should_register: true, + }) +} + +fn register_app_namespace( + openapi: &OpenApiCommandContext, + app_id: &str, + prepared: PreparedAppNamespaceRegistration, + public_namespace: bool, + comment: Option<&str>, + append_namespace_prefix: bool, + operator: Option<&str>, +) -> Result { + if !prepared.should_register { + return Ok(RegisteredAppNamespace { + name: prepared.name, created: false, }); } @@ -766,8 +813,8 @@ fn register_app_namespace( } let mut body = json!({ "appId": app_id, - "name": registration.name, - "format": registration.format, + "name": prepared.registration.name, + "format": prepared.registration.format, "isPublic": public_namespace, "appendNamespacePrefix": append_namespace_prefix, }); @@ -778,17 +825,51 @@ fn register_app_namespace( body["dataChangeCreatedBy"] = json!(operator); } let response = openapi.client.request("POST", &path, Some(body))?; + let actual_name = response + .data + .get("name") + .and_then(Value::as_str) + .unwrap_or(&prepared.name); + if actual_name != prepared.name { + return Err(CliError::invalid_input( + &format!( + "Apollo registered AppNamespace '{actual_name}' instead of approved '{}'; the namespace instance was not created", + prepared.name + ), + openapi.context.output, + )); + } Ok(RegisteredAppNamespace { - name: response - .data - .get("name") - .and_then(Value::as_str) - .unwrap_or(namespace_name) - .to_owned(), + name: actual_name.to_owned(), created: true, }) } +fn prefixed_public_app_namespace_name( + openapi: &OpenApiCommandContext, + app_id: &str, + registration: &AppNamespaceRegistration, +) -> Result { + let path = format!("/openapi/v1/apps/{}", encode_path_segment(app_id)); + let response = openapi.client.request("GET", &path, None)?; + let org_id = response + .data + .get("orgId") + .and_then(Value::as_str) + .map(str::trim) + .filter(|org_id| !org_id.is_empty()) + .ok_or_else(|| { + CliError::invalid_input( + "Apollo did not return the app orgId required to preview the prefixed public namespace name", + openapi.context.output, + ) + })?; + Ok(format!( + "{org_id}.{}", + app_namespace_registration_name(registration) + )) +} + struct ExistingAppNamespace { name: String, is_public: Option, @@ -879,17 +960,21 @@ fn stored_public_app_namespace_matches( stored_name: &str, registration: &AppNamespaceRegistration, ) -> bool { - let requested_name = if registration.format == "properties" { - registration.name.clone() - } else { - format!("{}.{}", registration.name, registration.format) - }; + let requested_name = app_namespace_registration_name(registration); stored_name == requested_name || stored_name .split_once('.') .is_some_and(|(_, suffix)| suffix == requested_name) } +fn app_namespace_registration_name(registration: &AppNamespaceRegistration) -> String { + if registration.format == "properties" { + registration.name.clone() + } else { + format!("{}.{}", registration.name, registration.format) + } +} + fn is_missing_app_namespace(error: &CliError) -> bool { matches!(error.http_status_code(), Some(404)) || (matches!(error.http_status_code(), Some(400)) @@ -1541,7 +1626,12 @@ fn render_mutation_response( operation: &MutationPlan, response: &OpenApiResponse, ) -> RenderedOutput { - writer.render_mutation_success(operation, response, response.render_table()) + let table_body = if response.data.is_null() { + format!("Mutation '{}' completed successfully.", operation.operation) + } else { + response.render_table() + }; + writer.render_mutation_success(operation, response, table_body) } fn render_openapi_response_with_data( @@ -1760,26 +1850,20 @@ fn redact_release_configurations_in_value(value: &mut Value) { } } -fn namespace_mutation_plan(command: &NamespaceCommand) -> Option { - match command { - NamespaceCommand::Create { - scope, +fn namespace_create_mutation_plan( + scope: &ClusterScopeArgs, + name: &str, + public_namespace: bool, + append_namespace_prefix: bool, +) -> MutationPlan { + MutationPlan::new("namespace.create") + .with_target(MutationScope::namespace( + &scope.app, + &scope.env, + &scope.cluster, name, - public_namespace, - append_namespace_prefix, - .. - } => Some( - MutationPlan::new("namespace.create") - .with_target(MutationScope::namespace( - &scope.app, - &scope.env, - &scope.cluster, - name, - )) - .with_namespace_kind(*public_namespace, *append_namespace_prefix), - ), - NamespaceCommand::List { .. } | NamespaceCommand::Get { .. } => None, - } + )) + .with_namespace_kind(public_namespace, append_namespace_prefix) } fn config_mutation_plan(command: &ConfigCommand) -> Option { @@ -1874,6 +1958,19 @@ fn prepare_mutation( Ok(plan) } +fn prepare_mutation_with_openapi_context( + cli: &Cli, + openapi: &OpenApiCommandContext, + plan: MutationPlan, +) -> Result { + let plan = plan.with_context( + openapi.context.profile.clone(), + openapi.context.server.as_deref(), + ); + confirm_mutation(cli, &plan, openapi.context.output)?; + Ok(plan) +} + fn confirm_mutation(cli: &Cli, plan: &MutationPlan, output: OutputFormat) -> Result<(), CliError> { let interactive = output == OutputFormat::Table && is_interactive_terminal(); let stdin = io::stdin(); @@ -2939,6 +3036,7 @@ mod tests { let rendered = error.render(); assert!(rendered.body.contains("no changes were made")); + assert!(!rendered.body.contains("Follow-up issue")); assert!( String::from_utf8(writer) .expect("utf8 output") @@ -2986,6 +3084,7 @@ mod tests { serde_json::from_str(&rendered.body).expect("confirmation json"); assert_eq!(json["error"]["code"], "confirmation_required"); assert_eq!(json["error"]["operation"]["operation"], "config.set"); + assert!(json["error"].get("follow_up_issue").is_none()); assert!(writer.is_empty()); } diff --git a/src/error.rs b/src/error.rs index cbd98a6..6a5d9e5 100644 --- a/src/error.rs +++ b/src/error.rs @@ -242,7 +242,7 @@ impl CliError { message: message.clone(), operation: operation.as_deref().cloned(), command: None, - follow_up_issue: Some(5626), + follow_up_issue: None, path: None, profile: None, }) diff --git a/src/mutation.rs b/src/mutation.rs index 7ef5dea..7035b8d 100644 --- a/src/mutation.rs +++ b/src/mutation.rs @@ -256,11 +256,34 @@ fn sanitize_path(path: &str) -> String { return "[REDACTED]".to_owned(); } let lowercase = segment.to_ascii_lowercase(); - redact_next = lowercase.contains("token") + let contains_sensitive_marker = lowercase.contains("token") || lowercase.contains("authorization") || lowercase.contains("password") || lowercase.contains("secret"); - segment.to_owned() + if !contains_sensitive_marker { + return segment.to_owned(); + } + + let is_route_marker = matches!( + lowercase.as_str(), + "token" + | "tokens" + | "authorization" + | "authorizations" + | "password" + | "passwords" + | "secret" + | "secrets" + | "user-tokens" + | "consumer-tokens" + | "consumers" + ); + if is_route_marker { + redact_next = true; + segment.to_owned() + } else { + "[REDACTED]".to_owned() + } }) .collect::>() .join("/") @@ -328,6 +351,20 @@ mod tests { assert_eq!(json["request"]["queryParameters"][0], "operator"); assert_eq!(json["request"]["queryParameters"][1], "token"); assert!(!json.to_string().contains("consumer-secret")); + + let inline_secret_plan = MutationPlan::new("api.post").with_request( + "POST", + "/openapi/v1/reset-token-abc/clientSecret=def/password-value", + ); + let inline_secret_json = + serde_json::to_value(&inline_secret_plan).expect("inline-secret plan json"); + assert_eq!( + inline_secret_json["request"]["path"], + "/openapi/v1/[REDACTED]/[REDACTED]/[REDACTED]" + ); + assert!(!inline_secret_json.to_string().contains("abc")); + assert!(!inline_secret_json.to_string().contains("def")); + assert!(!inline_secret_json.to_string().contains("value")); } #[test] diff --git a/src/output.rs b/src/output.rs index 3338303..d23bd3d 100644 --- a/src/output.rs +++ b/src/output.rs @@ -106,7 +106,8 @@ impl OutputWriter { }); } } - let value = Redactor.redact_json(value); + let redactor = Redactor; + let value = redactor.redact_json(value); RenderedOutput::stdout( serde_json::to_string_pretty(&value) .expect("structured mutation success json serialization"), diff --git a/tests/openapi.rs b/tests/openapi.rs index d3677f7..aba183c 100644 --- a/tests/openapi.rs +++ b/tests/openapi.rs @@ -853,6 +853,33 @@ fn yes_in_table_mode_prints_target_summary_before_mutation() { assert!(request.body.contains("s3cr3t")); } +#[test] +fn table_mutation_with_empty_response_prints_success_message() { + let server = TestServer::new(200, "application/json", ""); + let home = temp_home(); + write_config(&home, &profile_config(&server.url())); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "consumer-token") + .args([ + "--yes", + "config", + "set", + "--env", + "LOCAL", + "--app", + "demo", + "feature.demo", + "try-it", + ]) + .assert() + .success(); + + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + assert_eq!(stdout, "Mutation 'config.set' completed successfully.\n"); + assert!(!stdout.contains("null")); +} + #[test] fn api_mutation_json_plan_sanitizes_path_query_and_body() { let server = TestServer::empty(); @@ -1933,6 +1960,7 @@ fn namespace_create_allows_prefixed_public_when_only_unprefixed_private_exists() r#"{"name":"application.yml","isPublic":false}"#, ), (200, "application/json", r#"[]"#), + (200, "application/json", r#"{"orgId":"FX"}"#), (200, "application/json", r#"{"name":"FX.application.yml"}"#), (200, "application/json", "{}"), ]); @@ -1942,7 +1970,7 @@ fn namespace_create_allows_prefixed_public_when_only_unprefixed_private_exists() &profile_config_with_operator(&server.url(), "apollo-bot"), ); - base_command(&home) + let assert = base_command(&home) .env("APOLLO_TOKEN", "consumer-token") .args([ "--yes", @@ -1960,23 +1988,32 @@ fn namespace_create_allows_prefixed_public_when_only_unprefixed_private_exists() .assert() .success(); - let requests = server.requests(4); + let stdout = String::from_utf8(assert.get_output().stdout.clone()).expect("utf8 stdout"); + let json: Value = serde_json::from_str(&stdout).expect("namespace create json"); + assert_eq!( + json["operation"]["target"]["namespace"], + "FX.application.yml" + ); + + let requests = server.requests(5); assert_eq!( requests[0].path, "/openapi/v1/apps/demo/appnamespaces/application.yml" ); assert_eq!(requests[1].path, "/openapi/v1/apps/demo/appnamespaces"); - assert_eq!(requests[2].method, "POST"); + assert_eq!(requests[2].method, "GET"); + assert_eq!(requests[2].path, "/openapi/v1/apps/demo"); + assert_eq!(requests[3].method, "POST"); assert_eq!( - requests[2].path, + requests[3].path, "/openapi/v1/apps/demo/appnamespaces?appendNamespacePrefix=true" ); - assert_eq!(requests[3].method, "POST"); + assert_eq!(requests[4].method, "POST"); assert_eq!( - requests[3].path, + requests[4].path, "/openapi/v1/namespaces?operator=apollo-bot" ); - let namespace_body: Value = serde_json::from_str(&requests[3].body).expect("json body"); + let namespace_body: Value = serde_json::from_str(&requests[4].body).expect("json body"); assert_eq!(namespace_body[0]["appNamespaceName"], "FX.application.yml"); } @@ -1989,6 +2026,7 @@ fn namespace_create_does_not_reuse_suffix_colliding_public_appnamespace() { "application/json", r#"[{"name":"FX.foo.application.yml","isPublic":true}]"#, ), + (200, "application/json", r#"{"orgId":"FX"}"#), (200, "application/json", r#"{"name":"FX.application.yml"}"#), (200, "application/json", "{}"), ]); @@ -2016,16 +2054,67 @@ fn namespace_create_does_not_reuse_suffix_colliding_public_appnamespace() { .assert() .success(); - let requests = server.requests(4); - assert_eq!(requests[2].method, "POST"); + let requests = server.requests(5); + assert_eq!(requests[2].method, "GET"); + assert_eq!(requests[2].path, "/openapi/v1/apps/demo"); + assert_eq!(requests[3].method, "POST"); assert_eq!( - requests[2].path, + requests[3].path, "/openapi/v1/apps/demo/appnamespaces?appendNamespacePrefix=true" ); - let namespace_body: Value = serde_json::from_str(&requests[3].body).expect("json body"); + let namespace_body: Value = serde_json::from_str(&requests[4].body).expect("json body"); assert_eq!(namespace_body[0]["appNamespaceName"], "FX.application.yml"); } +#[test] +fn namespace_create_stops_if_apollo_returns_an_unapproved_prefixed_name() { + let server = TestServer::sequence(vec![ + (404, "application/json", r#"{"message":"not found"}"#), + (200, "application/json", r#"[]"#), + (200, "application/json", r#"{"orgId":"FX"}"#), + ( + 200, + "application/json", + r#"{"name":"OTHER.application.yml"}"#, + ), + ]); + let home = temp_home(); + write_config( + &home, + &profile_config_with_operator(&server.url(), "apollo-bot"), + ); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "consumer-token") + .args([ + "--yes", + "--output", + "json", + "namespace", + "create", + "--env", + "DEV", + "--app", + "demo", + "--public", + "application.yml", + ]) + .assert() + .code(1); + + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + assert!(stderr.contains("OTHER.application.yml")); + assert!(stderr.contains("FX.application.yml")); + assert!(stderr.contains("namespace instance was not created")); + + let requests = server.requests(4); + assert_eq!(requests[3].method, "POST"); + assert_eq!( + requests[3].path, + "/openapi/v1/apps/demo/appnamespaces?appendNamespacePrefix=true" + ); +} + #[test] fn namespace_create_treats_empty_appnamespace_lookup_as_missing() { let server = TestServer::sequence(vec![ From 3fc6bdca5ff160ac668131333de495b02da2d26e Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 18:32:22 +0800 Subject: [PATCH 3/5] refactor: remove follow-up issue errors --- README.md | 3 +-- README.zh.md | 2 +- src/command.rs | 2 -- src/error.rs | 11 ----------- src/output.rs | 5 ----- 5 files changed, 2 insertions(+), 21 deletions(-) diff --git a/README.md b/README.md index 2f78348..eb0c40d 100644 --- a/README.md +++ b/README.md @@ -277,8 +277,7 @@ Structured JSON errors include: - `code`: stable error code - `category`: stable category - `message`: human-readable message -- optional non-sensitive details such as `command`, `profile`, `path`, `operation`, or - `follow_up_issue` +- optional non-sensitive details such as `command`, `profile`, `path`, or `operation` Current error categories: diff --git a/README.zh.md b/README.zh.md index 697ca48..4be9d8c 100644 --- a/README.zh.md +++ b/README.zh.md @@ -244,7 +244,7 @@ apollo --profile dev auth capabilities - `code`：稳定错误码 - `category`：稳定错误分类 - `message`：人类可读错误信息 -- 可选的非敏感详情，例如 `command`、`profile`、`path`、`operation` 或 `follow_up_issue` +- 可选的非敏感详情，例如 `command`、`profile`、`path` 或 `operation` 当前错误分类： diff --git a/src/command.rs b/src/command.rs index f6b41af..00535ea 100644 --- a/src/command.rs +++ b/src/command.rs @@ -3036,7 +3036,6 @@ mod tests { let rendered = error.render(); assert!(rendered.body.contains("no changes were made")); - assert!(!rendered.body.contains("Follow-up issue")); assert!( String::from_utf8(writer) .expect("utf8 output") @@ -3084,7 +3083,6 @@ mod tests { serde_json::from_str(&rendered.body).expect("confirmation json"); assert_eq!(json["error"]["code"], "confirmation_required"); assert_eq!(json["error"]["operation"]["operation"], "config.set"); - assert!(json["error"].get("follow_up_issue").is_none()); assert!(writer.is_empty()); } diff --git a/src/error.rs b/src/error.rs index 6a5d9e5..3c70ecf 100644 --- a/src/error.rs +++ b/src/error.rs @@ -208,7 +208,6 @@ impl CliError { message: message.clone(), operation: None, command: None, - follow_up_issue: Some(5631), path: None, profile: None, }) @@ -220,7 +219,6 @@ impl CliError { message: format!("Invalid Apollo CLI config at {}: {}", path, message), operation: None, command: None, - follow_up_issue: None, path: Some(path.clone()), profile: None, }), @@ -231,7 +229,6 @@ impl CliError { message: format!("Cannot resolve Apollo CLI config path: {}", message), operation: None, command: None, - follow_up_issue: None, path: None, profile: None, }), @@ -242,7 +239,6 @@ impl CliError { message: message.clone(), operation: operation.as_deref().cloned(), command: None, - follow_up_issue: None, path: None, profile: None, }) @@ -254,7 +250,6 @@ impl CliError { message: message.clone(), operation: None, command: Some("auth".to_owned()), - follow_up_issue: Some(5630), path: None, profile: None, }), @@ -265,7 +260,6 @@ impl CliError { message: message.clone(), operation: None, command: None, - follow_up_issue: None, path: None, profile: None, }) @@ -277,7 +271,6 @@ impl CliError { message: message.clone(), operation: None, command: Some("auth".to_owned()), - follow_up_issue: Some(5630), path: None, profile: None, }), @@ -288,7 +281,6 @@ impl CliError { message: format!("OpenAPI request to {} failed: {}", path, message), operation: None, command: None, - follow_up_issue: None, path: Some(path.clone()), profile: None, }) @@ -308,7 +300,6 @@ impl CliError { ), operation: None, command: None, - follow_up_issue: None, path: Some(path.clone()), profile: None, }) @@ -320,7 +311,6 @@ impl CliError { message: format!("Profile '{}' was not found.", profile), operation: None, command: Some("profile".to_owned()), - follow_up_issue: Some(5629), path: None, profile: Some(profile.clone()), }), @@ -334,7 +324,6 @@ impl CliError { ), operation: None, command: Some(command.clone()), - follow_up_issue: None, path: None, profile: Some(profile.clone()), }) diff --git a/src/output.rs b/src/output.rs index d23bd3d..478950f 100644 --- a/src/output.rs +++ b/src/output.rs @@ -56,9 +56,6 @@ impl OutputWriter { if let Some(command) = &error.command { lines.push(format!("Command: {}", command)); } - if let Some(issue) = error.follow_up_issue { - lines.push(format!("Follow-up issue: #{}", issue)); - } RenderedOutput { stream: OutputStream::Stderr, body: ensure_trailing_newline(redactor.redact_text(&lines.join("\n"))), @@ -125,8 +122,6 @@ pub struct StructuredError { #[serde(skip_serializing_if = "Option::is_none")] pub command: Option, #[serde(skip_serializing_if = "Option::is_none")] - pub follow_up_issue: Option, - #[serde(skip_serializing_if = "Option::is_none")] pub path: Option, #[serde(skip_serializing_if = "Option::is_none")] pub profile: Option, From e7a8dec8a597dfbfb7401830587991653d8c96da Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 18:40:28 +0800 Subject: [PATCH 4/5] fix: bind mutations to approved context --- README.md | 6 ++ README.zh.md | 2 + src/command.rs | 177 ++++++++++++++++++++++++++++++++++++++++------- tests/openapi.rs | 33 +++++++++ 4 files changed, 193 insertions(+), 25 deletions(-) diff --git a/README.md b/README.md index eb0c40d..c24805b 100644 --- a/README.md +++ b/README.md @@ -316,6 +316,12 @@ non-interactive mode and in JSON mode, mutations require `--yes`; otherwise the `confirmation_required` error whose `operation` field contains the redacted plan. Rejection occurs before any OpenAPI request is sent. +Namespace creation may perform read-only preflight requests after that initial approval. If Apollo +resolves a different effective namespace name, such as adding an organization prefix to a public +namespace, the CLI shows the resolved plan and requires approval again before either mutation is +sent. If the selected profile, server, or output mode changes after approval, the CLI aborts before +sending an OpenAPI request and asks the caller to review the new context. + With `--yes`, table mode still writes the plan before the request. A successful JSON response stays one valid JSON document and preserves the existing top-level `status` and `data` fields while adding the top-level `operation` plan. diff --git a/README.zh.md b/README.zh.md index 4be9d8c..13e49c4 100644 --- a/README.zh.md +++ b/README.zh.md @@ -273,6 +273,8 @@ apollo --profile dev auth capabilities 在交互式 table 模式中，未传 `--yes` 的变更会把计划和默认拒绝的 `[y/N]` 提示写到 stderr。只有输入 `y` 或 `yes` 才会执行；输入 `n`、`no`、空行或遇到 EOF 都会拒绝。在非交互模式和 JSON 模式中，变更必须显式传入 `--yes`；否则 CLI 返回 `confirmation_required`，其 `operation` 字段包含脱敏计划。拒绝发生在任何 OpenAPI 请求发送之前。 +namespace 创建只会在首次批准后发送只读预检请求。如果 Apollo 解析出的最终 namespace 名称发生变化，例如为公共 namespace 添加组织前缀，CLI 会展示解析后的计划，并在发送任何变更请求前再次要求批准。如果批准后选中的 profile、server 或输出模式发生变化，CLI 会在发送 OpenAPI 请求前中止，并要求调用方重新检查新的运行上下文。 + 传入 `--yes` 时，table 模式仍会在请求前输出计划。成功的 JSON 输出仍是一个完整 JSON 文档，并保留现有顶层 `status` 和 `data` 字段，同时新增顶层 `operation` 计划。 ## OpenAPI 行为 diff --git a/src/command.rs b/src/command.rs index 00535ea..e41e8f9 100644 --- a/src/command.rs +++ b/src/command.rs @@ -550,7 +550,29 @@ fn execute_namespace( cli: &Cli, output: OutputFormat, ) -> Result { + let mutation_plan = match &command { + NamespaceCommand::Create { + scope, + name, + public_namespace, + append_namespace_prefix, + .. + } => Some(prepare_mutation( + cli, + output, + namespace_create_mutation_plan( + scope, + name, + *public_namespace, + *append_namespace_prefix, + ), + )?), + NamespaceCommand::List { .. } | NamespaceCommand::Get { .. } => None, + }; let openapi = openapi_context(cli, output)?; + if let Some(mutation_plan) = mutation_plan.as_ref() { + ensure_approved_runtime_context(mutation_plan, &openapi.context)?; + } match command { NamespaceCommand::List { scope } => { ensure_consumer_token_scoped_read_supported(&openapi, "namespace list")?; @@ -585,6 +607,7 @@ fn execute_namespace( comment, append_namespace_prefix, } => { + let mutation_plan = mutation_plan.expect("namespace create mutation plan"); let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -597,16 +620,19 @@ fn execute_namespace( public_namespace, append_namespace_prefix, )?; - let mutation_plan = prepare_mutation_with_openapi_context( - cli, - &openapi, - namespace_create_mutation_plan( - &scope, - &app_namespace_plan.name, - public_namespace, - append_namespace_prefix, - ), - )?; + let resolved_plan = namespace_create_mutation_plan( + &scope, + &app_namespace_plan.name, + public_namespace, + append_namespace_prefix, + ); + let contextualized_resolved_plan = + mutation_plan_with_openapi_context(&openapi, resolved_plan.clone())?; + let mutation_plan = if contextualized_resolved_plan == mutation_plan.plan { + mutation_plan + } else { + prepare_mutation_with_openapi_context(cli, &openapi, resolved_plan)? + }; let app_namespace = register_app_namespace( &openapi, &scope.app, @@ -634,7 +660,7 @@ fn execute_namespace( "clusterName": &scope.cluster, "appNamespaceName": &app_namespace.name, }]); - match openapi.mutation_request(&mutation_plan, "POST", &path, Some(body)) { + match openapi.mutation_request(&mutation_plan.plan, "POST", &path, Some(body)) { Ok(output) => Ok(output), Err(error) if is_namespace_create_reported_failed(&error) => { match openapi @@ -645,7 +671,7 @@ fn execute_namespace( let data = redact_nested_item_values(response.data.clone()); Ok(render_mutation_response_with_data( &openapi.writer, - &mutation_plan, + &mutation_plan.plan, &response, data, )) @@ -1030,6 +1056,9 @@ fn execute_config( .map(|plan| prepare_mutation(cli, output, plan)) .transpose()?; let openapi = openapi_context(cli, output)?; + if let Some(mutation_plan) = mutation_plan.as_ref() { + ensure_approved_runtime_context(mutation_plan, &openapi.context)?; + } match command { ConfigCommand::List { scope, page, size } => { ensure_config_item_read_supported(&openapi)?; @@ -1057,7 +1086,10 @@ fn execute_config( comment, operator, } => { - let mutation_plan = mutation_plan.as_ref().expect("config set mutation plan"); + let mutation_plan = &mutation_plan + .as_ref() + .expect("config set mutation plan") + .plan; let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1107,7 +1139,10 @@ fn execute_config( key, operator, } => { - let mutation_plan = mutation_plan.as_ref().expect("config delete mutation plan"); + let mutation_plan = &mutation_plan + .as_ref() + .expect("config delete mutation plan") + .plan; let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1150,7 +1185,10 @@ fn execute_config( target_namespace, operator, } => { - let mutation_plan = mutation_plan.as_ref().expect("config apply mutation plan"); + let mutation_plan = &mutation_plan + .as_ref() + .expect("config apply mutation plan") + .plan; let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1191,6 +1229,9 @@ fn execute_release( .map(|plan| prepare_mutation(cli, output, plan)) .transpose()?; let openapi = openapi_context(cli, output)?; + if let Some(mutation_plan) = mutation_plan.as_ref() { + ensure_approved_runtime_context(mutation_plan, &openapi.context)?; + } match command { ReleaseCommand::List { scope, page, size } => { ensure_consumer_token_scoped_read_supported(&openapi, "release list")?; @@ -1215,6 +1256,7 @@ fn execute_release( let mutation_plan = mutation_plan .as_ref() .expect("release create mutation plan"); + let mutation_plan = &mutation_plan.plan; let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1247,6 +1289,7 @@ fn execute_release( let mutation_plan = mutation_plan .as_ref() .expect("release rollback mutation plan"); + let mutation_plan = &mutation_plan.plan; let operator = operator_for_mutation( operator.as_deref(), &openapi.context, @@ -1274,6 +1317,9 @@ fn execute_api(args: ApiArgs, cli: &Cli, output: OutputFormat) -> Result Some(serde_json::from_str::(&body).map_err(|error| { CliError::invalid_input(&error.to_string(), openapi.context.output) @@ -1282,7 +1328,7 @@ fn execute_api(args: ApiArgs, cli: &Cli, output: OutputFormat) -> Result { - openapi.mutation_request(mutation_plan, args.method.as_str(), &args.path, body) + openapi.mutation_request(&mutation_plan.plan, args.method.as_str(), &args.path, body) } None => openapi.request(args.method.as_str(), &args.path, body), } @@ -1417,6 +1463,13 @@ struct MutationRuntimeContext { output: OutputFormat, } +struct ApprovedMutation { + plan: MutationPlan, + profile: Option, + server: String, + output: OutputFormat, +} + impl OpenApiCommandContext { fn request( &self, @@ -1951,24 +2004,58 @@ fn prepare_mutation( cli: &Cli, output: OutputFormat, plan: MutationPlan, -) -> Result { +) -> Result { let context = mutation_runtime_context(cli, output)?; - let plan = plan.with_context(context.profile, Some(&context.server)); + let plan = plan.with_context(context.profile.clone(), Some(&context.server)); confirm_mutation(cli, &plan, context.output)?; - Ok(plan) + Ok(ApprovedMutation { + plan, + profile: context.profile, + server: context.server, + output: context.output, + }) } fn prepare_mutation_with_openapi_context( cli: &Cli, openapi: &OpenApiCommandContext, plan: MutationPlan, -) -> Result { - let plan = plan.with_context( - openapi.context.profile.clone(), - openapi.context.server.as_deref(), - ); +) -> Result { + let server = required_server(&openapi.context, openapi.context.output)?; + let plan = plan.with_context(openapi.context.profile.clone(), Some(&server)); confirm_mutation(cli, &plan, openapi.context.output)?; - Ok(plan) + Ok(ApprovedMutation { + plan, + profile: openapi.context.profile.clone(), + server, + output: openapi.context.output, + }) +} + +fn mutation_plan_with_openapi_context( + openapi: &OpenApiCommandContext, + plan: MutationPlan, +) -> Result { + let server = required_server(&openapi.context, openapi.context.output)?; + Ok(plan.with_context(openapi.context.profile.clone(), Some(&server))) +} + +fn ensure_approved_runtime_context( + approved: &ApprovedMutation, + context: &RuntimeContext, +) -> Result<(), CliError> { + let server = required_server(context, context.output)?; + if approved.profile == context.profile + && approved.server == server + && approved.output == context.output + { + return Ok(()); + } + + Err(CliError::invalid_input( + "Apollo runtime context changed after confirmation; no OpenAPI request was sent. Re-run the command to review the current profile and server.", + approved.output, + )) } fn confirm_mutation(cli: &Cli, plan: &MutationPlan, output: OutputFormat) -> Result<(), CliError> { @@ -3110,6 +3197,46 @@ mod tests { assert!(!output.contains("Proceed with this mutation?")); } + #[test] + fn approved_runtime_context_rejects_profile_server_or_output_changes() { + let approved = super::ApprovedMutation { + plan: mutation_plan(), + profile: Some("dev".to_owned()), + server: "https://apollo.example.com".to_owned(), + output: OutputFormat::Table, + }; + let matching = crate::config::RuntimeContext { + profile: Some("dev".to_owned()), + server: Some("https://apollo.example.com".to_owned()), + output: OutputFormat::Table, + auth_mode: crate::cli::AuthMode::ConsumerToken, + operator: None, + credential: None, + }; + super::ensure_approved_runtime_context(&approved, &matching) + .expect("matching runtime context"); + + for changed in [ + crate::config::RuntimeContext { + profile: Some("prod".to_owned()), + ..matching.clone() + }, + crate::config::RuntimeContext { + server: Some("https://other.example.com".to_owned()), + ..matching.clone() + }, + crate::config::RuntimeContext { + output: OutputFormat::Json, + ..matching.clone() + }, + ] { + let error = super::ensure_approved_runtime_context(&approved, &changed) + .expect_err("changed runtime context should be rejected"); + assert!(error.render().body.contains("runtime context changed")); + assert!(error.render().body.contains("no OpenAPI request was sent")); + } + } + #[test] fn replaced_credential_to_delete_uses_implicit_native_for_legacy_profiles() { let existing = ProfileConfig { diff --git a/tests/openapi.rs b/tests/openapi.rs index aba183c..2097558 100644 --- a/tests/openapi.rs +++ b/tests/openapi.rs @@ -819,6 +819,39 @@ fn mutating_commands_require_yes_before_network_call() { server.assert_no_request(); } +#[test] +fn namespace_create_requires_initial_confirmation_before_preflight_requests() { + let server = TestServer::empty(); + let home = temp_home(); + write_config(&home, &profile_config(&server.url())); + + let assert = base_command(&home) + .env("APOLLO_TOKEN", "consumer-token") + .args([ + "--output", + "json", + "namespace", + "create", + "--env", + "DEV", + "--app", + "demo", + "--public", + "application.yml", + ]) + .assert() + .code(1); + + let stderr = String::from_utf8(assert.get_output().stderr.clone()).expect("utf8 stderr"); + let json: Value = serde_json::from_str(&stderr).expect("json stderr"); + assert_eq!(json["error"]["code"], "confirmation_required"); + assert_eq!( + json["error"]["operation"]["target"]["namespace"], + "application.yml" + ); + server.assert_no_request(); +} + #[test] fn yes_in_table_mode_prints_target_summary_before_mutation() { let server = TestServer::json(r#"{"key":"timeout","value":"3000"}"#); From c3251b85918ace4acd4f6961edd2daaadca94d58 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Sun, 30 Aug 2026 18:46:10 +0800 Subject: [PATCH 5/5] fix: redact encoded sensitive path segments --- src/mutation.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/mutation.rs b/src/mutation.rs index 7035b8d..8b5612a 100644 --- a/src/mutation.rs +++ b/src/mutation.rs @@ -255,7 +255,10 @@ fn sanitize_path(path: &str) -> String { redact_next = false; return "[REDACTED]".to_owned(); } - let lowercase = segment.to_ascii_lowercase(); + let Ok(decoded) = urlencoding::decode(segment) else { + return "[REDACTED]".to_owned(); + }; + let lowercase = decoded.to_ascii_lowercase(); let contains_sensitive_marker = lowercase.contains("token") || lowercase.contains("authorization") || lowercase.contains("password") @@ -365,6 +368,19 @@ mod tests { assert!(!inline_secret_json.to_string().contains("abc")); assert!(!inline_secret_json.to_string().contains("def")); assert!(!inline_secret_json.to_string().contains("value")); + + let encoded_secret_plan = MutationPlan::new("api.post").with_request( + "POST", + "/openapi/v1/%74%6f%6b%65%6e-secret-abc/%74%6f%6b%65%6e%73/consumer-secret", + ); + let encoded_secret_json = + serde_json::to_value(&encoded_secret_plan).expect("encoded-secret plan json"); + assert_eq!( + encoded_secret_json["request"]["path"], + "/openapi/v1/[REDACTED]/%74%6f%6b%65%6e%73/[REDACTED]" + ); + assert!(!encoded_secret_json.to_string().contains("secret-abc")); + assert!(!encoded_secret_json.to_string().contains("consumer-secret")); } #[test]