From 818d5cde91c92abb1378ed69bbbf5fa5cf8c702a Mon Sep 17 00:00:00 2001 From: Molecule AI Backend Engineer Date: Sun, 19 Apr 2026 17:52:52 +0000 Subject: [PATCH] fix(mcp): scrub secrets in commit_memory MCP tool path (#838 sibling) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- workspace-server/internal/handlers/mcp.go | 8 +- .../internal/handlers/mcp_test.go | 95 +++++++++++++++++++ 2 files changed, 101 insertions(+), 2 deletions(-) diff --git a/workspace-server/internal/handlers/mcp.go b/workspace-server/internal/handlers/mcp.go index c10976ed..abaa10c2 100644 --- a/workspace-server/internal/handlers/mcp.go +++ b/workspace-server/internal/handlers/mcp.go @@ -723,8 +723,12 @@ func (h *MCPHandler) toolCommitMemory(ctx context.Context, workspaceID string, a } memoryID := uuid.New().String() - // 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 so + // plain-text API keys pulled in via tool responses can't land in the + // memories table (and leak into shared TEAM scope). Reuses redactSecrets + // already shipped for the HTTP path in PR #881 — this was the MCP-bridge + // sibling the original fix missed. Runs on every write regardless of scope. + content, _ = redactSecrets(workspaceID, content) _, err := h.database.ExecContext(ctx, ` INSERT INTO agent_memories (id, workspace_id, content, scope, namespace) VALUES ($1, $2, $3, $4, $5) diff --git a/workspace-server/internal/handlers/mcp_test.go b/workspace-server/internal/handlers/mcp_test.go index 9f380048..c91bf98f 100644 --- a/workspace-server/internal/handlers/mcp_test.go +++ b/workspace-server/internal/handlers/mcp_test.go @@ -433,6 +433,101 @@ func TestMCPHandler_CommitMemory_GlobalScope_Blocked(t *testing.T) { } } +// TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert verifies +// the SAFE-T1201 (#838) fix on the MCP bridge path. PR #881 closed the HTTP +// handler but missed this one — an agent tool-call carrying plain-text +// credentials must have them scrubbed before the INSERT reaches the DB. +// +// The test asserts via the sqlmock `WithArgs` matcher that the content column +// binds the REDACTED form, not the raw input. sqlmock verifies the exact arg +// values, so a regression (removing the redactSecrets call) would fail with +// "argument mismatch" rather than silently persisting the secret. +func TestMCPHandler_CommitMemory_SecretInContent_IsRedactedBeforeInsert(t *testing.T) { + h, mock := newMCPHandler(t) + + // Content with three distinct secret patterns covered by redactSecrets: + // - env-var assignment (ANTHROPIC_API_KEY=) + // - Bearer token + // - sk-… prefixed key + rawContent := "key=ANTHROPIC_API_KEY=sk-ant-xxxxxxxxxxxxxxxx auth=Bearer ghp_yyyyyyyyyyyyy note=sk-proj-zzzzzzzzzzzzzzzzzzzz" + + // Derive what redactSecrets will produce so the sqlmock arg match is + // exact. This keeps the test brittle-on-purpose: if redactSecrets's + // output shape changes, this test must be re-derived, which surfaces + // the change during review. + expected, changed := redactSecrets("ws-1", rawContent) + if !changed { + t.Fatalf("precondition failed — redactSecrets must change the test content; got unchanged %q", expected) + } + if bytes.Contains([]byte(expected), []byte("sk-ant-xxxxxxxxxxxxxxxx")) { + t.Fatalf("precondition failed — redacted content still contains raw secret: %s", expected) + } + + mock.ExpectExec("INSERT INTO agent_memories"). + WithArgs(sqlmock.AnyArg(), "ws-1", expected, "LOCAL", "ws-1"). + WillReturnResult(sqlmock.NewResult(1, 1)) + + w := mcpPost(t, h, "ws-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 99, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "commit_memory", + "arguments": map[string]interface{}{ + "content": rawContent, + "scope": "LOCAL", + }, + }, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + var resp mcpResponse + json.Unmarshal(w.Body.Bytes(), &resp) + if resp.Error != nil { + t.Fatalf("unexpected JSON-RPC error: %+v", resp.Error) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("sqlmock mismatch — content was NOT redacted before insert: %v", err) + } +} + +// TestMCPHandler_CommitMemory_CleanContent_PassesThrough confirms that the +// redactor is a no-op on content with no credentials — a regression where +// redactSecrets corrupted benign content would be a user-visible bug. +func TestMCPHandler_CommitMemory_CleanContent_PassesThrough(t *testing.T) { + h, mock := newMCPHandler(t) + + cleanContent := "the quick brown fox jumps over the lazy dog — no secrets here" + + // Bind the exact string — no wildcards — so that any transformation + // (whitespace, case, truncation) would fail the arg match. + mock.ExpectExec("INSERT INTO agent_memories"). + WithArgs(sqlmock.AnyArg(), "ws-1", cleanContent, "TEAM", "ws-1"). + WillReturnResult(sqlmock.NewResult(1, 1)) + + w := mcpPost(t, h, "ws-1", map[string]interface{}{ + "jsonrpc": "2.0", + "id": 100, + "method": "tools/call", + "params": map[string]interface{}{ + "name": "commit_memory", + "arguments": map[string]interface{}{ + "content": cleanContent, + "scope": "TEAM", + }, + }, + }) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", w.Code, w.Body.String()) + } + if err := mock.ExpectationsWereMet(); err != nil { + t.Errorf("clean content should pass through unchanged: %v", err) + } +} + // ───────────────────────────────────────────────────────────────────────────── // tools/call — recall_memory // ─────────────────────────────────────────────────────────────────────────────