-
Notifications
You must be signed in to change notification settings - Fork 958
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
add-mask is leaking a secret in master if debug or ::echo::on is set #158
add-mask is leaking a secret in master if debug or ::echo::on is set #158
Conversation
} | ||
|
||
context.Error($"Unable to process command '{input}' successfully."); | ||
var commandInformation = extension.OmitEcho ? extension.Command : input; |
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.
if the omit echo command crashes or otherwise fails, we should only log the command name not the entire command
@@ -404,6 +405,7 @@ private static class RemoveMatcherCommandProperties | |||
public sealed class DebugCommandExtension : RunnerService, IActionCommandExtension | |||
{ | |||
public string Command => "debug"; | |||
public bool OmitEcho => 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.
Debug, Error, and Warning could be a candidate for ignoring echos (as they did previously), but they are commands and I feel like they should have both their command echo and their outputs appear in the logs.
The ADR specified all commands would work the same way, but we are breaking that for add-mask at the moment due to the security concerns.
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.
@bryanmacfarlane to weigh in
We should skip echo for debug, otherwise every debug statement gets written twice. It hinders debugging because the log is noisy and harder to read.
I would follow the same pattern for warning/error. Otherwise the message is written to the log twice. Looks silly.
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 agree with Eric
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.
@juliobbv we will need to amend the ADR for consistency sake, we can link this discussion
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.
@ericsciple , this has been updated, can you take a look?
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.
@thboop I'll amend the ADR to reflect the add-mask
, issue-command
, and debug
command logging behavior changes.
8b920aa
to
28ea219
Compare
Let's make sure this goes to master as well |
…158) * Output after processing command to avoid leaking mask * Remove extra noise output from echo changes * Omit Echoing of add-mask command * avoid echoing on debug/warning/error
…ctions#158) * Output after processing command to avoid leaking mask * Remove extra noise output from echo changes * Omit Echoing of add-mask command * avoid echoing on debug/warning/error
If debug variable is set or echo::on is used, add-mask will echo before the secret is registered and it will leak the secret. This bug did not ship and was caught in the release branch
Resolves #159, #157