feat: Added log redaction for sensitive information #213
Conversation
WalkthroughAdds a redaction feature to the logging package: two new public APIs for prefix and regex redaction, core redaction logic integrated into the logger, tests (including error path and benchmark), and documentation/examples demonstrating usage. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Logger
participant Redactor
participant Colorizer
participant Output
Caller->>Logger: Log(message)
Logger->>Redactor: redact(message) -- apply prefix & regex rules
Redactor-->>Logger: redactedMessage
Logger->>Colorizer: applyColors(redactedMessage)
Colorizer-->>Logger: coloredMessage
Logger->>Output: write(coloredMessage)
Output-->>Caller: persisted/logged
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
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
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
logging/logging.go (2)
74-91: Consider returning an error for API consistency.While
regexp.MustCompileon line 85 is safe here (sinceregexp.QuoteMetaescapes all special characters), this creates an inconsistent API withSetRedactionRegexwhich returns an error. For a library, panic-free error handling is preferable.Apply this diff to return an error:
-func (l *Logger) SetRedactionRules(rules map[string]string) { +func (l *Logger) SetRedactionRules(rules map[string]string) error { l.redactionRules = make([]RedactionRule, 0, len(rules)) for pattern, replacement := range rules { // Create a regex that matches the pattern followed by any non-space characters - regex := regexp.MustCompile(regexp.QuoteMeta(pattern) + `[^\s]+`) + regex, err := regexp.Compile(regexp.QuoteMeta(pattern) + `[^\s]+`) + if err != nil { + return fmt.Errorf("failed to compile pattern '%s': %w", pattern, err) + } l.redactionRules = append(l.redactionRules, RedactionRule{ Pattern: regex, Replacement: pattern + replacement, }) } + return nil }
115-123: Consider documenting the order-dependent rule application.Rules are applied sequentially (line 119), meaning earlier rules can affect later ones. While this is likely intentional, documenting this behavior would help users understand that rule order matters and avoid unintended cascading replacements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
logging/logging.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-16T00:59:45.522Z
Learnt from: andremartinsds
Repo: kashifkhan0771/utils PR: 40
File: url/url.go:76-79
Timestamp: 2024-11-16T00:59:45.522Z
Learning: In the `BuildURL` function within `url/url.go`, the current path validation regex is appropriate and should not be changed without a specific reason.
Applied to files:
logging/logging.go
📚 Learning: 2025-08-12T07:38:35.407Z
Learnt from: addvanced
Repo: kashifkhan0771/utils PR: 177
File: slugger/slugger.go:39-43
Timestamp: 2025-08-12T07:38:35.407Z
Learning: In the slugger package, the user addvanced mentioned they have a separate branch that refactors the substitution logic from using strings.ReplaceAll in a loop to using strings.NewReplacer, which is a more appropriate and efficient approach for handling multiple string replacements.
Applied to files:
logging/logging.go
🔇 Additional comments (4)
logging/logging.go (4)
10-10: LGTM!The
regexpimport is necessary for the redaction functionality and uses the standard library.
34-38: LGTM!The
RedactionRuletype is well-designed with appropriate field types and clear documentation.
137-140: LGTM!The redaction is correctly applied before color coding and formatting. The length check efficiently skips the redact call when no rules are configured.
93-113: Verify concurrent access safety for redaction rule updates.The previous partial-update concern has been resolved—the code now validates all patterns in a local slice before assigning to
l.redactionRules(line 111).However, both
SetRedactionRegexandSetRedactionRulesmodifyl.redactionRuleswithout synchronization, while thelog()method reads it. If redaction rules are updated concurrently with logging, a data race can occur.Verify the intended usage pattern: if the logger is configured once at startup and used immutably thereafter, no fix is needed. If runtime reconfiguration concurrent with logging is expected, add mutex protection to synchronize access to
l.redactionRules.
Signed-off-by: Kashif Khan <70996046+kashifkhan0771@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Me after pushing last commit and now with my own rules I cannot merge this |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
logging/logging.go (1)
74-91: Make SetRedactionRules robust to whitespace between key and valueThe current pattern requires a non-whitespace character immediately after
pattern. For example:
pattern = "password=", log"password=secret"→ matchespattern = "password:", log"password: secret"→ does not match (space after:breaks the pattern)Callers then need to pass
"password: "(with trailing space) as the key, which is easy to miss.Use
\s*to allow optional whitespace between the pattern and the value:- regex := regexp.MustCompile(regexp.QuoteMeta(pattern) + `[^\s]+`) + regex := regexp.MustCompile(regexp.QuoteMeta(pattern) + `\s*[^\s]+`)This handles both
password=secretandpassword: secretwithout requiring callers to remember a trailing space.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
logging/logging.go(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-16T00:59:45.522Z
Learnt from: andremartinsds
Repo: kashifkhan0771/utils PR: 40
File: url/url.go:76-79
Timestamp: 2024-11-16T00:59:45.522Z
Learning: In the `BuildURL` function within `url/url.go`, the current path validation regex is appropriate and should not be changed without a specific reason.
Applied to files:
logging/logging.go
📚 Learning: 2025-08-12T07:38:35.407Z
Learnt from: addvanced
Repo: kashifkhan0771/utils PR: 177
File: slugger/slugger.go:39-43
Timestamp: 2025-08-12T07:38:35.407Z
Learning: In the slugger package, the user addvanced mentioned they have a separate branch that refactors the substitution logic from using strings.ReplaceAll in a loop to using strings.NewReplacer, which is a more appropriate and efficient approach for handling multiple string replacements.
Applied to files:
logging/logging.go
🔇 Additional comments (4)
logging/logging.go (4)
34-48: RedactionRule struct and Logger wiring look appropriateThe struct shape (
*regexp.Regexp+ replacement) and storing a slice of rules onLoggeris a clean, extensible way to plug redaction into the existing logger without exposing internal details.
93-114: Atomic SetRedactionRegex configuration looks goodBuilding
rulesin a local slice and assigning tol.redactionRulesonly after all patterns compile ensures that on a compile error the logger’s redaction config is left untouched. This addresses the previous partial‑update concern and gives callers clear all‑or‑nothing semantics.
116-124: redact implementation is straightforward and appropriateApplying each compiled regex in sequence over a working copy of the message is clear and maintainable, and should be performant enough for the expected number of rules.
138-141: Redaction hook placement in log() is sensibleConditionally applying
l.redactonly when rules exist avoids extra work, and doing it before formatting/coloring ensures that only redacted content is written to the output while leaving prefixes and level metadata untouched.
3 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.