Quote script args and with args with spaces

For rundeck#298

Arguments to script/script-file/script-url were not being quoted.

Additionally, arguments with spaces were not being quoted.

If arguments have spaces even if not containing expansion variables then
they are quoted.

core/src/main/java/com/dtolabs/rundeck/core/cli/CLIUtils.java

@@ -107,6 +107,14 @@ public static String generateArgline(final String scriptpath, final String[] arg
         return sb.toString();
     }
 
+    /**
+     * Return true if the string contains any whitespace
+     * @param arg
+     * @return
+     */
+    public static boolean containsSpace(String arg) {
+        return StringUtils.containsAny(arg, " ");
+    }
     public static String quoteUnixShellArg(String arg) {
         StringBuilder stringBuilder = new StringBuilder();
         quoteUnixShellArg(stringBuilder, arg);
@@ -176,6 +184,9 @@ public static String escapeUnixShellChars(String str) {
     }
 
     private static final String UNIX_SHELL_CHARS = "\"';{}()&$\\|*?><";
+    /**
+     * non-space whitespace
+     */
     private static final String WS_CHARS = "\n\r\t";
 
     public static void escapeUnixShellChars(StringBuilder out, String str) {

core/src/main/java/com/dtolabs/rundeck/core/execution/ExecutionServiceImpl.java

@@ -294,10 +294,11 @@ public NodeExecutorResult executeCommand(final ExecutionContext context, final S
 
         final String[] nodeCommand = DataContextUtils.replaceDataReferences(command, nodeContext.getDataContext());
         Converter<String,String> quote = CLIUtils.argumentQuoteForOperatingSystem(node.getOsFamily());
-        //quote args that have substituted context input
+        //quote args that have substituted context input, or have whitespace
+        //allow other args to be used literally
         for (int i = 0; i < nodeCommand.length; i++) {
             String replaced = nodeCommand[i];
-            if(!replaced.equals(command[i])) {
+            if (!replaced.equals(command[i]) || CLIUtils.containsSpace(replaced)) {
                 nodeCommand[i] = quote.convert(replaced);
             }
         }

core/src/main/java/com/dtolabs/rundeck/core/execution/impl/local/LocalNodeExecutor.java

@@ -41,7 +41,6 @@
 import org.apache.tools.ant.BuildException;
 import org.apache.tools.ant.Project;
 import org.apache.tools.ant.taskdefs.ExecTask;
-import org.apache.tools.ant.types.Commandline;
 
 import java.util.Map;
 
@@ -112,10 +111,15 @@ private ExecTask buildExecTask(Project project, ExecTaskParameters taskParameter
         execTask.setTaskType("exec");
         execTask.setFailonerror(false);
         execTask.setProject(project);
-        final Commandline.Argument arg = execTask.createArg();
 
         execTask.setExecutable(taskParameters.getCommandexecutable());
-        arg.setLine(taskParameters.getCommandargline());
+        String[] commandargs = taskParameters.getCommandArgs();
+        if(null!=commandargs){
+            for (int i = 0; i < commandargs.length; i++) {
+                String commandarg = commandargs[i];
+                execTask.createArg().setValue(commandarg);
+            }
+        }
 
         //add Env elements to pass environment variables to the exec
 

core/src/main/java/com/dtolabs/rundeck/core/execution/script/ExecTaskParameterGeneratorImpl.java

@@ -52,52 +52,39 @@ public ExecTaskParameterGeneratorImpl() {
     public ExecTaskParameters generate(final INodeEntry nodeentry, final boolean command, final File scriptfile,
                                        final String[] args) throws ExecutionException {
         final String commandexecutable;
-        final String commandargline;
+        final String[] commandargs;
 
-        String commandString;
         if (!command && null == scriptfile) {
             throw new ExecutionException("Could not determine the command to dispatch");
         }
         if ("windows".equals(nodeentry.getOsFamily())) {
             //TODO: escape args properly for windows
             commandexecutable = "cmd.exe";
             if (command) {
-                commandargline = CLIUtils.generateArgline("/c", args, false);
+                ArrayList<String> list = new ArrayList<String>();
+                list.add(0, "/c");
+                list.addAll(Arrays.asList(args));
+                commandargs = list.toArray(new String[list.size()]);
             } else if (null != scriptfile) {
                 //commandString is the script file location
                 ArrayList<String> list = new ArrayList<String>();
                 list.add(scriptfile.getAbsolutePath());
                 if(args!=null && args.length>0){
                     list.addAll(Arrays.asList(args));
                 }
-                commandargline = CLIUtils.generateArgline("/c", list.toArray(new String[list.size()]), false);
+                list.add(0,"/c");
+                commandargs = list.toArray(new String[list.size()]);
             } else {
                 throw new ExecutionException("Could not determine the command to dispatch");
             }
         } else {
             if (command) {
                 commandexecutable = "/bin/sh";
-//
-//                final String[] quotedCommand = new String[args.length];
-//                {
-//                    Converter<String, String> quote = CLIUtils.argumentQuoteForOperatingSystem(nodeentry.getOsFamily());
-//                    for (int i = 0; i < args.length; i++) {
-//                        String replaced = args[i];
-//                        quotedCommand[i] = quote.convert(replaced);
-//                    }
-//                }
-//                commandString = StringUtils.join(args, " ");
-//                commandargline = CLIUtils.generateArgline("-c", new String[]{commandString},false);
-
-//                commandexecutable = args[0];
-//                String[] newargs = new String[args.length-1];
-//                System.arraycopy(args, 1, newargs, 0, newargs.length);
-                commandString = StringUtils.join(args," ");//CLIUtils.generateArgline(null,args,false);
-                commandargline = CLIUtils.generateArgline("-c",new String[]{commandString},false);
+                commandargs = new String[]{"-c", StringUtils.join(args, " ")};
             } else if (null != scriptfile) {
                 final String scriptPath = scriptfile.getAbsolutePath();
                 commandexecutable = scriptPath;
-                commandargline = CLIUtils.generateArgline(null, args, false);
+                commandargs=args;
             } else {
                 throw new ExecutionException("Could not determine the command to dispatch");
             }
@@ -107,8 +94,8 @@ public String getCommandexecutable() {
                 return commandexecutable;
             }
 
-            public String getCommandargline() {
-                return commandargline;
+            public String[] getCommandArgs() {
+                return commandargs;
             }
         };
     }

core/src/main/java/com/dtolabs/rundeck/core/execution/script/ExecTaskParameters.java

@@ -31,7 +31,15 @@
  */
 public interface ExecTaskParameters {
 
+    /**
+     * command to run
+     * @return
+     */
     public String getCommandexecutable() ;
 
-    public String getCommandargline() ;
+    /**
+     * Array of arguments for a command
+     * @return
+     */
+    public String[] getCommandArgs() ;
 }

core/src/main/java/com/dtolabs/rundeck/core/execution/workflow/steps/node/impl/ScriptFileNodeStepExecutor.java

@@ -123,12 +123,15 @@ public static NodeStepResult executeRemoteScript(final ExecutionContext context,
         }
 
         //replace data references
-        final String[] newargs = ScriptExecUtil.createScriptArgs(context.getDataContext(),
-                                                                 null,
-                                                                 args,
-                                                                 scriptInterpreter,
-                                                                 interpreterargsquoted,
-                                                                 filepath);
+        final String[] newargs = ScriptExecUtil.createScriptArgs(
+                context.getDataContext(),
+                node,
+                null,
+                args,
+                scriptInterpreter,
+                interpreterargsquoted,
+                filepath
+        );
         //XXX: windows specific call?
 
         return framework.getExecutionService().executeCommand(context, newargs, node);

core/src/main/java/com/dtolabs/rundeck/core/tasks/net/SSHTaskBuilder.java

@@ -23,8 +23,6 @@
 */
 package com.dtolabs.rundeck.core.tasks.net;
 
-import com.dtolabs.rundeck.core.CoreException;
-import com.dtolabs.rundeck.core.cli.CLIUtils;
 import com.dtolabs.rundeck.core.common.INodeEntry;
 import com.dtolabs.rundeck.core.dispatcher.DataContextUtils;
 import org.apache.commons.lang.StringUtils;
@@ -219,13 +217,12 @@ static void build(final SSHExecInterface sshexecTask,
 
         configureSSHBase(nodeentry, project, sshConnectionInfo, sshexecTask, loglevel);
 
-//        final String commandString = CLIUtils.generateArgline(null, args, false);
-        final String commandString = StringUtils.join(args, " ");
+        //nb: args are already quoted as necessary
+        final String commandString = StringUtils.join(args," ");
         sshexecTask.setCommand(commandString);
         sshexecTask.setTimeout(sshConnectionInfo.getSSHTimeout());
         sshexecTask.setOutputproperty("sshexec.output");
 
-
         DataContextUtils.addEnvVars(sshexecTask, dataContext);
     }
 

core/src/main/java/com/dtolabs/rundeck/core/utils/ScriptExecUtil.java

@@ -24,6 +24,8 @@
 */
 package com.dtolabs.rundeck.core.utils;
 
+import com.dtolabs.rundeck.core.cli.CLIUtils;
+import com.dtolabs.rundeck.core.common.INodeEntry;
 import com.dtolabs.rundeck.core.dispatcher.DataContextUtils;
 import com.dtolabs.utils.Streams;
 
@@ -107,6 +109,27 @@ public static String[] createScriptArgs(final Map<String, Map<String, String>> l
                                             final String[] scriptargsarr,
                                             final String scriptinterpreter,
                                             final boolean interpreterargsquoted, final String filepath) {
+        return createScriptArgs(localDataContext, null, scriptargs, scriptargsarr, scriptinterpreter,
+                interpreterargsquoted, filepath);
+    }
+
+    /**
+     * Generate argument array for a script file invocation
+     *
+     * @param localDataContext      data context properties to expand among the args
+     * @param node                  node to use for os-type argument quoting.
+     * @param scriptargs            arguments to the script file
+     * @param scriptargsarr         arguments to the script file as an array
+     * @param scriptinterpreter     interpreter invocation for the file, or null to invoke it directly
+     * @param interpreterargsquoted if true, pass the script file and args as a single argument to the interpreter
+     * @param filepath              remote filepath for the script
+     */
+    public static String[] createScriptArgs(final Map<String, Map<String, String>> localDataContext,
+            final INodeEntry node,
+            final String scriptargs,
+            final String[] scriptargsarr,
+            final String scriptinterpreter,
+            final boolean interpreterargsquoted, final String filepath) {
         final ArrayList<String> arglist = new ArrayList<String>();
         if (null != scriptinterpreter) {
             arglist.addAll(Arrays.asList(scriptinterpreter.split(" ")));
@@ -129,9 +152,25 @@ public static String[] createScriptArgs(final Map<String, Map<String, String>> l
             arglist.add(filepath);
             if (null != scriptargs) {
                 arglist.addAll(Arrays.asList(DataContextUtils.replaceDataReferences(scriptargs.split(" "),
-                                                                                    localDataContext)));
+                        localDataContext)));
             } else if (null != scriptargsarr) {
-                arglist.addAll(Arrays.asList(DataContextUtils.replaceDataReferences(scriptargsarr, localDataContext)));
+                final String[] newargs = DataContextUtils.replaceDataReferences(scriptargsarr, localDataContext);
+                Converter<String, String> quote;
+                if (null != node) {
+                    quote = CLIUtils.argumentQuoteForOperatingSystem(node.getOsFamily());
+                } else {
+                    quote = CLIUtils.argumentQuoteForOperatingSystem(null);
+                }
+                //quote args that have substituted context input, or have whitespace
+                //allow other args to be used literally
+                for (int i = 0; i < newargs.length; i++) {
+                    String replaced = newargs[i];
+                    if (!replaced.equals(scriptargsarr[i]) || CLIUtils.containsSpace(replaced)) {
+                        arglist.add(quote.convert(replaced));
+                    } else {
+                        arglist.add(replaced);
+                    }
+                }
             }
         }
         return arglist.toArray(new String[arglist.size()]);

core/src/test/java/com/dtolabs/rundeck/core/cli/TestCLIUtils.java

@@ -27,9 +27,6 @@
 import junit.framework.TestCase;
 import junit.framework.TestSuite;
 
-import java.util.Arrays;
-import java.util.List;
-
 public class TestCLIUtils extends TestCase {
     CLIUtils cliUtils;
 
@@ -82,4 +79,13 @@ public void testGenerateArglineSafe() throws Exception {
                 "test rm '*' '&&' 'do\tthings\t>/etc/passwd'",
                 CLIUtils.generateArgline("test", new String[]{"rm", "*", "&&", "do\tthings\t>/etc/passwd"}, false));
     }
+    public void testContainsWhitespace() throws Exception {
+        assertFalse("invalid",  CLIUtils.containsSpace(""));
+        assertFalse("invalid",  CLIUtils.containsSpace("asdf1234"));
+        assertTrue("invalid", CLIUtils.containsSpace("asdf123 4"));
+        assertTrue("invalid", CLIUtils.containsSpace("   "));
+        assertFalse("invalid", CLIUtils.containsSpace("asdf123\t4"));
+        assertFalse("invalid", CLIUtils.containsSpace("asdf123\n4"));
+        assertFalse("invalid", CLIUtils.containsSpace("asdf123\r4"));
+    }
 }

core/src/test/java/com/dtolabs/rundeck/core/execution/script/TestExecTaskParameterGenerator.java

@@ -32,7 +32,6 @@
 import java.io.File;
 
 public class TestExecTaskParameterGenerator extends TestCase {
-    ExecTaskParameterGenerator execTaskParameterGenerator;
     private File testScriptFile;
 
     public TestExecTaskParameterGenerator(String name) {
@@ -57,6 +56,14 @@ public static void main(String args[]) {
         junit.textui.TestRunner.run(suite());
     }
 
+    static void assertArrayEquals(String desc, String[] expected, String[] actual) {
+        assertEquals(desc, expected.length, actual.length);
+        for (int i = 0; i < expected.length; i++) {
+            String expect = expected[i];
+            String actualStr = actual[i];
+            assertEquals("pos " + i + ": " + desc, expect, actualStr);
+        }
+    }
 
     public void testGenerateCommand() throws Exception {
         /*
@@ -77,7 +84,7 @@ public void testGenerateCommand() throws Exception {
                 testArgs);
 
             assertEquals("Wrong executable for local unix command", "/bin/sh", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "-c id", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"-c", "id"}, params.getCommandArgs());
         }
         {
             //command input with unix os-family
@@ -96,7 +103,8 @@ public void testGenerateCommand() throws Exception {
                 testArgs);
 
             assertEquals("Wrong executable for local unix command", "/bin/sh", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "-c 'id && hostname'", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"-c","id && hostname"},
+                    params.getCommandArgs());
         }
         {
             //basic command with windows os-family
@@ -115,7 +123,7 @@ public void testGenerateCommand() throws Exception {
                 testArgs);
 
             assertEquals("Wrong executable for local windows command", "cmd.exe", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "/c id", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"/c", "id"}, params.getCommandArgs());
 
         }
         {
@@ -135,7 +143,8 @@ public void testGenerateCommand() throws Exception {
                 testArgs);
 
             assertEquals("Wrong executable for local windows command", "cmd.exe", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "/c id potato hell", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"/c", "id", "potato", "hell"},
+                    params.getCommandArgs());
 
         }
         {// quoted command with windows os-family
@@ -154,7 +163,7 @@ public void testGenerateCommand() throws Exception {
                 testArgs);
 
             assertEquals("Wrong executable for local windows command", "cmd.exe", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "/c echo 'test belief'", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"/c", "echo","test belief"}, params.getCommandArgs());
 
         }
     }
@@ -179,7 +188,7 @@ public void testGenerateScript() throws Exception{
                 testArgs);
             assertEquals("Wrong executable for local unix command", testScriptFile.getAbsolutePath(),
                 params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "", params.getCommandargline());
+            assertNull("Wrong argline for local command", params.getCommandArgs());
         }
         {
             //scriptfile with args, with unix os-family
@@ -200,7 +209,7 @@ public void testGenerateScript() throws Exception{
 
             assertEquals("Wrong executable for local unix command", testScriptFile.getAbsolutePath(),
                 params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "test args", params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"test","args"}, params.getCommandArgs());
         }
 
         {
@@ -220,8 +229,7 @@ public void testGenerateScript() throws Exception{
                 testArgs);
 
             assertEquals("Wrong executable for local unix command", "cmd.exe", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "/c " + testScriptFile.getAbsolutePath(),
-                params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"/c", testScriptFile.getAbsolutePath()}, params.getCommandArgs());
         }
         {
             //scriptfile with args, with windows os-family
@@ -240,8 +248,8 @@ public void testGenerateScript() throws Exception{
                 testArgs);
 
             assertEquals("Wrong executable for local unix command", "cmd.exe", params.getCommandexecutable());
-            assertEquals("Wrong argline for local command", "/c " + testScriptFile.getAbsolutePath()+" test args something",
-                params.getCommandargline());
+            assertArrayEquals("Wrong argline for local command", new String[]{"/c", testScriptFile.getAbsolutePath(),
+                    "test", "args", "something"}, params.getCommandArgs());
         }
     }
 }