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 // ─────────────────────────────────────────────────────────────────────────────