fix possible leakage of secrets in debug logging#5973
fix possible leakage of secrets in debug logging#5973drornir wants to merge 2 commits intonektos:masterfrom
Conversation
ChristopherHX
left a comment
ChristopherHX
left a comment
There was a problem hiding this comment.
this does not look right, the printableInput might prevent leakage that you hardcoded in yaml.
However if the workflow content contains a secret than it is no longer secret
e.g.
- run: |
echo ::add-mask::${{ vars.SOMETHING_YOU_MIGHT_WANTED_TO_HIDE }}
pkg/runner/expression.go
Outdated
|
|
||
| printable := regexp.MustCompile(`::add-mask::.*`).ReplaceAllString(fmt.Sprintf("%t", evaluated), "::add-mask::***)") | ||
| logger.Debugf("expression '%s' evaluated to '%s'", in, printable) | ||
| logger.Debugf("expression '%s' evaluated to '%t'", printableInput, evaluated) |
There was a problem hiding this comment.
evaluated being the text of run: , not only true or false
There was a problem hiding this comment.
Afaik '%t' converts it to a boolean.
Would you rather I masked it and printed it out as string?
There was a problem hiding this comment.
Yeah I realized it would print the string with an %!t(string=..., fixed
ChristopherHX
left a comment
ChristopherHX
left a comment
There was a problem hiding this comment.
I am unsure when merges become possible again: blocked by #5944
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5973 +/- ##
==========================================
+ Coverage 74.65% 76.72% +2.06%
==========================================
Files 73 73
Lines 11139 9203 -1936
==========================================
- Hits 8316 7061 -1255
+ Misses 2186 1507 -679
+ Partials 637 635 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Found this while browsing the source code.
I just thought it was odd to to a regex replace on the string 'true' or 'false' so it caught my attention.
I know it's not an important fix but I was here so why not.
reference to the commit that originally changed this:
4391a10#diff-4fe45e900ed33a3395bb42c8d8a85c1afb94987b2368bfe31d62e9e1ebd69060R124