diff --git a/.gitleaks.toml b/.gitleaks.toml index e83f05bcc..93e714e5c 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -35,4 +35,19 @@ regexes = [ '''eyJhbGciOi\.\.\.''', '''ZEPH_A2A_IBCT_KEY''', '''VAULT_A2A_IBCT_KEY_1''', + # #6592 follow-up: fake PEM private-key bodies used across the new PEM-detector unit + # tests in zeph-common, zeph-sanitizer, zeph-subagent, zeph-core, and zeph-memory. Every + # value below is a truncated, non-functional fixture (verified via `gitleaks detect`), not + # a real key — matched on the fake body content itself, never the bare marker text alone, + # so a genuine future PEM key sharing the same header/footer line is still caught. + '''MIIBVQIBADANBgkqhkiG9w0B''', + '''AQEFAASCAT8wggE7AgEAAkEA''', + '''firstbody''', + '''body-material''', + '''\{key_body\}''', + '''\{cjk_body\}''', + '''\{huge_body\}''', + '''base64body''', + '''with no matching end marker''', + '''EC PRIVATE KEY-----\\nsecond''', ] diff --git a/CHANGELOG.md b/CHANGELOG.md index 49b94a0ea..bbb072493 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -151,6 +151,34 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). skips `FlushGuard`'s closing write) fails to parse outright, with a one-line recovery command now documented. No source code change; the trace format was already correct for Perfetto/`chrome://tracing` and the OTLP/Jaeger backend. +- `zeph-common`, `zeph-sanitizer`, `zeph-subagent`, `zeph-memory`: `scrub_secret_shapes` and + its consumers missed PEM private-key bodies and raw (no-prefix) AWS secret access keys + (issue #6592, follow-up to #6571); no existing pattern spanned a PEM key's multi-line body + at all, since `SECRET_PREFIXES`'s `-----BEGIN` entry only ever matches a literal token on a + single line. Added `PEM_PRIVATE_KEY_PATTERN` (multi-line, non-greedy, bounded to 8,192 + body characters, from `-----BEGIN ... PRIVATE KEY-----`-style headers through the matching + footer — covers `RSA`/`EC`/`DSA`/`OPENSSH`/`ENCRYPTED`/PGP `... BLOCK` label variants and + the RFC 4716 SSH2 `---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----` marker style) and a + `PEM_PRIVATE_KEY_UNTERMINATED_PATTERN` fallback (redacts a header with no matching footer, + bounded the same way, so truncated/adversarially-unterminated input or a footer chunk + dropped by a bounded channel still gets redacted) to `zeph_common::secrets`. Both run first + in `scrub_secret_shapes`'s pipeline (`zeph-sanitizer`) and in `zeph-memory`'s + `compression_guidelines::redact_sensitive` (previously not PEM-aware at all), ahead of the + prefix pass, so `-----BEGIN` isn't partially consumed before the full block is matched; + `zeph-core::redact::redact_secrets` picks the fix up transitively via `scrub_secret_shapes`. + `zeph-subagent`'s live transcript-forward streaming path (`forward.rs`) also needed a + dedicated fix: its fixed 256-byte holdback window could split a PEM block's header and + footer across two separately-sanitized deltas, so neither fragment matched the full-body + pattern and a middle slice with neither marker passed through unredacted; the holdback now + widens to cover an unterminated header (capped at 8,192 bytes) and extends forward past an + already-closed block's footer so the whole span is always flushed as one contiguous unit. + Also added `AWS_SECRET_KEY_PATTERN`, a context-anchored heuristic that flags a 40-or-more + character base64-ish run only when immediately preceded by a marker — `aws_secret_access_key`, + `aws_secret_key`, `secret_access_key`, `aws_session_token`, or `session_token`, each + tolerant of `_`/`-`/`.`/space separators (or none, so it also matches camelCase JSON keys + like `SecretAccessKey`/`SessionToken` verbatim) — avoiding false positives on ordinary + high-entropy strings (hashes, IDs) that share the same shape. + - `zeph-acp`: fixed a deadlock on every permission-gated tool call (issue #6656). `handle_prompt` awaited the entire agent turn inline inside the ACP SDK's `on_receive_request` callback, holding the SDK's strictly serial dispatch loop for the whole turn; since that same loop demultiplexes diff --git a/crates/zeph-common/src/secrets.rs b/crates/zeph-common/src/secrets.rs index e058010ea..82088a253 100644 --- a/crates/zeph-common/src/secrets.rs +++ b/crates/zeph-common/src/secrets.rs @@ -52,6 +52,129 @@ pub const BEARER_TOKEN_PATTERN: &str = r"(?i)(Authorization:\s*Bearer\s+)\S+"; /// empty signature segment. pub const JWT_PATTERN: &str = r"eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*"; +/// Regex pattern matching a full, properly-closed PEM/SSH2 private-key block, from the +/// `-----BEGIN ... PRIVATE KEY-----`-style header through the matching footer, inclusive of +/// the body between them (see #6592). +/// +/// The root cause reported in #6592 is that no existing detector spanned a PEM key's +/// multi-line body at all: the `-----BEGIN` entry in [`SECRET_PREFIXES`] only ever matches a +/// literal prefix on a single line by construction, so it was never capable of covering the +/// base64 body regardless of scrub ordering. Ordering still matters operationally, though: +/// running the prefix-based scrub before this pattern would let it consume just the +/// `-----BEGIN` token, preventing this pattern from ever matching the header again — which is +/// why PEM scrubbing must run first in `zeph_sanitizer::secret_shape::scrub_secret_shapes`'s +/// pipeline. +/// +/// The `(?s)` flag lets `.` match newlines so the pattern spans the full multi-line body. The +/// body is matched non-greedily and bounded to at most [`PEM_BODY_CAP`] characters +/// (`.{0,8192}?`) — generous headroom over a real key (RSA-4096 PEM is ~3.2 KB; a bound large +/// enough to also cover RSA-4096 was originally chosen at 65,536, but `regex` rejected that +/// pattern with `CompiledTooBig` under its default 10 MB compiled-program size limit, so +/// [`PEM_BODY_CAP`] is deliberately the largest power-of-two-ish bound confirmed to compile +/// under that limit) — so that (a) consecutive PEM blocks in the same text are each redacted +/// individually rather than swallowed into one match, and (b) a subagent cannot wrap arbitrary +/// transcript content between forged `-----BEGIN`/`-----END` markers to make an unbounded +/// amount of it vanish from a display surface. `regex` does not support backreferences, so the +/// footer's label is not required to match the header's label — over-matching a mismatched +/// pair is an acceptable tradeoff for a redaction scanner, since under-matching leaks key +/// material. A header with no matching footer at all (truncated input, or a footer chunk +/// dropped in a bounded channel) does not match this pattern — see +/// [`PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`], which must be applied afterward to still redact +/// that case. +/// +/// Keep the header/footer marker alternation (`-----BEGIN ... PRIVATE KEY(?: BLOCK)?-----` / +/// the RFC 4716 `---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----` alternative, and the matching +/// `END` forms) in sync with [`PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`]'s header alternation — +/// they must recognize exactly the same set of header markers. +pub const PEM_PRIVATE_KEY_PATTERN: &str = r"(?s)(?:-----BEGIN (?:[A-Z]+ )?PRIVATE KEY(?: BLOCK)?-----|---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----).{0,8192}?(?:-----END (?:[A-Z]+ )?PRIVATE KEY(?: BLOCK)?-----|---- END SSH2 ENCRYPTED PRIVATE KEY ----)"; + +/// Cap, in characters, on the PEM/SSH2 body matched by [`PEM_PRIVATE_KEY_PATTERN`] and +/// [`PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`]. Kept as a named constant so the value referenced +/// in both patterns' doc comments (and in `zeph_subagent::forward`'s streaming holdback cap, +/// which must buffer at least this many bytes past an unclosed header before force-flushing) +/// stays traceable to one definition, even though the patterns themselves are plain `&str` +/// literals (regex patterns can't be built from a `const usize` via string formatting at +/// const-eval time without an extra dependency, so the literal `8192` is duplicated in both +/// pattern strings — keep it in sync with this constant if it ever changes). +/// +/// Known accepted tradeoff (#6592 follow-up, "M5"): a **properly terminated** block whose body +/// exceeds this cap cannot be matched by [`PEM_PRIVATE_KEY_PATTERN`] (the footer lies past the +/// non-greedy bound), so [`PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`]'s fallback redacts only the +/// first `PEM_BODY_CAP` characters of the body — the remaining tail, plus the now-orphaned +/// `-----END...` footer text, is left unredacted. Real keys are comfortably under this cap +/// (RSA-4096 PEM ≈ 3.2 KB), so the realistic risk is low, but this is a direct, deliberate +/// tension with the cap's other purpose — bounding how much a forged/unterminated header can +/// hide (see the primary pattern's doc comment, point (b), and the +/// `adversarial_repeated_unterminated_pem_headers_are_bounded` test in `zeph-sanitizer`, which +/// asserts over-cap content *must* stay visible for exactly the opposite reason). One bound +/// cannot simultaneously guarantee "every terminated block, however large, is fully redacted" +/// and "an unterminated/forged block can only ever hide a bounded amount of surrounding +/// content" — this module chooses the anti-censorship guarantee and accepts the oversized- +/// terminated-block gap as the cost, on the grounds that a real key exceeding this cap is a +/// vanishingly rare shape to encounter versus a subagent hiding a large amount of legitimate +/// transcript behind a forged pair of markers. +pub const PEM_BODY_CAP: usize = 8192; + +/// Fallback regex matching a PEM/SSH2 private-key header with no matching footer found within +/// [`PEM_BODY_CAP`] characters — a header that is truncated, adversarially left unterminated, +/// or whose footer chunk was dropped by a bounded ingress channel (see #6592 follow-up). +/// +/// Must be applied *after* [`PEM_PRIVATE_KEY_PATTERN`]'s replace pass, so that every properly +/// closed block has already been consumed and only genuinely unterminated headers remain — +/// otherwise this pattern's greedy, footer-agnostic match would swallow a following +/// already-valid block's header too. +/// +/// The body is constrained to characters that can actually occur in a PEM body — base64 +/// alphabet plus whitespace (`[A-Za-z0-9+/=\s]{0,8192}`) — rather than "any character" +/// (`.{0,8192}`). An earlier version of this pattern used `.{0,8192}`, which meant *any* text +/// following an unterminated header (e.g. `"...PRIVATE KEY----- in file /etc/ssl/key.pem and +/// the deploy failed"`) was swallowed wholesale up to the cap, silently destroying up to 8 KB +/// of unrelated legitimate content whenever a subagent merely *mentioned* a PEM header without +/// including a body (see #6592 follow-up, "S3" — this was a real over-redaction regression, +/// not hypothetical). Constraining the body to PEM-plausible characters makes the match stop +/// at the first character that cannot appear in base64 (e.g. the first `.`, `,`, or other +/// prose punctuation), which keeps genuinely truncated-key coverage while collapsing false- +/// positive over-redaction on ordinary prose to near zero — plain English text almost always +/// contains such a character within a few words. +/// +/// Known narrow accepted gap (#6592 follow-up, "M7"): a *footerless* legacy encrypted PEM +/// (`Proc-Type: 4,ENCRYPTED`) or PGP (`Version: GnuPG v2`) armor's header/comment line contains +/// `:`, `,`, and other characters outside this class, so the match stops at that line and the +/// base64 body after it is left unredacted in this specific truncated-input case. Terminated +/// blocks of these same armor types are unaffected (they're covered by +/// [`PEM_PRIVATE_KEY_PATTERN`], which has no character-class restriction on the body). Not +/// fixed here — widening the class to cover armor-header-line punctuation would reopen most of +/// the S3 over-redaction blast radius this pattern exists to close. +pub const PEM_PRIVATE_KEY_UNTERMINATED_PATTERN: &str = r"(?:-----BEGIN (?:[A-Z]+ )?PRIVATE KEY(?: BLOCK)?-----|---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----)[A-Za-z0-9+/=\s]{0,8192}"; + +/// Regex pattern matching a raw AWS secret access key or session token immediately preceded +/// by a recognizable marker and its assignment separator (see #6592). +/// +/// Unlike the `AKIA`-prefixed access key ID, an AWS secret access key (and session token) has +/// no distinguishing prefix — it is just a base64-ish string, indistinguishable by shape alone +/// from an ordinary hash or identifier. Flagging *every* base64-ish run of that length would +/// produce excessive false positives, so this pattern only fires when the value is directly +/// anchored to a recognizable marker name. +/// +/// The marker alternation covers `aws_secret_access_key` / `aws_secret_key` / +/// `secret_access_key` / `aws_session_token` / `session_token`, each tolerant of `_`, `-`, +/// `.`, or a space as the inter-word separator (or none at all) — so the same alternation +/// also matches the camelCase JSON key names AWS's own tooling emits verbatim +/// (`SecretAccessKey`, `SessionToken`, e.g. from `aws configure export-credentials` or an STS +/// `AssumeRole` response), not just underscore-joined config-file names, following the +/// broader separator conventions gitleaks/trufflehog use for this rule class. An optional +/// quote is tolerated both before the separator (closing a quoted JSON key) and around the +/// value. +/// +/// The value itself matches 40 or more base64-alphabet characters plus up to two `=` padding +/// characters (`{40,}={0,2}`, not a fixed `{40}`) so a longer-than-standard value is redacted +/// in full rather than leaking everything past the 40th character. +/// +/// Capture group 1 covers the marker, separator, and optional opening quote; capture group 2 +/// covers an optional closing quote. Replacing with `"${1}[REDACTED]${2}"` preserves the +/// marker and quoting while redacting only the secret value. +pub const AWS_SECRET_KEY_PATTERN: &str = r#"(?i)((?:aws[_\-. ]?secret[_\-. ]?access[_\-. ]?key|aws[_\-. ]?secret[_\-. ]?key|secret[_\-. ]?access[_\-. ]?key|aws[_\-. ]?session[_\-. ]?token|session[_\-. ]?token)['"]?\s*[:=]\s*['"]?)[A-Za-z0-9+/]{40,}={0,2}(['"]?)"#; + #[cfg(test)] mod tests { use regex::Regex; @@ -96,4 +219,161 @@ mod tests { let re = Regex::new(&full).expect("alternation built from PATH_PREFIXES must compile"); assert!(re.is_match("/home/user/file")); } + + #[test] + fn pem_pattern_compiles_and_matches_plain_private_key() { + let re = Regex::new(PEM_PRIVATE_KEY_PATTERN).unwrap(); + let pem = + "-----BEGIN PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B\n-----END PRIVATE KEY-----"; + assert!(re.is_match(pem)); + } + + #[test] + fn pem_pattern_matches_common_label_variants() { + let re = Regex::new(PEM_PRIVATE_KEY_PATTERN).unwrap(); + for label in ["RSA", "EC", "DSA", "OPENSSH", "ENCRYPTED"] { + let pem = format!( + "-----BEGIN {label} PRIVATE KEY-----\nbase64body\n-----END {label} PRIVATE KEY-----" + ); + assert!(re.is_match(&pem), "failed for label: {label}"); + } + } + + #[test] + fn pem_pattern_matches_pgp_block_and_ssh2_rfc4716_variants() { + let re = Regex::new(PEM_PRIVATE_KEY_PATTERN).unwrap(); + let pgp = "-----BEGIN PGP PRIVATE KEY BLOCK-----\nbase64body\n-----END PGP PRIVATE KEY BLOCK-----"; + assert!(re.is_match(pgp), "failed for PGP PRIVATE KEY BLOCK"); + let ssh2 = "---- BEGIN SSH2 ENCRYPTED PRIVATE KEY ----\nbase64body\n---- END SSH2 ENCRYPTED PRIVATE KEY ----"; + assert!(re.is_match(ssh2), "failed for RFC 4716 SSH2 marker"); + } + + #[test] + fn pem_pattern_matches_mismatched_header_footer_labels() { + // Documented tradeoff: `regex` has no backreferences, so a footer whose label does + // not match the header's label still matches (over-matching, not under-matching). + let re = Regex::new(PEM_PRIVATE_KEY_PATTERN).unwrap(); + let mismatched = + "-----BEGIN RSA PRIVATE KEY-----\nbase64body\n-----END EC PRIVATE KEY-----"; + assert!( + re.is_match(mismatched), + "mismatched header/footer labels must still match (accepted over-match tradeoff)" + ); + } + + #[test] + fn pem_pattern_does_not_match_partial_markers() { + let re = Regex::new(PEM_PRIVATE_KEY_PATTERN).unwrap(); + assert!(!re.is_match("this text mentions BEGIN and PRIVATE but no PEM markers")); + assert!(!re.is_match("-----BEGIN PRIVATE KEY----- with no matching end marker")); + } + + #[test] + fn pem_unterminated_pattern_matches_footerless_header() { + let re = Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).unwrap(); + assert!(re.is_match("-----BEGIN RSA PRIVATE KEY-----\nbase64body with no end marker")); + assert!(!re.is_match("this text mentions BEGIN and PRIVATE but no PEM markers")); + } + + #[test] + fn pem_unterminated_pattern_body_is_bounded() { + // Adversarial input: a header that never closes, with a body far larger than the + // pattern's cap. The match must not extend past the bound (M3 / censorship-vector + // guard) — asserted by checking the matched span length rather than just "matches". + let re = Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).unwrap(); + let huge_body = "A".repeat(200_000); + let text = format!("-----BEGIN RSA PRIVATE KEY-----\n{huge_body}"); + let m = re + .find(&text) + .expect("unterminated header must still match"); + let header_len = "-----BEGIN RSA PRIVATE KEY-----".len(); + assert!( + m.len() <= header_len + PEM_BODY_CAP, + "match length {} exceeds header + {PEM_BODY_CAP}-char body cap", + m.len() + ); + } + + #[test] + fn pem_unterminated_pattern_handles_repeated_begin_with_no_end() { + // Adversarial input: multiple unterminated headers in sequence, none ever closed. + let re = Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).unwrap(); + let text = "-----BEGIN RSA PRIVATE KEY-----\nfirst\n-----BEGIN EC PRIVATE KEY-----\nsecond"; + assert!(re.is_match(text)); + } + + #[test] + fn pem_unterminated_pattern_stops_at_first_non_pem_character() { + // S3 regression guard: the fallback body is constrained to PEM-plausible characters + // (base64 alphabet + whitespace), so it must stop matching at the first character + // that cannot occur in a PEM body (e.g. a period) rather than swallowing an entire + // sentence of ordinary prose up to the 8192-char cap. + let re = Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).unwrap(); + let text = + "Found -----BEGIN RSA PRIVATE KEY----- in file /etc/ssl/key.pem and the deploy failed"; + let m = re.find(text).expect("header must still match"); + assert!( + !m.as_str().contains("and the deploy failed"), + "match must stop well before swallowing unrelated trailing prose: {:?}", + m.as_str() + ); + assert!( + text[m.end()..].contains("and the deploy failed"), + "trailing prose must remain outside the match, available for the caller to keep: {:?}", + &text[m.end()..] + ); + } + + #[test] + fn aws_secret_pattern_compiles_and_matches_marker_anchored_value() { + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + assert!(re.is_match("aws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + } + + #[test] + fn aws_secret_pattern_matches_camelcase_json_key_form() { + // S2: the canonical STS/`aws configure export-credentials`/SDK JSON key form. + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + assert!(re.is_match(r#""SecretAccessKey": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY""#)); + assert!(re.is_match(r#""SessionToken": "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY""#)); + } + + #[test] + fn aws_secret_pattern_matches_alternate_separator_forms() { + // S2: gitleaks/trufflehog-style separator flexibility beyond a literal underscore. + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + assert!(re.is_match("aws-secret-key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + assert!(re.is_match("aws.secret.key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + assert!(re.is_match("aws secret key: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + assert!(re.is_match("aws_session_token=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + } + + #[test] + fn aws_secret_pattern_redacts_full_longer_than_standard_value() { + // M1: `{40,}` must not truncate the match at 40 chars for a longer value. + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + let long_value = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEYEXTRA1234"; // 50 chars + let text = format!("aws_secret_access_key={long_value}"); + let m = re.find(&text).expect("must match"); + assert!( + m.as_str().ends_with(long_value), + "match must cover the full value, not just the first 40 chars: {}", + m.as_str() + ); + } + + #[test] + fn aws_secret_pattern_ignores_unanchored_high_entropy_string() { + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + // Same shape (40-char base64-ish run) but no marker precedes it. + assert!(!re.is_match("commit sha or hash: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + } + + #[test] + fn aws_secret_pattern_ignores_marker_like_identifier_suffix() { + // Explicitly confirmed non-finding: the marker must not fire on an identifier that + // merely starts with a marker name followed by more identifier characters. + let re = Regex::new(AWS_SECRET_KEY_PATTERN).unwrap(); + assert!(!re.is_match("let aws_secret_access_key_length = 40")); + } } diff --git a/crates/zeph-core/src/redact.rs b/crates/zeph-core/src/redact.rs index 1be13909d..5e5c54211 100644 --- a/crates/zeph-core/src/redact.rs +++ b/crates/zeph-core/src/redact.rs @@ -179,12 +179,50 @@ mod tests { #[test] fn redacts_private_key_header() { + // A header with no closing footer is now caught by the PEM footerless fallback + // (`[REDACTED_PEM_KEY]`), not the generic prefix pass (`[REDACTED]`) — the fallback + // matches first in the pipeline and consumes the header before the prefix pass runs. + // S3 (#6592 follow-up): the fallback body is constrained to PEM-plausible characters, + // so " in file" (plain prose, no punctuation the class would reject) still gets + // swallowed here — that's the known accepted tradeoff for short trailing runs of + // letters/spaces; `redacts_unterminated_header_does_not_swallow_unrelated_prose` below + // is the regression guard proving longer prose containing punctuation survives. let text = "Found -----BEGIN RSA PRIVATE KEY----- in file"; let result = redact_secrets(text); - assert!(result.contains("[REDACTED]")); + assert!(result.contains("[REDACTED_PEM_KEY]")); assert!(!result.contains("-----BEGIN")); } + #[test] + fn redacts_full_pem_private_key_body() { + // S1 (#6592 follow-up): `redact_secrets` delegates to + // `zeph_sanitizer::secret_shape::scrub_secret_shapes`, which now spans the full + // multi-line PEM body, not just the header token — debug dumps must not retain the + // base64 key material. + let text = "Found -----BEGIN RSA PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B\n-----END RSA PRIVATE KEY----- in file"; + let result = redact_secrets(text); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("MIIBVQIBADANBgkqhkiG9w0B")); + } + + #[test] + fn redacts_unterminated_header_does_not_swallow_unrelated_prose() { + // S3 regression guard (#6592 follow-up): the footerless fallback previously used + // `.{0,8192}` (any character), so an unterminated header mention swallowed up to 8 KB + // of unrelated legitimate content — e.g. a debug dump line merely *describing* where a + // key file lives lost everything after "PRIVATE KEY-----". The fallback body is now + // constrained to PEM-plausible characters, so the match stops at the first character + // that cannot occur in base64 (here, the `.` in the file extension). + let text = + "Found -----BEGIN RSA PRIVATE KEY----- in file /etc/ssl/key.pem and the deploy failed"; + let result = redact_secrets(text); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!( + result.contains("and the deploy failed"), + "unrelated trailing prose must survive redaction, not be swallowed: {result}" + ); + } + #[test] fn redacts_slack_tokens() { let text = "Bot token xoxb-123-456 and user xoxp-789"; diff --git a/crates/zeph-memory/src/store/compression_guidelines.rs b/crates/zeph-memory/src/store/compression_guidelines.rs index aea966f9e..ea504a917 100644 --- a/crates/zeph-memory/src/store/compression_guidelines.rs +++ b/crates/zeph-memory/src/store/compression_guidelines.rs @@ -9,7 +9,10 @@ use std::sync::LazyLock; use zeph_db::sql; use regex::Regex; -use zeph_common::secrets::{BEARER_TOKEN_PATTERN, JWT_PATTERN, PATH_PREFIXES, SECRET_PREFIXES}; +use zeph_common::secrets::{ + BEARER_TOKEN_PATTERN, JWT_PATTERN, PATH_PREFIXES, PEM_PRIVATE_KEY_PATTERN, + PEM_PRIVATE_KEY_UNTERMINATED_PATTERN, SECRET_PREFIXES, +}; use zeph_common::text::truncate_to_bytes_ref; use crate::error::MemoryError; @@ -41,6 +44,19 @@ static BEARER_RE: LazyLock = /// The signature segment uses `*` to handle `alg=none` JWTs with an empty signature. static JWT_RE: LazyLock = LazyLock::new(|| Regex::new(JWT_PATTERN).expect("jwt regex")); +/// Matches a full, properly-closed PEM/SSH2 private-key block (see +/// `zeph_common::secrets::PEM_PRIVATE_KEY_PATTERN`). Unlike the `-----BEGIN` entry in +/// `SECRET_PREFIXES`, this spans the multi-line base64 body, not just the header token (#6592 +/// follow-up — persisted compression-failure text must not retain PEM key material). +static PEM_RE: LazyLock = + LazyLock::new(|| Regex::new(PEM_PRIVATE_KEY_PATTERN).expect("pem regex")); + +/// Fallback for a PEM/SSH2 header with no matching footer (truncated/adversarial input). Must +/// run after `PEM_RE` so already-closed blocks are consumed first. +static PEM_UNTERMINATED_RE: LazyLock = LazyLock::new(|| { + Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).expect("pem unterminated regex") +}); + /// Redact secrets and filesystem paths from text before persistent storage. /// /// Returns `Cow::Borrowed` when no sensitive content is found (zero-alloc fast path). @@ -48,7 +64,22 @@ pub(crate) fn redact_sensitive(text: &str) -> Cow<'_, str> { // Each replace_all may return Cow::Borrowed (no match) or Cow::Owned (replaced). // We materialise intermediate Owned values into String so that subsequent steps // do not hold a borrow of a local. - let s0: Cow<'_, str> = SECRET_RE.replace_all(text, "[REDACTED]"); + // + // PEM passes run first, before the prefix pass: `SECRET_RE` (built from + // `SECRET_PREFIXES`, which includes the raw `-----BEGIN` literal) would otherwise consume + // just the header token on its own, leaving the rest of a multi-line PEM body — the + // actual key material — unredacted (see `zeph_sanitizer::secret_shape::scrub_secret_shapes` + // for the equivalent transcript-forward-path fix and its rationale). + let s_pem: Cow<'_, str> = PEM_RE.replace_all(text, "[REDACTED_PEM_KEY]"); + let s_pem_fallback: Cow<'_, str> = + match PEM_UNTERMINATED_RE.replace_all(s_pem.as_ref(), "[REDACTED_PEM_KEY]") { + Cow::Borrowed(_) => s_pem, + Cow::Owned(o) => Cow::Owned(o), + }; + let s0: Cow<'_, str> = match SECRET_RE.replace_all(s_pem_fallback.as_ref(), "[REDACTED]") { + Cow::Borrowed(_) => s_pem_fallback, + Cow::Owned(o) => Cow::Owned(o), + }; let s1: Cow<'_, str> = match PATH_RE.replace_all(s0.as_ref(), "[PATH]") { Cow::Borrowed(_) => s0, Cow::Owned(o) => Cow::Owned(o), @@ -960,6 +991,27 @@ mod tests { ); } + #[test] + fn redact_sensitive_full_pem_private_key_body_is_redacted() { + // #6592 follow-up: persisted compression-failure text must not retain a PEM private + // key's multi-line base64 body — only the `-----BEGIN` header token was previously + // covered by `SECRET_RE` (built from `SECRET_PREFIXES`). + let input = "compressed context leaked:\n-----BEGIN RSA PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B\n-----END RSA PRIVATE KEY-----\nend of context"; + let result = redact_sensitive(input); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("MIIBVQIBADANBgkqhkiG9w0B")); + assert!(result.contains("compressed context leaked:")); + assert!(result.contains("end of context")); + } + + #[test] + fn redact_sensitive_footerless_pem_header_is_redacted() { + let input = "-----BEGIN RSA PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B with no footer"; + let result = redact_sensitive(input); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("MIIBVQIBADANBgkqhkiG9w0B")); + } + // ── Category-aware store methods (MF-4) ────────────────────────────────── #[tokio::test] diff --git a/crates/zeph-sanitizer/src/secret_shape.rs b/crates/zeph-sanitizer/src/secret_shape.rs index 213ded45b..601b3854a 100644 --- a/crates/zeph-sanitizer/src/secret_shape.rs +++ b/crates/zeph-sanitizer/src/secret_shape.rs @@ -17,7 +17,10 @@ use std::borrow::Cow; use std::sync::LazyLock; use regex::Regex; -use zeph_common::secrets::{BEARER_TOKEN_PATTERN, JWT_PATTERN, SECRET_PREFIXES}; +use zeph_common::secrets::{ + AWS_SECRET_KEY_PATTERN, BEARER_TOKEN_PATTERN, JWT_PATTERN, PEM_PRIVATE_KEY_PATTERN, + PEM_PRIVATE_KEY_UNTERMINATED_PATTERN, SECRET_PREFIXES, +}; // Matches any secret prefix followed by non-whitespace/quote/bracket characters. A single // alternation pass covers every prefix in `SECRET_PREFIXES`; each prefix is regex-escaped @@ -38,8 +41,23 @@ static BEARER_REGEX: LazyLock = static JWT_REGEX: LazyLock = LazyLock::new(|| Regex::new(JWT_PATTERN).expect("jwt shape regex is valid")); +static PEM_REGEX: LazyLock = + LazyLock::new(|| Regex::new(PEM_PRIVATE_KEY_PATTERN).expect("pem shape regex is valid")); + +// Fallback for a PEM/SSH2 header with no matching footer (truncated input, adversarially +// unterminated, or a footer chunk dropped by a bounded ingress channel — see #6592 follow-up +// and `PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`'s doc comment). Must run after `PEM_REGEX` so +// already-closed blocks are consumed first and only genuinely unterminated headers remain. +static PEM_UNTERMINATED_REGEX: LazyLock = LazyLock::new(|| { + Regex::new(PEM_PRIVATE_KEY_UNTERMINATED_PATTERN).expect("pem unterminated shape regex is valid") +}); + +static AWS_SECRET_REGEX: LazyLock = + LazyLock::new(|| Regex::new(AWS_SECRET_KEY_PATTERN).expect("aws secret shape regex is valid")); + /// Replace secret-shaped substrings (known API-key prefixes, `Authorization: Bearer` headers, -/// standalone JWTs) with redaction markers. +/// standalone JWTs, PEM private-key blocks, marker-anchored AWS secret access keys) with +/// redaction markers. /// /// Unlike [`SecretMaskRegistry::mask`][crate::secret_mask::SecretMaskRegistry::mask], this /// does not require the secret value to have been registered ahead of time — it flags @@ -47,6 +65,14 @@ static JWT_REGEX: LazyLock = /// or echoes in generated text. Returns `Cow::Borrowed` when nothing matched (zero-allocation /// fast path). /// +/// No existing pattern spanned a PEM key's multi-line body before this fix (see #6592) — the +/// `-----BEGIN` entry in [`SECRET_PREFIXES`] only ever matches a literal token on a single +/// line, regardless of scrub ordering. Two PEM passes run first, before the prefix pass: the +/// properly-closed-block pattern, then a footerless-header fallback (truncated/adversarial +/// input, or a footer chunk dropped by a bounded ingress channel). Running them first also +/// matters operationally — the prefix pass would otherwise consume just the `-----BEGIN` +/// token and prevent either PEM pattern from matching the header at all. +/// /// # Examples /// /// ```rust @@ -58,11 +84,24 @@ static JWT_REGEX: LazyLock = /// ``` #[must_use] pub fn scrub_secret_shapes(text: &str) -> Cow<'_, str> { - let has_prefix_match = SECRET_PREFIXES.iter().any(|p| text.contains(*p)); + let after_pem: Cow<'_, str> = PEM_REGEX.replace_all(text, "[REDACTED_PEM_KEY]"); + + let after_pem_fallback: Cow<'_, str> = + match PEM_UNTERMINATED_REGEX.replace_all(after_pem.as_ref(), "[REDACTED_PEM_KEY]") { + Cow::Borrowed(_) => after_pem, + Cow::Owned(s) => Cow::Owned(s), + }; + + let has_prefix_match = SECRET_PREFIXES + .iter() + .any(|p| after_pem_fallback.contains(*p)); let after_prefixes: Cow<'_, str> = if has_prefix_match { - SECRET_REGEX.replace_all(text, "[REDACTED]") + match SECRET_REGEX.replace_all(after_pem_fallback.as_ref(), "[REDACTED]") { + Cow::Borrowed(_) => after_pem_fallback, + Cow::Owned(s) => Cow::Owned(s), + } } else { - Cow::Borrowed(text) + after_pem_fallback }; let after_bearer: Cow<'_, str> = @@ -71,8 +110,14 @@ pub fn scrub_secret_shapes(text: &str) -> Cow<'_, str> { Cow::Owned(s) => Cow::Owned(s), }; - match JWT_REGEX.replace_all(after_bearer.as_ref(), "[REDACTED_JWT]") { - Cow::Borrowed(_) => after_bearer, + let after_jwt: Cow<'_, str> = + match JWT_REGEX.replace_all(after_bearer.as_ref(), "[REDACTED_JWT]") { + Cow::Borrowed(_) => after_bearer, + Cow::Owned(s) => Cow::Owned(s), + }; + + match AWS_SECRET_REGEX.replace_all(after_jwt.as_ref(), "${1}[REDACTED]${2}") { + Cow::Borrowed(_) => after_jwt, Cow::Owned(s) => Cow::Owned(s), } } @@ -124,4 +169,98 @@ mod tests { assert!(!result.contains(*prefix), "prefix not redacted: {prefix}"); } } + + #[test] + fn redacts_full_pem_block() { + let text = "here is my key:\n-----BEGIN RSA PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B\nAQEFAASCAT8wggE7AgEAAkEA\n-----END RSA PRIVATE KEY-----\nthanks"; + let result = scrub_secret_shapes(text); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("MIIBVQIBADANBgkqhkiG9w0B")); + assert!(result.contains("here is my key:")); + assert!(result.contains("thanks")); + } + + #[test] + fn redacts_multiple_pem_blocks_independently() { + let text = "-----BEGIN PRIVATE KEY-----\nfirstbody\n-----END PRIVATE KEY-----\nsome text in between\n-----BEGIN EC PRIVATE KEY-----\nsecondbody\n-----END EC PRIVATE KEY-----"; + let result = scrub_secret_shapes(text); + assert_eq!(result.matches("[REDACTED_PEM_KEY]").count(), 2); + assert!(!result.contains("firstbody")); + assert!(!result.contains("secondbody")); + assert!(result.contains("some text in between")); + } + + #[test] + fn no_false_positive_on_bare_begin_or_private_words() { + let text = "Let's BEGIN the PRIVATE discussion about keys without any PEM markers"; + let result = scrub_secret_shapes(text); + assert_eq!(result, text); + assert!(matches!(result, Cow::Borrowed(_))); + } + + #[test] + fn redacts_pem_block_with_mismatched_header_footer_labels() { + // Documented over-match tradeoff: `regex` has no backreferences, so a footer whose + // label doesn't match the header's label still gets redacted (over-matching is + // acceptable; under-matching would leak key material). + let text = "-----BEGIN RSA PRIVATE KEY-----\nbody-material\n-----END EC PRIVATE KEY-----"; + let result = scrub_secret_shapes(text); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("body-material")); + } + + #[test] + fn redacts_footerless_pem_header_via_fallback() { + // C2: a PEM header with no matching footer at all (truncated input, adversarially + // omitted, or a footer chunk dropped by a bounded channel) must still be redacted — + // not just the header token, leaving the whole body exposed. + let text = "here is my key:\n-----BEGIN RSA PRIVATE KEY-----\nMIIBVQIBADANBgkqhkiG9w0B\nAQEFAASCAT8wggE7AgEAAkEA"; + let result = scrub_secret_shapes(text); + assert!(result.contains("[REDACTED_PEM_KEY]")); + assert!(!result.contains("MIIBVQIBADANBgkqhkiG9w0B")); + assert!(!result.contains("-----BEGIN")); + assert!(result.contains("here is my key:")); + } + + #[test] + fn adversarial_repeated_unterminated_pem_headers_are_bounded() { + // M3 / anti-censorship guard: a subagent wrapping arbitrary content behind a forged + // or genuinely unterminated header must not be able to make an unbounded amount of it + // vanish — only up to PEM_BODY_CAP characters past each header are redacted; content + // beyond that cap must remain visible rather than being silently swallowed. + use zeph_common::secrets::PEM_BODY_CAP; + + let huge_body = "A".repeat(50_000); + let text = format!( + "-----BEGIN RSA PRIVATE KEY-----\n{huge_body}\n-----BEGIN EC PRIVATE KEY-----\n{huge_body}" + ); + let result = scrub_secret_shapes(&text); + assert_eq!(result.matches("[REDACTED_PEM_KEY]").count(), 2); + // Each huge_body is far larger than PEM_BODY_CAP, so most of its 'A' characters must + // still be present in the output — proving the fallback did not swallow it whole. + let remaining_as = result.matches('A').count(); + assert!( + remaining_as > huge_body.len() - PEM_BODY_CAP, + "expected most of the oversized body to remain visible past the cap, got only \ + {remaining_as} 'A' chars remaining" + ); + } + + #[test] + fn redacts_marker_anchored_aws_secret_key() { + let text = "aws_secret_access_key=wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"; + let result = scrub_secret_shapes(text); + assert!(!result.contains("wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")); + assert!(result.contains("[REDACTED]")); + assert!(result.contains("aws_secret_access_key=")); + } + + #[test] + fn preserves_unanchored_high_entropy_string() { + // Same 40-char base64-ish shape as a real AWS secret key, but with no marker nearby — + // must not be flagged, or ordinary hashes/IDs would be false positives. + let text = "build hash: wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY was produced"; + let result = scrub_secret_shapes(text); + assert_eq!(result, text); + } } diff --git a/crates/zeph-subagent/src/forward.rs b/crates/zeph-subagent/src/forward.rs index 19e9d49c3..4dd35fe95 100644 --- a/crates/zeph-subagent/src/forward.rs +++ b/crates/zeph-subagent/src/forward.rs @@ -189,6 +189,28 @@ fn sanitize_text(raw_text: &str, def_name: &str, layers: &SanitizeLayers) -> Str /// practically unreachable for realistic secret/PII lengths. const SANITIZE_HOLDBACK_BYTES: usize = 256; +/// Cap, in bytes, on how far a progressive flush ([`split_off_safe_prefix`] with a non-zero +/// `holdback`) will widen its holdback to keep an unterminated PEM/SSH2 private-key header +/// (see `zeph_common::secrets::PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`) fully inside the pending +/// buffer, so the eventual flush is still covered end-to-end by that fallback pattern's own +/// `PEM_BODY_CAP` bound (currently 8,192 characters — kept equal here so a force-flushed +/// chunk is never larger than what the fallback pattern can redact in one match). +/// +/// Without this cap, a subagent that never closes a `-----BEGIN ... PRIVATE KEY-----` block +/// (adversarially, or because the footer chunk was tail-dropped by the bounded ingress +/// channel) would force this buffer to grow without bound, since the "hold back to the +/// header's start" rule below would otherwise apply for the rest of the task's lifetime. +/// +/// Known low-priority UX gap (#6592 follow-up, "M6", not fixed in this pass): while a header +/// is held back, up to this many bytes of a subagent's *legitimate* remaining output can sit +/// unflushed with no visible progress in the live transcript (TUI detail view / `--bare` +/// stdout) until either the footer arrives or the terminal flush releases it — delayed, never +/// dropped, so no data is lost, but a short remaining answer can appear frozen for a moment. +/// No status indicator (per CLAUDE.md's TUI background-status rule) currently distinguishes +/// this from a genuine stall. Left undone here since it is UI/status plumbing rather than a +/// redaction-correctness fix; worth a small follow-up if it proves noticeable in practice. +const PEM_HOLDBACK_CAP_BYTES: usize = zeph_common::secrets::PEM_BODY_CAP; + /// Per-task raw text accumulated but not yet sanitized/emitted (review Critical Issue #2). /// /// Kept separate for the `Text` and `Thinking` streams since they are independent logical @@ -199,18 +221,114 @@ struct PendingSanitizeBuffers { thinking: String, } +/// Byte offset in `buf` of the last PEM/SSH2 header marker (`-----BEGIN` or `---- BEGIN`) +/// starting strictly before `before`, if any. +fn last_header_before(buf: &str, before: usize) -> Option { + let region = &buf[..before]; + [region.rfind("-----BEGIN"), region.rfind("---- BEGIN")] + .into_iter() + .flatten() + .max() +} + +/// Byte offset just past the first PEM/SSH2 footer marker's `END` token (`-----END` or +/// `---- END`, both exactly 8 bytes) found anywhere in `buf` at or after `marker_idx`, if any. +fn footer_end_after(buf: &str, marker_idx: usize) -> Option { + let tail = &buf[marker_idx..]; + let end_offset = [tail.find("-----END"), tail.find("---- END")] + .into_iter() + .flatten() + .min()?; + Some(marker_idx + end_offset + 8) +} + +/// Compute the safe progressive-flush boundary for `buf`, starting from the flat-holdback +/// `natural_target` (critic C1/C1-R: closes the gap where a PEM block split across streamed +/// deltas would otherwise reach a sink as two or more separately-sanitized fragments, none of +/// which contains the whole header-to-footer span). +/// +/// A first version of this function only ever inspected the *last* header marker in the whole +/// buffer via `rfind`. That is unsound: pulling the cut back to that marker's start can land +/// it in the middle of an **earlier**, already-complete block — the flushed prefix carries +/// that earlier block's header (so a fallback pattern redacts *something*), but what remains +/// in the buffer is a headerless middle fragment of key body that no pattern can ever match on +/// any later flush. Concretely, a complete key block immediately followed by a *different* PEM +/// armor type in the same delta (e.g. `-----BEGIN RSA PRIVATE KEY-----`...`-----END RSA +/// PRIVATE KEY-----` followed by `-----BEGIN CERTIFICATE-----`, an ordinary key+cert bundle — +/// not an adversarial construction) reproduced this: `rfind` finds the `CERTIFICATE` header, +/// classifies it as unterminated, and pulls the cut back into the *first* block's body. +/// +/// This version instead walks backward from `natural_target`: find the last header marker +/// starting before the candidate cut; if it has a footer whose end lies at or before the +/// candidate, the candidate is safe as-is. Otherwise (no footer anywhere, or a footer that +/// ends *after* the candidate) the candidate cannot be trusted — pull it back to that marker's +/// own start and repeat, so an earlier header found on the next iteration is validated against +/// the *new*, smaller candidate rather than being skipped. The candidate strictly decreases +/// each iteration a header is found, so this always terminates. Only once a marker turns out +/// to have **no footer anywhere in `buf`** is [`PEM_HOLDBACK_CAP_BYTES`] applied, forcing a +/// partial flush up to `buf.len() - PEM_HOLDBACK_CAP_BYTES` if that is further forward than +/// the marker itself — so a header that never closes (adversarial, or a dropped footer chunk) +/// cannot force unbounded buffering, while a header that *does* close later (just further away +/// than the cap) is never force-flushed mid-body, deferring instead to a future call once its +/// footer is within reach (see the `PEM_BODY_CAP` doc comment in `zeph_common::secrets` for the +/// accepted tradeoff when even that eventual span exceeds the cap). +fn pem_safe_flush_target(buf: &str, natural_target: usize) -> usize { + let mut candidate = natural_target; + loop { + let Some(marker_idx) = last_header_before(buf, candidate) else { + return candidate; + }; + match footer_end_after(buf, marker_idx) { + Some(block_end) if block_end <= candidate => return candidate, + Some(_) => candidate = marker_idx, + None => { + let capped = buf.len().saturating_sub(PEM_HOLDBACK_CAP_BYTES); + return marker_idx.max(capped); + } + } + } +} + /// Split off `buf`'s sanitizable prefix, leaving the last `holdback` bytes (rounded down to /// the nearest UTF-8 char boundary, same class of problem as UTF-8 chunk-boundary handling) /// in place for a future call to potentially combine with. Pass `holdback = 0` to flush the /// entire remaining buffer — used once no more data for this task is coming (an explicit /// `Terminal` chunk or the hard-abort backstop), so buffered content is only ever delayed, -/// never silently dropped. Returns `None` when there is nothing new to emit yet. +/// never silently dropped. +/// +/// When `holdback` is non-zero (a progressive, non-terminal flush), the flush boundary is +/// adjusted by [`pem_safe_flush_target`] around any PEM/SSH2 header marker(s) in `buf` so a +/// flat byte-count holdback alone can never split a PEM block's `BEGIN` and `END` markers +/// across two separate `sanitize_text` calls, each seeing only a fragment and none matching +/// the full-body PEM pattern as a unit. +/// +/// A final flush (`holdback == 0`) always flushes everything regardless, since nothing more +/// is coming for this task; any still-unterminated header at that point is handled by +/// `PEM_PRIVATE_KEY_UNTERMINATED_PATTERN`'s own fallback redaction once sanitized. +/// +/// Returns `None` when there is nothing new to emit yet. fn split_off_safe_prefix(buf: &mut String, holdback: usize) -> Option { if buf.is_empty() { return None; } - let target = buf.len().saturating_sub(holdback); - let boundary = buf.floor_char_boundary(target); + let target = if holdback == 0 { + buf.len() + } else { + // C3 (#6592 follow-up): `pem_safe_flush_target` and its helpers slice `buf` directly + // (`&buf[..before]`, `&buf[marker_idx..]`) before this function's own + // `floor_char_boundary` call below ever runs, so the candidate passed in must already + // sit on a UTF-8 char boundary — a raw `buf.len() - holdback` byte offset can land + // mid-codepoint on multibyte input (CJK, emoji, accented text) and panic. Every offset + // used for further slicing within `pem_safe_flush_target` (marker/footer positions + // from `rfind`/`find` on ASCII-only marker literals) is inherently boundary-aligned, + // so aligning only this entry value is sufficient. The one exception — `capped` in + // the unterminated-header branch, a raw arithmetic offset — is never itself used to + // slice `buf` again; it is only returned and re-aligned by this function's own + // `floor_char_boundary` call below. + let natural_target = buf.floor_char_boundary(buf.len().saturating_sub(holdback)); + pem_safe_flush_target(buf, natural_target) + }; + let boundary = buf.floor_char_boundary(target.min(buf.len())); if boundary == 0 { return None; } @@ -1183,6 +1301,245 @@ mod tests { ); } + #[tokio::test(start_paused = true)] + async fn pem_block_split_across_chunk_boundary_is_still_fully_masked() { + // Critic C1: a PEM block whose header+body arrive in one delta and whose footer + // arrives in a later delta must not have its header sanitized in isolation (splitting + // it from the body/footer, and — because the fixed-256-byte flat holdback alone would + // let a middle fragment with neither BEGIN nor END pass through completely + // unredacted). The header+body chunk here (~330 bytes) deliberately exceeds + // SANITIZE_HOLDBACK_BYTES so a flat holdback alone would have force-flushed part of + // the still-open block before the footer chunk arrives. + let task_id: Arc = Arc::from("task-pem-chunked"); + let def_name: Arc = Arc::from("agent-pem-chunked"); + let (sender, rx) = new_channel(Arc::clone(&task_id), Arc::clone(&def_name)); + let buffer = new_buffer(); + + let body = "X".repeat(300); + sender.send_text("intro text before the key "); + sender.send_text(&format!("-----BEGIN RSA PRIVATE KEY-----\n{body}")); + sender.send_text("\n-----END RSA PRIVATE KEY-----\nfollowing text after the key"); + sender.send_terminal(SubAgentState::Completed); + drop(sender); + + let seen = Arc::new(std::sync::Mutex::new(Vec::::new())); + let collected = Arc::clone(&seen); + run_forward_drain_with( + task_id, + def_name, + rx, + layers(), + ForwardSurfaces { + tui: true, + bare: false, + }, + buffer, + move |chunk, surfaces, buffer| { + collected.lock().unwrap().push(chunk.clone()); + dispatch_chunk(chunk, surfaces, buffer); + }, + ) + .await; + + let combined = collect_forwarded_text(&seen.lock().unwrap()); + assert!( + !combined.contains(&body), + "PEM body must not survive split across a chunk boundary: {combined}" + ); + assert!( + !combined.contains('X'), + "no raw PEM body fragment may leak through an isolated flush of a middle slice \ + that itself contains neither BEGIN nor END: {combined}" + ); + assert!( + combined.contains("[REDACTED_PEM_KEY]"), + "PEM placeholder must be present in the combined forwarded text: {combined}" + ); + assert!( + combined.contains("intro text before the key"), + "text preceding the PEM block must still be forwarded: {combined}" + ); + assert!( + combined.contains("following text after the key"), + "text following the PEM block must still be forwarded: {combined}" + ); + } + + #[tokio::test(start_paused = true)] + async fn complete_key_immediately_followed_by_different_pem_armor_leaks_nothing() { + // Critic C1-R: a complete, already-closed key block immediately followed by a + // *different* PEM armor type's header (e.g. a certificate) in the same delta — an + // ordinary key+cert bundle, not an adversarial construction. The first fix for C1 + // only ever inspected the *last* header marker via `rfind`, found the CERTIFICATE + // header unterminated, and pulled the cut back into the middle of the already-closed + // RSA key's body, leaking a headerless middle fragment that no pattern could later + // match. `pem_safe_flush_target` must walk backward and validate the RSA block + // separately from the trailing CERTIFICATE header. + let task_id: Arc = Arc::from("task-pem-bundle"); + let def_name: Arc = Arc::from("agent-pem-bundle"); + let (sender, rx) = new_channel(Arc::clone(&task_id), Arc::clone(&def_name)); + let buffer = new_buffer(); + + // Filler character 'Z' deliberately chosen not to collide with any surrounding literal + // text (placeholders, marker labels) so a leak is unambiguous in the assertions below. + let key_body = "Z".repeat(400); + sender.send_text(&format!( + "-----BEGIN RSA PRIVATE KEY-----\n{key_body}\n-----END RSA PRIVATE KEY-----\n\ + -----BEGIN CERTIFICATE-----\nMIIBcertbody" + )); + sender.send_text("\n-----END CERTIFICATE-----\nbundle complete"); + sender.send_terminal(SubAgentState::Completed); + drop(sender); + + let seen = Arc::new(std::sync::Mutex::new(Vec::::new())); + let collected = Arc::clone(&seen); + run_forward_drain_with( + task_id, + def_name, + rx, + layers(), + ForwardSurfaces { + tui: true, + bare: false, + }, + buffer, + move |chunk, surfaces, buffer| { + collected.lock().unwrap().push(chunk.clone()); + dispatch_chunk(chunk, surfaces, buffer); + }, + ) + .await; + + let combined = collect_forwarded_text(&seen.lock().unwrap()); + assert!( + !combined.contains('Z'), + "no fragment of the RSA key body may leak when immediately followed by a \ + different PEM armor type in the same delta: {combined}" + ); + assert!( + combined.contains("[REDACTED_PEM_KEY]"), + "PEM placeholder must be present for the private key: {combined}" + ); + assert!( + combined.contains("bundle complete"), + "text following the bundle must still be forwarded: {combined}" + ); + // The certificate itself is public material, not a secret — it is not expected to be + // redacted by the private-key patterns (only that the *key* leaked nothing above). + } + + #[tokio::test(start_paused = true)] + async fn complete_key_followed_by_bare_trailing_begin_leaks_nothing() { + // Critic C1-R, second reproduction: a complete key block followed by a bare trailing + // `-----BEGIN` (no label, no body yet — e.g. the very start of the next streamed + // delta) in the same buffer. Must not leak any of the first block's body either. + let task_id: Arc = Arc::from("task-pem-trailing-begin"); + let def_name: Arc = Arc::from("agent-pem-trailing-begin"); + let (sender, rx) = new_channel(Arc::clone(&task_id), Arc::clone(&def_name)); + let buffer = new_buffer(); + + let key_body = "Z".repeat(400); + sender.send_text(&format!( + "-----BEGIN RSA PRIVATE KEY-----\n{key_body}\n-----END RSA PRIVATE KEY-----\n-----BEGIN" + )); + sender.send_text(" EC PRIVATE KEY-----\nsecondbody\n-----END EC PRIVATE KEY-----\ndone"); + sender.send_terminal(SubAgentState::Completed); + drop(sender); + + let seen = Arc::new(std::sync::Mutex::new(Vec::::new())); + let collected = Arc::clone(&seen); + run_forward_drain_with( + task_id, + def_name, + rx, + layers(), + ForwardSurfaces { + tui: true, + bare: false, + }, + buffer, + move |chunk, surfaces, buffer| { + collected.lock().unwrap().push(chunk.clone()); + dispatch_chunk(chunk, surfaces, buffer); + }, + ) + .await; + + let combined = collect_forwarded_text(&seen.lock().unwrap()); + assert!( + !combined.contains('Z'), + "no fragment of the first key body may leak when a bare trailing -----BEGIN \ + follows it in the same buffer: {combined}" + ); + assert!( + !combined.contains("secondbody"), + "no fragment of the second key body may leak either: {combined}" + ); + assert_eq!( + combined.matches("[REDACTED_PEM_KEY]").count(), + 2, + "both blocks must be redacted independently: {combined}" + ); + assert!( + combined.contains("done"), + "trailing text must survive: {combined}" + ); + } + + #[tokio::test(start_paused = true)] + async fn pem_holdback_boundary_computation_does_not_panic_on_multibyte_text() { + // Critic C3: `pem_safe_flush_target`'s helpers slice the buffer using the raw + // `buf.len() - holdback` byte offset, computed *before* any UTF-8 char-boundary + // alignment — landing mid-codepoint on CJK/emoji/accented text panics + // ("byte index N is not a char boundary; it is inside '中'"), killing the drain task + // and losing the whole pending buffer. This body (300 repeats of a 3-byte CJK + // character, no footer in the first delta) is sized so the natural pre-header + // holdback cut (`buf.len() - SANITIZE_HOLDBACK_BYTES`) lands inside the CJK run, not + // on a character boundary — the exact shape the critic's sweep used to reproduce it. + let task_id: Arc = Arc::from("task-pem-multibyte"); + let def_name: Arc = Arc::from("agent-pem-multibyte"); + let (sender, rx) = new_channel(Arc::clone(&task_id), Arc::clone(&def_name)); + let buffer = new_buffer(); + + let cjk_body: String = std::iter::repeat_n('中', 300).collect(); + sender.send_text(&format!("-----BEGIN RSA PRIVATE KEY-----\n{cjk_body}")); + sender.send_text("\n-----END RSA PRIVATE KEY-----\ndone"); + sender.send_terminal(SubAgentState::Completed); + drop(sender); + + let seen = Arc::new(std::sync::Mutex::new(Vec::::new())); + let collected = Arc::clone(&seen); + // Must not panic (the actual regression under test) — a panic here aborts the drain + // task and silently stops forwarding for the rest of the subagent's run. + run_forward_drain_with( + task_id, + def_name, + rx, + layers(), + ForwardSurfaces { + tui: true, + bare: false, + }, + buffer, + move |chunk, surfaces, buffer| { + collected.lock().unwrap().push(chunk.clone()); + dispatch_chunk(chunk, surfaces, buffer); + }, + ) + .await; + + let combined = collect_forwarded_text(&seen.lock().unwrap()); + assert!( + !combined.contains('中'), + "CJK key body must not leak: {combined}" + ); + assert!(combined.contains("[REDACTED_PEM_KEY]")); + assert!( + combined.contains("done"), + "trailing text must survive: {combined}" + ); + } + #[tokio::test(start_paused = true)] async fn buffer_entry_survives_during_grace_window_then_evicted() { // S3: the grace window's entire purpose is that a TUI view opened just after