local_working_copy: do not try to remove old file traversing symlinks

I'm not sure if this was attackable before, but it should be better to not
try to remove file across symlinks.

The disk_path is now returned from create_parent_dirs() to clarify that the
path is identical.

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<bool, CheckoutError> {
-    let parent_path = repo_path.parent().expect("repo path shouldn't be root");
+) -> Result<Option<PathBuf>, 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(_) => {

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) {