Improve secret filter #144
Conversation
…stency. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
📝 WalkthroughWalkthroughAdd per-instance secret-detection regex to Config, thread it through overlay/merge and secret-hiding, update HideSecrets signature to accept a regexp, add tests and docs, update CI workflow YAML formatting and indirect go.mod deps, and bump SPDX header years across many files. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Config as Config
participant Merge as MergeLogic
participant Hider as HideSecrets
participant Detector as SecretDetector
User->>Config: SetSecretRE(customRE)
Config-->>Config: store secretRE
User->>Config: Overlay(otherConfig)
Config->>Merge: pass secretRE
Merge-->>Config: merged Config (with secretRE)
User->>Config: ToSecretsHidden(node)
Config->>Hider: HideSecrets(node, hideStructure, secretRE)
Hider->>Detector: match secrets using secretRE
Detector-->>Hider: returns matches
Hider-->>Config: node with secrets hidden
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI agents
In `@config_test.go`:
- Line 1: Add a unit test that covers the new SetSecretRE method: create or
reuse a config instance, call SetSecretRE with a specific regex (e.g., a pattern
matching "MYSECRET" or a custom key), then exercise the same secret-hiding logic
used by TestSecretsHidden/TestSecretsHiddenStructured to assert that values
matching the custom regex are masked while non-matching values are unchanged;
name the new test (e.g., TestSetSecretRE) and mirror the existing test structure
and assertions so it verifies SetSecretRE actually alters the masking behavior.
In `@README.md`:
- Around line 430-433: The code example has a missing assignment operator: when
calling templig.FromFile[Config] you must capture the returned value(s) using
:=; update the example so the variable c is declared via short assignment from
templig.FromFile[Config] (and still call c.SetSecretRE with
regexp.MustCompile(templig.SecretDefaultRE + "|token")), ensuring the call site
matches the templig.FromFile[Config] return signature.
In `@secrets.go`:
- Around line 27-29: The comment for HideSecrets incorrectly references
[SecretRE] even though the regex is now passed in; update the doc comment to say
that secrets are identified by the provided regexp parameter (secretRE) supplied
by the caller. Also make HideSecrets nil-safe by checking if secretRE is nil
before any use (e.g., before calling secretRE.MatchString in the function body)
and handle nil by either returning early or treating it as "no matches" to avoid
a panic; ensure references to the function name HideSecrets and the parameter
secretRE are used to locate where to add the nil-check and doc update.
🧹 Nitpick comments (3)
CHANGELOG.md (1)
7-9: Minor: Inconsistent terminology (regex vs regexp).Line 7 uses "regex" while line 9 uses "regexp". Consider using consistent terminology throughout.
📝 Suggested fix
-- introduced `Config.SetSecretRE` to allow users to define their own secret regex +- introduced `Config.SetSecretRE` to allow users to define their own secret regexp per instanceOr alternatively, change "regexp" to "regex" on line 9.
secrets_test.go (1)
272-272: Tests correctly updated for new signature; consider adding custom regex test coverage.The test updates correctly pass
templig.SecretREto match the newHideSecretssignature. However, the PR introduces per-instance secret regex customization, but no tests exercise this feature with a custom regex pattern.Consider adding a test case that verifies
HideSecretsworks correctly with a non-default regex pattern to ensure the per-instance functionality is properly covered.Also applies to: 293-293, 318-318
config.go (1)
314-318: Consider adding nil-safety or documentation forSetSecretRE.The
SetSecretREmethod allows settingnil, which would cause a panic whenHideSecretsis later called (sincesecretRE.MatchStringwould be invoked on nil). Consider either:
- Adding a nil check that falls back to the default
SecretRE, or- Documenting that
nilis not a valid inputOption 1: Add nil-safety
// SetSecretRE sets the regular expression to be used for hiding secrets of that specific instance. +// If re is nil, the default [SecretRE] is used. func (c *Config[T]) SetSecretRE(re *regexp.Regexp) { + if re == nil { + re = SecretRE + } c.secretRE = re }
Signed-off-by: Alexander Adam <alphaone23@gmail.com>
…ult logic. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config.go`:
- Around line 322-328: SecretRE on Config[T] dereferences c.secretRE without
guarding for nil which can panic for zero-valued or uninitialized configs;
modify the SecretRE method to check if c.secretRE == nil and either return nil
(or initialize a sensible default regexp) instead of dereferencing, and update
callers or the method's docstring to reflect the nil-return behavior (or that it
lazily initializes) so callers like any code invoking Config.SecretRE know to
handle a nil result.
🧹 Nitpick comments (2)
secrets.go (1)
27-39: Consider reusing the pre-compiledSecretREinstead of compiling a new regex.When
secretREis nil, the code compiles a new regex fromSecretDefaultRE. However,SecretRE(line 20) is already a pre-compiled version of this same pattern. Reusing it avoids redundant compilation overhead.♻️ Suggested optimization
if secretRE == nil { - secretRE = regexp.MustCompile(SecretDefaultRE) + secretRE = SecretRE }config.go (1)
188-190: Documentation refers to global[SecretRE]but per-instance regex is now used.The doc comment mentions hiding secrets "using the
[SecretRE]", but the implementation now uses the per-instancec.secretRE. Consider updating the documentation to reflect this change (e.g., "using the configured secret regex").
…ng behavior for unset values. Signed-off-by: Alexander Adam <alphaone23@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@config.go`:
- Around line 323-333: The SecretRE method currently creates a value copy of
c.secretRE (tmp := *c.secretRE), which duplicates the regexp's internal mutex;
update SecretRE to return a safe copy by calling c.secretRE.Copy() instead,
i.e., replace the value-copy logic in the SecretRE method to use
c.secretRE.Copy() and return that result (handling the nil case as before) to
avoid mutex duplication and deadlocks.
The detection of secrets can now be configured on a per-instance basis rather than globally.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.