Make compilers run directly, not inside shells

Names on disk and config files should not have to be safe for shells to
interpret. As close to possible, we should take literals and give literals to
the OS kernel to operate on directly. For filename globs, it is our
responsiblility to expand them, and if we had no problem with backwards
compatibility, we would insist config files' SCRIPT_ARGUMENTS parameters are
tuples of discrete values. Delegating those to a shell breaks clear boundaries
of interpreetation and will always be prone to errors, oversight, and
incompatibility.

So, now, we never take names that are unsafe and try to make then safe for a
shell, because we don't need a shell.

This fixes #444, which had problems with Windows paths being insensible to the
crazy quoting we hoped would make a filename safe for a shell.

This fixes #494, which had a compiler-attempt stdout captured as part of a
string interpreted by a shell. When the compiler didn't exist, that shell
expression STILL created empty files, and the pipeline thenafter served an
empty file as if it were real compiler output.

pipeline/compilers/__init__.py

@@ -1,16 +1,14 @@
 from __future__ import unicode_literals
 
 import os
-
-try:
-    from shlex import quote
-except ImportError:
-    from pipes import quote
+import subprocess
+from tempfile import NamedTemporaryFile
 
 from django.contrib.staticfiles import finders
 from django.contrib.staticfiles.storage import staticfiles_storage
 from django.core.files.base import ContentFile
 from django.utils.encoding import smart_bytes
+from django.utils.six import string_types
 
 from pipeline.conf import settings
 from pipeline.exceptions import CompilerError
@@ -40,8 +38,8 @@ def _compile(input_path):
                         infile = finders.find(input_path)
                     outfile = self.output_path(infile, compiler.output_extension)
                     outdated = compiler.is_outdated(input_path, output_path)
-                    compiler.compile_file(quote(infile), quote(outfile),
-                        outdated=outdated, force=force)
+                    compiler.compile_file(infile, outfile,
+                                          outdated=outdated, force=force)
                     return output_path
             else:
                 return input_path
@@ -90,18 +88,55 @@ def is_outdated(self, infile, outfile):
 
 
 class SubProcessCompiler(CompilerBase):
-    def execute_command(self, command, content=None, cwd=None):
-        import subprocess
-        pipe = subprocess.Popen(command, shell=True, cwd=cwd,
-                                stdout=subprocess.PIPE, stdin=subprocess.PIPE,
-                                stderr=subprocess.PIPE)
-        if content:
-            content = smart_bytes(content)
-        stdout, stderr = pipe.communicate(content)
-        if stderr.strip():
-            raise CompilerError(stderr)
-        if self.verbose:
-            print(stderr)
-        if pipe.returncode != 0:
-            raise CompilerError("Command '{0}' returned non-zero exit status {1}".format(command, pipe.returncode))
-        return stdout
+    def execute_command(self, command, cwd=None, stdout_captured=None):
+        """Execute a command at cwd, saving its normal output at
+        stdout_captured. Errors, defined as nonzero return code or a failure
+        to start execution, will raise a CompilerError exception with a
+        description of the cause. They do not write output.
+
+        This is file-system safe (any valid file names are allowed, even with
+        spaces or crazy characters) and OS agnostic (existing and future OSes
+        that Python supports should already work).
+
+        The only thing weird here is that any incoming command arg item may
+        itself be a tuple. This allows compiler implementations to look clean
+        while supporting historical string config settings and maintaining
+        backwards compatibility. Thus, we flatten one layer deep.
+         ((env, foocomp), infile, (-arg,)) -> (env, foocomp, infile, -arg)
+        """
+        argument_list = []
+        for flattening_arg in command:
+            if isinstance(flattening_arg, string_types):
+                argument_list.append(flattening_arg)
+            else:
+                argument_list.extend(flattening_arg)
+
+        try:
+            # We always catch stdout in a file, but we may not have a use for it.
+            temp_file_container = cwd or os.path.dirname(stdout_captured or "") or os.getcwd()
+            with NamedTemporaryFile(delete=False, dir=temp_file_container) as stdout:
+                compiling = subprocess.Popen(argument_list, cwd=cwd,
+                                             stdout=stdout,
+                                             stderr=subprocess.PIPE)
+                _, stderr = compiling.communicate()
+
+            if compiling.returncode != 0:
+                stdout_captured = None  # Don't save erroneous result.
+                raise CompilerError(
+                    "{0!r} exit code {1}\n{2}".format(argument_list, compiling.returncode, stderr))
+
+            # User wants to see everything that happened.
+            if self.verbose:
+                with open(stdout.name) as out:
+                    print(out.read())
+                print(stderr)
+        except OSError as e:
+            stdout_captured = None  # Don't save erroneous result.
+            raise CompilerError(e)
+        finally:
+            # Decide what to do with captured stdout.
+            if stdout_captured:
+                os.rename(stdout.name, os.path.join(cwd or os.curdir, stdout_captured))
+            else:
+                os.remove(stdout.name)
+

pipeline/compilers/coffee.py

@@ -13,10 +13,10 @@ def match_file(self, path):
     def compile_file(self, infile, outfile, outdated=False, force=False):
         if not outdated and not force:
             return  # File doesn't need to be recompiled
-        command = "%s -cp %s %s > %s" % (
+        command = (
             settings.COFFEE_SCRIPT_BINARY,
+            "-cp",
             settings.COFFEE_SCRIPT_ARGUMENTS,
             infile,
-            outfile
         )
-        return self.execute_command(command)
+        return self.execute_command(command, stdout_captured=outfile)

pipeline/compilers/es6.py

@@ -13,10 +13,11 @@ def match_file(self, path):
     def compile_file(self, infile, outfile, outdated=False, force=False):
         if not outdated and not force:
             return  # File doesn't need to be recompiled
-        command = "%s %s %s -o %s" % (
+        command = (
             settings.BABEL_BINARY,
             settings.BABEL_ARGUMENTS,
             infile,
+            "-o",
             outfile
         )
         return self.execute_command(command)

pipeline/compilers/less.py

@@ -14,10 +14,9 @@ def match_file(self, filename):
 
     def compile_file(self, infile, outfile, outdated=False, force=False):
         # Pipe to file rather than provide outfile arg due to a bug in lessc
-        command = "%s %s %s > %s" % (
+        command = (
             settings.LESS_BINARY,
             settings.LESS_ARGUMENTS,
             infile,
-            outfile
         )
-        return self.execute_command(command, cwd=dirname(infile))
+        return self.execute_command(command, cwd=dirname(infile), stdout_captured=outfile)

pipeline/compilers/livescript.py

@@ -13,10 +13,10 @@ def match_file(self, path):
     def compile_file(self, infile, outfile, outdated=False, force=False):
         if not outdated and not force:
             return  # File doesn't need to be recompiled
-        command = "%s -cp %s %s > %s" % (
+        command = (
             settings.LIVE_SCRIPT_BINARY,
+            "-cp",
             settings.LIVE_SCRIPT_ARGUMENTS,
             infile,
-            outfile
         )
-        return self.execute_command(command)
+        return self.execute_command(command, stdout_captured=outfile)

pipeline/compilers/sass.py

@@ -13,7 +13,7 @@ def match_file(self, filename):
         return filename.endswith(('.scss', '.sass'))
 
     def compile_file(self, infile, outfile, outdated=False, force=False):
-        command = "%s %s %s %s" % (
+        command = (
             settings.SASS_BINARY,
             settings.SASS_ARGUMENTS,
             infile,

pipeline/compilers/stylus.py

@@ -13,7 +13,7 @@ def match_file(self, filename):
         return filename.endswith('.styl')
 
     def compile_file(self, infile, outfile, outdated=False, force=False):
-        command = "%s %s %s" % (
+        command = (
             settings.STYLUS_BINARY,
             settings.STYLUS_ARGUMENTS,
             infile

pipeline/conf.py

@@ -1,6 +1,8 @@
 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
 
+import shlex
+
 from django.conf import settings as _settings
 
 DEFAULTS = {
@@ -77,12 +79,26 @@
 
 
 class PipelineSettings(object):
-    '''
+    """
     Container object for pipeline settings
-    '''
+    """
     def __init__(self, wrapped_settings):
         DEFAULTS.update(wrapped_settings)
         self.__dict__ = DEFAULTS
 
+    def __getattr__(self, name):
+        if hasattr(self, name):
+            value = getattr(self, name)
+        elif name in self:
+            value = DEFAULTS[name]
+        else:
+            raise AttributeError("'%s' setting not found" % name)
+
+        if name.endswith(("_BINARY", "_ARGUMENTS")):
+            if isinstance(value, (type(u""), type(b""))):
+                return tuple(shlex.split(value))
+            return tuple(value)
+
+        return value
 
 settings = PipelineSettings(_settings.PIPELINE)

tests/tests/test_compiler.py

@@ -1,15 +1,70 @@
 from __future__ import unicode_literals
 
+import os
+
 from django.test import TestCase
 
 from pipeline.conf import settings
-from pipeline.compilers import Compiler, CompilerBase
+from pipeline.compilers import Compiler, CompilerBase, SubProcessCompiler
 from pipeline.collector import default_collector
-
+from pipeline.exceptions import CompilerError
 
 from tests.utils import _
 
 
+class FailingCompiler(SubProcessCompiler):
+    output_extension = 'junk'
+
+    def match_file(self, path):
+        return path.endswith('.coffee')
+
+    def compile_file(self, infile, outfile, outdated=False, force=False):
+        command = (("/usr/bin/env", "false",),)
+        return self.execute_command(command)
+
+
+class InvalidCompiler(SubProcessCompiler):
+    output_extension = 'junk'
+
+    def match_file(self, path):
+        return path.endswith('.coffee')
+
+    def compile_file(self, infile, outfile, outdated=False, force=False):
+        command = (
+            ("this-exists-nowhere-as-a-command-and-should-fail",),
+            infile,
+            outfile
+        )
+        return self.execute_command(command)
+
+
+class CopyingCompiler(SubProcessCompiler):
+    output_extension = 'junk'
+
+    def match_file(self, path):
+        return path.endswith('.coffee')
+
+    def compile_file(self, infile, outfile, outdated=False, force=False):
+        command = (
+            ("cp",),
+            ("--no-dereference", "--preserve=links",),
+            infile,
+            outfile
+        )
+        return self.execute_command(command)
+
+
+class LineNumberingCompiler(SubProcessCompiler):
+    output_extension = 'junk'
+
+    def match_file(self, path):
+        return path.endswith('.coffee')
+
+    def compile_file(self, infile, outfile, outdated=False, force=False):
+        command = (("/usr/bin/env", "cat"), ("-n",), infile,)
+        return self.execute_command(command, stdout_captured=outfile)
+
+
 class DummyCompiler(CompilerBase):
     output_extension = 'js'
 
@@ -20,7 +75,7 @@ def compile_file(self, infile, outfile, outdated=False, force=False):
         return
 
 
-class CompilerTest(TestCase):
+class DummyCompilerTest(TestCase):
     def setUp(self):
         default_collector.collect()
         self.compiler = Compiler()
@@ -45,3 +100,76 @@ def test_compile(self):
     def tearDown(self):
         default_collector.clear()
         settings.COMPILERS = self.old_compilers
+
+
+class CompilerStdoutTest(TestCase):
+    def setUp(self):
+        default_collector.collect()
+        self.compiler = Compiler()
+        self.old_compilers = settings.COMPILERS
+        settings.PIPELINE_COMPILERS = ['tests.tests.test_compiler.LineNumberingCompiler']
+
+    def test_output_path(self):
+        output_path = self.compiler.output_path("js/helpers.coffee", "js")
+        self.assertEqual(output_path, "js/helpers.js")
+
+    def test_compile(self):
+        paths = self.compiler.compile([_('pipeline/js/dummy.coffee')])
+        self.assertEqual([_('pipeline/js/dummy.junk')], list(paths))
+
+    def tearDown(self):
+        default_collector.clear()
+        settings.PIPELINE_COMPILERS = self.old_compilers
+
+
+class CompilerSelfWriterTest(TestCase):
+    def setUp(self):
+        default_collector.collect()
+        self.compiler = Compiler()
+        self.old_compilers = settings.PIPELINE_COMPILERS
+        settings.PIPELINE_COMPILERS = ['tests.tests.test_compiler.CopyingCompiler']
+
+    def test_output_path(self):
+        output_path = self.compiler.output_path("js/helpers.coffee", "js")
+        self.assertEqual(output_path, "js/helpers.js")
+
+    def test_compile(self):
+        paths = self.compiler.compile([_('pipeline/js/dummy.coffee')])
+        default_collector.collect()
+        self.assertEqual([_('pipeline/js/dummy.junk')], list(paths))
+
+    def tearDown(self):
+        default_collector.clear()
+        settings.COMPILERS = self.old_compilers
+
+
+class InvalidCompilerTest(TestCase):
+    def setUp(self):
+        default_collector.collect()
+        self.compiler = Compiler()
+        self.old_compilers = settings.COMPILERS
+        settings.COMPILERS = ['tests.tests.test_compiler.InvalidCompiler']
+
+    def test_compile(self):
+        self.assertRaises(CompilerError,
+            self.compiler.compile, [_('pipeline/js/dummy.coffee')])
+
+    def tearDown(self):
+        default_collector.clear()
+        settings.COMPILERS = self.old_compilers
+
+
+class FailingCompilerTest(TestCase):
+    def setUp(self):
+        default_collector.collect()
+        self.compiler = Compiler()
+        self.old_compilers = settings.COMPILERS
+        settings.COMPILERS = ['tests.tests.test_compiler.FailingCompiler']
+
+    def test_compile(self):
+        self.assertRaises(CompilerError,
+                self.compiler.compile, [_('pipeline/js/dummy.coffee'),])
+
+    def tearDown(self):
+        default_collector.clear()
+        settings.COMPILERS = self.old_compilers