feat(secrets): add secrets detection and redaction package#42
Conversation
Introduce pkg/secrets with configurable detectors and a redactor to prevent accidental leakage of sensitive data in logs/configs. - Detectors: AWS access keys, GitHub tokens, Slack tokens, Google API keys, Stripe secrets, JWTs, private keys, and Bearer tokens - Redactor: supports nested structures and strings; configurable mask (default [REDACTED]), keys to redact, and max input length; integrates with detector to mask found secrets - Errors: ErrInvalidRedactorConfig, ErrInvalidSecretConfig, ErrSecretInputTooLong, ErrSecretDetected - Add docs (doc.go) and tests (pkg/secrets/secrets_test.go) Refs: introduce NewSecretDetector, NewRedactor and related options.
There was a problem hiding this comment.
Pull request overview
This PR introduces a new pkg/secrets package that provides configurable secret detection and redaction capabilities to prevent accidental leakage of sensitive data in logs and configuration dumps. The implementation includes pattern-based detection for common secret types (AWS keys, GitHub tokens, JWTs, etc.) and a flexible redactor that can process nested data structures.
Changes:
- Added secret detection with 8 built-in patterns and configurable options for custom patterns, max input length, and redaction masks
- Added redactor with support for nested structures, configurable sensitive keys, optional detector integration, and depth limits
- Added comprehensive error types and basic test coverage
- Updated documentation in README, usage guide, and security checklist
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/secrets/doc.go | Package documentation for the new secrets package |
| pkg/secrets/errors.go | Defines error types for configuration and detection failures |
| pkg/secrets/detect.go | Implements SecretDetector with pattern-based detection and redaction |
| pkg/secrets/redact.go | Implements Redactor for structured data with key-based and pattern-based redaction |
| pkg/secrets/secrets_test.go | Basic test coverage for detector and redactor functionality |
| docs/usage.md | Documentation for secret detection and redaction APIs |
| docs/security-checklist.md | Security guidance for using the secrets package |
| README.md | Example usage in the main README |
| cspell.json | Adds spelling exceptions for new terms |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| package secrets | ||
|
|
||
| import "testing" | ||
|
|
||
| func TestSecretDetectorDetectAny(t *testing.T) { | ||
| detector, err := NewSecretDetector() | ||
| if err != nil { | ||
| t.Fatalf("expected detector, got %v", err) | ||
| } | ||
|
|
||
| err = detector.DetectAny("AKIA1234567890ABCD12") | ||
| if err != ErrSecretDetected { | ||
| t.Fatalf("expected ErrSecretDetected, got %v", err) | ||
| } | ||
| } | ||
|
|
||
| func TestSecretDetectorRedact(t *testing.T) { | ||
| detector, err := NewSecretDetector() | ||
| if err != nil { | ||
| t.Fatalf("expected detector, got %v", err) | ||
| } | ||
|
|
||
| input := "token=ghp_abcdefghijklmnopqrstuvwxyz1234567890" | ||
| output, matches, err := detector.Redact(input) | ||
| if err != nil { | ||
| t.Fatalf("expected redacted, got %v", err) | ||
| } | ||
|
|
||
| if len(matches) == 0 { | ||
| t.Fatalf("expected matches, got none") | ||
| } | ||
|
|
||
| if output == input { | ||
| t.Fatalf("expected redacted output") | ||
| } | ||
| } | ||
|
|
||
| func TestRedactorKeys(t *testing.T) { | ||
| redactor, err := NewRedactor() | ||
| if err != nil { | ||
| t.Fatalf("expected redactor, got %v", err) | ||
| } | ||
|
|
||
| fields := map[string]any{ | ||
| "password": "secret", | ||
| "user": "alice", | ||
| } | ||
|
|
||
| redacted := redactor.RedactFields(fields) | ||
| if redacted["password"] == "secret" { | ||
| t.Fatalf("expected password redacted") | ||
| } | ||
|
|
||
| if redacted["user"] != "alice" { | ||
| t.Fatalf("expected user intact") | ||
| } | ||
| } | ||
|
|
||
| func TestRedactorDetector(t *testing.T) { | ||
| detector, err := NewSecretDetector() | ||
| if err != nil { | ||
| t.Fatalf("expected detector, got %v", err) | ||
| } | ||
|
|
||
| redactor, err := NewRedactor(WithRedactionDetector(detector)) | ||
| if err != nil { | ||
| t.Fatalf("expected redactor, got %v", err) | ||
| } | ||
|
|
||
| fields := map[string]any{ | ||
| "note": "token=ghp_abcdefghijklmnopqrstuvwxyz1234567890", | ||
| } | ||
|
|
||
| redacted := redactor.RedactFields(fields) | ||
| if redacted["note"] == fields["note"] { | ||
| t.Fatalf("expected detector to redact note") | ||
| } | ||
| } |
There was a problem hiding this comment.
The test suite lacks coverage for several important scenarios including: error conditions (ErrSecretInputTooLong, invalid configurations), edge cases (max depth limits, empty/nil values), the WithSecretPattern and WithSecretPatterns options, the WithSecretMaxLength and WithSecretMask options, nested structure redaction (maps within maps, slices within maps), and the WithRedactionKeys and WithRedactionMaxDepth options. These untested code paths represent significant functionality that could harbor bugs.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| return fields | ||
| } | ||
|
|
There was a problem hiding this comment.
The documentation for redactValue is missing. This is a core internal function that implements the redaction logic and has a boolean return value whose meaning is unclear. The function should have a comment explaining what the boolean return value indicates and the purpose of the depth and key parameters.
| // redactValue applies redaction logic to value and returns the (possibly) redacted value. | |
| // The depth parameter tracks the current recursion depth and is compared against maxDepth | |
| // to prevent unbounded traversal. The key parameter is the field or map key associated | |
| // with value and is used to determine whether key-based redaction should be applied. | |
| // The boolean return value is true when value was processed by the redaction logic | |
| // (for example, when it is a supported container or string type, or when key-based | |
| // masking was considered), and false when value was returned unchanged because it is | |
| // unsupported or beyond the configured maxDepth. |
| func (r *Redactor) redactStringValue(value string) string { | ||
| if r.opts.detector == nil { | ||
| return value | ||
| } | ||
|
|
||
| redacted, _, err := r.opts.detector.Redact(value) | ||
| if err == nil { | ||
| return redacted | ||
| } | ||
|
|
||
| return r.RedactString(value) | ||
| } |
There was a problem hiding this comment.
The redactStringValue function calls Redact on every string value when a detector is configured, even if no secrets are present. This involves running multiple regex patterns against every string in the data structure, which could be expensive for large datasets. Consider adding early exit conditions such as minimum string length checks or pattern pre-filtering to avoid unnecessary regex operations on strings that clearly don't contain secrets.
| cfg.patterns = append(cfg.patterns, SecretPattern{Name: name, Pattern: pattern}) | ||
|
|
There was a problem hiding this comment.
The WithSecretPattern function lacks validation for the name and pattern parameters. If either is an empty string, it will be caught later during compilation, but the error will be less clear. Adding validation here would provide better error messages to users. Additionally, there's no check to prevent duplicate pattern names, which could lead to confusion when analyzing SecretMatch results.
| cfg.patterns = append(cfg.patterns, SecretPattern{Name: name, Pattern: pattern}) | |
| trimmedName := strings.TrimSpace(name) | |
| if trimmedName == "" || pattern == "" { | |
| return ErrInvalidSecretConfig | |
| } | |
| for _, p := range cfg.patterns { | |
| if p.Name == trimmedName { | |
| return ErrInvalidSecretConfig | |
| } | |
| } | |
| cfg.patterns = append(cfg.patterns, SecretPattern{Name: trimmedName, Pattern: pattern}) |
| {Name: "slack-token", Pattern: `xox[baprs]-[A-Za-z0-9-]{10,}`}, | ||
| {Name: "google-api-key", Pattern: `AIza[0-9A-Za-z_-]{35}`}, | ||
| {Name: "stripe-secret", Pattern: `sk_live_[0-9a-zA-Z]{24,}`}, | ||
| {Name: "jwt", Pattern: `eyJ[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+`}, |
There was a problem hiding this comment.
The JWT pattern at line 296 is overly broad and may produce false positives. The pattern eyJ[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+ matches any string with three base64url-encoded segments, but doesn't enforce minimum lengths. This could match short arbitrary strings like "eyJ1.eyJ2.eyJ3". Consider adding minimum length constraints for each segment to reduce false positives, such as eyJ[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}.
| {Name: "jwt", Pattern: `eyJ[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+\.[a-zA-Z0-9_-]+`}, | |
| {Name: "jwt", Pattern: `eyJ[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}\.[a-zA-Z0-9_-]{10,}`}, |
| func WithRedactionKeys(keys ...string) RedactorOption { | ||
| return func(cfg *redactorOptions) error { | ||
| if len(keys) == 0 { | ||
| return ErrInvalidRedactorConfig | ||
| } | ||
|
|
||
| if cfg.keys == nil { | ||
| cfg.keys = make(map[string]struct{}) | ||
| } | ||
|
|
||
| for _, key := range keys { | ||
| value := normalizeRedactionKey(key) | ||
| if value == "" { | ||
| continue | ||
| } | ||
|
|
||
| cfg.keys[value] = struct{}{} | ||
| } | ||
|
|
||
| if len(cfg.keys) == 0 { | ||
| return ErrInvalidRedactorConfig | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
The WithRedactionKeys option has incomplete validation that could lead to unexpected behavior. If all provided keys normalize to empty strings (e.g., all whitespace), the function returns ErrInvalidRedactorConfig at line 87. However, if cfg.keys already has entries from default keys, the validation passes even though no new keys were actually added. This inconsistency in behavior based on initialization state could confuse users.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
- Introduces .gitleaksignore to suppress a known false positive from gitleaks. - Allowlisted entry: ae0ffc5:pkg/secrets/secrets_test.go:aws-access-token:11 - Prevents CI/security scans from failing while preserving realistic test data. - No production code changes; this only adjusts security tooling configuration.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Introduce pkg/secrets with configurable detectors and a redactor to prevent accidental leakage of sensitive data in logs/configs.
Refs: introduce NewSecretDetector, NewRedactor and related options.