Skip to content

Commit 447c8bf

Browse files
committed
fix(submodule): apply git's transport policy to .gitmodules URLs
`Submodule.update` clones a submodule from the URL recorded in `.gitmodules`, and fetches the remotes of an initialized submodule, with the transport policy git applies to a URL the user typed. Since git 2.38.1 (CVE-2022-39253) `protocol.file.allow` defaults to `user`, and `git submodule update` runs its clones and fetches with `GIT_PROTOCOL_FROM_USER=0`, so git refuses the `file` transport for a URL that comes from `.gitmodules` unless `protocol.file.allow=always` is configured. GitPython did not set that variable, so a `.gitmodules` like [submodule "module"] path = module url = /path/on/the/victims/machine made `Repo.submodule_update(init=True)` and `Submodule.update(init=True)` clone that local repository into the checkout, and `RootModule.update` fetch from such a URL when a new commit changed it, while `git submodule update --init` fails with `fatal: transport 'file' not allowed`. Whoever authored the superproject controls `.gitmodules`, so a tool that updates the submodules of repositories it is given copied a local repository of the author's choosing through git's local clone path, which hardlinks objects and skips the fsck checks a transport fetch applies. `Submodule.update` now runs the clone and the fetch of every remote with `GIT_PROTOCOL_FROM_USER=0`, and `RootModule.update` does the same for the fetch from a changed URL. Git then applies its own policy: `http`, `https`, `git` and `ssh` are unaffected, while `file` and any other protocol whose `protocol.<name>.allow` is `user` is refused unless `always` is configured, the same opt-in `git submodule update` honors. `allow_unsafe_protocols=True` on `Submodule.update` keeps treating the URL as the caller's, and a `GIT_PROTOCOL_FROM_USER` value in the `env` passed to `update` still wins. `Submodule.add` is unchanged because its URL is supplied by the caller. The tests that update submodules from local repositories now set `protocol.file.allow=always`, as the ones that call `git submodule add` already did; the helper for that moved from `test/test_submodule.py` to `test/lib/helper.py` as `patch_git_config` because `test/test_submodule_no_fetch.py` and `test/test_docs.py` need it too. Adds `test_update_applies_git_transport_policy_to_gitmodules_url` and `test_root_update_applies_git_transport_policy_to_changed_url` in `test/test_submodule.py`, which fail before this change and pass after. `ruff check`, `ruff format --check` and `mypy` pass. The pytest suite passes except for eight tests that fail the same way without this change in a clone not prepared with `init-tests-after-clone.sh`.
1 parent ccc4811 commit 447c8bf

6 files changed

Lines changed: 131 additions & 37 deletions

File tree

‎git/objects/submodule/base.py‎

Lines changed: 22 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,21 @@ def _module_abspath(
361361
)
362362
raise NotADirectoryError()
363363

364+
@classmethod
365+
def _transport_env(cls, allow_unsafe_protocols: bool = False) -> Dict[str, str]:
366+
"""
367+
:return:
368+
Environment for clones and fetches whose URL comes from ``.gitmodules``.
369+
370+
:manpage:`git-submodule(1)` runs those with ``GIT_PROTOCOL_FROM_USER=0``, so
371+
that git refuses every transport whose ``protocol.<name>.allow`` policy is
372+
``user``, which it is for ``file`` since git 2.38.1, unless the user
373+
configured ``protocol.<name>.allow=always``.
374+
"""
375+
if allow_unsafe_protocols:
376+
return {}
377+
return {"GIT_PROTOCOL_FROM_USER": "0"}
378+
364379
@classmethod
365380
def _clone_repo(
366381
cls,
@@ -849,7 +864,10 @@ def update(
849864
They only take effect with the `init` option.
850865
851866
:param allow_unsafe_protocols:
852-
Allow unsafe protocols to be used, like ``ext``.
867+
Allow unsafe protocols to be used, like ``ext``. This also runs the clone
868+
and the fetches as if their URL came from the user, so that git accepts a
869+
transport whose ``protocol.<name>.allow`` policy is ``user``, such as
870+
``file``, without ``protocol.<name>.allow=always`` being configured.
853871
854872
:param allow_unsafe_options:
855873
Allow unsafe options to be used, like ``--upload-pack``.
@@ -900,7 +918,8 @@ def fetch_remotes(module_repo: "Repo") -> None:
900918
)
901919
# ===============================
902920
if not dry_run:
903-
remote.fetch(progress=progress)
921+
with module_repo.git.custom_environment(**self._transport_env(allow_unsafe_protocols)):
922+
remote.fetch(progress=progress)
904923
# END handle dry-run
905924
# ===============================
906925
if i == len_rmts - 1:
@@ -987,7 +1006,7 @@ def fetch_remotes(module_repo: "Repo") -> None:
9871006
self.path,
9881007
self.name,
9891008
n=True,
990-
env=env,
1009+
env={**self._transport_env(allow_unsafe_protocols), **(env or {})},
9911010
multi_options=clone_multi_options,
9921011
allow_unsafe_options=allow_unsafe_options,
9931012
allow_unsafe_protocols=allow_unsafe_protocols,

‎git/objects/submodule/root.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -305,7 +305,8 @@ def update( # type: ignore[override]
305305
else:
306306
assert nn not in [r.name for r in rmts]
307307
smr = smm.create_remote(nn, sm_url)
308-
smr.fetch(progress=progress)
308+
with smm.git.custom_environment(**sm._transport_env()):
309+
smr.fetch(progress=progress)
309310

310311
# If we have a tracking branch, it should be available
311312
# in the new remote as well.

‎test/lib/helper.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
"with_rw_directory",
1111
"with_rw_repo",
1212
"with_rw_and_rw_remote_repo",
13+
"patch_git_config",
1314
"PathLikeMock",
1415
"TestBase",
1516
"VirtualEnvironment",
@@ -39,6 +40,7 @@
3940
import textwrap
4041
import time
4142
import unittest
43+
from unittest import mock
4244
import venv
4345
from typing import Union, Type, Tuple
4446

@@ -84,6 +86,26 @@ def fixture(name):
8486
# { Adapters
8587

8688

89+
@contextlib.contextmanager
90+
def patch_git_config(name, value):
91+
"""Temporarily add a git config name-value pair, using environment variables."""
92+
pair_index = int(os.getenv("GIT_CONFIG_COUNT", "0"))
93+
94+
# This is recomputed each time the context is entered, for compatibility with
95+
# existing GIT_CONFIG_* environment variables, even if changed in this process.
96+
patcher = mock.patch.dict(
97+
os.environ,
98+
{
99+
"GIT_CONFIG_COUNT": str(pair_index + 1),
100+
f"GIT_CONFIG_KEY_{pair_index}": name,
101+
f"GIT_CONFIG_VALUE_{pair_index}": value,
102+
},
103+
)
104+
105+
with patcher:
106+
yield
107+
108+
87109
class StringProcessAdapter:
88110
"""Allows strings to be used as process objects returned by subprocess.Popen.
89111

‎test/test_docs.py‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
import os.path
99

1010
from test.lib import TestBase
11-
from test.lib.helper import with_rw_directory, with_rw_repo
11+
from test.lib.helper import patch_git_config, with_rw_directory, with_rw_repo
1212

1313

1414
class Tutorials(TestBase):
@@ -479,6 +479,7 @@ def test_references_and_objects(self, rw_dir):
479479
repo.git.clear_cache()
480480

481481
@with_rw_repo("c15a6e1923a14bc760851913858a3942a4193cdb")
482+
@patch_git_config("protocol.file.allow", "always")
482483
def test_submodules(self, repo):
483484
# [1-test_submodules]
484485
sms = repo.submodules

0 commit comments

Comments
 (0)