Issue #9309 - More comprehensive escaping of command line options

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
jetty · Feb 3, 2023 · 45fdebc · 45fdebc
1 parent 016de2f
commit 45fdebc
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 34 deletions.
diff --git a/jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java b/jetty-start/src/main/java/org/eclipse/jetty/start/CommandLineBuilder.java
@@ -19,6 +19,17 @@
 
 public class CommandLineBuilder
 {
+    // Matrix of 7-bit characters that needs escaping on command line.
+    private static final boolean[] NEEDS_ESCAPING = new boolean[127];
+
+    static
+    {
+        for (char c : " %$\\{}[]()\"|<>&;`!".toCharArray())
+        {
+            NEEDS_ESCAPING[c] = true;
+        }
+    }
+
     public static File findExecutable(File root, String path)
     {
         String npath = path.replace('/', File.separatorChar);
@@ -59,21 +70,28 @@ public static String findJavaBin()
      *
      * @param arg the argument to quote
      * @return the quoted and escaped argument
+     * @deprecated use {@link #escape(String) instead}
      */
+    @Deprecated
     public static String quote(String arg)
     {
-        boolean needsQuoting = (arg.indexOf(' ') >= 0) || (arg.indexOf('"') >= 0);
-        if (!needsQuoting)
-        {
-            return arg;
-        }
+        return escape(arg);
+    }
+
+    /**
+     * Escape the raw string to make it suitable for use on a command line.
+     *
+     * @param arg the argument to escape
+     * @return the escaped argument
+     */
+    public static String escape(String arg)
+    {
         StringBuilder buf = new StringBuilder();
-        // buf.append('"');
         boolean escaped = false;
         boolean quoted = false;
         for (char c : arg.toCharArray())
         {
-            if (!quoted && !escaped && ((c == '"') || (c == ' ')))
+            if (!quoted && !escaped && NEEDS_ESCAPING[c])
             {
                 buf.append("\\");
             }
@@ -85,7 +103,6 @@ public static String quote(String arg)
             escaped = (c == '\\');
             buf.append(c);
         }
-        // buf.append('"');
         return buf.toString();
     }
 
@@ -113,7 +130,7 @@ public void addArg(String arg)
     {
         if (arg != null)
         {
-            args.add(quote(arg));
+            args.add(escape(arg));
         }
     }
 
@@ -136,11 +153,11 @@ public void addEqualsArg(String name, String value)
     {
         if ((value != null) && (value.length() > 0))
         {
-            args.add(quote(name + "=" + value));
+            args.add(escape(name + "=" + value));
         }
         else
         {
-            args.add(quote(name));
+            args.add(escape(name));
         }
     }
 
@@ -180,7 +197,7 @@ public String toString(String delim)
             {
                 buf.append(delim);
             }
-            buf.append(quote(arg));
+            buf.append(arg); // we assume escaping has occurred during addArg
         }
 
         return buf.toString();

diff --git a/jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java b/jetty-start/src/test/java/org/eclipse/jetty/start/CommandLineBuilderTest.java
@@ -21,54 +21,49 @@
 
 public class CommandLineBuilderTest
 {
-    private CommandLineBuilder cmd = new CommandLineBuilder("java");
-
-    @BeforeEach
-    public void setUp()
-    {
-        cmd.addEqualsArg("-Djava.io.tmpdir", "/home/java/temp dir/");
-        cmd.addArg("--version");
-    }
-
     @Test
     public void testSimpleCommandline()
     {
+        CommandLineBuilder cmd = new CommandLineBuilder("java");
+        cmd.addEqualsArg("-Djava.io.tmpdir", "/home/java/temp dir/");
+        cmd.addArg("--version");
         assertThat(cmd.toString(), is("java -Djava.io.tmpdir=/home/java/temp\\ dir/ --version"));
     }
 
     @Test
-    public void testQuotingSimple()
+    public void testEscapedSimple()
     {
-        assertQuoting("/opt/jetty", "/opt/jetty");
+        assertEscaping("/opt/jetty", "/opt/jetty");
     }
 
     @Test
-    public void testQuotingSpaceInPath()
+    public void testEscapedSpaceInPath()
     {
-        assertQuoting("/opt/jetty 7/home", "/opt/jetty\\ 7/home");
+        assertEscaping("/opt/jetty 7/home", "/opt/jetty\\ 7/home");
     }
 
     @Test
-    public void testQuotingSpaceAndQuotesInPath()
+    public void testEscapedSpaceAndQuotesInPath()
     {
-        assertQuoting("/opt/jetty 7 \"special\"/home", "/opt/jetty\\ 7\\ \\\"special\\\"/home");
+        assertEscaping("/opt/jetty 7 \"special\"/home", "/opt/jetty\\ 7\\ \\\"special\\\"/home");
     }
 
     @Test
-    public void testToStringIsQuotedEvenIfArgsAreNotQuotedForProcessBuilder()
+    public void testEscapedFormattingString()
     {
-        System.out.println(cmd.toString());
+        assertEscaping("%{client}a - %u %{dd/MMM/yyyy:HH:mm:ss ZZZ|GMT}t \"%r\" %s %O \"%{Referer}i\" \"%{User-Agent}i\"",
+            "\\%\\{client\\}a\\ -\\ \\%u\\ \\%\\{dd/MMM/yyyy:HH:mm:ss\\ ZZZ\\|GMT\\}t\\ \\\"\\%r\\\"\\ \\%s\\ \\%O\\ \\\"\\%\\{Referer\\}i\\\"\\ \\\"\\%\\{User-Agent\\}i\\\"");
     }
 
     @Test
-    public void testQuoteQuotationMarks()
+    public void testEscapeQuotationMarks()
     {
-        assertQuoting("-XX:OnOutOfMemoryError='kill -9 %p'", "-XX:OnOutOfMemoryError='kill -9 %p'");
+        assertEscaping("-XX:OnOutOfMemoryError='kill -9 %p'", "-XX:OnOutOfMemoryError='kill -9 %p'");
     }
 
-    private void assertQuoting(String raw, String expected)
+    private void assertEscaping(String raw, String expected)
     {
-        String actual = CommandLineBuilder.quote(raw);
-        assertThat("Quoted version of [" + raw + "]", actual, is(expected));
+        String actual = CommandLineBuilder.escape(raw);
+        assertThat("Escaped version of [" + raw + "]", actual, is(expected));
     }
 }