Commits

Tom Davies committed 81960e7

review re-work

  • Participants
  • Parent commits 3461519
  • Branches fix-param-handling

Comments (0)

Files changed (5)

File src/main/java/com/atlassian/utils/process/DefaultExternalProcessFactory.java

         process.setUseWindowsEncodingWorkaround(settings.isUseWindowsEncodingWorkaround());
         process.setWorkingDir(settings.getWorkingDirectory());
         process.setUseQuotesInBatArgumentsWorkaround(settings.isUseQuotesInBatArgumentsWorkaround());
+        process.setEscapeInternalDoubleQuotesOnWindows(settings.isEscapeInternalDoubleQuotesOnWindows());
 
         for (ProcessMonitor monitor : settings.getMonitors()) {
             if (monitor == null) {

File src/main/java/com/atlassian/utils/process/ExternalProcessBuilder.java

         settings.setUseQuotesInBatArgumentsWorkaround(true);
         return this;
     }
+
+    /**
+     * Escape any internal double quotes in arguments. This has no effect on non-Windows platforms.
+     *
+     * @return {@code this}
+     */
+    public ExternalProcessBuilder escapeInternalDoubleQuotesOnWindows() {
+        settings.setEscapeInternalDoubleQuotesOnWindows(true);
+        return this;
+    }
 }

File src/main/java/com/atlassian/utils/process/ExternalProcessImpl.java

     private boolean suppressSpecialWindowsBehaviour;
     private boolean useWindowsEncodingWorkaround;
     private boolean useQuotesInBatArgumentsWorkaround;
+    private boolean escapeInternalDoubleQuotesOnWindows = false;
 
     private volatile LatchedRunnable errorPump;
     private volatile long executionTimeout;
      * @param handler the process handler to manage the execution of this process
      */
     public ExternalProcessImpl(ExecutorService executorService, List<String> command, ProcessHandler handler) {
-        this.command = escapeArguments(command);
+        this.command = command;
         setHandler(handler);
 
         this.executorService = executorService;
         this.useQuotesInBatArgumentsWorkaround = useQuotesInBatArgumentsWorkaround;
     }
 
+    public void setEscapeInternalDoubleQuotesOnWindows(boolean escapeInternalDoubleQuotesOnWindows) {
+        this.escapeInternalDoubleQuotesOnWindows = escapeInternalDoubleQuotesOnWindows;
+    }
+
     private boolean areOutputPumpsRunning() {
         LatchedRunnable output = outputPump;
         LatchedRunnable error = errorPump;
         this.monitors.remove(monitor);
     }
 
-    private boolean isWindows() {
+    // protected for use in tests
+    protected boolean isWindows() {
         return OS_NAME.contains("windows");
     }
 
         try {
             this.startTime = System.currentTimeMillis();
             notifyBeforeStart();
-            this.process = createProcess(command, environment, workingDir);
+            this.process = createProcess(escapeArguments(command), environment, workingDir);
             setupIOPumps();
         } catch (IOException e) {
             processException = new ProcessNotStartedException(command.get(0) + " could not be started", e);
         setIdleTimeout(timeout);
     }
 
-
-    private List<String> escapeArguments(List<String> commandArgs) {
-        if (isWindows()) {
+    // protected for use in tests
+    protected List<String> escapeArguments(List<String> commandArgs) {
+        if (escapeInternalDoubleQuotesOnWindows && isWindows()) {
             for (int i = 0; i < commandArgs.size(); ++i) {
-                commandArgs.set(i, escapeArg(commandArgs.get(i)));
+                commandArgs.set(i, escapeArgument(commandArgs.get(i)));
             }
         }
         return commandArgs;
     }
 
     /**
-     * On windows we need to ensure that we escape any double quotes in the argument, (but not any already escaped quotes)
-     * @param arg
-     * @return
+     * On windows we need to ensure that we escape any double quotes in the argument.
+     *
+     * We don't escape already escaped quotes, and we don't escape quotes which surround the String
+     *
+     * @param s the String to escape
+     * @return the escaped String
      */
-    protected String escapeArg(String arg) {
-        String escapedArg = arg;
-        if (arg.contains("\"")) {
+    private String escapeArgument(String s) {
+        String escapedArg = s;
+        if (s.contains("\"") && !leadingAndTrailingQuotesOnly(s)) {
             boolean escapeNextChar = false;
-            StringBuilder sb = new StringBuilder("\"");
-            for (char c : arg.toCharArray()) {
+            boolean isQuoted = leadingAndTrailingQuotes(s);
+            if (isQuoted) {
+                // strip the surrounding quotes and reinstate them after escaping the argument
+                s = s.substring(1, s.length() - 1);
+            }
+            StringBuilder sb = new StringBuilder(isQuoted ? "\"" : "");
+            for (char c : s.toCharArray()) {
                 if (escapeNextChar) {
                     escapeNextChar = false;
                 } else if (c == '"') {
             if (escapeNextChar) {
                 sb.append('\\'); // string ended with unmatched escape character, add another escape char.
             }
-            sb.append('\"');
+            if (isQuoted) {
+                sb.append('\"');
+            }
             escapedArg = sb.toString();
         }
         return escapedArg;
     }
+
+    private boolean leadingAndTrailingQuotesOnly(String s) {
+        return
+                leadingAndTrailingQuotes(s) &&
+                s.indexOf('"', 1) == s.length() - 1;
+    }
+
+    private boolean leadingAndTrailingQuotes(String s) {
+        return
+                s.length() > 1 &&
+                s.startsWith("\"") &&
+                s.endsWith("\"");
+    }
 }

File src/main/java/com/atlassian/utils/process/ExternalProcessSettings.java

     private File workingDirectory;
 
     private boolean useQuotesInBatArgumentsWorkaround;
+    private boolean escapeInternalDoubleQuotesOnWindows = false;
 
     public ExternalProcessSettings() {
         environment = new HashMap<String, String>();
             throw new IllegalStateException("A ProcessHandler is required");
         }
     }
+
+    public boolean isEscapeInternalDoubleQuotesOnWindows() {
+        return escapeInternalDoubleQuotesOnWindows;
+    }
+
+    public void setEscapeInternalDoubleQuotesOnWindows(boolean escapeInternalDoubleQuotesOnWindows) {
+        this.escapeInternalDoubleQuotesOnWindows = escapeInternalDoubleQuotesOnWindows;
+    }
 }

File src/test/java/com/atlassian/utils/process/DoubleQuoteEscapingTest.java

 package com.atlassian.utils.process;
 
+import java.util.Arrays;
 import java.util.Collections;
 
 import org.junit.Test;
 public class DoubleQuoteEscapingTest {
     @Test
     public void testEscaping() {
-        ExternalProcessImpl testProcess = new ExternalProcessImpl(null, Collections.<String>emptyList(), null);
-        testEscape(testProcess, "foo", "foo", false);
-        testEscape(testProcess, "f\"oo", "f\\\"oo", true); // unescaped quote is escaped
-        testEscape(testProcess, "f\\\"oo", "f\\\"oo", true); // already escaped " stays escaped
-        testEscape(testProcess, "\"foo\"", "\\\"foo\\\"", true); // leading and trailing quotes are escaped, and string is surrounded with quotes
-        testEscape(testProcess, "fo\"o\\", "fo\\\"o\\\\", true); // trailing \ is turned into an escaped \, so that it doesn't escape the added surrounding quote
-        testEscape(testProcess, "fo\\\"o\\\\", "fo\\\"o\\\\", true); // trailing \\ is left alone
-        testEscape(testProcess, "fo\"o\\\\o\\o\\x", "fo\\\"o\\\\o\\o\\x", true); // other escaped characters are left alone
-        testEscape(testProcess, "foo\\", "foo\\", false); // strings with no embedded quotes are not touched
+        ExternalProcessImpl testProcess = new ExternalProcessImpl(null, Collections.<String>emptyList(), null) {
+            @Override
+            protected boolean isWindows() {
+                return true;
+            }
+        };
+        testNotEscaped(testProcess, "f\"oo"); // we haven't turned the option on, so no escaping yet
+        testProcess.setEscapeInternalDoubleQuotesOnWindows(true);
+        testNotEscaped(testProcess, "foo"); // no quote, unchanged
+        testEscape(testProcess, "f\"oo", "f\\\"oo"); // unescaped quote is escaped
+        testNotEscaped(testProcess, "f\\\"oo"); // already escaped " stays escaped
+        testNotEscaped(testProcess, "\"foo\""); // leading and trailing quotes are not escaped
+        testEscape(testProcess, "\"fo\"o\"", "\"fo\\\"o\""); // leading and trailing quotes with an embedded quote have the embedded quote escaped, but not the leading and trailing quotes
+        testEscape(testProcess, "fo\"o\\", "fo\\\"o\\\\"); // trailing \ is turned into an escaped \, so that it doesn't escape the added surrounding quote (even though we don't add quotes, Java may, so we don't want our arguments to end with an escape character)
+        testNotEscaped(testProcess, "fo\\\"o\\\\"); // trailing \\ is left alone
+        testEscape(testProcess, "fo\"o\\\\o\\o\\x", "fo\\\"o\\\\o\\o\\x"); // other escaped characters are left alone
+        testEscape(testProcess, "foo\\", "foo\\"); // strings with no embedded quotes are not touched
+        testEscape(testProcess, "\"foo", "\\\"foo"); // leading quotes are escaped *if* there is no trailing quote
+        testEscape(testProcess, "foo\"", "foo\\\""); // trailing quotes are escaped *if* there is no leading quote
+        testEscape(testProcess, "\"", "\\\""); // a single double quote is escaped
     }
 
-    private void testEscape(ExternalProcessImpl testProcess, String before, String expected, boolean shouldHaveSurroundingQuotes) {
-        String q = shouldHaveSurroundingQuotes ? "\"" : "";
-        Assert.assertEquals("The string '" + before + "' was not correctly escaped", q + expected + q, testProcess.escapeArg(before));
+    private void testEscape(ExternalProcessImpl testProcess, String before, String expected) {
+        Assert.assertEquals("The string '" + before + "' was not correctly escaped", expected, escapeSingleArgument(testProcess, before));
+    }
+
+    private void testNotEscaped(ExternalProcessImpl testProcess, String before) {
+        Assert.assertEquals("The string '" + before + "' was modified when it should have been left untouched", before, escapeSingleArgument(testProcess, before));
+    }
+
+    private String escapeSingleArgument(ExternalProcessImpl testProcess, String before) {
+        return testProcess.escapeArguments(Arrays.asList(before)).get(0);
     }
 }