-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix possible leakage of secrets in debug logging #5973
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
ChristopherHX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
evaluated being the text of run: , not only true or false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I realized it would print the string with an %!t(string=..., fixed
ChristopherHX
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
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