diff --git a/modules/repos/downloader.py b/modules/repos/downloader.py index d081d706..cf85c6de 100644 --- a/modules/repos/downloader.py +++ b/modules/repos/downloader.py @@ -189,30 +189,20 @@ class GitDownloader(Downloader): raise DownloadError(f'Submodules are nested more than {MAXIMUM_SUBMODULE_DEPTH} levels deep') git = lambda *args, **kwargs: self._git(download, ctx, directory, *args, **kwargs) # noqa: E731 - # NUL-delimited: a submodule's name and URL can contain spaces and newlines. Each record is "key\nvalue". - output = await git('config', '--file', '.gitmodules', '-z', '--get-regexp', r'^submodule\..*\.(url|path)$', - check=False) - urls, paths = [], [] - for record in output.split('\0'): - if not record: - continue - key, newline, value = record.partition('\n') - if not newline or not key.startswith('submodule.'): - raise DownloadError(f'Refusing to download submodules, cannot read .gitmodules: {record!r}') - if key.endswith('.url'): - urls.append(value) - elif key.endswith('.path'): - paths.append(value) - else: - raise DownloadError(f'Refusing to download submodules, cannot read .gitmodules: {record!r}') + paths = await self._git_config_values(download, ctx, directory, '.path', '--file', '.gitmodules') + + # git resolves each submodule's URL (a relative URL is resolved against this repo's remote, and may be + # another host) into this repo's config. Those are the URLs git fetches, so those are checked. + await git('submodule', 'init') + await git('submodule', 'sync') + urls = await self._git_config_values(download, ctx, directory, '.url', '--local') repo_host = (urlparse(prepared.url).hostname or '').lower() try: fetch_config = await asyncio.to_thread(submodule_fetch_config, urls, repo_host) except ValueError as e: raise DownloadError(str(e)) - await git('submodule', 'sync') - args = ['submodule', 'update', '--init', '--force', '--progress'] + args = ['submodule', 'update', '--force', '--progress'] if prepared.mode == 'snapshot': args.extend(['--depth', '1']) await git(*args, progress=True, config=fetch_config) @@ -222,6 +212,22 @@ class GitDownloader(Downloader): if submodule.is_dir() and directory.resolve() in submodule.parents: await self._check_out_submodules(download, ctx, submodule, prepared, depth + 1) + async def _git_config_values(self, download: Download, ctx: DownloadContext, directory: pathlib.Path, + suffix: str, *source: str) -> list: + """The values of the `submodule.` keys in a git config. Refuses anything it cannot read.""" + # NUL-delimited: a submodule's name and URL can contain spaces and newlines. Each record is "key\nvalue". + output = await self._git(download, ctx, directory, 'config', *source, '-z', '--get-regexp', + rf'^submodule\..*\{suffix}$', check=False) + values = [] + for record in output.split('\0'): + if not record: + continue + key, newline, value = record.partition('\n') + if not newline or not key.startswith('submodule.') or not key.endswith(suffix): + raise DownloadError(f'Refusing to download submodules, cannot read their config: {record!r}') + values.append(value) + return values + async def _is_ancestor(self, download: Download, ctx: DownloadContext, directory: pathlib.Path, ancestor: str, descendant: str) -> bool: cmd = git_command('merge-base', '--is-ancestor', ancestor, descendant) diff --git a/modules/repos/lib.py b/modules/repos/lib.py index c1f773e0..39974ef3 100644 --- a/modules/repos/lib.py +++ b/modules/repos/lib.py @@ -134,14 +134,13 @@ ABSOLUTE_SUBMODULE_URL_RE = re.compile( r'^https://(?P[A-Za-z0-9.-]+|\[[0-9A-Fa-f:.]+\])' r'(?::(?P[0-9]{1,5}))?' r'(?:/[A-Za-z0-9._~%!$&\'()*+,;=:/-]*)?$') -RELATIVE_SUBMODULE_URL_RE = re.compile(r'^\.\.?/[A-Za-z0-9._~%/-]*$') def _check_submodule_url(url: str, repo_host: str) -> Tuple[Optional[str], Optional[Tuple[str, int, List[str]]]]: - """Returns why the URL must not be fetched (or None), and the (host, port, addresses) to pin it to (or None).""" - if RELATIVE_SUBMODULE_URL_RE.match(url): - # Relative to the repo's own remote. - return None, None + """Returns why the URL must not be fetched (or None), and the (host, port, addresses) to pin it to (or None). + + `url` must be absolute: git resolves relative submodule URLs (which can climb to another host, e.g. + `../../../10.0.0.1/x`), and the result is checked.""" match = ABSOLUTE_SUBMODULE_URL_RE.match(url) if not match: return 'only plain https:// submodule URLs can be downloaded', None @@ -168,8 +167,8 @@ def submodule_url_error(url: str, repo_host: str) -> Optional[str]: """Why a submodule URL must not be fetched, or None when it may be. A submodule's URL is chosen by whoever controls the repo, so it may not make WROLPi fetch from its own network - (e.g. `https://localhost:8443/...` or `https://10.0.0.1/...`). Allowed: URLs relative to the repo's remote, - the repo's own host (which the user chose), and hosts with only public addresses.""" + (e.g. `https://localhost:8443/...` or `https://10.0.0.1/...`). Allowed: the repo's own host (which the user + chose), and hosts with only public addresses. A relative URL must first be resolved by git.""" return _check_submodule_url(url, repo_host)[0] diff --git a/modules/repos/test/test_extras.py b/modules/repos/test/test_extras.py index 9e181f2f..d7510356 100644 --- a/modules/repos/test/test_extras.py +++ b/modules/repos/test/test_extras.py @@ -187,8 +187,9 @@ async def test_archive_after_session_closes(test_session, async_client, git_remo @pytest.mark.parametrize('url,resolved,allowed', [ - ('../library.git', None, True), # Relative to the repo's own remote. - ('./library', None, True), + # Relative URLs are resolved by git (against the repo's remote) before they are checked. + ('../library.git', None, False), + ('./library', None, False), ('https://git.example.com/owner/library', None, True), # The repo's own host. ('https://codeberg.org/owner/library', ['217.197.91.145'], True), ('https://localhost/library', ['127.0.0.1'], False), @@ -295,9 +296,36 @@ def test_submodule_fetch_config(): fetch.""" with mock.patch('modules.repos.lib.resolve_host_addresses', return_value=['217.197.91.145', '2606:4700::1111']): config = lib.submodule_fetch_config(['https://codeberg.org/a/b', 'https://codeberg.org:8443/c/d', - '../relative', 'https://git.example.com/own/host'], 'git.example.com') + 'https://git.example.com/own/host'], 'git.example.com') assert config == ( 'http.followRedirects=false', 'http.curloptResolve=codeberg.org:443:217.197.91.145,[2606:4700::1111]', 'http.curloptResolve=codeberg.org:8443:217.197.91.145,[2606:4700::1111]', ) + + + +@pytest.mark.parametrize('relative_url,allowed', [ + ('../library', True), # https://git.example.com/owner/library + ('../../../10.0.0.5/library', False), # git resolves this to https://10.0.0.5/library +]) +@pytest.mark.asyncio +async def test_relative_submodule_url(test_session, git_remote_factory, repos_download_manager, relative_url, + allowed): + """A relative submodule URL is checked where git resolves it, which may be another host.""" + library = git_remote_factory('library') + remote = git_remote_factory() + remote.add_submodule('vendor/library', library, url=relative_url) + + repo = lib.create_repository(test_session, remote.url, submodules=True) + await repos_download_manager.wait_for_all_downloads() + test_session.expire_all() + + download = test_session.query(Download).one() + if allowed: + assert download.status == DownloadStatus.complete, download.error + assert (repo.directory / 'vendor/library/README.md').is_file() + else: + assert download.status == DownloadStatus.deferred + assert 'Refusing to download submodule https://10.0.0.5/library' in download.error + assert not (repo.directory / 'vendor/library/README.md').exists()