Skip to content

fix(acp): redact secret-shaped env values from wire logs and observer feeds - #2869

Open
jatinder14 wants to merge 2 commits into
block:mainfrom
jatinder14:fix/acp-redact-secret-shaped-env-logs
Open

fix(acp): redact secret-shaped env values from wire logs and observer feeds#2869
jatinder14 wants to merge 2 commits into
block:mainfrom
jatinder14:fix/acp-redact-secret-shaped-env-logs

Conversation

@jatinder14

@jatinder14 jatinder14 commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Redact secret-shaped keys (*_API_KEY, *_TOKEN, *_SECRET, camelCase apiKey, ACP {name,value} env pairs, …) before acp::wire debug logs and observer acp_read/acp_write emit.
  • Agent stdin/stdout is unchanged; only logging/observer copies are scrubbed.
  • Adds unit tests covering map env, ACP env arrays, and wire-line redaction.

Fixes #2819

Test plan

  • cargo test -p buzz-acp --lib redact::
  • Spot-check: with RUST_LOG=acp::wire=debug, trigger an agent notification containing MCP env and confirm keys land as [REDACTED] in logs/Raw Event rail

… feeds

Agents can rebroadcast MCP configs (including plaintext API keys) in custom
notifications. Scrub secret-shaped keys before acp::wire debug logs and
observer emit so traces/transcripts never persist those values. Agent
stdin/stdout is unchanged.

Closes block#2819

Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Jatinder Mahajan <jatinder.mahajan@certifyos.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@jatinder14
jatinder14 force-pushed the fix/acp-redact-secret-shaped-env-logs branch from c8ca7dd to d44334b Compare July 25, 2026 17:57
@maikunari

Copy link
Copy Markdown

Reporter here — thanks for the fast pickup. Reviewed against the actual payload I observed: the array-form env: [{name, value}] handling matches what grok emits in _x.ai/mcp/servers_updated, and the sink coverage (observer + both wire directions + parse-error paths) looks complete for this crate. Two points:

  1. Suggest redacting all values inside mcpServers[].env, unconditionally — the secret-shaped heuristic misses credentials in non-secret-named variables (DATABASE_URL=postgres://user:password@host, AUTH_HEADER, etc.). Env values are agent config, never display data, so there's no cost to blanket redaction within that path; keep the heuristic for generic JSON elsewhere.
  2. The original report also mentioned desktop-side logging — if the managed-agent runtime logs raw notifications anywhere in desktop/src-tauri, that path isn't covered by this PR (fine as a follow-up, just noting scope).

With point 1 addressed I'd consider this a complete fix for the buzz-acp half. 🐝

@jatinder14

jatinder14 commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for checking this against the real _x.ai/mcp/servers_updated payload, @maikunari. Glad the array-form env: [{name, value}] path matches what you saw. Appreciate the review.

MCP server environment values are agent configuration, so redact them
regardless of their variable names while retaining heuristic redaction for
generic JSON.

Signed-off-by: Jatinder Mahajan <jatinder.mahajan@certifyos.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@jatinder14

jatinder14 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Thanks @maikunari. Updated in ad50de0.

mcpServers[].env values are now always redacted for both object maps and {name, value} arrays, including names like DATABASE_URL. Elsewhere we still only redact secret-shaped keys. Added a test for the blanket MCP env case, and confirmed generic env values outside that path are unchanged.

Agreed on desktop-side logging. That managed-agent path sits outside buzz-acp, so I will leave it as a follow-up.

@jatinder14
jatinder14 force-pushed the fix/acp-redact-secret-shaped-env-logs branch from ad50de0 to c2e0f1e Compare July 28, 2026 02:44
@Bartok9

Bartok9 commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Independent review (Bartok9) — 2026-07-30

Verdict

Solid defensive fix for #2819. Prefer landing this over a parallel stack.

What I checked

  • Wire path still talks to the agent unmutated; only acp::wire debug lines + observer payloads are scrubbed — correct boundary.
  • Dual shape coverage: secret-shaped keys (apiKey, *_TOKEN, …) and full MCP env under mcpServers (handles Grok-style {name,value} pairs and map envs / DATABASE_URL). Matches the failure mode where agents rebroadcast MCP config.
  • Parse-error observe lines also redact the raw line — nice; avoids side channel.
  • Unit tests pin the interesting cases (incl. keyboard false positive avoidance).

Non-blockers

  • Branch may want a rebase onto latest main before merge (hygiene).
  • Optional later: shared helper with desktop’s split_config_key if they ever drift (comment already notes the mirror).

Recommend merge. Thanks @jatinder14 — this is the right layer for the defensive half of #2819.

@jatinder14

Copy link
Copy Markdown
Contributor Author

Thanks @Bartok9 for the thorough independent review. Glad the wire/observer boundary and the blanket mcpServers env redaction look right.

I'll rebase onto latest main for hygiene. Agreed that sharing a helper with desktop's split_config_key can be a follow-up if those paths start to drift.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

buzz-acp: scrub env values from agent notification logging

3 participants