Copilot commented on code in PR #12028:
URL: https://github.com/apache/cloudstack/pull/12028#discussion_r2517441745
##########
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java:
##########
@@ -67,11 +67,13 @@ String removeCommandSensitiveInfoForLogging(String command)
{
public ProcessRunner(ExecutorService executor) {
this.executor = executor;
commandLogReplacements.add(new Ternary<>("ipmitool", "-P\\s+\\S+", "-P
*****"));
+ commandLogReplacements.add(new Ternary<>("ipmitool",
"(?i)(password)(\\s+)\\S+(\\s+)\\S+", "$1 ****$2****"));
Review Comment:
The regex replacement pattern is incorrect. The pattern has 3 capturing
groups: (1) 'password', (2) whitespace after 'password', (3) whitespace after
userId. The replacement `$1 ****$2****` produces 'password ****<space>****'
instead of the expected 'password **** ****'. Change the replacement to `$1
**** ****` with a literal space between the asterisks.
```suggestion
commandLogReplacements.add(new Ternary<>("ipmitool",
"(?i)(password)(\\s+)\\S+(\\s+)\\S+", "$1 **** ****"));
```
##########
utils/src/main/java/org/apache/cloudstack/utils/process/ProcessRunner.java:
##########
@@ -109,14 +112,16 @@ public Integer call() throws Exception {
}
});
try {
- logger.debug("Waiting for a response from command [{}].
Defined timeout: [{}].", commandLog, timeOut.getStandardSeconds());
+ logger.debug("Waiting for a response from command [{}].
Defined timeout: [{}].", commandLog,
+ timeOut.getStandardSeconds());
retVal = processFuture.get(timeOut.getStandardSeconds(),
TimeUnit.SECONDS);
} catch (ExecutionException e) {
- logger.warn("Failed to complete the requested command [{}] due
to execution error.", commands, e);
+ logger.warn("Failed to complete the requested command [{}] due
to execution error.", commandLog, e);
Review Comment:
The log message references `commandLog` but should reference `commands` to
match the error message context. This change introduces a regression where the
original command list context is lost. The sensitive information should already
be removed via `removeCommandSensitiveInfoForLogging`, so using `commandLog`
here is correct. However, this represents a change from logging the raw command
list to logging the sanitized string, which alters the log output format.
--
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]
