From 0e670e9234d37465ee37f561e302efd91da7ec59 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Tue, 20 Oct 2026 13:55:39 -0400 Subject: [PATCH] Exempt ULIDs from entropy-based secret redaction Fabro's run, session, turn, message and pair IDs are ULIDs. A ULID occasionally has enough distinct characters to cross the secret scanner's 4.5-bit entropy threshold, and the stream redaction then rewrites the Web UI link to `/runs/REDACTED` and the run branch to `fabro/run/REDACTED`. Two nightly releases in the last week failed on `attach_replays_completed_detached_run` this way, and each fix so far protected one field at a time. Teach the entropy scanner the format instead: a token that is exactly 26 uppercase Crockford base32 characters and decodes to a timestamp between 2020 and 2100 is an identifier, not a secret, and is left alone wherever it appears. Trailing `-ULID`, `_ULID` and `=ULID` segments are stripped before measuring, so `20261020-` and `-` are handled, and the secret in the latter is still found. The gitleaks credential patterns run on every token as before. This mirrors detect-secrets' built-in UUID filter. A secret would have to take exactly this shape and land in the date window to be missed, which is about one in a hundred of the already rare tokens encoded as 26 uppercase base32 characters. Co-Authored-By: Claude Fable 5.1 --- Cargo.lock | 1 + .../src/commands/run/petri_stream.rs | 24 ++- lib/foundation/fabro-redact/Cargo.toml | 2 + lib/foundation/fabro-redact/src/entropy.rs | 168 ++++++++++++++++++ lib/foundation/fabro-redact/src/jsonl.rs | 41 ++++- lib/foundation/fabro-redact/src/lib.rs | 8 +- 6 files changed, 236 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 78840a28c..27a2dab40 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2595,6 +2595,7 @@ dependencies = [ "toml 0.8.23", "tracing", "tracing-subscriber", + "ulid", "url", ] diff --git a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs index 28c374b78..2748f5779 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs +++ b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs @@ -1329,7 +1329,8 @@ mod tests { #[test] fn redacted_raw_lines_keep_the_typed_run_id() { let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; - let mut event = petri(1, json!({"record": {"body": {"value": run_id}}})); + let secret = "xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"; + let mut event = petri(1, json!({"record": {"body": {"value": secret}}})); event.run_id = run_id.parse().expect("a valid run ID"); let line = raw_line(&event).expect("a redacted line"); @@ -1338,4 +1339,25 @@ mod tests { assert_eq!(decoded.stream_seq, event.stream_seq); assert_eq!(decoded.item["record"]["body"]["value"], "REDACTED"); } + + #[test] + fn raw_lines_preserve_run_ids_that_cross_the_entropy_threshold() { + // This ID has enough distinct characters to read as a secret to the + // entropy scanner; the Web UI link and run branch must survive. + let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; + for (kind, field, text) in [ + ( + "run.created", + "web_url", + format!("http://localhost:3000/runs/{run_id}"), + ), + ("run.branch", "run_branch", format!("fabro/run/{run_id}")), + ] { + let mut event = platform(1, &json!({"kind": kind, field: text, "content": run_id})); + event.run_id = run_id.parse().expect("a valid run ID"); + let line = raw_line(&event).expect("a redacted line"); + let decoded: RunStreamItem = serde_json::from_str(&line).expect("a valid stream item"); + assert_eq!(decoded, event); + } + } } diff --git a/lib/foundation/fabro-redact/Cargo.toml b/lib/foundation/fabro-redact/Cargo.toml index 168c0d74f..a1b6deca1 100644 --- a/lib/foundation/fabro-redact/Cargo.toml +++ b/lib/foundation/fabro-redact/Cargo.toml @@ -22,6 +22,8 @@ ref-cast.workspace = true regex.workspace = true serde_json.workspace = true thiserror.workspace = true +# Recognizes ULID-shaped tokens so Fabro's own IDs are never scanned as secrets. +ulid.workspace = true url.workspace = true [build-dependencies] diff --git a/lib/foundation/fabro-redact/src/entropy.rs b/lib/foundation/fabro-redact/src/entropy.rs index ec6029c3c..fa906f525 100644 --- a/lib/foundation/fabro-redact/src/entropy.rs +++ b/lib/foundation/fabro-redact/src/entropy.rs @@ -1,6 +1,7 @@ use std::sync::LazyLock; use regex::Regex; +use ulid::Ulid; use super::Region; @@ -11,6 +12,24 @@ static SECRET_PATTERN: LazyLock = const ENTROPY_THRESHOLD: f64 = 4.5; +/// The length of a ULID's canonical text form. +const ULID_LEN: usize = 26; + +/// The timestamp window a token must fall in to be recognized as a ULID: +/// 2020-01-01T00:00:00Z through 2100-01-01T00:00:00Z. +/// +/// Fabro's run, session, turn, message and pair IDs are ULIDs, and a +/// ULID occasionally has enough distinct characters to cross the entropy +/// threshold. Exempting the format keeps those IDs intact everywhere they +/// appear (URLs, branch names, commit messages, free text) instead of +/// protecting one field at a time. The window bounds what the exemption +/// can miss: a secret would have to be exactly 26 uppercase Crockford +/// base32 characters whose leading 48 bits decode to a date in this +/// range, which is roughly one in a hundred of the already rare tokens +/// that take that shape. The gitleaks patterns still run on every token. +const ULID_MIN_TIMESTAMP_MS: u64 = 1_577_836_800_000; +const ULID_MAX_TIMESTAMP_MS: u64 = 4_102_444_800_000; + /// Compute Shannon entropy (bits per byte) of a string. pub(super) fn shannon_entropy(s: &str) -> f64 { if s.is_empty() { @@ -68,6 +87,13 @@ pub(super) fn find_entropy_regions(s: &str) -> Vec { start += offset; } + // Identifiers are not secrets: a token that is a ULID, or ends in + // `-ULID`, `_ULID` or `=ULID` segments, is measured without them. + let end = start + ulid_free_len(&s[start..end]); + if end - start < 10 { + continue; + } + if shannon_entropy(&s[start..end]) > ENTROPY_THRESHOLD { regions.push(Region { start, end }); } @@ -97,10 +123,152 @@ fn assignment_value_offset(token: &str) -> Option { Some(eq + 1) } +/// Whether `token` is a ULID in canonical form (26 uppercase Crockford +/// base32 characters) with a timestamp inside the recognized window. +/// +/// The alphabet is checked by hand because the `ulid` crate's decoder is +/// lenient about case; the exemption is meant to be strict. +fn is_ulid(token: &str) -> bool { + token.len() == ULID_LEN + && token.bytes().all(|b| { + matches!( + b, + b'0'..=b'9' | b'A'..=b'H' | b'J' | b'K' | b'M' | b'N' | b'P'..=b'T' | b'V'..=b'Z' + ) + }) + && Ulid::from_string(token).is_ok_and(|ulid| { + (ULID_MIN_TIMESTAMP_MS..ULID_MAX_TIMESTAMP_MS).contains(&ulid.timestamp_ms()) + }) +} + +/// The length of `token` once every trailing ULID segment is removed. +/// +/// A token that is itself a ULID shrinks to nothing. A trailing ULID +/// preceded by `-`, `_` or `=` is stripped together with its separator, +/// repeatedly, so `20261020-` leaves the date and +/// `-` leaves the secret to be measured on its own. Twenty-six +/// trailing characters that are not set off by a separator stay part of the +/// token: they could be the tail of a longer secret. +fn ulid_free_len(token: &str) -> usize { + let mut len = token.len(); + loop { + let Some(id_start) = len.checked_sub(ULID_LEN) else { + return len; + }; + if !is_ulid(&token[id_start..len]) { + return len; + } + if id_start == 0 { + return 0; + } + if !matches!(token.as_bytes()[id_start - 1], b'-' | b'_' | b'=') { + return len; + } + len = id_start - 1; + } +} + #[cfg(test)] mod tests { use super::*; + /// Run IDs that cross the entropy threshold: each has at least 23 + /// distinct characters out of 26. + const HIGH_ENTROPY_ULIDS: [&str; 2] = + ["01M5228EHCT0P73Z4BYGDRWJNS", "01M5228EHCT0P73Z4BYGDRWJNV"]; + + const SECRET: &str = "xK9mZ2vL8nQ5rT1wY4bC7dF0gH3jE6p"; + + #[test] + fn high_entropy_ulids_cross_the_threshold_on_their_own() { + for id in HIGH_ENTROPY_ULIDS { + let e = shannon_entropy(id); + assert!(e > ENTROPY_THRESHOLD, "{id}: expected > 4.5, got {e}"); + } + } + + #[test] + fn ulids_are_not_secrets() { + for id in HIGH_ENTROPY_ULIDS { + for text in [ + id.to_string(), + format!("fabro/run/{id}"), + format!("http://localhost:3000/runs/{id}"), + format!("run_id={id}"), + format!("20261020-{id}"), + format!("fabro-run-{id}"), + format!("checkpoint_{id}_{id}"), + ] { + let regions = find_entropy_regions(&text); + assert!(regions.is_empty(), "{text}: got regions {regions:?}"); + } + } + } + + #[test] + fn secrets_beside_a_ulid_are_still_found() { + let id = HIGH_ENTROPY_ULIDS[0]; + let text = format!("{SECRET}-{id}"); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: 0, + end: SECRET.len(), + }]); + let text = format!("https://{SECRET}@example.com/runs/{id}"); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: "https://".len(), + end: "https://".len() + SECRET.len(), + }]); + } + + #[test] + fn a_ulid_glued_to_other_characters_is_scanned_as_a_whole() { + // No separator sets the ID off, so the token is measured intact. + let text = format!("x{}", HIGH_ENTROPY_ULIDS[0]); + assert!(shannon_entropy(&text) > ENTROPY_THRESHOLD); + assert_eq!(find_entropy_regions(&text), vec![Region { + start: 0, + end: text.len(), + }]); + } + + #[test] + fn ulid_recognition_is_strict() { + let id = HIGH_ENTROPY_ULIDS[0]; + assert!(is_ulid(id)); + // Lowercase is not canonical, even though the crate would decode it. + assert!(!is_ulid(&id.to_ascii_lowercase())); + assert!(!find_entropy_regions(&id.to_ascii_lowercase()).is_empty()); + // Wrong length or alphabet. + assert!(!is_ulid(&id[1..])); + assert!(!is_ulid(&format!("{id}0"))); + assert!(!is_ulid(&format!("I{}", &id[1..]))); + // A random secret that happens to be 26 base32 characters but whose + // timestamp falls outside the window is still a secret. + let ulid = Ulid::from_string(id).unwrap(); + let year_2010 = Ulid::from_parts(1_262_304_000_000, ulid.random()).to_string(); + let year_2150 = Ulid::from_parts(5_680_281_600_000, ulid.random()).to_string(); + assert!(!is_ulid(&year_2010), "{year_2010}"); + assert!(!is_ulid(&year_2150), "{year_2150}"); + // The window edges. + assert!(is_ulid( + &Ulid::from_parts(ULID_MIN_TIMESTAMP_MS, ulid.random()).to_string() + )); + assert!(!is_ulid( + &Ulid::from_parts(ULID_MAX_TIMESTAMP_MS, ulid.random()).to_string() + )); + } + + #[test] + fn ulid_free_len_strips_trailing_segments_only() { + let id = HIGH_ENTROPY_ULIDS[0]; + assert_eq!(ulid_free_len(id), 0); + assert_eq!(ulid_free_len(&format!("abc-{id}")), 3); + assert_eq!(ulid_free_len(&format!("abc_{id}-{id}")), 3); + assert_eq!(ulid_free_len(&format!("{id}-abc")), id.len() + 4); + assert_eq!(ulid_free_len(&format!("x{id}")), id.len() + 1); + assert_eq!(ulid_free_len("short"), 5); + } + #[test] fn entropy_empty_string() { assert!(shannon_entropy("").abs() < f64::EPSILON); diff --git a/lib/foundation/fabro-redact/src/jsonl.rs b/lib/foundation/fabro-redact/src/jsonl.rs index 84c48457c..f83faf62f 100644 --- a/lib/foundation/fabro-redact/src/jsonl.rs +++ b/lib/foundation/fabro-redact/src/jsonl.rs @@ -209,19 +209,50 @@ mod tests { #[test] fn redact_jsonl_line_preserves_ids_repeated_in_scanned_values() { - // This run ID from a failed CLI attach exceeds the entropy threshold. - // Redacting an event's context value must not also rewrite its envelope. + // Redacting an event's context value must not also rewrite its + // envelope, even when the envelope's ID field holds the same text. + let input = serde_json::json!({ + "run_id": HIGH_ENTROPY_SECRET, + "item": { "context": { "value": HIGH_ENTROPY_SECRET } }, + }); + let result = redact_jsonl_line(&input.to_string()); + let redacted: Value = serde_json::from_str(&result).unwrap(); + + assert_eq!(redacted["run_id"], HIGH_ENTROPY_SECRET); + assert_eq!(redacted["item"]["context"]["value"], "REDACTED"); + assert_eq!(redacted, redact_json_value(input)); + } + + #[test] + fn redact_jsonl_line_keeps_run_ids_wherever_they_appear() { + // This run ID from a failed CLI attach exceeds the entropy threshold + // on its own; the ULID exemption keeps it intact in scanned values, + // not only in the skipped `run_id` envelope field. let run_id = "01M5228EHCT0P73Z4BYGDRWJNS"; let input = serde_json::json!({ "run_id": run_id, - "item": { "context": { "value": run_id } }, + "item": { "record": { + "web_url": format!("http://localhost:3000/runs/{run_id}"), + "run_branch": format!("fabro/run/{run_id}"), + "message": format!("checkpoint for run {run_id}: {HIGH_ENTROPY_SECRET}"), + } }, }); let result = redact_jsonl_line(&input.to_string()); let redacted: Value = serde_json::from_str(&result).unwrap(); assert_eq!(redacted["run_id"], run_id); - assert_eq!(redacted["item"]["context"]["value"], "REDACTED"); - assert_eq!(redacted, redact_json_value(input)); + assert_eq!( + redacted["item"]["record"]["web_url"], + format!("http://localhost:3000/runs/{run_id}") + ); + assert_eq!( + redacted["item"]["record"]["run_branch"], + format!("fabro/run/{run_id}") + ); + assert_eq!( + redacted["item"]["record"]["message"], + format!("checkpoint for run {run_id}: REDACTED") + ); } #[test] diff --git a/lib/foundation/fabro-redact/src/lib.rs b/lib/foundation/fabro-redact/src/lib.rs index 84d8bca28..f07496f0e 100644 --- a/lib/foundation/fabro-redact/src/lib.rs +++ b/lib/foundation/fabro-redact/src/lib.rs @@ -1,8 +1,12 @@ //! Secret and credential redaction utilities. //! //! `redact_string`, `redact_json_value`, and `redact_jsonl_line` provide -//! generic secret scanning. [`DisplaySafeUrl`] provides deterministic URL -//! redaction for logging and error-message boundaries. +//! generic secret scanning: Shannon entropy over alphanumeric tokens plus +//! gitleaks credential patterns. Tokens that are ULIDs (Fabro's run, session, +//! turn, message and pair IDs) are exempt from the entropy layer, so a run ID +//! that happens to have many distinct characters is never turned into +//! `REDACTED`. [`DisplaySafeUrl`] provides deterministic URL redaction for +//! logging and error-message boundaries. mod entropy; mod gitleaks;