From 537af83c344420994a6a34dd18623f132398d062 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 6 Sep 2023 17:44:25 -0400 Subject: [PATCH 1/4] Only set safe.directory on Cygwin (which needs it) This stops setting the current directory as an explicit safe directory on CI for non-Windows systems, where this is not needed because the repository has the ownership Git expects. The step name is updated accordingly to reflect its now narrower purpose. This also adds shell quoting to $(pwd) in the Cygwin workflow. In practice, on CI, the path is very unlikely to contain whitespace, but double-quoting $ expansions on which splitting and globbing are unwanted is more robust and better expresses intent. This also has the benefit that users who use the CI workflows as a guide to commands they run locally, where on Windows they may very well have spaces somewhere in this absolute path, will use a correct command. --- .github/workflows/cygwin-test.yml | 6 +++--- .github/workflows/pythonpackage.yml | 5 ++--- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 6ba63f019..618bec405 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -12,7 +12,7 @@ jobs: SHELLOPTS: igncr TMP: "/tmp" TEMP: "/tmp" - + steps: - name: Force LF line endings run: git config --global core.autocrlf input @@ -24,8 +24,8 @@ jobs: packages: python39 python39-pip python39-virtualenv git - name: Tell git to trust this repo shell: bash.exe -eo pipefail -o igncr "{0}" - run: | - /usr/bin/git config --global --add safe.directory $(pwd) + run: | + /usr/bin/git config --global --add safe.directory "$(pwd)" /usr/bin/git config --global protocol.file.allow always - name: Install dependencies and prepare tests shell: bash.exe -eo pipefail -o igncr "{0}" diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 68988f2a7..bc50cab29 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -52,9 +52,8 @@ jobs: set -x mypy -p git - - name: Tell git to trust this repo - run: | - /usr/bin/git config --global --add safe.directory $(pwd) + - name: Tell git to allow file protocol even for submodules + run: | /usr/bin/git config --global protocol.file.allow always - name: Test with pytest From 92d9ae22132c97f764f0108da30062c24392cc2b Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 6 Sep 2023 18:02:20 -0400 Subject: [PATCH 2/4] Use env vars on CI to set protocol.file.allow Instead of setting a global git configuration. This makes no significant difference for security on CI, but it is an iterative step toward a more specific way of setting them that will apply on CI and locally and require less configuration. In addition, this shows an approach more similar to what users who do not want to carefully review the security impact of changing the global setting can use locally (and which is more secure). --- .github/workflows/cygwin-test.yml | 5 ++++- .github/workflows/pythonpackage.yml | 8 ++++---- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index 618bec405..b612ca94a 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -26,7 +26,6 @@ jobs: shell: bash.exe -eo pipefail -o igncr "{0}" run: | /usr/bin/git config --global --add safe.directory "$(pwd)" - /usr/bin/git config --global protocol.file.allow always - name: Install dependencies and prepare tests shell: bash.exe -eo pipefail -o igncr "{0}" run: | @@ -47,4 +46,8 @@ jobs: shell: bash.exe -eo pipefail -o igncr "{0}" run: | /usr/bin/python -m pytest + env: + GIT_CONFIG_COUNT: "1" + GIT_CONFIG_KEY_0: protocol.file.allow + GIT_CONFIG_VALUE_0: always continue-on-error: false diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index bc50cab29..207714810 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -52,14 +52,14 @@ jobs: set -x mypy -p git - - name: Tell git to allow file protocol even for submodules - run: | - /usr/bin/git config --global protocol.file.allow always - - name: Test with pytest run: | set -x pytest + env: + GIT_CONFIG_COUNT: "1" + GIT_CONFIG_KEY_0: protocol.file.allow + GIT_CONFIG_VALUE_0: always continue-on-error: false - name: Documentation From 4f594cd2cbf68caabb7d3f22397104f5aa1b49b7 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 6 Sep 2023 18:59:12 -0400 Subject: [PATCH 3/4] Set protocol.file.allow only in tests that need it Instead of setting environment variables just on CI and for the the entire pytest command, this has the two test cases that need protocol.file.allow to be set to "always" (instead of "user") set them, via a shared fixture, just while those tests are running. Both on CI and for local test runs, this makes it no longer necessary to set this in a global configuration or through environment variables, reducing the setup needed to run the tests. --- .github/workflows/cygwin-test.yml | 4 ---- .github/workflows/pythonpackage.yml | 4 ---- test/test_submodule.py | 22 +++++++++++++++++++++- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/.github/workflows/cygwin-test.yml b/.github/workflows/cygwin-test.yml index b612ca94a..808dc5608 100644 --- a/.github/workflows/cygwin-test.yml +++ b/.github/workflows/cygwin-test.yml @@ -46,8 +46,4 @@ jobs: shell: bash.exe -eo pipefail -o igncr "{0}" run: | /usr/bin/python -m pytest - env: - GIT_CONFIG_COUNT: "1" - GIT_CONFIG_KEY_0: protocol.file.allow - GIT_CONFIG_VALUE_0: always continue-on-error: false diff --git a/.github/workflows/pythonpackage.yml b/.github/workflows/pythonpackage.yml index 207714810..a6af507d1 100644 --- a/.github/workflows/pythonpackage.yml +++ b/.github/workflows/pythonpackage.yml @@ -56,10 +56,6 @@ jobs: run: | set -x pytest - env: - GIT_CONFIG_COUNT: "1" - GIT_CONFIG_KEY_0: protocol.file.allow - GIT_CONFIG_VALUE_0: always continue-on-error: false - name: Documentation diff --git a/test/test_submodule.py b/test/test_submodule.py index 982226411..a039faaf3 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1,12 +1,13 @@ # -*- coding: utf-8 -*- # This module is part of GitPython and is released under # the BSD License: http://www.opensource.org/licenses/bsd-license.php +import contextlib import os import shutil import tempfile from pathlib import Path import sys -from unittest import skipIf +from unittest import mock, skipIf import pytest @@ -31,6 +32,23 @@ import os.path as osp +@contextlib.contextmanager +def _allow_file_protocol(): + """Temporarily set protocol.file.allow to always, using environment variables.""" + pair_index = int(os.getenv("GIT_CONFIG_COUNT", "0")) + + # This is recomputed each time the context is entered, for compatibility with + # existing GIT_CONFIG_* environment variables, even if changed in this process. + patcher = mock.patch.dict(os.environ, { + "GIT_CONFIG_COUNT": str(pair_index + 1), + f"GIT_CONFIG_KEY_{pair_index}": "protocol.file.allow", + f"GIT_CONFIG_VALUE_{pair_index}": "always", + }) + + with patcher: + yield + + class TestRootProgress(RootUpdateProgress): """Just prints messages, for now without checking the correctness of the states""" @@ -709,6 +727,7 @@ def test_add_empty_repo(self, rwdir): # end for each checkout mode @with_rw_directory + @_allow_file_protocol() def test_list_only_valid_submodules(self, rwdir): repo_path = osp.join(rwdir, "parent") repo = git.Repo.init(repo_path) @@ -737,6 +756,7 @@ def test_list_only_valid_submodules(self, rwdir): """, ) @with_rw_directory + @_allow_file_protocol() def test_git_submodules_and_add_sm_with_new_commit(self, rwdir): parent = git.Repo.init(osp.join(rwdir, "parent")) parent.git.submodule("add", self._small_repo_url(), "module") From f6c326288a04d17907081b065c436332e60115de Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 6 Sep 2023 23:24:34 +0000 Subject: [PATCH 4/4] Redesign new decorator to better separate concerns _allow_file_protocol was effectively a _patch_git_config fixture, being no no shorter, simpler, or clearer by hard-coding the specific name and value to patch. So this changes it to be that. As a secondary issue, it previously was called with no arguments, then that would be used as a decorator. That was unintutive and it was easy to omit the parentheses accidentally. This resolves that. --- test/test_submodule.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/test_submodule.py b/test/test_submodule.py index a039faaf3..d906a5d5b 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -33,16 +33,16 @@ @contextlib.contextmanager -def _allow_file_protocol(): - """Temporarily set protocol.file.allow to always, using environment variables.""" +def _patch_git_config(name, value): + """Temporarily add a git config name-value pair, using environment variables.""" pair_index = int(os.getenv("GIT_CONFIG_COUNT", "0")) # This is recomputed each time the context is entered, for compatibility with # existing GIT_CONFIG_* environment variables, even if changed in this process. patcher = mock.patch.dict(os.environ, { "GIT_CONFIG_COUNT": str(pair_index + 1), - f"GIT_CONFIG_KEY_{pair_index}": "protocol.file.allow", - f"GIT_CONFIG_VALUE_{pair_index}": "always", + f"GIT_CONFIG_KEY_{pair_index}": name, + f"GIT_CONFIG_VALUE_{pair_index}": value, }) with patcher: @@ -727,7 +727,7 @@ def test_add_empty_repo(self, rwdir): # end for each checkout mode @with_rw_directory - @_allow_file_protocol() + @_patch_git_config("protocol.file.allow", "always") def test_list_only_valid_submodules(self, rwdir): repo_path = osp.join(rwdir, "parent") repo = git.Repo.init(repo_path) @@ -756,7 +756,7 @@ def test_list_only_valid_submodules(self, rwdir): """, ) @with_rw_directory - @_allow_file_protocol() + @_patch_git_config("protocol.file.allow", "always") def test_git_submodules_and_add_sm_with_new_commit(self, rwdir): parent = git.Repo.init(osp.join(rwdir, "parent")) parent.git.submodule("add", self._small_repo_url(), "module")