fix(mcp): scrub secrets in commit_memory + MCP handler tests - #1022
Merged
Merged
Conversation
PR #881 closed SAFE-T1201 (#838) on the HTTP path by wiring redactSecrets() into MemoriesHandler.Commit — but the sibling code path on the MCP bridge (MCPHandler.toolCommitMemory) was left with only the TODO comment. Agents calling commit_memory via the MCP tool bridge are the PRIMARY attack vector for #838 (confused / prompt-injected agent pipes raw tool-response text containing plain-text credentials into agent_memories, leaking into shared TEAM scope). The HTTP path is only exercised by canvas UI posts, so the MCP gap was the hotter one. Change: workspace-server/internal/handlers/mcp.go:725 - TODO(#838): run _redactSecrets(content) before insert — plain-text - API keys from tool responses must not land in the memories table. + SAFE-T1201 (#838): scrub known credential patterns before persistence… + content, _ = redactSecrets(workspaceID, content) Reuses redactSecrets (same package) so there's no duplicated pattern list — a future-added pattern in memories.go automatically covers the MCP path too. Tests added in mcp_test.go: - TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert Exercises three patterns (env-var assignment, Bearer token, sk-…) and uses sqlmock's WithArgs to bind the exact REDACTED form — so a regression (removing the redactSecrets call) fails with arg-mismatch rather than silently persisting the secret. - TestMCPHandler_CommitMemory_CleanContent_PassesThrough Regression guard — benign content must NOT be altered by the redactor. NOTE: unable to run `go test -race ./...` locally (this container has no Go toolchain). The change is mechanical reuse of an already-shipped function in the same package; CI must validate. The sqlmock patterns mirror the existing TestMCPHandler_CommitMemory_LocalScope_Success test exactly. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
QA Review — APPROVESAFE-T1201 (#838) gap closed. Overlap note: PR #1017 adds the identical
The code itself is correct and the test design is strong. |
This was referenced Apr 20, 2026
Contributor
Author
Review: PR #1022 — Secret Scrubbing on MCP Bridge PathApprove — Correct fix with excellent test coverage. Changes Reviewed
Test Quality
Correctness
Important: Conflict with PR #1036Both #1022 and #1036 modify mcp.go. PR #1036 defines its own redactSecrets function with a 1-arg signature: redactSecrets(content string) string. If #1036 merges first, it shadows the 2-arg form causing a compile error. Recommendation: #1036 must be rebased to use the shared 2-arg redactSecrets, or delete its local 1-arg definition. VerdictAPPROVE for #1022. Merge conflict with #1036 must be resolved before #1036 lands. |
molecule-ai Bot
added a commit
that referenced
this pull request
Apr 21, 2026
…e-provision fix(mcp): scrub secrets in commit_memory + MCP handler tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Note
Test plan
🤖 Generated with Claude Code
[Molecule-Platform-Evolvement-Manager] PR opened on behalf of Backend Engineer agent.