fix(files): refuse to replace a folder that holds the source

files/localfilesystem.cpp

@@ -321,6 +321,21 @@ static void validateDestination(const std::string& source, const std::string& de
         throw Failure(EINVAL, destination);
 }
 
+// A folder that holds the source cannot be replaced: merging into it would write the
+// source's copy into the source itself, and a move would then delete both.
+static void validateReplacement(const std::string& source, const std::string& destination)
+{
+    const std::string sourceParent = canonicalPath(absolutePathOf(source));
+    const std::string destinationParent = canonicalPath(absolutePathOf(destination));
+    if (sourceParent.empty() || destinationParent.empty())
+        return;
+
+    const std::string src = joined(sourceParent, fileNameOf(source));
+    const std::string dst = joined(destinationParent, fileNameOf(destination));
+    if (src.starts_with(dst + '/'))
+        throw Failure(EINVAL, destination);
+}
+
 static void mergeStaged(int source, int target, const std::string& path, OperationContext& ctx)
 {
     children(source, path, [&](const std::string& name, const std::string& childPath) {
@@ -395,6 +410,8 @@ static std::string publish(int parent, const std::string& temporary, const std::
             throw Failure(EEXIST, destination);
 
         if (policy == ConflictPolicy::Replace) {
+            validateReplacement(source, destination);
+
             // Copies are not synced one by one; the replacement must be on disk before the
             // original goes.
             check(::syncfs(parent), destination);
@@ -517,6 +534,7 @@ std::string LocalFileSystem::move(const std::string& source, const std::string&
     }
 
     if (code == EEXIST && policy == ConflictPolicy::Replace) {
+        validateReplacement(source, destination);
         if (::renameat(input.fd.value, input.name.c_str(), output.fd.value, output.name.c_str()) == 0)
             return destination;
         if (errno != EXDEV && errno != ENOTEMPTY && errno != EEXIST)

files/tests/files_test.cpp

@@ -389,6 +389,38 @@ TEST_F(FilesTest, SelfAndDescendantProtection)
     EXPECT_EQ(errorCode([&] { LocalFileSystem::remove("/", ctx); }), EINVAL);
 }
 
+TEST_F(FilesTest, ReplacingAFolderThatHoldsTheSourceIsRefused)
+{
+    TemporaryDirectory dir;
+    ASSERT_TRUE(makePath(dir.filePath("b/b/b")));
+    writeFile(dir.filePath("b/b/b/inner"), "inner");
+    writeFile(dir.filePath("b/b/file"), "file");
+    writeFile(dir.filePath("b/keep"), "keep");
+
+    OperationContext ctx;
+    EXPECT_EQ(errorCode([&] {
+                  LocalFileSystem::copy(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::Replace, ctx);
+              }),
+              EINVAL);
+
+    EXPECT_EQ(errorCode([&] {
+                  LocalFileSystem::move(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::Replace, ctx);
+              }),
+              EINVAL);
+
+    EXPECT_EQ(readFile(dir.filePath("b/b/b/inner")), "inner");
+    EXPECT_EQ(readFile(dir.filePath("b/b/file")), "file");
+    EXPECT_EQ(readFile(dir.filePath("b/keep")), "keep");
+    EXPECT_FALSE(exists(dir.filePath("b/file")));
+
+    EXPECT_EQ(LocalFileSystem::move(dir.filePath("b/b"), dir.filePath("b"), ConflictPolicy::KeepBoth, ctx),
+              dir.filePath("b (1)"));
+    EXPECT_EQ(readFile(dir.filePath("b (1)/b/inner")), "inner");
+
+    for (const std::string& name : entries(dir.path()))
+        EXPECT_FALSE(name.starts_with(".cutefish-")) << name;
+}
+
 TEST_F(FilesTest, DestinationSymlinkIsNotFollowed)
 {
     TemporaryDirectory dir;