Only make config more permissive in tests that need it

[EliahKagan]

Closes #1544

Edit: When making this PR, I didn't notice #1647, which was recently opened (but opened before this PR; #1647 came first). That PR includes an alternative to the approach taken here. It uses a GitPython feature to set protocol.file.allow in git command-like arguments, while this patches GIT_CONFIG_* environment variables. Both approaches are specific and, I believe, robust. The approach in this PR avoids using more GitPython features that are not conceptually under test in the submodule tests. But the approach in #1647 is significantly more compact, which might be considered the more important benefit. I will not mind if this is closed in preference for #1647! At this time, both #1647 and this PR also contain changes beyond those that directly address #1544, and another option, if the approach in #1647 is chosen, would be for me to narrow this PR to make only CI changes, after #1647 is accepted.

(The original description of the changes in this PR, and the rationale for them, follows.)

protocol.file.allow

This eliminates the need for users running the test suite to set protocol.file.allow to always in the global git configuration. Setting it globally to always has security implications, as alluded to in #1544, and as noted in the description in git release notes of how CVE-2022-39253 was fixed, and in git/git@a1d4f67 where its default value was changed from always to user.

On CI, this was not directly a problem, because the CI runner is isolated and not being used to clone unrelated less-trusted repositories. But users are likely to be looking at the CI workflows to figure out how to overcome the fatal: transport 'file' not allowed error locally. Furthermore, by having the test suite make the change automatically and temporarily by modifying process environment variables, the needed setup is simplified for everyone.

The approach taken here is inspired by #1544 (comment) and makes use of GIT_CONFIG_* environment variables. But it is more specific than suggested there, instead temporarily patching the environment only during the runs of the two specific tests that require it, test_list_only_valid_submodules and test_git_submodules_and_add_sm_with_new_commit. This is done without much code duplication, so it can be applied easily to any future test cases that require it. (I think this probably won't ever be needed outside test_submodule.py, because Git's default value of protocol.file.allow is user, not never.)

I patched GIT_CONFIG_* variables in such a way that existing assignments to GIT_CONFIG_* variables, if present, are still used, rather than being replaced or causing an error. I considered patching GIT_ALLOW_PROTOCOL instead, but I decided against it because it may be useful for people running the tests to be able to change what other protocols are allowed/disallowed. Patching GIT_ALLOW_PROTOCOL in a way that respected that would be more complicated than patching GIT_CONFIG_* variables.

safe.directory

protocol.file.allow is one of two security-related configuration options that were set on CI. The other is safe.directory. This is not needed in the pythonpackage.yml workflow, because the cloned repository's files are always owned by the same user that is running pytest and thus git, so I removed it from there. The cygwin-test.yml workflow does currently need it, and I shell-quoted $(pwd) there, which is slightly more robust and better expresses the intent that no splitting or globbing be performed, but otherwise retained it.

I was unsure if I should include changes related to safe.directory in this PR, or open a separate PR. The protocol.file.allow and safe.directory customizations were presented as closely related in the workflows. More importantly, to decide where to put the fixture/helper used for patching protocol.file.allow in test_submodule.py, I checked that it would not be needed elsewhere, by verifying that no test cases inherently require safe.directory to be set (but that it just works around a Cygwin-specific issue). For the same reason, it seems to me that the changes may be easier to review together than separately, as well. However, I would be pleased to make any requested changes to this PR, including splitting out safe.directory-related changes to a separate PR if desired.

[Byron]

This looks great to me, thanks a lot for the initiative and contribution!

Since this PR is complete for what it is supposed to achieve, let's merge it and deal with safe.directory in a separate PR. Thank you.

[EliahKagan]

@Byron No problem!

This PR actually had already dealt with safe.directory, in 537af83. It didn't attempt to harmonize the ownership situation in cygwin-test.yml, but it did make the changes described in the safe.directory portion of the PR description. I hope that's okay! If you'd like those changes undone or done differently, I could open a new PR for that.

[Byron]

I hope that's okay!

I think it's OK as long you think the same :).