[fix]: sanitize ErrorDetailsParsed so raw payloads honor disable_content_logging

[citrocat]

Summary

The logging plugin serialized a sanitized copy of the provider error into entry.ErrorDetails, but stored the original, unsanitized error in entry.ErrorDetailsParsed. logstore.Log.SerializeFields — run from the BeforeCreate GORM hook on every insert — re-serializes ErrorDetailsParsed and overwrites ErrorDetails, and nothing on the write path clears the parsed field first. The sanitization was therefore defeated end to end: with disable_content_logging: true (or raw storage off), error rows still persisted the full raw request/response payloads attached to the error, and each queued write entry pinned those payloads in memory until batch flush.

Changes

• plugins/logging/main.go — new helper applyErrorDetailsToEntry that sanitizes once and stores the same sanitized copy in both ErrorDetails and ErrorDetailsParsed; the four direct bifrostErr assignment sites now use it. The MCP tool log path (PostMCPHook) sanitizes its MCPToolLog.ErrorDetailsParsed assignment the same way.
• plugins/logging/operations.go — applyStreamingOutputToEntry (which serialized the unsanitized error into both fields) now uses the helper.
• plugins/logging/sanitize_test.go — regression tests that round-trip through logstore.Log.SerializeFields (the actual overwrite path): raw markers must not survive with content logging disabled, must survive when explicitly enabled, and a nil error is a no-op.
• plugins/logging/changelog.md — changelog entry.

Behavior with content logging + raw storage both enabled is unchanged — sanitizeErrorForLogging passes the original through in that configuration.

Not covered (maintainer input wanted)

updateLogEntry's UpdateLogData.ErrorDetails path (operations.go) has the same shape, but shouldStoreRaw is not in scope there (the function only receives contentLoggingEnabled). Rather than guess at threading the flag through, I left it and flagged it in #4872 — happy to extend this PR if you point me at the preferred plumbing.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature
• Breaking change
• Documentation update

Affected areas

• Core
• Transports
• Providers
• Plugins
• UI
• Docs

How to test

cd plugins/logging
go test -race -run TestApplyErrorDetails -v .
go test -race -count=1 .

Full go test -race on plugins/logging passes; gofmt clean.

Breaking changes

None. Configurations with raw persistence enabled see identical output; configurations with it disabled now get what the config promised.

Related issues

Closes #4872

Security considerations

This is primarily a data-privacy fix: prevents raw request/response content from being persisted to the logs store when the operator has disabled content logging.

Checklist

• Tests added for the change
• Changelog updated (plugins/logging/changelog.md)
• Commit message follows [type]: description format
• PR kept small and focused (+109/−28)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2fda7ec2-8698-4e37-8185-a5f72f7f6bce

📥 Commits

Reviewing files that changed from the base of the PR and between 9072397 and a87d792.

📒 Files selected for processing (1)

• plugins/logging/main.go

🚧 Files skipped from review as they are similar to previous changes (1)

• plugins/logging/main.go

---

📝 Walkthrough

Summary by CodeRabbit

• Bug Fixes
  • Error details are now consistently sanitized and stored across streaming, non-streaming, and tool/MCP error logging, preventing raw request/response payloads from being retained when content logging and raw storage are disabled.
  • Error details are serialized immediately so the logged output always matches the sanitized parsed content.
• Tests
  • Added unit tests verifying sanitized vs raw-preserving behavior, nil-error handling, and correct tool/MCP logging serialization.

Walkthrough

Logging now stores sanitized error details consistently in both parsed and serialized fields across LLM, MCP, and streaming error paths. Tests cover sanitized, raw-enabled, nil, and MCP cases, and the changelog notes the fix.

Changes

Sanitize ErrorDetailsParsed

Layer / File(s)	Summary
Sanitization helpers plugins/logging/main.go	Adds helpers that sanitize a BifrostError, serialize the sanitized copy when possible, and write the sanitized struct into log entry error fields.
LLM and MCP error paths plugins/logging/main.go	Updates the PostLLMHook minimal, non-streaming, and streaming error paths, plus the PostMCPHook error path, to use the shared sanitization helpers instead of storing raw parsed errors.
Streaming output error path plugins/logging/operations.go	Routes applyStreamingOutputToEntry's error handling through the shared helper and passes the content-logging and raw-storage flags into that path.
Tests and changelog plugins/logging/sanitize_test.go, plugins/logging/changelog.md	Adds unit tests for sanitized, raw-preserving, nil-error, and MCP behaviors, and adds a changelog note for the error-detail sanitization change.

Estimated code review effort: 3 (Moderate) | ~25 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main fix: sanitizing ErrorDetailsParsed to respect disable_content_logging.
Description check	✅ Passed	The description follows the template well and includes summary, changes, testing, related issues, security, and checklist sections.
Linked Issues check	✅ Passed	The PR addresses #4872 by storing sanitized error data in both fields across the affected logging paths and adding regression tests.
Out of Scope Changes check	✅ Passed	The changes stay focused on the logging sanitization fix, tests, and changelog with no unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.2)

level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies"

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Confidence Score: 5/5

Safe to merge — the fix is narrowly scoped to the logging plugin, correctly closes the sanitization bypass via a sanitize-once helper, and the regression tests directly exercise the GORM overwrite path that caused the leak.

The change touches only the logging plugin's error-detail assignment sites. The new helpers are straightforward: sanitize once, serialize immediately, store the same copy in both fields. The streaming path that was missing sanitization entirely is now covered. The updateLogEntry gap is pre-existing, explicitly acknowledged, and tracked separately. Tests exercise the concrete failure scenario (SerializeFields overwriting a sanitized ErrorDetails with an unsanitized ErrorDetailsParsed) and the raw-enabled preserve-as-is path.

No files require special attention. The streaming path in operations.go is now correctly covered by the helper.

Important Files Changed

Filename	Overview
plugins/logging/main.go	Introduces applyErrorDetailsToEntry and applyErrorDetailsToMCPEntry helpers that sanitize once and store the same sanitized copy in both ErrorDetails (serialized immediately) and ErrorDetailsParsed; replaces four open-coded assignment sites on LLM paths and fixes the MCP path that previously stored the raw pointer without any sanitization at all.
plugins/logging/operations.go	applyStreamingOutputToEntry now delegates to applyErrorDetailsToEntry, closing the gap where the streaming error path serialized the unsanitized error into both fields and never called sanitizeErrorForLogging.
plugins/logging/sanitize_test.go	New regression tests round-trip through logstore.Log.SerializeFields (the actual DB-write overwrite path) to confirm raw markers are absent when content logging is disabled, preserved when enabled, no-op for nil errors, and that the MCPToolLog counterpart serializes immediately.
plugins/logging/changelog.md	Changelog entry added for the fix.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Hook as PostLLMHook / PostMCPHook
    participant Helper as applyErrorDetailsToEntry
    participant Sanitize as sanitizeErrorForLogging
    participant Entry as logstore.Log / MCPToolLog
    participant Batch as Async Batch Writer
    participant GORM as GORM BeforeCreate
    participant SF as SerializeFields()
    participant DB as Database

    Hook->>Helper: bifrostErr, contentLoggingEnabled, shouldStoreRaw
    Helper->>Sanitize: bifrostErr, flags
    alt "contentLoggingEnabled && shouldStoreRaw"
        Sanitize-->>Helper: original err pointer (raw preserved)
    else content logging disabled OR raw storage off
        Sanitize-->>Helper: "cloned err (RawRequest=nil, RawResponse=nil)"
    end
    Helper->>Helper: sonic.Marshal(sanitizedErr) → ErrorDetails string (immediate)
    Helper->>Entry: "entry.ErrorDetails = serialized JSON"
    Helper->>Entry: "entry.ErrorDetailsParsed = sanitizedErr (same sanitized copy)"
    Hook->>Batch: enqueue entry

    Note over Batch,DB: async batch write
    Batch->>GORM: BeforeCreate(entry)
    GORM->>SF: entry.SerializeFields()
    SF->>Entry: "ErrorDetails = marshal(ErrorDetailsParsed)"
    Note over SF,Entry: Both fields hold sanitized copy so overwrite is safe
    SF-->>GORM: ok
    GORM->>DB: INSERT (no raw payloads leak when disabled)

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Hook as PostLLMHook / PostMCPHook
    participant Helper as applyErrorDetailsToEntry
    participant Sanitize as sanitizeErrorForLogging
    participant Entry as logstore.Log / MCPToolLog
    participant Batch as Async Batch Writer
    participant GORM as GORM BeforeCreate
    participant SF as SerializeFields()
    participant DB as Database

    Hook->>Helper: bifrostErr, contentLoggingEnabled, shouldStoreRaw
    Helper->>Sanitize: bifrostErr, flags
    alt "contentLoggingEnabled && shouldStoreRaw"
        Sanitize-->>Helper: original err pointer (raw preserved)
    else content logging disabled OR raw storage off
        Sanitize-->>Helper: "cloned err (RawRequest=nil, RawResponse=nil)"
    end
    Helper->>Helper: sonic.Marshal(sanitizedErr) → ErrorDetails string (immediate)
    Helper->>Entry: "entry.ErrorDetails = serialized JSON"
    Helper->>Entry: "entry.ErrorDetailsParsed = sanitizedErr (same sanitized copy)"
    Hook->>Batch: enqueue entry

    Note over Batch,DB: async batch write
    Batch->>GORM: BeforeCreate(entry)
    GORM->>SF: entry.SerializeFields()
    SF->>Entry: "ErrorDetails = marshal(ErrorDetailsParsed)"
    Note over SF,Entry: Both fields hold sanitized copy so overwrite is safe
    SF-->>GORM: ok
    GORM->>DB: INSERT (no raw payloads leak when disabled)

Loading

_{Reviews (3): Last reviewed commit: "Merge branch 'dev' into fix/sanitize-err..." | Re-trigger Greptile}

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ citrocat
❌ akshaydeo
_{You have signed the CLA already but the status is still pending? Let us recheck it.}