Reject an empty option name in getMatchingOptions - #434
#434Reject an empty option name in getMatchingOptions#434farkhalit wants to merge 2 commits intoapache:masterapache/commons-cli:masterfrom farkhalit:empty-longopt-matchfarkhalit/commons-cli:empty-longopt-matchCopy head branch name to clipboard
Conversation
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.
There was a problem hiding this comment.
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#getMatchingOptionsso an empty cleaned option name yields no matches. - Add unit coverage ensuring empty/
-/--inputs don’t match, and--=/etc/shadowis rejected by bothDefaultParserandPosixParser.
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.
| // an empty name is not a partial name, it would match every long option | ||
| if (Util.isEmpty(clean)) { | ||
| return matchingOpts; | ||
| } |
There was a problem hiding this comment.
Went with the null-tolerant reading here. Util.isEmpty is the guard this package already uses for name checks, and returning an empty list for null lines up with the contract (no match means empty list) rather than leaking an NPE from startsWith. I made that explicit in the Javadoc and added a null case to the test.
garydgregory
left a comment
There was a problem hiding this comment.
@farkhalit
Please review the copilot comment and update the comment and/or the implementation.
TY!
|
@farkhalit ping 🔔 |
Signed-off-by: farkhalit rida <farkhlait@sproutxp.com>
|
Addressed the copilot note. I kept the Util.isEmpty guard and made the null case intentional: getMatchingOptions(null) now returns an empty list instead of throwing from startsWith, which matches the documented no-match contract. Updated the Javadoc to say so and added a null assertion to the test. Pushed. |
Repro:
--=/etc/shadowparsed against anOptionsthat holds a single long option--config-file. BothDefaultParserandPosixParserreturn aCommandLinewithconfig-fileset to/etc/shadow, and an empty argument list. Nothing on the command line named that option.Cause:
getMatchingOptionsstrips the hyphens off--, leaving an empty name, andlongOpt.startsWith("")holds for every entry, so an empty name matches the entire long option table.handleLongOptionWithEqualsplits--=Vinto opt--and valueV, sees one match, and binds.PosixParser.flattenreaches the same list from its own--branch. With more than one long option defined the same token raisesAmbiguousOptionExceptionnaming all of them rather than rejecting it, andisLongOptionclassifies-=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
--=Vnow reacheshandleUnknownTokenlike 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-filesees--=...name nothing, while the parser behind it binds the value anyway.mvn; that'smvnon the command line by itself.