From ef572fbd7117eef716a8c6dc4725c9e63b9d8891 Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Wed, 23 Sep 2026 21:23:16 +0530 Subject: [PATCH 1/2] fix property value leak in DefaultParser.handleProperties handleProperties applied the Properties value to the Option registered in Options instead of the copy that handleOption adds to the CommandLine, so the value survived the parse() call and every later parse() on the same Options reused it, even when the option was given on the command line. --- .../org/apache/commons/cli/DefaultParser.java | 13 +++++++------ .../commons/cli/AbstractParserTestCase.java | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 6 deletions(-) diff --git a/src/main/java/org/apache/commons/cli/DefaultParser.java b/src/main/java/org/apache/commons/cli/DefaultParser.java index 4dd7dfec2..9643623ff 100644 --- a/src/main/java/org/apache/commons/cli/DefaultParser.java +++ b/src/main/java/org/apache/commons/cli/DefaultParser.java @@ -505,16 +505,17 @@ private void handleProperties(final Properties properties) throws ParseException if (!cmd.hasOption(option) && !selected) { // get the value from the properties final String value = properties.getProperty(option); - if (opt.hasArg()) { - if (opt.isValuesEmpty()) { - opt.processValue(stripLeadingAndTrailingQuotesDefaultOff(value)); - } - } else if (!("yes".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value) || "1".equalsIgnoreCase(value))) { + if (!opt.hasArg() && !("yes".equalsIgnoreCase(value) || "true".equalsIgnoreCase(value) || "1".equalsIgnoreCase(value))) { // if the value is not yes, true or 1 then don't add the option to the CommandLine continue; } + // handleOption() adds a copy of the option to the CommandLine; apply the value to that copy, not to the + // option registered in options, otherwise the value survives this parse() call and is reused by the next one handleOption(opt); - currentOption = null; + if (currentOption != null) { + currentOption.processValue(stripLeadingAndTrailingQuotesDefaultOff(value)); + currentOption = null; + } } } } diff --git a/src/test/java/org/apache/commons/cli/AbstractParserTestCase.java b/src/test/java/org/apache/commons/cli/AbstractParserTestCase.java index 58657821a..57371a448 100644 --- a/src/test/java/org/apache/commons/cli/AbstractParserTestCase.java +++ b/src/test/java/org/apache/commons/cli/AbstractParserTestCase.java @@ -667,6 +667,23 @@ void testReuseOptionsTwice() throws Exception { assertThrows(MissingOptionException.class, () -> parser.parse(options, new String[0])); } + @Test + void testReuseOptionsWithProperties() throws Exception { + final Options options = new Options(); + options.addOption(OptionBuilder.hasArg().create('a')); + // first parsing, the value comes from the properties + final Properties properties = new Properties(); + properties.setProperty("a", "one"); + assertEquals("one", parse(parser, options, null, properties).getOptionValue("a")); + // second parsing, with the same Options instance and the value on the command line + final CommandLine cmd = parser.parse(options, new String[] { "-a", "two" }); + assertEquals("two", cmd.getOptionValue("a")); + assertEquals(0, cmd.getArgList().size()); + // third parsing, with the same Options instance and another default value + properties.setProperty("a", "three"); + assertEquals("three", parse(parser, options, null, properties).getOptionValue("a")); + } + @Test void testShortOptionConcatenatedQuoteHandling() throws Exception { final CommandLine cl = parser.parse(options, new String[] { "-b\"quoted string\"" }); From 67cb328ae6cc391122ad3742c8295c36bf06e9ea Mon Sep 17 00:00:00 2001 From: Naveed Khan Date: Mon, 26 Oct 2026 18:21:45 +0530 Subject: [PATCH 2/2] keep the isValuesEmpty guard when applying a property value Only apply the Properties value to the cloned option when it has no value yet, so an option registered with a preset value keeps it instead of failing with 'Cannot add value, list full.' --- .../java/org/apache/commons/cli/DefaultParser.java | 4 ++-- .../org/apache/commons/cli/DefaultParserTest.java | 12 ++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/cli/DefaultParser.java b/src/main/java/org/apache/commons/cli/DefaultParser.java index 9643623ff..86034e46f 100644 --- a/src/main/java/org/apache/commons/cli/DefaultParser.java +++ b/src/main/java/org/apache/commons/cli/DefaultParser.java @@ -512,10 +512,10 @@ private void handleProperties(final Properties properties) throws ParseException // handleOption() adds a copy of the option to the CommandLine; apply the value to that copy, not to the // option registered in options, otherwise the value survives this parse() call and is reused by the next one handleOption(opt); - if (currentOption != null) { + if (currentOption != null && currentOption.isValuesEmpty()) { currentOption.processValue(stripLeadingAndTrailingQuotesDefaultOff(value)); - currentOption = null; } + currentOption = null; } } } diff --git a/src/test/java/org/apache/commons/cli/DefaultParserTest.java b/src/test/java/org/apache/commons/cli/DefaultParserTest.java index c162323f0..d1734dc08 100644 --- a/src/test/java/org/apache/commons/cli/DefaultParserTest.java +++ b/src/test/java/org/apache/commons/cli/DefaultParserTest.java @@ -23,6 +23,7 @@ Licensed to the Apache Software Foundation (ASF) under one or more import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.HashSet; +import java.util.Properties; import java.util.Set; import java.util.stream.Stream; @@ -400,6 +401,17 @@ void testParseSkipNonHappyPath() throws ParseException { assertTrue(e.getMessage().contains("-d")); } + @Test + void testPropertyDoesNotOverridePresetValue() throws ParseException { + final Option a = Option.builder("a").hasArg().get(); + a.getValuesList().add("preset"); + final Options options = new Options().addOption(a); + final Properties properties = new Properties(); + properties.setProperty("a", "fromProperties"); + assertEquals("preset", new DefaultParser().parse(options, null, properties).getOptionValue("a")); + assertEquals("[preset]", a.getValuesList().toString()); + } + @Override @Test @Disabled("Test case handled in the parameterized tests as \"DEFAULT behavior\"")