From 4b034ae3eec8d98b4580aee29023a7bc874ceeef Mon Sep 17 00:00:00 2001 From: Roland Date: Tue, 6 Oct 2026 19:03:07 -0600 Subject: [PATCH] repos: submodule links must stay in the repo; import checks before it moves * A submodule's working-tree `.git` names the git directory git uses for it. Before any submodule command (update, or deinit when submodules are turned off), and before an import, every `.git` below the top of the working tree must be a file pointing inside the repo's own .git/modules; a link, a directory, or a file pointing elsewhere could make git use a config that was never checked. * Import validates and sanitizes the clone where it is, then moves it, and moves it back if marking it fails, so a refused or failed import never leaves a clone moved without a Repo. Co-Authored-By: Claude Opus 5.5 --- modules/repos/downloader.py | 6 ++- modules/repos/lib.py | 71 +++++++++++++++++++++++----- modules/repos/test/test_import.py | 30 ++++++++++++ modules/repos/test/test_ownership.py | 48 +++++++++++++++++++ 4 files changed, 141 insertions(+), 14 deletions(-) diff --git a/modules/repos/downloader.py b/modules/repos/downloader.py index 2944d4fc..be41c7f0 100644 --- a/modules/repos/downloader.py +++ b/modules/repos/downloader.py @@ -13,7 +13,7 @@ from wrolpi.downloader import Downloader, Download, DownloadContext, DownloadRes from wrolpi.errors import DownloadError from .lib import git_command, git_env, find_readme, get_repo_size, is_valid_branch, read_readme_text, \ submodule_fetch_config, resolve_repo_addresses, pin_config, HostAddresses, clone_ownership_error, mark_clone, \ - sanitize_git_config, new_clone_token + sanitize_git_config, new_clone_token, working_tree_git_error from .models import Repository __all__ = ['GitDownloader', 'git_downloader', 'PreparedRepo', 'ExecutedRepo', 'parse_git_progress'] @@ -185,6 +185,10 @@ class GitDownloader(Downloader): git commands that git runs).""" directory = prepared.directory git = lambda *args, **kwargs: self._git(download, ctx, directory, *args, **kwargs) # noqa: E731 + # Every submodule command uses the git directory a submodule's `.git` names; it must be one that was checked. + if (error := await asyncio.to_thread(working_tree_git_error, directory)) and ( + prepared.submodules or (directory / '.git/modules').is_dir()): + return error if not prepared.submodules: if (directory / '.git/modules').is_dir(): # Submodules were turned off; the repo is a mirror of only its own files now. diff --git a/modules/repos/lib.py b/modules/repos/lib.py index 2ceb6aa9..ab95e80d 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -37,14 +37,14 @@ from .models import Repository, REPO_MODES logger = logger.getChild(__name__) __all__ = ['ParsedRepoURL', 'parse_repo_url', 'repo_url_key', 'git_command', 'git_env', 'REPO_MARKER', - 'clone_ownership_error', 'mark_clone', 'sanitize_git_config', 'create_repository', 'remote_url_key', - 'remote_https_url', 'resolve_import_directory', 'inspect_import', 'import_repository', - 'update_repository', 'delete_repository', 'get_repositories', 'list_repo_tree', 'find_readme', - 'repos_destination_root', 'validate_repos_destination', 'is_valid_branch', 'submodule_url_error', - 'HostAddresses', 'resolve_repo_addresses', 'pin_config', 'is_public_address', 'submodule_fetch_config', - 'read_readme_text', 'search_repos', 'count_repos', 'search_repos_by_name', 'get_repo_log', - 'get_repo_root', 'repo_archive_name', 'stream_repo_archive', 'ReposConfig', 'repos_config', - 'get_repos_config', 'save_repos_config', 'import_repos_config', 'DEFAULT_REPO_FREQUENCY'] + 'clone_ownership_error', 'mark_clone', 'sanitize_git_config', 'working_tree_git_error', + 'create_repository', 'remote_url_key', 'remote_https_url', 'resolve_import_directory', 'inspect_import', + 'import_repository', 'update_repository', 'delete_repository', 'get_repositories', 'list_repo_tree', + 'find_readme', 'repos_destination_root', 'validate_repos_destination', 'is_valid_branch', + 'submodule_url_error', 'HostAddresses', 'resolve_repo_addresses', 'pin_config', 'is_public_address', + 'submodule_fetch_config', 'read_readme_text', 'search_repos', 'count_repos', 'search_repos_by_name', + 'get_repo_log', 'get_repo_root', 'repo_archive_name', 'stream_repo_archive', 'ReposConfig', + 'repos_config', 'get_repos_config', 'save_repos_config', 'import_repos_config', 'DEFAULT_REPO_FREQUENCY'] DEFAULT_REPO_FREQUENCY = DownloadFrequency.weekly.value @@ -472,6 +472,35 @@ def _sanitize_submodule_git_dirs(modules: pathlib.Path, root: pathlib.Path): _sanitize_submodule_git_dirs(child / 'modules', root) +def working_tree_git_error(directory: pathlib.Path) -> Optional[str]: + """Why a `.git` in the repo's working tree (below its top) must not be used, or None. + + A submodule's `.git` file names the git directory git uses for it. It must point inside the repo's own + .git/modules, which is what `sanitize_git_config` checks; a link, a directory, or a file pointing elsewhere could + make git use a config which was never checked.""" + root = directory.resolve() + modules = root / '.git' / 'modules' + for dir_path, dir_names, file_names in os.walk(root): + here = pathlib.Path(dir_path) + if '.git' in dir_names: + dir_names.remove('.git') # Never walk into a git directory. + if here != root: + return f'{here / ".git"} is a directory, not a submodule\'s link' + continue + if '.git' not in file_names: + continue + git_file = here / '.git' + if git_file.is_symlink(): + return f'{git_file} is a link' + content = git_file.read_text(errors='replace')[:4096].strip() + if not content.startswith('gitdir: ') or '\n' in content: + return f'{git_file} is not a submodule\'s link' + target = (here / content[len('gitdir: '):]).resolve() + if modules not in target.parents: + return f'{git_file} points outside the repo\'s submodules' + return None + + def find_readme(directory: pathlib.Path) -> Optional[str]: """Return the name of the preferred README file in `directory`, if any.""" try: @@ -869,22 +898,38 @@ async def import_repository(session: Session, directory: str, url: str = None, c repo = _create_repository_records(session, parsed.url, tag_name, mode, branch, info['name'], description, None, submodules) destination = repo.directory - if destination.resolve() != source: + moving = destination.resolve() != source + if moving: if source in destination.resolve().parents: raise InvalidRepo('A repo cannot be moved into itself') if destination.exists(): raise RepoConflict(f'{get_relative_to_media_directory(destination)} already exists') + + # Everything which can refuse the import happens before the clone is moved. + if error := await asyncio.to_thread(working_tree_git_error, source): + raise InvalidRepo(f'{directory} cannot be imported: {error}') + try: + await asyncio.to_thread(sanitize_git_config, source) + except ValueError as e: + raise InvalidRepo(f'{directory} cannot be imported: {e}') + await asyncio.to_thread(_git_output_sync, source, 'config', '--replace-all', 'remote.origin.url', parsed.url) + + if moving: destination.parent.mkdir(parents=True, exist_ok=True) await asyncio.to_thread(shutil.move, source, destination) logger.info(f'Imported repo moved from {source} to {destination}') + try: + mark_clone(destination, repo.clone_token) + except BaseException: + if moving: + # Leave the clone where it was found. + await asyncio.to_thread(shutil.move, destination, source) + raise + if moving: # Forget the files that were indexed in the old location. from wrolpi.files.worker import file_worker file_worker.queue_refresh([source]) - await asyncio.to_thread(sanitize_git_config, destination) - await asyncio.to_thread(_git_output_sync, destination, 'config', '--replace-all', 'remote.origin.url', parsed.url) - mark_clone(destination, repo.clone_token) - for key, value in _read_head_commit(destination).items(): setattr(repo, key, value) repo.readme_path = find_readme(destination) diff --git a/modules/repos/test/test_import.py b/modules/repos/test/test_import.py index 612f9867..54e852a0 100644 --- a/modules/repos/test/test_import.py +++ b/modules/repos/test/test_import.py @@ -207,3 +207,33 @@ async def test_import_refuses(test_session, test_directory, async_client, test_w repo = await lib.import_repository(test_session, 'code/ssh-clone', url='https://git.example.com/owner/library', confirm=True) assert repo.url == 'https://git.example.com/owner/library' + + +@pytest.mark.asyncio +async def test_import_checks_before_moving(test_session, test_directory, async_client, test_wrolpi_config, + user_clone): + """Everything which can refuse an import is checked before the clone is moved.""" + directory, _ = user_clone() + (directory / 'other').mkdir() + (directory / 'other/.git').write_text(f'gitdir: {test_directory}\n') + with pytest.raises(InvalidRepo, match='outside'): + await lib.import_repository(test_session, 'code/kiwix-tools', confirm=True) + test_session.rollback() + assert (directory / '.git').is_dir() + assert not (test_directory / 'repos/kiwix-tools').exists() + assert test_session.query(Repository).count() == 0 + + +@pytest.mark.asyncio +async def test_import_moves_back_on_failure(test_session, test_directory, async_client, test_wrolpi_config, + user_clone): + """If the import fails after the move, the clone is moved back.""" + from mock import mock + directory, _ = user_clone() + with mock.patch('modules.repos.lib.mark_clone', side_effect=OSError('disk full')): + with pytest.raises(OSError): + await lib.import_repository(test_session, 'code/kiwix-tools', confirm=True) + test_session.rollback() + assert (directory / '.git').is_dir() + assert not (test_directory / 'repos/kiwix-tools').exists() + assert test_session.query(Repository).count() == 0 diff --git a/modules/repos/test/test_ownership.py b/modules/repos/test/test_ownership.py index 18430f37..f59e1465 100644 --- a/modules/repos/test/test_ownership.py +++ b/modules/repos/test/test_ownership.py @@ -398,3 +398,51 @@ async def test_clone_token_survives_the_config(test_session, async_client, git_r download = await update(test_session, repos_download_manager) assert download.status == DownloadStatus.complete, download.error assert test_session.query(lib.Repository).one().head_sha == head + + +@pytest.mark.parametrize('plant', ['outside', 'directory', 'link', 'garbage']) +def test_working_tree_git_files(tmp_path, plant): + """A submodule's `.git` (in the working tree) decides which git directory git uses for it; it must be a file + pointing inside the repo's own .git/modules.""" + repo = tmp_path / 'repo' + (repo / '.git/modules/library').mkdir(parents=True) + (repo / 'vendor/library').mkdir(parents=True) + (repo / 'vendor/library/.git').write_text('gitdir: ../../.git/modules/library\n') + assert lib.working_tree_git_error(repo) is None + + evil = tmp_path / 'evil' + evil.mkdir() + (repo / 'other').mkdir() + target = repo / 'other/.git' + if plant == 'outside': + target.write_text(f'gitdir: {evil}\n') + elif plant == 'directory': + target.mkdir() + elif plant == 'link': + target.symlink_to(evil) + elif plant == 'garbage': + target.write_text('not a gitdir line\n') + assert lib.working_tree_git_error(repo) + + +@pytest.mark.parametrize('submodules', [True, False]) +@pytest.mark.asyncio +async def test_update_refuses_planted_submodule_git_file(test_session, test_directory, git_remote_factory, + repos_download_manager, submodules): + """Submodule commands (update, and deinit when submodules are turned off) never use a git directory outside + the repo.""" + library = git_remote_factory('library') + remote = git_remote_factory() + remote.add_submodule('vendor/library', library) + repo = lib.create_repository(test_session, remote.url, submodules=True) + await repos_download_manager.wait_for_all_downloads() + assert (repo.directory / 'vendor/library/lib.c').exists() or (repo.directory / 'vendor/library/README.md').exists() + + evil = test_directory / 'evil-gitdir' + evil.mkdir() + (repo.directory / 'vendor/library/.git').write_text(f'gitdir: {evil}\n') + lib.update_repository(test_session, repo.id, submodules=submodules) + download = await update(test_session, repos_download_manager) + assert download.status == DownloadStatus.deferred + assert 'outside' in download.error + assert list(evil.iterdir()) == []