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')