diff --git a/modules/repos/downloader.py b/modules/repos/downloader.py index a3dc241f..9e566659 100644 --- a/modules/repos/downloader.py +++ b/modules/repos/downloader.py @@ -11,7 +11,7 @@ from wrolpi.common import logger from wrolpi.dates import now from wrolpi.downloader import Downloader, Download, DownloadContext, DownloadResult, make_progress_callback from wrolpi.errors import DownloadError -from .lib import git_command, git_env, find_readme, get_repo_size +from .lib import git_command, git_env, find_readme, get_repo_size, is_valid_branch from .models import Repository __all__ = ['GitDownloader', 'git_downloader', 'PreparedRepo', 'ExecutedRepo', 'parse_git_progress'] @@ -162,6 +162,9 @@ class GitDownloader(Downloader): try: default_branch = await self._default_branch(download, ctx, prepared) branch = prepared.branch or default_branch + # The remote chooses its default branch's name, and branches are passed to git as arguments. + if not is_valid_branch(branch): + raise DownloadError(f'Invalid branch name: {branch!r}') if (directory / '.git').is_dir(): await self._update(download, ctx, prepared, branch) else: diff --git a/modules/repos/lib.py b/modules/repos/lib.py index f4648caa..b2caabdc 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -27,7 +27,7 @@ logger = logger.getChild(__name__) __all__ = ['ParsedRepoURL', 'parse_repo_url', 'repo_url_key', 'git_command', 'git_env', 'create_repository', 'update_repository', 'delete_repository', 'get_repositories', 'list_repo_tree', 'find_readme', - 'repo_destination_variables', 'ReposConfig', 'repos_config', 'get_repos_config', 'save_repos_config', + 'repo_destination_variables', 'is_valid_branch', 'ReposConfig', 'repos_config', 'get_repos_config', 'save_repos_config', 'import_repos_config', 'DEFAULT_REPO_FREQUENCY'] DEFAULT_REPO_FREQUENCY = DownloadFrequency.weekly.value @@ -75,6 +75,9 @@ def parse_repo_url(url: str) -> ParsedRepoURL: parts = [i for i in path.split('/') if i] if not parts: raise InvalidRepo('Repo URL must have a path') + if any(i in ('.', '..') for i in parts): + # The name and owner are used in the Repo's directory. + raise InvalidRepo('Repo URL must not contain "." or ".." in its path') name = parts[-1].removesuffix('.git') if not name: raise InvalidRepo('Repo URL must have a name') @@ -85,6 +88,33 @@ def parse_repo_url(url: str) -> ParsedRepoURL: return ParsedRepoURL(url=f'{parsed.scheme}://{netloc}{path}', host=host, owner=owner, name=name, branch=branch) +# Characters git forbids in a ref name (`git check-ref-format`), plus whitespace. +INVALID_BRANCH_CHARACTERS_RE = re.compile(r'[\x00-\x20\x7f~^:?*\[\\]') + + +def is_valid_branch(branch: str) -> bool: + """Is `branch` a valid git branch name (the rules of `git check-ref-format --branch`)? + + Branch names are passed to git as arguments, so a name starting with `-` (an option) is never valid.""" + if not branch or not isinstance(branch, str): + return False + if branch.startswith('-') or branch == '@' or '@{' in branch or '..' in branch: + return False + if INVALID_BRANCH_CHARACTERS_RE.search(branch): + return False + if branch.startswith('/') or branch.endswith('/') or '//' in branch or branch.endswith('.'): + return False + for component in branch.split('/'): + if component.startswith('.') or component.endswith('.lock'): + return False + return True + + +def _validate_branch(branch: Optional[str]): + if branch is not None and not is_valid_branch(branch): + raise ValidationError(f'Invalid branch name: {branch!r}') + + def repo_url_key(url: str) -> str: """Two URLs of the same repo have the same key (`.git` suffix and case of the host are ignored).""" parsed = urlparse(url.strip()) @@ -164,6 +194,20 @@ def _validate_mode(mode: str): raise ValidationError(f'Repo mode must be one of {", ".join(REPO_MODES)}') +def _validate_directory(directory: pathlib.Path): + """A Repo's directory is strictly inside the media directory; it is cloned into, and may be deleted.""" + media_directory = get_media_directory() + directory = pathlib.Path(directory) + if '..' in directory.parts or not directory.is_absolute(): + raise InvalidRepo(f'Invalid repo directory: {directory}') + try: + relative = directory.relative_to(media_directory) + except ValueError: + raise InvalidRepo('Repo directory must be in the media directory') + if not relative.parts: + raise InvalidRepo('Repo directory can not be the media directory') + + def _validate_frequency(frequency: int): if not isinstance(frequency, int) or frequency <= 0: raise ValidationError('Repo frequency must be a positive number of seconds') @@ -177,7 +221,10 @@ def create_repository(session: Session, url: str, tag_name: str = None, frequenc _validate_mode(mode) _validate_frequency(frequency) branch = branch or parsed.branch or None + _validate_branch(branch) name = (name or parsed.name).strip() + if name in ('', '.', '..'): + raise InvalidRepo(f'Invalid repo name: {name!r}') if get_repository_by_url(session, parsed.url): raise RepoConflict(f'A repo with this URL already exists: {parsed.url}') @@ -195,8 +242,7 @@ def create_repository(session: Session, url: str, tag_name: str = None, frequenc session.flush([repo]) directory = directory or collection.format_destination(tag_name) - if not str(directory.resolve()).startswith(str(get_media_directory().resolve()) + '/'): - raise InvalidRepo('Repo directory must be in the media directory') + _validate_directory(directory) if session.query(Collection).filter(Collection.directory == directory, Collection.id != collection.id).first(): raise RepoConflict(f'Another collection already uses the directory {get_relative_to_media_directory(directory)}') collection.directory = directory @@ -221,7 +267,9 @@ def update_repository(session: Session, repo_id: int, description: str = None, f _validate_mode(mode) repo.mode = mode if branch is not None: - repo.branch = branch.strip() or None + branch = branch.strip() or None + _validate_branch(branch) + repo.branch = branch if frequency is not None: _validate_frequency(frequency) if download := repo.download: @@ -415,6 +463,8 @@ class ReposConfig(ConfigFile): _validate_frequency(frequency) directory = data.get('directory') directory = get_media_directory() / directory if directory else None + if directory: + _validate_directory(directory) tag_name = data.get('tag_name') repo = get_repository_by_url(session, parsed.url) @@ -430,7 +480,9 @@ class ReposConfig(ConfigFile): if directory: collection.directory = directory repo.mode = mode - repo.branch = data.get('branch') or None + branch = data.get('branch') or None + _validate_branch(branch) + repo.branch = branch if download := repo.download: download.frequency = frequency else: diff --git a/modules/repos/test/test_downloader.py b/modules/repos/test/test_downloader.py index b962669c..18c09efe 100644 --- a/modules/repos/test/test_downloader.py +++ b/modules/repos/test/test_downloader.py @@ -2,6 +2,7 @@ import shutil import subprocess import pytest +from mock import mock from modules.repos import lib from modules.repos.downloader import parse_git_progress @@ -209,3 +210,17 @@ def test_parse_git_progress(): assert parse_git_progress('remote: Counting objects: 100% (5/5), done.') == \ dict(stage='Counting objects', percent=100) assert parse_git_progress("Cloning into 'example'...") is None + + +@pytest.mark.asyncio +async def test_invalid_remote_default_branch(test_session, git_remote_factory, repos_download_manager): + """The remote chooses its default branch name; an invalid one is never passed to git.""" + remote = git_remote_factory() + repo = lib.create_repository(test_session, remote.url) + with mock.patch('modules.repos.downloader.GitDownloader._default_branch', return_value='--upload-pack=x'): + await repos_download_manager.wait_for_all_downloads() + + download, = test_session.query(Download).all() + assert download.status == DownloadStatus.deferred + assert 'Invalid branch' in download.error + assert not repo.directory.exists() diff --git a/modules/repos/test/test_lib.py b/modules/repos/test/test_lib.py index e541f04f..8182bf1f 100644 --- a/modules/repos/test/test_lib.py +++ b/modules/repos/test/test_lib.py @@ -6,7 +6,7 @@ from modules.repos.models import Repository from wrolpi.collections.models import Collection from wrolpi.common import get_wrolpi_config from wrolpi.downloader import Download, DownloadFrequency -from wrolpi.errors import UnknownDirectory +from wrolpi.errors import UnknownDirectory, ValidationError from wrolpi.files.models import FileGroup @@ -34,6 +34,9 @@ def test_parse_repo_url(url, expected): 'https://user:token@github.com/kiwix/kiwix-tools', 'https://github.com/kiwix/kiwix-tools?tab=readme', 'https://github.com', + 'https://github.com/a/..', + 'https://github.com/../b', + 'https://github.com/a/./b', '', ]) def test_parse_repo_url_invalid(url): @@ -208,3 +211,68 @@ async def test_repos_directory_is_ignored(test_session, test_directory, test_wro await refresh_files() assert [i.primary_path.name for i in test_session.query(FileGroup)] == ['other.txt'] + + +@pytest.mark.parametrize('branch,valid', [ + ('main', True), + ('feature/x-1', True), + ('release-1.2', True), + ('--upload-pack=touch /tmp/pwned', False), + ('-b', False), + ('a..b', False), + ('a b', False), + ('a~1', False), + ('a^', False), + ('a:b', False), + ('a?', False), + ('a*', False), + ('a[', False), + ('a\\b', False), + ('a@{1}', False), + ('@', False), + ('a/', False), + ('/a', False), + ('a//b', False), + ('a.lock', False), + ('a/.b', False), + ('a.', False), + ('a\x00b', False), + ('', False), +]) +def test_is_valid_branch(branch, valid): + assert lib.is_valid_branch(branch) is valid + + +@pytest.mark.asyncio +async def test_invalid_branch_rejected(test_session, async_client, test_wrolpi_config): + """A branch is passed to git, it must never be read as an option.""" + with pytest.raises(ValidationError): + lib.create_repository(test_session, 'https://github.com/a/b', branch='--upload-pack=x') + with pytest.raises(ValidationError): + lib.create_repository(test_session, 'https://github.com/a/b/tree/a..b') + repo = lib.create_repository(test_session, 'https://github.com/a/b') + with pytest.raises(ValidationError): + lib.update_repository(test_session, repo.id, branch='-x') + + +@pytest.mark.asyncio +async def test_repo_directory_traversal(test_session, test_directory, async_client, test_wrolpi_config): + """A Repo's directory is always strictly inside the media directory, and never escapes its template.""" + for name in ('..', '.', ' '): + with pytest.raises(InvalidRepo): + lib.create_repository(test_session, 'https://github.com/a/b', tag_name='tag', name=name) + test_session.rollback() + + with pytest.raises(InvalidRepo): + lib.create_repository(test_session, 'https://github.com/a/b', directory=test_directory / 'repos/../../etc') + test_session.rollback() + with pytest.raises(InvalidRepo): + lib.create_repository(test_session, 'https://github.com/a/b', directory=test_directory) + test_session.rollback() + + # The config can not move an existing Repo outside the media directory. + lib.create_repository(test_session, 'https://github.com/a/b') + with pytest.raises(InvalidRepo): + lib.ReposConfig._import_repo(test_session, dict(url='https://github.com/a/b', directory='../outside')) + test_session.rollback() + assert test_session.query(Repository).one().directory == test_directory / 'repos/b'