From fe77649548f4b3b669d7addd682adde8c5612f95 Mon Sep 17 00:00:00 2001 From: Kirill Turanskiy Date: Thu, 12 Mar 2026 03:19:25 +0300 Subject: [PATCH 1/2] refactor: move vault from global state to Server struct (C2 tech debt) Replace global vaultOnce/sharedVault/vaultInitErr with Server struct fields. Vault is still lazily initialized on first credential tool use, but now scoped to the server instance for better test isolation. --- internal/mcp/server.go | 6 ++++++ internal/mcp/tools_credential.go | 34 +++++++++++--------------------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/internal/mcp/server.go b/internal/mcp/server.go index 2286a3f9..6c09ac61 100644 --- a/internal/mcp/server.go +++ b/internal/mcp/server.go @@ -11,9 +11,12 @@ import ( "strings" "time" + "sync" + "github.com/thebtf/engram/internal/chunking" "github.com/thebtf/engram/internal/collections" "github.com/thebtf/engram/internal/consolidation" + "github.com/thebtf/engram/internal/crypto" "github.com/thebtf/engram/internal/db/gorm" "github.com/thebtf/engram/internal/embedding" graphpkg "github.com/thebtf/engram/internal/graph" @@ -47,6 +50,9 @@ type Server struct { embedSvc *embedding.Service chunkManager *chunking.Manager graphStore graphpkg.GraphStore + vault *crypto.Vault + vaultInitErr error + vaultOnce sync.Once backfillStatusFunc func() (any, error) version string } diff --git a/internal/mcp/tools_credential.go b/internal/mcp/tools_credential.go index d497541b..cdc594a3 100644 --- a/internal/mcp/tools_credential.go +++ b/internal/mcp/tools_credential.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "sync" "github.com/rs/zerolog/log" "github.com/thebtf/engram/internal/config" @@ -12,27 +11,18 @@ import ( "github.com/thebtf/engram/pkg/models" ) -// Vault singleton — initialized lazily on first credential operation. -// If initialization fails (missing key file, invalid key), the error is permanent. -// Restart the server after fixing the key configuration to retry. -// Test isolation: vault state is package-level; tests that trigger getVault() -// with failing config will poison all subsequent tests in the same binary. -var ( - sharedVault *crypto.Vault - vaultInitErr error - vaultOnce sync.Once -) - -// getVault returns the shared Vault, initializing it lazily on first call. -func getVault() (*crypto.Vault, error) { - vaultOnce.Do(func() { +// getVault returns the Server's Vault, initializing it lazily on first call. +// The vault is a singleton within the server instance; initialization errors +// are permanent — restart the server after fixing key configuration to retry. +func (s *Server) getVault() (*crypto.Vault, error) { + s.vaultOnce.Do(func() { cfg := config.Get() - sharedVault, vaultInitErr = crypto.NewVault(cfg) - if vaultInitErr != nil { - log.Error().Err(vaultInitErr).Msg("vault: failed to initialize") + s.vault, s.vaultInitErr = crypto.NewVault(cfg) + if s.vaultInitErr != nil { + log.Error().Err(s.vaultInitErr).Msg("vault: failed to initialize") } }) - return sharedVault, vaultInitErr + return s.vault, s.vaultInitErr } // handleStoreCredential encrypts and stores a credential observation. @@ -70,7 +60,7 @@ func (s *Server) handleStoreCredential(ctx context.Context, args json.RawMessage return "", fmt.Errorf("project is required for project-scoped credentials") } - v, err := getVault() + v, err := s.getVault() if err != nil { return "", fmt.Errorf("vault not available: %w", err) } @@ -143,7 +133,7 @@ func (s *Server) handleGetCredential(ctx context.Context, args json.RawMessage) return "", fmt.Errorf("name is required") } - v, err := getVault() + v, err := s.getVault() if err != nil { return "", fmt.Errorf("vault not available — configure ENGRAM_ENCRYPTION_KEY or ENGRAM_ENCRYPTION_KEY_FILE: %w", err) } @@ -286,7 +276,7 @@ func (s *Server) handleVaultStatus(ctx context.Context, _ json.RawMessage) (stri // Only load fingerprint and key source when vault is already configured (read existing key). keySource := "" if keyConfigured { - if v, err := getVault(); err == nil && v != nil { + if v, err := s.getVault(); err == nil && v != nil { fingerprint = v.Fingerprint() keySource = v.KeySource() } From 00fe5ff732f865820c57550e0cb44b122712fc50 Mon Sep 17 00:00:00 2001 From: Kirill Turanskiy Date: Thu, 12 Mar 2026 03:19:36 +0300 Subject: [PATCH 2/2] feat: activate secret detection at ingestion boundaries Wire existing privacy.RedactSecrets() into store_memory handler and hook event ingestion (handlers_ingest.go). Warn-and-redact pattern: log detection at WARN level, redact secrets before storage. Resolves TECHNICAL_DEBT.md P1: privacy/secrets.go dead code activation. --- TECHNICAL_DEBT.md | 14 ++++++++++++++ internal/mcp/tools_memory.go | 5 +++-- internal/worker/handlers_ingest.go | 11 +++++++++++ 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/TECHNICAL_DEBT.md b/TECHNICAL_DEBT.md index 05af0786..55ebbc6a 100644 --- a/TECHNICAL_DEBT.md +++ b/TECHNICAL_DEBT.md @@ -1,5 +1,19 @@ # Technical Debt +## Privacy / Secret Detection (2026-03-12) + +### ~~P1: privacy/secrets.go functions never called from production code~~ — RESOLVED 2026-03-12 +`ContainsSecrets()`, `RedactSecrets()`, and `SanitizeObservation()` in `internal/privacy/secrets.go` +were fully implemented and tested but unreachable from any production path. + +**Resolution:** +- `internal/mcp/tools_memory.go` `handleStoreMemory`: changed from hard-reject to warn-and-redact. + Content containing secrets is now logged at WARN level and redacted via `RedactSecrets()` before storage. +- `internal/worker/handlers_ingest.go` `handleIngestEvent`: secret detection added on `toolInputStr` and + `toolResultStr` immediately after stringification, before any pipeline processing. Secrets are redacted + in-memory; the raw event stored in `raw_events` may still contain original data (source-of-truth store). + + ## Credential Storage (2026-03-12) ### ~~S2: vault_status missing key_source field~~ — RESOLVED 2026-03-12 diff --git a/internal/mcp/tools_memory.go b/internal/mcp/tools_memory.go index 0ba38e4b..a456d5a9 100644 --- a/internal/mcp/tools_memory.go +++ b/internal/mcp/tools_memory.go @@ -64,9 +64,10 @@ func (s *Server) handleStoreMemory(ctx context.Context, args json.RawMessage) (s Msg("store_memory: content truncated to soft limit") } - // Reject content containing plaintext secrets — use store_credential instead. + // Redact secrets from content before storing — warn and continue rather than reject. if privacy.ContainsSecrets(params.Content) { - return "", fmt.Errorf("content appears to contain secrets (API keys, tokens, passwords). Use store_credential for secret values, not store_memory") + log.Warn().Msg("store_memory: content contains secrets — redacting before storage") + params.Content = privacy.RedactSecrets(params.Content) } // Classify observation type from content keywords when not provided. diff --git a/internal/worker/handlers_ingest.go b/internal/worker/handlers_ingest.go index 1f320222..b605aff6 100644 --- a/internal/worker/handlers_ingest.go +++ b/internal/worker/handlers_ingest.go @@ -12,6 +12,7 @@ import ( "github.com/rs/zerolog/log" "github.com/thebtf/engram/internal/pipeline" + "github.com/thebtf/engram/internal/privacy" "github.com/thebtf/engram/pkg/models" ) @@ -98,6 +99,16 @@ func (s *Service) handleIngestEvent(w http.ResponseWriter, r *http.Request) { toolInputStr := toJSONString(req.ToolInput) toolResultStr := toJSONString(req.ToolResult) + // Redact secrets from tool input/result before any pipeline processing. + if privacy.ContainsSecrets(toolInputStr) { + log.Warn().Str("tool", req.ToolName).Msg("ingest: tool_input contains secrets — redacting before pipeline processing") + toolInputStr = privacy.RedactSecrets(toolInputStr) + } + if privacy.ContainsSecrets(toolResultStr) { + log.Warn().Str("tool", req.ToolName).Msg("ingest: tool_result contains secrets — redacting before pipeline processing") + toolResultStr = privacy.RedactSecrets(toolResultStr) + } + // Filter: skip tools that should never be observed if pipeline.ShouldSkipTool(req.ToolName) { w.Header().Set("Content-Type", "application/json")