repos: validate branch names and repo directories

Branch names are passed to git as arguments and come from the API, the
config, a `/tree/<branch>` URL, or the remote itself (its default
branch).  A name starting with `-` would be read as an option, so every
branch must pass git's ref-name rules, including in the downloader just
before use.

A repo URL with a `.` or `..` path segment (or a `..` name) could format
a directory outside its template (e.g. `repos/<tag>/..` is the whole
repos directory).  Such URLs and names are rejected, and every repo
directory, including one from repos.yaml, must be strictly inside the
media directory with no `..`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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:

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:

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()

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'