I would like to write tests, I looked around through the test suite, but it still escapes me how to structure these tests. Since these options affect howgit is executed, it seems like it would have to connect to the network? Or is there a test that already replaces the protocol helper, e.g.git-remote-https, to avoid network access?

It really isn't easy to test it at all, and probably impossible to test it exhaustively. So I am fine admitting defeat on this one, particularly because the tests I could imagine would be so specificand spotty that they barely have any value.

if we can only rungit, that could mean we pick#2027 over#2026. I don't think it makes sense to have an allowlist of other executables GitPython could run.@EliahKagan, your thoughts?

if we can only rungit, that could mean we pick#2027 over#2026. I don't think it makes sense to have an allowlist of other executables GitPython could run.@EliahKagan, your thoughts?

I haven't reviewed this draft PR in detail, and I also don't know what it will be like and how it will document the "safe mode" feature once it's done. But my first impression is that the changes being proposed here should not directly impose any requirements on whetheris_cygwin_git runs an external subprocess besidesgit:

• Although the PR title makes it seem like this prevents all external subprocesses exceptgit from being executed, it is otherwise described more specifically to preventgit subprocesses from executing other external subprocesses.
• If this covers all subprocesses GitPython creates, rather than only those throughgit, then there are already more subprocesses that need to be suppressed:at leastps.
• If these changes are to be "minimally invasive" as characterized in#2029 (review), then I think they shouldn't impose requirements on code that doesn't operate on any repository or use any repository or URL specific paths or state.

However, that may not be the whole story. Although I don't think "safe mode" should prohibit the use of subprocesses inis_cygwin_git, it may be that they should be avoided in general when not needed for the same reasons. As currently written, it takes some effort to verify thatis_cygwin_git does not introduce an untrusted search path vulnerability.

Furthermore, regarding the use ofuname inis_cygwin_git, while I think it's probably not a vulnerability, it's not really ideal to assume adjacent executables are similarly trusted. It might be considered a valuable security enhancement to avoid assuming that just because an executable has a standard name and resides in the same directory asgit that it is safe or reasonable to execute it.

FYI this is my very simple test suite:

#!/usr/bin/python3importosimportgitimporttempfileforurlin ['git@gitlab.com:fdroid/ci-test-app.git','ssh://gitlab.com/fdroid/ci-test-app.git','git://gitlab.com/fdroid/ci-test-app.git',]:print('====================',url)d=tempfile.mkdtemp(prefix='foo_py_')repo=git.Repo.clone_from(url,d,safe=True)# clone from fake URL should prompt for passwordd=tempfile.mkdtemp(prefix='foo_py_')forurlin ['https://github.com/asdfasdfasdf/adsfasdfasdf.git','https://gitlab.com/asdfasdfasdf/adsfasdfasdf.git','https://codeberg.org/asdfasdfasdf/adsfasdfasdf.git',]:try:repo=git.Repo.clone_from(url,d,safe=True)exceptgit.exc.GitCommandErrorase:print('repo',e)url='https://gitlab.com/fdroid/ci-test-app.git'd=tempfile.mkdtemp(prefix='foo_py_')print(d)repo=git.Repo.init(d,safe=True)origin=repo.create_remote("origin",url)origin.fetch()d=tempfile.mkdtemp(prefix='foo_py_')print(d)repo=git.Repo.clone_from(url,d,safe=True)print(type(repo))print('SUCCESS')

I think I should leave it to@EliahKagan to review this PR, due to its nature of being very relevant to the security of the project.

To me it still is the question if there is a good case for supporting this - I'd think the answer should be "yes" if f-droid benefits, but I fear that it lures people into a false sense of safety. Maybesafe=true should be changed to something more specific at the very least, because it's definitely notsafe by all means.

F-Droid has been using this approach for years, and will switch to this code once it is merged. As for the name, I'm open to suggestions. I used the word "safe" following the example of parser libs I've seen, where it means features are disabled in the interest of security. Like ruamel.yaml uses "safe" mode to mean a YAML parser that does not parse anything but lists, dicts, and scalars (e.g. no objects). From what see, I think this clearly does make it safer to use.