From cceab35ec1a5201ededb44bb220b6c33e0728456 Mon Sep 17 00:00:00 2001 From: Mick Grove Date: Thu, 30 Apr 2026 10:56:35 -0700 Subject: [PATCH 1/4] copilot fixes --- src/direct_validate.rs | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-) diff --git a/src/direct_validate.rs b/src/direct_validate.rs index 508edec..c3a177a 100644 --- a/src/direct_validate.rs +++ b/src/direct_validate.rs @@ -595,10 +595,17 @@ pub async fn run_direct_validation( } } - // Execute validation based on type + // Execute validation based on type. Errors from the HTTP / gRPC + // pathways (DNS failure, SSRF preflight, request build, request + // execution, timeout) used to short-circuit the whole `validate` + // command via `?`, which left stdout empty and made downstream + // tools (and integration tests) unable to distinguish "no rule" + // from "validation attempted, infrastructure failed". Match the + // pattern used by AWS / GCP / raw branches below: surface the + // error as a non-valid result with the error in `message`. let mut result = match validation { Validation::Http(http_validation) => { - execute_http_validation( + match execute_http_validation( http_validation, &globals, &client, @@ -607,17 +614,37 @@ pub async fn run_direct_validation( args.retries, global_args.allow_internal_ips, ) - .await? + .await + { + Ok(r) => r, + Err(e) => DirectValidationResult { + rule_id: String::new(), + rule_name: String::new(), + is_valid: false, + status_code: None, + message: format!("HTTP validation error: {}", e), + }, + } } Validation::Grpc(grpc_validation_cfg) => { - execute_grpc_validation( + match execute_grpc_validation( grpc_validation_cfg, &globals, &parser, timeout, global_args.allow_internal_ips, ) - .await? + .await + { + Ok(r) => r, + Err(e) => DirectValidationResult { + rule_id: String::new(), + rule_name: String::new(), + is_valid: false, + status_code: None, + message: format!("gRPC validation error: {}", e), + }, + } } Validation::AWS => { From b89c95204342b59870a9bcd226d612f4bf1b2c0e Mon Sep 17 00:00:00 2001 From: Mick Grove Date: Thu, 30 Apr 2026 11:28:45 -0700 Subject: [PATCH 2/4] copilot fixes --- src/direct_validate.rs | 41 +++++++++++++++++++++++------------ tests/cli_validate_revoke.rs | 42 ++++++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/src/direct_validate.rs b/src/direct_validate.rs index c3a177a..ba53070 100644 --- a/src/direct_validate.rs +++ b/src/direct_validate.rs @@ -617,13 +617,22 @@ pub async fn run_direct_validation( .await { Ok(r) => r, - Err(e) => DirectValidationResult { - rule_id: String::new(), - rule_name: String::new(), - is_valid: false, - status_code: None, - message: format!("HTTP validation error: {}", e), - }, + Err(e) => { + // Don't surface the underlying error: it can embed + // the rendered URL with `{{ TOKEN }}` substituted + // (i.e. the secret) or `--var` / `--arg` values. + // Log the detail for debugging and emit a generic + // message to stdout. + debug!("HTTP validation failed: {e}"); + DirectValidationResult { + rule_id: String::new(), + rule_name: String::new(), + is_valid: false, + status_code: None, + message: "HTTP validation failed (see debug logs for details)" + .to_string(), + } + } } } Validation::Grpc(grpc_validation_cfg) => { @@ -637,13 +646,17 @@ pub async fn run_direct_validation( .await { Ok(r) => r, - Err(e) => DirectValidationResult { - rule_id: String::new(), - rule_name: String::new(), - is_valid: false, - status_code: None, - message: format!("gRPC validation error: {}", e), - }, + Err(e) => { + debug!("gRPC validation failed: {e}"); + DirectValidationResult { + rule_id: String::new(), + rule_name: String::new(), + is_valid: false, + status_code: None, + message: "gRPC validation failed (see debug logs for details)" + .to_string(), + } + } } } diff --git a/tests/cli_validate_revoke.rs b/tests/cli_validate_revoke.rs index ed018d2..f9de895 100644 --- a/tests/cli_validate_revoke.rs +++ b/tests/cli_validate_revoke.rs @@ -181,6 +181,48 @@ mod validate { .stdout(contains("Rule:").and(contains("Result:"))); } + /// HTTP infrastructure failures (DNS, SSRF preflight, connection + /// refused, etc.) must surface as a structured DirectValidationResult + /// rather than short-circuiting the validate command. Also verifies + /// the secret is not leaked into stdout via the error message. + #[test] + fn validate_http_failure_emits_structured_result() { + let secret = "ghp_redaction_test_secret_value_xyz"; + let assert = Command::new(assert_cmd::cargo::cargo_bin!("kingfisher")) + .args([ + "validate", + "--rule", + "kingfisher.github.2", + secret, + "--format", + "json", + "--allow-internal-ips", + "--endpoint", + "github=http://127.0.0.1:1", + "--timeout", + "2", + "--retries", + "0", + "--no-update-check", + ]) + .assert(); + let output = assert.get_output(); + assert!(output.status.code().is_some_and(|code| code == 0 || code == 1)); + let stdout = String::from_utf8(output.stdout.clone()).expect("stdout should be UTF-8"); + let decoded: Value = serde_json::from_str(&stdout).expect("json should decode"); + assert_eq!(decoded.get("rule_id").and_then(|v| v.as_str()), Some("kingfisher.github.2")); + assert_eq!(decoded.get("is_valid").and_then(|v| v.as_bool()), Some(false)); + let message = decoded.get("message").and_then(|v| v.as_str()).unwrap_or(""); + assert!( + message.contains("HTTP validation failed"), + "message should explain the failure, got: {message}" + ); + // The CLI must never echo the user's secret back to stdout, even + // when the upstream validation fails. We emit a generic error + // message and only log the underlying detail at debug level. + assert!(!stdout.contains(secret), "secret must not appear in stdout output, got: {stdout}"); + } + #[test] fn validate_with_timeout() { Command::new(assert_cmd::cargo::cargo_bin!("kingfisher")) From 87f6bd818f7f6d8dfb208cfa83d8eb1eaa54cdff Mon Sep 17 00:00:00 2001 From: Mick Grove Date: Thu, 30 Apr 2026 11:40:22 -0700 Subject: [PATCH 3/4] copilot fixes --- src/direct_validate.rs | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/src/direct_validate.rs b/src/direct_validate.rs index ba53070..c6914e5 100644 --- a/src/direct_validate.rs +++ b/src/direct_validate.rs @@ -602,7 +602,11 @@ pub async fn run_direct_validation( // tools (and integration tests) unable to distinguish "no rule" // from "validation attempted, infrastructure failed". Match the // pattern used by AWS / GCP / raw branches below: surface the - // error as a non-valid result with the error in `message`. + // failure as a non-valid result with a generic `message`. The + // underlying error is intentionally NOT included in stdout or in + // debug logs because the rendered URL / headers / body can + // contain `{{ TOKEN }}` substituted to the secret (and any + // `--var` / `--arg` values). let mut result = match validation { Validation::Http(http_validation) => { match execute_http_validation( @@ -617,20 +621,19 @@ pub async fn run_direct_validation( .await { Ok(r) => r, - Err(e) => { - // Don't surface the underlying error: it can embed - // the rendered URL with `{{ TOKEN }}` substituted - // (i.e. the secret) or `--var` / `--arg` values. - // Log the detail for debugging and emit a generic - // message to stdout. - debug!("HTTP validation failed: {e}"); + Err(_e) => { + // Intentionally drop the underlying error: it can + // embed the rendered URL with `{{ TOKEN }}` + // substituted (i.e. the secret) or `--var` / + // `--arg` values. Logging it (even at debug) would + // leak credentials into stderr when -v is on. + debug!("HTTP validation failed"); DirectValidationResult { rule_id: String::new(), rule_name: String::new(), is_valid: false, status_code: None, - message: "HTTP validation failed (see debug logs for details)" - .to_string(), + message: "HTTP validation failed".to_string(), } } } @@ -646,15 +649,14 @@ pub async fn run_direct_validation( .await { Ok(r) => r, - Err(e) => { - debug!("gRPC validation failed: {e}"); + Err(_e) => { + debug!("gRPC validation failed"); DirectValidationResult { rule_id: String::new(), rule_name: String::new(), is_valid: false, status_code: None, - message: "gRPC validation failed (see debug logs for details)" - .to_string(), + message: "gRPC validation failed".to_string(), } } } From 632bb0113d16b59a7649675067afd2c6c60cab5f Mon Sep 17 00:00:00 2001 From: Mick Grove Date: Thu, 30 Apr 2026 12:07:15 -0700 Subject: [PATCH 4/4] copilot fixes --- tests/cli_validate_revoke.rs | 69 ++++++++++++++++++++++++++++++++++++ 1 file changed, 69 insertions(+) diff --git a/tests/cli_validate_revoke.rs b/tests/cli_validate_revoke.rs index f9de895..49fa107 100644 --- a/tests/cli_validate_revoke.rs +++ b/tests/cli_validate_revoke.rs @@ -223,6 +223,75 @@ mod validate { assert!(!stdout.contains(secret), "secret must not appear in stdout output, got: {stdout}"); } + /// gRPC infrastructure failures must surface as a structured + /// DirectValidationResult and must not leak the secret into stdout. + /// Mirrors `validate_http_failure_emits_structured_result` for the + /// gRPC code path. + #[test] + fn validate_grpc_failure_emits_structured_result() { + let tmp = TempDir::new().unwrap(); + // Custom rule with gRPC validation pointing at an unreachable port + // so we deterministically trigger a connection failure rather than + // depending on any built-in provider's reachability. + fs::write( + tmp.path().join("custom_grpc_rule.yml"), + r#" +rules: + - name: Custom gRPC Rule + id: test.custom.grpc + pattern: "grpc_[a-z0-9]{8}" + validation: + type: Grpc + content: + request: + url: http://127.0.0.1:1/example.Service/Method + headers: + content-type: application/grpc + x-token: "{{ TOKEN }}" + response_matcher: + - report_response: true + - type: HeaderMatch + header: grpc-status + expected: + - "0" +"#, + ) + .unwrap(); + + let secret = "grpc_redaction_test_secret_xyz"; + let assert = Command::new(assert_cmd::cargo::cargo_bin!("kingfisher")) + .args([ + "validate", + "--rule", + "test.custom.grpc", + secret, + "--rules-path", + tmp.path().to_str().unwrap(), + "--no-builtins", + "--format", + "json", + "--allow-internal-ips", + "--timeout", + "2", + "--retries", + "0", + "--no-update-check", + ]) + .assert(); + let output = assert.get_output(); + assert!(output.status.code().is_some_and(|code| code == 0 || code == 1)); + let stdout = String::from_utf8(output.stdout.clone()).expect("stdout should be UTF-8"); + let decoded: Value = serde_json::from_str(&stdout).expect("json should decode"); + assert_eq!(decoded.get("rule_id").and_then(|v| v.as_str()), Some("test.custom.grpc")); + assert_eq!(decoded.get("is_valid").and_then(|v| v.as_bool()), Some(false)); + let message = decoded.get("message").and_then(|v| v.as_str()).unwrap_or(""); + assert!( + message.contains("gRPC validation failed"), + "message should explain the failure, got: {message}" + ); + assert!(!stdout.contains(secret), "secret must not appear in stdout output, got: {stdout}"); + } + #[test] fn validate_with_timeout() { Command::new(assert_cmd::cargo::cargo_bin!("kingfisher"))