[#2087] Mask parameters in trace log when echo=false for interactive …

…options

RELEASE-NOTES.md

@@ -22,9 +22,10 @@ Artifacts in this release are signed by Remko Popma (6601 E5C0 8DCC BB96).
 
 
 ## <a name="4.7.5-fixes"></a> Fixed issues
+* [#2083][#2084] Enhancement: Java 22 update: improve logic for detecting if the output stream is connected to a terminal. Thanks to [Liam Miller-Cushon](https://github.com/cushon) for the pull request.
+* [#2087] Enhancement: Mask parameters in trace log when `echo=false` for `interactive` options and positional parameters. Thanks to [szzsolt](https://github.com/szzsolt) for raising this.
 * [#2060] Bugfix: Fix positional parameters bug with late-resolved arity variable. Thanks to [daisukeoto](https://github.com/daisukeoto) for raising this.
 * [#2074][#2075] Bugfix: Don't generate auto-complete for hidden attributes in `picocli.shell.jline3.PicoCommand`. Thanks to [clebertsuconic](https://github.com/clebertsuconic) for the pull request.
-* [#2083][#2084] Enhancement: Java 22 update: improve logic for detecting if the output stream is connected to a terminal. Thanks to [Liam Miller-Cushon](https://github.com/cushon) for the pull request.
 * [#2059] Bugfix: ArgGroup with `exclusive=false` and `multiplicity=1` should require at least one option; fix regression and refine solution introduced in [#1848][#2030].  Thanks to [Utkarsh Mittal](https://github.com/utmittal) for raising this.
 * [#2080] DOC: Improve GraalVM documentation: add `graalvm-native-image-plugin`. Thanks to [Bhavik Patel](https://github.com/bhavikp19) for the pull request.
 * [#2045] DOC: Commit html files with LF line-endings. Thanks to [Fridrich Strba](https://github.com/fridrich) for the pull request.

src/main/java/picocli/CommandLine.java

@@ -13346,6 +13346,8 @@ String toString(String separator) {
      * Helper class responsible for processing command line arguments.
      */
     private class Interpreter {
+        /** Value displayed in trace logs for options with echo=false. */
+        private static final String MASKED_VALUE = "*****(masked)"; // see #2087
         private final Map<Class<?>, ITypeConverter<?>> converterRegistry = new HashMap<Class<?>, ITypeConverter<?>>();
         private boolean isHelpRequested;
         private int position;
@@ -13707,10 +13709,11 @@ private boolean applyDefault(IDefaultValueProvider defaultValueProvider, ArgSpec
             String defaultValue = fromProvider == null ? arg.defaultValue() : arg.interpolate(fromProvider);
             String provider = defaultValueProvider == null ? "" : (" from " + defaultValueProvider.toString());
 
+            String displayDefaultVal = arg.echo() ? defaultValue : MASKED_VALUE;
             Tracer tracer = CommandLine.tracer();
             if (defaultValue != null && !ArgSpec.NULL_VALUE.equals(defaultValue)) {
                 if (tracer.isDebug()) {
-                    tracer.debug("Applying defaultValue (%s)%s to %s on %s", defaultValue, provider, arg, arg.scopeString());}
+                    tracer.debug("Applying defaultValue (%s)%s to %s on %s", displayDefaultVal, provider, arg, arg.scopeString());}
                 Range arity = arg.arity().min(Math.max(1, arg.arity().min));
                 applyOption(arg, false, LookBehind.SEPARATE, false, arity, stack(defaultValue), new HashSet<ArgSpec>(), arg.toString);
                 arg.valueIsDefaultValue = true;
@@ -13727,7 +13730,7 @@ private boolean applyDefault(IDefaultValueProvider defaultValueProvider, ArgSpec
                     if (ArgSpec.NULL_VALUE.equals(arg.originalDefaultValue)) {
                         defaultValue = null;
                         if (tracer.isDebug()) {
-                            tracer.debug("Applying defaultValue (%s)%s to %s on %s", defaultValue, provider, arg, arg.scopeString());}
+                            tracer.debug("Applying defaultValue (%s)%s to %s on %s", displayDefaultVal, provider, arg, arg.scopeString());}
                         arg.setValue(defaultValue);
                         arg.valueIsDefaultValue = true;
                         return true;
@@ -14275,13 +14278,8 @@ private int applyValueToSingleValuedField(ArgSpec argSpec,
             } else {
                 consumed = 1;
                 if (interactiveValue != null) {
-                    if (argSpec.echo()) {
-                        initValueMessage = "Setting %s to %3$s (interactive value) for %4$s on %5$s";
-                        overwriteValueMessage = "Overwriting %s value with %3$s (interactive value) for %s on %5$s";
-                    } else {
-                        initValueMessage = "Setting %s to *** (masked interactive value) for %4$s on %5$s";
-                        overwriteValueMessage = "Overwriting %s value with *** (masked interactive value) for %s on %5$s";
-                    }
+                    initValueMessage = "Setting %s to %3$s (interactive value) for %4$s on %5$s";
+                    overwriteValueMessage = "Overwriting %s value with %3$s (interactive value) for %s on %5$s";
                 }
                 // #1642 Negatable options should negate explicit values
                 if ((cls == Boolean.class || cls == Boolean.TYPE) && arity.min >= 1) {
@@ -14321,7 +14319,8 @@ private int applyValueToSingleValuedField(ArgSpec argSpec,
             if (argSpec.typeInfo().isOptional()) {
                 newValue = getOptionalOfNullable(newValue);
             }
-            if (tracer.isInfo()) { tracer.info(traceMessage, argSpec.toString(), String.valueOf(oldValue), String.valueOf(newValue), argDescription, argSpec.scopeString()); }
+            String displayVal = !argSpec.interactive() || argSpec.echo() ? String.valueOf(newValue) : MASKED_VALUE;
+            if (tracer.isInfo()) { tracer.info(traceMessage, argSpec.toString(), String.valueOf(oldValue), displayVal, argDescription, argSpec.scopeString()); }
             int pos = getPosition(argSpec);
             argSpec.setValue(newValue);
             parseResultBuilder.addOriginalStringValue(argSpec, actualValue);// #279 track empty string value if no command line argument was consumed
@@ -14429,7 +14428,8 @@ private void consumeOneMapArgument(ArgSpec argSpec,
                 String rawMapValue = keyValue.length == 1 ? argSpec.mapFallbackValue() : keyValue[1];
                 Object mapValue = tryConvert(argSpec, index, valueConverter, rawMapValue, 1);
                 result.put(mapKey, mapValue);
-                if (tracer.isInfo()) { tracer.info("Putting [%s : %s] in %s<%s, %s> %s for %s on %s", String.valueOf(mapKey), String.valueOf(mapValue),
+                String displayVal = !argSpec.interactive() || argSpec.echo() ? String.valueOf(mapValue) : MASKED_VALUE;
+                if (tracer.isInfo()) { tracer.info("Putting [%s : %s] in %s<%s, %s> %s for %s on %s", String.valueOf(mapKey), displayVal,
                         result.getClass().getSimpleName(), classes[0].getSimpleName(), classes[1].getSimpleName(), argSpec.toString(), argDescription, argSpec.scopeString()); }
                 parseResultBuilder.addStringValue(argSpec, keyValue[0]);
                 parseResultBuilder.addStringValue(argSpec, rawMapValue);
@@ -14700,7 +14700,8 @@ private int consumeOneArgument(ArgSpec argSpec,
                 Object stronglyTypedValue = tryConvert(argSpec, index, converter, value, 0);
                 result.add(stronglyTypedValue);
                 if (tracer().isInfo()) {
-                    tracer().info("Adding [%s] to %s for %s on %s", String.valueOf(result.get(result.size() - 1)), argSpec.toString(), argDescription, argSpec.scopeString());
+                    String displayVal = !argSpec.interactive() || argSpec.echo() ? String.valueOf(stronglyTypedValue) : MASKED_VALUE;
+                    tracer().info("Adding [%s] to %s for %s on %s", displayVal, argSpec.toString(), argDescription, argSpec.scopeString());
                 }
                 parseResultBuilder.addStringValue(argSpec, value);
             }
@@ -14720,7 +14721,7 @@ private boolean canConsumeOneArgument(ArgSpec argSpec, LookBehind lookBehind, bo
                     tryConvert(argSpec, -1, converter, value, 0);
                 }
                 return true;
-            } catch (PicocliException ex) {
+            } catch (PicocliException ex) { // Note: we don't mask hidden (echo=false) options to facilitate debugging
                 tracer().debug("%s cannot be assigned to %s: type conversion fails: %s.", arg, argDescription, ex.getMessage());
                 return false;
             }

src/test/java/picocli/InteractiveArgTest.java

@@ -63,7 +63,7 @@ class App {
             String trace = errBaos.toString();
             assertThat(trace, containsString("User entered 10 characters"));
             assertThat(trace, containsString(
-                "Setting " + specX.toString() + " to *** (masked interactive value)"));
+                "Setting " + specX.toString() + " to *****(masked) (interactive value)"));
             assertThat(trace, not(containsString("1234567890")));
 
             cmd.parseArgs("-z", "678");
@@ -607,7 +607,7 @@ class App {
             String trace = errBaos.toString();
             assertThat(trace, containsString("User entered 10 characters"));
             assertThat(trace, containsString(
-                "Setting " + specX.toString() + " to *** (masked interactive value)"));
+                "Setting " + specX.toString() + " to *****(masked) (interactive value)"));
             assertThat(trace, not(containsString("1234567890")));
         } finally {
             System.setOut(out);