fix(agent): re-introduce credential scrubbing on the tinyagents tool seam (tinyhumansai#4453)

src/openhuman/agent/harness/credentials.rs

@@ -1,50 +1,115 @@
 use regex::Regex;
 use std::sync::LazyLock;
 
+/// Key/value secrets: `token=…`, `api_key: "…"`, `password='…'`, etc. Matches a
+/// known credential key followed by `:`/`=` and a value of ≥8 chars (quoted or
+/// bare). This is the legacy pattern the in-house engine ran on every tool
+/// output.
 static SENSITIVE_KV_REGEX: LazyLock<Regex> = LazyLock::new(|| {
     Regex::new(r#"(?i)(token|api[_-]?key|password|secret|user[_-]?key|bearer|credential)["']?\s*[:=]\s*(?:"([^"]{8,})"|'([^']{8,})'|([a-zA-Z0-9_\-\.]{8,}))"#).unwrap()
 });
 
+/// Standalone AWS access-key IDs (`AKIA…`, `ASIA…`) — a fixed 20-char token that
+/// carries no `key: value` framing, so the KV regex above never catches a bare
+/// occurrence (env dumps, JSON API responses, shell output).
+static AWS_ACCESS_KEY_REGEX: LazyLock<Regex> =
+    LazyLock::new(|| Regex::new(r"\b((?:AKIA|ASIA)[0-9A-Z]{16})\b").unwrap());
+
+/// Standalone OpenAI-style secret keys (`sk-…`, `sk-proj-…`) that appear without
+/// a preceding credential key.
+static OPENAI_KEY_REGEX: LazyLock<Regex> =
+    LazyLock::new(|| Regex::new(r"\b(sk-[A-Za-z0-9_\-]{20,})\b").unwrap());
+
+/// `Bearer <token>` authorization values (space-separated, so the KV regex —
+/// which needs a `:`/`=` immediately after the keyword — misses them).
+static BEARER_TOKEN_REGEX: LazyLock<Regex> =
+    LazyLock::new(|| Regex::new(r"(?i)(bearer)\s+([A-Za-z0-9._\-]{8,})").unwrap());
+
+/// Redact a raw secret value, preserving up to the first 4 chars for context.
+/// UTF-8 safe (slices on a char boundary). Values ≤4 chars are fully redacted.
+fn redact_value(val: &str) -> String {
+    let prefix = if val.chars().count() > 4 {
+        match val.char_indices().nth(4) {
+            Some((idx, _)) => &val[..idx],
+            None => val,
+        }
+    } else {
+        ""
+    };
+    format!("{prefix}*[REDACTED]")
+}
+
 /// Scrub credentials from tool output to prevent accidental exfiltration.
-/// Replaces known credential patterns with a redacted placeholder while preserving
-/// a small prefix for context.
+///
+/// Replaces known credential patterns with a redacted placeholder while
+/// preserving a small prefix for context. Runs the key/value pattern first, then
+/// standalone well-known secret formats (AWS access keys, OpenAI `sk-` keys,
+/// `Bearer` tokens) that carry no `key: value` framing. Idempotent: re-scrubbing
+/// already-redacted text is a no-op (the redacted prefix is ≤4 chars and no
+/// pattern re-matches `*[REDACTED]`).
+///
+/// This is the single source of truth for credential scrubbing on the agent
+/// path — see the `CredentialScrubMiddleware` seam
+/// (`src/openhuman/tinyagents/middleware.rs`) which runs it over every raw tool
+/// output before summarization/caps/persistence.
 pub(crate) fn scrub_credentials(input: &str) -> String {
-    SENSITIVE_KV_REGEX
-        .replace_all(input, |caps: &regex::Captures| {
-            let full_match = &caps[0];
-            let key = &caps[1];
-            let val = caps
-                .get(2)
-                .or(caps.get(3))
-                .or(caps.get(4))
-                .map(|m| m.as_str())
-                .unwrap_or("");
-
-            // Preserve first 4 chars for context, then redact
-            let prefix = if val.chars().count() > 4 {
-                match val.char_indices().nth(4) {
-                    Some((idx, _)) => &val[..idx],
-                    None => val,
-                }
+    // 1. Key/value secrets (legacy engine behaviour). The reconstructed output
+    //    preserves the original delimiter/quoting so the model still sees a
+    //    well-formed line.
+    let kv_scrubbed = SENSITIVE_KV_REGEX.replace_all(input, |caps: &regex::Captures| {
+        let full_match = &caps[0];
+        let key = &caps[1];
+        let val = caps
+            .get(2)
+            .or(caps.get(3))
+            .or(caps.get(4))
+            .map(|m| m.as_str())
+            .unwrap_or("");
+
+        // Preserve first 4 chars for context, then redact.
+        let prefix = if val.chars().count() > 4 {
+            match val.char_indices().nth(4) {
+                Some((idx, _)) => &val[..idx],
+                None => val,
+            }
+        } else {
+            ""
+        };
+
+        if full_match.contains(':') {
+            if full_match.contains('"') {
+                format!("\"{key}\": \"{prefix}*[REDACTED]\"")
             } else {
-                ""
-            };
-
-            if full_match.contains(':') {
-                if full_match.contains('"') {
-                    format!("\"{}\": \"{}*[REDACTED]\"", key, prefix)
-                } else {
-                    format!("{}: {}*[REDACTED]", key, prefix)
-                }
-            } else if full_match.contains('=') {
-                if full_match.contains('"') {
-                    format!("{}=\"{}*[REDACTED]\"", key, prefix)
-                } else {
-                    format!("{}={}*[REDACTED]", key, prefix)
-                }
+                format!("{key}: {prefix}*[REDACTED]")
+            }
+        } else if full_match.contains('=') {
+            if full_match.contains('"') {
+                format!("{key}=\"{prefix}*[REDACTED]\"")
             } else {
-                format!("{}: {}*[REDACTED]", key, prefix)
+                format!("{key}={prefix}*[REDACTED]")
             }
+        } else {
+            format!("{key}: {prefix}*[REDACTED]")
+        }
+    });
+
+    // 2. `Bearer <token>` — redact the token, keep the scheme keyword so the
+    //    shape (`Authorization: Bearer …`) survives for the model. Runs before
+    //    the bare-token passes so a `Bearer AKIA…` is redacted as one unit.
+    let bearer_scrubbed = BEARER_TOKEN_REGEX.replace_all(&kv_scrubbed, |caps: &regex::Captures| {
+        format!("{} {}", &caps[1], redact_value(&caps[2]))
+    });
+
+    // 3. Standalone AWS access-key IDs.
+    let aws_scrubbed = AWS_ACCESS_KEY_REGEX
+        .replace_all(&bearer_scrubbed, |caps: &regex::Captures| {
+            redact_value(&caps[1])
+        });
+
+    // 4. Standalone OpenAI-style `sk-` keys.
+    OPENAI_KEY_REGEX
+        .replace_all(&aws_scrubbed, |caps: &regex::Captures| {
+            redact_value(&caps[1])
         })
         .to_string()
 }
@@ -70,4 +135,51 @@ mod tests {
         let output = scrub_credentials(input);
         assert!(output.contains("api_key: 1234*[REDACTED]"));
     }
+
+    #[test]
+    fn test_scrub_bare_aws_access_key() {
+        // No `key: value` framing — a bare AWS access-key ID in shell/env output.
+        let input = "export AWS: AKIAIOSFODNN7EXAMPLE is the id";
+        let output = scrub_credentials(input);
+        assert!(
+            output.contains("AKIA*[REDACTED]"),
+            "bare AWS key must be redacted, got: {output}"
+        );
+        assert!(!output.contains("AKIAIOSFODNN7EXAMPLE"));
+    }
+
+    #[test]
+    fn test_scrub_bare_openai_key() {
+        let input = "the key is sk-abcdefghijklmnopqrstuvwxyz012345 ok";
+        let output = scrub_credentials(input);
+        assert!(
+            output.contains("sk-a*[REDACTED]"),
+            "bare sk- key must be redacted, got: {output}"
+        );
+        assert!(!output.contains("abcdefghijklmnop"));
+    }
+
+    #[test]
+    fn test_scrub_bearer_token_space_separated() {
+        // `Authorization: Bearer <token>` — the token is space-separated from the
+        // scheme keyword, which the KV regex (needs `:`/`=`) never catches.
+        let input = "Authorization: Bearer abcdef0123456789tokenvalue";
+        let output = scrub_credentials(input);
+        assert!(
+            output.contains("Bearer abcd*[REDACTED]"),
+            "bearer token must be redacted, got: {output}"
+        );
+        assert!(!output.contains("abcdef0123456789tokenvalue"));
+    }
+
+    #[test]
+    fn test_scrub_idempotent() {
+        let input = "api_key: sk-abcdefghijklmnopqrstuvwxyz012345";
+        let once = scrub_credentials(input);
+        let twice = scrub_credentials(&once);
+        assert_eq!(
+            once, twice,
+            "scrubbing already-redacted text must be a no-op"
+        );
+    }
 }

src/openhuman/agent/harness/mod.rs

@@ -24,7 +24,7 @@
 pub mod agent_graph;
 pub mod archivist;
 pub(crate) mod builtin_definitions;
-mod credentials;
+pub(crate) mod credentials;
 pub mod definition;
 pub(crate) mod definition_loader;
 pub mod fork_context;

src/openhuman/tinyagents/middleware.rs

@@ -1119,6 +1119,78 @@ impl ToolMiddleware<()> for ApprovalSecurityMiddleware {
     }
 }
 
+/// `wrap_tool`: scrub known credential patterns from a tool's **raw** output
+/// before it enters the transcript, summarization, per-result caps, on-disk
+/// persistence, or the tool-outcome sink (issue #4453). Restores the legacy
+/// engine's `scrub_credentials` pass (v0.58.7 `engine/tools.rs`), which the
+/// tinyagents migration dropped — leaving secrets in tool output (env dumps,
+/// config reads, API responses, shell output) reaching model context (and thus
+/// third-party providers), `session_raw` transcripts, worker-thread mirrors, and
+/// [`ToolCallOutcome`](super::ToolCallOutcome) records. Violates the project rule
+/// "Never log secrets or full PII."
+///
+/// Installed as a `wrap_tool` middleware (**not** `after_tool`) deliberately: the
+/// crate agent loop runs the wrap onion to completion (`run_wrapped_tool`) and
+/// only then runs the `after_tool` chain (`run_after_tool`), so this scrub always
+/// sees the RAW tool output and always runs BEFORE summarization / caps /
+/// tokenjuice ([`ToolOutputMiddleware`]) and before the transcript push —
+/// independent of `after_tool` registration order (which #4464 reworks). Because
+/// it is installed on the shared turn harness, both the parent chat path and the
+/// sub-agent path are covered. The scrub patterns are owned by
+/// [`crate::openhuman::agent::harness::credentials`] (single source of truth).
+///
+/// Registered as the **innermost** tool-wrap layer so its post-`next` redaction
+/// runs before any outer `wrap_tool` post-processing inspects the result — e.g.
+/// so the approval gate's terminal audit row never records a raw secret from the
+/// tool's error text.
+pub(super) struct CredentialScrubMiddleware;
+
+#[async_trait]
+impl ToolMiddleware<()> for CredentialScrubMiddleware {
+    fn name(&self) -> &str {
+        "credential_scrub"
+    }
+
+    async fn wrap_tool(
+        &self,
+        ctx: &mut RunContext<()>,
+        state: &(),
+        call: TaToolCall,
+        next: ToolHandler<'_, (), ()>,
+    ) -> TaResult<MiddlewareToolOutcome> {
+        let tool_name = call.name.clone();
+        let call_id = call.id.clone();
+        let mut result = next.run(ctx, state, call).await?.into_result();
+
+        // Scrub the model-facing content. Everything downstream (transcript push,
+        // ToolOutputMiddleware summarization/caps, ToolOutcomeCaptureMiddleware
+        // sink, session_raw persistence, worker-thread mirror) derives from this
+        // field, so a single scrub here covers every surface.
+        let scrubbed =
+            crate::openhuman::agent::harness::credentials::scrub_credentials(&result.content);
+        if scrubbed != result.content {
+            tracing::debug!(
+                tool = %tool_name,
+                call_id = %call_id,
+                before_bytes = result.content.len(),
+                after_bytes = scrubbed.len(),
+                "[tinyagents::mw] credential_scrub redacted tool output"
+            );
+            result.content = scrubbed;
+        }
+
+        // The error text can echo the raw output (a tool that fails with a secret
+        // in the message), and the tool-outcome sink / model-facing error surface
+        // consume it — scrub it too.
+        if let Some(err) = result.error.take() {
+            result.error =
+                Some(crate::openhuman::agent::harness::credentials::scrub_credentials(&err));
+        }
+
+        Ok(MiddlewareToolOutcome::Result(result))
+    }
+}
+
 /// `wrap_tool`: refuse a tool whose scope is
 /// [`ToolScope::CliRpcOnly`](crate::openhuman::tools::ToolScope) inside the
 /// autonomous agent loop (issue #4249). The in-house engine ran this gate in

src/openhuman/tinyagents/mod.rs

@@ -1389,6 +1389,16 @@ fn assemble_turn_harness(
         )));
     }
 
+    // Credential scrubbing (`wrap_tool`): redact known secret patterns from every
+    // raw tool output before it enters the transcript, summarization/caps, on-disk
+    // persistence, or the tool-outcome sink (issue #4453 — restores the legacy
+    // `engine/tools.rs` scrub the tinyagents migration dropped). Registered LAST
+    // among the tool middlewares so it is the innermost wrap layer: its
+    // post-execution redaction runs before any outer `wrap_tool` post-processing
+    // (e.g. the approval-gate audit) can observe a raw secret. Installed on the
+    // shared harness, so both the parent and sub-agent paths are covered.
+    harness.push_tool_middleware(Arc::new(middleware::CredentialScrubMiddleware));
+
     // Malformed-argument recovery (`before_tool`): coerce a call's non-object
     // arguments (invalid JSON parses to Null) to `{}` so a single bad tool call is
     // recoverable — the harness would otherwise reject it against an object schema