Make the Executor class reliable.

Plugins do not need to escape the command arguments or remove the line
breaks and care about how the will get parsed anymore, they just need to
pass a list of args.

Do not spawn an additional shell, for more clarity, simplicity and possibly
security.

4 files changed, 61 insertions, 28 deletions

diff --git a/plugins/link.py b/plugins/link.py
index 8d757be6..427d718a 100644
--- a/plugins/link.py
+++ b/plugins/link.py
@@ -7,7 +7,6 @@ from plugin import BasePlugin
 from xhtml import clean_text
 import common
 import tabs
-import pipes
 
 url_pattern = re.compile(r'\b(http[s]?://(?:\S+))\b', re.I|re.U)
 
@@ -42,8 +41,7 @@ class Plugin(BasePlugin):
             nb = 1
         link = self.find_link(nb)
         if link:
-            link = pipes.quote(link)
-            self.core.exec_command("%s %s" % (self.config.get('browser', 'firefox'), link))
+            self.core.exec_command([self.config.get('browser', 'firefox'), link])
         else:
             self.core.information('No URL found.', 'Warning')
 
diff --git a/plugins/simple_notify.py b/plugins/simple_notify.py
index c2cbb198..d274e0ee 100644
--- a/plugins/simple_notify.py
+++ b/plugins/simple_notify.py
@@ -1,7 +1,7 @@
 from plugin import BasePlugin
 from xhtml import clean_text, get_body_from_message_stanza
 from timed_events import DelayedEvent
-import pipes
+import shlex
 
 class Plugin(BasePlugin):
     def init(self):
@@ -25,14 +25,15 @@ class Plugin(BasePlugin):
         body = clean_text(get_body_from_message_stanza(message))
         if not body:
             return
-        command = self.config.get('command', '').strip()
-        if not command:
+        command_str = self.config.get('command', '').strip()
+        if not command_str:
             self.core.information('No notification command was provided in the configuration file', 'Warning')
             return
-        self.core.exec_command(command % {'body':pipes.quote(body), 'from':pipes.quote(fro)})
-        after_command = self.config.get('after_command', '').strip()
-        if not after_command:
+        command = [arg % {'body': body.replace('\n', ' '), 'from': fro} for arg in shlex.split(command_str)]
+        self.core.exec_command(command)
+        after_command_str = self.config.get('after_command', '').strip()
+        if not after_command_str:
             return
-        delayed_event = DelayedEvent(self.config.get('delay', 1), self.core.exec_command, after_command % {'body':pipes.quote(body), 'from':pipes.quote(fro)})
+        after_command = [arg % {'body': body.replace('\n', ' '), 'from': fro} for arg in shlex.split(after_command_str)]
+        delayed_event = DelayedEvent(self.config.get('delay', 1), self.core.exec_command, after_command)
         self.core.add_timed_event(delayed_event)
-4
diff --git a/src/core.py b/src/core.py
index bd83c51f..9609ec44 100644
--- a/src/core.py
+++ b/src/core.py
@@ -11,6 +11,7 @@ import os
 import sys
 import time
 import curses
+import pipes
 import ssl
 
 from functools import reduce
@@ -492,17 +493,32 @@ class Core(object):
 
     def exec_command(self, command):
         """
-        Execute an external command on the local or a remote
-        machine, depending on the conf. For example, to open a link in a
-        browser, do exec_command("firefox http://poezio.eu"),
-        and this will call the command on the correct computer.
-        The remote execution is done by writing the command on a fifo.
-        That fifo has to be on the machine where poezio is running, and
-        accessible (through sshfs for example) from the local machine (where
-        poezio is not running). A very simple daemon reads on that fifo,
-        and executes any command that is read in it.
-        """
-        command = '%s\n' % (command,)
+        Execute an external command on the local or a remote machine,
+        depending on the conf. For example, to open a link in a browser, do
+        exec_command(["firefox", "http://poezio.eu"]), and this will call
+        the command on the correct computer.
+
+        The command argument is a list of strings, not quoted or escaped in
+        any way. The escaping is done here if needed.
+
+        The remote execution is done
+        by writing the command on a fifo.  That fifo has to be on the
+        machine where poezio is running, and accessible (through sshfs for
+        example) from the local machine (where poezio is not running). A
+        very simple daemon (daemon.py) reads on that fifo, and executes any
+        command that is read in it. Since we can only write strings to that
+        fifo, each argument has to be pipes.quote()d. That way the
+        shlex.split on the reading-side of the daemon will be safe.
+
+        You cannot use a real command line with pipes, redirections etc, but
+        this function supports a simple case redirection to file: if the
+        before-last argument of the command is ">" or ">>", then the last
+        argument is considered to be a filename where the command stdout
+        will be written. For example you can do exec_command(["echo",
+        "coucou les amis coucou coucou", ">", "output.txt"]) and this will
+        work. If you try to do anything else, your |, [, <<, etc will be
+        interpreted as normal command arguments, not shell special tokens.
+        """
         if config.get('exec_remote', 'false') == 'true':
             # We just write the command in the fifo
             if not self.remote_fifo:
@@ -511,16 +527,17 @@ class Core(object):
                 except (OSError, IOError) as e:
                     self.information('Could not open fifo file for writing: %s' % (e,), 'Error')
                     return
+            command_str = ' '.join([pipes.quote(arg.replace('\n', ' ')) for arg in command]) + '\n'
             try:
-                self.remote_fifo.write(command)
+                self.remote_fifo.write(command_str)
             except (IOError) as e:
-                self.information('Could not execute [%s]: %s' % (command.strip(), e,), 'Error')
+                self.information('Could not execute %s: %s' % (command, e,), 'Error')
                 self.remote_fifo = None
         else:
-            e = Executor(command.strip())
+            e = Executor(command)
             try:
                 e.start()
-            except ValueError as e: # whenever shlex fails
+            except ValueError as e:
                 self.information('%s' % (e,), 'Error')
 
 
diff --git a/src/daemon.py b/src/daemon.py
index 5d8c9fab..08cf480f 100755
--- a/src/daemon.py
+++ b/src/daemon.py
@@ -38,17 +38,34 @@ class Executor(threading.Thread):
     def __init__(self, command):
         threading.Thread.__init__(self)
         self.command = command
+        # check for > or >> special case
+        self.filename = None
+        self.redirection_mode = 'w'
+        if len(command) >= 3:
+            if command[-2] in ('>', '>>'):
+                self.filename = command.pop(-1)
+                if command[-1] == '>>':
+                    self.redirection_mode = 'a'
+                command.pop(-1)
 
     def run(self):
         log.info('executing %s' % (self.command,))
-        subprocess.call(['sh', '-c', self.command])
+        stdout = None
+        if self.filename:
+            try:
+                stdout = open(self.filename, self.redirection_mode)
+            except (OSError, IOError) as e:
+                log.error('Could not open redirection file: %s (%s)' % (self.filename, e,))
+                return
+        subprocess.call(self.command, stdout=stdout)
 
 def main():
     while True:
         line = sys.stdin.readline()
         if line == '':
             break
-        e = Executor(line)
+        command = shlex.split(line)
+        e = Executor(command)
         e.start()
 
 if __name__ == '__main__':