diff --git a/readthedocs/core/validators.py b/readthedocs/core/validators.py index 3117119841a..c8523f2d619 100644 --- a/readthedocs/core/validators.py +++ b/readthedocs/core/validators.py @@ -52,6 +52,11 @@ def __call__(self, value): @deconstructible class RepositoryURLValidator(object): + disallow_relative_url = True + + # Pattern for ``git@github.com: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,51 @@ def __call__(self, value): if allow_private_repos: valid_schemes += private_schemes url = urlparse(value) - if ( - ( # pylint: disable=too-many-boolean-expressions - 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 - # ``git@github.com: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 ``git@github.com: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:: + + ```` 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() diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index abcd6cb44d1..827f9f39d6a 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -1,6 +1,7 @@ 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 @@ -106,11 +107,16 @@ 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): hg_repo = make_test_hg() diff --git a/readthedocs/rtd_tests/utils.py b/readthedocs/rtd_tests/utils.py index c227778c180..a2a4cded84f 100644 --- a/readthedocs/rtd_tests/utils.py +++ b/readthedocs/rtd_tests/utils.py @@ -69,6 +69,14 @@ def make_test_git(): 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( @@ -77,6 +85,7 @@ def make_test_git(): )) 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) diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 79f0d34791a..a6859256eff 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -13,7 +13,7 @@ import git from six import PY2, StringIO -from readthedocs.core.validators import validate_repository_url +from readthedocs.core.validators import validate_submodule_url from readthedocs.projects.exceptions import RepositoryError from readthedocs.vcs_support.base import BaseVCS, VCSVersion @@ -79,7 +79,7 @@ def are_submodules_valid(self): repo = git.Repo(self.working_dir) for submodule in repo.submodules: try: - validate_repository_url(submodule.url) + validate_submodule_url(submodule.url) except ValidationError: return False return True