diff --git a/src/host.rs b/src/host.rs index cc59c24..8fe0c3f 100644 --- a/src/host.rs +++ b/src/host.rs @@ -137,7 +137,18 @@ impl CommandHost { return Err(Error::Uncertain("HOST_RESPONSE_TOO_LARGE".into())); } let status = child.wait().await?; - let value: Value = serde_json::from_slice(&bytes)?; + // A failed host may also print malformed output. Parse leniently + // so its exit identity is not replaced by a JSON parse error. + let parsed = serde_json::from_slice::(&bytes); + if !status.success() && parsed.is_err() { + let code = status + .code() + .map_or_else(|| "signal".to_string(), |code| code.to_string()); + return Err(invalid(format!( + "HOST_COMMAND_FAILED: {action} (exit {code}): host response was not valid JSON" + ))); + } + let value = parsed?; if value.get("protocolVersion") == Some(&json!(1)) && value.get("ok") == Some(&json!(false)) && value.get("uncertain") == Some(&json!(true)) @@ -147,7 +158,22 @@ impl CommandHost { ))); } if !status.success() { - return Err(invalid(format!("HOST_COMMAND_FAILED: {action}"))); + // Keep the host's own reason and exit code (issue slock#8609): + // without them a rollback only says "HOST_COMMAND_FAILED". The + // host writes this text for operators; it is bounded and + // stripped of control characters before it reaches receipts. + let code = status + .code() + .map_or_else(|| "signal".to_string(), |code| code.to_string()); + let detail = value + .get("error") + .and_then(Value::as_str) + .map(host_error_detail) + .filter(|detail| !detail.is_empty()) + .map_or_else(String::new, |detail| format!(": {detail}")); + return Err(invalid(format!( + "HOST_COMMAND_FAILED: {action} (exit {code}){detail}" + ))); } if value.get("protocolVersion") != Some(&json!(1)) || value.get("ok") != Some(&json!(true)) @@ -242,3 +268,150 @@ impl Host for CommandHost { Ok(e) } } + +const HOST_ERROR_DETAIL_MAX_CHARS: usize = 240; + +/// A host-supplied failure reason, reduced to one bounded printable line. +const CREDENTIAL_MARKERS: &[&str] = &[ + "authorization", + "bearer", + "token", + "secret", + "password", + "passwd", + "api_key", + "api key", + "api-key", + "apikey", + "access_key", + "access key", + "access-key", + "client_secret", + "client secret", + "client-secret", + "cookie", + "database_url", + "database-url", +]; + +fn has_credential_marker(line: &str) -> bool { + let line = line.to_ascii_lowercase(); + CREDENTIAL_MARKERS + .iter() + .any(|marker| line.contains(marker)) +} + +/// `scheme://user@host` or `scheme://user:pass@host`: a token used as the +/// username is as sensitive as a password. +fn has_url_userinfo(line: &str) -> bool { + let mut rest = line; + while let Some(index) = rest.find("://") { + let after = &rest[index + 3..]; + let authority = after + .split(|c: char| c.is_whitespace() || matches!(c, '/' | '?' | '#')) + .next() + .unwrap_or_default(); + if authority.rfind('@').is_some_and(|at| at > 0) { + return true; + } + rest = after; + } + false +} + +fn redact_opaque_word(word: &str) -> &str { + let opaque = word.len() >= 24 + && word + .chars() + .all(|c| c.is_ascii_alphanumeric() || "._-+".contains(c)); + if opaque { "" } else { word } +} + +/// The host's reason reaches the terminal and the receipt, so redact it before +/// it is shortened. Redaction works on whole lines of the raw text: a line that +/// names a credential or carries URL userinfo is replaced entirely (a key and +/// its value may be split by spaces, quotes or a line break), and a credential +/// line ending in `:` or `=` also takes the next non-empty line. +fn redact_host_error(raw: &str) -> String { + let mut out: Vec<&str> = Vec::new(); + let mut redact_next = false; + for line in raw.lines() { + let trimmed = line.trim(); + if trimmed.is_empty() { + continue; + } + let carried = std::mem::take(&mut redact_next); + let credential = has_credential_marker(line); + if credential && matches!(trimmed.chars().last(), Some(':' | '=')) { + redact_next = true; + } + if carried || credential || has_url_userinfo(line) { + out.push(""); + continue; + } + out.extend(line.split_whitespace().map(redact_opaque_word)); + } + out.join(" ") +} + +fn host_error_detail(raw: &str) -> String { + let redacted = redact_host_error(raw); + let mut out = String::new(); + let mut chars = 0; + for ch in redacted.chars() { + let ch = if ch.is_control() { ' ' } else { ch }; + if ch == ' ' && out.ends_with(' ') { + continue; + } + if chars == HOST_ERROR_DETAIL_MAX_CHARS { + out.push('…'); + break; + } + out.push(ch); + chars += 1; + } + out.trim().to_string() +} + +#[cfg(test)] +mod host_error_detail_tests { + use super::host_error_detail; + + #[test] + fn credential_lines_are_redacted_before_display() { + for (raw, leaked) in [ + ("login failed: password = hunter2", "hunter2"), + ("token : abc", "abc"), + ("Authorization: Basic c2hvcnQ=", "c2hvcnQ="), + ("Cookie: theme=dark; sid=abc123", "abc123"), + ( + "clone https://ghp_shortTok@github.com/o/r.git failed", + "ghp_shortTok", + ), + ("connect postgres://u:pw123@h/db refused", "pw123"), + ( + "config:\n password:\n\n short-secret-value\nnext", + "short-secret-value", + ), + ] { + let detail = host_error_detail(raw); + assert!( + !detail.contains(leaked), + "{raw:?} leaked {leaked:?} as {detail:?}" + ); + assert!(detail.contains(""), "{raw:?} -> {detail:?}"); + } + } + + #[test] + fn harmless_reason_is_kept() { + assert_eq!( + host_error_detail("launchctl bootstrap failed: Input/output error"), + "launchctl bootstrap failed: Input/output error", + ); + assert_eq!( + host_error_detail("config:\n password:\n x\nservice exited"), + "config: service exited" + ); + } +} diff --git a/tests/process.rs b/tests/process.rs index e10d0aa..3417d36 100644 --- a/tests/process.rs +++ b/tests/process.rs @@ -237,3 +237,78 @@ async fn rejected_upgrade_is_held_without_recovering_another_writers_transaction assert!(recovery.recovery_file.is_some()); Ok(()) } + +#[tokio::test] +async fn native_controller_failure_keeps_the_hosts_reason_and_exit_code() -> Result<()> { + let root = tempdir()?; + let mut host = CommandHost::new( + vec![fixture().into(), "--controller".into()], + root.path().join("state"), + 3000, + )?; + host.cwd = Some(root.path().into()); + fs::write(root.path().join("effect-failed"), b"fixture")?; + let error = host.probe().await.unwrap_err(); + assert!(!error.is_uncertain(), "a reported failure is definite"); + let message = error.to_string(); + assert!( + message.contains("HOST_COMMAND_FAILED: probe (exit 3): candidate was not started"), + "{message}" + ); + assert!( + !message.chars().any(char::is_control), + "control characters are stripped: {message:?}" + ); + assert!( + message.chars().count() < 400, + "the host's reason is bounded: {}", + message.chars().count() + ); + Ok(()) +} + +#[tokio::test] +async fn native_controller_failure_redacts_credentials_in_the_hosts_reason() -> Result<()> { + let root = tempdir()?; + let mut host = CommandHost::new( + vec![fixture().into(), "--controller".into()], + root.path().join("state"), + 3000, + )?; + host.cwd = Some(root.path().into()); + fs::write(root.path().join("effect-failed-credential"), b"fixture")?; + let message = host.probe().await.unwrap_err().to_string(); + assert!( + message.contains("HOST_COMMAND_FAILED: probe (exit 3): login failed"), + "{message}" + ); + assert!( + !message.contains("pw123"), + "the credential leaked: {message}" + ); + assert!(message.contains(""), "{message}"); + Ok(()) +} + +#[tokio::test] +async fn native_controller_failure_with_malformed_output_keeps_the_exit_code() -> Result<()> { + let root = tempdir()?; + let mut host = CommandHost::new( + vec![fixture().into(), "--controller".into()], + root.path().join("state"), + 3000, + )?; + host.cwd = Some(root.path().into()); + fs::write(root.path().join("effect-failed-malformed"), b"fixture")?; + let error = host.probe().await.unwrap_err(); + assert!( + !error.is_uncertain(), + "a non-zero exit is a definite failure" + ); + let message = error.to_string(); + assert!( + message.contains("HOST_COMMAND_FAILED: probe (exit 4): host response was not valid JSON"), + "{message}" + ); + Ok(()) +} diff --git a/tests/support/fixture.rs b/tests/support/fixture.rs index ff2f686..01d5ff9 100644 --- a/tests/support/fixture.rs +++ b/tests/support/fixture.rs @@ -69,6 +69,28 @@ fn run() -> Result<(), Box> { std::io::stdout().write_all(&vec![b'x'; 70000])?; return Ok(()); } + if root.join("effect-failed").exists() { + // A definite failure with the host's own message, a control + // character and a long tail (bounded by the carrier). + let message = format!("candidate was not started\u{1b}[31m: {}", "y".repeat(600)); + println!( + "{}", + serde_json::json!({"protocolVersion":1,"ok":false,"uncertain":false,"error":message}) + ); + std::process::exit(3); + } + if root.join("effect-failed-credential").exists() { + let message = "login failed\nAuthorization: Bearer pw123"; + println!( + "{}", + serde_json::json!({"protocolVersion":1,"ok":false,"uncertain":false,"error":message}) + ); + std::process::exit(3); + } + if root.join("effect-failed-malformed").exists() { + println!("host crashed before writing a response {{"); + std::process::exit(4); + } if root.join("effect-uncertain").exists() { println!( "{}",