From fdb847cd87dfb521272a4843cb9df4349980822e Mon Sep 17 00:00:00 2001 From: atlas Date: Thu, 24 Sep 2026 11:35:53 +0200 Subject: [PATCH] hive-priv: stop run_forge_admin's bail! from reproducing --password The comment above the bail! named the gap itself: stderr went through redact_secret_line, but args.join(" ") did not. hive-c0re passes a live --password value as an argument on user create and change-password, so any non-zero forgejo admin exit put the plaintext password in the error string, and from there into hive-c0re's warn! log (journal + VictoriaLogs) and hivectl's returned error. Add describe_forge_admin, a pure function that names the invocation by its leading verb path and stops at the first flag, same approach as hive-c0re's own describe_forge_admin (forge/mod.rs, from #2936) and for the same reason: the verbs are a closed set this crate chooses, argument values never are, so an allowlist over shape excludes any future secret-bearing flag by construction instead of by someone remembering to redact its value. Unit tests cover: a --password value dropped, the --password=value form also dropped (it starts with '-', so the take_while excludes the whole argument), and a control that the verb path still appears. Closes #4670. --- hive-priv/src/main.rs | 93 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 83 insertions(+), 10 deletions(-) diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index fbe84b13..5a1be8d5 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -2351,6 +2351,32 @@ fn contains_secret_shaped_run(line: &str) -> bool { false } +/// Name a `forgejo admin` invocation for an error message without +/// reproducing its arguments: keep the leading verb path, stop at the +/// first flag. `["user", "change-password", "--username", "iris", +/// "--password", "…"]` becomes `forgejo admin user change-password`. +/// +/// Same allowlist as hive-c0re's `describe_forge_admin` +/// (`hive-c0re/src/forge/mod.rs`), for the same reason: the verbs are a +/// closed set this crate chooses itself, but hive-c0re passes a live +/// `--password` value as an argument on some calls, and argument values +/// are never safe to assume closed. Redacting the value after +/// `--password` instead would repeat the bug `redact_secret_line`'s own +/// doc comment describes — a denylist of one flag name fails open the +/// moment a differently named secret-bearing flag is added. +fn describe_forge_admin(args: &[String]) -> String { + let verbs: Vec<&str> = args + .iter() + .take_while(|a| !a.starts_with('-')) + .map(String::as_str) + .collect(); + if verbs.is_empty() { + "forgejo admin".to_owned() + } else { + format!("forgejo admin {}", verbs.join(" ")) + } +} + /// Run `forgejo admin ` inside the `hive-forge` container as the /// `forgejo` unix user. Requires root (for nsenter into the container's /// namespaces). Returns `(stdout, stderr)`. @@ -2392,17 +2418,20 @@ async fn run_forge_admin(args: &[String]) -> Result<(String, String)> { if !out.status.success() { // Redact here too. The error string is propagated to the caller and // ends up logged; a partial-failure stderr can carry the same - // material stdout would have. Redacting the log but not the error - // is the same "two of three sites" gap that makes these leaks - // survive a fix. + // material stdout would have. And name the invocation via + // `describe_forge_admin` rather than `args.join(" ")`: some callers + // pass a live `--password` value as an argument, so reproducing the + // raw vector here is the same "two of three sites" gap that makes + // these leaks survive a fix — the log was redacted, but the error + // was not. let safe_stderr: String = stderr .lines() .map(|l| redact_secret_line(l).into_owned()) .collect::>() .join("; "); bail!( - "forgejo admin {} failed ({}): {}", - args.join(" "), + "{} failed ({}): {}", + describe_forge_admin(args), out.status, safe_stderr.trim() ); @@ -3243,11 +3272,11 @@ mod tests { use super::{ AgentTmpfilesEntry, BindMount, OwnedFd, PAUSED_MARKER_FILE, PrivRequest, agent_tmpfiles_content, check_fd_agreement, clear_runner_credentials, - contains_secret_shaped_run, ensure_plain_filename, git_overlay_flags, limits_dropin_body, - matrix_token_filename, open_export_dest, partial_name, redact_secret_line, - remove_marker_in, single_output_path, toplevel_attr, validate_account_name, - validate_credential_name, validate_snapshot_name, write_agent_dir_file, - write_bridge_dns_marker_in, write_state_file_nofollow, + contains_secret_shaped_run, describe_forge_admin, ensure_plain_filename, git_overlay_flags, + limits_dropin_body, matrix_token_filename, open_export_dest, partial_name, + redact_secret_line, remove_marker_in, single_output_path, toplevel_attr, + validate_account_name, validate_credential_name, validate_snapshot_name, + write_agent_dir_file, write_bridge_dns_marker_in, write_state_file_nofollow, }; use std::path::PathBuf; use std::sync::atomic::{AtomicU32, Ordering}; @@ -3527,6 +3556,50 @@ mod tests { ); } + /// `run_forge_admin`'s `bail!` names the invocation through this + /// function instead of `args.join(" ")` — the regression this test + /// guards. A `--password` value never survives to the error string. + #[test] + fn describe_forge_admin_does_not_reproduce_a_password_argument() { + let described = describe_forge_admin(&[ + "user".to_owned(), + "create".to_owned(), + "--username".to_owned(), + "x".to_owned(), + "--password".to_owned(), + "S3cret".to_owned(), + ]); + assert_eq!(described, "forgejo admin user create"); + assert!(!described.contains("S3cret"), "{described}"); + } + + /// The `--password=value` single-argument form is also excluded: it + /// starts with `-`, so it never enters the kept verb path. + #[test] + fn describe_forge_admin_redacts_the_equals_sign_form_too() { + let described = describe_forge_admin(&[ + "user".to_owned(), + "create".to_owned(), + "--password=S3cret".to_owned(), + ]); + assert!(!described.contains("S3cret"), "{described}"); + } + + /// Control: a non-secret argument (the verb path) still appears, so + /// the message stays useful for diagnosing which call failed. + #[test] + fn describe_forge_admin_keeps_the_verb_path() { + assert_eq!( + describe_forge_admin(&["user".to_owned(), "change-password".to_owned()]), + "forgejo admin user change-password" + ); + assert_eq!( + describe_forge_admin(&["--help".to_owned()]), + "forgejo admin" + ); + assert_eq!(describe_forge_admin(&[]), "forgejo admin"); + } + /// The regression this function exists for. The keyword rule passes /// this line straight through — it says nothing about a password — so /// only the shape rule catches it.