feat: Added log redaction for sensitive information #213
Conversation
WalkthroughAdds redaction support to the logging package: new RedactionRule type, Logger.redactionRules field, SetRedactionRules and SetRedactionRegex public methods, a redact helper integrated into the logging flow, tests and benchmarks, and documentation/examples demonstrating prefix and regex-based redaction. No other API changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
logging/README.md (1)
56-69: Clarify overwrite semantics and usage timing for redaction APIs (optional)The behavioral description matches the implementation, but two subtle points might be worth calling out:
- Each call to
SetRedactionRulesorSetRedactionRegexreplaces the entire set of redaction rules (they are not additive).- These setters are intended to be called during logger setup, not while the logger is being used concurrently.
Adding a short note about “last call wins” semantics and recommended usage at initialization would help avoid surprises for callers who expect to mix rule types or update rules at runtime.
logging/logging_test.go (1)
100-163: Redaction tests look solid; consider one extra assertion in the multi-field caseThe table-driven tests for
SetRedactionRulescover:
- Single-field redaction (password, credit card).
- Multiple rules applied to one message.
- “No rules” behavior.
In the “success - redact multiple fields” case, you already verify that the email is redacted and the raw email address is absent. Since the message also contains a password, you could optionally assert that the password portion is redacted as well to exercise all configured rules in the same log line, but this is not strictly necessary for correctness.
Otherwise, the structure and expectations look good.
logging/logging.go (2)
93-112: SetRedactionRegex behavior is good; note overwrite & ordering semantics (optional)
SetRedactionRegexcorrectly:
- Clears any existing rules.
- Compiles each pattern and returns a wrapped error on failure.
- Stores the compiled regexp + replacement for use in
redact.Two subtle behaviors to be aware of:
- Calling
SetRedactionRegexdiscards any rules previously configured viaSetRedactionRules(and vice versa). If callers expect to combine both kinds of rules, they must supply the full set in one call or you’ll need an “append” variant.- When multiple regex patterns can match overlapping substrings, their application order is determined by Go’s map iteration order, which is intentionally randomized. That’s usually fine but can be surprising for overlapping rules.
You may want to document these points, or, if deterministic ordering is important, consider taking a
[]RedactionRuleor[]struct{Pattern, Replacement string}instead ofmap[string]string.
114-122: redact helper is straightforward; consider moving the empty-rules check inside
redactimplements a simple “apply all rules in sequence” pipeline, which matches the documented behavior.If you adopt a mutex (as suggested above), it might be cleaner to:
- Always call
redactfromlog, and- Do the “no rules” fast-path inside
redactunder the read lock.For example:
func (l *Logger) redact(message string) string { - redactedMessage := message - for _, rule := range l.redactionRules { - redactedMessage = rule.Pattern.ReplaceAllString(redactedMessage, rule.Replacement) - } - return redactedMessage + l.redactionMu.RLock() + defer l.redactionMu.RUnlock() + + if len(l.redactionRules) == 0 { + return message + } + + redactedMessage := message + for _, rule := range l.redactionRules { + redactedMessage = rule.Pattern.ReplaceAllString(redactedMessage, rule.Replacement) + } + return redactedMessage }Then in
logyou can simply domessage = l.redact(message)unconditionally, which keeps the concurrency story consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
logging/EXAMPLES.md(1 hunks)logging/README.md(1 hunks)logging/logging.go(4 hunks)logging/logging_test.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
logging/logging_test.go (1)
logging/logging.go (2)
NewLogger(62-72)INFO(29-29)
🔇 Additional comments (5)
logging/logging_test.go (2)
165-237: Regex-based redaction tests and invalid-pattern coverage look goodThe
TestLoggerRedactionRegexcases nicely exercise:
- Typical PII shapes (email, credit-card number, phone number).
- That the clear-text value is removed from the output.
- Error handling when an invalid regex pattern is provided.
The early return when
wantErris true correctly prevents logging when pattern compilation fails, matching the intended contract ofSetRedactionRegex.No issues from a correctness standpoint.
251-260: Benchmark usingb.Loop()is appropriate; ensure Go toolchain supports it
BenchmarkLoggerWithRedactionis structured correctly aroundfor b.Loop() { ... }and exercises the redaction path under load, which is exactly what you want here.Just make sure your CI and consumers are on a Go version that supports
testing.B.Loop(Go 1.24+); otherwise this benchmark won’t compile.logging/logging.go (3)
34-48: RedactionRule and Logger integration are conceptually soundEmbedding compiled
regexp.RegexpinRedactionRuleand attaching a[]RedactionRuletoLoggeris a clean design. It keeps the logging hot path working with precompiled patterns, which is good for performance, and it’s consistent with the tests and docs.No issues here.
136-139: Redaction integration point inlogis correctHooking redaction right before formatting the log line ensures:
- Only the message body is redacted (prefix, level, and timestamp are untouched).
- Redaction happens before color-coding and output, matching the docs and tests.
Once concurrency around
redactionRulesis addressed, this integration point looks good.
74-91: Potential data race when updating redaction rules on a live logger
SetRedactionRulesreplacesl.redactionRuleswithout synchronization. Iflogreads froml.redactionRulesconcurrently (a common pattern for shared loggers), this introduces a data race.This changes the previous API surface, which exposed no mutating methods after construction, making shared loggers effectively read-only and safe from races.
Consider one of:
- Guarding access with a
sync.RWMutex- Using an
atomic.Valueor copy-on-write pattern to swap the slice safely- Explicitly documenting that redaction setters must only be called during initialization, before the logger is shared
Example of a minimal RWMutex-based approach:
-import ( - "fmt" - "io" - "os" - "regexp" - "time" -) +import ( + "fmt" + "io" + "os" + "regexp" + "sync" + "time" +) type Logger struct { - minLevel LogLevel // Minimum log level for messages to be logged - prefix string // Prefix to prepend to all log messages - output io.Writer // Output destination for log messages (e.g., os.Stdout) - disableColors bool // Flag to disable color codes (useful for testing or non-ANSI terminals) - redactionRules []RedactionRule // Rules for redacting sensitive information + minLevel LogLevel // Minimum log level for messages to be logged + prefix string // Prefix to prepend to all log messages + output io.Writer // Output destination for log messages (e.g., os.Stdout) + disableColors bool // Flag to disable color codes (useful for testing or non-ANSI terminals) + redactionMu sync.RWMutex // Protects redactionRules + redactionRules []RedactionRule // Rules for redacting sensitive information } func (l *Logger) SetRedactionRules(rules map[string]string) { - l.redactionRules = make([]RedactionRule, 0, len(rules)) + l.redactionMu.Lock() + defer l.redactionMu.Unlock() + l.redactionRules = make([]RedactionRule, 0, len(rules)) for pattern, replacement := range rules { regex := regexp.MustCompile(regexp.QuoteMeta(pattern) + `[^\s]+`) l.redactionRules = append(l.redactionRules, RedactionRule{ Pattern: regex, Replacement: pattern + replacement, }) } }Lock
SetRedactionRegexsimilarly and acquireRLock/RUnlockinredact(removing thelen(l.redactionRules)check fromlog, lettingredactearly-out when there are no rules).
|
Please fix the build failures |
|
Created #214 to fix the test, it will always fail in December. |
|
Gitleaks is reporting a leaked secret at |
Lach-dev
force-pushed
the
add-log-redaction
branch
from
14bf5b0 to
1157f3a
Compare
|
Removed from history. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
time/time_test.go (1)
498-506: December overflow handling in age test looks correctThe revised month/year calculation avoids the December failure while keeping the expected age at 24 across all months. The logic is sound; only minor nit is that for December the birthday ends up in January of the next year, so the test name no longer exactly describes the scenario, but behavior is still correct for
CalculateAge.logging/EXAMPLES.md (1)
157-187: Use obvious placeholders instead of secret‑shaped values in examplesThe redaction example is functionally correct, but the sample values (
mysecretpass123,1234-5678-9876-5432) look like real credentials and are likely what triggered the Gitleaks alert. To avoid noise from secret scanners and make it clear these are placeholders, consider using more neutral tokens and updating the output accordingly, e.g.:- // Log messages with sensitive data - logger.Info("User logged in with password:mysecretpass123") - logger.Error("Payment failed for credit_card=1234-5678-9876-5432") + // Log messages with sensitive data (placeholders for demo purposes) + logger.Info("User logged in with password:<USER_PASSWORD>") + logger.Error("Payment failed for credit_card=<CARD_NUMBER>")And:
-[2025-01-09 12:34:56] [INFO] MyApp: User logged in with password:***REDACTED*** -[2025-01-09 12:34:56] [ERROR] MyApp: Payment failed for credit_card=***REDACTED*** +[2025-01-09 12:34:56] [INFO] MyApp: User logged in with password:***REDACTED*** +[2025-01-09 12:34:56] [ERROR] MyApp: Payment failed for credit_card=***REDACTED***(The redacted output stays the same; only the original literals change.) This should keep the example clear while satisfying secret‑scanning tools.
logging/README.md (1)
56-69: Redaction API docs align well with implementationThe new Redaction section accurately describes how
SetRedactionRulestreats keys as literal prefixes plus non-space tokens and howSetRedactionRegexbehaves (including invalid-pattern errors). If you want to be extra explicit, you could add a short note that each call replaces any previously configured redaction rules, but as written the docs are already clear and consistent with the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
logging/EXAMPLES.md(1 hunks)logging/README.md(1 hunks)logging/logging.go(4 hunks)logging/logging_test.go(2 hunks)time/time_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- logging/logging_test.go
🔇 Additional comments (1)
logging/logging.go (1)
34-38: Redaction integration into Logger is well-placed and consistentThe introduction of
RedactionRule, theredactionRulesfield, and theredacthelper is clean, and applying redaction inlogbefore color formatting and output keeps the behavior predictable. Usingregexp.QuoteMetaforSetRedactionRulesto treat rule keys as literal prefixes while appending[^\s]+gives a sensible default for “prefix + token” style secrets, and the replacement of only the message body (not prefix, level, or timestamp) is exactly what you want here.Also applies to: 43-47, 74-92, 114-122, 136-139
| // SetRedactionRegex allows users to define custom regex patterns for redaction. | ||
| // This provides more flexibility than SetRedactionRules. | ||
| // | ||
| // Parameters: | ||
| // - patterns: A map where keys are regex patterns and values are replacement strings. | ||
| func (l *Logger) SetRedactionRegex(patterns map[string]string) error { | ||
| l.redactionRules = make([]RedactionRule, 0, len(patterns)) | ||
| for pattern, replacement := range patterns { | ||
| regex, err := regexp.Compile(pattern) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid regex pattern '%s': %w", pattern, err) | ||
| } | ||
| l.redactionRules = append(l.redactionRules, RedactionRule{ | ||
| Pattern: regex, | ||
| Replacement: replacement, | ||
| }) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Avoid partially updating redaction rules on SetRedactionRegex error
SetRedactionRegex currently assigns a new slice to l.redactionRules before validating all patterns. If any pattern fails to compile:
- The method returns an error (good), but
l.redactionRuleshas already been mutated, potentially containing only a subset of the new patterns and discarding any previously configured rules.
For callers, it’s natural to assume that on error the logger’s configuration remains unchanged. Here, a typo in one regex can silently drop existing working redaction rules, weakening protection.
Consider compiling into a local slice and only assigning to l.redactionRules after all patterns succeed:
-func (l *Logger) SetRedactionRegex(patterns map[string]string) error {
- l.redactionRules = make([]RedactionRule, 0, len(patterns))
- for pattern, replacement := range patterns {
- regex, err := regexp.Compile(pattern)
- if err != nil {
- return fmt.Errorf("invalid regex pattern '%s': %w", pattern, err)
- }
- l.redactionRules = append(l.redactionRules, RedactionRule{
- Pattern: regex,
- Replacement: replacement,
- })
- }
-
- return nil
-}
+func (l *Logger) SetRedactionRegex(patterns map[string]string) error {
+ rules := make([]RedactionRule, 0, len(patterns))
+ for pattern, replacement := range patterns {
+ regex, err := regexp.Compile(pattern)
+ if err != nil {
+ return fmt.Errorf("invalid regex pattern '%s': %w", pattern, err)
+ }
+ rules = append(rules, RedactionRule{
+ Pattern: regex,
+ Replacement: replacement,
+ })
+ }
+
+ l.redactionRules = rules
+ return nil
+}This keeps the logger’s redaction configuration atomic from the caller’s perspective: either all new patterns are installed, or none are.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SetRedactionRegex allows users to define custom regex patterns for redaction. | |
| // This provides more flexibility than SetRedactionRules. | |
| // | |
| // Parameters: | |
| // - patterns: A map where keys are regex patterns and values are replacement strings. | |
| func (l *Logger) SetRedactionRegex(patterns map[string]string) error { | |
| l.redactionRules = make([]RedactionRule, 0, len(patterns)) | |
| for pattern, replacement := range patterns { | |
| regex, err := regexp.Compile(pattern) | |
| if err != nil { | |
| return fmt.Errorf("invalid regex pattern '%s': %w", pattern, err) | |
| } | |
| l.redactionRules = append(l.redactionRules, RedactionRule{ | |
| Pattern: regex, | |
| Replacement: replacement, | |
| }) | |
| } | |
| return nil | |
| } | |
| // SetRedactionRegex allows users to define custom regex patterns for redaction. | |
| // This provides more flexibility than SetRedactionRules. | |
| // | |
| // Parameters: | |
| // - patterns: A map where keys are regex patterns and values are replacement strings. | |
| func (l *Logger) SetRedactionRegex(patterns map[string]string) error { | |
| rules := make([]RedactionRule, 0, len(patterns)) | |
| for pattern, replacement := range patterns { | |
| regex, err := regexp.Compile(pattern) | |
| if err != nil { | |
| return fmt.Errorf("invalid regex pattern '%s': %w", pattern, err) | |
| } | |
| rules = append(rules, RedactionRule{ | |
| Pattern: regex, | |
| Replacement: replacement, | |
| }) | |
| } | |
| l.redactionRules = rules | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In logging/logging.go around lines 93 to 112, SetRedactionRegex mutates
l.redactionRules before validating all regexes, which can leave the logger in a
partially-updated state if one pattern fails; to fix, build and validate a local
slice of RedactionRule (compiling each pattern) and only assign it to
l.redactionRules after all patterns compile successfully so the existing rules
remain unchanged on error, returning the compile error without modifying the
logger state.
2 participants
Description:
Add log redaction support to the
loggingpackage ( Issue #44 )Purpose:
Changes:
logging/logging.go:RedactionRuletype,SetRedactionRules,SetRedactionRegex, andredactusage inlog.logging/logging_test.gocovering:logging/EXAMPLES.md.logging/README.mdto describe redaction usage.Checklist:
make test.make lint.If added a new utility function or updated existing function logic:
EXAMPLES.mdREADME.mdSummary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.