-
-
Notifications
You must be signed in to change notification settings - Fork 954
Override bash executable, defaulting to Git for Windows git bash over WSL bash #1791
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
77f74ce
4bde10d
6676668
50c74f4
bf919f6
f065d1f
0c14cac
8200ad1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1064,19 +1064,19 @@ def test_hook_uses_shell_not_from_cwd(self, rw_dir, case): | |
shutil.copy(fixture_path("polyglot"), hook_path("polyglot", repo.git_dir)) | ||
payload = Path(rw_dir, "payload.txt") | ||
|
||
if type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.WslNoDistro}: | ||
# The real shell can't run, but the impostor should still not be used. | ||
with self.assertRaises(HookExecutionError): | ||
with maybe_chdir: | ||
run_commit_hook("polyglot", repo.index) | ||
self.assertFalse(payload.exists()) | ||
else: | ||
# The real shell should run, and not the impostor. | ||
with maybe_chdir: | ||
run_commit_hook("polyglot", repo.index) | ||
self.assertFalse(payload.exists()) | ||
output = Path(rw_dir, "output.txt").read_text(encoding="utf-8") | ||
self.assertEqual(output, "Ran intended hook.\n") | ||
# if type(_win_bash_status) in {WinBashStatus.Absent, WinBashStatus.WslNoDistro}: | ||
# # The real shell can't run, but the impostor should still not be used. | ||
# with self.assertRaises(HookExecutionError): | ||
# with maybe_chdir: | ||
# run_commit_hook("polyglot", repo.index) | ||
# self.assertFalse(payload.exists()) | ||
# else: | ||
Comment on lines
+1067
to
+1073
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure what the best thing is to do here right now. Some of the modifications to the test suite to take advantage of the improvements in this pull request could be done later. The purpose of this test method is to verify that an untrusted repository (or, more realistically, an untrusted branch, possibly from a fork) with a malicious It is a shortcoming of #1792, which fixed CVE-2024-22190 and added this test, that both scenarios would not be tested in a single run of the test suite. This would have been tricky to do prior to the changes in this pull request, but I probably could and maybe should have done it. Instead, this tested whichever of the two scenarios applied to the test system. Both scenarios were common prior to the changes in this pull request, which makes the broken-or-unavailable Possible approaches:
I think any of those approaches, and probably various others I have not thought of, are reasonable. |
||
# The real shell should run, and not the impostor. | ||
with maybe_chdir: | ||
run_commit_hook("polyglot", repo.index) | ||
self.assertFalse(payload.exists()) | ||
output = Path(rw_dir, "output.txt").read_text(encoding="utf-8") | ||
self.assertEqual(output, "Ran intended hook.\n") | ||
|
||
# @pytest.mark.xfail( | ||
# type(_win_bash_status) is WinBashStatus.Absent, | ||
|