Skip to content

Reject an empty option name in getMatchingOptions#434

Open
farkhalit wants to merge 1 commit into
apache:masterfrom
farkhalit:empty-longopt-match
Open

Reject an empty option name in getMatchingOptions#434
farkhalit wants to merge 1 commit into
apache:masterfrom
farkhalit:empty-longopt-match

Conversation

@farkhalit

Copy link
Copy Markdown

Repro: --=/etc/shadow parsed against an Options that holds a single long option --config-file. Both DefaultParser and PosixParser return a CommandLine with config-file set to /etc/shadow, and an empty argument list. Nothing on the command line named that option.

Cause: getMatchingOptions strips the hyphens off --, leaving an empty name, and longOpt.startsWith("") holds for every entry, so an empty name matches the entire long option table. handleLongOptionWithEqual splits --=V into opt -- and value V, sees one match, and binds. PosixParser.flatten reaches the same list from its own -- branch. With more than one long option defined the same token raises AmbiguousOptionException naming all of them rather than rejecting it, and isLongOption classifies -= as an option, so -= cannot be passed as a value to the option before it.

Fix: an empty name is not a partial name, so return no matches for it. Both parsers share that one lookup, and --=V now reaches handleUnknownToken like any other token that names no option.

What this costs an application is the assumption that a long option is only set when its name appears in argv. A launcher or wrapper that inspects argv and refuses, say, --config-file sees --=... name nothing, while the parser behind it binds the value anyway.

  • Read the contribution guidelines for this project.
  • Read the ASF Generative Tooling Guidance if you use Artificial Intelligence (AI).
  • I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?
  • Run a successful build using the default Maven goal with mvn; that's mvn on the command line by itself.
  • Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.

An empty name after hyphen stripping matched every long option, so the token "--=value" bound the value to a long option that was never named.
@garydgregory garydgregory changed the title reject an empty option name in getMatchingOptions Reject an empty option name in getMatchingOptions Jul 22, 2026
@garydgregory
garydgregory requested a review from Copilot July 23, 2026 12:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a parsing edge case where an “empty” long option name (e.g., --=value) could incorrectly match and bind to any defined long option due to partial-matching behavior in Options#getMatchingOptions.

Changes:

  • Add an early-return in Options#getMatchingOptions so an empty cleaned option name yields no matches.
  • Add unit coverage ensuring empty/-/-- inputs don’t match, and --=/etc/shadow is rejected by both DefaultParser and PosixParser.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
src/main/java/org/apache/commons/cli/Options.java Prevents empty cleaned option names from matching all long options in partial-match lookup.
src/test/java/org/apache/commons/cli/OptionsTest.java Adds regression tests for empty-name matching and --=value rejection across parsers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +222 to +225
// an empty name is not a partial name, it would match every long option
if (Util.isEmpty(clean)) {
return matchingOpts;
}

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@farkhalit
Please review the copilot comment and update the comment and/or the implementation.
TY!

@garydgregory

Copy link
Copy Markdown
Member

@farkhalit ping 🔔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants