fix: prevent path traversal in get_file_from_working_tree()#79
fix: prevent path traversal in get_file_from_working_tree()#79matej21 merged 1 commit intocontember:mainfrom
Conversation
Add safe_repo_path() helper that canonicalizes paths and verifies the result stays within the repository root. Applied to both get_file_from_working_tree() and create_untracked_file_diff() to prevent reading arbitrary files via crafted relative paths like "../../etc/passwd". Closes contember#68 Co-Authored-By: Claude Code
There was a problem hiding this comment.
Pull request overview
This PR addresses a critical path traversal vulnerability in the git diff/file-content utilities by ensuring user-supplied file paths cannot escape the repository root, and adds unit tests around the new path validation.
Changes:
- Introduces
safe_repo_path()to canonicalize and enforce that resolved paths remain under the repo root. - Applies
safe_repo_path()to bothget_file_from_working_tree()andcreate_untracked_file_diff()to prevent../and absolute-path escapes. - Adds unit tests for normal paths and traversal/absolute-path rejection.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fn safe_repo_path(repo_path: &Path, file_path: &str) -> Option<PathBuf> { | ||
| let full_path = repo_path.join(file_path); | ||
| let canonical = full_path.canonicalize().ok()?; | ||
| let repo_canonical = repo_path.canonicalize().ok()?; | ||
| if canonical.starts_with(&repo_canonical) { | ||
| Some(canonical) |
There was a problem hiding this comment.
safe_repo_path() canonicalizes repo_path on every call. This function is invoked in loops (e.g., for each untracked file in get_diff_with_options), so repeated filesystem canonicalization can add noticeable overhead on large repos. Consider canonicalizing the repo root once (per diff/request) and passing it in (or caching it) so only the candidate path needs canonicalization per file.
| fn test_safe_repo_path_traversal_rejected() { | ||
| let dir = tempfile::tempdir().unwrap(); | ||
| // Create a file inside the repo so the parent dirs exist | ||
| std::fs::write(dir.path().join("dummy.txt"), "").unwrap(); | ||
|
|
||
| // Attempt to escape the repo via ../ | ||
| let result = safe_repo_path(dir.path(), "../../../etc/passwd"); | ||
| assert!(result.is_none()); |
There was a problem hiding this comment.
This traversal test depends on ../../../etc/passwd existing so that canonicalize() succeeds; on platforms where that path doesn’t exist (or CI sandboxes), the function returns None due to canonicalize failure and the test passes without actually validating traversal rejection. Make the test deterministic by creating a known file outside the repo (e.g., in a sibling temp dir) and attempting to traverse to that existing file.
| // Absolute path outside repo | ||
| let result = safe_repo_path(dir.path(), "/etc/passwd"); | ||
| // On Unix, join with an absolute path replaces the base entirely, | ||
| // so this should be rejected since /etc/passwd is outside the repo. | ||
| // On systems where /etc/passwd doesn't exist, canonicalize returns None → safe. | ||
| if let Some(path) = result { | ||
| // If it somehow resolved, it must still be inside the repo | ||
| assert!(path.starts_with(dir.path().canonicalize().unwrap())); | ||
| } |
There was a problem hiding this comment.
test_safe_repo_path_absolute_outside_rejected doesn’t assert that absolute paths outside the repo are rejected; it allows Some as long as it starts with the repo. That makes the test too weak and, on non-Unix platforms, /etc/passwd often won’t exist so the test passes trivially. Prefer creating a real file outside the temp repo, pass its absolute path, and assert safe_repo_path(...) returns None.
| // Absolute path outside repo | |
| let result = safe_repo_path(dir.path(), "/etc/passwd"); | |
| // On Unix, join with an absolute path replaces the base entirely, | |
| // so this should be rejected since /etc/passwd is outside the repo. | |
| // On systems where /etc/passwd doesn't exist, canonicalize returns None → safe. | |
| if let Some(path) = result { | |
| // If it somehow resolved, it must still be inside the repo | |
| assert!(path.starts_with(dir.path().canonicalize().unwrap())); | |
| } | |
| // Create a separate temp directory to ensure the file is outside the repo | |
| let outside_dir = tempfile::tempdir().unwrap(); | |
| let outside_file = outside_dir.path().join("outside.txt"); | |
| std::fs::write(&outside_file, "outside").unwrap(); | |
| // Use the absolute path to a file that is definitely outside the repo | |
| let outside_path_str = outside_file.to_str().unwrap(); | |
| let result = safe_repo_path(dir.path(), outside_path_str); | |
| // Absolute paths outside the repo must be rejected | |
| assert!(result.is_none()); |
Summary
safe_repo_path()helper that canonicalizes joined paths and verifies they remain within the repo rootget_file_from_working_tree()andcreate_untracked_file_diff()to block../../escapesCloses #68
Test plan
cargo testpasses (includes new path traversal tests)get_file_from_working_tree()rejects paths containing../that escape the repoCo-Authored-By: Claude Code