fix: Normalisation of Windows paths during journal clean-up after recursive deletion

src/libsync/propagatorjobs.cpp

@@ -25,6 +25,7 @@
 
 #include <filesystem>
 #include <ctime>
+#include <optional>
 
 
 namespace OCC {
@@ -33,6 +34,30 @@
 Q_LOGGING_CATEGORY(lcPropagateLocalMkdir, "nextcloud.sync.propagator.localmkdir", QtInfoMsg)
 Q_LOGGING_CATEGORY(lcPropagateLocalRename, "nextcloud.sync.propagator.localrename", QtInfoMsg)
 
+namespace {
+
+std::optional<QString> journalRelativePath(const QString &syncRoot, const QString &filesystemPath)
+{
+    const auto normalizedRoot = QDir::cleanPath(QDir::fromNativeSeparators(syncRoot));
+    const auto normalizedPath = QDir::cleanPath(QDir::fromNativeSeparators(filesystemPath));
+    if (normalizedRoot.isEmpty() || normalizedPath.isEmpty()) {
+        return std::nullopt;
+    }
+
+    auto relativePath = QDir::fromNativeSeparators(QDir{normalizedRoot}.relativeFilePath(normalizedPath));
+    relativePath = QDir::cleanPath(relativePath);
+    if (relativePath == QLatin1Char('.')) {
+        relativePath.clear();
+    }
+    if (QDir::isAbsolutePath(relativePath) || relativePath == QStringLiteral("..")
+        || relativePath.startsWith(QStringLiteral("../"))) {
+        return std::nullopt;
+    }
+    return relativePath;
+}
+
+} // namespace
+
 QByteArray localFileIdFromFullId(const QByteArray &id)
 {
     return id.left(8);
@@ -50,6 +75,7 @@
 {
     QString absolute = propagator()->fullLocalPath(_item->_file + path);
     QList<QPair<QString, bool>> deleted;
+    QString conversionError;
     const auto fileInfo = QFileInfo{absolute};
     const auto parentFolderPath = fileInfo.dir().absolutePath();
     const auto parentPermissionsHandler = FileSystem::FilePermissionsRestore{parentFolderPath, FileSystem::FolderPermissions::ReadWrite};
@@ -58,10 +84,18 @@
 
     Q_EMIT propagator()->touchedFile(absolute);
 
+    // FileSystem::removeRecursively() reports deleted items as absolute filesystem paths using
+    // the host's native separators. Convert them before storing them for journal cleanup, which
+    // uses normalized paths relative to the sync folder.
     const auto success = FileSystem::removeRecursively(absolute,
-                                                       [&deleted](const QString &path, bool isDir) {
+                                                       [&deleted, &conversionError, root = propagator()->localPath()](const QString &path, bool isDir) {
                                                            // by prepending, a folder deletion may be followed by content deletions
-                                                           deleted.prepend(qMakePair(path, isDir));
+                                                           const auto relativePath = journalRelativePath(root, path);
+                                                           if (!relativePath) {
+                                                               conversionError = QStringLiteral("Filesystem path is outside the sync root");
+                                                               return;
+                                                           }
+                                                           deleted.prepend(qMakePair(*relativePath, isDir));
                                                        },
                                                        nullptr,
                                                        nullptr,
@@ -85,20 +119,22 @@
         // Do it while avoiding redundant delete calls to the journal.
         QString deletedDir;
         for (const auto &it : deleted) {
-            if (!it.first.startsWith(propagator()->localPath())) {
-                continue;
-            }
             if (isPathInsideDeletedDir(it.first, deletedDir)) {
                 continue;
             }
-            if (it.second) {
-                deletedDir = it.first;
-            }
-            if (!propagator()->_journal->deleteFileRecord(it.first.mid(propagator()->localPath().size()), it.second)) {
-                qCWarning(lcPropagateLocalRemove) << "Failed to delete file record from local DB" << it.first.mid(propagator()->localPath().size());
+            if (propagator()->_journal->deleteFileRecord(it.first, it.second)) {
+                if (it.second) {
+                    deletedDir = it.first;
+                }
+            } else {
+                qCWarning(lcPropagateLocalRemove) << "Failed to delete file record from local DB" << it.first;
             }
         }
     }
+    if (!conversionError.isEmpty()) {
+        qCWarning(lcPropagateLocalRemove) << "Failed to convert a deleted filesystem path to a journal path:" << conversionError;
+        return false;
+    }
     return success;
 }
 
@@ -596,4 +632,4 @@
 }
 }
 
-#include "moc_propagatorjobs.cpp"
+#include "moc_propagatorjobs.cpp"

test/testlockedfiles.cpp

@@ -8,7 +8,7 @@
  * any purpose.
  */
 
 #include <QtTest>
 #include <QDir>
 #include "syncenginetestutils.h"
 #include "lockwatcher.h"
@@ -338,6 +338,76 @@
         QVERIFY(fakeFolder.syncOnce());
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
+
+    void testPartialRecursiveRemoteRemovalNormalisesJournalPaths()
+    {
+        FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+        const auto journalRoot = fakeFolder.localPath();
+        const auto deletedCallbackPath = QDir::toNativeSeparators(journalRoot + QStringLiteral("A/a2"));
+        QVERIFY(!deletedCallbackPath.startsWith(journalRoot));
+
+        fakeFolder.remoteModifier().remove(QStringLiteral("A"));
+        fakeFolder.scheduleSync();
+        fakeFolder.execUntilBeforePropagation();
+
+        // Lock the file after discovery so the folder removal fails during recursive cleanup.
+        const auto lockedFile = makeHandle(fakeFolder.localPath() + QStringLiteral("A/a1"), 0);
+        QVERIFY(lockedFile != INVALID_HANDLE_VALUE);
+
+        const auto syncResult = fakeFolder.execUntilFinished();
+        CloseHandle(lockedFile);
+
+        QVERIFY(!syncResult);
+        QVERIFY(QFile::exists(fakeFolder.localPath() + QStringLiteral("A/a1")));
+        QVERIFY(!QFile::exists(fakeFolder.localPath() + QStringLiteral("A/a2")));
+
+        SyncJournalFileRecord lockedRecord;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a1"), &lockedRecord));
+        QVERIFY(lockedRecord.isValid());
+
+        SyncJournalFileRecord deletedRecord;
+        QVERIFY(fakeFolder.syncJournal().getFileRecord(QStringLiteral("A/a2"), &deletedRecord));
+        QVERIFY(!deletedRecord.isValid());
+    }
+
+    void testPartialRecursiveRemoteRemovalDoesNotDeleteRemoteFileOnNextSync()
+    {
+        FakeFolder fakeFolder{FileInfo::A12_B12_C12_S12()};
+        const auto journalRoot = fakeFolder.localPath();
+        const auto deletedCallbackPath = QDir::toNativeSeparators(journalRoot + QStringLiteral("A/a2"));
+        QVERIFY(!deletedCallbackPath.startsWith(journalRoot));
+
+        fakeFolder.remoteModifier().remove(QStringLiteral("A"));
+        fakeFolder.scheduleSync();
+        fakeFolder.execUntilBeforePropagation();
+
+        const auto lockedFile = makeHandle(fakeFolder.localPath() + QStringLiteral("A/a1"), 0);
+        QVERIFY(lockedFile != INVALID_HANDLE_VALUE);
+
+        const auto syncResult = fakeFolder.execUntilFinished();
+        CloseHandle(lockedFile);
+
+        QVERIFY(!syncResult);
+
+        // Recreate the remote folder before the next sync. A stale journal entry for a2
+        // would interpret the remote file as a local removal and delete it remotely.
+        fakeFolder.remoteModifier().mkdir(QStringLiteral("A"));
+        fakeFolder.remoteModifier().insert(QStringLiteral("A/a1"), 4);
+        fakeFolder.remoteModifier().insert(QStringLiteral("A/a2"), 4);
+
+        auto remoteA2Deleted = false;
+        fakeFolder.setServerOverride([&remoteA2Deleted](QNetworkAccessManager::Operation op, const QNetworkRequest &request, QIODevice *) {
+            if (op == QNetworkAccessManager::DeleteOperation && request.url().path().endsWith(QStringLiteral("/A/a2"))) {
+                remoteA2Deleted = true;
+            }
+            return static_cast<QNetworkReply *>(nullptr);
+        });
+
+        QVERIFY(fakeFolder.syncOnce());
+        QVERIFY(!remoteA2Deleted);
+        QVERIFY(fakeFolder.remoteModifier().find(QStringLiteral("A/a2")));
+        QVERIFY(QFile::exists(fakeFolder.localPath() + QStringLiteral("A/a2")));
+    }
 #endif
 };