fix(#2911): name the forgejo admin verb in errors, not its arguments
forge_admin interpolated its whole argument vector into the error context, and two callers pass a live operator password in that vector (user create --password, user change-password). Any failure of those commands wrote the password to hive-c0re's log in cleartext -- and the likeliest trigger is forgejo rejecting a weak password, so the secret got logged precisely because forgejo refused it. Redacting the value after --password would repeat the bug the issue is about: redact_password_line matched one keyword and a differently named secret walked past it. A denylist fails open, silently, and the next secret-bearing flag would leak until someone extended the list. describe_forge_admin keeps the leading verb path and stops at the first flag, so "user create --username iris --password ..." is reported as "forgejo admin user create". The verbs are a closed set this crate chooses itself; argument values never are, so a new flag is excluded by construction. Nothing useful is lost -- the context says which operation failed, and the underlying error already carries forgejo's own message about why. The same pattern in hive-priv is deliberately untouched: that crate runs as root and the redactor's shape is still an open question on the issue. This change holds under either answer.
This commit is contained in:
parent
a9e05d3bb1
commit
10285ff764
1 changed files with 67 additions and 1 deletions
|
|
@ -147,6 +147,31 @@ pub async fn is_present() -> bool {
|
||||||
stdout.lines().any(|l| l.trim() == FORGE_CONTAINER)
|
stdout.lines().any(|l| l.trim() == FORGE_CONTAINER)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Name a `forgejo admin` invocation for an error message without
|
||||||
|
/// reproducing its arguments: keep the leading verb path, stop at the
|
||||||
|
/// first flag. `["user", "create", "--username", "iris", "--password",
|
||||||
|
/// "…"]` becomes `forgejo admin user create`.
|
||||||
|
///
|
||||||
|
/// The verbs are an **allowlist**, and that is the whole point. Some
|
||||||
|
/// callers pass a live secret as an argument value (`--password`), so a
|
||||||
|
/// message built from the raw vector puts it in the log. Listing the
|
||||||
|
/// flags to *hide* instead would repeat the bug this guards against: a
|
||||||
|
/// newly added secret-bearing flag would leak until someone remembered
|
||||||
|
/// to extend the list. Verbs are a closed set this crate chooses
|
||||||
|
/// itself; argument values never are.
|
||||||
|
fn describe_forge_admin(args: &[&str]) -> String {
|
||||||
|
let verbs: Vec<&str> = args
|
||||||
|
.iter()
|
||||||
|
.take_while(|a| !a.starts_with('-'))
|
||||||
|
.copied()
|
||||||
|
.collect();
|
||||||
|
if verbs.is_empty() {
|
||||||
|
"forgejo admin".to_owned()
|
||||||
|
} else {
|
||||||
|
format!("forgejo admin {}", verbs.join(" "))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// Run `forgejo admin <args>` inside the hive-forge container as the
|
/// Run `forgejo admin <args>` inside the hive-forge container as the
|
||||||
/// forgejo user (the only uid with write access to the state dir).
|
/// forgejo user (the only uid with write access to the state dir).
|
||||||
/// Returns stdout on success; bails with stderr context on failure.
|
/// Returns stdout on success; bails with stderr context on failure.
|
||||||
|
|
@ -158,7 +183,7 @@ async fn forge_admin(args: &[&str]) -> Result<String> {
|
||||||
// nsenter: stat of /proc/<pid>/ns/user failed: Permission denied
|
// nsenter: stat of /proc/<pid>/ns/user failed: Permission denied
|
||||||
let (stdout, _stderr) = crate::priv_client::run_forge_admin(args)
|
let (stdout, _stderr) = crate::priv_client::run_forge_admin(args)
|
||||||
.await
|
.await
|
||||||
.with_context(|| format!("forgejo admin {} (via hive-priv)", args.join(" ")))?;
|
.with_context(|| format!("{} (via hive-priv)", describe_forge_admin(args)))?;
|
||||||
Ok(stdout)
|
Ok(stdout)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -561,3 +586,44 @@ pub async fn ensure_config_pr_webhook(
|
||||||
tracing::info!(%target_url, "forge: config-pr webhook created on org {CONFIG_ORG}");
|
tracing::info!(%target_url, "forge: config-pr webhook created on org {CONFIG_ORG}");
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(test)]
|
||||||
|
mod tests {
|
||||||
|
use super::describe_forge_admin;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn describe_keeps_the_verb_path_and_drops_every_value() {
|
||||||
|
assert_eq!(
|
||||||
|
describe_forge_admin(&[
|
||||||
|
"user",
|
||||||
|
"create",
|
||||||
|
"--username",
|
||||||
|
"iris",
|
||||||
|
"--email",
|
||||||
|
"iris@hyperhive.local",
|
||||||
|
"--random-password",
|
||||||
|
]),
|
||||||
|
"forgejo admin user create"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn describe_does_not_reproduce_a_password_argument() {
|
||||||
|
let described = describe_forge_admin(&[
|
||||||
|
"user",
|
||||||
|
"change-password",
|
||||||
|
"--username",
|
||||||
|
"iris",
|
||||||
|
"--password",
|
||||||
|
"correct-horse-battery-staple",
|
||||||
|
]);
|
||||||
|
assert_eq!(described, "forgejo admin user change-password");
|
||||||
|
assert!(!described.contains("correct-horse-battery-staple"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn describe_survives_a_leading_flag_and_an_empty_vector() {
|
||||||
|
assert_eq!(describe_forge_admin(&["--help"]), "forgejo admin");
|
||||||
|
assert_eq!(describe_forge_admin(&[]), "forgejo admin");
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue