-
Notifications
You must be signed in to change notification settings - Fork 14
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
Process: add sensitive option to prevent verbose reporting of the command #256
Conversation
src/main/perl/Process.pm
Outdated
|
||
A boolean specifying whether the command (and args) contain sensitive information | ||
(like passwords). If C<sensitive> is true, the commandline will not be reported | ||
(by default the commanlione is reported in with verbose). |
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.
commanlione => command line. Also "by default" do you mean only if the log
option is not undef
? Or do you really mean "in all cases unless you set this" ?
src/main/perl/Process.pm
Outdated
(like passwords). If C<sensitive> is true, the commandline will not be reported | ||
(by default the commanlione is reported in with verbose). | ||
|
||
This does not cover command output. If th eoutput (stdout or stderr) contains |
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.
the output
src/main/perl/Process.pm
Outdated
(by default the commanlione is reported in with verbose). | ||
|
||
This does not cover command output. If th eoutput (stdout or stderr) contains | ||
sensitve information, make sure to handle it yourself via C<stdout> and/or C<stderr> |
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.
Should reference the log
option here? (i.e. recommend setting it to undef?) Generally I am a bit confused as to how this interacts with the log option so it would be good to see that clarified.
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'm not sure how to formulate it. the code here does not report the output, but eg execute
sends it to the real stdout (and that's also why it's not part of the logs, see #245).
@ned21 i made some changes to address your remarks, but also changed that the sensitive option only applies ot the arguments. the binary itself can be reported by some other methods also (and i'm not sure the binary itself is ever sensitive) |
src/main/perl/Process.pm
Outdated
|
||
A boolean specifying whether the arguments contain sensitive information | ||
(like passwords). If C<sensitive> is true, the commandline will not be reported | ||
(by default and when C<log> option is used, the commandline is reported |
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.
Sorry to nitpick but I think by default when the C<log> option is used
is clearer.
Thanks for the changes. All good except one minor nitpick about removing what I believe to be an extraneous "and". Then you can squash. |
74bc4c8
to
7928652
Compare
@ned21 should be fine now |
@ned21 are you happy with this now? |
Fixes #254