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

 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 @@ class Compiler(object):
                         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 @@ class CompilerBase(object):
 
 
 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 @@ class CoffeeScriptCompiler(SubProcessCompiler):
     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 @@ class ES6Compiler(SubProcessCompiler):
     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 @@ class LessCompiler(SubProcessCompiler):
 
     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 @@ class LiveScriptCompiler(SubProcessCompiler):
     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 @@ class SASSCompiler(SubProcessCompiler):
         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 @@ class StylusCompiler(SubProcessCompiler):
         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

 # -*- coding: utf-8 -*-
 from __future__ import unicode_literals
 
+import shlex
+
 from django.conf import settings as _settings
 
 DEFAULTS = {
@@ -77,12 +79,26 @@ DEFAULTS = {
 
 
 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

 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 @@ class DummyCompiler(CompilerBase):
         return
 
 
-class CompilerTest(TestCase):
+class DummyCompilerTest(TestCase):
     def setUp(self):
         default_collector.collect()
         self.compiler = Compiler()
@@ -45,3 +100,76 @@ class CompilerTest(TestCase):
     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

tests/tests/test_conf.py

+# -*- coding: utf-8 -*-
+from __future__ import unicode_literals
+
+from django.test import TestCase
+
+from pipeline.conf import PipelineSettings
+
+class TestSettings(TestCase):
+
+    def test_3unicode(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_BINARY": "env actualprogram" })
+        self.assertEqual(s.PIPELINE_FOO_BINARY, ('env', 'actualprogram'))
+
+    def test_2unicode(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_BINARY": u"env actualprogram" })
+        self.assertEqual(s.PIPELINE_FOO_BINARY, ('env', 'actualprogram'))
+
+    def test_2bytes(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_BINARY": "env actualprogram" })
+        self.assertEqual(s.PIPELINE_FOO_BINARY, ('env', 'actualprogram'))
+
+    def test_expected_splitting(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_BINARY": "env actualprogram" })
+        self.assertEqual(s.PIPELINE_FOO_BINARY, ('env', 'actualprogram'))
+
+    def test_expected_preservation(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_BINARY": r"actual\ program" })
+        self.assertEqual(s.PIPELINE_FOO_BINARY, ('actual program',))
+
+    def test_tuples_are_normal(self):
+        s = PipelineSettings(dict(), DEFAULTS={ "PIPELINE_FOO_ARGUMENTS": ("explicit", "with", "args") })
+        self.assertEqual(s.PIPELINE_FOO_ARGUMENTS, ('explicit', 'with', 'args'))