V3/santize logs #4807
V3/santize logs #4807
Conversation
- Add Sanitizer type with configurable field/pattern-based redaction - Include default redaction for passwords, tokens, API keys, JWTs - Support case-insensitive substring matching for field names - Support regex patterns for value-based detection - Support recursive sanitization of nested JSON/maps - Add CustomSanitizeFunc for developer override control - Add comprehensive unit tests and benchmarks
- Add SanitizingHandler to wrap slog.Handler for automatic sanitization - Add SanitizeOptions to application Options struct - Wrap application logger with sanitizing handler on initialization - Expose Sanitizer() method on App for public API access - Add comprehensive handler tests
- Add Log Sanitization section to security guide - Document default redacted fields and patterns - Add configuration examples and options table - Add Sanitizer API reference to application doc
WalkthroughAdds a configurable log sanitization subsystem: a Sanitizer type with options, app-level wiring and accessor, a slog SanitizingHandler wrapper, documentation and examples, and comprehensive tests and benchmarks. Changes
sequenceDiagram
autonumber
actor AppInit as App Initialization
participant Sanitizer as Sanitizer
participant Logger as slog.Logger
participant Handler as Underlying Handler
AppInit->>Sanitizer: NewSanitizer(SanitizeOptions)
AppInit->>Logger: WrapLoggerWithSanitizer(logger, Sanitizer)
Note over Logger,Handler: At runtime when logging occurs
Logger->>Handler: Handle(record with attrs)
Handler->>Sanitizer: Sanitize attributes (SanitizeValue / SanitizeMap)
Sanitizer-->>Handler: Return sanitized attributes
Handler-->>Handler: Forward sanitized record to underlying handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🪛 markdownlint-cli2 (0.18.1)v3/UNRELEASED_CHANGELOG.md20-20: Multiple headings with the same content (MD024, no-duplicate-heading) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
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. Comment |
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
docs/src/content/docs/guides/security.mdx (1)
222-225: Consider consistent spelling of "sanitize" throughout the document.The static analysis tool flagged that this document mixes "sanitise" (British spelling, used in existing content at line 36) and "sanitize" (American spelling, used in the new content). For consistency, consider using one spelling throughout the file.
v3/pkg/application/sanitize_handler.go (1)
90-110: Consider usingstrings.Joinfor cleaner path construction.The current implementation manually builds the path string. Using
strings.Joinwould be more idiomatic.// buildPath constructs the full path for an attribute key. func (h *SanitizingHandler) buildPath(key string) string { if len(h.groups) == 0 { return key } - - path := "" - for _, g := range h.groups { - if path != "" { - path += "." - } - path += g - } + path := strings.Join(h.groups, ".") if key != "" { - if path != "" { - path += "." - } - path += key + path = path + "." + key } return path }Note: This would require adding
"strings"to the imports.v3/pkg/application/sanitize_test.go (3)
635-641: Test handler doesn't preserveWithAttrs/WithGroupstate.The
testHandlerreturns itself without storing attributes or group names. This is acknowledged in the test comment at line 757, but it meansTestSanitizingHandler_WithAttrscannot fully verify that pre-set attributes are sanitized. Consider enhancing the test handler to track accumulated attrs for more complete coverage.type testHandler struct { - entries []testLogEntry + entries []testLogEntry + preAttrs []slog.Attr + groupNames []string }
837-852: Benchmark includes non-benchmark work in timing.Resetting
underlying.entries = nilinside the timed loop adds overhead to the measurement. Useb.StopTimer()/b.StartTimer()or pre-allocate the slice with sufficient capacity outside the loop.func BenchmarkSanitizingHandler(b *testing.B) { underlying := &testHandler{} sanitizer := NewSanitizer(nil) handler := NewSanitizingHandler(underlying, sanitizer) logger := slog.New(handler) b.ResetTimer() for i := 0; i < b.N; i++ { - underlying.entries = nil // Reset + underlying.entries = underlying.entries[:0] // Reuse backing array logger.Info("test", "username", "john", "password", "secret123", "count", 42, ) } }
854-869: Same benchmark overhead issue.Apply the same fix to reuse the slice backing array instead of reallocating.
for i := 0; i < b.N; i++ { - underlying.entries = nil + underlying.entries = underlying.entries[:0]v3/pkg/application/sanitize.go (3)
208-221: Array element paths lose index specificity.The path for array elements uses
parentPath + "[]"without the index, so all elements share the same path. IfCustomSanitizeFuncneeds to distinguish between array positions, it cannot. Consider including the index:func (s *Sanitizer) sanitizeSlice(slice []any, parentPath string) []any { result := make([]any, len(slice)) for i, v := range slice { - // For array elements, use index in path - path := parentPath - if path != "" { - path = parentPath + "[]" - } + path := fmt.Sprintf("%s[%d]", parentPath, i) + if parentPath == "" { + path = fmt.Sprintf("[%d]", i) + } // Array elements don't have a "key" for field matching, // but we still process nested structures and check patterns result[i] = s.SanitizeValue("", v, path) } return result }
231-241: Invalid JSON with pattern match returns quoted replacement.When JSON parsing fails but the raw string matches a sensitive pattern, the method returns
"***"(with JSON string quotes). This changes the data from invalid-JSON bytes to a valid JSON string, which may surprise callers expecting the original structure to be preserved.Consider returning just the replacement bytes without the quotes, or documenting this behavior explicitly.
for _, pattern := range s.patterns { if pattern.MatchString(str) { - return []byte(`"` + s.replacement + `"`) + // Return replacement as raw bytes (not JSON-encoded) + return []byte(s.replacement) } }
92-99: Thread-safety is implicitly safe for concurrent reads.The
Sanitizerstruct is safe for concurrent use after construction since all fields are only read (never written) during sanitization. Consider adding a brief doc comment to clarify this:// Sanitizer handles redaction of sensitive data in logs and other output. +// A Sanitizer is safe for concurrent use after construction. type Sanitizer struct {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/src/content/docs/guides/security.mdx(3 hunks)docs/src/content/docs/reference/application.mdx(1 hunks)v3/examples/log-sanitization/README.md(1 hunks)v3/examples/log-sanitization/main.go(1 hunks)v3/pkg/application/application.go(3 hunks)v3/pkg/application/application_options.go(1 hunks)v3/pkg/application/sanitize.go(1 hunks)v3/pkg/application/sanitize_handler.go(1 hunks)v3/pkg/application/sanitize_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-13T01:05:02.267Z
Learnt from: fbbdev
Repo: wailsapp/wails PR: 4066
File: v3/pkg/application/messageprocessor_call.go:174-174
Timestamp: 2025-02-13T01:05:02.267Z
Learning: When handling JSON marshaling errors in Wails v3, the error message from json.Marshal provides sufficient debugging context. Logging raw data is unnecessary and could make logs harder to read.
Applied to files:
docs/src/content/docs/guides/security.mdx
🧬 Code graph analysis (3)
v3/pkg/application/application_options.go (1)
v3/pkg/application/sanitize.go (1)
SanitizeOptions(47-90)
v3/pkg/application/application.go (2)
v3/pkg/application/sanitize.go (2)
SanitizeOptions(47-90)Sanitizer(93-99)v3/pkg/application/sanitize_handler.go (1)
WrapLoggerWithSanitizer(176-182)
v3/examples/log-sanitization/main.go (1)
v3/pkg/application/sanitize.go (2)
SanitizeOptions(47-90)Sanitizer(93-99)
🪛 Gitleaks (8.30.0)
v3/pkg/application/sanitize_test.go
[high] 154-154: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
[high] 155-155: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
[high] 147-147: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
[high] 228-228: Uncovered a JSON Web Token, which may lead to unauthorized access to web applications and sensitive user data.
(jwt)
🪛 LanguageTool
docs/src/content/docs/guides/security.mdx
[uncategorized] ~224-~224: Do not mix variants of the same word (‘sanitize’ and ‘sanitise’) within a single text.
Context: ...# Log Sanitization Wails automatically sanitizes sensitive data in IPC logs to prevent a...
(EN_WORD_COHERENCY)
[uncategorized] ~322-~322: Do not mix variants of the same word (‘sanitize’ and ‘sanitise’) within a single text.
Context: ...'t expose sensitive data in logs (Wails sanitizes IPC logs by default) - Don't use weak e...
(EN_WORD_COHERENCY)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
🔇 Additional comments (24)
v3/examples/log-sanitization/README.md (1)
1-43: Documentation is clear and comprehensive.The README effectively demonstrates the sanitization feature with clear sections for running the example, what gets redacted by default, and how to customize configuration. The platform status table is a nice touch for cross-platform awareness.
v3/pkg/application/application_options.go (1)
61-65: Well-designed API addition.The pointer type
*SanitizeOptionsappropriately distinguishes between "use defaults" (nil) and "explicitly configured" states. The comment clearly documents the default behavior and how to disable sanitization.v3/pkg/application/application.go (3)
61-65: Sanitizer integration is correctly placed in the initialization flow.The sanitizer is initialized after the logger is set up but before any logging occurs (signal handling, startup logs). The conditional wrapping when
!result.sanitizer.IsDisabled()avoids unnecessary overhead when sanitization is disabled.
387-387: Clean addition of the sanitizer field.The field is appropriately placed alongside other application-wide resources like
Logger.
432-436: Accessor method follows established patterns.The
Sanitizer()method is consistent with other accessor methods in the App struct (e.g.,Config(),Context()). The documentation clearly explains its purpose for manual sanitization using application-configured rules.docs/src/content/docs/guides/security.mdx (3)
226-301: Comprehensive documentation of the sanitization feature.The new section effectively covers default protection, custom configuration with all available options, the configuration options table, and the public API. The code examples are clear and demonstrate real-world usage patterns.
322-325: Good additions to the "Don't" list.These new items reinforce the importance of the sanitization feature and caution against disabling it in production.
339-340: Valuable checklist additions.These items remind developers to configure app-specific sensitive fields and avoid bypassing sanitization in custom logging code.
docs/src/content/docs/reference/application.mdx (1)
365-430: API documentation is clear and complete.The new Sanitizer section follows the established documentation patterns in this file. The method signature, usage examples for
SanitizeMapandSanitizeJSON, configuration options, and cross-reference to the Security Guide provide comprehensive coverage for developers.v3/examples/log-sanitization/main.go (2)
11-42: Well-structured configuration example.The configuration demonstrates all major options (RedactFields, RedactPatterns, CustomSanitizeFunc, Replacement) with clear inline comments. The CustomSanitizeFunc example showing path-based redaction is particularly useful.
44-112: Comprehensive demonstration of sanitization scenarios.The example covers:
- Default field redaction (password, token, apiKey)
- Custom field redaction (cardNumber, cvv, ssn)
- Pattern matching (JWT, Bearer tokens)
- CustomSanitizeFunc (payment path handling)
- Nested structures
- JSON sanitization
This provides developers with clear, runnable examples of each feature.
v3/pkg/application/sanitize_handler.go (7)
11-27: Clean handler implementation with proper nil handling.The constructor appropriately creates a default sanitizer if none is provided, ensuring the handler always has a valid sanitizer instance.
34-51: Correct record handling with immutability.Creating a new
slog.Recordinstead of mutating the original is the right approach. The early return when sanitization is disabled provides good optimization.
53-75: WithAttrs properly maintains immutability.The handler correctly returns a new
SanitizingHandlerinstance with sanitized attributes, preserving the immutability semantics expected ofslog.Handler.WithAttrs.
77-88: WithGroup correctly maintains group stack for path building.The slice copy with pre-allocated capacity (line 79) is a good practice to avoid modifying the parent handler's groups slice.
112-133: Recursive attribute sanitization handles nested groups correctly.The recursive call to
sanitizeAttrwith the accumulated path properly handles arbitrarily nested group structures.
135-172: Value sanitization handles all slog.Value kinds appropriately.The implementation correctly:
- Resolves
LogValuerinterfaces before processing- Handles string values directly
- Processes
KindAnyfor complex types- Returns the original value for primitive types (Int64, Bool, etc.) unless the key triggers redaction
The comparison with
h.sanitizer.Replacement()on line 165 correctly identifies when a value was redacted.
174-182: Convenience wrapper is straightforward and handles nil logger correctly.The nil check prevents panics when wrapping a nil logger.
v3/pkg/application/sanitize_test.go (2)
147-162: Static analysis false positives – intentional test data.Gitleaks flagged the JWT token (line 147) and API key patterns (lines 154-157) as potential secrets. These are intentional test fixtures used to verify that the sanitizer correctly redacts such patterns. No action needed.
1-869: Comprehensive test suite with good coverage.The test file thoroughly covers:
- Default initialization, disabled mode, custom replacement
- Field and pattern matching (case-insensitive, substring)
- Nested maps, slices, JSON, and
json.RawMessage- Custom sanitize functions (partial and full override)
- Path tracking
- Handler integration with groups and attributes
- Performance benchmarks
v3/pkg/application/sanitize.go (4)
158-173: Field and pattern matching is O(fields × keys) + O(patterns × strings).For typical configurations this is fine, but with many custom fields/patterns on high-throughput log paths, this could become a bottleneck. If performance becomes an issue, consider:
- Pre-building a single combined regex from all fields
- Using a
map[string]struct{}for exact field lookups (separate from substring matching)This is informational—current implementation is acceptable for most use cases.
12-27: Default redact fields cover common secrets well.The default field list includes authentication tokens, cryptographic keys, and session identifiers with case-insensitive substring matching. This provides good out-of-the-box protection.
One consideration: fields like
"auth"and"pass"may cause false positives (e.g.,"passenger","authority"). The substring matching is documented, so users can disable defaults if needed.
103-142: Constructor handles nil options and merging correctly.The
NewSanitizerfunction properly:
- Returns safe defaults when
optsis nil- Short-circuits when disabled
- Merges custom fields/patterns with defaults unless
DisableDefaultsis set
176-183: Recursive handling of nested types is correct.The switch statement properly handles
map[string]any,[]any, andjson.RawMessagefor deep sanitization. Other types pass through unchanged as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
v3/UNRELEASED_CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
v3/UNRELEASED_CHANGELOG.md
21-21: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run Go Tests v3 (macos-latest, 1.24)
- GitHub Check: Run Go Tests v3 (ubuntu-latest, 1.24)
- GitHub Check: Run Go Tests v3 (windows-latest, 1.24)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
|
Do you think it'd be better to wrap this in a custom slog logger so the options are on that rather than the application itself? |
2 participants
Adds automatic redaction of sensitive data in IPC logs.
Changes
SanitizeOptionsin application optionsSanitizer()API for custom useexamples/log-sanitizationSanitizeOptions:
RedactFields: additional field names to redact (merged with defaults)RedactPatterns: additional regex patterns to match valuesCustomSanitizeFunc: full control - return (value, true) to override or (_, false) to fallthroughReplacement: custom replacement string (default: "***")DisableDefaults: only use explicitly specified fields/patternsDisabled: completely disable sanitizationDefault redacted fields: password, passwd, pwd, token, secret, apikey, api_key, auth, authorization, credential, bearer, jwt, private, privatekey, private_key, session, sessionid, session_id, cookie, csrf, xsrf, access_token, refresh_token, signing, encryption_key
Default patterns: JWT tokens, Bearer tokens, API keys (sk_live_, pk_test_)
Usage:
Manually Sanitize
Enabled by default. Set Disabled: true to opt out.
Doctor
AI Slop
This pull request introduces a comprehensive log sanitization feature to the Wails application framework. Sensitive data in IPC logs is now automatically redacted by default, with extensive configurability for developers. The changes include a new sanitizer API, documentation updates, and an example application demonstrating the feature.
Log Sanitization Core Implementation:
Sanitizertype andSanitizeOptionsstruct inv3/pkg/application/sanitize.goto handle automatic redaction of sensitive data in logs and user data. The sanitizer supports default sensitive fields/patterns, custom fields/patterns, a custom function for advanced control, and options to disable or customize the behavior.application.Newand wraps the logger unless explicitly disabled. The sanitizer instance is stored on theAppstruct and exposed via the newSanitizer()method. [1] [2] [3]Configuration and API Exposure:
application.Optionsstruct with aSanitizeOptionsfield, allowing developers to configure log sanitization at application startup.Documentation and Examples:
security.mdx) with a detailed section on log sanitization, including default protections, custom configuration, full control options, and best practices. The security checklist and "Do/Don't" sections were also updated to reflect the new feature. [1] [2] [3]application.mdx) to document the sanitizer API and configuration options.v3/examples/log-sanitization) with a README and demo code showing how to configure and use the sanitizer, as well as what data gets redacted by default. [1] [2]These changes provide secure, flexible, and developer-friendly mechanisms to prevent accidental leakage of secrets in logs, with clear documentation and practical examples.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.