fix(a2a): redact structured error metadata - #106
Conversation
292d8b8 to
b8a1f25
Compare
625262a to
7b0e57c
Compare
* fix(flow): redact processor error events * test(flow): capture callback error events before exit * fix(platform): preserve adjacent redacted fields --------- Co-authored-by: XnLemon <xianingawa@gmail.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
agent/claudecode/claude_agent.go (1)
104-113: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAdd
api-keyto the sensitive marker list (agent/claudecode/claude_agent.go:217-225).
api_key/apikeyare covered, but common header forms likeX-Api-Key:still bypass the hook scrubber.中文
将
api-key加入敏感标记列表（agent/claudecode/claude_agent.go:217-225）。
api_key/apikey已覆盖，但像X-Api-Key:这类常见头格式仍会绕过 hook 脱敏。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/claudecode/claude_agent.go` around lines 104 - 113, Update the sensitive marker list used by the raw output hook scrubber in the relevant handler to include “api-key” alongside the existing “api_key” and “apikey” markers, preserving the current matching behavior so header forms such as “X-Api-Key:” are redacted.
🧹 Nitpick comments (2)
internal/flow/error_redaction.go (1)
13-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication with
internal/a2a/error_redaction.go.
RedactErrorTexthere andredactErrorTextininternal/a2a/error_redaction.goare functionally identical fail-closed wrappers aroundplatform.NewRedactor().Redact. Given the two live in different packages (a2a-private vs. flow-public) this is a reasonable boundary, but if a third consumer emerges, consider hoisting the wrapper intoplatformitself to avoid a third copy.中文
本文件的 `RedactErrorText` 与 `internal/a2a/error_redaction.go` 中的 `redactErrorText` 在功能上完全一致，都是对 `platform.NewRedactor().Redact` 的 fail-closed 包装。鉴于两者分属不同包（a2a 私有 vs. flow 公开），当前的边界划分是合理的；但如果未来出现第三个使用方，建议将该包装函数提升到 `platform` 包中，避免第三份重复代码。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/flow/error_redaction.go` around lines 13 - 28, The current duplication is acceptable across the separate a2a-private and flow-public package boundaries; make no change to RedactError or RedactErrorText. Only if a third consumer is introduced, move the shared fail-closed redaction wrapper into platform and update both existing callers to reuse it.agent/claudecode/claude_agent.go (1)
217-235: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding hyphenated
api-keymarker.Current markers cover
api_key/apikeybut not the hyphenatedapi-keyform, which is a common HTTP header variant (e.g.X-Api-Key:). Worth adding for parity with the other marker forms already covered.中文
当前标记覆盖了 `api_key`/`apikey`，但未覆盖带连字符的 `api-key` 形式（例如常见的 `X-Api-Key:` 头）。建议补充该变体，与其他标记形式保持一致覆盖。🔒️ Proposed addition
sensitiveMarkers := [][]byte{ []byte("authorization:"), []byte("authorization="), []byte("api_key"), []byte("apikey"), + []byte("api-key"), []byte("cookie:"), []byte("cookie="), }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agent/claudecode/claude_agent.go` around lines 217 - 235, Update sensitiveCLIOutputIndex by adding the hyphenated “api-key” marker to sensitiveMarkers, preserving the existing case-insensitive matching and earliest-marker selection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/redaction.go`:
- Around line 20-23: Update the fallback Authorization pattern in the redaction
rules so it masks the entire header value through the line ending, including
unrecognized multi-segment schemes and fields such as Credential and Signature.
Preserve the existing specialized patterns, and add a regression test covering
an unrecognized multi-part Authorization value.
---
Outside diff comments:
In `@agent/claudecode/claude_agent.go`:
- Around line 104-113: Update the sensitive marker list used by the raw output
hook scrubber in the relevant handler to include “api-key” alongside the
existing “api_key” and “apikey” markers, preserving the current matching
behavior so header forms such as “X-Api-Key:” are redacted.
---
Nitpick comments:
In `@agent/claudecode/claude_agent.go`:
- Around line 217-235: Update sensitiveCLIOutputIndex by adding the hyphenated
“api-key” marker to sensitiveMarkers, preserving the existing case-insensitive
matching and earliest-marker selection behavior.
In `@internal/flow/error_redaction.go`:
- Around line 13-28: The current duplication is acceptable across the separate
a2a-private and flow-public package boundaries; make no change to RedactError or
RedactErrorText. Only if a third consumer is introduced, move the shared
fail-closed redaction wrapper into platform and update both existing callers to
reuse it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a149c175-9387-43ce-b3dd-7db075a72e1f
📒 Files selected for processing (18)
agent/claudecode/claude_agent.gointernal/a2a/a2a.gointernal/a2a/a2a_test.gointernal/a2a/error_redaction.gointernal/flow/error_redaction.gointernal/flow/error_redaction_test.gointernal/flow/llmflow/llmflow.gointernal/flow/llmflow/llmflow_test.gointernal/flow/processor/content.gointernal/flow/processor/content_test.gointernal/flow/processor/functioncall.gointernal/flow/processor/functioncall_test.gointernal/flow/processor/transfer.gointernal/flow/processor/transfer_test.goplatform/redaction.goplatform/types_test.goserver/a2a/server.goserver/a2a/server_test.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@platform/redaction.go`:
- Around line 69-78: Update the fallback logic in the Authorization handling
block to skip re-redaction only when strings.TrimSpace(match[idx+1:]) exactly
equals "****", not when it merely has that prefix. Preserve re-redaction for
values with trailing Credential, Signature, or other fields, and add a
regression test covering a known Bearer or Basic scheme with trailing fields.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 468ed611-fa8c-4e11-b79a-3856c908c9e7
📒 Files selected for processing (4)
agent/claudecode/claude_agent.goagent/claudecode/claude_agent_test.goplatform/redaction.goplatform/types_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- platform/types_test.go
- agent/claudecode/claude_agent.go
| if strings.HasPrefix(strings.TrimSpace(lower), "authorization") { | ||
| if idx := strings.IndexAny(match, ":="); idx >= 0 { | ||
| // Specialized patterns redact known schemes first. Preserve the | ||
| // remaining line so adjacent sensitive fields are handled separately. | ||
| if strings.HasPrefix(strings.TrimSpace(match[idx+1:]), "****") { | ||
| return match | ||
| } | ||
| if match[idx] == ':' { | ||
| return match[:idx+1] + " ****" | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not preserve a redacted prefix when the Authorization value has a suffix.
For Authorization: Bearer raw-token Credential=AKIA..., the specialized matcher first produces Authorization: **** Credential=AKIA.... The fallback then returns that whole match unchanged because the remainder merely starts with ****, leaking Credential/Signature fields. Only skip re-redaction when the trimmed remainder is exactly ****, and add a regression test for a known Bearer/Basic scheme with trailing fields.
中文
当 Authorization 值仍带有后缀时，不要保留已脱敏前缀。
对于 Authorization: Bearer raw-token Credential=AKIA...，专用匹配器会先生成 Authorization: **** Credential=AKIA...。随后兜底规则因剩余内容只是以 **** 开头而原样返回整行，导致 Credential/Signature 字段泄露。只有当去空白后的剩余内容恰好等于 **** 时才应跳过再次脱敏，并补充已知 Bearer/Basic 方案带尾部字段的回归测试。
This matters because the same redacted text is surfaced through the A2A and flow error paths. As per path instructions, this Go review prioritizes semantic and security compatibility and is bilingual.
Proposed fix
- if strings.HasPrefix(strings.TrimSpace(match[idx+1:]), "****") {
+ if strings.TrimSpace(match[idx+1:]) == "****" {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if strings.HasPrefix(strings.TrimSpace(lower), "authorization") { | |
| if idx := strings.IndexAny(match, ":="); idx >= 0 { | |
| // Specialized patterns redact known schemes first. Preserve the | |
| // remaining line so adjacent sensitive fields are handled separately. | |
| if strings.HasPrefix(strings.TrimSpace(match[idx+1:]), "****") { | |
| return match | |
| } | |
| if match[idx] == ':' { | |
| return match[:idx+1] + " ****" | |
| } | |
| if idx := strings.IndexAny(match, ":="); idx >= 0 { | |
| // Specialized patterns redact known schemes first. Preserve the | |
| // remaining line so adjacent sensitive fields are handled separately. | |
| if strings.TrimSpace(match[idx+1:]) == "****" { | |
| return match | |
| } | |
| if match[idx] == ':' { | |
| return match[:idx+1] + " ****" | |
| } |
| if strings.HasPrefix(strings.TrimSpace(lower), "authorization") { | |
| if idx := strings.IndexAny(match, ":="); idx >= 0 { | |
| // Specialized patterns redact known schemes first. Preserve the | |
| // remaining line so adjacent sensitive fields are handled separately. | |
| if strings.HasPrefix(strings.TrimSpace(match[idx+1:]), "****") { | |
| return match | |
| } | |
| if match[idx] == ':' { | |
| return match[:idx+1] + " ****" | |
| } | |
| if idx := strings.IndexAny(match, ":="); idx >= 0 { | |
| // Specialized patterns redact known schemes first. Preserve the | |
| // remaining line so adjacent sensitive fields are handled separately. | |
| if strings.TrimSpace(match[idx+1:]) == "****" { | |
| return match | |
| } | |
| if match[idx] == ':' { | |
| return match[:idx+1] + " ****" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@platform/redaction.go` around lines 69 - 78, Update the fallback logic in the
Authorization handling block to skip re-redaction only when
strings.TrimSpace(match[idx+1:]) exactly equals "****", not when it merely has
that prefix. Preserve re-redaction for values with trailing Credential,
Signature, or other fields, and add a regression test covering a known Bearer or
Basic scheme with trailing fields.
Source: Path instructions
2 participants
Objective
Close the next Phase2 cross-protocol redaction gap by preventing A2A structured error metadata and task error message bodies from exposing sensitive runtime details.
Completed Changes
ResponseErrormetadata fields written throughWithResponseErrorMetadata, includingType,Message,Code, andParam.ResponseErrorFromMetadata.Status.Message.Partsso the response body cannot bypass redacted metadata.Validation
go test ./internal/a2a -count=1go test ./server/a2a -count=1 -run 'TestBuildTaskErrorMetadata|TestBuildTaskErrorMessage|TestMessageProcessor_ProcessMessage_StructuredTaskError|TestMessageProcessor_ProcessBatchStreamingEvents_StructuredTaskError|TestMessageProcessor_ProcessBatchStreamingEvents_StopAgentStructuredTaskError|Test.*ResponseError|Test.*StructuredError|Test.*ErrorMetadata|Test.*FinalArtifact|Test.*FailedTask'go test ./agent/a2aagent -count=1 -run 'TestDefaultA2AEventConverter_ConvertStreamingToEvents_FailedStatus|TestDefaultA2AEventConverter_ConvertStreamingToEvents_CompletedStatusStructuredError|TestDefaultA2AEventConverter_ConvertStreamingToEvents_FinalArtifactStructuredError|TestDefaultA2AEventConverter_ConvertToEvents_FailedTask|TestA2AAgent'go vet ./internal/a2a ./server/a2a ./agent/a2aagentgit diff --check0 High / 0 Medium / 0 Lowafter fixing the A2AStatus.Message.Partsbypass.Known Risks / Limitations
Follow-up Work