Pip test suite submodule handling broken after Git update

[cboylan]

Description

A recent Git security update has made Git far more selective about the submodules that it will allow. In particular file:/// submodules are not accepted by default. The problem here is that pip tests with git submodules and git rejects the setup by default. The good news is that pip constructs the submodule itself which means it controls all of its content. The security issue only appears to be a problem with untrusted git repos. In this case we can simply tell git to trust the submodule instead because pip trusts it.

Expected behavior

The pip testsuite should run and pass without git failures.

pip version

main

Python version

3.8

OS

Ubuntu Focal

How to Reproduce

1. Update your git package on Ubuntu Focal to latest
2. Run the pip testsuite using nox

Output

tests/functional/test_install_vcs_git.py:551: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/lib/git_submodule_helpers.py:73: in _create_test_package_with_submodule
    env.run(
        env        = <tests.lib.PipTestEnvironment object at 0x7ffaf1747250>
        pkg_path   = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg/testpkg')
        rel_path   = 'testpkg/static'
        submodule_path = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg_submodule')
        version_pkg_path = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg')
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <tests.lib.PipTestEnvironment object at 0x7ffaf1747250>
cwd = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg')
allow_stderr_error = False, allow_stderr_warning = False, allow_error = False
args = ('git', 'submodule', 'add', '/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg_submodule', 'testpkg/static')
kw = {'expect_stderr': True}, expect_error = None

    def run(
        self,
        *args: str,
        cwd: Optional[StrPath] = None,
        allow_stderr_error: Optional[bool] = None,
        allow_stderr_warning: Optional[bool] = None,
        allow_error: bool = False,
        **kw: Any,
    ) -> TestPipResult:
        """
        :param allow_stderr_error: whether a logged error is allowed in
            stderr.  Passing True for this argument implies
            `allow_stderr_warning` since warnings are weaker than errors.
        :param allow_stderr_warning: whether a logged warning (or
            deprecation message) is allowed in stderr.
        :param allow_error: if True (default is False) does not raise
            exception when the command exit value is non-zero.  Implies
            expect_error, but in contrast to expect_error will not assert
            that the exit value is zero.
        :param expect_error: if False (the default), asserts that the command
            exits with 0.  Otherwise, asserts that the command exits with a
            non-zero exit code.  Passing True also implies allow_stderr_error
            and allow_stderr_warning.
        :param expect_stderr: whether to allow warnings in stderr (equivalent
            to `allow_stderr_warning`).  This argument is an abbreviated
            version of `allow_stderr_warning` and is also kept for backwards
            compatibility.
        """
        if self.verbose:
            print(f">> running {args} {kw}")
    
        cwd = cwd or self.cwd
        if sys.platform == "win32":
            # Partial fix for ScriptTest.run using `shell=True` on Windows.
            args = tuple(str(a).replace("^", "^^").replace("&", "^&") for a in args)
    
        if allow_error:
            kw["expect_error"] = True
    
        # Propagate default values.
        expect_error = kw.get("expect_error")
        if expect_error:
            # Then default to allowing logged errors.
            if allow_stderr_error is not None and not allow_stderr_error:
                raise RuntimeError(
                    "cannot pass allow_stderr_error=False with expect_error=True"
                )
            allow_stderr_error = True
    
        elif kw.get("expect_stderr"):
            # Then default to allowing logged warnings.
            if allow_stderr_warning is not None and not allow_stderr_warning:
                raise RuntimeError(
                    "cannot pass allow_stderr_warning=False with expect_stderr=True"
                )
            allow_stderr_warning = True
    
        if allow_stderr_error:
            if allow_stderr_warning is not None and not allow_stderr_warning:
                raise RuntimeError(
                    "cannot pass allow_stderr_warning=False with "
                    "allow_stderr_error=True"
                )
    
        # Default values if not set.
        if allow_stderr_error is None:
            allow_stderr_error = False
        if allow_stderr_warning is None:
            allow_stderr_warning = allow_stderr_error
    
        # Pass expect_stderr=True to allow any stderr.  We do this because
        # we do our checking of stderr further on in check_stderr().
        kw["expect_stderr"] = True
>       result = super().run(cwd=cwd, *args, **kw)
E       AssertionError: Script returned code: 128

__class__  = <class 'tests.lib.PipTestEnvironment'>
allow_error = False
allow_stderr_error = False
allow_stderr_warning = False
args       = ('git', 'submodule', 'add', '/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg_submodule', 'testpkg/static')
cwd        = PosixPath('/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg')
expect_error = None
kw         = {'expect_stderr': True}
self       = <tests.lib.PipTestEnvironment object at 0x7ffaf1747250>

tests/lib/__init__.py:687: AssertionError
----------------------------- Captured stdout call -----------------------------
Script result: git submodule add /tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg_submodule testpkg/static
  return code: 128
-- stderr: --------------------
Cloning into '/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg/testpkg/static'...
fatal: transport 'file' not allowed
fatal: clone of '/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg_submodule' into submodule path '/tmp/pytest-of-runner/pytest-1/popen-gw1/test_check_submodule_addition0/workspace/scratch/version_pkg/testpkg/static' failed

Code of Conduct

• I agree to follow the PSF Code of Conduct.

[cboylan]

It turns out that https://github.com/pypa/pip/blob/main/src/pip/_internal/vcs/git.py#L490-L493 does run submodule commands that are effected in the package install path. I think that means that currently release pip is also broken against latest git and this isn't just a test suite problem. It also invalidates my assumption that the submodule is trusted because pip controls its content. This may be true in the test suite case but not generally when people install packages in git repos.

I'm not sure what the best way to address this is. One possibility is that this would now be an error unless the user sets some git config env var to override the defaults indicating they trust all the submodules involved. That would require updating pip's tests to address these changing expectations.