Fix up some of the logic around repo and submodule URLs

readthedocs/core/validators.py

@@ -52,6 +52,11 @@ def __call__(self, value):
 @deconstructible
 class RepositoryURLValidator(object):
 
+    disallow_relative_url = True
+
+    # Pattern for ``[email protected]:user/repo`` pattern
+    re_git_user = re.compile(r'^[\w]+@.+')
+
     def __call__(self, value):
         allow_private_repos = getattr(settings, 'ALLOW_PRIVATE_REPOS', False)
         public_schemes = ['https', 'http', 'git', 'ftps', 'ftp']
@@ -60,28 +65,50 @@ def __call__(self, value):
         if allow_private_repos:
             valid_schemes += private_schemes
         url = urlparse(value)
-        if (
-                (
-                    url.scheme not in valid_schemes and \
-                    '@' not in value and \
-                    not value.startswith('lp:')
-                ) or \
-                (
-                    value.startswith('/') or \
-                    value.startswith('file://') or \
-                    value.startswith('.')
-                )
-        ):
-            # Avoid ``/path/to/local/file`` and ``file://`` scheme but allow
-            # ``[email protected]:user/project.git`` and ``lp:bazaar``
-            raise ValidationError(_('Invalid scheme for URL'))
-        elif '&&' in value or '|' in value:
+
+        # Malicious characters go first
+        if '&&' in value or '|' in value:
             raise ValidationError(_('Invalid character in the URL'))
-        elif (
-                ('@' in value or url.scheme in private_schemes) and
-                not allow_private_repos
-        ):
-            raise ValidationError('Clonning via SSH is not supported')
-        return value
+        elif url.scheme in valid_schemes:
+            return value
+
+        # Repo URL is not a supported scheme at this point, but there are
+        # several cases where we might support it
+        # Launchpad
+        elif value.startswith('lp:'):
+            return value
+        # Relative paths are conditionally supported
+        elif value.startswith('.') and not self.disallow_relative_url:
+            return value
+        # SSH cloning and ``[email protected]:user/project.git``
+        elif self.re_git_user.search(value) or url.scheme in private_schemes:
+            if allow_private_repos:
+                return value
+            else:
+                # Throw a more helpful error message
+                raise ValidationError('Manual cloning via SSH is not supported')
+
+        # No more valid URLs without supported URL schemes
+        raise ValidationError(_('Invalid scheme for URL'))
+
+
+class SubmoduleURLValidator(RepositoryURLValidator):
+
+    """
+    A URL validator for repository submodules
+
+    If a repository has a relative submodule, the URL path is effectively the supermodule's remote ``origin`` URL with the relative path applied.
+
+    From the git docs::
+
+        ``<repository>`` is the URL of the new submodule's origin repository.
+        This may be either an absolute URL, or (if it begins with ``./`` or
+        ``../``), the location relative to the superproject's default remote
+        repository
+    """
+
+    disallow_relative_url = False
+
 
 validate_repository_url = RepositoryURLValidator()
+validate_submodule_url = SubmoduleURLValidator()

readthedocs/projects/exceptions.py

@@ -39,6 +39,10 @@ class RepositoryError(BuildEnvironmentError):
         'Private repositories are not supported.'
     )
 
+    INVALID_SUBMODULES = _(
+        'One or more submodule URLs are not valid.'
+    )
+
     def get_default_message(self):
         if settings.ALLOW_PRIVATE_REPOS:
             return self.PRIVATE_ALLOWED

readthedocs/projects/views/private.py

@@ -177,6 +177,7 @@ def project_version_detail(request, project_slug, version_slug):
                 broadcast(
                     type='app', task=tasks.clear_artifacts, args=[version.pk])
                 version.built = False
+                version.machine = False
                 version.save()
         url = reverse('project_version_list', args=[project.slug])
         return HttpResponseRedirect(url)

readthedocs/rtd_tests/tests/test_backend.py

@@ -1,9 +1,11 @@
 from __future__ import absolute_import
 from os.path import exists
 
+import pytest
 from django.contrib.auth.models import User
 import django_dynamic_fixture as fixture
 
+from readthedocs.projects.exceptions import RepositoryError
 from readthedocs.projects.models import Project, Feature
 from readthedocs.rtd_tests.base import RTDTestCase
 
@@ -101,6 +103,19 @@ def test_skip_submodule_checkout(self):
         self.assertTrue(self.project.has_feature(Feature.SKIP_SUBMODULES))
         self.assertFalse(repo.are_submodules_available())
 
+    def test_check_submodule_urls(self):
+        repo = self.project.vcs_repo()
+        repo.checkout('submodule')
+        self.assertTrue(repo.are_submodules_valid())
+        repo.checkout('relativesubmodule')
+        self.assertTrue(repo.are_submodules_valid())
+
+    @pytest.mark.xfail(strict=True, reason="Fixture is not working correctly")
+    def test_check_invalid_submodule_urls(self):
+        with self.assertRaises(RepositoryError) as e:
+            repo.checkout('invalidsubmodule')
+            self.assertEqual(e.msg, RepositoryError.INVALID_SUBMODULES)
+
 
 class TestHgBackend(RTDTestCase):
     def setUp(self):

readthedocs/rtd_tests/tests/test_sync_versions.py

@@ -388,7 +388,7 @@ def test_update_inactive_stable_version(self):
 
         version_stable = Version.objects.get(slug=STABLE)
         self.assertFalse(version_stable.active)
-        self.assertEqual(version_stable.identifier, '0.9')
+        self.assertEqual(version_stable.identifier, '1.0.0')
 
     def test_stable_version_tags_over_branches(self):
         version_post_data = {

readthedocs/rtd_tests/utils.py

@@ -4,7 +4,7 @@
 
 import logging
 import subprocess
-from os import chdir, environ, getcwd
+from os import chdir, environ, getcwd, mkdir
 from os.path import abspath, join as pjoin
 from shutil import copytree
 from tempfile import mkdtemp
@@ -49,11 +49,31 @@ def make_test_git():
     # Set up the actual repository
     log.info(check_output(['git', 'add', '.'], env=env))
     log.info(check_output(['git', 'commit', '-m"init"'], env=env))
-    # Add repo itself as submodule
-    log.info(check_output(['git', 'checkout', '-b', 'submodule'], env=env))
-    log.info(check_output(['git', 'submodule', 'add', '-b', 'master', './', 'submodule'], env=env))
+
+    # Add fake repo as submodule. We need to fake this here because local path
+    # URL are not allowed and using a real URL will require Internet to clone
+    # the repo
+    log.info(check_output(['git', 'checkout', '-b', 'submodule', 'master'], env=env))
+    # https://stackoverflow.com/a/37378302/2187091
+    mkdir(pjoin(directory, 'foobar'))
+    gitmodules_path = pjoin(directory, '.gitmodules')
+    with open(gitmodules_path, 'w') as fh:
+        fh.write('''[submodule "foobar"]\n\tpath = foobar\n\turl = https://foobar.com/git\n''')
+    log.info(check_output(['git', 'update-index', '--add', '--cacheinfo', '160000', '233febf4846d7a0aeb95b6c28962e06e21d13688', 'foobar'], env=env))
     log.info(check_output(['git', 'add', '.'], env=env))
     log.info(check_output(['git', 'commit', '-m"Add submodule"'], env=env))
+
+    # Add a relative submodule URL in the relativesubmodule branch
+    log.info(check_output(['git', 'checkout', '-b', 'relativesubmodule', 'master'], env=env))
+    log.info(check_output(['git', 'submodule', 'add', '-b', 'master', './', 'relativesubmodule'], env=env))
+    log.info(check_output(['git', 'add', '.'], env=env))
+    log.info(check_output(['git', 'commit', '-m"Add relative submodule"'], env=env))
+    # Add an invalid submodule URL in the invalidsubmodule branch
+    log.info(check_output(['git', 'checkout', '-b', 'invalidsubmodule', 'master'], env=env))
+    log.info(check_output(['git', 'submodule', 'add', '-b', 'master', 'file:///tmp/', 'invalidsubmodule'], env=env))
+    log.info(check_output(['git', 'add', '.'], env=env))
+    log.info(check_output(['git', 'commit', '-m"Add invalid submodule"'], env=env))
+
     # Checkout to master branch again
     log.info(check_output(['git', 'checkout', 'master'], env=env))
     chdir(path)

readthedocs/vcs_support/backends/git.py

@@ -9,8 +9,12 @@
 import os
 import re
 
+import git
+from django.core.exceptions import ValidationError
+from git.exc import BadName
 from six import PY2, StringIO
 
+from readthedocs.core.validators import validate_submodule_url
 from readthedocs.projects.exceptions import RepositoryError
 from readthedocs.vcs_support.base import BaseVCS, VCSVersion
 
@@ -71,6 +75,16 @@ def are_submodules_available(self):
         code, out, _ = self.run('git', 'submodule', 'status', record=False)
         return code == 0 and bool(out)
 
+    def are_submodules_valid(self):
+        """Test that all submodule URLs are valid."""
+        repo = git.Repo(self.working_dir)
+        for submodule in repo.submodules:
+            try:
+                validate_submodule_url(submodule.url)
+            except ValidationError:
+                return False
+        return True
+
     def fetch(self):
         code, _, _ = self.run('git', 'fetch', '--tags', '--prune')
         if code != 0:
@@ -230,18 +244,24 @@ def checkout(self, identifier=None):
         # Update submodules, temporarily allow for skipping submodule checkout
         # step for projects need more submodule configuration.
         if self.are_submodules_available():
-            self.run('git', 'submodule', 'sync')
-            self.run(
-                'git',
-                'submodule',
-                'update',
-                '--init',
-                '--recursive',
-                '--force',
-            )
-
+            if self.are_submodules_valid():
+                self.checkout_submodules()
+            else:
+                raise RepositoryError(RepositoryError.INVALID_SUBMODULES)
         return code, out, err
 
+    def checkout_submodules(self):
+        """Checkout all repository submodules recursively."""
+        self.run('git', 'submodule', 'sync')
+        self.run(
+            'git',
+            'submodule',
+            'update',
+            '--init',
+            '--recursive',
+            '--force',
+        )
+
     def find_ref(self, ref):
         # Check if ref starts with 'origin/'
         if ref.startswith('origin/'):
@@ -254,8 +274,13 @@ def find_ref(self, ref):
         return ref
 
     def ref_exists(self, ref):
-        code, _, _ = self.run('git', 'show-ref', ref, record_as_success=True)
-        return code == 0
+        try:
+            r = git.Repo(self.working_dir)
+            if r.commit(ref):
+                return True
+        except BadName:
+            return False
+        return False
 
     @property
     def env(self):

requirements/pip.txt

@@ -1,5 +1,5 @@
 # Base packages
-pip==9.0.1
+pip==9.0.3
 appdirs==1.4.3
 virtualenv==15.1.0
 docutils==0.14
@@ -47,6 +47,7 @@ dnspython==1.15.0
 
 # VCS
 httplib2==0.10.3
+GitPython==2.1.8
 
 # Search
 elasticsearch==1.5.0