fix: use exact key matching in report sanitize to prevent false redaction#2636
Open
saiashok0981 wants to merge 2 commits into
Open
fix: use exact key matching in report sanitize to prevent false redaction#2636saiashok0981 wants to merge 2 commits into
saiashok0981 wants to merge 2 commits into
Conversation
…tion
Problem
-------
The `sanitize()` function in `report.go` protects sensitive values in
`pack report` output by redacting quoted strings on matching TOML field
lines. However, it uses `strings.HasPrefix` to detect field names, which
causes false-positive matches on any key that merely *starts with* one of
the sensitive names.
For example, the sensitive field list includes "image" and "name".
A future config key such as "image-mirror = ..." or "name-prefix = ..."
would unintentionally have its value redacted, because "image-mirror"
has the prefix "image".
Additionally, the redaction regex (`"(.*?)"`) is compiled fresh inside
`sanitize()` on every line, even though it is a constant expression.
Repeated compilation is wasteful when `pack report` processes a config
file with many lines.
Fix
---
- Replace the `strings.HasPrefix` check with a field-anchored regex that
requires the match to be `<field-name>` followed immediately by
optional whitespace and `=`. This ensures only the exact TOML key is
matched, not keys that happen to share a prefix.
- Pre-compile both the value-redaction regex and the field-match helper
at package initialisation time so they are compiled exactly once.
The new pattern for each sensitive field is:
`^<field>\s*=`
This is consistent with TOML syntax, where `key = value` and
`key=value` are both valid.
Signed-off-by: [email protected]
jjbustamante
left a comment
Member
There was a problem hiding this comment.
Thanks for this contribution. Would you mind fixing the DCO check and also adding test coverage to reproduce the behavior? This looks like a bug, if the bug doesn't exist, you can also create it and link this PR to that fix
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The
sanitize()function inreport.goprotects sensitive values inpack reportoutput by redacting quoted strings on matching TOML field lines. However, it usesstrings.HasPrefixto detect field names, which causes false-positive matches on any key that merely starts with one of the sensitive names.For example, the sensitive field list includes "image" and "name". A future config key such as "image-mirror = ..." or "name-prefix = ..." would unintentionally have its value redacted, because "image-mirror" has the prefix "image".
Additionally, the redaction regex (
"(.*?)") is compiled fresh insidesanitize()on every line, even though it is a constant expression. Repeated compilation is wasteful whenpack reportprocesses a config file with many lines.Fix
strings.HasPrefixcheck with a field-anchored regex that requires the match to be<field-name>followed immediately by optional whitespace and=. This ensures only the exact TOML key is matched, not keys that happen to share a prefix.The new pattern for each sensitive field is:
^<field>\s*=This is consistent with TOML syntax, where
key = valueandkey=valueare both valid.Signed-off-by: [email protected]
Summary
Output
Before
After
Documentation
Related
Resolves #___