Fix bug with symlink creation (#3028)

* Fix bug with symlink creation

Previously, this was executing a command using a shell, but without properly
escaping the input. This instead passes a list in to Popen, to ensure escape is
working properly.

* More refactoring

* Fix argument missed in refactor

Author: agjohnson

readthedocs/core/symlink.py

@@ -157,11 +157,11 @@ def symlink_cnames(self, domain=None):
 
             # CNAME to doc root
             symlink = os.path.join(self.CNAME_ROOT, dom.domain)
-            run('ln -nsf {0} {1}'.format(self.project_root, symlink))
+            run(['ln', '-nsf', self.project_root, symlink])
 
             # Project symlink
             project_cname_symlink = os.path.join(self.PROJECT_CNAME_ROOT, dom.domain)
-            run('ln -nsf %s %s' % (self.project.doc_path, project_cname_symlink))
+            run(['ln', '-nsf', self.project.doc_path, project_cname_symlink])
 
     def remove_symlink_cname(self, domain):
         """Remove CNAME symlink"""
@@ -198,7 +198,13 @@ def symlink_subprojects(self):
                 symlink_dir = os.sep.join(symlink.split(os.path.sep)[:-1])
                 if not os.path.lexists(symlink_dir):
                     safe_makedirs(symlink_dir)
-                run('ln -nsf %s %s' % (docs_dir, symlink))
+                # TODO this should use os.symlink, not a call to shell. For now,
+                # this passes command as a list to be explicit about escaping
+                # characters like spaces.
+                status, _, stderr = run(['ln', '-nsf', docs_dir, symlink])
+                if status > 0:
+                    log.error('Could not symlink path: status=%d error=%s',
+                              status, stderr)
 
         # Remove old symlinks
         if os.path.exists(self.subproject_root):
@@ -228,7 +234,7 @@ def symlink_translations(self):
             self._log(u"Symlinking translation: {0}->{1}".format(language, slug))
             symlink = os.path.join(self.project_root, language)
             docs_dir = os.path.join(self.WEB_ROOT, slug, language)
-            run('ln -nsf {0} {1}'.format(docs_dir, symlink))
+            run(['ln', '-nsf', docs_dir, symlink])
 
         # Remove old symlinks
         for lang in os.listdir(self.project_root):
@@ -259,7 +265,7 @@ def symlink_single_version(self):
         if version is not None:
             docs_dir = os.path.join(settings.DOCROOT, self.project.slug,
                                     'rtd-builds', version.slug)
-            run('ln -nsf %s/ %s' % (docs_dir, symlink))
+            run(['ln', '-nsf', docs_dir, symlink])
 
     def symlink_versions(self):
         """Symlink project's versions
@@ -279,7 +285,7 @@ def symlink_versions(self):
             self._log(u"Symlinking Version: %s" % version)
             symlink = os.path.join(version_dir, version.slug)
             docs_dir = os.path.join(settings.DOCROOT, self.project.slug, 'rtd-builds', version.slug)
-            run('ln -nsf {0} {1}'.format(docs_dir, symlink))
+            run(['ln', '-nsf', docs_dir, symlink])
             versions.add(version.slug)
 
         # Remove old symlinks

readthedocs/projects/utils.py

@@ -10,6 +10,7 @@
 from httplib2 import Http
 
 import redis
+import six
 from django.conf import settings
 from django.core.cache import cache
 
@@ -42,9 +43,13 @@ def find_file(filename):
     return matches
 
 
-def run(*commands, **kwargs):
+def run(*commands):
     """Run one or more commands
 
+    Each argument in `commands` can be passed as a string or as a list. Passing
+    as a list is the preferred method, as space escaping is more explicit and it
+    avoids the need for executing anything in a shell.
+
     If more than one command is given, then this is equivalent to
     chaining them together with ``&&``; if all commands succeed, then
     ``(status, out, err)`` will represent the last successful command.
@@ -66,19 +71,27 @@ def run(*commands, **kwargs):
     cwd = os.getcwd()
     if not commands:
         raise ValueError("run() requires one or more command-line strings")
-    shell = kwargs.get('shell', False)
 
     for command in commands:
-        if shell:
-            log.info("Running commands in a shell")
-            run_command = command
-        else:
+        # If command is a string, split it up by spaces to pass into Popen.
+        # Otherwise treat the command as an iterable.
+        if isinstance(command, six.string_types):
             run_command = command.split()
-        log.info("Running: '%s' [%s]", command, cwd)
+        else:
+            try:
+                run_command = list(command)
+                command = ' '.join(command)
+            except TypeError:
+                run_command = command
+        log.info('Running command: cwd=%s command=%s', cwd, command)
         try:
-            p = subprocess.Popen(run_command, shell=shell, cwd=cwd,
-                                 stdout=subprocess.PIPE,
-                                 stderr=subprocess.PIPE, env=environment)
+            p = subprocess.Popen(
+                run_command,
+                cwd=cwd,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.PIPE,
+                env=environment
+            )
 
             out, err = p.communicate()
             ret = p.returncode

readthedocs/rtd_tests/tests/test_project_symlinks.py

@@ -322,6 +322,53 @@ def test_subproject_alias(self):
             filesystem['private_web_root'] = public_root
         self.assertFilesystem(filesystem)
 
+    def test_subproject_alias_with_spaces(self):
+        """Symlink pass adds symlink for subproject alias"""
+        self.project.add_subproject(self.subproject, alias='Sweet Alias')
+        self.symlink.symlink_subprojects()
+        filesystem = {
+            'private_cname_project': {},
+            'private_cname_root': {},
+            'private_web_root': {
+                'kong': {'en': {}},
+                'sub': {'en': {}},
+            },
+            'public_cname_project': {},
+            'public_cname_root': {},
+            'public_web_root': {
+                'kong': {
+                    'en': {'latest': {
+                        'type': 'link',
+                        'target': 'user_builds/kong/rtd-builds/latest',
+                    }},
+                    'projects': {
+                        'sub': {
+                            'type': 'link',
+                            'target': 'public_web_root/sub',
+                        },
+                        'Sweet Alias': {
+                            'type': 'link',
+                            'target': 'public_web_root/sub',
+                        },
+                    }
+                },
+                'sub': {
+                    'en': {'latest': {
+                        'type': 'link',
+                        'target': 'user_builds/sub/rtd-builds/latest',
+                    }}
+                }
+            }
+        }
+        if self.privacy == 'private':
+            public_root = filesystem['public_web_root'].copy()
+            private_root = filesystem['private_web_root'].copy()
+            public_root['kong']['projects']['sub']['target'] = 'private_web_root/sub'
+            public_root['kong']['projects']['Sweet Alias']['target'] = 'private_web_root/sub'
+            filesystem['public_web_root'] = private_root
+            filesystem['private_web_root'] = public_root
+        self.assertFilesystem(filesystem)
+
     def test_remove_subprojects(self):
         """Nonexistant subprojects are unlinked"""
         self.project.add_subproject(self.subproject)