From 1f6b48ae74026a12f955f2f15f9f08823d744515 Mon Sep 17 00:00:00 2001 From: Chad Miller Date: Sat, 12 Sep 2015 23:56:08 -0400 Subject: [PATCH] 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 | 79 +++++++++++++----- pipeline/compilers/coffee.py | 6 +- pipeline/compilers/es6.py | 3 +- pipeline/compilers/less.py | 5 +- pipeline/compilers/livescript.py | 6 +- pipeline/compilers/sass.py | 2 +- pipeline/compilers/stylus.py | 2 +- pipeline/conf.py | 20 ++++- tests/tests/test_compiler.py | 134 ++++++++++++++++++++++++++++++- tests/tests/test_conf.py | 32 ++++++++ 10 files changed, 250 insertions(+), 39 deletions(-) create mode 100644 tests/tests/test_conf.py diff --git a/pipeline/compilers/__init__.py b/pipeline/compilers/__init__.py index 491bc217..3b713c73 100644 --- a/pipeline/compilers/__init__.py +++ b/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) + diff --git a/pipeline/compilers/coffee.py b/pipeline/compilers/coffee.py index 4c1fa642..8260ede6 100644 --- a/pipeline/compilers/coffee.py +++ b/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) diff --git a/pipeline/compilers/es6.py b/pipeline/compilers/es6.py index 1e81d57e..a5cedd77 100644 --- a/pipeline/compilers/es6.py +++ b/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) diff --git a/pipeline/compilers/less.py b/pipeline/compilers/less.py index 709cd5ae..fa81747b 100644 --- a/pipeline/compilers/less.py +++ b/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) diff --git a/pipeline/compilers/livescript.py b/pipeline/compilers/livescript.py index 35c5e812..f72f896b 100644 --- a/pipeline/compilers/livescript.py +++ b/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) diff --git a/pipeline/compilers/sass.py b/pipeline/compilers/sass.py index 495f130a..d05c87ec 100644 --- a/pipeline/compilers/sass.py +++ b/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, diff --git a/pipeline/compilers/stylus.py b/pipeline/compilers/stylus.py index c70d7ee0..320efd9e 100644 --- a/pipeline/compilers/stylus.py +++ b/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 diff --git a/pipeline/conf.py b/pipeline/conf.py index bdb4a614..151f489c 100644 --- a/pipeline/conf.py +++ b/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) diff --git a/tests/tests/test_compiler.py b/tests/tests/test_compiler.py index 15b07821..372496c4 100644 --- a/tests/tests/test_compiler.py +++ b/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 diff --git a/tests/tests/test_conf.py b/tests/tests/test_conf.py new file mode 100644 index 00000000..b9ad05c3 --- /dev/null +++ b/tests/tests/test_conf.py @@ -0,0 +1,32 @@ +# -*- 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'))