Copilot commented on code in PR #12024:
URL: https://github.com/apache/cloudstack/pull/12024#discussion_r2514521014
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -437,11 +469,8 @@ public String executeIgnoreExitValue(OutputInterpreter
interpreter, int exitValu
Task timedoutTask = new Task(log, ir);
timedoutTask.run();
- if (!_passwordCommand) {
- _logger.warn(String.format("Timed out: %s. Output is:
%s", buildCommandLine(command), timedoutTask.getResult()));
- } else {
- _logger.warn(String.format("Timed out: %s",
buildCommandLine(command)));
- }
+ _logger.warn(String.format("Timed out: %s. Output is: %s",
commandLine,
+ timedoutTask.getResult()));
Review Comment:
The timeout handling in `executeIgnoreExitValue` should respect the
`_passwordCommand` flag to avoid logging sensitive information. The original
code checked `_passwordCommand` before logging the output on timeout, but this
change always logs the output regardless. This could expose sensitive data when
a password command times out.
Consider restoring the conditional logic:
```java
if (_passwordCommand) {
_logger.warn(String.format("Timed out: %s", commandLine));
} else {
_logger.warn(String.format("Timed out: %s. Output is: %s", commandLine,
timedoutTask.getResult()));
}
```
```suggestion
if (_passwordCommand) {
_logger.warn(String.format("Timed out: %s",
commandLine));
} else {
_logger.warn(String.format("Timed out: %s. Output is:
%s", commandLine, timedoutTask.getResult()));
}
```
##########
utils/src/main/java/com/cloud/utils/script/Script.java:
##########
@@ -360,9 +390,11 @@ public String execute(OutputInterpreter interpreter) {
public String executeIgnoreExitValue(OutputInterpreter interpreter, int
exitValue) {
String[] command = _command.toArray(new String[_command.size()]);
+ String commandLine = buildCommandLine(command);
- if (_logger.isDebugEnabled()) {
- _logger.debug(String.format("Executing: %s",
buildCommandLine(command).split(KeyStoreUtils.KS_FILENAME)[0]));
+ if (_logger.isDebugEnabled() && !avoidLoggingCommand) {
+ _logger.debug(
+ String.format("Executing: %s",
commandLine.split(KeyStoreUtils.KS_FILENAME)[0]));
Review Comment:
Inconsistent logging behavior between `execute` and `executeIgnoreExitValue`
methods. The `execute` method (line 249) removed the `avoidLoggingCommand`
check since `buildCommandLine()` now properly masks sensitive data, but
`executeIgnoreExitValue` still has this check. Additionally, the
`split(KeyStoreUtils.KS_FILENAME)[0]` may no longer be necessary since
sensitive arguments are now masked by `buildCommandLine()`.
Consider making the logging consistent with the `execute` method:
```java
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Executing: %s", commandLine));
}
```
```suggestion
if (_logger.isDebugEnabled()) {
_logger.debug(String.format("Executing: %s", commandLine));
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
