Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions src/git_ops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ impl GitOperations for GitOpsManager {

// And removed from index
let _ = std::process::Command::new("git")
.args(["rm", "--cached", "-r", "--ignore-unmatch"])
.args(["rm", "--cached", "-r", "--ignore-unmatch", "--"])
.arg(&opts.path)
.current_dir(workdir)
.output();
Expand Down Expand Up @@ -525,7 +525,8 @@ impl GitOperations for GitOpsManager {
.or_else(|_| {
// CLI fallback: use git read-tree to apply sparse checkout
let output = std::process::Command::new("git")
.args(["-C", path, "read-tree", "-mu", "HEAD"])
.current_dir(path)
.args(["read-tree", "-mu", "HEAD"])
.output()
.context("Failed to run git read-tree")?;
if output.status.success() {
Expand Down
89 changes: 89 additions & 0 deletions tests/security_tests.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// SPDX-FileCopyrightText: 2025 Adam Poulemanos <[email protected]>
//
// SPDX-License-Identifier: LicenseRef-PlainMIT OR MIT

//! Security tests to ensure robustness against various attack vectors.

mod common;
use common::TestHarness;
use std::fs;
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import: use std::fs; isn't referenced anywhere in this test file. Removing it will avoid warnings and keep the test focused.

Suggested change
use std::fs;

Copilot uses AI. Check for mistakes.

#[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
let malicious_path = "-c";

// This should not fail with "unknown option" or similar error from git -C
// It might still fail for other reasons if the path is invalid for a submodule,
// but it shouldn't be interpreted as a flag to the 'git' command itself.

// Note: Using add_submodule via harness.
// We need to make sure the directory doesn't exist or is handled.

let result = harness.run_submod(&[
"add",
&remote_url,
"--name",
"hyphen-sub",
"--path",
malicious_path,
]);

// The operation might fail because "-c" is a weird path, but it shouldn't be a Command Injection.
// If it was interpreted as `git -C -c`, it would fail with "unknown option: -c" or similar.

match result {
Ok(output) => {
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(!stderr.contains("unknown option: -c"), "Potential command injection detected: git interpreted path as a flag");
},
Err(e) => {
let err_msg = e.to_string();
assert!(!err_msg.contains("unknown option: -c"), "Potential command injection detected: git interpreted path as a flag");
}
}
Comment on lines +25 to +56
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test_path_with_hyphen_injection can pass even if the submod add invocation fails for unrelated reasons (argument parsing, network, etc.), and it also doesn't ensure the failure path that runs the git rm --cached ... cleanup (where the new -- separator matters) is actually executed. Consider asserting a concrete expected outcome (e.g., command succeeds and the -c directory exists), or explicitly forcing the error/cleanup path and asserting it does not treat -c as an option.

Suggested change
// A path starting with a hyphen that could be a git flag
let malicious_path = "-c";
// This should not fail with "unknown option" or similar error from git -C
// It might still fail for other reasons if the path is invalid for a submodule,
// but it shouldn't be interpreted as a flag to the 'git' command itself.
// Note: Using add_submodule via harness.
// We need to make sure the directory doesn't exist or is handled.
let result = harness.run_submod(&[
"add",
&remote_url,
"--name",
"hyphen-sub",
"--path",
malicious_path,
]);
// The operation might fail because "-c" is a weird path, but it shouldn't be a Command Injection.
// If it was interpreted as `git -C -c`, it would fail with "unknown option: -c" or similar.
match result {
Ok(output) => {
let stderr = String::from_utf8_lossy(&output.stderr);
assert!(!stderr.contains("unknown option: -c"), "Potential command injection detected: git interpreted path as a flag");
},
Err(e) => {
let err_msg = e.to_string();
assert!(!err_msg.contains("unknown option: -c"), "Potential command injection detected: git interpreted path as a flag");
}
}
// 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"));

Copilot uses AI. Check for mistakes.
}

#[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";
Copy link

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test intends to validate behavior for a path that starts with a hyphen, but path is set to "./-sparse" (starts with .). Using "-sparse" (or an equivalent that preserves the leading - without a ./ prefix) would better exercise the injection scenario described in the PR.

Suggested change
let path = "./-sparse";
let path = "-sparse";

Copilot uses AI. Check for mistakes.

// 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.

harness.run_submod_success(&[
"add",
&remote_url,
"--name",
"sparse-hyphen",
"--path",
path,
"--sparse-paths",
"src",
]).expect("Failed to add submodule with hyphenated path");

// Verify it worked
assert!(harness.dir_exists("-sparse/src"));
}
}
Loading