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,18 +119,21 @@
         // 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;
 }
 

test/testsyncdelete.cpp

@@ -8,10 +8,14 @@
  * any purpose.
  */
 
 #include <QtTest>
 #include "syncenginetestutils.h"
 #include <syncengine.h>
 
+#ifdef Q_OS_WIN
+#include <windows.h>
+#endif
+
 using namespace OCC;
 
 class TestSyncDelete : public QObject
@@ -67,6 +71,37 @@
         QVERIFY(fakeFolder.currentRemoteState().find("B/b1"));
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
+
+    void partiallyFailedRecursiveRemovalCleansJournal()
+    {
+#ifndef Q_OS_WIN
+        QSKIP("Requires Windows file-sharing semantics");
+#else
+        FakeFolder fakeFolder{ FileInfo::A12_B12_C12_S12() };
+        fakeFolder.remoteModifier().remove(QStringLiteral("A"));
+        fakeFolder.scheduleSync();
+        fakeFolder.execUntilBeforePropagation();
+
+        const auto lockedFilePath = FileSystem::longWinPath(
+            QDir::toNativeSeparators(fakeFolder.localPath() + QStringLiteral("A/a1")));
+        const auto lockedFile = CreateFileW(reinterpret_cast<const wchar_t *>(lockedFilePath.utf16()),
+            GENERIC_READ, 0, nullptr, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, nullptr);
+        QVERIFY(lockedFile != INVALID_HANDLE_VALUE);
+
+        const auto syncResult = fakeFolder.execUntilFinished();
+        CloseHandle(lockedFile);
+
+        QVERIFY(!syncResult);
+
+        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());
+#endif
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncDelete)