fix(priv): scan for a secret-shaped RUN, not a whitespace-delimited word
argus caught a false negative in the shape rule, and it sits exactly in the property the change is sold on -- "a new secret type is caught by default". `split_whitespace()` yields `"<token>,"` for a token with punctuation glued to it, and the comma fails the alphabet check for the whole word, so the line passes unredacted with the credential in it. `[<token>]`, `"<token>"`, `(<token>)` and a no-whitespace-at-all blob all defeat it the same way. Whitespace is not what delimits a secret; the alphabet is. So scan the line for a maximal run of >=32 alphabet characters and let punctuation reset the counter. Simpler than the version it replaces, and it closes the gap by construction rather than by enumerating the delimiters someone might glue on next. The existing tests all passed against the broken version because I wrote them from the same mental model that produced the bug -- every fixture had a space before the token. The new test carries the six shapes that used to slip through.
This commit is contained in:
parent
9eaf66546b
commit
462f353c10
1 changed files with 61 additions and 18 deletions
|
|
@ -1724,21 +1724,37 @@ fn redact_secret_line(line: &str) -> std::borrow::Cow<'_, str> {
|
|||
if line.to_ascii_lowercase().contains("password") {
|
||||
return std::borrow::Cow::Borrowed("[redacted: line mentions a password]");
|
||||
}
|
||||
if line.split_whitespace().any(looks_like_secret) {
|
||||
if contains_secret_shaped_run(line) {
|
||||
return std::borrow::Cow::Borrowed("[redacted: line contains a secret-shaped token]");
|
||||
}
|
||||
std::borrow::Cow::Borrowed(line)
|
||||
}
|
||||
|
||||
/// True when a whitespace-delimited word looks like an opaque credential:
|
||||
/// at least 32 characters drawn only from the hex / base64url alphabet.
|
||||
/// 32 is below forgejo's 40-hex access token and above the ordinary
|
||||
/// English words and file paths that appear in `forgejo admin` output.
|
||||
fn looks_like_secret(word: &str) -> bool {
|
||||
word.len() >= 32
|
||||
&& word
|
||||
.bytes()
|
||||
.all(|b| b.is_ascii_alphanumeric() || matches!(b, b'+' | b'/' | b'=' | b'_' | b'-'))
|
||||
/// True when the line contains an unbroken run of at least 32 characters
|
||||
/// from the hex / base64url alphabet. 32 sits below forgejo's 40-hex
|
||||
/// access token and above the ordinary words and path segments that
|
||||
/// appear in `forgejo admin` output.
|
||||
///
|
||||
/// ⚠️ Scans for a RUN, not for a whitespace-delimited word. An earlier
|
||||
/// version split on whitespace and required the whole word to match,
|
||||
/// which a secret with punctuation glued to it defeats: `"<token>,"` and
|
||||
/// `"[<token>]"` both fail an all-chars check on the word while still
|
||||
/// containing the credential in full. Whitespace is not what delimits a
|
||||
/// secret — the alphabet is (thanks @argus for catching it).
|
||||
fn contains_secret_shaped_run(line: &str) -> bool {
|
||||
const MIN: usize = 32;
|
||||
let mut run = 0usize;
|
||||
for b in line.bytes() {
|
||||
if b.is_ascii_alphanumeric() || matches!(b, b'+' | b'/' | b'=' | b'_' | b'-') {
|
||||
run += 1;
|
||||
if run >= MIN {
|
||||
return true;
|
||||
}
|
||||
} else {
|
||||
run = 0;
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// Run `forgejo admin <args>` inside the `hive-forge` container as the
|
||||
|
|
@ -2522,8 +2538,8 @@ async fn sync_agent_tmpfiles(agents: &[AgentTmpfilesEntry]) -> Result<(String, S
|
|||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::{
|
||||
OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, limits_dropin_body,
|
||||
looks_like_secret, redact_secret_line, remove_marker_in, write_state_file_nofollow,
|
||||
OwnedFd, PAUSED_MARKER_FILE, PrivRequest, check_fd_agreement, contains_secret_shaped_run,
|
||||
limits_dropin_body, redact_secret_line, remove_marker_in, write_state_file_nofollow,
|
||||
};
|
||||
use std::path::PathBuf;
|
||||
use std::sync::atomic::{AtomicU32, Ordering};
|
||||
|
|
@ -2660,23 +2676,50 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn secret_shape_boundaries() {
|
||||
// Ordinary forgejo-admin output survives: no word is long enough.
|
||||
// Ordinary forgejo-admin output survives: no run is long enough.
|
||||
assert_eq!(
|
||||
redact_secret_line("Command 'user' 'create' finished with no errors."),
|
||||
"Command 'user' 'create' finished with no errors."
|
||||
);
|
||||
// 31 chars is below the floor, 32 is at it.
|
||||
assert!(!looks_like_secret(&"a".repeat(31)));
|
||||
assert!(looks_like_secret(&"a".repeat(32)));
|
||||
assert!(!contains_secret_shaped_run(&"a".repeat(31)));
|
||||
assert!(contains_secret_shaped_run(&"a".repeat(32)));
|
||||
// Punctuation breaks the run — a sentence never trips it however long.
|
||||
assert!(!looks_like_secret(
|
||||
assert!(!contains_secret_shaped_run(
|
||||
"this.is.a.very.long.dotted.identifier.but.not.a.secret"
|
||||
));
|
||||
// base64url and hex alphabets both count.
|
||||
assert!(looks_like_secret(
|
||||
assert!(contains_secret_shaped_run(
|
||||
"ZGVhZGJlZWZkZWFkYmVlZmRlYWRiZWVmZGVhZA=="
|
||||
));
|
||||
assert!(looks_like_secret("aG93-dy_there-aG93dy1theresomething"));
|
||||
assert!(contains_secret_shaped_run(
|
||||
"aG93-dy_there-aG93dy1theresomething"
|
||||
));
|
||||
}
|
||||
|
||||
/// argus on the review: a whitespace-delimited check is defeated by
|
||||
/// punctuation glued to the secret — the punctuation joins the "word"
|
||||
/// and fails the alphabet test for the whole run, while the credential
|
||||
/// sits there in full. Scanning for a RUN rather than a WORD closes it.
|
||||
/// These are the shapes that used to slip through.
|
||||
#[test]
|
||||
fn secret_is_caught_with_punctuation_glued_to_it() {
|
||||
const TOK: &str = "0123456789abcdef0123456789abcdef01234567";
|
||||
for line in [
|
||||
format!("token: {TOK},"),
|
||||
format!("token: {TOK}."),
|
||||
format!("using [{TOK}] now"),
|
||||
format!("value=\"{TOK}\""),
|
||||
format!("(created {TOK})"),
|
||||
// no whitespace anywhere -- one glued blob
|
||||
format!("Bearer:{TOK};next"),
|
||||
] {
|
||||
assert_eq!(
|
||||
redact_secret_line(&line),
|
||||
"[redacted: line contains a secret-shaped token]",
|
||||
"leaked through: {line}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Unique scratch dir per test, no external tempfile dep.
|
||||
|
|
|
|||
Loading…
Reference in a new issue