From d0ea6ab3132463f67e2cef458b9b936e45f8c4db Mon Sep 17 00:00:00 2001 From: Roland Date: Tue, 6 Oct 2026 18:58:18 -0600 Subject: [PATCH] repos: a clone is owned by a random token, not its URL The ownership marker held the Repo's URL key, which anyone can write, so a copied clone, an extracted archive or a hand-made `.git` passed the check and was reset and cleaned. Each Repo now has a random clone_token (repository column and repos.yaml, so a Repo restored from the config still owns its clone); the marker holds it, and only a clone holding the Repo's token is updated. Co-Authored-By: Claude Opus 5.5 --- .../2026_10_06_1500_repository_clone_token.py | 22 ++++++++ modules/repos/downloader.py | 11 ++-- modules/repos/lib.py | 39 +++++++++---- modules/repos/models.py | 3 + modules/repos/test/test_config.py | 5 +- modules/repos/test/test_ownership.py | 56 ++++++++++++++++++- 6 files changed, 116 insertions(+), 20 deletions(-) create mode 100644 alembic/versions/2026_10_06_1500_repository_clone_token.py diff --git a/alembic/versions/2026_10_06_1500_repository_clone_token.py b/alembic/versions/2026_10_06_1500_repository_clone_token.py new file mode 100644 index 00000000..61d936af --- /dev/null +++ b/alembic/versions/2026_10_06_1500_repository_clone_token.py @@ -0,0 +1,22 @@ +"""A git Repo's clone holds a random token: repository.clone_token. + +Revision ID: 2026_10_06_1500 +Revises: 2026_10_06_1400 +""" +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = '2026_10_06_1500' +down_revision = '2026_10_06_1400' +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column('repository', sa.Column('clone_token', sa.String(), nullable=True)) + + +def downgrade(): + with op.batch_alter_table('repository') as batch_op: + batch_op.drop_column('clone_token') diff --git a/modules/repos/downloader.py b/modules/repos/downloader.py index a1ee2858..2944d4fc 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 + sanitize_git_config, new_clone_token from .models import Repository __all__ = ['GitDownloader', 'git_downloader', 'PreparedRepo', 'ExecutedRepo', 'parse_git_progress'] @@ -44,6 +44,7 @@ class PreparedRepo: mode: str branch: Optional[str] submodules: bool = False + clone_token: Optional[str] = None # The repo's host, resolved once by execute_download; every fetch from it connects to these addresses. addresses: Optional[HostAddresses] = None @@ -85,8 +86,10 @@ class GitDownloader(Downloader): if not repo: raise DownloadError(f'No Repo for this download, add it on the Repos page: {download.url}') directory = repo.collection.get_or_set_directory(session) + if not repo.clone_token: + repo.clone_token = new_clone_token() return PreparedRepo(repository_id=repo.id, url=repo.url, directory=directory, mode=repo.mode, - branch=repo.branch, submodules=repo.submodules) + branch=repo.branch, submodules=repo.submodules, clone_token=repo.clone_token) async def _git(self, download: Download, ctx: DownloadContext, cwd: pathlib.Path, *args, config: tuple = (), progress: bool = False, check: bool = True, @@ -127,7 +130,7 @@ class GitDownloader(Downloader): args.extend(['--depth', '1', '--single-branch', '--no-tags']) args.extend(['--', prepared.url, str(directory)]) await self._git(download, ctx, directory.parent, *args, progress=True, prepared=prepared) - mark_clone(directory, prepared.url) + mark_clone(directory, prepared.clone_token) async def _update(self, download: Download, ctx: DownloadContext, prepared: PreparedRepo, branch: str): directory = prepared.directory @@ -136,7 +139,7 @@ class GitDownloader(Downloader): # Only a clone WROLPi made (or imported) for this Repo is reset and cleaned; its config cannot change what # git runs or where it connects, and it is fetched from the Repo's URL, whatever its origin says. - if error := clone_ownership_error(directory, prepared.url): + if error := clone_ownership_error(directory, prepared.clone_token): raise DownloadError(error) try: await asyncio.to_thread(sanitize_git_config, directory) diff --git a/modules/repos/lib.py b/modules/repos/lib.py index 1870dfcf..2ceb6aa9 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -4,6 +4,8 @@ import ipaddress import os import pathlib import re +import hmac +import secrets import shutil import socket import stat @@ -316,9 +318,13 @@ def git_env() -> dict: return env -# WROLPi marks the clones it makes (and imports) with this file in `.git`; it holds the Repo's `repo_url_key`. +# WROLPi marks the clones it makes (and imports) with this file in `.git`; it holds the Repo's `clone_token`. REPO_MARKER = 'wrolpi-repo' + +def new_clone_token() -> str: + return secrets.token_hex(16) + # The only settings kept in a clone's config: anything else (include.path, url.*.insteadOf, http.proxy, # core.sshCommand, ...) could change what git runs or where it connects. ALLOWED_GIT_CONFIG_RE = re.compile( @@ -339,8 +345,8 @@ def _borrowing_git_file(git_dir: pathlib.Path) -> Optional[str]: return None -def clone_ownership_error(directory: pathlib.Path, url: str) -> Optional[str]: - """Why the clone in `directory` must not be updated as the Repo with `url`, or None. +def clone_ownership_error(directory: pathlib.Path, clone_token: Optional[str]) -> Optional[str]: + """Why the clone in `directory` must not be updated as the Repo with `clone_token`, or None. WROLPi resets and cleans its clones, so it only updates the ones it made (or imported) for this Repo.""" git_dir = directory / '.git' @@ -349,19 +355,20 @@ def clone_ownership_error(directory: pathlib.Path, url: str) -> Optional[str]: if borrowed := _borrowing_git_file(git_dir): return f'{directory} uses another git directory (.git/{borrowed})' marker = git_dir / REPO_MARKER - if marker.is_symlink() or not marker.is_file() or marker.read_text().strip() != repo_url_key(url): + if not clone_token or marker.is_symlink() or not marker.is_file() \ + or not hmac.compare_digest(marker.read_text()[:100].strip(), clone_token): return f'{directory} was not created by WROLPi. Import it from the Repos page' return None -def mark_clone(directory: pathlib.Path, url: str): +def mark_clone(directory: pathlib.Path, clone_token: str): marker = directory / '.git' / REPO_MARKER # The clone may hold a link here (e.g. one being imported); never write through it. if marker.is_symlink(): marker.unlink() fd = os.open(marker, os.O_WRONLY | os.O_CREAT | os.O_TRUNC | os.O_NOFOLLOW, 0o644) with os.fdopen(fd, 'w') as fh: - fh.write(f'{repo_url_key(url)}\n') + fh.write(f'{clone_token}\n') def _git_config_entries(config_file: pathlib.Path) -> List[Tuple[str, str]]: @@ -653,10 +660,12 @@ def _validate_frequency(frequency: int): def create_repository(session: Session, url: str, tag_name: str = None, frequency: int = DEFAULT_REPO_FREQUENCY, mode: str = 'full', branch: str = None, name: str = None, description: str = None, - directory: pathlib.Path = None, submodules: bool = False) -> Repository: + directory: pathlib.Path = None, submodules: bool = False, + clone_token: str = None) -> Repository: """Create a Repo (Collection + Repository + recurring Download). The download clones it.""" _validate_frequency(frequency) - repo = _create_repository_records(session, url, tag_name, mode, branch, name, description, directory, submodules) + repo = _create_repository_records(session, url, tag_name, mode, branch, name, description, directory, submodules, + clone_token) _create_repository_download(session, repo, frequency) return repo @@ -674,7 +683,7 @@ def _create_repository_download(session: Session, repo: Repository, frequency: i def _create_repository_records(session: Session, url: str, tag_name: str, mode: str, branch: Optional[str], name: Optional[str], description: Optional[str], directory: Optional[pathlib.Path], - submodules: bool) -> Repository: + submodules: bool, clone_token: str = None) -> Repository: """Create (but do not commit) a Repo's Collection and Repository, in their directory.""" parsed = parse_repo_url(url) _validate_mode(mode) @@ -695,7 +704,8 @@ def _create_repository_records(session: Session, url: str, tag_name: str, mode: session.add(collection) session.flush([collection]) repo = Repository(url=parsed.url, host=parsed.host, owner=parsed.owner, mode=mode, branch=branch, - submodules=bool(submodules), collection=collection) + submodules=bool(submodules), clone_token=clone_token or new_clone_token(), + collection=collection) repo.update_search_name() session.add(repo) session.flush([repo]) @@ -873,7 +883,7 @@ async def import_repository(session: Session, directory: str, url: str = None, c 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, parsed.url) + mark_clone(destination, repo.clone_token) for key, value in _read_head_commit(destination).items(): setattr(repo, key, value) @@ -1251,7 +1261,8 @@ class ReposConfig(ConfigFile): if not repo: return create_repository(session, parsed.url, tag_name=tag_name, frequency=frequency, mode=mode, branch=data.get('branch'), name=name, description=data.get('description'), - directory=directory, submodules=bool(data.get('submodules'))) + directory=directory, submodules=bool(data.get('submodules')), + clone_token=data.get('clone_token')) collection = repo.collection collection.name = name @@ -1262,6 +1273,8 @@ class ReposConfig(ConfigFile): collection.directory = directory repo.mode = mode repo.submodules = bool(data.get('submodules')) + if data.get('clone_token'): + repo.clone_token = data['clone_token'] branch = data.get('branch') or None _validate_branch(branch) repo.branch = branch @@ -1291,6 +1304,8 @@ class ReposConfig(ConfigFile): branch=repo.branch, submodules=repo.submodules, frequency=download.frequency if download else None, + # Restoring a Repo from the config keeps its clone. + clone_token=repo.clone_token, ) data.append(entry) diff --git a/modules/repos/models.py b/modules/repos/models.py index 607afd78..6920a1bb 100644 --- a/modules/repos/models.py +++ b/modules/repos/models.py @@ -31,6 +31,9 @@ class Repository(ModelHelper, Base): mode = Column(String, nullable=False, default='full') # The branch to check out; None follows the remote's default branch. branch = Column(String) + # Random; written in the clone's .git (see lib.mark_clone). Only a clone holding it is updated, so a clone which + # WROLPi did not make for this Repo is never reset or cleaned. + clone_token = Column(String) # Also download the repo's submodules. submodules = Column(Boolean, nullable=False, default=False, server_default=false()) default_branch = Column(String) diff --git a/modules/repos/test/test_config.py b/modules/repos/test/test_config.py index 862b9b3b..b6b98974 100644 --- a/modules/repos/test/test_config.py +++ b/modules/repos/test/test_config.py @@ -16,7 +16,10 @@ async def test_dump_and_import(test_session, test_directory, async_client, test_ config = lib.get_repos_config() assert config.get_file().is_file() - assert config.repos == [ + # Each Repo's random clone token is kept, so a restored Repo still owns its clone. + dumped = [dict(i) for i in config.repos] + assert all(len(i.pop('clone_token')) == 32 for i in dumped) + assert dumped == [ dict(name='b', url='https://github.com/a/b', directory='repos/b', tag_name=None, description=None, mode='snapshot', branch='dev', submodules=False, frequency=DownloadFrequency.weekly), dict(name='kiwix-tools', url='https://github.com/kiwix/kiwix-tools', directory='repos/software/kiwix-tools', diff --git a/modules/repos/test/test_ownership.py b/modules/repos/test/test_ownership.py index 07832908..18430f37 100644 --- a/modules/repos/test/test_ownership.py +++ b/modules/repos/test/test_ownership.py @@ -26,7 +26,10 @@ async def test_clone_is_marked(test_session, git_remote_factory, repos_download_ remote = git_remote_factory() repo = lib.create_repository(test_session, remote.url) await repos_download_manager.wait_for_all_downloads() - assert (repo.directory / '.git' / lib.REPO_MARKER).read_text().strip() == lib.repo_url_key(remote.url) + test_session.expire_all() + # The marker holds the Repo's own random token, which nothing else can know (unlike its URL). + assert len(repo.clone_token) == 32 + assert (repo.directory / '.git' / lib.REPO_MARKER).read_text().strip() == repo.clone_token @pytest.mark.asyncio @@ -178,10 +181,10 @@ def test_marker_and_config_do_not_follow_links(tmp_path): victim.write_text('precious') (git_dir / lib.REPO_MARKER).symlink_to(victim) - lib.mark_clone(repo, 'https://github.com/a/b') + lib.mark_clone(repo, 'a1b2c3') assert victim.read_text() == 'precious' assert not (git_dir / lib.REPO_MARKER).is_symlink() - assert (git_dir / lib.REPO_MARKER).read_text().strip() == 'github.com/a/b' + assert (git_dir / lib.REPO_MARKER).read_text().strip() == 'a1b2c3' (git_dir / 'config').write_text('[http]\n\tproxy = evil\n[core]\n\tbare = false\n') (git_dir / 'config.wrolpi').symlink_to(victim) @@ -348,3 +351,50 @@ def test_nested_submodule_git_directories(tmp_path): (inner / 'commondir').write_text(f'{tmp_path}\n') with pytest.raises(ValueError): lib.sanitize_git_config(repo) + + + +@pytest.mark.asyncio +async def test_copied_clone_is_not_owned(test_session, test_directory, git_remote_factory, repos_download_manager): + """A clone with a marker WROLPi did not write (e.g. copied from another WROLPi, or made by hand) is not owned: + the marker must hold this Repo's own token.""" + remote = git_remote_factory() + directory = test_directory / 'repos/example' + directory.parent.mkdir(parents=True) + git(directory.parent, 'clone', '--quiet', str(remote.path), str(directory)) + # What a marker used to hold, which anyone could write. + (directory / '.git' / lib.REPO_MARKER).write_text(lib.repo_url_key(remote.url)) + (directory / 'my-work.txt').write_text('uncommitted') + + lib.create_repository(test_session, remote.url) + await repos_download_manager.wait_for_all_downloads() + download = test_session.query(Download).one() + assert download.status == DownloadStatus.deferred + assert 'was not created by WROLPi' in download.error + assert (directory / 'my-work.txt').exists() + + +@pytest.mark.asyncio +async def test_clone_token_survives_the_config(test_session, async_client, git_remote_factory, + repos_download_manager, await_switches): + """The token is in repos.yaml, so a Repo restored from the config still owns its clone.""" + remote = git_remote_factory() + repo = lib.create_repository(test_session, remote.url) + await repos_download_manager.wait_for_all_downloads() + await await_switches() + token = repo.clone_token + config = lib.get_repos_config() + assert config.repos[0]['clone_token'] == token + + # Lose the database, restore it from the config. + lib._delete_repository(test_session, repo) + test_session.commit() + config.import_config() + test_session.expire_all() + restored = test_session.query(lib.Repository).one() + assert restored.clone_token == token + + head = remote.commit({'new.txt': 'new'}, 'New') + 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