diff --git a/lib/src/local_working_copy.rs b/lib/src/local_working_copy.rs index b1aca8c5c..6341ff8b1 100644 --- a/lib/src/local_working_copy.rs +++ b/lib/src/local_working_copy.rs @@ -426,7 +426,7 @@ fn sparse_patterns_from_proto( } /// Creates intermediate directories from the `working_copy_path` to the -/// `repo_path` parent. +/// `repo_path` parent. Returns disk path for the `repo_path` file. /// /// If an intermediate directory exists and if it is a symlink, this function /// will return an error. The `working_copy_path` directory may be a symlink. @@ -437,8 +437,8 @@ fn sparse_patterns_from_proto( fn create_parent_dirs( working_copy_path: &Path, repo_path: &RepoPath, -) -> Result { - let parent_path = repo_path.parent().expect("repo path shouldn't be root"); +) -> Result, CheckoutError> { + let (parent_path, basename) = repo_path.split().expect("repo path shouldn't be root"); let mut dir_path = working_copy_path.to_owned(); for c in parent_path.components() { dir_path.push(c.to_fs_name().map_err(|err| err.with_path(repo_path))?); @@ -451,7 +451,7 @@ fn create_parent_dirs( .unwrap_or(false) => {} Err(err) => { if dir_path.is_file() { - return Ok(true); + return Ok(None); } return Err(CheckoutError::Other { message: format!( @@ -463,7 +463,14 @@ fn create_parent_dirs( } } } - Ok(false) + + let mut file_path = dir_path; + file_path.push( + basename + .to_fs_name() + .map_err(|err| err.with_path(repo_path))?, + ); + Ok(Some(file_path)) } /// Removes existing file named `disk_path` if any. @@ -1424,8 +1431,14 @@ impl TreeState { } else { stats.updated_files += 1; } - let disk_path = path.to_fs_path(&self.working_copy_path)?; + // Create parent directories no matter if after.is_present(). This + // ensures that the path never traverses symlinks. + let Some(disk_path) = create_parent_dirs(&self.working_copy_path, &path)? else { + changed_file_states.push((path, FileState::placeholder())); + stats.skipped_files += 1; + continue; + }; if present_before { remove_old_file(&disk_path)?; } else if disk_path.exists() { @@ -1433,14 +1446,6 @@ impl TreeState { stats.skipped_files += 1; continue; } - if after.is_present() { - let skip = create_parent_dirs(&self.working_copy_path, &path)?; - if skip { - changed_file_states.push((path, FileState::placeholder())); - stats.skipped_files += 1; - continue; - } - } // TODO: Check that the file has not changed before overwriting/removing it. let file_state = match after { MaterializedTreeValue::Absent | MaterializedTreeValue::AccessDenied(_) => { diff --git a/lib/tests/test_local_working_copy.rs b/lib/tests/test_local_working_copy.rs index 25a31b397..f55f598fe 100644 --- a/lib/tests/test_local_working_copy.rs +++ b/lib/tests/test_local_working_copy.rs @@ -1238,6 +1238,49 @@ fn test_existing_directory_symlink() { assert!(!workspace_root.parent().unwrap().join("escaped").exists()); } +#[test] +fn test_check_out_file_removal_over_existing_directory_symlink() { + if !check_symlink_support().unwrap() { + eprintln!("Skipping test because symlink isn't supported"); + return; + } + + let settings = testutils::user_settings(); + let mut test_workspace = TestWorkspace::init(&settings); + let repo = &test_workspace.repo; + let workspace_root = test_workspace.workspace.workspace_root().to_owned(); + + let file_path = RepoPath::from_internal_string("parent/escaped"); + let tree1 = create_tree(repo, &[(file_path, "contents")]); + let tree2 = create_tree(repo, &[]); + let commit1 = commit_with_tree(repo.store(), tree1.id()); + let commit2 = commit_with_tree(repo.store(), tree2.id()); + + // Check out "parent/escaped". + let ws = &mut test_workspace.workspace; + ws.check_out(repo.op_id().clone(), None, &commit1).unwrap(); + + // Pretend that "parent" was a symlink, which might be created by + // e.g. checking out "PARENT" on case-insensitive fs. The file + // "parent/escaped" would be skipped in that case. + std::fs::remove_file(file_path.to_fs_path_unchecked(&workspace_root)).unwrap(); + std::fs::remove_dir(workspace_root.join("parent")).unwrap(); + try_symlink("..", workspace_root.join("parent")).unwrap(); + let victim_file_path = workspace_root.parent().unwrap().join("escaped"); + std::fs::write(&victim_file_path, "").unwrap(); + assert!(file_path.to_fs_path_unchecked(&workspace_root).exists()); + + // Check out empty tree, which tries to remove "parent/escaped". + let result = ws.check_out(repo.op_id().clone(), None, &commit2); + assert_matches!( + result, + Err(CheckoutError::Other { message, .. }) if message.contains("create parent dir") + ); + + // "../escaped" shouldn't be removed. + assert!(victim_file_path.exists()); +} + #[test_case("../pwned"; "escape from root")] #[test_case("sub/../../pwned"; "escape from sub dir")] fn test_check_out_malformed_file_path(file_path_str: &str) {