-
Notifications
You must be signed in to change notification settings - Fork 1
🔒 fix potential argument injection in git commands #56
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,72 @@ | ||||||||||
| // SPDX-FileCopyrightText: 2025 Adam Poulemanos <89049923+bashandbone@users.noreply.github.com> | ||||||||||
| // | ||||||||||
| // SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT | ||||||||||
|
|
||||||||||
| //! Security tests to ensure robustness against various attack vectors. | ||||||||||
|
|
||||||||||
| mod common; | ||||||||||
| use common::TestHarness; | ||||||||||
|
|
||||||||||
| #[cfg(test)] | ||||||||||
| mod tests { | ||||||||||
| use super::*; | ||||||||||
|
|
||||||||||
| #[test] | ||||||||||
| fn test_path_with_hyphen_injection() { | ||||||||||
| let harness = TestHarness::new().expect("Failed to create test harness"); | ||||||||||
| harness.init_git_repo().expect("Failed to init git repo"); | ||||||||||
|
|
||||||||||
| let remote_repo = harness | ||||||||||
| .create_test_remote("hyphen-remote") | ||||||||||
| .expect("Failed to create remote"); | ||||||||||
| let remote_url = format!("file://{}", remote_repo.display()); | ||||||||||
|
|
||||||||||
| // A path starting with a hyphen that could be a git flag. | ||||||||||
| // This should be handled as a literal path component, not interpreted | ||||||||||
| // as an option to git or to any cleanup command that operates on paths. | ||||||||||
| let malicious_path = "-c"; | ||||||||||
|
|
||||||||||
| harness.run_submod_success(&[ | ||||||||||
| "add", | ||||||||||
| &remote_url, | ||||||||||
| "--name", | ||||||||||
| "hyphen-sub", | ||||||||||
| "--path", | ||||||||||
| malicious_path, | ||||||||||
| ]).expect("Failed to add submodule with hyphenated path"); | ||||||||||
|
|
||||||||||
| // Verify the submodule was actually created at the requested path. | ||||||||||
| assert!(harness.dir_exists("-c")); | ||||||||||
| } | ||||||||||
|
|
||||||||||
| #[test] | ||||||||||
| fn test_sparse_checkout_with_hyphen_path() { | ||||||||||
| let harness = TestHarness::new().expect("Failed to create test harness"); | ||||||||||
| harness.init_git_repo().expect("Failed to init git repo"); | ||||||||||
|
|
||||||||||
| let remote_repo = harness | ||||||||||
| .create_complex_remote("sparse-hyphen") | ||||||||||
| .expect("Failed to create remote"); | ||||||||||
| let remote_url = format!("file://{}", remote_repo.display()); | ||||||||||
|
|
||||||||||
| // Path starting with hyphen | ||||||||||
| let path = "-sparse"; | ||||||||||
|
|
||||||||||
| // Ensure the directory exists to trigger the CLI fallback in apply_sparse_checkout if needed, | ||||||||||
| // although apply_sparse_checkout is usually called after gix/git2 which might fail or be bypassed. | ||||||||||
|
|
||||||||||
|
Comment on lines
+55
to
+57
|
||||||||||
| // Ensure the directory exists to trigger the CLI fallback in apply_sparse_checkout if needed, | |
| // although apply_sparse_checkout is usually called after gix/git2 which might fail or be bypassed. | |
| // Add a sparse submodule at a hyphen-prefixed path and verify the path is | |
| // treated literally rather than being interpreted as a command-line option. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change hardens the
git rmcleanup against hyphen-prefixed pathspecs, but the newly added integration tests don’t appear to exercise this cleanup path (it only runs when gix+git2 add_submodule fails and the code falls back to the CLI). Consider adding a targeted test that forces the fallback cleanup to run with a path starting with-/--...and asserts the overall operation succeeds (or at least that the cleanup doesn’t invoke unintendedgit rmoptions).