Commons CLI
  1. Commons CLI
  2. CLI-185

Commons CLI incorrectly stripping leading and trailing quotes

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.1, 1.2
    • Fix Version/s: 1.3
    • Component/s: Parser
    • Labels:
      None
    • Environment:

      Description

      org.apache.commons.cli.Parser.processArgs() calls Util.stripLeadingAndTrailingQuotes() for all argument values. IMHO this is incorrect and totally broken.

      It is trivial to create a simple test for this. Output:

      $ java -cp target/clitest.jar Clitest --balloo "this is a \"test\""
      Value of argument balloo is 'this is a "test'.

      The argument 'balloo' should indeed keep its trailing double quote. It is what the shell gives it, so don't try to do something clever to it.

      The offending code was committed here:
      http://svn.apache.org/viewvc?view=rev&revision=129874
      and has been there for more than 6 years . Why was this committed in the first place?

      The fix is trivial, just get rid of Util.stripLeadingAndTrailingQuotes(), and consequently avoid calling it from Parser.processArgs().

        Issue Links

          Activity

          Hide
          Emmanuel Bourg added a comment -

          This was committed to fix CLI-22. I do agree that the quotes should not be removed.

          Show
          Emmanuel Bourg added a comment - This was committed to fix CLI-22 . I do agree that the quotes should not be removed.
          Hide
          Emmanuel Bourg added a comment -

          This is a regression introduced with CLI 1.1, CLI 1.0 didn't trim the quotes.

          Show
          Emmanuel Bourg added a comment - This is a regression introduced with CLI 1.1, CLI 1.0 didn't trim the quotes.
          Hide
          Einar M R Rosenvinge added a comment -

          Any info on when 1.3 will be out?

          Show
          Einar M R Rosenvinge added a comment - Any info on when 1.3 will be out?
          Hide
          Emmanuel Bourg added a comment -

          We just started working on CLI 1.3, it's not ready to be released yet. You can use a snapshot once the issue is fixed, but if you absolutely need a released component we might roll out a 1.2.1 bug fix release.

          Show
          Emmanuel Bourg added a comment - We just started working on CLI 1.3, it's not ready to be released yet. You can use a snapshot once the issue is fixed, but if you absolutely need a released component we might roll out a 1.2.1 bug fix release.
          Hide
          Einar M R Rosenvinge added a comment -

          If you could release a 1.2.1 bugfix release with this fix, I'd be forever grateful. I'm depending on it for a big project. Thanks!

          Show
          Einar M R Rosenvinge added a comment - If you could release a 1.2.1 bugfix release with this fix, I'd be forever grateful. I'm depending on it for a big project. Thanks!
          Hide
          Andrew Shirley added a comment -

          This is probably a windows vs. linux issue (or more accurately command.com vs bash I guess)

          I can't see how the original issues CLI-22 would have existed as bash shouldn't have passed the quote marks on to Java.

          A fix for this issue should probably continue to work both when quotes are passed in and when they aren't. How should CLI differentiate between

          ./Clitest --balloo "\"this is a test\""

          run on bash and

          ./Clitest --balloo "this is a test"

          run on command.com?

          Show
          Andrew Shirley added a comment - This is probably a windows vs. linux issue (or more accurately command.com vs bash I guess) I can't see how the original issues CLI-22 would have existed as bash shouldn't have passed the quote marks on to Java. A fix for this issue should probably continue to work both when quotes are passed in and when they aren't. How should CLI differentiate between ./Clitest --balloo "\"this is a test\"" run on bash and ./Clitest --balloo "this is a test" run on command.com?
          Hide
          Emmanuel Bourg added a comment -

          Even on Windows the quotes aren't passed to the application. I don't understand why John Keyes had to remove quotes in CLI-22.

          Show
          Emmanuel Bourg added a comment - Even on Windows the quotes aren't passed to the application. I don't understand why John Keyes had to remove quotes in CLI-22 .
          Hide
          Einar M R Rosenvinge added a comment -

          At least on all *nix platforms (sh/bash/zsh/tcsh/whatever...), modifying the actual arguments in this way is totally broken.

          If you really must do something clever on Windows, make sure it's Windows-only by checking the host platform or something.

          Show
          Einar M R Rosenvinge added a comment - At least on all *nix platforms (sh/bash/zsh/tcsh/whatever...), modifying the actual arguments in this way is totally broken. If you really must do something clever on Windows, make sure it's Windows-only by checking the host platform or something.
          Hide
          Andrew Shirley added a comment -

          fwiw, I have done a bit of experimenting and it seems I don't understand windows as well as I thought (which wasn't much to start with!).

          It seems double quote is not passed through by cmd.exe (although single quotes are passed through and cmd.exe doesn't require double quotes to be matched). If someone types:

          java tmp 'foo'

          on windows, I think it is safe to assume they know they are on windows at that they intend the single quote to be part of the argument (as if they had typed the following on bash et al)

          java tmp '\foo\'

          In conclusion, quotes (single or double) shouldn't be stripped at all and people should be careful when using single quotes in a platform ignorant way.

          Show
          Andrew Shirley added a comment - fwiw, I have done a bit of experimenting and it seems I don't understand windows as well as I thought (which wasn't much to start with!). It seems double quote is not passed through by cmd.exe (although single quotes are passed through and cmd.exe doesn't require double quotes to be matched). If someone types: java tmp 'foo' on windows, I think it is safe to assume they know they are on windows at that they intend the single quote to be part of the argument (as if they had typed the following on bash et al) java tmp '\foo\' In conclusion, quotes (single or double) shouldn't be stripped at all and people should be careful when using single quotes in a platform ignorant way.
          Hide
          Emmanuel Bourg added a comment -

          The single quotes are never stripped, it's only the double quotes.

          I tried to remove the stripping, it broke two tests:

          • test15648 in BugsTest: that's the test of CLI-22, it checks if the quotes of an argument are removed after the parsing. This can be easily changed.
          • testWorkaround2 in BugCLI148Test: this is the test for CLI-148, this one is more tricky to fix. Is the workaround introduced in CLI-148 worth keeping? Is it possible to keep the workaround without messing with other cases like the one exposed by Einar?
          Show
          Emmanuel Bourg added a comment - The single quotes are never stripped, it's only the double quotes. I tried to remove the stripping, it broke two tests: test15648 in BugsTest: that's the test of CLI-22 , it checks if the quotes of an argument are removed after the parsing. This can be easily changed. testWorkaround2 in BugCLI148Test: this is the test for CLI-148 , this one is more tricky to fix. Is the workaround introduced in CLI-148 worth keeping? Is it possible to keep the workaround without messing with other cases like the one exposed by Einar?
          Hide
          Einar M R Rosenvinge added a comment -

          It seems that BugCLI148Test.test2() relies on double quotes to parse this as a value and not an argument. Fine, but then it should not automagically remove the quotes; that should be left for the user and this should be documented as a known issue. The workaround is bad, bad, bad.

          The proper fix for CLI-148 is of course to actually support command lines like program -arg -val, where 'arg' is the name of the argument and '-val' is the actual value. I do understand, however, that this is a lot more work to implement.

          Show
          Einar M R Rosenvinge added a comment - It seems that BugCLI148Test.test2() relies on double quotes to parse this as a value and not an argument. Fine, but then it should not automagically remove the quotes; that should be left for the user and this should be documented as a known issue. The workaround is bad, bad, bad. The proper fix for CLI-148 is of course to actually support command lines like program -arg -val, where 'arg' is the name of the argument and '-val' is the actual value. I do understand, however, that this is a lot more work to implement.
          Hide
          Ajay Kidave added a comment -

          A 1.2.1 release with this fixed would be great, it would make Commons CLI usable for my project.

          Show
          Ajay Kidave added a comment - A 1.2.1 release with this fixed would be great, it would make Commons CLI usable for my project.
          Hide
          Jim Jagielski added a comment -

          Unfortunately, it's not clear that there is consensus on whether the fix creates more problems than it addresses. In other words, how many people will be surprised by this change and how many environments will it turn a working instance into a broken one?

          Show
          Jim Jagielski added a comment - Unfortunately, it's not clear that there is consensus on whether the fix creates more problems than it addresses. In other words, how many people will be surprised by this change and how many environments will it turn a working instance into a broken one?
          Hide
          Einar M R Rosenvinge added a comment -

          True, this could break working code in production. But such code does rely on a "feature" in Commons-CLI that is just faulty, so sooner or later you will have to flip the switch.

          I would say that you should do it sooner rather than later, and make sure to document the changed behavior carefully in the release notes.

          Show
          Einar M R Rosenvinge added a comment - True, this could break working code in production. But such code does rely on a "feature" in Commons-CLI that is just faulty, so sooner or later you will have to flip the switch. I would say that you should do it sooner rather than later, and make sure to document the changed behavior carefully in the release notes.
          Hide
          Jim Jagielski added a comment -

          How about if for 1.2.1 we add a boolean that allows for people to decide whether or not they want quotes stripped or not.? This can ease the transition...

          comments?

          Show
          Jim Jagielski added a comment - How about if for 1.2.1 we add a boolean that allows for people to decide whether or not they want quotes stripped or not.? This can ease the transition... comments?
          Hide
          Einar M R Rosenvinge added a comment -

          Which is worse?

          • Changing behavior of a method without changing the API at all (could make client code "silently" just break in prod.)
          • Changing the API in a minor release (requires recompile of client code)

          For me any of these would work, since I don't rely on Commons-CLI for legacy code.

          Show
          Einar M R Rosenvinge added a comment - Which is worse? Changing behavior of a method without changing the API at all (could make client code "silently" just break in prod.) Changing the API in a minor release (requires recompile of client code) For me any of these would work, since I don't rely on Commons-CLI for legacy code.
          Hide
          Ajay Kidave added a comment -

          In my case, removing the quotes for an argument is fine, I don't think this behavior should be changed. The issue is, for an argument like "abc"test"" (nested double quotes), the removal of quotes removes the last two quotes, the argument becomes abc"test instead of being abc"test" .

          Looking at the original patch, this could happen if stripLeadingAndTrailingQuotes function gets called two times, first one will do the proper stripping, second stripping will incorrectly remove the last quote. A possible fix would be to check if first and last char are both quotes. If both are, then do the stripping. This change should not break the API compatibility, unless someone depends on the incorrect behavior.

          Show
          Ajay Kidave added a comment - In my case, removing the quotes for an argument is fine, I don't think this behavior should be changed. The issue is, for an argument like "abc"test"" (nested double quotes), the removal of quotes removes the last two quotes, the argument becomes abc"test instead of being abc"test" . Looking at the original patch, this could happen if stripLeadingAndTrailingQuotes function gets called two times, first one will do the proper stripping, second stripping will incorrectly remove the last quote. A possible fix would be to check if first and last char are both quotes. If both are, then do the stripping. This change should not break the API compatibility, unless someone depends on the incorrect behavior.
          Hide
          Einar M R Rosenvinge added a comment -

          But Ajay, you are wrong. If a program is given the argument "abc"test"" (which means it has been given the argument --something "\"abc\"test\"\"" if run through bash), then it must be presented with "abc"test"" through Commons-CLI.

          There is no such thing as proper stripping of double quotes. Get rid of it.

          Show
          Einar M R Rosenvinge added a comment - But Ajay, you are wrong . If a program is given the argument "abc"test"" (which means it has been given the argument --something "\"abc\"test\"\"" if run through bash), then it must be presented with "abc"test"" through Commons-CLI. There is no such thing as proper stripping of double quotes. Get rid of it.
          Hide
          Ajay Kidave added a comment -

          There are two issues here. First is the behavior of whether or not to strip quotes. The second is a bug where abc"test" becomes abc"test . What I meant was that fixing the second issue should not depend on the first. The bug can be fixed even if the stripping behavior is retained as it is.

          Show
          Ajay Kidave added a comment - There are two issues here. First is the behavior of whether or not to strip quotes. The second is a bug where abc"test" becomes abc"test . What I meant was that fixing the second issue should not depend on the first. The bug can be fixed even if the stripping behavior is retained as it is.
          Hide
          Emmanuel Bourg added a comment -

          The workaround for CLI-148 is the only justification for the removal of the quotes, I'll see if this can be addressed in the new DefaultParser.

          In the meantime I will follow Ajay suggestion to remove the quotes only if there is one at the beginning and one at the end. And to take Einar's example into account this will only happen if the value contains exactly 2 quotes. If the value contains more than 2 quotes, no stripping is performed.

          This will preserve the workaround for CLI-148 and eliminate erroneous stripping.

          Show
          Emmanuel Bourg added a comment - The workaround for CLI-148 is the only justification for the removal of the quotes, I'll see if this can be addressed in the new DefaultParser. In the meantime I will follow Ajay suggestion to remove the quotes only if there is one at the beginning and one at the end. And to take Einar's example into account this will only happen if the value contains exactly 2 quotes. If the value contains more than 2 quotes, no stripping is performed. This will preserve the workaround for CLI-148 and eliminate erroneous stripping.
          Hide
          Einar M R Rosenvinge added a comment -

          OK, if breaking CLI-148 is not an option, then the proposed fix will at least make things much better.

          Do you have any news on when the next release (containing this fix) is due?

          Show
          Einar M R Rosenvinge added a comment - OK, if breaking CLI-148 is not an option, then the proposed fix will at least make things much better. Do you have any news on when the next release (containing this fix) is due?

            People

            • Assignee:
              Unassigned
              Reporter:
              Einar M R Rosenvinge
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development