From 2625ed9fc074091c531c27ffcba7902771130261 Mon Sep 17 00:00:00 2001 From: Steve Kowalik Date: Tue, 20 Dec 2022 17:05:50 +1100 Subject: Forbid unsafe protocol URLs in Repo.clone{,_from}() Since the URL is passed directly to git clone, and the remote-ext helper will happily execute shell commands, so by default disallow URLs that contain a "::" unless a new unsafe_protocols kwarg is passed. (CVE-2022-24439) Fixes #1515 --- test/test_repo.py | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) (limited to 'test') diff --git a/test/test_repo.py b/test/test_repo.py index 6382db7e..53cae3cd 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -13,6 +13,7 @@ import pathlib import pickle import sys import tempfile +import uuid from unittest import mock, skipIf, SkipTest import pytest @@ -37,6 +38,7 @@ from git import ( ) from git.exc import ( BadObject, + UnsafeOptionsUsedError, ) from git.repo.fun import touch from test.lib import TestBase, with_rw_repo, fixture @@ -263,6 +265,40 @@ class TestRepo(TestBase): to_path=rw_dir, ) + def test_unsafe_options(self): + self.assertFalse(Repo.unsafe_options("github.com/deploy/deploy")) + + def test_unsafe_options_ext_url(self): + self.assertTrue(Repo.unsafe_options("ext::ssh")) + + def test_unsafe_options_multi_options_upload_pack(self): + self.assertTrue(Repo.unsafe_options("", ["--upload-pack='touch foo'"])) + + def test_unsafe_options_multi_options_config_user(self): + self.assertFalse(Repo.unsafe_options("", ["--config user"])) + + def test_unsafe_options_multi_options_config_protocol(self): + self.assertTrue(Repo.unsafe_options("", ["--config protocol.foo"])) + + def test_clone_from_forbids_helper_urls_by_default(self): + with self.assertRaises(UnsafeOptionsUsedError): + Repo.clone_from("ext::sh -c touch% /tmp/foo", "tmp") + + @with_rw_repo("HEAD") + def test_clone_from_allow_unsafe(self, repo): + bad_filename = pathlib.Path(f'{tempfile.gettempdir()}/{uuid.uuid4()}') + bad_url = f'ext::sh -c touch% {bad_filename}' + try: + repo.clone_from( + bad_url, 'tmp', + multi_options=["-c protocol.ext.allow=always"], + unsafe_protocols=True + ) + except GitCommandError: + pass + self.assertTrue(bad_filename.is_file()) + bad_filename.unlink() + @with_rw_repo("HEAD") def test_max_chunk_size(self, repo): class TestOutputStream(TestBase): -- cgit v1.2.1 From e6108c7997f5c8f7361b982959518e982b973230 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Fri, 23 Dec 2022 20:19:52 -0500 Subject: Block unsafe options and protocols by default --- test/test_repo.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'test') diff --git a/test/test_repo.py b/test/test_repo.py index 53cae3cd..a937836f 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -38,7 +38,8 @@ from git import ( ) from git.exc import ( BadObject, - UnsafeOptionsUsedError, + UnsafeOptionError, + UnsafeProtocolError, ) from git.repo.fun import touch from test.lib import TestBase, with_rw_repo, fixture @@ -281,7 +282,7 @@ class TestRepo(TestBase): self.assertTrue(Repo.unsafe_options("", ["--config protocol.foo"])) def test_clone_from_forbids_helper_urls_by_default(self): - with self.assertRaises(UnsafeOptionsUsedError): + with self.assertRaises(UnsafeOptionError): Repo.clone_from("ext::sh -c touch% /tmp/foo", "tmp") @with_rw_repo("HEAD") -- cgit v1.2.1 From fd2c6da5f82009398d241dc07603fbcd490ced29 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 27 Dec 2022 16:56:43 -0500 Subject: Updates from review --- test/test_git.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'test') diff --git a/test/test_git.py b/test/test_git.py index 6ba833b4..e7d236de 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -39,12 +39,12 @@ class TestGit(TestBase): self.assertEqual(git.call_args, ((["git", "version"],), {})) def test_call_unpack_args_unicode(self): - args = Git._Git__unpack_args("Unicode€™") + args = Git._unpack_args("Unicode€™") mangled_value = "Unicode\u20ac\u2122" self.assertEqual(args, [mangled_value]) def test_call_unpack_args(self): - args = Git._Git__unpack_args(["git", "log", "--", "Unicode€™"]) + args = Git._unpack_args(["git", "log", "--", "Unicode€™"]) mangled_value = "Unicode\u20ac\u2122" self.assertEqual(args, ["git", "log", "--", mangled_value]) -- cgit v1.2.1 From b92f01a3a38fc8e171d08575c69de9733811faa6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 27 Dec 2022 18:22:58 -0500 Subject: Update/add tests for Repo.clone* --- test/test_repo.py | 148 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 121 insertions(+), 27 deletions(-) (limited to 'test') diff --git a/test/test_repo.py b/test/test_repo.py index a937836f..72320184 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -13,7 +13,6 @@ import pathlib import pickle import sys import tempfile -import uuid from unittest import mock, skipIf, SkipTest import pytest @@ -226,6 +225,7 @@ class TestRepo(TestBase): "--config submodule.repo.update=checkout", "--config filter.lfs.clean='git-lfs clean -- %f'", ], + allow_unsafe_options=True, ) self.assertEqual(cloned.config_reader().get_value("submodule", "active"), "repo") @@ -266,39 +266,133 @@ class TestRepo(TestBase): to_path=rw_dir, ) - def test_unsafe_options(self): - self.assertFalse(Repo.unsafe_options("github.com/deploy/deploy")) + @with_rw_repo("HEAD") + def test_clone_unsafe_options(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + rw_repo.clone(tmp_dir, multi_options=[unsafe_option]) - def test_unsafe_options_ext_url(self): - self.assertTrue(Repo.unsafe_options("ext::ssh")) + @with_rw_repo("HEAD") + def test_clone_unsafe_options_allowed(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + ] + for i, unsafe_option in enumerate(unsafe_options): + destination = tmp_dir / str(i) + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + rw_repo.clone(destination, multi_options=[unsafe_option], allow_unsafe_options=True) + + unsafe_options = [ + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for i, unsafe_option in enumerate(unsafe_options): + destination = tmp_dir / str(i) + assert not destination.exists() + rw_repo.clone(destination, multi_options=[unsafe_option], allow_unsafe_options=True) + assert destination.exists() - def test_unsafe_options_multi_options_upload_pack(self): - self.assertTrue(Repo.unsafe_options("", ["--upload-pack='touch foo'"])) + @with_rw_repo("HEAD") + def test_clone_safe_options(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + options = [ + "--depth=1", + "--single-branch", + "-q", + ] + for option in options: + destination = tmp_dir / option + assert not destination.exists() + rw_repo.clone(destination, multi_options=[option]) + assert destination.exists() - def test_unsafe_options_multi_options_config_user(self): - self.assertFalse(Repo.unsafe_options("", ["--config user"])) + @with_rw_repo("HEAD") + def test_clone_from_unsafe_options(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + Repo.clone_from(rw_repo.working_dir, tmp_dir, multi_options=[unsafe_option]) - def test_unsafe_options_multi_options_config_protocol(self): - self.assertTrue(Repo.unsafe_options("", ["--config protocol.foo"])) + @with_rw_repo("HEAD") + def test_clone_from_unsafe_options_allowed(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + ] + for i, unsafe_option in enumerate(unsafe_options): + destination = tmp_dir / str(i) + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + Repo.clone_from( + rw_repo.working_dir, destination, multi_options=[unsafe_option], allow_unsafe_options=True + ) - def test_clone_from_forbids_helper_urls_by_default(self): - with self.assertRaises(UnsafeOptionError): - Repo.clone_from("ext::sh -c touch% /tmp/foo", "tmp") + unsafe_options = [ + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for i, unsafe_option in enumerate(unsafe_options): + destination = tmp_dir / str(i) + assert not destination.exists() + Repo.clone_from(rw_repo.working_dir, destination, multi_options=[unsafe_option], allow_unsafe_options=True) + assert destination.exists() @with_rw_repo("HEAD") - def test_clone_from_allow_unsafe(self, repo): - bad_filename = pathlib.Path(f'{tempfile.gettempdir()}/{uuid.uuid4()}') - bad_url = f'ext::sh -c touch% {bad_filename}' - try: - repo.clone_from( - bad_url, 'tmp', - multi_options=["-c protocol.ext.allow=always"], - unsafe_protocols=True - ) - except GitCommandError: - pass - self.assertTrue(bad_filename.is_file()) - bad_filename.unlink() + def test_clone_from_safe_options(self, rw_repo): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + options = [ + "--depth=1", + "--single-branch", + "-q", + ] + for option in options: + destination = tmp_dir / option + assert not destination.exists() + Repo.clone_from(rw_repo.common_dir, destination, multi_options=[option]) + assert destination.exists() + + def test_clone_from_unsafe_procol(self): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + Repo.clone_from(url, tmp_dir) + + def test_clone_from_unsafe_procol_allowed(self): + tmp_dir = pathlib.Path(tempfile.mkdtemp()) + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::/foo", + ] + for url in urls: + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + Repo.clone_from(url, tmp_dir, allow_unsafe_protocols=True) @with_rw_repo("HEAD") def test_max_chunk_size(self, repo): -- cgit v1.2.1 From c8ae33b9314a7d3716827b5cb705a3cd0a2e4a46 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 27 Dec 2022 19:15:40 -0500 Subject: More tests --- test/test_remote.py | 211 +++++++++++++++++++++++++++++++++++++++++++++++++ test/test_submodule.py | 3 +- 2 files changed, 213 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/test_remote.py b/test/test_remote.py index 7df64c20..9583724f 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -23,6 +23,8 @@ from git import ( GitCommandError, ) from git.cmd import Git +from pathlib import Path +from git.exc import UnsafeOptionError, UnsafeProtocolError from test.lib import ( TestBase, with_rw_repo, @@ -690,6 +692,215 @@ class TestRemote(TestBase): with self.assertRaisesRegex(GitCommandError, "src refspec __BAD_REF__ does not match any"): rem.push("__BAD_REF__") + @with_rw_repo("HEAD") + def test_set_unsafe_url(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + remote.set_url(url) + + @with_rw_repo("HEAD") + def test_set_unsafe_url_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + remote.set_url(url, allow_unsafe_protocols=True) + assert list(remote.urls)[-1] == url + + @with_rw_repo("HEAD") + def test_add_unsafe_url(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + remote.add_url(url) + + @with_rw_repo("HEAD") + def test_add_unsafe_url_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + remote.add_url(url, allow_unsafe_protocols=True) + assert list(remote.urls)[-1] == url + + @with_rw_repo("HEAD") + def test_create_remote_unsafe_url(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + Remote.create(rw_repo, "origin", url) + + @with_rw_repo("HEAD") + def test_create_remote_unsafe_url_allowed(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for i, url in enumerate(urls): + remote = Remote.create(rw_repo, f"origin{i}", url, allow_unsafe_protocols=True) + assert remote.url == url + + @with_rw_repo("HEAD") + def test_fetch_unsafe_url(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + remote.fetch(url) + + @with_rw_repo("HEAD") + def test_fetch_unsafe_url_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + remote.fetch(url, allow_unsafe_protocols=True) + + @with_rw_repo("HEAD") + def test_fetch_unsafe_options(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + remote.fetch(**unsafe_option) + + @with_rw_repo("HEAD") + def test_fetch_unsafe_options_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] + for unsafe_option in unsafe_options: + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + remote.fetch(**unsafe_option, allow_unsafe_options=True) + + @with_rw_repo("HEAD") + def test_pull_unsafe_url(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + remote.pull(url) + + @with_rw_repo("HEAD") + def test_pull_unsafe_url_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + remote.pull(url, allow_unsafe_protocols=True) + + @with_rw_repo("HEAD") + def test_pull_unsafe_options(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + remote.pull(**unsafe_option) + + @with_rw_repo("HEAD") + def test_pull_unsafe_options_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] + for unsafe_option in unsafe_options: + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + remote.pull(**unsafe_option, allow_unsafe_options=True) + + @with_rw_repo("HEAD") + def test_push_unsafe_url(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + remote.push(url) + + @with_rw_repo("HEAD") + def test_push_unsafe_url_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::17/foo", + ] + for url in urls: + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + remote.push(url, allow_unsafe_protocols=True) + + @with_rw_repo("HEAD") + def test_push_unsafe_options(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + { + "receive-pack": f"touch {tmp_file}", + "exec": f"touch {tmp_file}", + } + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + remote.push(**unsafe_option) + + @with_rw_repo("HEAD") + def test_push_unsafe_options_allowed(self, rw_repo): + remote = rw_repo.remote("origin") + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + { + "receive-pack": f"touch {tmp_file}", + "exec": f"touch {tmp_file}", + } + ] + for unsafe_option in unsafe_options: + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + remote.push(**unsafe_option, allow_unsafe_options=True) + class TestTimeouts(TestBase): @with_rw_repo("HEAD", bare=False) diff --git a/test/test_submodule.py b/test/test_submodule.py index fef6bda3..3ac29b9a 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1026,7 +1026,7 @@ class TestSubmodule(TestBase): ) # Act - sm.update(init=True, clone_multi_options=["--config core.eol=true"]) + sm.update(init=True, clone_multi_options=["--config core.eol=true"], allow_unsafe_options=True) # Assert sm_config = GitConfigParser(file_or_files=osp.join(parent.git_dir, "modules", sm_name, "config")) @@ -1070,6 +1070,7 @@ class TestSubmodule(TestBase): sm_name, url=self._small_repo_url(), clone_multi_options=["--config core.eol=true"], + allow_unsafe_options=True, ) # Assert -- cgit v1.2.1 From 9dc43926207b2205d77511c6ffd40944199f0c2d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 27 Dec 2022 20:07:18 -0500 Subject: Submodule tests --- test/test_submodule.py | 116 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 115 insertions(+), 1 deletion(-) (limited to 'test') diff --git a/test/test_submodule.py b/test/test_submodule.py index 3ac29b9a..5b162217 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -3,6 +3,8 @@ # the BSD License: http://www.opensource.org/licenses/bsd-license.php import os import shutil +import tempfile +from pathlib import Path import sys from unittest import skipIf @@ -12,7 +14,13 @@ import git from git.cmd import Git from git.compat import is_win from git.config import GitConfigParser, cp -from git.exc import InvalidGitRepositoryError, RepositoryDirtyError +from git.exc import ( + GitCommandError, + InvalidGitRepositoryError, + RepositoryDirtyError, + UnsafeOptionError, + UnsafeProtocolError, +) from git.objects.submodule.base import Submodule from git.objects.submodule.root import RootModule, RootUpdateProgress from git.repo.fun import find_submodule_git_dir, touch @@ -1090,3 +1098,109 @@ class TestSubmodule(TestBase): sm_config = GitConfigParser(file_or_files=osp.join(parent.git_dir, "modules", sm_name, "config")) with self.assertRaises(cp.NoOptionError): sm_config.get_value("core", "eol") + + @with_rw_repo("HEAD") + def test_submodule_add_unsafe_url(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::/foo", + ] + for url in urls: + with self.assertRaises(UnsafeProtocolError): + Submodule.add(rw_repo, "new", "new", url) + + @with_rw_repo("HEAD") + def test_submodule_add_unsafe_url_allowed(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::/foo", + ] + for url in urls: + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + Submodule.add(rw_repo, "new", "new", url, allow_unsafe_protocols=True) + + @with_rw_repo("HEAD") + def test_submodule_add_unsafe_options(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + Submodule.add(rw_repo, "new", "new", str(tmp_dir), clone_multi_options=[unsafe_option]) + + @with_rw_repo("HEAD") + def test_submodule_add_unsafe_options_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + for unsafe_option in unsafe_options: + with self.assertRaises(GitCommandError): + Submodule.add( + rw_repo, "new", "new", str(tmp_dir), clone_multi_options=[unsafe_option], allow_unsafe_options=True + ) + + @with_rw_repo("HEAD") + def test_submodule_update_unsafe_url(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::/foo", + ] + for url in urls: + submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=url) + with self.assertRaises(UnsafeProtocolError): + submodule.update() + + @with_rw_repo("HEAD") + def test_submodule_update_unsafe_url_allowed(self, rw_repo): + urls = [ + "ext::sh -c touch% /tmp/pwn", + "fd::/foo", + ] + for url in urls: + submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=url) + # The URL will be allowed into the command, but the command will + # fail since we don't have that protocol enabled in the Git config file. + with self.assertRaises(GitCommandError): + submodule.update(allow_unsafe_protocols=True) + + @with_rw_repo("HEAD") + def test_submodule_update_unsafe_options(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=str(tmp_dir)) + for unsafe_option in unsafe_options: + with self.assertRaises(UnsafeOptionError): + submodule.update(clone_multi_options=[unsafe_option]) + + @with_rw_repo("HEAD") + def test_submodule_update_unsafe_options_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" + unsafe_options = [ + f"--upload-pack='touch {tmp_file}'", + f"-u 'touch {tmp_file}'", + "--config=protocol.ext.allow=always", + "-c protocol.ext.allow=always", + ] + submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=str(tmp_dir)) + for unsafe_option in unsafe_options: + with self.assertRaises(GitCommandError): + submodule.update(clone_multi_options=[unsafe_option], allow_unsafe_options=True) -- cgit v1.2.1 From f4f2658d5d308b3fb9162e50cd4c7b346e7a0a47 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 28 Dec 2022 21:48:29 -0500 Subject: Updates from review --- test/test_remote.py | 71 +++++++++++++++++++++++++++++++++++++++++--------- test/test_repo.py | 14 +++++++++- test/test_submodule.py | 41 ++++++++++++++++++++++++++--- 3 files changed, 109 insertions(+), 17 deletions(-) (limited to 'test') diff --git a/test/test_remote.py b/test/test_remote.py index 9583724f..3a47afab 100644 --- a/test/test_remote.py +++ b/test/test_remote.py @@ -694,84 +694,107 @@ class TestRemote(TestBase): @with_rw_repo("HEAD") def test_set_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): remote.set_url(url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_set_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: remote.set_url(url, allow_unsafe_protocols=True) assert list(remote.urls)[-1] == url + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_add_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): remote.add_url(url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_add_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: remote.add_url(url, allow_unsafe_protocols=True) assert list(remote.urls)[-1] == url + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_create_remote_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): Remote.create(rw_repo, "origin", url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_create_remote_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for i, url in enumerate(urls): remote = Remote.create(rw_repo, f"origin{i}", url, allow_unsafe_protocols=True) assert remote.url == url + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_fetch_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): remote.fetch(url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_fetch_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: @@ -779,6 +802,7 @@ class TestRemote(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): remote.fetch(url, allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_fetch_unsafe_options(self, rw_repo): @@ -789,6 +813,7 @@ class TestRemote(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): remote.fetch(**unsafe_option) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_fetch_unsafe_options_allowed(self, rw_repo): @@ -798,25 +823,32 @@ class TestRemote(TestBase): unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] for unsafe_option in unsafe_options: # The options will be allowed, but the command will fail. + assert not tmp_file.exists() with self.assertRaises(GitCommandError): remote.fetch(**unsafe_option, allow_unsafe_options=True) + assert tmp_file.exists() @with_rw_repo("HEAD") def test_pull_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): remote.pull(url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_pull_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: @@ -824,6 +856,7 @@ class TestRemote(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): remote.pull(url, allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_pull_unsafe_options(self, rw_repo): @@ -834,6 +867,7 @@ class TestRemote(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): remote.pull(**unsafe_option) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_pull_unsafe_options_allowed(self, rw_repo): @@ -843,25 +877,32 @@ class TestRemote(TestBase): unsafe_options = [{"upload-pack": f"touch {tmp_file}"}] for unsafe_option in unsafe_options: # The options will be allowed, but the command will fail. + assert not tmp_file.exists() with self.assertRaises(GitCommandError): remote.pull(**unsafe_option, allow_unsafe_options=True) + assert tmp_file.exists() @with_rw_repo("HEAD") def test_push_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): remote.push(url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_push_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" remote = rw_repo.remote("origin") urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: @@ -869,6 +910,7 @@ class TestRemote(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): remote.push(url, allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_push_unsafe_options(self, rw_repo): @@ -882,8 +924,10 @@ class TestRemote(TestBase): } ] for unsafe_option in unsafe_options: + assert not tmp_file.exists() with self.assertRaises(UnsafeOptionError): remote.push(**unsafe_option) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_push_unsafe_options_allowed(self, rw_repo): @@ -898,8 +942,11 @@ class TestRemote(TestBase): ] for unsafe_option in unsafe_options: # The options will be allowed, but the command will fail. + assert not tmp_file.exists() with self.assertRaises(GitCommandError): remote.push(**unsafe_option, allow_unsafe_options=True) + assert tmp_file.exists() + tmp_file.unlink() class TestTimeouts(TestBase): diff --git a/test/test_repo.py b/test/test_repo.py index 72320184..5874dbe6 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -279,6 +279,7 @@ class TestRepo(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): rw_repo.clone(tmp_dir, multi_options=[unsafe_option]) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_clone_unsafe_options_allowed(self, rw_repo): @@ -290,9 +291,12 @@ class TestRepo(TestBase): ] for i, unsafe_option in enumerate(unsafe_options): destination = tmp_dir / str(i) + assert not tmp_file.exists() # The options will be allowed, but the command will fail. with self.assertRaises(GitCommandError): rw_repo.clone(destination, multi_options=[unsafe_option], allow_unsafe_options=True) + assert tmp_file.exists() + tmp_file.unlink() unsafe_options = [ "--config=protocol.ext.allow=always", @@ -331,6 +335,7 @@ class TestRepo(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): Repo.clone_from(rw_repo.working_dir, tmp_dir, multi_options=[unsafe_option]) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_clone_from_unsafe_options_allowed(self, rw_repo): @@ -342,11 +347,14 @@ class TestRepo(TestBase): ] for i, unsafe_option in enumerate(unsafe_options): destination = tmp_dir / str(i) + assert not tmp_file.exists() # The options will be allowed, but the command will fail. with self.assertRaises(GitCommandError): Repo.clone_from( rw_repo.working_dir, destination, multi_options=[unsafe_option], allow_unsafe_options=True ) + assert tmp_file.exists() + tmp_file.unlink() unsafe_options = [ "--config=protocol.ext.allow=always", @@ -374,16 +382,19 @@ class TestRepo(TestBase): def test_clone_from_unsafe_procol(self): tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::17/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): Repo.clone_from(url, tmp_dir) + assert not tmp_file.exists() def test_clone_from_unsafe_procol_allowed(self): tmp_dir = pathlib.Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ "ext::sh -c touch% /tmp/pwn", "fd::/foo", @@ -393,6 +404,7 @@ class TestRepo(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): Repo.clone_from(url, tmp_dir, allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_max_chunk_size(self, repo): diff --git a/test/test_submodule.py b/test/test_submodule.py index 5b162217..13878df2 100644 --- a/test/test_submodule.py +++ b/test/test_submodule.py @@ -1101,18 +1101,23 @@ class TestSubmodule(TestBase): @with_rw_repo("HEAD") def test_submodule_add_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::/foo", ] for url in urls: with self.assertRaises(UnsafeProtocolError): Submodule.add(rw_repo, "new", "new", url) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_add_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::/foo", ] for url in urls: @@ -1120,6 +1125,7 @@ class TestSubmodule(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): Submodule.add(rw_repo, "new", "new", url, allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_add_unsafe_options(self, rw_repo): @@ -1134,6 +1140,7 @@ class TestSubmodule(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): Submodule.add(rw_repo, "new", "new", str(tmp_dir), clone_multi_options=[unsafe_option]) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_add_unsafe_options_allowed(self, rw_repo): @@ -1142,6 +1149,16 @@ class TestSubmodule(TestBase): unsafe_options = [ f"--upload-pack='touch {tmp_file}'", f"-u 'touch {tmp_file}'", + ] + for unsafe_option in unsafe_options: + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + Submodule.add( + rw_repo, "new", "new", str(tmp_dir), clone_multi_options=[unsafe_option], allow_unsafe_options=True + ) + assert not tmp_file.exists() + + unsafe_options = [ "--config=protocol.ext.allow=always", "-c protocol.ext.allow=always", ] @@ -1153,19 +1170,24 @@ class TestSubmodule(TestBase): @with_rw_repo("HEAD") def test_submodule_update_unsafe_url(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::/foo", ] for url in urls: submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=url) with self.assertRaises(UnsafeProtocolError): submodule.update() + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_update_unsafe_url_allowed(self, rw_repo): + tmp_dir = Path(tempfile.mkdtemp()) + tmp_file = tmp_dir / "pwn" urls = [ - "ext::sh -c touch% /tmp/pwn", + f"ext::sh -c touch% {tmp_file}", "fd::/foo", ] for url in urls: @@ -1174,6 +1196,7 @@ class TestSubmodule(TestBase): # fail since we don't have that protocol enabled in the Git config file. with self.assertRaises(GitCommandError): submodule.update(allow_unsafe_protocols=True) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_update_unsafe_options(self, rw_repo): @@ -1189,6 +1212,7 @@ class TestSubmodule(TestBase): for unsafe_option in unsafe_options: with self.assertRaises(UnsafeOptionError): submodule.update(clone_multi_options=[unsafe_option]) + assert not tmp_file.exists() @with_rw_repo("HEAD") def test_submodule_update_unsafe_options_allowed(self, rw_repo): @@ -1197,6 +1221,15 @@ class TestSubmodule(TestBase): unsafe_options = [ f"--upload-pack='touch {tmp_file}'", f"-u 'touch {tmp_file}'", + ] + submodule = Submodule(rw_repo, b"\0" * 20, name="new", path="new", url=str(tmp_dir)) + for unsafe_option in unsafe_options: + # The options will be allowed, but the command will fail. + with self.assertRaises(GitCommandError): + submodule.update(clone_multi_options=[unsafe_option], allow_unsafe_options=True) + assert not tmp_file.exists() + + unsafe_options = [ "--config=protocol.ext.allow=always", "-c protocol.ext.allow=always", ] -- cgit v1.2.1