feat(mcp): redact secret values from exec output#357
Conversation
Scan stdout/stderr from the exec tool for resolved secret values and replace them with [REDACTED] before returning to the agent. This prevents agents from exfiltrating secrets via commands like `printenv`. Enabled by default; disable with `mcp.redact_output = false`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable security feature to redact secret values from the exec tool's output in the MCP server. The changes are well-structured, with updates to configuration, documentation, and the server logic. The implementation of the redaction function redact_secrets has a potential correctness bug and performance bottleneck. I've provided a detailed comment with a suggested fix using the aho-corasick crate, which would make the implementation more robust and efficient. The addition of unit tests is great, but they would benefit from covering the edge case I've pointed out.
| fn redact_secrets(text: &str, secret_values: &[(String, String)]) -> String { | ||
| let mut values: Vec<&str> = secret_values | ||
| .iter() | ||
| .map(|(_, v)| v.as_str()) | ||
| .filter(|v| !v.trim().is_empty()) | ||
| .collect(); | ||
|
|
||
| if values.is_empty() { | ||
| return text.to_string(); | ||
| } | ||
|
|
||
| values.sort_unstable(); | ||
| values.dedup(); | ||
| // Sort longest first for greedy replacement | ||
| values.sort_by_key(|v| std::cmp::Reverse(v.len())); | ||
|
|
||
| let mut result = text.to_string(); | ||
| for secret in &values { | ||
| result = result.replace(secret, "[REDACTED]"); | ||
| } | ||
| result | ||
| } |
There was a problem hiding this comment.
The current implementation of redact_secrets has a correctness issue and a performance issue.
Correctness Bug:
If a secret happens to be a substring of the replacement text "[REDACTED]" (e.g., DACT), the redaction will be applied incorrectly. The loop iterates and replaces secrets one by one on the evolving result string. This means already redacted parts can be re-redacted.
For example, with secrets secret and DACT, redacting "a secret" would produce "a [REDACTED]", which would then be re-scanned for DACT, resulting in "a [RE[REDACTED]ED]".
Performance:
The implementation iterates through each secret and calls String::replace on the entire output. This can be very inefficient for large outputs or a large number of secrets, as it results in multiple passes over the output string and repeated string allocations.
A more correct and performant approach is to use the Aho-Corasick algorithm, which is designed for multi-pattern string matching in a single pass. The aho-corasick crate is an excellent implementation for this. It will solve both the correctness and performance issues. You would need to add aho-corasick to your Cargo.toml (e.g., aho-corasick = "1.1").
fn redact_secrets(text: &str, secret_values: &[(String, String)]) -> String {
// This requires adding the `aho-corasick` crate to your dependencies.
let values: Vec<&str> = secret_values
.iter()
.map(|(_, v)| v.as_str())
.filter(|v| !v.trim().is_empty())
.collect();
if values.is_empty() {
return text.to_string();
}
// Using Aho-Corasick is more efficient and correct than repeated `replace`.
// It avoids re-scanning the replacement string, and handles overlapping
// matches correctly. `LeftmostLongest` ensures that if "abc" and "abcdef"
// are secrets, "abcdef" is matched.
let ac = match aho_corasick::AhoCorasick::builder()
.match_kind(aho_corasick::MatchKind::LeftmostLongest)
.build(&values)
{
Ok(ac) => ac,
Err(_) => {
// This should not happen with valid utf-8 strings.
// Return original text as a safe fallback.
return text.to_string();
}
};
ac.replace_all(text, &vec!["[REDACTED]"; values.len()])
}
Greptile SummaryThis PR hardens the MCP
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant FnoxMcpServer
participant Subprocess
participant redact_secrets
Agent->>FnoxMcpServer: exec(command)
FnoxMcpServer->>FnoxMcpServer: ensure_resolved() — batch-resolve env=true secrets
FnoxMcpServer->>FnoxMcpServer: build env_vars Vec<(name, raw_value)>
FnoxMcpServer->>Subprocess: spawn(command, env=env_vars)
Subprocess-->>FnoxMcpServer: stdout_buf + stderr_buf
alt mcp_config.redact_output() == true
FnoxMcpServer->>redact_secrets: redact_secrets(stdout_raw, env_vars)
Note over redact_secrets: trim values, skip len < MIN_REDACT_LENGTH(3)<br/>build AhoCorasick (LeftmostLongest)<br/>replace_all → "[REDACTED]"
redact_secrets-->>FnoxMcpServer: redacted stdout
FnoxMcpServer->>redact_secrets: redact_secrets(stderr_raw, env_vars)
Note over redact_secrets: automaton rebuilt from same patterns
redact_secrets-->>FnoxMcpServer: redacted stderr
else redact_output == false
FnoxMcpServer->>FnoxMcpServer: stdout_raw / stderr_raw as-is
end
FnoxMcpServer-->>Agent: CallToolResult(redacted stdout + stderr)
|
Switch from sequential String::replace to Aho-Corasick with leftmost-longest matching. This avoids corrupting [REDACTED] markers when a secret value is a substring of the replacement text. Also skip secrets shorter than 3 characters to avoid false-positive redaction that corrupts output readability. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Trim whitespace from secret values before using them as both the length guard and the Aho-Corasick pattern. Prevents trailing newlines (common when secrets are loaded from files) from causing redaction misses when the output contains the value without the newline. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Change redact_secrets to return Result<String, McpError> so that an Aho-Corasick build failure propagates as an MCP error instead of silently returning unredacted output. The exec tool now returns an internal error rather than leaking secrets if the automaton cannot be constructed. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
### 🚀 Features - **(sync)** add --local-file output target by [@florian-lackner365](https://github.com/florian-lackner365) in [#317](#317) ### 🐛 Bug Fixes - properly handle auth prompt in batch providers by [@johnpyp](https://github.com/johnpyp) in [#349](#349) ### 🛡️ Security - **(mcp)** redact secret values from exec output by [@jdx](https://github.com/jdx) in [#357](#357) ### 📦️ Dependency Updates - lock file maintenance by [@renovate[bot]](https://github.com/renovate[bot]) in [#344](#344) - update jdx/mise-action digest to 5228313 by [@renovate[bot]](https://github.com/renovate[bot]) in [#351](#351) - update swatinem/rust-cache digest to e18b497 by [@renovate[bot]](https://github.com/renovate[bot]) in [#352](#352) - update taiki-e/upload-rust-binary-action digest to 381995c by [@renovate[bot]](https://github.com/renovate[bot]) in [#353](#353) - update dependency vue to v3.5.30 by [@renovate[bot]](https://github.com/renovate[bot]) in [#354](#354) - update rust crate openssl-sys to v0.9.112 by [@renovate[bot]](https://github.com/renovate[bot]) in [#355](#355) - update rust crate clap to v4.6.0 by [@renovate[bot]](https://github.com/renovate[bot]) in [#356](#356) ### New Contributors - @florian-lackner365 made their first contribution in [#317](#317) --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
exectool for resolved secret values and replaces them with[REDACTED]before returning output to the agentprintenvorecho $SECRETmcp.redact_output = falseCloses discussion #350
Test plan
redact_secretsfunction (5 tests, all passing)fnox mcp, use exec to runprintenvand verify values are redactedmcp.redact_output = falseand verify raw values are returned🤖 Generated with Claude Code
Note
Medium Risk
Changes MCP
execbehavior to post-process stdout/stderr and hide resolved secret values by default, which could affect tooling that expects exact output and carries some risk of false-positive/negative redaction.Overview
Prevents secret exfiltration via MCP
execoutput by scanning captured stdout/stderr for resolved secret values and replacing matches with[REDACTED]before returning results to the agent.Adds configurable control via
mcp.redact_output(default true), wires it through config merging and the published JSON schema, and documents the new security behavior and limitations. Implements redaction usingaho-corasick(leftmost-longest) and adds unit tests covering overlap/length/empty handling.Written by Cursor Bugbot for commit a1b1b52. This will update automatically on new commits. Configure here.