From 8ec966c81dee4e9b36f8ccd66bd3ef5e7fbfab16 Mon Sep 17 00:00:00 2001 From: Thoxvi Yuan Date: Fri, 3 Jul 2026 13:21:55 +0800 Subject: [PATCH 1/2] [fix]: sanitize ErrorDetailsParsed so raw payloads honor disable_content_logging The logging plugin serialized a sanitized copy of the provider error into entry.ErrorDetails but then stored the original, unsanitized error in entry.ErrorDetailsParsed. logstore's SerializeFields (run from the BeforeCreate GORM hook on every insert) re-serializes ErrorDetailsParsed and overwrites ErrorDetails, so the sanitization was defeated: with content logging or raw storage disabled, error rows still persisted the full raw request/response payloads attached to the error. The queue entry also pinned those payloads in memory until flush. Store the sanitized error in both fields via a shared helper (applyErrorDetailsToEntry) at every direct assignment site, including the streaming-output path and the MCP tool log path. Behavior with content logging + raw storage enabled is unchanged (the original error is passed through). Note: updateLogEntry's UpdateLogData.ErrorDetails path is not covered here because shouldStoreRaw is not in scope there; flagged in the PR. Affected packages/files: - plugins/logging/main.go - plugins/logging/operations.go - plugins/logging/sanitize_test.go - plugins/logging/changelog.md --- plugins/logging/changelog.md | 1 + plugins/logging/main.go | 47 ++++++++++--------- plugins/logging/operations.go | 9 ++-- plugins/logging/sanitize_test.go | 80 ++++++++++++++++++++++++++++++++ 4 files changed, 109 insertions(+), 28 deletions(-) create mode 100644 plugins/logging/sanitize_test.go diff --git a/plugins/logging/changelog.md b/plugins/logging/changelog.md index e69de29bb2d..7575cf3232f 100644 --- a/plugins/logging/changelog.md +++ b/plugins/logging/changelog.md @@ -0,0 +1 @@ +[fix]: sanitize ErrorDetailsParsed so raw payloads honor disable_content_logging [@citrocat](https://github.com/citrocat) diff --git a/plugins/logging/main.go b/plugins/logging/main.go index be9fbf000d3..65622098ed6 100644 --- a/plugins/logging/main.go +++ b/plugins/logging/main.go @@ -108,6 +108,24 @@ func applyLargePayloadPreviewsToEntry(ctx *schemas.BifrostContext, entry *logsto } } +// applyErrorDetailsToEntry stores the sanitized error on the entry. Both the +// serialized string and the parsed struct must hold the sanitized copy: +// logstore's SerializeFields re-serializes ErrorDetailsParsed on write (it +// takes precedence over ErrorDetails), so an unsanitized parsed struct would +// leak raw request/response payloads to the store even when content logging +// is disabled. Serialization happens immediately since bifrostErr may be +// released back to the pool before the async batch writer processes the entry. +func applyErrorDetailsToEntry(entry *logstore.Log, bifrostErr *schemas.BifrostError, contentLoggingEnabled, shouldStoreRaw bool) { + if bifrostErr == nil { + return + } + sanitizedErr := sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw) + if data, err := sonic.Marshal(sanitizedErr); err == nil { + entry.ErrorDetails = string(data) + } + entry.ErrorDetailsParsed = sanitizedErr +} + // sanitizeErrorForLogging returns a shallow copy of err with ExtraFields.RawRequest and // RawResponse cleared when raw-byte persistence is disabled, preventing raw bytes from // leaking into entry.ErrorDetails via JSON serialization. @@ -921,10 +939,7 @@ func (p *LoggerPlugin) PostLLMHook(ctx *schemas.BifrostContext, result *schemas. } applyModelAlias(entry, originalModelRequested, resolvedModelUsed) applyResolvedAliasInfo(entry, resolvedKeyAlias) - if data, err := sonic.Marshal(sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw)); err == nil { - entry.ErrorDetails = string(data) - } - entry.ErrorDetailsParsed = bifrostErr + applyErrorDetailsToEntry(entry, bifrostErr, contentLoggingEnabled, shouldStoreRaw) if nodeID, _ := p.clusterNodeID.Load().(string); nodeID != "" { entry.ClusterNodeID = &nodeID } @@ -1056,13 +1071,7 @@ func (p *LoggerPlugin) PostLLMHook(ctx *schemas.BifrostContext, result *schemas. tracer.CleanupStreamAccumulator(traceID) } - // Serialize error details immediately since bifrostErr may be released - // back to the pool before the async batch writer processes this entry. - // Also set ErrorDetailsParsed for UI callback (JSON serialization uses this field). - if data, err := sonic.Marshal(sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw)); err == nil { - entry.ErrorDetails = string(data) - } - entry.ErrorDetailsParsed = bifrostErr + applyErrorDetailsToEntry(entry, bifrostErr, contentLoggingEnabled, shouldStoreRaw) if shouldStoreRaw && contentLoggingEnabled { if bifrostErr.ExtraFields.RawRequest != nil { rawReqBytes, err := sonic.Marshal(bifrostErr.ExtraFields.RawRequest) @@ -1103,10 +1112,7 @@ func (p *LoggerPlugin) PostLLMHook(ctx *schemas.BifrostContext, result *schemas. entry.Status = logStatusForError(bifrostErr) entry.Stream = true applyModelAlias(entry, originalModelRequested, resolvedModelUsed) - if data, err := sonic.Marshal(sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw)); err == nil { - entry.ErrorDetails = string(data) - } - entry.ErrorDetailsParsed = bifrostErr + applyErrorDetailsToEntry(entry, bifrostErr, contentLoggingEnabled, shouldStoreRaw) // Backfill raw request/response on streaming-error path so cancellation/timeout // log entries still carry raw payloads when content logging + raw storage are // enabled. Mirrors the non-streaming Path A pattern at line 872. Prefer the @@ -1185,13 +1191,7 @@ func (p *LoggerPlugin) PostLLMHook(ctx *schemas.BifrostContext, result *schemas. if bifrostErr != nil { entry.Status = logStatusForError(bifrostErr) applyModelAlias(entry, originalModelRequested, resolvedModelUsed) - // Serialize error details immediately since bifrostErr may be released - // back to the pool before the async batch writer processes this entry. - // Also set ErrorDetailsParsed for UI callback (JSON serialization uses this field). - if data, err := sonic.Marshal(sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw)); err == nil { - entry.ErrorDetails = string(data) - } - entry.ErrorDetailsParsed = bifrostErr + applyErrorDetailsToEntry(entry, bifrostErr, contentLoggingEnabled, shouldStoreRaw) // Realtime turns that fail mid-stream still need their input transcript // surfaced — backfill from bifrostErr.ExtraFields.RawRequest if present. if requestType == schemas.RealtimeRequest { @@ -1595,7 +1595,8 @@ func (p *LoggerPlugin) PostMCPHook(ctx *schemas.BifrostContext, resp *schemas.Bi if bifrostErr != nil { entry.Status = "error" - entry.ErrorDetailsParsed = bifrostErr + shouldStoreRaw, _ := ctx.Value(schemas.BifrostContextKeyShouldStoreRawInLogs).(bool) + entry.ErrorDetailsParsed = sanitizeErrorForLogging(bifrostErr, p.contentLoggingEnabled(ctx), shouldStoreRaw) } else if resp != nil { entry.Status = "success" if p.contentLoggingEnabled(ctx) { diff --git a/plugins/logging/operations.go b/plugins/logging/operations.go index 29b8cc4c64d..606858874e2 100644 --- a/plugins/logging/operations.go +++ b/plugins/logging/operations.go @@ -386,11 +386,10 @@ func (p *LoggerPlugin) applyStreamingOutputToEntry(entry *logstore.Log, streamRe // Handle error case first if streamResponse.Data.ErrorDetails != nil { entry.Status = logStatusForError(streamResponse.Data.ErrorDetails) - entry.ErrorDetailsParsed = streamResponse.Data.ErrorDetails - // Serialize error details immediately to avoid use-after-free with pooled errors - if data, err := sonic.Marshal(streamResponse.Data.ErrorDetails); err == nil { - entry.ErrorDetails = string(data) - } + // Serializes immediately to avoid use-after-free with pooled errors, and + // stores the sanitized copy in both fields (SerializeFields re-serializes + // ErrorDetailsParsed on write, so it must not hold raw payloads). + applyErrorDetailsToEntry(entry, streamResponse.Data.ErrorDetails, contentLoggingEnabled, shouldStoreRaw) latF := float64(streamResponse.Data.Latency) entry.Latency = &latF } else { diff --git a/plugins/logging/sanitize_test.go b/plugins/logging/sanitize_test.go new file mode 100644 index 00000000000..e60f720c0b6 --- /dev/null +++ b/plugins/logging/sanitize_test.go @@ -0,0 +1,80 @@ +package logging + +import ( + "strings" + "testing" + + "github.com/maximhq/bifrost/core/schemas" + "github.com/maximhq/bifrost/framework/logstore" +) + +func errorWithRawPayloads() *schemas.BifrostError { + return &schemas.BifrostError{ + IsBifrostError: false, + Error: &schemas.ErrorField{Message: "provider rejected request"}, + ExtraFields: schemas.BifrostErrorExtraFields{ + RawRequest: map[string]any{"messages": "RAW_REQUEST_MARKER"}, + RawResponse: map[string]any{"body": "RAW_RESPONSE_MARKER"}, + }, + } +} + +// Regression test: logstore's SerializeFields re-serializes ErrorDetailsParsed +// on write, overwriting ErrorDetails. If the parsed field holds the +// unsanitized error, raw request/response payloads reach the store even when +// content logging is disabled. +func TestApplyErrorDetailsToEntry_SanitizedSurvivesSerializeFields(t *testing.T) { + entry := &logstore.Log{ID: "req-1"} + applyErrorDetailsToEntry(entry, errorWithRawPayloads(), false, false) + + if entry.ErrorDetailsParsed == nil { + t.Fatal("ErrorDetailsParsed should be set") + } + if entry.ErrorDetailsParsed.ExtraFields.RawRequest != nil || + entry.ErrorDetailsParsed.ExtraFields.RawResponse != nil { + t.Error("ErrorDetailsParsed should not retain raw payloads when content logging is disabled") + } + + // Simulate the DB write path (BeforeCreate calls SerializeFields). + if err := entry.SerializeFields(); err != nil { + t.Fatalf("SerializeFields() error: %v", err) + } + if strings.Contains(entry.ErrorDetails, "RAW_REQUEST_MARKER") || + strings.Contains(entry.ErrorDetails, "RAW_RESPONSE_MARKER") { + t.Error("serialized ErrorDetails must not contain raw payloads when content logging is disabled") + } + if !strings.Contains(entry.ErrorDetails, "provider rejected request") { + t.Error("serialized ErrorDetails should still contain the error message") + } +} + +// When content logging and raw storage are both enabled, raw payloads are +// intentionally preserved. +func TestApplyErrorDetailsToEntry_RawPreservedWhenEnabled(t *testing.T) { + entry := &logstore.Log{ID: "req-2"} + applyErrorDetailsToEntry(entry, errorWithRawPayloads(), true, true) + + if entry.ErrorDetailsParsed == nil { + t.Fatal("ErrorDetailsParsed should be set") + } + if entry.ErrorDetailsParsed.ExtraFields.RawRequest == nil { + t.Error("raw payloads should be preserved when content logging and raw storage are enabled") + } + if err := entry.SerializeFields(); err != nil { + t.Fatalf("SerializeFields() error: %v", err) + } + if !strings.Contains(entry.ErrorDetails, "RAW_REQUEST_MARKER") { + t.Error("serialized ErrorDetails should contain raw payloads when explicitly enabled") + } +} + +func TestApplyErrorDetailsToEntry_NilError(t *testing.T) { + entry := &logstore.Log{ID: "req-3"} + applyErrorDetailsToEntry(entry, nil, false, false) + if entry.ErrorDetailsParsed != nil { + t.Error("nil error should leave ErrorDetailsParsed nil") + } + if entry.ErrorDetails != "" { + t.Error("nil error should leave ErrorDetails empty") + } +} From 90723977917dbf7240a0d8207864b59911dea8b5 Mon Sep 17 00:00:00 2001 From: Thoxvi Yuan Date: Fri, 3 Jul 2026 13:56:28 +0800 Subject: [PATCH 2/2] [fix]: serialize MCP error details immediately via parallel helper Adds applyErrorDetailsToMCPEntry (MCPToolLog counterpart of applyErrorDetailsToEntry) so the MCP error path serializes the sanitized error into ErrorDetails at assignment time instead of deferring entirely to the BeforeCreate hook, matching the LLM paths' pool-safety pattern. Per review feedback. Affected packages/files: - plugins/logging/main.go - plugins/logging/sanitize_test.go --- plugins/logging/main.go | 16 +++++++++++++++- plugins/logging/sanitize_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/plugins/logging/main.go b/plugins/logging/main.go index 65622098ed6..ee1603db463 100644 --- a/plugins/logging/main.go +++ b/plugins/logging/main.go @@ -126,6 +126,20 @@ func applyErrorDetailsToEntry(entry *logstore.Log, bifrostErr *schemas.BifrostEr entry.ErrorDetailsParsed = sanitizedErr } +// applyErrorDetailsToMCPEntry is the MCPToolLog counterpart of +// applyErrorDetailsToEntry: same sanitize-once, serialize-immediately, +// store-sanitized-copy-in-both-fields semantics. +func applyErrorDetailsToMCPEntry(entry *logstore.MCPToolLog, bifrostErr *schemas.BifrostError, contentLoggingEnabled, shouldStoreRaw bool) { + if bifrostErr == nil { + return + } + sanitizedErr := sanitizeErrorForLogging(bifrostErr, contentLoggingEnabled, shouldStoreRaw) + if data, err := sonic.Marshal(sanitizedErr); err == nil { + entry.ErrorDetails = string(data) + } + entry.ErrorDetailsParsed = sanitizedErr +} + // sanitizeErrorForLogging returns a shallow copy of err with ExtraFields.RawRequest and // RawResponse cleared when raw-byte persistence is disabled, preventing raw bytes from // leaking into entry.ErrorDetails via JSON serialization. @@ -1596,7 +1610,7 @@ func (p *LoggerPlugin) PostMCPHook(ctx *schemas.BifrostContext, resp *schemas.Bi if bifrostErr != nil { entry.Status = "error" shouldStoreRaw, _ := ctx.Value(schemas.BifrostContextKeyShouldStoreRawInLogs).(bool) - entry.ErrorDetailsParsed = sanitizeErrorForLogging(bifrostErr, p.contentLoggingEnabled(ctx), shouldStoreRaw) + applyErrorDetailsToMCPEntry(entry, bifrostErr, p.contentLoggingEnabled(ctx), shouldStoreRaw) } else if resp != nil { entry.Status = "success" if p.contentLoggingEnabled(ctx) { diff --git a/plugins/logging/sanitize_test.go b/plugins/logging/sanitize_test.go index e60f720c0b6..b0be37edc86 100644 --- a/plugins/logging/sanitize_test.go +++ b/plugins/logging/sanitize_test.go @@ -78,3 +78,31 @@ func TestApplyErrorDetailsToEntry_NilError(t *testing.T) { t.Error("nil error should leave ErrorDetails empty") } } + +// MCPToolLog counterpart: same sanitization semantics, and ErrorDetails is +// serialized immediately rather than deferred to the BeforeCreate hook. +func TestApplyErrorDetailsToMCPEntry_SanitizedAndSerializedImmediately(t *testing.T) { + entry := &logstore.MCPToolLog{ID: "mcp-1"} + applyErrorDetailsToMCPEntry(entry, errorWithRawPayloads(), false, false) + + if entry.ErrorDetailsParsed == nil { + t.Fatal("ErrorDetailsParsed should be set") + } + if entry.ErrorDetailsParsed.ExtraFields.RawRequest != nil || + entry.ErrorDetailsParsed.ExtraFields.RawResponse != nil { + t.Error("ErrorDetailsParsed should not retain raw payloads when content logging is disabled") + } + if entry.ErrorDetails == "" { + t.Error("ErrorDetails should be serialized immediately, not deferred to BeforeCreate") + } + if strings.Contains(entry.ErrorDetails, "RAW_REQUEST_MARKER") { + t.Error("serialized ErrorDetails must not contain raw payloads when content logging is disabled") + } + + if err := entry.SerializeFields(); err != nil { + t.Fatalf("SerializeFields() error: %v", err) + } + if strings.Contains(entry.ErrorDetails, "RAW_REQUEST_MARKER") { + t.Error("serialized ErrorDetails must not contain raw payloads after SerializeFields") + } +}